Configuration for different UI densities.

RESOLVED FIXED in Firefox 58

Status

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: rndmustafa, Assigned: rndmustafa, Mentored)

Tracking

Version 3
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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
(Assignee)

Updated

a year ago
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
Comment hidden (typo)
Comment hidden (typo)
Status: REOPENED → NEW
Comment hidden (mozreview-request)
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 hidden (mozreview-request)
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 9

a year ago
mozreview-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+
Comment hidden (mozreview-request)
ni'ing myself to see if we can get this landed.
Flags: needinfo?(mconley)
Landing.
Flags: needinfo?(mconley)

Comment 13

a year ago
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)

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/43aea4af4a20
Status: ASSIGNED → RESOLVED
Last Resolved: a year agoa year ago
status-firefox58: --- → fixed
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.