1358495 - Implement getComputedStyleWithoutFlushing or equivalent

Status: RESOLVED WONTFIX

(Core :: DOM: CSS Object Model, enhancement)

Description

[Florian Quèze [:florian]]

Front-end code often needs to get computed style data, but sometimes we know it's OK if the data is slightly stale. getBoundsWithoutFlushing on the nsIDOMWindowUtils interface is very useful, but sometimes not enough.

Comment 1

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: Bug 1358495 - Part 2: Implement window.getComputedStyleWithoutFlushing.

Sometimes we know it is OK to get computed style without flushing because the
data is slightly stale.

Make this API as chrome-only because content might not need this.

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

Comment 2

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Comment on attachment 8862366 [details]
Bug 1358495 - Part 2: Implement window.getComputedStyleWithoutFlushing.

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

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 8862366 [details]
Bug 1358495 - Part 2: Implement window.getComputedStyleWithoutFlushing.

https://reviewboard.mozilla.org/r/134316/#review137576

This generally looks OK.  A few comments below, so I'd like to see another version of the patch.  Also, you'll need to get a DOM peer's review on the .webidl file change.

::: dom/base/nsGlobalWindow.cpp:11047
(Diff revision 2)
> +already_AddRefed<nsICSSDeclaration>
> +nsGlobalWindow::GetComputedStyleWithoutFlushingOuter(Element& aElt,
> +                                                     const nsAString& aPseudoElt)

Instead of duplicating GetComputedStyleOuter, let's just keep that one method, but add an argument to it for the aWithoutFlushing flag.

::: dom/webidl/Window.webidl:133
(Diff revision 2)
> +// bug 1358495
> +partial interface Window {
> +  [NewObject, Throws, ChromeOnly] CSSStyleDeclaration? getComputedStyleWithoutFlushing(Element elt, optional DOMString pseudoElt = "");
> +};

Let's put this instead in the "Mozilla-specific stuff" partial interface below.

::: layout/style/nsComputedDOMStyle.h:80
(Diff revision 2)
> -                     AnimationFlag aFlag = eWithAnimation);
> +                     AnimationFlag aFlag = eWithAnimation,
> +                     bool aWithoutFlushing = false);

I think it would be good to use an enum, rather than a boolean, like we do for the AnimationFlag.  So how about:

  enum FlushingFlag {
    eNormalFlushing,
    eWithoutFlushing,
  };

or something like that.

::: layout/style/nsComputedDOMStyle.h:757
(Diff revision 2)
>    /**
>     * Whether we include animation rules in the computed style.
>     */
>    AnimationFlag mAnimationFlag;
>  
> +  bool mWithoutFlushing;

Please add a comment saying what this means.

