Closed
Bug 1372309
Opened 6 years ago
Closed 6 years ago
Flip photon structure pref on nightly to get automated test coverage as well as nightly visibility / user testing exposure
Categories
(Firefox :: General, enhancement, P1)
Tracking
()
People
(Reporter: Gijs, Assigned: Gijs)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: [photon-structure])
Attachments
(4 files, 1 obsolete file)
We should flip the pref on nightly so we get more testing. To do: - Test failures (see https://treeherder.mozilla.org/#/jobs?repo=try&revision=4414fdff7cbac8170706e341a4861c1f30d4153e ) -- jetpack tests need to have the pref disabled. We don't care about fixing them as we'll get rid of jetpack support for 57, I think? (Andy, can you confirm?) -- I forgot to update this line: https://dxr.mozilla.org/mozilla-central/rev/981da978f1f686ad024fa958c9d27d2f8acc5ad0/browser/components/uitour/test/browser_UITour_availableTargets.js#8-9 as described, because the patch that landed that ended up landing after the other bug. This will fix browser/components/uitour/test/browser_UITour_availableTargets.js -- browser/base/content/test/sync/browser_sync.js - not entirely sure, I expect it's just looking at the wrong label. -- browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js - as noted in bug 1354109 comment 20, this is a result of the difference in height between the overflow panel and the main menu panel. I intend to disable photon during the test (as we do in a number of CUI tests and will be doing for jetpack) and file a separate webextension bug on the issue, which also affects pre-photon code (just not in situations the test is exercising). - Talos pushes. I don't expect any surprises, but then, they would be surprises, so we best make sure... - Ensure cedar flips the pref back to false so we continue testing non-photon there in all tests that don't explicitly flip it on/off. CC mconley for this.
Flags: qe-verify-
Flags: needinfo?(amckay)
|
||
Comment 1•6 years ago
|
||
Confirmed, jetpack add-ons won't exist on 57, so disabling them is fine.
Flags: needinfo?(amckay)
|
Assignee | |
Updated•6 years ago
|
Iteration: --- → 56.1 - Jun 26
Priority: -- → P1
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 13•6 years ago
|
||
Series of updates for jetpack here because it turns out that my last-minute tidy-ups to the patch still turned jetpack orange: https://treeherder.mozilla.org/#/jobs?repo=try&revision=efd7620b7103&selectedJob=106955642 This is because apparently the jetpack tests don't support setup/teardown methods, and my attempts to set the pref in the first test and reset it in the last backfired because the tests run alphabetically rather than in order, and the last test in the file runs before the test which uses AREA_PANEL. Updated to set + reset the pref in the relevant subtests themselves. :-\
|
|
Assignee | |
Comment 14•6 years ago
|
||
New trypush with only jetpack tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcdbc32c4eba
|
|
Assignee | |
Comment 15•6 years ago
|
||
Trypushes look OK, talos checks (still needs to run...): https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=daee936cbf57ae9ae6f6fa4031884d64a92cb1a8&newProject=try&newRevision=474560ecb65cc1de3b68909deec9e3f9e12e56b1&framework=1&showOnlyImportant=0
|
||
Comment 16•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8877505 [details] Bug 1372309 - make popup resize test not care about rounding errors so it works on local hidpi machines, https://reviewboard.mozilla.org/r/148960/#review153526 ::: browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:214 (Diff revision 1) > > ok(getHeight() > height, `Browser height should increase (${getHeight()} > ${height})`); > > is(win.innerWidth, innerWidth, "Window width should not change"); > ok(win.innerHeight >= innerHeight, `Window height should increase (${win.innerHeight} >= ${innerHeight})`); > - is(win.scrollMaxY, 0, "Document should not be vertically scrollable"); > + Assert.lessOrEqual(win.scrollMaxY, 1, "Document should not be vertically scrollable"); nit: should this (and the others below) be `Assert.less()`? If scrollMaxY is exactly 1, it will be scrollable yes?
Attachment #8877505 -
Flags: review?(aswan) → review+
|
|
||
Comment 17•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8877506 [details] Bug 1372309 - force height of overflow panel when testing size of resizing popups, https://reviewboard.mozilla.org/r/148962/#review153528 ::: browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:67 (Diff revision 1) > await extension.unload(); > }); > > async function testPopupSize(standardsMode, browserWin = window, arrowSide = "top") { > let docType = standardsMode ? "<!DOCTYPE html>" : ""; > + browserWin.document.getElementById("widget-overflow-mainView").style.minHeight = "600px"; Is this going to break on non-photon-enabled builds? (if those are in fact a thing we need to be concerned about?)
Attachment #8877506 -
Flags: review?(aswan) → review+
|
|
||
Comment 18•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8877507 [details] Bug 1372309 - make test-ui-action-button turn off photon and make it work on hidpi devices, https://reviewboard.mozilla.org/r/148964/#review153530 Ugh, thanks for slogging through addon-sdk so somebody else didn't have to... ::: addon-sdk/source/test/test-ui-action-button.js:843 (Diff revision 3) > }); > > let { node, id: widgetId } = getWidget(button.id); > let { devicePixelRatio } = node.ownerGlobal; > > - let size = 16 * devicePixelRatio; > + let size = [5, 16, 32, 64].find(x => (16 * devicePixelRatio) <= x); Ha, I love the symmetry of `x => foo <= x`
Attachment #8877507 -
Flags: review?(aswan) → review+
|
|
Assignee | |
Comment 19•6 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #16) > > - is(win.scrollMaxY, 0, "Document should not be vertically scrollable"); > > + Assert.lessOrEqual(win.scrollMaxY, 1, "Document should not be vertically scrollable"); > > nit: should this (and the others below) be `Assert.less()`? If scrollMaxY > is exactly 1, it will be scrollable yes? That would be better, but AFAICT scrolMaxY is rounded to ints. And the item is sometimes scrollable, IIRC, but I guess that's a rounding error somewhere, if it's literally exactly 1px? We can file a followup for this if you prefer, it doesn't happen in automation but it happens for me both pre- and post-photon when running locally on a hidpi machine... Let me know what you want me to do...
Flags: needinfo?(aswan)
|
|
Assignee | |
Comment 20•6 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #17) > > + browserWin.document.getElementById("widget-overflow-mainView").style.minHeight = "600px"; > > Is this going to break on non-photon-enabled builds? (if those are in fact > a thing we need to be concerned about?) Good catch, yes, it would (on non-MOZ_PHOTON_THEME builds, irrespective of the pref) because I started ifdef'ing that code (it wasn't before) in bug 1370986. I'll update it to check whether the element is non-null.
|
|
||
Comment 21•6 years ago
|
||
(In reply to :Gijs from comment #19) > (In reply to Andrew Swan [:aswan] from comment #16) > > > - is(win.scrollMaxY, 0, "Document should not be vertically scrollable"); > > > + Assert.lessOrEqual(win.scrollMaxY, 1, "Document should not be vertically scrollable"); > > > > nit: should this (and the others below) be `Assert.less()`? If scrollMaxY > > is exactly 1, it will be scrollable yes? > > That would be better, but AFAICT scrolMaxY is rounded to ints. And the item > is sometimes scrollable, IIRC, but I guess that's a rounding error > somewhere, if it's literally exactly 1px? We can file a followup for this if > you prefer, it doesn't happen in automation but it happens for me both pre- > and post-photon when running locally on a hidpi machine... Let me know what > you want me to do... I'm happy to keep it as-is but I haven't worked much with this test, I'm going to redirect to Kris who has...
Flags: needinfo?(aswan) → needinfo?(kmaglione+bmo)
|
||
Comment 22•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8877508 [details] Bug 1372309 - fix browser_sync.js and browser_UITour_availableTargets.js to work with photon structure enabled, https://reviewboard.mozilla.org/r/148966/#review153868 Exactly right.
Attachment #8877508 -
Flags: review?(mdeboer) → review+
|
|
||
Comment 23•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8877509 [details] Bug 1372309 - turn on photon structure on nightly only, https://reviewboard.mozilla.org/r/148968/#review153870 Ship it!
Attachment #8877509 -
Flags: review?(mdeboer) → review+
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•6 years ago
|
Attachment #8877505 -
Attachment is obsolete: true
|
||
Comment 28•6 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/91ebb584fc06 force height of overflow panel when testing size of resizing popups, r=aswan https://hg.mozilla.org/integration/autoland/rev/809a0de511f9 make test-ui-action-button turn off photon and make it work on hidpi devices, r=aswan https://hg.mozilla.org/integration/autoland/rev/ce3716d606bf fix browser_sync.js and browser_UITour_availableTargets.js to work with photon structure enabled, r=mikedeboer https://hg.mozilla.org/integration/autoland/rev/0e85dde9ec9b turn on photon structure on nightly only, r=mikedeboer
|
|
Assignee | |
Comment 29•6 years ago
|
||
Let's move the hidpi stuff for the popup resize webextension test to bug 1373238.
Flags: needinfo?(kmaglione+bmo)
|
||
Comment 30•6 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/91ebb584fc06 https://hg.mozilla.org/mozilla-central/rev/809a0de511f9 https://hg.mozilla.org/mozilla-central/rev/ce3716d606bf https://hg.mozilla.org/mozilla-central/rev/0e85dde9ec9b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
I see performance improvements: == Change summary for alert #7352 (as of June 15 2017 14:04 UTC) == Improvements: 21% cart summary windows7-32 pgo e10s 24.82 -> 19.54 17% cart summary windows7-32 opt e10s 33.50 -> 27.81 15% cart summary osx-10-10 opt e10s 33.67 -> 28.70 15% cart summary windows10-64 opt e10s 27.96 -> 23.84 9% cart summary linux64 pgo e10s 21.09 -> 19.09 9% cart summary linux64 opt e10s 24.70 -> 22.56 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7352
|
|
Assignee | |
Comment 32•6 years ago
|
||
(In reply to Joel Maher ( :jmaher) from comment #31) > I see performance improvements: > == Change summary for alert #7352 (as of June 15 2017 14:04 UTC) == > > Improvements: > > 21% cart summary windows7-32 pgo e10s 24.82 -> 19.54 > 17% cart summary windows7-32 opt e10s 33.50 -> 27.81 > 15% cart summary osx-10-10 opt e10s 33.67 -> 28.70 > 15% cart summary windows10-64 opt e10s 27.96 -> 23.84 > 9% cart summary linux64 pgo e10s 21.09 -> 19.09 > 9% cart summary linux64 opt e10s 24.70 -> 22.56 > > For up to date results, see: > https://treeherder.mozilla.org/perf.html#/alerts?id=7352 Yes, these were on the trypush as well. It's pretty neat, though I'm not sure we'll be able to hang on to these when we finish the styling / detailing on customize mode (which is what this relates to) - see bug 1354123 and bug 1354145. Good to have a record though, because I think the standard for what we end up shipping will need to be "no worse than pre-photon" (though obviously it would be nice if we can hang on to all of these gains). Also good to know because of course, because this is behind a pref, this will seem to "regress" (compared to nightly) when we merge this to beta (where the pref gets flipped again) - though it'll be no worse than 55 is on beta. :-)
|
||
Comment 33•6 years ago
|
||
(In reply to Joel Maher ( :jmaher) from comment #31) > I see performance improvements: Great! These are almost certainly legitimate. The pref flip changes which panel appears in customize mode. We skip painting all of the icons in the hamburger menu.
You need to log in
before you can comment on or make changes to this bug.
Description
•