Closed Bug 1346825 Opened 8 years ago Closed 8 years ago

Setup build configuration for Firefox Screenshots in Firefox

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox54 fixed, firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

(
URL
)

Details

Attachments

(5 files, 10 obsolete files)

Add Screenshot's disabled preferences to telemetry reporting. r?bsmedberg
1.63 KB, patch
benjamin
: review+
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review
Setup build configuration for Firefox Screenshots in Firefox. v5
14.14 KB, patch
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review
Export Screenshots v6.3.0
524.91 KB, patch
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review
Disable browser_inlinesettings_browser.js on Windows due to intermittent failures
1.01 KB, patch
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review
Bug 1346825 - add Screenshots to the xperf whitelist
59 bytes, text/x-review-board-request
kmag
: review+
gchang
: approval-mozilla-beta+
Details
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.
Attached patch Build system patch (obsolete) — — Splinter Review
Depends on: 1347482
Blocks: firefox-screenshots
Summary: Setup build configuration for PageShot in Firefox → Setup build configuration for Firefox Screenshots in Firefox
Attached 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
Attached patch Build system patch v3 (obsolete) — — Splinter Review
Good catch, here's the fixed version.
Attachment #8851763 - Attachment is obsolete: true
Attached 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
Depends on: 1352387
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
Attached patch Example export for Screenshots (obsolete) — — Splinter Review
MozReview-Commit-ID: FotxWZ7hf1X
Attachment #8852200 - Attachment is obsolete: true
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)
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+
Attachment #8853389 - Flags: review?(benjamin) → review+
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.
Attachment #8853388 - Attachment is obsolete: true
Blocks: 1355569
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+
(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.
Attached 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)
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+
Blocks: 1355998
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.
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
Correct file this time.
Attachment #8857866 - Attachment is obsolete: true
We ended up exporting version 6.3.0 of Screenshots.
Attachment #8857220 - Attachment is obsolete: true
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.
Blocks: 1356243
Depends on: 1356394
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.
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 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+
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.
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.

Attachment

General

Creator:
Created:
Updated:
Size: