Closed Bug 1430608 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

This will be needed to keep the Shadow Root styles around, and also to fix bug 
548397.
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...
Attachment #8942673 - Flags: review?(cam) → review+
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 :)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/16d1bd6dba91
Make nsMediaFeatures work with a document, not a pres context. r=heycam
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f628439e6f52
followup - Suppress valgrind error in MediaList::evaluate on a CLOSED TREE.
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.
Attachment #8943510 - Flags: review?(jseward)
https://hg.mozilla.org/mozilla-central/rev/16d1bd6dba91
https://hg.mozilla.org/mozilla-central/rev/f628439e6f52
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Pushed by ryanvm@gmail.com:
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
You need to log in before you can comment on or make changes to this bug.