1459498 - Another round of CSS Loader cleanup.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

Slow steps...

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1459498: Refactor nsStyleLinkElement so that all the stylesheet information comes from one place.

I've kept the nsAutoStrings in the StyleSheetInfo class on the hopes that the
compiler does RVO, but if it doesn't I can remove I guess.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1459498: Use a different enum to differentiate IsAlternate from HasAlternateRel.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1459498: Use StyleSheetInfo more.

Not as much as I'd like, because we have all the internal loads and child
sheets, but...

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1459498: Remove useless CreateSheet arguments.

This is one of the most important steps for bug 1459498. After this I can use
StyleSheetInfo to compute IsAlternate. Still all the preferred stylesheet stuff
is crazy...

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1459498: Avoid a useless QI.

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

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8973530 [details]
Bug 1459498: Avoid a useless QI.

https://reviewboard.mozilla.org/r/241892/#review247746

::: layout/style/Loader.cpp:882
(Diff revision 1)
>  
>  nsresult
>  Loader::CheckContentPolicy(nsIPrincipal* aLoadingPrincipal,
>                             nsIPrincipal* aTriggeringPrincipal,
>                             nsIURI* aTargetURI,
> -                           nsISupports* aContext,
> +                           nsINode* aContext,

drive-by nit: aContext is a pretty bland name though - it could mean just about anything.  Perhaps we should rename it aRequestingNode to make it more specific while we're here?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

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

(Ignore the keyframes-* tests, they're from bug 1458189 and are expected failure).

(In reply to Mats Palmgren (:mats) from comment #6)
> Comment on attachment 8973530 [details]
> Bug 1459498: Avoid a useless QI.
> 
> https://reviewboard.mozilla.org/r/241892/#review247746
> 
> ::: layout/style/Loader.cpp:882
> (Diff revision 1)
> >  
> >  nsresult
> >  Loader::CheckContentPolicy(nsIPrincipal* aLoadingPrincipal,
> >                             nsIPrincipal* aTriggeringPrincipal,
> >                             nsIURI* aTargetURI,
> > -                           nsISupports* aContext,
> > +                           nsINode* aContext,
> 
> drive-by nit: aContext is a pretty bland name though - it could mean just
> about anything.  Perhaps we should rename it aRequestingNode to make it more
> specific while we're here?

Agreed, will do :)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8973530 [details]
Bug 1459498: Avoid a useless QI.

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

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8973526 [details]
Bug 1459498: Refactor nsStyleLinkElement so that all the stylesheet information comes from one place.

https://reviewboard.mozilla.org/r/241884/#review247770

::: commit-message-cd193:3
(Diff revision 1)
> +I've kept the nsAutoStrings in the StyleSheetInfo class on the hopes that the
> +compiler does RVO, but if it doesn't I can remove I guess.

Yeah, I'm not certain.  To be honest it's probably fine just to use nsString for all of these.

::: dom/base/nsStyleLinkElement.h:45
(Diff revision 1)
> +    Yes,
> +    No
> +  };
> +
> +
> +  struct MOZ_STACK_CLASS StyleSheetInfo

Nit: call this something else, just to avoid it looking confusing given we also have mozilla::StyleSheetInfo?  Maybe just SheetInfo?

::: dom/base/nsStyleLinkElement.h:57
(Diff revision 1)
> +    bool mHasAlternateRel : 1;
> +    bool mIsInline : 1;

Nit: being a stack-only class, it's probably not worth making these bitfields.

::: dom/base/nsStyleLinkElement.cpp:48
(Diff revision 1)
> +  IsInline aIsInline
> +)
> +  : mURI(aURI)

Nit: I'm pretty sure prevailing style would be to put the closing paren at the end of the last argument.

::: dom/base/nsStyleLinkElement.cpp:301
(Diff revision 1)
>    // Loader could be null during unlink, see bug 1425866.
>    if (!doc || !doc->CSSLoader() || !doc->CSSLoader()->GetEnabled()) {
>      return Update { };
>    }
>  
> -  bool isInline;
> +  auto info = GetStyleSheetInfo();

Please write out the type for the variable, since it's not too unwieldy.  From the code following it's not so easy to see what it is.

::: dom/html/HTMLLinkElement.cpp:430
(Diff revision 1)
>    nsAutoString rel;
>    GetAttr(kNameSpaceID_None, nsGkAtoms::rel, rel);
>    uint32_t linkTypes = nsStyleLinkElement::ParseLinkTypes(rel);
> -  // Is it a stylesheet link?
>    if (!(linkTypes & nsStyleLinkElement::eSTYLESHEET)) {
> -    return;
> +    return { };

I think this is kind of unclear that we're returning a Nothing here.  For a moment I thought that perhaps the braced list was being passed through to the StyleSheetInfo constructor.

Can you instead write this as `return Nothing();`, and below?

::: dom/html/HTMLLinkElement.cpp:452
(Diff revision 1)
> -  // If we get here we assume that we're loading a css file, so set the
> -  // type to 'text/css'
> -  aType.AssignLiteral("text/css");
> +  nsAutoString href;
> +  GetAttr(kNameSpaceID_None, nsGkAtoms::href, href);
> +  if (href.IsEmpty()) {

Do we no longer need to set the type to "text/css"?

::: dom/html/HTMLLinkElement.cpp:469
(Diff revision 1)
> +  // should take care of serializing it properly.
> +  nsContentUtils::ASCIIToLower(media);
> +
> +  nsCOMPtr<nsIURI> uri = Link::GetURI();
> +  nsCOMPtr<nsIPrincipal> prin = mTriggeringPrincipal;
> +  return Some(StyleSheetInfo {

I'm still not a fan of using braces for constructing objects with non-trivial constructors like this...

::: dom/svg/SVGStyleElement.cpp:232
(Diff revision 1)
> +  GetAttr(kNameSpaceID_None, nsGkAtoms::media, media);
>    // The SVG spec is formulated in terms of the CSS2 spec,
>    // which specifies that media queries are case insensitive.
> -  nsContentUtils::ASCIIToLower(aMedia);
> +  nsContentUtils::ASCIIToLower(media);
>  
> -  GetAttr(kNameSpaceID_None, nsGkAtoms::type, aType);
> +  // FIXME(emilio): Why doesn't this do the same as HTMLStyleElement?

It really should do the same.  Feel free to fix this up here.

Or if you can add a common implementation of this method somewhere for both HTMLStyleElement and SVGStyleElement to use, that would be even better.  Followup is fine though.

::: dom/svg/SVGStyleElement.cpp:243
(Diff revision 1)
> -CORSMode
> -SVGStyleElement::GetCORSMode() const
> -{
> -  return AttrValueToCORSMode(GetParsedAttr(nsGkAtoms::crossorigin));
> +
> +  return Some(StyleSheetInfo {
> +    *OwnerDoc(),
> +    this,
> +    nullptr,
> +    nullptr,

Why do we not pass a principal in here?  This is an existing problem, but I don't see why we should be doing anything different from HTMLStyleElement here...

::: dom/svg/SVGStyleElement.cpp:245
(Diff revision 1)
> -{
> -  return AttrValueToCORSMode(GetParsedAttr(nsGkAtoms::crossorigin));
> +    *OwnerDoc(),
> +    this,
> +    nullptr,
> +    nullptr,
> +    net::ReferrerPolicy::RP_Unset,
> +    AttrValueToCORSMode(GetParsedAttr(nsGkAtoms::crossorigin)),

Can you either add a separate SVGStyleElement::GetCORSMode() method for this, or remove HTMLStyleElement's one and inline it in the StyleSheetInfo constructor call like this, for consistency?

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8973527 [details]
Bug 1459498: Use a different enum to differentiate IsAlternate from HasAlternateRel.

https://reviewboard.mozilla.org/r/241886/#review247772

::: dom/base/nsStyleLinkElement.h:62
(Diff revision 1)
>      mozilla::CORSMode mCORSMode;
>      nsAutoString mTitle;
>      nsAutoString mMedia;
>      nsAutoString mIntegrity;
>  
> +    bool mIsAlternate : 1;

Is this needed?

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8973528 [details]
Bug 1459498: Use StyleSheetInfo more.

https://reviewboard.mozilla.org/r/241888/#review247776

::: dom/base/nsContentSink.cpp:801
(Diff revision 1)
>    // If this is a fragment parser, we don't want to observe.
>    // We don't support CORS for processing instructions

The CORS part of this comment probably now belongs on top of the `info` declaration.  Although it has questionable value so maybe just remove it.

::: dom/base/nsStyleLinkElement.cpp:50
(Diff revision 1)
>    const nsAString& aTitle,
>    const nsAString& aMedia,
>    HasAlternateRel aHasAlternateRel,
>    IsInline aIsInline
>  )
> -  : mURI(aURI)
> +  : mContent(aContent)

Should this be in a previous patch?

::: layout/style/Loader.h
(Diff revision 1)
> -   * @param aElement the element linking to the stylesheet.  This must not be
> -   *                 null and must implement nsIStyleSheetLinkingElement.
> -   * @param aBuffer the stylesheet data
> -   * @param aTriggeringPrincipal The principal of the scripted caller that
> -   *                             initiated the load, if available. Otherwise
> -   *                             null.
> -   * @param aLineNumber the line number at which the stylesheet data started.
> -   * @param aTitle the title of the sheet.
> -   * @param aMedia the media string for the sheet.
> -   * @param aReferrerPolicy the referrer policy for loading the sheet.

It would be good to move the comments for things which are now in the StyleSheetInfo (if they're useful, which some of them are) over to the StyleSheetInfo member declarations.  Same for the other methods below.

::: layout/style/Loader.cpp:1958
(Diff revision 1)
>                                            isAlternate == IsAlternate::Yes,
>                                            matched == MediaMatched::Yes,

As mentioned in a previous bug, might be nice to use these types in SheetLoadData too.

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8973529 [details]
Bug 1459498: Remove useless CreateSheet arguments.

https://reviewboard.mozilla.org/r/241890/#review247792

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8973530 [details]
Bug 1459498: Avoid a useless QI.

https://reviewboard.mozilla.org/r/241892/#review247794

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03477c2e776a
Refactor nsStyleLinkElement so that all the stylesheet information comes from one place. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f3d4b8b6d9
Use a different enum to differentiate IsAlternate from HasAlternateRel. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/00978d0f6e27
Use StyleSheetInfo more. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f3986a987fa
Remove useless CreateSheet arguments. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a0526903d90
Remove a useless QI. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/88b5778a01ca
Rename nsIStyleSheetLinkingElement::StyleSheetInfo to SheetInfo. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb3186662dda
Make SheetLoadData constructor take enums instead of booleans. rs=heycam

Comment 15

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/03477c2e776a
https://hg.mozilla.org/mozilla-central/rev/c8f3d4b8b6d9
https://hg.mozilla.org/mozilla-central/rev/00978d0f6e27
https://hg.mozilla.org/mozilla-central/rev/9f3986a987fa
https://hg.mozilla.org/mozilla-central/rev/3a0526903d90
https://hg.mozilla.org/mozilla-central/rev/88b5778a01ca
https://hg.mozilla.org/mozilla-central/rev/fb3186662dda