Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

s/Photon/WebExtension/ for page action #3967

Merged
merged 2 commits into from May 1, 2018

Conversation

Projects
None yet
6 participants
@chenba
Copy link
Collaborator

chenba commented Jan 12, 2018

Fix #3756

@ianb

This comment has been minimized.

Copy link
Contributor

ianb commented Jan 12, 2018

We'll want to do performance testing on this again before landing

@ianb ianb changed the title s/Photon/WebExtension/ for page action [HOLD] s/Photon/WebExtension/ for page action Jan 22, 2018

@ianb

This comment has been minimized.

Copy link
Contributor

ianb commented Jan 22, 2018

We're going to hold on this bug until 61

@ianb ianb referenced this pull request Jan 22, 2018

Closed

Prototype Chrome add-on #3998

7 of 8 tasks complete
@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Feb 16, 2018

@chenba To do the performance testing Ian mentioned, you'll need to export your patch to a copy of mozilla-central, then push to Try with artifact builds disabled and 5 Talos retries. (Check out the firefox-export doc in docs/ for details.) It doesn't hurt to run all the regular unit tests, too, just to be sure nothing's amiss with your particular m-c base commit. My usual incantation is:

./mach try -b o -p linux64,macosx64,win32,win64 -u all -t all --rebuild-talos 5 --no-artifact

Once you've kicked that off, you'll get a link to view the results in treeherder, which is a rather byzantine UI. I have some notes here on how to get from a treeherder link to tables of Talos results: #2822

@chenba chenba force-pushed the chenba:3756-webextension-page-action branch 3 times, most recently from bc1cd53 to bb4d518 Feb 16, 2018

@chenba

This comment has been minimized.

@chenba chenba changed the title [HOLD] s/Photon/WebExtension/ for page action s/Photon/WebExtension/ for page action Feb 22, 2018

@chenba

This comment has been minimized.

Copy link
Collaborator Author

chenba commented Mar 12, 2018

I asked jmaher in #perf about the worst result on Talos ("perf_reftest opt e10s stylo" in linux64), and he said " I see this looks to be a regression as it trends higher". 😦

@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Mar 12, 2018

I asked jmaher in #perf about the worst result on Talos ("perf_reftest opt e10s stylo" in linux64), and he said " I see this looks to be a regression as it trends higher". 😦

Sadly, looks like this is blocked on upstream perf issues. I'd suggest filing a bugzilla bug for the addons team to prioritize this along with their other work. You can probably ask aswan in teamaddons for advice on filing the bug (which component, what info to include).

@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Mar 15, 2018

@chenba Would you mind retriggering the Talos tests? I spoke with the addons team, and they didn't see any connection between the perf_reftest regression and the code changes in this PR. Maybe it was just an outlier, and another run will show no significant regression?

@chenba

This comment has been minimized.

Copy link
Collaborator Author

chenba commented Mar 15, 2018

@6a68 Thanks for following up on this.

New results after merging latest central.

image

Nothing as extreme, but more regressions this time.

@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Mar 15, 2018

@chenba OK, I asked aswan and kmag about this. They suspect the regressions are due to Talos noise, and not any underlying code change. However, jmaher is generally the arbiter of what's a significant / meaningful Talos regression, and he's afk. I'm on PTO tomorrow. Feel free to either ping him in #perf tomorrow, or I can follow up Monday.

@chenba

This comment has been minimized.

Copy link
Collaborator Author

chenba commented Mar 15, 2018

@6a68 Thanks. Just caught up with the IRC conversations. So, wait for John-Galt, and try again later?

@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Mar 22, 2018

