Setup build configuration for Firefox Screenshots in Firefox

RESOLVED FIXED in Firefox 54

Status

defect
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
mozilla55
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

()

Attachments

(5 attachments, 10 obsolete attachments)

1.63 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
14.14 KB, patch
Details | Diff | Splinter Review
524.91 KB, patch
Details | Diff | Splinter Review
1.01 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
kmag
: review+
Details
Assignee

Description

2 years ago
Pageshot is preparing to be part of Firefox as a system add-on.

This bug is for the patches that will hook it up to part of the build system.

We aren't ready to land this yet, we're using this as a placeholder for the time being to store the necessary patches.
Assignee

Comment 1

2 years ago
Posted patch Build system patch (obsolete) — Splinter Review
Assignee

Updated

2 years ago
Depends on: 1347482
Summary: Setup build configuration for PageShot in Firefox → Setup build configuration for Firefox Screenshots in Firefox
Assignee

Comment 2

2 years ago
Posted patch Build system patch v2 (obsolete) — Splinter Review
Updated patch for pageshot -> screenshots. Also added prefs from https://github.com/mozilla-services/screenshots/issues/2304
Attachment #8846680 - Attachment is obsolete: true
Comment on attachment 8851763 [details] [diff] [review]
Build system patch v2

Review of attachment 8851763 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like there's still a couple references to "pageshot" in here
Assignee

Comment 4

2 years ago
Posted patch Build system patch v3 (obsolete) — Splinter Review
Good catch, here's the fixed version.
Attachment #8851763 - Attachment is obsolete: true
Assignee

Comment 5

2 years ago
Posted patch Build system patch v4 (obsolete) — Splinter Review
Updated patch to include test configuration for https://github.com/mozilla-services/screenshots/pull/2515
Attachment #8851958 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Depends on: 1352387
Assignee

Comment 6

2 years ago
Screenshots is a system add-on imported from https://github.com/mozilla-services/screenshots/.

This is the initial build system patch for building screenshots. ESLint is ignored since Screenshots currently use their own version (this may change in the future).

MozReview-Commit-ID: 4OEcaduaeWE
Assignee

Comment 8

2 years ago
MozReview-Commit-ID: FotxWZ7hf1X
Assignee

Updated

2 years ago
Attachment #8852200 - Attachment is obsolete: true
Assignee

Comment 9

2 years ago
Comment on attachment 8853388 [details] [diff] [review]
Setup build configuration for Firefox Screenshots in Firefox. r?gps

This is the build system changes we'll need for Screenshots.

We're expecting to land it next week, hence getting it up for review now.
Attachment #8853388 - Flags: review?(gps)
Assignee

Comment 10

2 years ago
Comment on attachment 8853389 [details] [diff] [review]
Add Screenshot's disabled preferences to telemetry reporting. r?bsmedberg

As per https://github.com/mozilla-services/screenshots/issues/2304 - Screenshots would like to report via telemetry extra information on if it is enabled or not. Adding these preferences to this list appears to be the easiest way to do that.
Attachment #8853389 - Flags: review?(benjamin)
Comment on attachment 8853388 [details] [diff] [review]
Setup build configuration for Firefox Screenshots in Firefox. r?gps

Review of attachment 8853388 [details] [diff] [review]:
-----------------------------------------------------------------

You don't need a build peer to review simple changes to moz.build files like these. But I'll happily rubber stamp this for you.
Attachment #8853388 - Flags: review?(gps) → review+

Updated

2 years ago
Attachment #8853389 - Flags: review?(benjamin) → review+
Assignee

Comment 12

2 years ago
Updated patch, I had to list all the webextension files separately, to make windows builds happy (this is scripted in the screenshots export scripts).

For now, I've also listed the L10n files as dupe-files-allowed, but I'm going to make those go away in the next iteration of the export scripts for screenshots.
Assignee

Updated

2 years ago
Attachment #8853388 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Blocks: 1355569
Assignee

Comment 13

