Closed Bug 1411339 Opened 3 years ago Closed 3 years ago

Configuration for different UI densities.

Categories

(Testing :: mozscreenshots, defect)

Version 3
defect
Not set
normal

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: rndmustafa, Assigned: rndmustafa, Mentored)

References

Details

Attachments

(1 file)

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: 3 years ago3 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.