553359 - Cache whether the PresContext is chrome or not

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

Attachment: patch

For lazy frame construction we should cache on the prescontext if it is chrome or not because lazy frame construction calls IsChrome frequently.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 433372 [details] [diff] [review]
patch

You don't need the NS_HIDDEN gunk (hidden visibility is default now) and you can make IsChrome() inline since it just returns mIsChromeCacheValue.  r=me with that.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Oh, and make call the new member mIsChrome?

Comment 3

[Timothy Nikkel (:tnikkel)]

Made those changes in my tree. Thanks.

Comment 4

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/b4fcd21cb6e5

Comment 5

[Timothy Nikkel (:tnikkel)]

Backed out because something in the push caused tsvg_opacity regression.
http://hg.mozilla.org/mozilla-central/rev/a5009861bd1b
http://hg.mozilla.org/mozilla-central/rev/820bf1a6374c

Comment 6

[Timothy Nikkel (:tnikkel)]

Landed again
http://hg.mozilla.org/mozilla-central/rev/ae2093359899
but backed out because it looks like this was responsible for the tsvg_opacity regression.
http://hg.mozilla.org/mozilla-central/rev/83bf27b04ddc
http://hg.mozilla.org/mozilla-central/rev/e4e482050eba

Comment 7

[Timothy Nikkel (:tnikkel)]

I realized that if IsChrome is never called we are actually doing extra work getting the value of IsChrome the slow way when the container is set or the docshell type changes.

So instead get the IsChrome value the first time we are asked for it, and cache the value, and keep around another bool that tells us if we have the real IsChrome value cached or not. I landed that and it seems to have fixed the tsvg_opacity regression.

http://hg.mozilla.org/mozilla-central/rev/7d5962d3a808

Will request post-landing review and make any changes based on that review.

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 438927 [details] [diff] [review]
patch v2

InvalidateIsChromeCacheInternal should be inline (i.e., in
nsPresContext.h).

nsPresContext::SetContainer should then call InvalidateIsChromeCache
(which should then inline to the same thing).

You could probably use |mutable| to avoid having to un-|const|-ify
the methods that you did, but it's probably not worth the bother.

r=dbaron with that

Comment 10

[Timothy Nikkel (:tnikkel)]

Ah, I did not know about mutable. I will land those changes.

Comment 11

[Timothy Nikkel (:tnikkel)]

Landed those changes
http://hg.mozilla.org/mozilla-central/rev/cc5c2ab5038a