2 years ago
Revised build patch, with disabling screenshots by default as per discussions & disabling it within tests explicitly until we fix those issues.
Attachment #8855871 - Attachment is obsolete: true
Attachment #8857211 - Flags: review?(dtownsend)
Comment on attachment 8857211 [details] [diff] [review]
Setup build configuration for Firefox Screenshots in Firefox. v3

Review of attachment 8857211 [details] [diff] [review]:
-----------------------------------------------------------------

::: .gitignore
@@ +7,4 @@
>  TAGS
>  tags
>  ID
> +!/browser/extensions/screenshots/webextension/_locales/id/

Why do we have to git ignore this but not hg ignore it?

::: browser/extensions/screenshots/moz.build
@@ +4,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DEFINES['MOZ_APP_VERSION'] = CONFIG['MOZ_APP_VERSION']
> +DEFINES['MOZ_APP_MAXVERSION'] = CONFIG['MOZ_APP_MAXVERSION']

These don't seem to be used
Attachment #8857211 - Flags: review?(dtownsend) → review+
Assignee

Comment 15

2 years ago
(In reply to Dave Townsend [:mossop] from comment #14)
> ::: .gitignore
> @@ +7,4 @@
> >  TAGS
> >  tags
> >  ID
> > +!/browser/extensions/screenshots/webextension/_locales/id/
> 
> Why do we have to git ignore this but not hg ignore it?

On Mac, 'ID' matches 'id' due to the case sensitivity wonders. The hg ignore allows things like ^ and $ and so has a different definition and gets away with it.

> 
> ::: browser/extensions/screenshots/moz.build
> @@ +4,5 @@
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +DEFINES['MOZ_APP_VERSION'] = CONFIG['MOZ_APP_VERSION']
> > +DEFINES['MOZ_APP_MAXVERSION'] = CONFIG['MOZ_APP_MAXVERSION']
> 
> These don't seem to be used

I'll drop those.
Assignee

Comment 16

2 years ago
Posted patch Export Screenshots (obsolete) — Splinter Review
Just looking for an rs here. This is an export from the latest version of the repo. I'll probably re-export it again tomorrow.

Try push of the patch set here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a39b5364658dc0b78f43133cc7b0896411146b46
Attachment #8857220 - Flags: review?(dtownsend)
Assignee

Updated

2 years ago
Attachment #8853390 - Attachment is obsolete: true
Comment on attachment 8857220 [details] [diff] [review]
Export Screenshots

Review of attachment 8857220 [details] [diff] [review]:
-----------------------------------------------------------------

No real need to rs here but here it is anyway
Attachment #8857220 - Flags: review?(dtownsend) → review+
Assignee

Updated

2 years ago
Blocks: 1355998
Assignee

Comment 18

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/81b1a2207776041d4bd044c146858f7b706b045c
Bug 1346825 - Add Screenshot's disabled preferences to telemetry reporting. r=bsmedberg

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4b155c9ee3db31f5494e1d15bac0984932ed7a2
Bug 1346825 - Setup build configuration for Firefox Screenshots in Firefox (and disable screenshots by default). r=gps,Mossop

https://hg.mozilla.org/integration/mozilla-inbound/rev/426aa68b32f0c6405bc297498d6f80996f4e013a
Bug 1346825 - Import Screenshots version 6.3.0 into mozilla-central. rs=Mossop.

https://hg.mozilla.org/integration/mozilla-inbound/rev/80999e98d40b20db3ffa455e69596ffac2e6af3f
Bug 1346825 - Disable browser_inlinesettings_browser.js on Windows due to highly frequent intermittent failures when legacy based system add-ons are present. rs=rhelmer.
Assignee

Comment 19

2 years ago
Just to help with future tracking, adding the final patches that were pushed. This one was updated for review comments.
Attachment #8857211 - Attachment is obsolete: true
Assignee

Comment 20

2 years ago
Correct file this time.
Attachment #8857866 - Attachment is obsolete: true
Assignee

Comment 21

2 years ago
We ended up exporting version 6.3.0 of Screenshots.
Attachment #8857220 - Attachment is obsolete: true
Assignee

Comment 22

2 years ago
This disables browser_inlinesettings_browser.js on Windows due to highly frequent intermittent failures. Bug 1355998 is for fixing those failures.

We agreed this with rhelmer on irc, hence rs=rhelmer.
Assignee

Updated

2 years ago
Blocks: 1356243
Assignee

Updated

2 years ago
Depends on: 1356394
Assignee

Comment 24

2 years ago
I filed bug 1356394 on the failure, tracking it there.
Flags: needinfo?(standard8)
as a note when this landed we saw AWSY (memory) regressions:
== Change summary for alert #6011 (as of April 13 2017 09:30 UTC) ==

Regressions:

  5%  JS summary windows7-32-vm opt      135930388.43 -> 142447253.3
  5%  JS summary windows10-64-vm opt     194972652.65 -> 204197221.71
  4%  Explicit Memory summary windows10-64-vm opt 167380200.4 -> 173798738.33
  3%  Explicit Memory summary windows7-32-vm opt 126129645.9 -> 130526468.43
  3%  Resident Memory summary windows10-64-vm opt 675096556.02 -> 692951986.82

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6011


and when this was backed out we saw the corresponding improvements:
== Change summary for alert #6018 (as of April 13 2017 18:23 UTC) ==

Improvements:

  4%  JS summary windows7-32-vm opt      142178387.12 -> 135935795.13
  4%  JS summary windows10-64-vm opt     203936231.57 -> 195417494.88
  3%  Explicit Memory summary windows10-64-vm opt 173921203.66 -> 168035907.23
  3%  Explicit Memory summary windows7-32-vm opt 130388188.65 -> 126031225.01
  2%  Resident Memory summary windows10-64-vm opt 692403058.48 -> 676974632.95

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6018
(In reply to Joel Maher ( :jmaher) from comment #25)
> as a note when this landed we saw AWSY (memory) regressions:

I suspect that these will go away once the change from bug 1356394 is applied.

Comment 27

2 years ago
Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8af371d73f7d
Add Screenshot's disabled preferences to telemetry reporting. r=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe7fd9dff84d
Setup build configuration for Firefox Screenshots in Firefox (and disable screenshots by default). r=gps,Mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/b925de7ffa23
Import Screenshots version 6.3.0 into mozilla-central. rs=Mossop.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d6a3f088cfc
Disable browser_inlinesettings_browser.js on Windows due to highly frequent intermittent failures when legacy based system add-ons are present. rs=rhelmer.
Comment on attachment 8853389 [details] [diff] [review]
Add Screenshot's disabled preferences to telemetry reporting. r?bsmedberg

Approval Request Comment
[Feature/Bug causing the regression]:
No regression. This lands Firefox Screenshots (https://wiki.mozilla.org/Firefox/Screenshots) in Beta 54.

[User impact if declined]:
Screenshots has been identified as a top line feature for the 54 launch (based on high user engagement through Test Pilot), not landing will affect org-wide goals.

[Is this code covered by automated tests?]:
Some, yes.

[Has the fix been verified in Nightly?]:
Yes, https://github.com/mozilla-services/screenshots/issues/2681#issuecomment-295300379

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
4 patches in bug 1346825, and 1 in bug 1352387.

[Is the change risky?]:
Mildly.

[Why is the change risky/not risky?]:
Risky because it's a product feature that didn't bake in Nightly/Aurora, not that risky because we are landing it pref'd off, and doing a staged rollout via a system add-on (bug 1351424).


[String changes made/needed]:
None.
Attachment #8853389 - Flags: approval-mozilla-beta?
Comment on attachment 8857867 [details] [diff] [review]
Setup build configuration for Firefox Screenshots in Firefox. v5

Approval Request Comment

See comment 29.
Attachment #8857867 - Flags: approval-mozilla-beta?
Comment on attachment 8857870 [details] [diff] [review]
Export Screenshots v6.3.0

Approval Request Comment

See comment 29.
Attachment #8857870 - Flags: approval-mozilla-beta?
Comment on attachment 8857872 [details] [diff] [review]
Disable browser_inlinesettings_browser.js on Windows due to intermittent failures

Approval Request Comment

See comment 29.
Attachment #8857872 - Flags: approval-mozilla-beta?
Comment on attachment 8853389 [details] [diff] [review]
Add Screenshot's disabled preferences to telemetry reporting. r?bsmedberg

Screenshot is an important feature in 54. Beta54+. Should be in 54 beta 2.
Attachment #8853389 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8857867 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8857870 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8857872 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Wes Kocher (:KWierso) from comment #35)
> This appears to have caused talos failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=93309644&repo=mozilla-
> beta
> 
> Backed out in
> https://hg.mozilla.org/releases/mozilla-beta/rev/
> 521748d0ac619170e8b6ecc11f349401e54ca414

I believe the problem is that this needs to be whitelisted here as it causes a new file open on startup:

https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/xtalos/xperf_whitelist.json?q=xperf_whitelist.json&redirect_type=direct#19

I'm not sure why we didn't see this on mozilla-central, is this test only run for beta? If not we should ensure the tests isn't broken on m-c.
Flags: needinfo?(standard8) → needinfo?(wkocher)
Comment hidden (mozreview-request)

Comment 38

2 years ago
mozreview-review
Comment on attachment 8860582 [details]
Bug 1346825 - add Screenshots to the xperf whitelist

https://reviewboard.mozilla.org/r/132598/#review135456
Attachment #8860582 - Flags: review?(kmaglione+bmo) → review+

Comment 39

2 years ago
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fddb0c922370
add Screenshots to the xperf whitelist r=kmag
Comment on attachment 8860582 [details]
Bug 1346825 - add Screenshots to the xperf whitelist

This should fix the failing test on the mozilla-beta merge that caused the backout.
Attachment #8860582 - Flags: approval-mozilla-beta?
Comment on attachment 8860582 [details]
Bug 1346825 - add Screenshots to the xperf whitelist

Per comment #33, Beta54+.
Attachment #8860582 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Iris Hsiao [:ihsiao] from comment #42)
> https://hg.mozilla.org/mozilla-central/rev/fddb0c922370

shouldn't we have uplifted all patches here instead of just one ? see comment #35 where Wes backed out all changes
Flags: needinfo?(ihsiao)
(In reply to Carsten Book [:Tomcat] from comment #43)
> (In reply to Iris Hsiao [:ihsiao] from comment #42)
> > https://hg.mozilla.org/mozilla-central/rev/fddb0c922370
> 
> shouldn't we have uplifted all patches here instead of just one ? see
> comment #35 where Wes backed out all changes

ah never mind :) my bad
Flags: needinfo?(ihsiao)
(In reply to Robert Helmer [:rhelmer] from comment #36)
> (In reply to Wes Kocher (:KWierso) from comment #35)
> > This appears to have caused talos failures like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=93309644&repo=mozilla-
> > beta
> > 
> > Backed out in
> > https://hg.mozilla.org/releases/mozilla-beta/rev/
> > 521748d0ac619170e8b6ecc11f349401e54ca414
> 
> I believe the problem is that this needs to be whitelisted here as it causes
> a new file open on startup:
> 
> https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/xtalos/
> xperf_whitelist.json?q=xperf_whitelist.json&redirect_type=direct#19
> 
> I'm not sure why we didn't see this on mozilla-central, is this test only
> run for beta? If not we should ensure the tests isn't broken on m-c.

Unsure why it didn't fail things on trunk. Maybe jmaher knows?
Flags: needinfo?(wkocher) → needinfo?(jmaher)
that test runs on trunk and beta, I wonder if the whitelist didn't get updated on beta?  we have it properly for the trunk code.
Flags: needinfo?(jmaher)
kmag thought that perhaps IO improvements we've been landing in Nightly might be giving us more of a buffer before we fail xperf now (AFAICT that test tracks how many reads are done total, not necessarily how which/how many files are opened although that's part of the diagnostic output - but maybe someone can correct if this is wrong).
The test also tracks any IO operations on any files we aren't expecting to be opened. The only other possibility I can think of is that when we don't start the WebExtension portion of the extension, only the bootstrap.js file actually needs to be loaded, and that can be read from the startup cache without opening any new files.

Updated

2 years ago
See Also: → 1419836
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 55 → mozilla55
You need to log in before you can comment on or make changes to this bug.