Implement getComputedStyleWithoutFlushing or equivalent

RESOLVED WONTFIX

Status

()

Core
DOM: CSS Object Model
RESOLVED WONTFIX
7 months ago
5 months ago

People

(Reporter: florian, Assigned: wcpan)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

7 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Whiteboard: [qf] → [qf:p1]

Comment 3

7 months ago
mozreview-review
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.
Attachment #8862366 - Flags: review?(cam) → review-

Updated

7 months ago
Blocks: 1358387
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 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.
Attachment #8862801 - Flags: review?(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?
(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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Comment on attachment 8862366 [details]
Bug 1358495 - Part 2: Implement window.getComputedStyleWithoutFlushing.

Cancelling review for now.
Attachment #8862366 - Flags: review?(cam)

Comment 12

6 months ago
Assigning to the patch author.
Assignee: nobody → wpan

Updated

6 months ago
No longer blocks: 1358387
Closing this bug because bug 1363805 should be a more appropriate optimization that can cover this case.
Status: NEW → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.