James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) Assignee
	Description • 7 years ago

This is part of the web manifest spec and seems pretty useful.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 1 • 7 years ago

In particular, this is the 'display-mode' media feature.

Bug 1104916 says we already did this.  Is that not the case?

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) Assignee
	Comment 2 • 7 years ago

Oh, I hadn't seen that, thanks! It looks like it only does 'fullscreen' and 'browser' right now, so we need to add support for 'standalone' and 'minimal-ui' for PWA.

	Eugen Sawin [:esawin]
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8895918 [details]
Bug 1369815 - Add display mode to GeckoViewSettings

https://reviewboard.mozilla.org/r/167196/#review172406

This won't work because of our lazy-load-register mechanics.

We only load a module's frame script and register its content module when the chrome module is registered (implicitly triggered by assigning a listener in GeckoView).

Since this does not fall into the usual app-provided listener pattern, it would be best to provide a dedicate module for this, with its listener set internally in GeckoView or we could move this to GeckoViewSettings.

	Dylan Roeh (:droeh) (he/him)
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8895919 [details]
Bug 1369815 - Set the display mode for standalone PWA

https://reviewboard.mozilla.org/r/167198/#review172496

	Eugen Sawin [:esawin]
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8895918 [details]
Bug 1369815 - Add display mode to GeckoViewSettings

