1432490 - Do a bit of cleanup in nsComputedDOMStyle::GetStyleContext / GetStyleContextNoFlush.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

We override the pres shell argument all the time anyway from GetPropertyValue, unless the composed doc has no shell, to avoid mixing rule trees and such.

This simplifies the general setup from getComputedStyle by only making us care about one pres shell.

Once we fix bug 548397, we can think of supporting it again, which should be easy-ish, without adding much complexity.

I've also fixed a couple GetComposed / GetUncomposedDoc uses that I found and were bogus while at it.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1432490: Make nsComputedDOMStyle::GetStyleContext / GetStyleContextNoFlush not take a presShell.

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

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8944733 [details]
Bug 1432490: Make nsComputedDOMStyle::GetStyleContext / GetStyleContextNoFlush not take a presShell.

https://reviewboard.mozilla.org/r/214886/#review220536


Static analysis found 3 defects in this patch.
 - 3 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: accessible/base/StyleInfo.h:19
(Diff revision 1)
>  namespace a11y {
>  
>  class StyleInfo
>  {
>  public:
> -  StyleInfo(dom::Element* aElement, nsIPresShell* aPresShell);
> +  StyleInfo(dom::Element* aElement);

Error: Bad implicit conversion constructor for 'styleinfo' [clang-tidy: mozilla-implicit-constructor]

::: accessible/base/StyleInfo.h:19
(Diff revision 1)
>  namespace a11y {
>  
>  class StyleInfo
>  {
>  public:
> -  StyleInfo(dom::Element* aElement, nsIPresShell* aPresShell);
> +  StyleInfo(dom::Element* aElement);

Error: Bad implicit conversion constructor for 'styleinfo' [clang-tidy: mozilla-implicit-constructor]

::: accessible/base/StyleInfo.cpp:17
(Diff revision 1)
>  #include "nsIFrame.h"
>  
>  using namespace mozilla;
>  using namespace mozilla::a11y;
>  
> -StyleInfo::StyleInfo(dom::Element* aElement, nsIPresShell* aPresShell) :
> +StyleInfo::StyleInfo(dom::Element* aElement) :

Error: Bad implicit conversion constructor for 'styleinfo' [clang-tidy: mozilla-implicit-constructor]

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8944733 [details]
Bug 1432490: Make nsComputedDOMStyle::GetStyleContext / GetStyleContextNoFlush not take a presShell.

Well, apparently we do have a few tests for this... Let me see what to do about them before re-asking review.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Hmm, chances are this would break some stuff until we fix bug 548397 at least... I should be able to simplify a bit the setup though, most of the GetStyleContext callers just call with the document shell, so I can just keep the multi-presshell weirdness in nsComputedDOMStyle...

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Hmm, chances are this would break some stuff until we fix bug 548397 at least...

Right.

Also, in case you didn't notice, the spec for getComputedStyle around this multi-document case has marginal bearing at best on what browsers actually do... :(

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8944733 [details]
Bug 1432490: Make nsComputedDOMStyle::GetStyleContext / GetStyleContextNoFlush not take a presShell.

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8944733 [details]
Bug 1432490: Make nsComputedDOMStyle::GetStyleContext / GetStyleContextNoFlush not take a presShell.

https://reviewboard.mozilla.org/r/214886/#review228372

r=me with the nits below

::: commit-message-30952:3
(Diff revision 2)
> +Bug 1432490: Make nsComputedDOMStyle::GetStyleContext / GetStyleContextNoFlush not take a presShell. r?bz
> +
> +Everyone calls them from the shell of the current composed document, and this

s/from/with/

::: commit-message-30952:16
(Diff revision 2)
> +
> +That's technically a behavior change on the cases that used to pass nullptr, but
> +I think all callers are fine with that. I could also just add a special function
> +for that particular case, it may be worth it.
> +
> +Also it changes a few very suspicious usages GetUncomposedDoc.

"of GetUncomposedDoc" but as I comment below I think a bunch of those should be spun out into a separate bug.

::: dom/canvas/CanvasRenderingContext2D.cpp:1206
(Diff revision 2)
>      *aColor = value.GetColorValue();
>    } else {
>      // otherwise resolve it
>      nsCOMPtr<nsIPresShell> presShell = GetPresShell();
>      RefPtr<nsStyleContext> parentContext;
> -    if (mCanvasElement && mCanvasElement->IsInUncomposedDoc()) {
> +    if (mCanvasElement && mCanvasElement->IsInComposedDoc()) {

So this is a behavior change.  Do you mind doing that in a separate bug, with review from someone who understands the spec bits for this?  I can be that someone if needed; would have to do some reading.

::: dom/canvas/CanvasRenderingContext2D.cpp:1979
(Diff revision 2)
>  
>    mResetLayer = true;
>  
>    SetInitialState();
>  
> +  if (!mCanvasElement || !mCanvasElement->IsInComposedDoc()) {

This is also changing from IsInUncomposedDoc.  Again, please put this behavior changes in a separate bug (same one as the other behavior change is fine).

::: dom/canvas/CanvasRenderingContext2D.cpp:2715
(Diff revision 2)
>  static already_AddRefed<GeckoStyleContext>
> -GetFontParentStyleContext(Element* aElement, nsIPresShell* aPresShell,
> +GetFontParentStyleContext(Element* aElement,
> +                          nsIPresShell* aPresShell,
>                            ErrorResult& aError)
>  {
> -  if (aElement && aElement->IsInUncomposedDoc()) {
> +  if (aElement && aElement->IsInComposedDoc()) {

Again, behavior change, separate bug.

::: dom/canvas/CanvasRenderingContext2D.cpp:2894
(Diff revision 2)
>    ServoStyleSet* styleSet = aPresShell->StyleSet()->AsServo();
>  
> -  RefPtr<ServoStyleContext> parentStyle;
> +  RefPtr<nsStyleContext> parentStyle;
>    // have to get a parent style context for inherit-like relative
>    // values (2em, bolder, etc.)
> -  if (aElement && aElement->IsInUncomposedDoc()) {
> +  if (aElement && aElement->IsInComposedDoc()) {

Behavior change, separate bug.

::: dom/canvas/CanvasRenderingContext2D.cpp:4540
(Diff revision 2)
>  
>    // for now, default to ltr if not in doc
>    bool isRTL = false;
>  
>    RefPtr<nsStyleContext> canvasStyle;
> -  if (mCanvasElement && mCanvasElement->IsInUncomposedDoc()) {
> +  if (mCanvasElement && mCanvasElement->IsInComposedDoc()) {

Behavior change, separate bug.

::: dom/html/nsGenericHTMLElement.cpp:3032
(Diff revision 2)
>  }
>  
>  static bool
>  IsOrHasAncestorWithDisplayNone(Element* aElement, nsIPresShell* aPresShell)
>  {
> +  // FIXME(emilio): In the servo case it should suffice with something like:

s/with/to do/

Could file a followup to make this change, if the presshell's style set is servo, right?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8944733 [details]
Bug 1432490: Make nsComputedDOMStyle::GetStyleContext / GetStyleContextNoFlush not take a presShell.

https://reviewboard.mozilla.org/r/214886/#review228372

> "of GetUncomposedDoc" but as I comment below I think a bunch of those should be spun out into a separate bug.

I removed all those changes of GetUncomposedDoc. Will file separate bugs for them.

Thanks for the review!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01dbdfc733d2
Make nsComputedDOMStyle::GetStyleContext / GetStyleContextNoFlush not take a presShell. r=bz

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/319210e2f742
followup: Fix mac-only bustage on a CLOSED TREE. r=me

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/01dbdfc733d2
https://hg.mozilla.org/mozilla-central/rev/319210e2f742