Flip photon structure pref on nightly to get automated test coverage as well as nightly visibility / user testing exposure

RESOLVED FIXED in Firefox 56

Status

()

Firefox
General
P1
normal
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

53 Branch
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 wontfix, firefox56 fixed)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

2 months ago
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

2 months ago
Confirmed, jetpack add-ons won't exist on 57, so disabling them is fine.
Flags: needinfo?(amckay)
(Assignee)

Updated

2 months ago
Iteration: --- → 56.1 - Jun 26
Priority: -- → P1
(Assignee)

Updated

2 months ago
Depends on: 1372635
Depends on: 1372837
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

2 months 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

2 months ago
New trypush with only jetpack tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcdbc32c4eba
(Assignee)

Comment 15

2 months 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

2 months 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

2 months 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

2 months 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

2 months 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

2 months 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

2 months 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

2 months 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

2 months 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+
(Assignee)

Updated

2 months ago
See Also: → bug 1373238
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8877505 - Attachment is obsolete: true

Comment 28

2 months 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

2 months ago
Let's move the hidpi stuff for the popup resize webextension test to bug 1373238.
Flags: needinfo?(kmaglione+bmo)

Comment 30

2 months 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
Last Resolved: 2 months ago
status-firefox56: affected → fixed
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

2 months 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. :-)
(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.

Updated

2 months ago
Depends on: 1373703
(Assignee)

Updated

2 months ago
No longer depends on: 1373703
No longer depends on: 1372837

Updated

2 months ago
Depends on: 1373963

Updated

2 months ago
Depends on: 1373961

Updated

2 months ago
Depends on: 1374603

Updated

2 months ago
Depends on: 1374608
(Assignee)

Updated

2 months ago
No longer depends on: 1374608
(Assignee)

Updated

2 months ago
No longer depends on: 1374603
(Assignee)

Updated

2 months ago
No longer depends on: 1373961
You need to log in before you can comment on or make changes to this bug.