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 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 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.

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...

> 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 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 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!

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/01dbdfc733d2 Make nsComputedDOMStyle::GetStyleContext / GetStyleContextNoFlush not take a presShell. r=bz

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/319210e2f742 followup: Fix mac-only bustage on a CLOSED TREE. r=me

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