Since we'll be touching control center again, it would help with ui-review and catching regressions if we add a configuration to mozscreenshots to capture images of the UI. We should probably include images with the following things: * Tracking protection on and disabled * Site permissions specified and not specified * Opening the subviews * Different security states (e.g. HTTPS, HTTP, trusted about pages, file:, etc.) Of course anything is better than nothing so we can do what's simplest first. We can combine combinations of these into one screenshot where it makes sense (to not lose useful coverage) like we do for Tabs.jsm. Perhaps the last one should be its own configuration JSM (or part of Tabs.jsm) that you can combine with the control center one to get all of the combinations. This just requires adding a file to https://mxr.mozilla.org/mozilla-central/source/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/ and writing a function for each image which returns a promise when the UI is ready to be captured. See https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots for more documentation.

Review commit: https://reviewboard.mozilla.org/r/67126/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67126/

Matt, I don't think the combination thing works with the approach I'm taking where we mostly load a site and inspect the corresponding security state. I would actually say we don't really need combination checking here. The security state box is completely separate from the permission box and the tracking protection box. We can either load e.g. an HTTP tracking page AND an HTTPS tracking page in the same config or we just choose one security state, e.g. normal HTTPS and accept that it's very unlikely to be broken when using HTTP instead.

Comment on attachment 8774673 [details] Bug 1268648 - Add a mozscreenshots configuration for control center. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67126/diff/1-2/

Comment on attachment 8774673 [details] Bug 1268648 - Add a mozscreenshots configuration for control center. https://reviewboard.mozilla.org/r/67126/#review66624 Thanks. The main issue is the job failure shown on Try. Also note that XP has been timing out every day since the permissionPrompt job was enabled so this may not actually get captured on XP depending on the ordering (until that's fixed). This may also push us over the time limit on other platforms so it would be useful to do a try push with `shouldCapture` modified to run like a Nightly would. ::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/ControlCenter.jsm:7 (Diff revision 2) > + * 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/. */ > + > +"use strict"; > + > +this.EXPORTED_SYMBOLS = ["ControlCenter"]; From the try push: > > 11 INFO TEST-UNEXPECTED-FAIL | browser/tools/mozscreenshots/browser_screenshots.js | A promise chain failed to handle a rejection: - at chrome://browser/content/browser.js:840 - Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebNavigation.loadURIWithOptions] ::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/ControlCenter.jsm:22 (Diff revision 2) > +const HTTP_PASSWORD_PAGE = "http://test2.example.org/extensions/mozscreenshots/browser/chrome/mozscreenshots/lib/controlCenter/password.html"; > +const MIXED_CONTENT_URL = "https://example.com/extensions/mozscreenshots/browser/chrome/mozscreenshots/lib/controlCenter/mixed.html"; > +const MIXED_ACTIVE_CONTENT_URL = "https://example.com/extensions/mozscreenshots/browser/chrome/mozscreenshots/lib/controlCenter/mixed_active.html"; > +const MIXED_PASSIVE_CONTENT_URL = "https://example.com/extensions/mozscreenshots/browser/chrome/mozscreenshots/lib/controlCenter/mixed_passive.html"; > +const TRACKING_PAGE = "http://tracking.example.org/extensions/mozscreenshots/browser/chrome/mozscreenshots/lib/controlCenter/tracking.html"; The avoid the long lines you could put the common path portion (/extensions/mozscreenshots/browser/chrome/mozscreenshots/lib/controlCenter/) in a `const`. ::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/ControlCenter.jsm:218 (Diff revision 2) > + > +function* loadPage(url) { > + let browserWindow = Services.wm.getMostRecentWindow("navigator:browser"); > + let gBrowser = browserWindow.gBrowser; > + BrowserTestUtils.loadURI(gBrowser.selectedBrowser, url); > + yield BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser); I guess we don't need to pass the URL as the third argument since we're not listening for subframes and we're doing the load right above? I wonder if it would be useful still to avoid an about:blank load? ::: browser/tools/mozscreenshots/mozscreenshots/extension/lib/controlCenter/mixed.html:2 (Diff revision 2) > +<!DOCTYPE HTML> > +<html dir="ltr" xml:lang="en-US" lang="en-US"> Do we really need any of these attributes in the HTML files? ::: browser/tools/mozscreenshots/mozscreenshots/extension/lib/controlCenter/mixed.html:8 (Diff revision 2) > + <head> > + <meta charset="utf8"> > + <title>Mixed Content test</title> > + </head> > + <body> > + <iframe src="http://example.com"></iframe> Perhaps this should be visibility:hidden so we don't detect a different if that page changes contents? ::: browser/tools/mozscreenshots/mozscreenshots/extension/lib/controlCenter/mixed_active.html:8 (Diff revision 2) > + <head> > + <meta charset="utf8"> > + <title>Mixed Active Content test</title> > + </head> > + <body> > + <iframe src="http://example.com"></iframe> Perhaps this should be visibility:hidden so we don't detect a different if that page changes contents? ::: browser/tools/mozscreenshots/mozscreenshots/extension/lib/controlCenter/tracking.html:1 (Diff revision 2) > +<!DOCTYPE HTML> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public You have the license on this file but not the other HTML ones

So Bug 1293986 deals with the XP problems, it's not strictly blocking this one but I'll set it to blocking for the record. As for the general timeout situation, above try push shows several timeouts when running the full set with this config on top. I think we'll have to increase the timeout for these builds. More configurations simply means more time. Matt, do you know how we can do that? :)

Link showing the ss jobs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a615b579aea4&filter-tier=1&filter-tier=2&filter-tier=3&exclusion_profile=false (In reply to Johann Hofmann [:johannh] from comment #11) > So Bug 1293986 deals with the XP problems, it's not strictly blocking this > one but I'll set it to blocking for the record. Agreed. Thanks for looking at that. > As for the general timeout situation, above try push shows several timeouts > when running the full set with this config on top. I think we'll have to > increase the timeout for these builds. More configurations simply means more > time. Matt, do you know how we can do that? :) IIRC you need to change https://hg.mozilla.org/build/buildbot-configs/file/0a13fdb3f8c1/mozilla-tests/config.py#l444 which isn't in central. I don't think there is a way to test this on Try so we'll need to estimate the time needed. I think 1800 is fine. At some point we may want to split the job into chunks instead.

1200 seconds was the lowest timeout so I think just bumping to 1800 is fine. 3 'script_maxtime': 12000, 2 'script_maxtime': 1800, 2 'script_maxtime': 4800, 2 'script_maxtime': 5400, 40 'script_maxtime': 7200,

Comment on attachment 8779846 [details] Bug 1268648 - Increase the script_timeout for mochitest-browser-screenshots to 1800s. https://reviewboard.mozilla.org/r/70754/#review68122

Thanks Armen! buildbot-config landed: https://hg.mozilla.org/build/buildbot-configs/rev/e538f7f45a14 We'll need to wait for a buildbot reconfig for this to take affect. Johann, you can ask in #releng tomorrow to get one done or ask when the last reconfig was. IIRC they happen roughly daily.

Actually, I think you can see when it gets merged to the production branch at https://hg.mozilla.org/build/buildbot-configs/graph as a sign that a reconfig probably happened.

In production: https://hg.mozilla.org/build/buildbot-configs/rev/e538f7f45a14

Excellent, I retriggered the ss jobs on https://treeherder.mozilla.org/#/jobs?repo=try&revision=a615b579aea4&filter-tier=1&filter-tier=2&filter-tier=3&exclusion_profile=false and it seemed to work so far (24 minutes on https://treeherder.mozilla.org/#/jobs?repo=try&revision=a615b579aea4&filter-tier=1&filter-tier=2&filter-tier=3&exclusion_profile=false&selectedJob=25517184 )

Alright, thanks for taking care of this! I'll land this patch along with the one from Bug 1293986 if you get around to approving that one tonight :)

https://hg.mozilla.org/integration/fx-team/rev/61340bd78a4bf05890f4b84a6ca57ba01c0ee29b Bug 1268648 - Add a mozscreenshots configuration for control center. r=MattN

I guess we want this to ride the trains.

Yeah, no uplift necessary.