1430608 - Make nsMediaFeatures work with a document, not a pres context.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

This will be needed to keep the Shadow Root styles around, and also to fix bug 
548397.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1430608: Make nsMediaFeatures work with a document, not a pres context.

Returning a zero-sized viewport in the case there's no shell / pres context.

For now, no other change yet. After this we can start tweaking the ShadowRoot
Stylist setup, and even try to return computed styles without a document \o/.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8942673 [details]
Bug 1430608: Make nsMediaFeatures work with a document, not a pres context.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8942673 [details]
Bug 1430608: Make nsMediaFeatures work with a document, not a pres context.

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

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8942673 [details]
Bug 1430608: Make nsMediaFeatures work with a document, not a pres context.

https://reviewboard.mozilla.org/r/212926/#review218808

::: dom/base/nsIDocument.h:2218
(Diff revision 3)
> +  bool IsSVGGlyphDocument() const {
> +    return mIsSVGGlyph;
> +  }

Nit: the gfx code uses the term "SVG glyphs document" (i.e. plural), so for consistency we should use the same here.  Also mIsSVGGlyphsDocument.

::: dom/base/nsIDocument.h:3547
(Diff revision 3)
>    bool mEncodingMenuDisabled : 1;
>  
>    // True if dom.webcomponents.enabled pref is set when document is created.
>    bool mIsWebComponentsEnabled : 1;
>  
> +  // True if this document is for an SVG glyph.

Maybe s/SVG glyph/SVG-in-OpenType font/.

::: layout/style/nsMediaFeatures.cpp:92
(Diff revision 3)
> -  nsSize size;
> -  if (aPresContext->IsRootPaginatedDocument())
> +  nsIPresShell* presShell = aDocument->GetShell();
> +  if (!presShell) {

There are now a few places in this file where we need to get a prescontext for the document.  Can you add a helper function that does the GetShell() / GetPresContext() calls?  Then some of the functions can simplify down to

  if (nsPresContext* pc = GetPresContext(aDocument)) {
    ... 
  } else {
    ... default ...
  }

::: layout/style/nsMediaFeatures.cpp:94
(Diff revision 3)
>  static nsSize
> -GetSize(nsPresContext* aPresContext)
> +GetSize(nsIDocument* aDocument)
>  {
> -  nsSize size;
> -  if (aPresContext->IsRootPaginatedDocument())
> +  nsIPresShell* presShell = aDocument->GetShell();
> +  if (!presShell) {
> +    return { };

List initialization syntax feels a bit weird here.  Just do `return nsSize();` instead to make it look more obvious?  (And below in a few places.)

Also, in practice, do we ever need to evaluate media queries for a document with no pres context?  If so, it seems like the viewport size we use in this case should be defined by a spec (similarly for other media features), and we should file an issue about that.  (I assume there's no appropriate spec wording currently.)

::: layout/style/nsMediaFeatures.cpp:131
(Diff revision 3)
> -  // It would be nice to call
> +  return nsContentUtils::ShouldResistFingerprinting(aDocument->GetDocShell());
> -  // nsLayoutUtils::GetDeviceContextForScreenInfo here, except for two
> -  // things:  (1) it can flush, and flushing is bad here, and (2) it
> -  // doesn't really get us consistency in multi-monitor situations
> -  // *anyway*.

This comment still seems relevant.  Keep it?

::: layout/style/nsMediaFeatures.cpp:212
(Diff revision 3)
> -    orientation = NS_STYLE_ORIENTATION_PORTRAIT;
> -  }
> +  int32_t orientation = size.width > size.height
> +    ? NS_STYLE_ORIENTATION_LANDSCAPE : NS_STYLE_ORIENTATION_PORTRAIT;

Nit: if rewording this to use a ternary operator, do the same in GetOrientation too.

::: layout/style/nsMediaFeatures.cpp:270
(Diff revision 3)
>  
>  static void
> -GetColor(nsPresContext* aPresContext, const nsMediaFeature*,
> +GetColor(nsIDocument* aDocument, const nsMediaFeature*,
>           nsCSSValue& aResult)
>  {
>    uint32_t depth = 24; // Use depth of 24 when resisting fingerprinting.

Update the comment to mention this is also for prescontext-less situations?

::: layout/style/nsMediaFeatures.cpp:346
(Diff revision 3)
> -static void
> -GetDisplayMode(nsPresContext* aPresContext, const nsMediaFeature*,
> +static nsIDocument*
> +TopDocument(nsIDocument* aDocument)
> -               nsCSSValue& aResult)
>  {
> -  nsCOMPtr<nsISupports> container;
> -  RefPtr<nsIDocShell> docShell;
> +  nsIDocument* current = aDocument;
> +  while (nsIDocument* parent = current->GetParentDocument()) {

I actually can't tell whether following the parent document chain and getting that document's shell is equivalent to calling GetRootPresContext() and getting that pres context's document's shell (when the pres context does exist).

I assume this is right if the display-mode tests still pass...

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8942673 [details]
Bug 1430608: Make nsMediaFeatures work with a document, not a pres context.

https://reviewboard.mozilla.org/r/212926/#review218808

> List initialization syntax feels a bit weird here.  Just do `return nsSize();` instead to make it look more obvious?  (And below in a few places.)
> 
> Also, in practice, do we ever need to evaluate media queries for a document with no pres context?  If so, it seems like the viewport size we use in this case should be defined by a spec (similarly for other media features), and we should file an issue about that.  (I assume there's no appropriate spec wording currently.)

I like the list syntax a bit better, fwiw :)

As of right now we won't evaluate media queries without a pres context, but we will need to to fix bug 548397.

Relevant issues in the specs are:

 * https://github.com/whatwg/html/issues/1813
 * https://github.com/w3c/csswg-drafts/issues/571

So evaluating media queries with a 0x0 viewport seems like the right thing to do.

> I actually can't tell whether following the parent document chain and getting that document's shell is equivalent to calling GetRootPresContext() and getting that pres context's document's shell (when the pres context does exist).
> 
> I assume this is right if the display-mode tests still pass...

Yeah, I got it wrong at the beginning and all those were orange :)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16d1bd6dba91
Make nsMediaFeatures work with a document, not a pres context. r=heycam

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f628439e6f52
followup - Suppress valgrind error in MediaList::evaluate on a CLOSED TREE.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: followup - Suprress valgrind error in MediaList::evaluate

This was pushed to unbust the tree. But since I'm not totally familiar with the related thing, it would be still good to have a post-push rubberstamp.

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/16d1bd6dba91
https://hg.mozilla.org/mozilla-central/rev/f628439e6f52

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/7861a7df0c4d
Loosen the Valgrind MediaList::evaluate suppression to cover both trunk and beta. r=emilio, a=RyanVM on a CLOSED TREE