::: layout/style/nsComputedDOMStyle.cpp:758
(Diff revision 2)
>    if (!document) {
>      ClearStyleContext();
>      return;
>    }
>  
> +  if (!mWithoutFlushing) {

One other change that it looks like you'll need is to AssertFlushedPendingReflows, which is called from various property getters, so that it's OK if there are pending unflushed reflows, when our new flag is set.

Comment 4

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: Bug 1358495 - Part 1: Add window.getComputedStyleWithoutFlushing to IDL.

Sometimes we know it is OK to get computed style without flushing because the
data is slightly stale.

Make this API as chrome-only because content might not need this.

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

Comment 5

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Comment on attachment 8862366 [details]
Bug 1358495 - Part 2: Implement window.getComputedStyleWithoutFlushing.

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8862366 [details]
Bug 1358495 - Part 2: Implement window.getComputedStyleWithoutFlushing.

https://reviewboard.mozilla.org/r/134316/#review137708

Don't you need to change AssertFlushedPendingReflows to no-op when in non-flushing mode?  Or something?

Generally, in non-flushing mode, with this patch, the value of mFlushedPendingReflows is garbage, right?

::: layout/style/nsComputedDOMStyle.h:768
(Diff revision 3)
>     * Whether we include animation rules in the computed style.
>     */
>    AnimationFlag mAnimationFlag;
>  
> +  /**
> +   * Whether we should ignore style flushing while getting properties.

s/ignore/skip/ ?

Also, does this really skip only style flushin, or skip layout flushing too, for layout-dependent stuff?

::: layout/style/nsComputedDOMStyle.cpp:764
(Diff revision 3)
>    if (!document) {
>      ClearStyleContext();
>      return;
>    }
>  
> +  AssertFlushedPendingReflows();

How can you assert that here?  Why would it be true?

This doesn't make sense to me...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8862801 [details]
Bug 1358495 - Part 1: Add window.getComputedStyleWithoutFlushing to IDL.

https://reviewboard.mozilla.org/r/134720/#review137706

I assume when landing you will fold the changesets together.  In fact, you should do it right no.  Otherwise you introduce a non-building changeset, which makes bisection of other things suffer.

::: commit-message-2cca3:6
(Diff revision 1)
> +Bug 1358495 - Part 1: Add window.getComputedStyleWithoutFlushing to IDL. r?bz
> +
> +Sometimes we know it is OK to get computed style without flushing because the
> +data is slightly stale.
> +
> +Make this API as chrome-only because content might not need this.

Not only might not, but should not, without a spec, etc.  This API is really easy to misuse.

::: dom/webidl/Window.webidl:370
(Diff revision 1)
>     * Same as nsIDOMWindow.windowRoot, useful for event listener targeting.
>     */
>    [ChromeOnly, Throws]
>    readonly attribute WindowRoot? windowRoot;
> +
> +  // bug 1358495

You don't need the bug number in the comment.  That's clear enough from the blame.

What you _should_ have, however, is a comment explaining what the method actually does.  Because it does two quite different flush-avoidances, and it's not obvious at all to the caller that one of those can cause this method to return null (and probably throw in the caller) when getComputedStyle would not have.

That needs to be clearly documented, because it makes this method _extremely_ dangerous to use.  I don't see any way callers could actually sanely use it, honestly...

Actually, maybe we should just remove that avoidance of flushing the parent, because again I really don't see how this API can sanely be used with that.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8862366 [details]
Bug 1358495 - Part 2: Implement window.getComputedStyleWithoutFlushing.

https://reviewboard.mozilla.org/r/134316/#review137712

::: layout/style/nsComputedDOMStyle.cpp:781
(Diff revision 3)
>  #endif
> +  }
>  
>    mPresShell = document->GetShell();
>    if (!mPresShell || !mPresShell->GetPresContext()) {
>      ClearStyleContext();

So this will possibly get hit where it would not have been hit with getComputedStyle, and cause gets to throw because there is no presshell.  Should we perhaps be flushing the parent here anyway or something?

Comment 9

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #7)
> Actually, maybe we should just remove that avoidance of flushing the parent,
> because again I really don't see how this API can sanely be used with that.

Do you mean flushing parent document just like what nsDocument::FlushPendingNotifications do?
If so, seems that does not make too many differences from the original behavior?

I think it might be OK to throw an exception if there is no PresShell if we document this well.
e.g. This API must be called after DOMContentLoaded.
Do we need to worry about BFCache in chrome-only case?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Do you mean flushing parent document just like what nsDocument::FlushPendingNotifications do?

No, I mean the parent->FlushPendingNotifications() call in nsGlobalWindow::GetComputedStyleOuter.

> If so, seems that does not make too many differences from the original behavior?

That's possible.

> I think it might be OK to throw an exception if there is no PresShell if we document this well.

Script has no control over whether there's a presshell right this second or not.  The rules governing it are not something UI developers should need to learn...

> e.g. This API must be called after DOMContentLoaded.

That gives you no guarantee of a presshell.  The presshell in a child document can go away (synchonously) on any DOM mutation in the parent document that can cause nsIFrames to be destroyed and won't be recreated until parent is flushed, either explicitly or by refresh driver.

So you might have no presshell, temporarily, just because your parent did something with flexbox that caused a frame reconstruct or so.

> Do we need to worry about BFCache in chrome-only case?

Probably not.

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8862366 [details]
Bug 1358495 - Part 2: Implement window.getComputedStyleWithoutFlushing.

Cancelling review for now.

Comment 12

[Jet Villegas (inactive)]

Assigning to the patch author.

Comment 13

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Closing this bug because bug 1363805 should be a more appropriate optimization that can cover this case.