1456435 - Make UpdateStyleSheet less bool-happy.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

In preparation for bug 1386840, where we're going to add more state.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1456435: Don't clone a URI for sheet loading.

nsIURI is immutable now.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1456435: Make UpdateStyleSheet less bool-happy.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1456435: Less bool outparam in Loader too.

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1456435: Make the loader a bit less outparam-happy.

Returning the same type and UpdateStyleSheet.

This hopefully helps seeing how the data flows between the methods, instead of
the messy bits we had before.

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8970520 [details]
Bug 1456435: Don't clone a URI for sheet loading.

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

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8970521 [details]
Bug 1456435: Make UpdateStyleSheet less bool-happy.

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8970522 [details]
Bug 1456435: Less bool outparam in Loader too.

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

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8970523 [details]
Bug 1456435: Make the loader a bit less outparam-happy.

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

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8970522 [details]
Bug 1456435: Less bool outparam in Loader too.

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8970523 [details]
Bug 1456435: Make the loader a bit less outparam-happy.

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

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8970521 [details]
Bug 1456435: Make UpdateStyleSheet less bool-happy.

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

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8970522 [details]
Bug 1456435: Less bool outparam in Loader too.

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

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8970523 [details]
Bug 1456435: Make the loader a bit less outparam-happy.

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

Comment 14

[Emilio Cobos Álvarez (:emilio)]

This is green modulo the CSP assertion that I removed on the last version, which was wrong:

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

This should be the whole thing as published now:

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

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8970520 [details]
Bug 1456435: Don't clone a URI for sheet loading.

https://reviewboard.mozilla.org/r/239288/#review245616

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8970521 [details]
Bug 1456435: Make UpdateStyleSheet less bool-happy.

https://reviewboard.mozilla.org/r/239290/#review245618

::: dom/base/nsIStyleSheetLinkingElement.h:105
(Diff revision 3)
>     * @param [out] aWillNotify whether aObserver will be notified when the sheet
>     *                          loads.  If this is false, then either we didn't
>     *                          start the sheet load at all, the load failed, or
>     *                          this was an inline sheet that completely finished
>     *                          loading.  In the case when the load failed the
>     *                          failure code will be returned.
>     * @param [out] whether the sheet is an alternate sheet.  This value is only
>     *              meaningful if aWillNotify is true.

This needs updating.