@chenba OK, looks like a possible performance fix has landed (https://bugzil.la/1446250), though that patch caused the page action icon to disappear in Nightly (https://bugzil.la/1447943, filed on this repo as #4252). I'm assuming we'll see a bug fix land soon, which will also touch the same performance-sensitive code path inside Firefox, so it's probably not worth re-running the Talos tests just yet. Hopefully we'll be able to land this sometime next week.

@6a68 6a68 changed the title s/Photon/WebExtension/ for page action [HOLD] s/Photon/WebExtension/ for page action Mar 22, 2018

@chenba

This comment has been minimized.

@chenba chenba referenced this pull request Mar 26, 2018

Merged

Enable context menu icon. #4264

@@ -30,6 +30,12 @@
]
}
],
"page_action": {
"browser_style": false,

This comment has been minimized.

Copy link
@ExE-Boss

ExE-Boss Apr 3, 2018

Shouldn’t this be true even if the page action doesn’t use a popup?

This comment has been minimized.

Copy link
@chenba

chenba Apr 4, 2018

Author Collaborator

MDN: "Although this key defaults to false, it's recommended that you include it and set it to true." So, kinda yes? I'll give that try.

@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Apr 4, 2018

@chenba Looks like the upstream fix landed on Friday. Mind running the Talos tests again?

@chenba chenba force-pushed the chenba:3756-webextension-page-action branch from bb4d518 to 4c51ef1 Apr 5, 2018

@chenba

This comment has been minimized.

Copy link
Collaborator Author

chenba commented Apr 5, 2018

Here are the latest Talos results: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=05e4184149693989665156187437cb3038fe66b5&framework=1&showOnlyComparable=1&showOnlyImportant=1&selectedTimeRange=172800

jmaher comments:

motionmark looks problematic, so does tp5o responsiveness

for motionmark, specifically: motionmark_animometer Leaves opt e10s stylo, motionmark_htmlsuite CSS_bouncing_clipped_rects opt e10s stylo, and motionmark_htmlsuite Leaves_2.0 opt e10s stylo
https://treeherder.mozilla.org/perf.html#/graphs?highlightedRevisions=05e418414969&series=%5B%22mozilla-central%22,%2279e2535e906a7db6ca5105112746295fbe93b5e3%22,1,%221%22%5D&series=%5B%22try%22,%2279e2535e906a7db6ca5105112746295fbe93b5e3%22,1,%221%22%5D&timerange=172800 seems to be bi-modal, possibly you have triggered a case where it is now a single mode- maybe things are not that bad- but worth some deeper investigation

@@ -30,8 +30,11 @@
]
}
],
"icons": {
"32": "icons/icon-v2.svg"

This comment has been minimized.

Copy link
@ExE-Boss

ExE-Boss Apr 5, 2018

The icons key shouldn’t be removed, as it’s necessary for the icon to show up in about:addons and a few other places.

This comment has been minimized.

Copy link
@chenba

chenba Apr 5, 2018

Author Collaborator

The docs say this can be an object or string. And changing it doesn't make a difference for about:addons. @ExE-Boss Did you see a difference?

This comment has been minimized.

Copy link
@ExE-Boss

ExE-Boss Apr 5, 2018

That’s because this extension is currently a bootstrapped extension with an embedded WebExtension, so the icon of the bootstrapped extension is used as a fallback, but this should still be kept so that once this extension becomes a full WebExtension (see bug 1422437), we don’t forget to add it back.

@kmaglione

This comment has been minimized.

Copy link

kmaglione commented Apr 5, 2018

Looking at 1, I'm not really concerned by any of the results. The Linux responsiveness regression may be real, since we still don't have OOP extensions on Linux, but it's pretty slight either way. The motionmark regression is almost certainly not real, since the changed code should only have any effect during navigation and tab switch, which shouldn't happen during those tests.

@chenba chenba force-pushed the chenba:3756-webextension-page-action branch from 4c51ef1 to d473861 Apr 6, 2018

@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Apr 17, 2018

@chenba Have you talked to jmaher in #perf about kmag's comments? I think we should be ok to land this, if he agrees the regressions are acceptable.

@ExE-Boss

This comment has been minimized.

Copy link

ExE-Boss commented Apr 17, 2018

Also, let’s not forget to re-add the icons manifest key.

@chenba

This comment has been minimized.

Copy link
Collaborator Author

chenba commented Apr 18, 2018

@chenba

This comment has been minimized.

Copy link
Collaborator Author

chenba commented Apr 18, 2018

@6a68 Thanks for the reminder. I just ping'd jmaher in #perf. We'll see he'll say. 🤞

@ExE-Boss

This comment has been minimized.

Copy link

ExE-Boss commented Apr 18, 2018

@chenba That’s the page_action key, not the icons key. The icons key is still being removed by that commit.

Lines 28–41 of manifest.json.template in this PR:

"selector/callBackground.js",
"sitehelper.js"
]
}
],
"page_action": {
"browser_style": true,
"default_icon" : {
"32": "icons/icon-v2.svg"
},
"default_title": "__MSG_contextMenuLabel__",
"show_matches": ["<all_urls>"]
},
"web_accessible_resources": [

Lines 28–36 of manifest.json.template in master:

"selector/callBackground.js",
"sitehelper.js"
]
}
],
"icons": {
"32": "icons/icon-v2.svg"
},
"web_accessible_resources": [

@chenba

This comment has been minimized.

Copy link
Collaborator Author

chenba commented Apr 18, 2018

@ExE-Boss d'oh, I mis-read/misunderstood your comments regarding the icons from the beginning. Sorry for being dense. I'll restore that part of the manifest.

@chenba chenba force-pushed the chenba:3756-webextension-page-action branch from d473861 to 4908da5 Apr 18, 2018

@jmaher

This comment has been minimized.

Copy link

jmaher commented Apr 19, 2018

regarding the regressions, I see tp5o and tp5o_responsiveness as have regressions. tp5o is only a regression on linux and an improvement on osx/windows. responsiveness is the largest item of concern- if there is anything we can do to improve that prior to landing it would be a good thing.

@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Apr 25, 2018

Hmm, some conflicting discussion here. I asked kmag about this in #teamaddons:

_6a68: I'm not worried about a linux-only responsiveness regression there. That's more or less expected until we turn on OOP extensions.
4:48 PM A small regression anyway.
4:48 PM If it's next to an improvement on Windows/OS-X, I'm even less worried.

I think we are cleared to land this patch, finally! @chenba is this ready for a final review?

@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Apr 26, 2018

hmm. I don't remember why this comment was added:

We're going to hold on this bug until 61

@ianb Were we waiting till 61 to avoid changing the page action menu in ESR 60?

@chenba

This comment has been minimized.

Copy link
Collaborator Author

chenba commented Apr 26, 2018

@6a68 Yes, it is.

@chenba chenba changed the title [HOLD] s/Photon/WebExtension/ for page action s/Photon/WebExtension/ for page action Apr 26, 2018

@6a68

6a68 approved these changes May 1, 2018

Copy link
Member

6a68 left a comment

LGTM. Nice work!

@6a68 6a68 merged commit 02b4c6e into mozilla-services:master May 1, 2018

1 check was pending

ci/circleci: build CircleCI is running your tests
Details
@chenba

This comment has been minimized.

Copy link
Collaborator Author

chenba commented Jun 4, 2018

Bug 1466575 is tracking the uplift of this patch into Firefox.

testeaxeax added a commit to testeaxeax/screenshots that referenced this pull request Jun 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.