1411339 - Configuration for different UI densities.

Status: RESOLVED FIXED

(Testing :: mozscreenshots, defect)

Description

[rndmustafa]

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

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Thanks for filing this, Rand.

This configuration should test the default, compact and touch densities.

Comment 2

[Johann Hofmann [:johannh]]

Let me dupe bug 1369754 since you've started working on this one :)

Comment 3

[Johann Hofmann [:johannh]]

Argh, said I and did the opposite of what I intended to do, sorry.

Comment 5

[rndmustafa]

Attachment: Bug 1411339 - Configuration for different UI densities.

Review commit: https://reviewboard.mozilla.org/r/194088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194088/

Comment 6

[Matthew N. [:MattN]]

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 7

[rndmustafa]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194088/diff/1-2/

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

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!

Comment 10

[rndmustafa]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194088/diff/2-3/

Comment 11

[Mike Conley (:mconley) (:⚙️)]

ni'ing myself to see if we can get this landed.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Landing.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43aea4af4a20
Configuration for different UI densities. r=jaws,mconley

Comment 14

[Matthew N. [:MattN]]

Shouldn't we actually schedule this to run on m-c? Either as part of primaryUI or some other browser_* in a directory?

Comment 15

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/43aea4af4a20

Comment 16

[Mike Conley (:mconley) (:⚙️)]

(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!