::: dom/base/nsIStyleSheetLinkingElement.h:116
(Diff revision 3)
>     * @param [out] whether the sheet is an alternate sheet.  This value is only
>     *              meaningful if aWillNotify is true.
>     * @param aForceUpdate whether we wand to force the update, flushing the
>     *                     cached version if any.
>     */
> -  virtual nsresult UpdateStyleSheet(nsICSSLoaderObserver* aObserver,
> +  virtual mozilla::Result<Update, nsresult>

(Hmm, is there anything that prevents us from generating Err(NS_OK) accidentally?)

::: dom/base/nsStyleLinkElement.h:86
(Diff revision 3)
>     * @param aForceUpdate true will force the update even if the URI has not
>     *                     changed.  This should be used in cases when something
>     *                     about the content that affects the resulting sheet
>     *                     changed but the URI may not have changed.

This too.

Mind documenting aOldShadowRoot while you're here too?

::: dom/base/nsStyleLinkElement.h:125
(Diff revision 3)
>     *                     removed the node from the document.
>     * @param aOldShadowRoot The ShadowRoot that used to contain the style.
>     *                     Passed as a parameter because on an update, the node
>     *                     is removed from the tree before the sheet is removed
>     *                     from the ShadowRoot.
>     * @param aForceUpdate true will force the update even if the URI has not

And this.  (Though if you want to stick to a real doc comment you'll need to give the ForceUpdate argument a name...)

::: dom/base/nsStyleLinkElement.cpp:341
(Diff revision 3)
>  
>    if (thisContent->IsInAnonymousSubtree() &&
>        thisContent->IsAnonymousContentInSVGUseSubtree()) {
>      // Stylesheets in <use>-cloned subtrees are disabled until we figure out
>      // how they should behave.
> -    return NS_OK;
> +    return Update { };

I don't know how that coding style thread shook out, but let me put on the record that I generally don't like {} initialization syntax like this when it's not necessary. :)  (It makes me wonder if there's something special going on -- is it initializing fields or calling a constructor?)

::: dom/base/nsStyleLinkElement.cpp:395
(Diff revision 3)
> -        return NS_OK; // We already loaded this stylesheet
> +        // If href is empty and this is not inline style then just bail.
> +        // IsAlternate doesn't really matter.

Is this the right comment here?  If href were empty would we have a non-null nsIURI?

::: dom/xml/nsXMLContentSink.cpp:606
(Diff revision 3)
> -      bool willNotify;
> -      bool isAlternate;
> -      rv = ssle->UpdateStyleSheet(mRunsToCompletion ? nullptr : this,
> -                                  &willNotify,
> -                                  &isAlternate);
> +      auto updateOrError =
> +        ssle->UpdateStyleSheet(mRunsToCompletion ? nullptr : this);
> +      if (updateOrError.isOk() &&
> +          updateOrError.unwrap().ShouldBlock() &&
> +          !mRunsToCompletion) {

I think we need to return updateOrError's error value if it has one.

(It's a shame there's no way to easily do something like MOZ_TRY but grab out the error value.  And that Result<> in general isn't super ergonomic like Rust's...)

(Also I wonder if just "update" is a better name than "updateOrError", here and below.  Though I guess the "auto" does cloud what the actual type is here...)

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8970521 [details]
Bug 1456435: Make UpdateStyleSheet less bool-happy.

https://reviewboard.mozilla.org/r/239290/#review245638

::: dom/base/nsStyleLinkElement.cpp:341
(Diff revision 3)
>  
>    if (thisContent->IsInAnonymousSubtree() &&
>        thisContent->IsAnonymousContentInSVGUseSubtree()) {
>      // Stylesheets in <use>-cloned subtrees are disabled until we figure out
>      // how they should behave.
> -    return NS_OK;
> +    return Update { };

I think it's nice, it reminds me of rust's shorthand syntax when it passes fields around, like:


```
return Update { willNotify, isAlternate };
```

But I guess I can change it. I tend think that since it's more general we should generally prefer it, but... :)

::: dom/base/nsStyleLinkElement.cpp:395
(Diff revision 3)
> -        return NS_OK; // We already loaded this stylesheet
> +        // If href is empty and this is not inline style then just bail.
> +        // IsAlternate doesn't really matter.

Nope, copy pasta, whoops.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8970521 [details]
Bug 1456435: Make UpdateStyleSheet less bool-happy.

https://reviewboard.mozilla.org/r/239290/#review245618

> (Hmm, is there anything that prevents us from generating Err(NS_OK) accidentally?)

https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/mfbt/ResultExtensions.h#30

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 8970522 [details]
Bug 1456435: Less bool outparam in Loader too.

https://reviewboard.mozilla.org/r/239292/#review245636

::: dom/base/nsStyleLinkElement.cpp:438
(Diff revision 4)
>    net::ReferrerPolicy referrerPolicy = GetLinkReferrerPolicy();
>    if (referrerPolicy == net::RP_Unset) {
>      referrerPolicy = doc->GetReferrerPolicy();
>    }
>  
> -  bool isAlternate;
> +  IsAlternate isAlternate = IsAlternate::No;

Use "auto" here, to match its use at the bottom of the function?

::: layout/style/Loader.h:467
(Diff revision 4)
>    void RemoveObserver(nsICSSLoaderObserver* aObserver);
>  
>    // These interfaces are public only for the benefit of static functions
>    // within nsCSSLoader.cpp.
>  
>    // IsAlternate can change our currently selected style set if none

This needs updating.

::: layout/style/Loader.cpp:2406
(Diff revision 4)
>    RefPtr<SheetLoadData> evt =
>      new SheetLoadData(this, EmptyString(), // title doesn't matter here
>                        aURI,
>                        aSheet,
>                        aElement,
> -                      aWasAlternate,
> +                      aWasAlternate == IsAlternate::Yes,

Maybe the SheetLoadData constructor should take an IsAlternate (even though it stores it in a bitfield)?

Comment 20

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/04 to 09/05) from comment #18)
> > (Hmm, is there anything that prevents us from generating Err(NS_OK) accidentally?)
> 
> https://searchfox.org/mozilla-central/rev/
> 78dbe34925f04975f16cb9a5d4938be714d41897/mfbt/ResultExtensions.h#30

Oh, cool.  That also suggests you can use MOZ_TRY in some places in your patches.

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8970523 [details]
Bug 1456435: Make the loader a bit less outparam-happy.

https://reviewboard.mozilla.org/r/239294/#review245644

::: dom/base/nsStyleLinkElement.cpp:436
(Diff revision 4)
>    net::ReferrerPolicy referrerPolicy = GetLinkReferrerPolicy();
>    if (referrerPolicy == net::RP_Unset) {
>      referrerPolicy = doc->GetReferrerPolicy();
>    }
>  
> -  IsAlternate isAlternate = IsAlternate::No;
> +  css::Loader::LoadSheetResult result;

Unused?

::: layout/style/Loader.h:196
(Diff revision 4)
>    typedef mozilla::net::ReferrerPolicy ReferrerPolicy;
>  
>  public:
>    typedef nsIStyleSheetLinkingElement::IsAlternate IsAlternate;
> +  typedef nsIStyleSheetLinkingElement::Completed Completed;
> +  typedef nsIStyleSheetLinkingElement::Update LoadSheetResult;

Is Update too unclear?  Otherwise would prefer to have the names matching...

::: layout/style/Loader.h:243
(Diff revision 4)
>     *        May be null.
> -   * @param [out] aCompleted whether parsing of the sheet completed.
> -   * @param [out] aIsAlternate whether the stylesheet ended up being an
> -   *        alternate sheet.
>     */
> -  nsresult LoadInlineStyle(nsIContent* aElement,
> +  Result<LoadSheetResult, nsresult>

So much result. :)

Comment 22

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #20)
> (In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/04 to 09/05)
> from comment #18)
> > > (Hmm, is there anything that prevents us from generating Err(NS_OK) accidentally?)
> > 
> > https://searchfox.org/mozilla-central/rev/
> > 78dbe34925f04975f16cb9a5d4938be714d41897/mfbt/ResultExtensions.h#30
> 
> Oh, cool.  That also suggests you can use MOZ_TRY in some places in your
> patches.

Yeah, though I mostly want MOZ_TRY_VAR, and that's a pain to use, because it means I cannot use auto. :(

Comment 23

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #19)
> ::: layout/style/Loader.cpp:2406
> (Diff revision 4)
> >    RefPtr<SheetLoadData> evt =
> >      new SheetLoadData(this, EmptyString(), // title doesn't matter here
> >                        aURI,
> >                        aSheet,
> >                        aElement,
> > -                      aWasAlternate,
> > +                      aWasAlternate == IsAlternate::Yes,
> 
> Maybe the SheetLoadData constructor should take an IsAlternate (even though
> it stores it in a bitfield)?

That was slightly annoying because makes SheetLoadData.h depend on nsIStyleLinkElement, and then we need to add moar typedefs.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #21)
> Comment on attachment 8970523 [details]
> Bug 1456435: Make the loader a bit less outparam-happy.
> 
> https://reviewboard.mozilla.org/r/239294/#review245644
> 
> ::: dom/base/nsStyleLinkElement.cpp:436
> (Diff revision 4)
> >    net::ReferrerPolicy referrerPolicy = GetLinkReferrerPolicy();
> >    if (referrerPolicy == net::RP_Unset) {
> >      referrerPolicy = doc->GetReferrerPolicy();
> >    }
> >  
> > -  IsAlternate isAlternate = IsAlternate::No;
> > +  css::Loader::LoadSheetResult result;
> 
> Unused?
> 
> ::: layout/style/Loader.h:196
> (Diff revision 4)
> >    typedef mozilla::net::ReferrerPolicy ReferrerPolicy;
> >  
> >  public:
> >    typedef nsIStyleSheetLinkingElement::IsAlternate IsAlternate;
> > +  typedef nsIStyleSheetLinkingElement::Completed Completed;
> > +  typedef nsIStyleSheetLinkingElement::Update LoadSheetResult;
> 
> Is Update too unclear?  Otherwise would prefer to have the names matching...

I thought it wasn't super clear, but if you prefer that, I surely can.

> ::: layout/style/Loader.h:243
> (Diff revision 4)
> >     *        May be null.
> > -   * @param [out] aCompleted whether parsing of the sheet completed.
> > -   * @param [out] aIsAlternate whether the stylesheet ended up being an
> > -   *        alternate sheet.
> >     */
> > -  nsresult LoadInlineStyle(nsIContent* aElement,
> > +  Result<LoadSheetResult, nsresult>
> 
> So much result. :)

Heh :)

Comment 25

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/04 to 09/05) from comment #24)
> (In reply to Cameron McCormack (:heycam) from comment #21)
> > 
> > ::: layout/style/Loader.h:196
> > (Diff revision 4)
> > >    typedef mozilla::net::ReferrerPolicy ReferrerPolicy;
> > >  
> > >  public:
> > >    typedef nsIStyleSheetLinkingElement::IsAlternate IsAlternate;
> > > +  typedef nsIStyleSheetLinkingElement::Completed Completed;
> > > +  typedef nsIStyleSheetLinkingElement::Update LoadSheetResult;
> > 
> > Is Update too unclear?  Otherwise would prefer to have the names matching...
> 
> I thought it wasn't super clear, but if you prefer that, I surely can.

I ended up not doing it because I didn't like the LoadSheetResult in Update, nor Update in LoadStyleLink or such... Maybe you can think of a better name that captures both use cases? FWIW in the beginning I planned for them to be two different types, but in the end they were so similar that...

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd2fe5744270
Don't clone a URI for sheet loading. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/52a7abd4ee49
Make UpdateStyleSheet less bool-happy. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/144000c6901b
Less bool outparam in Loader too. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd6cdf470fa4
Make the loader a bit less outparam-happy. r=heycam

Comment 27

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/fd2fe5744270
https://hg.mozilla.org/mozilla-central/rev/52a7abd4ee49
https://hg.mozilla.org/mozilla-central/rev/144000c6901b
https://hg.mozilla.org/mozilla-central/rev/bd6cdf470fa4

Comment 28

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/04 to 09/05) from comment #25)
> I ended up not doing it because I didn't like the LoadSheetResult in Update,
> nor Update in LoadStyleLink or such... Maybe you can think of a better name
> that captures both use cases? FWIW in the beginning I planned for them to be
> two different types, but in the end they were so similar that...

Not a big deal, so fine to leave as is.