Closed Bug 1372309 Opened 7 years ago Closed 7 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)

Product:
Firefox
Component:
General
Version:
53 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Iteration:
56.1 - Jun 26
Tracking Flags:
Tracking Status
firefox55 --- wontfix
firefox56 --- fixed

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)

Bug 1372309 - force height of overflow panel when testing size of resizing popups,
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1372309 - make test-ui-action-button turn off photon and make it work on hidpi devices,
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1372309 - fix browser_sync.js and browser_UITour_availableTargets.js to work with photon structure enabled,
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1372309 - turn on photon structure on nightly only,
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details 
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)
Confirmed, jetpack add-ons won't exist on 57, so disabling them is fine.
Flags: needinfo?(amckay)
Iteration: --- → 56.1 - Jun 26
Priority: -- → P1
Depends on: 1372635
Depends on: 1372837
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. :-\
New trypush with only jetpack tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcdbc32c4eba
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 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 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+
(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)
(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.
(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 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 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+
See Also: → 1373238
Attachment #8877505 - Attachment is obsolete: true
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
Let's move the hidpi stuff for the popup resize webextension test to bug 1373238.
Flags: needinfo?(kmaglione+bmo)
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
(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. :-)
(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.
Depends on: 1373703
No longer depends on: 1373703
Depends on: 1373963
Depends on: 1373961
Depends on: 1374603
Depends on: 1374608
No longer depends on: 1374608
No longer depends on: 1374603
No longer depends on: 1373961
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: