Configuration for different UI densities.

RESOLVED FIXED in Firefox 58

Status

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: rndmustafa, Assigned: rndmustafa, Mentored)

Tracking

Version 3
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36
Mentor: jaws, mconley
Thanks for filing this, Rand.

This configuration should test the default, compact and touch densities.
Assignee: nobody → rndmustafa
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Status: REOPENED → NEW
Duplicate of this bug: 1369754
Status: NEW → ASSIGNED
Comment on attachment 8922916 [details]
Bug 1411339 - Configuration for different UI densities.

https://reviewboard.mozilla.org/r/194088/#review199164

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/UIDensities.jsm:26
(Diff revision 1)
> +        browserWindow.gCustomizeMode.setUIDensity(browserWindow.gUIDensity.MODE_COMPACT);
> +      },
> +
> +    },
> +
> +    normal: {

Hello,

One requirement for configuration names (that probably isn't documented anywhere) is that they should be unique across all of the configuration JSMs. This is so that one can look at the image filename and know exactly what the image was capturing. Unfortunately "normal" is already used by WindowSize.jsm so it should be changed. I would suggest appending the suffix "Density" to all three configurations to keep them consistent while also not conflicting.

Thanks for improving mozscreenshots!
Comment on attachment 8922916 [details]
Bug 1411339 - Configuration for different UI densities.

https://reviewboard.mozilla.org/r/194088/#review200184

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/UIDensities.jsm:18
(Diff revision 2)
> +    compactDensity: {
> +      async applyConfig() {

You should supply a selector for these. Probably `#navigator-toolbox, #appMenu-popup, #widget-overflow`

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/UIDensities.jsm:23
(Diff revision 2)
> +    compactDensity: {
> +      async applyConfig() {
> +        let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");
> +        browserWindow.gCustomizeMode.setUIDensity(browserWindow.gUIDensity.MODE_COMPACT);
> +      },
> +

nit, remove this blank line

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/UIDensities.jsm:31
(Diff revision 2)
> +    normalDensity: {
> +      async applyConfig() {
> +        let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");
> +        browserWindow.gCustomizeMode.setUIDensity(browserWindow.gUIDensity.MODE_NORMAL);
> +      },
> +

nit, remove this blank line

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/UIDensities.jsm:39
(Diff revision 2)
> +
> +    },
> +
> +  },
> +

nit, remove these blank lines.
Attachment #8922916 - Flags: review+
Comment on attachment 8922916 [details]
Bug 1411339 - Configuration for different UI densities.

https://reviewboard.mozilla.org/r/194088/#review200630

With jaws' issues fixed, I'm into this. Thanks, Rand!
Attachment #8922916 - Flags: review?(mconley) → review+
ni'ing myself to see if we can get this landed.
Flags: needinfo?(mconley)
Landing.
Flags: needinfo?(mconley)
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/43aea4af4a20
Configuration for different UI densities. r=jaws,mconley
Shouldn't we actually schedule this to run on m-c? Either as part of primaryUI or some other browser_* in a directory?
Flags: needinfo?(mconley)
https://hg.mozilla.org/mozilla-central/rev/43aea4af4a20
Status: ASSIGNED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #14)
> Shouldn't we actually schedule this to run on m-c? Either as part of
> primaryUI or some other browser_* in a directory?

Yep, filed bug 1416796 for this. Thanks!
Flags: needinfo?(mconley)
You need to log in before you can comment on or make changes to this bug.