Closed Bug 1329262 Opened 5 years ago Closed 4 years ago

Enable compact themes in mozscreenshots


(Firefox :: Theme, defect, P2)




Firefox 58
Tracking Status
firefox58 --- fixed


(Reporter: bgrins, Assigned: bgrins)




(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:  Key difference is that instead of setting devtools.theme, we will switch between the following theme ids:
Component: General → Theme
Priority: -- → P2
Assignee: nobody → bgrinstead
Try run:

I believe we still need to sign the addon.  Are there instructions somewhere for this?
I know that for Talos test add-ons, we sign using these instructions:
(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:

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:
New try push after debugging the 'timeouts' that weren't actually preventing screenshots from being taken:  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;

::: 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

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;

Attachment #8828547 - Flags: review?(MattN+bmo) → review+
Pushed by
Enable compact themes in mozscreenshots;r=MattN
Remove verifyConfigHelper call;r=MattN
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Depends on: 1332821
I had to backout the adding of the two new configurations for causing bug 1332821. Once that bug is fixed we can re-land.
Resolution: FIXED → ---
Target Milestone: Firefox 53 → ---
Pushed by
Enable compact themes in mozscreenshots. r=MattN
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.