https://reviewboard.mozilla.org/r/167196/#review172514

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewSettings.java:38
(Diff revision 3)
> +
> +        DisplayMode(int mode) {
> +            mMode = mode;
> +        }
> +
> +        public int value() {

Do we want to expose the value publicly? If not, we don't need that method.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewSettings.java:58
(Diff revision 3)
> +
> +    /*
> +     * Key to specify which display-mode we should use
> +     */
> +    public static final Key<Boolean> USE_DISPLAY_MODE =
> +        new Key<Boolean>("useDisplayMode");

Should be just "displayMode", the "use"-prefix indicates a boolean setting.

Also, please set the default value in the constructor.

We should also change the order of default-initialization to assure that USE_MULTIPROCESS is set first, so that the frame scripts for the other settings are loaded.

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm:42
(Diff revision 3)
>  
>    onSettingsUpdate() {
>      debug("onSettingsUpdate: " + JSON.stringify(this.settings));
>  
>      this.useTrackingProtection = !!this.settings.useTrackingProtection;
> +    this.useDisplayMode = this.settings.useDisplayMode;

Same: this.displayMode.

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm:83
(Diff revision 3)
>      }
>      parentNode.appendChild(this.browser);
> +
> +    // Re-set the display mode, as we probably need to set it on
> +    // a different docshell now
> +    this.useDisplayMode = this.useDisplayMode;

We don't support switching e10s modes after startup, so we should not need this.

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm:86
(Diff revision 3)
> +    // Re-set the display mode, as we probably need to set it on
> +    // a different docshell now
> +    this.useDisplayMode = this.useDisplayMode;
> +  }
> +
> +  get useDisplayMode() {

displayMode

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm:90
(Diff revision 3)
> +
> +  get useDisplayMode() {
> +    return this._displayMode;
> +  }
> +
> +  set useDisplayMode(aMode) {

displayMode

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm:92
(Diff revision 3)
> +    return this._displayMode;
> +  }
> +
> +  set useDisplayMode(aMode) {
> +    if (!this.useMultiprocess) {
> +      this.window.QueryInterface(Ci.nsIInterfaceRequestor)

I assume we don't need to set this for the chrome window with e10s?

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm:95
(Diff revision 3)
> +  set useDisplayMode(aMode) {
> +    if (!this.useMultiprocess) {
> +      this.window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                   .getInterface(Ci.nsIDocShell)
> +                   .displayMode = aMode;
> +    } else {

Is it unsafe to load this frame script with e10s disabled? We might not need the check at all.

	Cameron McCormack (:heycam)
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8895917 [details]
Bug 1369815 - Add display mode to nsIDocShell and use it for media queries

https://reviewboard.mozilla.org/r/167194/#review172568

This is going in the right direction, but I'd like to see another version of the patch.

I think we should handle dynamic changes to the docshell's display mode values, just like for example nsDocShell::SetDeviceSizeIsPageSize does.  But we will need to call MediaFeatureValuesChangedAllDocuments rather than MediaFeatureValuesChanged:

http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/layout/base/nsPresContext.h#283-289

(It might be that it's not possible for the value to change once it's set by the app manifest mechanism, but other users of nsIDocShell::SetDisplayMode might come along in the future.)

::: docshell/base/nsDocShell.h:1053
(Diff revision 1)
>    // origin attribute set.
>    uint32_t mPrivateBrowsingId;
>  
>    nsString mInterceptedDocumentId;
>  
> +  uint32_t mDisplayMode;

Please add a comment saying what this is, and what kinds of values it can take.

::: docshell/base/nsIDocShell.idl:1158
(Diff revision 1)
> +  const unsigned long DISPLAY_MODE_BROWSER = 1;
> +  const unsigned long DISPLAY_MODE_STANDALONE = 2;
> +  const unsigned long DISPLAY_MODE_FULLSCREEN = 3;
> +  const unsigned long DISPLAY_MODE_MINIMAL_UI = 4;

I think we should use the same constant values that the display-mode media feature uses to represent these values, unless there is a good reason not to:

http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/layout/style/nsStyleConsts.h#1266-1270

::: layout/style/nsMediaFeatures.cpp:310
(Diff revision 1)
> +  if (!aPresContext) {
> +    return;
> +  }

Is there a specific reason to return without setting a value?  Previously, we would set NS_STYLE_DISPLAY_MODE_BROWSER if we didn't have a pres context.  Whether that's an observable change, I'm not sure.  Maybe calling window.matchMedia() on a display:none iframe's window would stop matching `(display-mode: browser)` in that case?

I think we should leave the old behaviour.

::: layout/style/nsMediaFeatures.cpp:334
(Diff revision 1)
> +  }
> +
> +  int32_t displayMode = NS_STYLE_DISPLAY_MODE_BROWSER;

Then add a static_assert in here that the values of the nsIDocShell constants and the NS_STYLE_DISPLAY_MODE_* ones match up, and drop the switch statement and just assign it.

Also, please add a comment to nsStyleConsts.h where the NS_STYLE_DISPLAY_MODE_* constants are defined to point to GeckoViewSettings.java where the values have to match up and we have no way of asserting this.

	Cameron McCormack (:heycam)
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8895976 [details]
Bug 1369815 - Add mochitest for additional display modes

https://reviewboard.mozilla.org/r/167248/#review172570

::: layout/style/test/chrome/test_display_mode.html:87
(Diff revision 2)
> +  // Test entering display mode mode through docshell
> +  setDisplayMode(Components.interfaces.nsIDocShell.DISPLAY_MODE_STANDALONE);
> +  shouldApply("all and (display-mode: standalone)");
> +  shouldNotApply("all and (display-mode: fullscreen)");
> +  shouldNotApply("all and (display-mode: browser)");
> +  shouldNotApply("all and (display-mode: minimal-ui)");

I'm a little surprised this passes without informing the pres context that the docshell's display mode value changed.  Can you additionally test some computed value based on an @media rule in the document, e.g.

  @media (display-mode: standalone) {
    #display { color: blue; }
  }

then check that it's blue after the display mode is updated?

	Cameron McCormack (:heycam)
	Comment 21 • 7 years ago
mozreview-review

Comment on attachment 8895917 [details]
Bug 1369815 - Add display mode to nsIDocShell and use it for media queries

https://reviewboard.mozilla.org/r/167194/#review173248

Looks good, thanks!

::: docshell/base/nsIDocShell.idl:1169
(Diff revision 2)
> +  const unsigned long DISPLAY_MODE_STANDALONE = 2;
> +  const unsigned long DISPLAY_MODE_FULLSCREEN = 3;
> +
> +  /**
> +   * Display mode for this docshell. Defaults to DISPLAY_MODE_BROWSER.
> +   * Media queries only look at the value in the parent docshell.

Parent or top-most?

	Pulsebot
	Comment 22 • 7 years ago

Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b956ceaaf8b
Add display mode to nsIDocShell and use it for media queries r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/d25a5921d961
Add display mode to GeckoViewSettings r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/a758c86dd864
Set the display mode for standalone PWA r=droeh
https://hg.mozilla.org/integration/mozilla-inbound/rev/e018aed68975
Add mochitest for additional display modes r=heycam

	Pulsebot
	Comment 23 • 7 years ago

Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8b0238e34be
fix JS linting error r=me

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 24 • 7 years ago

Backed out bug 1369817, bug 1369815, bug 1389269 and bug 1126479 for crashing in mochitest-chrome-3's layout/style/test/chrome/test_stylesheet_clone_import_rule.html on debug:

bug 1369817
https://hg.mozilla.org/integration/mozilla-inbound/rev/4af3a98934c427226e095246a098008bc3474ee7
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd81631bfad32fdc2fa46a8937d189caa21cb3d

bug 1369815
https://hg.mozilla.org/integration/mozilla-inbound/rev/7780a4af90284e7f812d44ec32adb68dcbcc94b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/76f15802d69c2835eaa13e92df4bee34637f3b6c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4871dc084c88feecdbabe1b3a9acdb5ffaa71a2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bbb1bd8335d0c49ac477e60e85a03029c8c6a17
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bc485e4ba834888fb7f5bbb97f9a91c33c1a485

bug 1389269
https://hg.mozilla.org/integration/mozilla-inbound/rev/78e49684354331b02d3e425d2afcd757559bd7db

bug 1126479
https://hg.mozilla.org/integration/mozilla-inbound/rev/bec2a9eac2d9c7ec52543e5b77a6ee38c3f6879f
https://hg.mozilla.org/integration/mozilla-inbound/rev/337a6416a05bc3a9fac980eaa54084b78cd639b6

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4fb119706335b308f049949e5641565b035e4f80&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=123274657&repo=mozilla-inbound

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 25 • 7 years ago

So Try shows this is indeed from this patchset: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06dc0a37528eddc566bf22c4db30090d2e24922c&selectedJob=123311402

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) Assignee
	Comment 26 • 7 years ago

Cameron, the mochitest appears to cause a shutdown leak, and I don't understand why. It looks like simply setting the displayMode on the docshell causes this. Do you have any idea what could be going on?

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) Assignee
	Comment 27 • 7 years ago
mozreview-review-reply

Comment on attachment 8895918 [details]
Bug 1369815 - Add display mode to GeckoViewSettings

https://reviewboard.mozilla.org/r/167196/#review172514

> Do we want to expose the value publicly? If not, we don't need that method.

We do, because we need the integer for the setInteger() method.

> I assume we don't need to set this for the chrome window with e10s?

Correct, for e10s we only need to set the parent docshell in the content process.

> Is it unsafe to load this frame script with e10s disabled? We might not need the check at all.

It's safe, but it sets the value on the wrong docshell.

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) Assignee
	Comment 28 • 7 years ago
mozreview-review-reply

Comment on attachment 8895976 [details]
Bug 1369815 - Add mochitest for additional display modes

https://reviewboard.mozilla.org/r/167248/#review172570

> I'm a little surprised this passes without informing the pres context that the docshell's display mode value changed.  Can you additionally test some computed value based on an @media rule in the document, e.g.
> 
>   @media (display-mode: standalone) {
>     #display { color: blue; }
>   }
> 
> then check that it's blue after the display mode is updated?

I'm calling nsPresContext::MediaFeatureValuesChangedAllDocuments() when the display mode changes, isn't that enough? I can add some more rules.

	Cameron McCormack (:heycam)
	Comment 29 • 7 years ago

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #28)
> I'm calling nsPresContext::MediaFeatureValuesChangedAllDocuments() when the
> display mode changes, isn't that enough?

Yes, that should be enough.  But I think you weren't calling it in the patch at the time I looked at the test, so I was surprised it would be passing without it.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #26)
> Cameron, the mochitest appears to cause a shutdown leak, and I don't
> understand why. It looks like simply setting the displayMode on the docshell
> causes this. Do you have any idea what could be going on?

I don't, unfortunately.  Xidorn, do you have any idea what might be causing a leak with this @import-related test?  (Not stylo.)

	Xidorn Quan [:xidorn] UTC+11
	Comment 30 • 7 years ago

Hmm, this seems to be the same issue as bug 1387490 actually. There is nothing to do about the @import test. It crashes simply because it happens to be the last test before shutdown. That says, bug 1387490 affects not only stylo, but also non-stylo, as shown in this case.

	Xidorn Quan [:xidorn] UTC+11
	Comment 31 • 7 years ago

FWIW, I didn't manage to run this test locally... It seems that the iframe page doesn't load for me. I have no idea why.

	Pulsebot
	Comment 32 • 7 years ago

Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a7d96d920d6
Add display mode to nsIDocShell and use it for media queries r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ce3ad26c529
Add display mode to GeckoViewSettings r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8428fa4fe1a
Set the display mode for standalone PWA r=droeh
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb774244a1b
Add mochitest for additional display modes r=heycam

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 33 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5a7d96d920d6
https://hg.mozilla.org/mozilla-central/rev/6ce3ad26c529
https://hg.mozilla.org/mozilla-central/rev/e8428fa4fe1a
https://hg.mozilla.org/mozilla-central/rev/5bb774244a1b

	Jonathan Kew [:jfkthame]
	Comment 34 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e73aaa0359f17bd9f1e8c0e241ee1146a7d18c9f

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) Assignee
	Comment 35 • 7 years ago

Whoops, we figured this out earlier -- I was leaking the PresContext in nsDocShell::SetDisplayMode(). Sorry for the false alarm!

	Chris Mills [:cmills]
	Comment 36 • 7 years ago

I've added a note to the Fx57 rel notes to cover this:

https://developer.mozilla.org/en-US/Firefox/Releases/57#CSS

I've also made the browser support situation clear on

https://developer.mozilla.org/en-US/docs/Web/CSS/@media/display-mode
https://developer.mozilla.org/en-US/docs/Web/CSS/@media
https://developer.mozilla.org/en-US/docs/Web/CSS/Media_Queries/Using_media_queries
https://developer.mozilla.org/en-US/docs/Web/Manifest#display

Let me know if that looks OK. Thanks!