Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Enable context menu icon. #4264

Merged
merged 1 commit into from
Mar 28, 2018

Conversation

chenba
Copy link
Collaborator

@chenba chenba commented Mar 23, 2018

Fixes #3858

Copy link
Member

@jaredhirsch jaredhirsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chenba This change looks good, but I suspect adding the icon to the manifest may impact Talos performance. Would you be up for testing this patch on the Try server?

If you'd like to learn to do the export, this is a good opportunity; you wouldn't need to mess with changelog generation or tagging, but just run the export script (EXPORT_MC_LOCATION=path/to/gecko ./bin/export_mc.py --no-commit --no-switch-branch). If you'd like to skip that step, this is a very small patch, so it'd be fine to directly edit the files inside mozilla-central, then commit inside m-c, and push to Try.

Whichever approach you opt for, I'd suggest the incantation from the export doc: ./mach try -b o -p linux64,macosx64,win32,win64 -u all -t all --rebuild-talos 5 --no-artifact.

@jaredhirsch
Copy link
Member

I forgot to mention that the export documentation is inside /docs.

@chenba
Copy link
Collaborator Author

chenba commented Mar 26, 2018

Yep, no prob. Already used the export script for #3967

What's the alternative if there's a performance regression? Try different image formats?

@jaredhirsch
Copy link
Member

What's the alternative if there's a performance regression? Try different image formats?

Probably best to just ask the addons team if we see a regression; they might fix it themselves, or offer us a workaround. In the past, regressions have tended to be code that runs on each page load, like tabs.onUpdated, and not code that seems like it would only run once, like creating a context menu item with an icon - but you never know :-P

@chenba
Copy link
Collaborator Author

chenba commented Mar 27, 2018

Talos results.

@jaredhirsch
Copy link
Member

@chenba Great! Would you mind pinging jmaher in #perf to see if he thinks the results are ok?

@chenba
Copy link
Collaborator Author

chenba commented Mar 28, 2018

jmaher said: "I think it is ok- the good news it is gave us data to look at- the orange/red items are that color because it looks like a possible or likely regression; in these cases by looking at the graphs and zooming in, you can see that most data points are bi-modal and on the try push it falls heavier on a single mode- therefore making the numbers different"

😌

@chenba chenba dismissed jaredhirsch’s stale review March 28, 2018 20:24

We have Talos results and jmaher's input on them.

Copy link
Member

@jaredhirsch jaredhirsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following up with the Talos tests, @chenba. We'll have to keep this patch in mind if we see perf regressions in Nightly after our next export. Merging.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants