Closed Bug 1329262 Opened 3 years ago Closed 2 years ago

Enable compact themes in mozscreenshots

Categories

(Firefox :: Theme, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(2 files)

In Bug 1314091 we are switching the devedition theme to be two separate 'compact' themes.  Let's get these up and running in mozscreenshots to make it easier to detect regressions and view changes across platforms.

There's an existing devedition configuration here: https://dxr.mozilla.org/mozilla-central/source/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/DevEdition.jsm.  Key difference is that instead of setting devtools.theme, we will switch between the following theme ids:

firefox-compact-light@mozilla.org
firefox-compact-dark@mozilla.org
Component: General → Theme
Priority: -- → P2
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1887bc3b948354c23b563d0141e5d180e020f454&filter-tier=1&filter-tier=2&filter-tier=3

I believe we still need to sign the addon.  Are there instructions somewhere for this?
(In reply to Mike Conley (:mconley) - Getting through review / needinfo backlog from comment #3)
> I know that for Talos test add-ons, we sign using these instructions:
> https://wiki.mozilla.org/EngineeringProductivity/HowTo/SignExtensions

Thanks, I'm guessing this is about what we need to do here as well.  I also like the idea of turning this into a Temporary Addon / not an addon at all if it was possible to avoid this step in the future.
I'm seeing a red 'ss' job on Windows XP opt although I think I've seen that on every try push I've done for mozscreenshots: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1887bc3b948354c23b563d0141e5d180e020f454&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=69668640
New try push after debugging the 'timeouts' that weren't actually preventing screenshots from being taken: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5fac79e03adcc4f70491b3fe8b3d1d138c4f3d8.  Fixed this orange by updating the requestLongerTimeout for the normal case and relying instead on the buildbot timeout to kill off a job that's actually timed out.
Comment on attachment 8827584 [details]
Bug 1329262 - Enable compact themes in mozscreenshots;

https://reviewboard.mozilla.org/r/105216/#review106488

::: browser/tools/mozscreenshots/head.js:17
(Diff revision 2)
> -  requestLongerTimeout(10);
> +  // This timeout doesn't actually end the job even if it is hit - the buildbot timeout will
> +  // handle things for us if the test actually hangs.

In case you were wondering the buildbot timeout is 30 minutes and specified at https://hg.mozilla.org/build/buildbot-configs/annotate/ed364aa06e0dcaed2148d41f0b2707f9cfcfb0a8/mozilla-tests/config.py#l402

It seems like we're at most 21 minutes on your try push so that's good.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/LightweightThemes.jsm:94
(Diff revision 2)
>  function verifyConfigHelper() {
>    return new Promise((resolve, reject) => {
>      let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");
>      if (browserWindow.document.documentElement.hasAttribute("lwtheme")) {
>        resolve("verifyConfigHelper");
>      } else {
>        reject("The @lwtheme attribute wasn't present so themes may not be available");
>      }
>    });
>  }

In a separate commit can you remove this `verifyConfigHelper` function and its callers since bug 854126 made it unnecessary AFAIK. I don't think there are remaining cases where LWT aren't used. Please mention that bug number in the commit message.
Attachment #8827584 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8828547 [details]
Bug 1329262 - Remove verifyConfigHelper call;

https://reviewboard.mozilla.org/r/105898/#review106856

Thanks
Attachment #8828547 - Flags: review?(MattN+bmo) → review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/018d06dd2851
Enable compact themes in mozscreenshots;r=MattN
https://hg.mozilla.org/integration/autoland/rev/7159d5238506
Remove verifyConfigHelper call;r=MattN
https://hg.mozilla.org/mozilla-central/rev/018d06dd2851
https://hg.mozilla.org/mozilla-central/rev/7159d5238506
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
I had to backout the adding of the two new configurations for causing bug 1332821. Once that bug is fixed we can re-land.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 53 → ---
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15cd9f40c411
Enable compact themes in mozscreenshots. r=MattN
https://hg.mozilla.org/mozilla-central/rev/15cd9f40c411
Status: REOPENED → RESOLVED
Closed: 3 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.