Avoid parsing css text if there are identical inline style blocks.

Avoid parsing css text if there are identical inline style blocks.

(1) StyleEngine has two hashmaps, one is a mapping from css text to StyleSheetContents and the other is a mapping from StyleSheetContents to css text. When destroying StyleSheetContents, using the above mapping to remove css text.
The hashmaps are only for non-quirks document. Because non-quirks document cannot share styles with quirks document.
 
(2) made StyleSheetContents::checkLoaded to invoke new clients' sheetLoaded. To do so,
  (2-1) moved m_loadCompleted flag from StyleSheetContents to CSSStyleSheet,
  (2-2) added more "protect" to StyleSheetContents::checkLoaded to avoid CSSStyleSheet (i.e. client) and ownerNode being deleted by scripts, and
  (2-3) modified StyleSheetContents::loadCompleted() to see whether all clients completed loading.

(3) LinkStyle::setCSSStyleSheet should use StyleSheetContents' checkLoaded. Because now checkLoaded invokes only new clients' sheetLoaded. We don't need to directly invoke LinkStyle::sheetLoaded (this breaks m_loadCompleted flag and causes assertion failure). 

BUG=308781
TEST=all existing tests covers, so the tests should pass.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165139

[tasak, 2013-10-18 10:27:59]

Looking at http://www.chromestatus.com/features, size of inline style blocks
are:

Size = 6739
Size = 90
Size = 6739
Size = 90
Size = 6739
Size = 90
Size = 6739
Size = 90
... (repeat)

I'm not sure whether it is possible for StyleElement to have AtomicString whose
length = 6739.
(Currently < 256 is cached.)

[dglazkov, 2013-10-18 16:56:18]

Neat!

https://codereview.chromium.org/28553005/diff/1/Source/core/dom/StyleElement.cpp
File Source/core/dom/StyleElement.cpp (right):

https://codereview.chromium.org/28553005/diff/1/Source/core/dom/StyleElement....
Source/core/dom/StyleElement.cpp:38: static const unsigned
cacheableTextContentSizeLimit = 256;
How did you arrive at this number?

https://codereview.chromium.org/28553005/diff/1/Source/core/dom/StyleElement....
Source/core/dom/StyleElement.cpp:120: if (!m_sheetText.isEmpty() &&
m_sheet->ownerDocument() && m_sheet->ownerDocument()->isActive())
It might be a clearer (though more complex) factoring to introduce a switchable
block (cached vs. uncached) of machinery that deals with creating/clearing/etc.
stylesheets and using the one that's appropriate at the moment, rather than
having all these checks in the code. The switchable block solution would be a
lot less prone to code rot and accidental check omission.

https://codereview.chromium.org/28553005/diff/1/Source/core/dom/StyleEngine.h
File Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/28553005/diff/1/Source/core/dom/StyleEngine.h...
Source/core/dom/StyleEngine.h:164: // Mapping from textContents of StyleElement
to a pair of StyleSheetContent(second) and its usages(first).
Can we move this into a separate class, which is contained by StyleEngine.h?

[esprehn_google.com, 2013-10-19 03:18:49]

Lets hold off on this. I don't think the 256 limit is the way to go.
Instead I think we should consider something that caches the full thing and
uses time based caching.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[tasak, 2013-10-21 12:27:37]

Thank you for reviewing.

>Instead I think we should consider something that caches the full thing and
uses time based caching.

I modified the code to cache the full thing and copied MatchedPropertiesCache
logic to StyleSheetContentsCache.

So I would like to ask about "time based caching".
We need some timer to invoke StyleSheetContentsCache::clear?

https://codereview.chromium.org/28553005/diff/1/Source/core/dom/StyleElement.cpp
File Source/core/dom/StyleElement.cpp (right):

https://codereview.chromium.org/28553005/diff/1/Source/core/dom/StyleElement....
Source/core/dom/StyleElement.cpp:38: static const unsigned
cacheableTextContentSizeLimit = 256;
On 2013/10/18 16:56:19, Dimitri Glazkov wrote:
> How did you arrive at this number?

I have no strong reason. So I removed this magic number.

https://codereview.chromium.org/28553005/diff/1/Source/core/dom/StyleElement....
Source/core/dom/StyleElement.cpp:120: if (!m_sheetText.isEmpty() &&
m_sheet->ownerDocument() && m_sheet->ownerDocument()->isActive())
On 2013/10/18 16:56:19, Dimitri Glazkov wrote:
> It might be a clearer (though more complex) factoring to introduce a
switchable
> block (cached vs. uncached) of machinery that deals with
creating/clearing/etc.
> stylesheets and using the one that's appropriate at the moment, rather than
> having all these checks in the code. The switchable block solution would be a
> lot less prone to code rot and accidental check omission.

I added a comments which describes the condition.

https://codereview.chromium.org/28553005/diff/1/Source/core/dom/StyleEngine.h
File Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/28553005/diff/1/Source/core/dom/StyleEngine.h...
Source/core/dom/StyleEngine.h:164: // Mapping from textContents of StyleElement
to a pair of StyleSheetContent(second) and its usages(first).
On 2013/10/18 16:56:19, Dimitri Glazkov wrote:
> Can we move this into a separate class, which is contained by StyleEngine.h?

Done.

[dglazkov, 2013-10-21 18:24:09]

https://codereview.chromium.org/28553005/diff/100001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContentsCache.cpp (right):

https://codereview.chromium.org/28553005/diff/100001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContentsCache.cpp:91: if (cacheItem->hasOneRef())
I see. Can you not just make them RefPtrs and sweep for 0s?

https://codereview.chromium.org/28553005/diff/100001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContentsCache.h (right):

https://codereview.chromium.org/28553005/diff/100001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContentsCache.h:38: class CachedStyleSheetContents {
StyleSheetContentsCacheItem?

https://codereview.chromium.org/28553005/diff/100001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContentsCache.h:44: bool hasOneRef() { return
m_refCount == 1; }
Whoa! Why did you reinvent RefPtr here? :)

https://codereview.chromium.org/28553005/diff/100001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/28553005/diff/100001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.h:116: StyleSheetContents*
findStyleSheetContents(const AtomicString&);
Now that you have nice Cache member, why not just expose it directly and avoid
these plumb-through methods?

[tasak, 2013-10-25 05:23:13]

Thank you for reviewing.

I would like to add the result of ShadowDOM/StyleSheetInsert:

(Before)
Ignoring warm-up run (134.29799978621304 ms)
113.0349999293685 ms
113.62499999813735 ms
113.55600017122924 ms
113.66800009272993 ms
113.84900007396936 ms

(After)
Ignoring warm-up run (95.7500000949949 ms)
95.52900004200637 ms
95.19300004467368 ms
96.12900018692017 ms
95.81500012427568 ms
96.17800009436905 ms

https://codereview.chromium.org/28553005/diff/100001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContentsCache.cpp (right):

https://codereview.chromium.org/28553005/diff/100001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContentsCache.cpp:91: if (cacheItem->hasOneRef())
On 2013/10/21 18:24:10, Dimitri Glazkov wrote:
> I see. Can you not just make them RefPtrs and sweep for 0s?

I see.
I changed the code to use RefPtrs and to remove cached items which are not
shared by any other style elements.

https://codereview.chromium.org/28553005/diff/100001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContentsCache.h (right):

https://codereview.chromium.org/28553005/diff/100001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContentsCache.h:38: class CachedStyleSheetContents {
On 2013/10/21 18:24:10, Dimitri Glazkov wrote:
> StyleSheetContentsCacheItem?

Done.

https://codereview.chromium.org/28553005/diff/100001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContentsCache.h:44: bool hasOneRef() { return
m_refCount == 1; }
On 2013/10/21 18:24:10, Dimitri Glazkov wrote:
> Whoa! Why did you reinvent RefPtr here? :)

I see.
I re-implemented RefPtr...
I made StyleSheetContentsCacheItem inherit from RefCounted<...>.

https://codereview.chromium.org/28553005/diff/100001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/28553005/diff/100001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.h:116: StyleSheetContents*
findStyleSheetContents(const AtomicString&);
On 2013/10/21 18:24:10, Dimitri Glazkov wrote:
> Now that you have nice Cache member, why not just expose it directly and avoid
> these plumb-through methods?

Done.

[esprehn, 2013-10-30 21:33:49]

I think by using two maps (for bidirectional lookup) and letting the
CSSStyleSheet own the Contents instead of the cache we can make this simpler and
not need to sweep the cache.

https://codereview.chromium.org/28553005/diff/180011/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContentsCache.cpp (right):

https://codereview.chromium.org/28553005/diff/180011/Source/core/css/StyleShe...
Source/core/css/StyleSheetContentsCache.cpp:60:
m_sweepTimer.startOneShot(sweepTimeInSeconds);
On second thought I think this timer is too complex and it also doesn't share
between pages in the same process. We can use a static map to make this simpler.
Inside StyleEngine.cpp

static HashMap<AtomicString, StyleSheetContents*>& textToSheetCache()
{
  DEFINE_STATIC_LOCAL(HashMap<AtomicString, StyleSheetContents*>, cache, ());
  return cache;
}

static HashMap<AtomicString, StyleSheetContents*>& sheetToTextCache()
{
  DEFINE_STATIC_LOCAL(HashMap<StyleSheetContents*, AtomicString>, cache, ());
  return cache;
}

then inside ~StyleSheetContents you remove yourself from the cache (since now
you're dead, the cache itself doesn't keep you alive) by looking up the entry in
the sheet -> text map and then removing that entry and the one in the
textToSheetCache.

Inside StyleEngine::createSheet() you check the textToSheetCache to to find a
cached entry, if there isn't one you add one to both maps. The CSSStyleSheet is
the thing keep sheet contents alive though, not the cache. That removes all this
sweeping logic as well.

https://codereview.chromium.org/28553005/diff/180011/Source/core/dom/StyleEle...
File Source/core/dom/StyleElement.cpp (right):

https://codereview.chromium.org/28553005/diff/180011/Source/core/dom/StyleEle...
Source/core/dom/StyleElement.cpp:38: static const unsigned
cacheableTextContentSizeLimit = (unsigned)-1; // UNLIMITED
Not used, remove this constant.

https://codereview.chromium.org/28553005/diff/180011/Source/core/dom/StyleEle...
Source/core/dom/StyleElement.cpp:123:
m_sheet->ownerDocument()->styleEngine()->styleSheetContentsCache().remove(m_sheetText);
I don't think you should need this.

https://codereview.chromium.org/28553005/diff/180011/Source/core/dom/StyleEle...
Source/core/dom/StyleElement.cpp:125: m_sheetText = AtomicString();
You shouldn't need to store the string like this, the cache can be handled by
hooks inside StyleSheetContents

https://codereview.chromium.org/28553005/diff/180011/Source/core/dom/StyleEle...
Source/core/dom/StyleElement.cpp:155: if (StyleSheetContents* contents =
document.styleEngine()->styleSheetContentsCache().find(sheetText)) {
You should just always go through the cache and encapsulate the logic in there.

m_sheet = document.styleEngine().createSheet(text);

Internally it should do the hash table lookup or parse if needed.

https://codereview.chromium.org/28553005/diff/180011/Source/core/dom/StyleEle...
Source/core/dom/StyleElement.cpp:159: m_sheet =
CSSStyleSheet::createInline(contents->copy(), e, startPosition);
Why are you creating a copy? That's bad.

https://codereview.chromium.org/28553005/diff/180011/Source/core/dom/StyleEle...
Source/core/dom/StyleElement.cpp:177: }
Remove this, it should all be handled inside StyleEngine:createSheet(text)

[tasak, 2013-11-06 10:18:22]

Thank you for reviewing.

I would like to confirm one thing. We should share the same StyleSheetContents
with multiple StyleElements?

If so, firstly we need to refactor CSSStyleSheet, StyleSheetContents,
StyleRuleImport, HTMLLinkElement, LinkStyle and so on.

Because those codes assume StyleSheetContents has only 1 owner node.

https://codereview.chromium.org/28553005/diff/180011/Source/core/dom/StyleEle...
File Source/core/dom/StyleElement.cpp (right):

https://codereview.chromium.org/28553005/diff/180011/Source/core/dom/StyleEle...
Source/core/dom/StyleElement.cpp:159: m_sheet =
CSSStyleSheet::createInline(contents->copy(), e, startPosition);
On 2013/10/30 21:33:50, esprehn wrote:
> Why are you creating a copy? That's bad.

If we don't copy, 1 stylesheetcontents has multiple owner nodes.

So in StyleSheetContents::checkLoaded(), we cannot know what style element needs
"sheetLoaded".

https://codereview.chromium.org/28553005/diff/180011/Source/core/dom/StyleEle...
Source/core/dom/StyleElement.cpp:177: }
On 2013/10/30 21:33:50, esprehn wrote:
> Remove this, it should all be handled inside StyleEngine:createSheet(text)

However, we only knows whether a given stylesheetcontents is cacheable or not
after checkLoaded().

I think, it would be better to invoke the checkLoaded after updating
StyleElement::m_sheet and StyleElement::m_loading.

[ojan, 2013-11-06 23:35:57]

On 2013/11/06 10:18:22, tasak wrote:
> Thank you for reviewing.
> 
> I would like to confirm one thing. We should share the same StyleSheetContents
> with multiple StyleElements?
> 
> If so, firstly we need to refactor CSSStyleSheet, StyleSheetContents,
> StyleRuleImport, HTMLLinkElement, LinkStyle and so on.
> 
> Because those codes assume StyleSheetContents has only 1 owner node.
> 
>
https://codereview.chromium.org/28553005/diff/180011/Source/core/dom/StyleEle...
> File Source/core/dom/StyleElement.cpp (right):
> 
>
https://codereview.chromium.org/28553005/diff/180011/Source/core/dom/StyleEle...
> Source/core/dom/StyleElement.cpp:159: m_sheet =
> CSSStyleSheet::createInline(contents->copy(), e, startPosition);
> On 2013/10/30 21:33:50, esprehn wrote:
> > Why are you creating a copy? That's bad.
> 
> If we don't copy, 1 stylesheetcontents has multiple owner nodes.
> 
> So in StyleSheetContents::checkLoaded(), we cannot know what style element
needs
> "sheetLoaded".

We only call checkLoaded when we've just created a StyleSheetContents, but after
that point it can have multiple owner nodes. Take a look at
LinkStyle::setCSSStyleSheet. The thing to do is to take that snippet of code and
move it into StyleEngine::createSheet.

[tasak, 2013-11-07 11:01:05]

On 2013/11/06 23:35:57, ojan wrote:

> We only call checkLoaded when we've just created a StyleSheetContents, but
after
> that point it can have multiple owner nodes. Take a look at
> LinkStyle::setCSSStyleSheet. The thing to do is to take that snippet of code
and
> move it into StyleEngine::createSheet.

My concern is that we have more than two owner nodes when reusing the same
StyleSheetContents.
Some of the owner nodes expect "sheetLoaded", but others expect "not
sheetLoaded"(because sheetLoaded has been already invoked).

So I added a new method:checkLoadedFor and modified checkLoaded to support
multiple clients.

[tasak, 2013-11-13 11:21:19]

Would you review this CL?

[esprehn, 2013-11-15 10:27:37]

This is looking much better. You should not need a clearSheetCache() call, and
doing so is bad anyway since it clears a global cache across all documents. The
normal lifetime of the sheets should manage clearing the cache for you.

https://codereview.chromium.org/28553005/diff/390001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/28553005/diff/390001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:106: m_isMutable = true;
You can't make yourself mutable like this, you have to copy on write.

https://codereview.chromium.org/28553005/diff/390001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:391: bool loadCompleted =
ownerNode->sheetLoaded();
How could the sheet have loaded for one node but not the other? This doesn't
seem possible.

https://codereview.chromium.org/28553005/diff/390001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/28553005/diff/390001/Source/core/dom/Document...
Source/core/dom/Document.cpp:499: StyleEngine::clearSheetCache();
You shouldn't need to clear the sheet cache like this, the when the owner nodes
are freed they should drop their refs and the result should clear the cache.

https://codereview.chromium.org/28553005/diff/390001/Source/core/dom/Document...
Source/core/dom/Document.cpp:632: StyleEngine::clearSheetCache();
Ditto.

https://codereview.chromium.org/28553005/diff/390001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/28553005/diff/390001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:58: typedef HashMap<AtomicString,
StyleSheetContents*> TextToSheetCache;
No need for the typedefs since they're inside functions.

https://codereview.chromium.org/28553005/diff/390001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:65: typedef HashMap<StyleSheetContents*,
AtomicString> SheetToTextCache;
Ditto.

https://codereview.chromium.org/28553005/diff/390001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:475: sheetToTextCache().clear();
Should not be needed. This is bad too, you're clearing the cache for _all_
documents not just this one so unloading an iframe would nuke the cache across
the whole process.

[adamk, 2013-12-03 18:56:39]

What's the status of this patch? I very much look forward to it landing.

[adamk, 2013-12-10 18:42:37]

On 2013/12/03 18:56:39, adamk wrote:
> What's the status of this patch? I very much look forward to it landing.

Pinging again as it's been a week since my last ping.

[adamk, 2014-01-09 01:11:23]

On 2013/12/10 18:42:37, adamk wrote:
> On 2013/12/03 18:56:39, adamk wrote:
> > What's the status of this patch? I very much look forward to it landing.
> 
> Pinging again as it's been a week since my last ping.

And once more for the new year. Is this still being worked on? As far as I know
this is still pretty important for avoiding the overhead of <style> in custom
element shadow DOM.

[tasak, 2014-01-09 02:03:25]

On 2014/01/09 01:11:23, adamk wrote:
> On 2013/12/10 18:42:37, adamk wrote:
> > On 2013/12/03 18:56:39, adamk wrote:
> > > What's the status of this patch? I very much look forward to it landing.
> > 
> > Pinging again as it's been a week since my last ping.
> 
> And once more for the new year. Is this still being worked on? As far as I
know
> this is still pretty important for avoiding the overhead of <style> in custom
> element shadow DOM.

I'm sorry. I'm waiting until RuleSet is moved to StyleSheetContents (and now
CSSStyleSheet).

This patch will conflict with the refactoring and will cause many regressions.

Now I will try to fix this issue again.

[tasak, 2014-01-09 09:24:49]

Thank you for reviewing.

And I'm sorry, adamk. I will focus on this CL.

https://codereview.chromium.org/28553005/diff/390001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/28553005/diff/390001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:106: m_isMutable = true;
On 2013/11/15 10:27:37, esprehn wrote:
> You can't make yourself mutable like this, you have to copy on write.

I see. I moved the logic to CSSStyleSheet::willMutateRules.

https://codereview.chromium.org/28553005/diff/390001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:391: bool loadCompleted =
ownerNode->sheetLoaded();
On 2013/11/15 10:27:37, esprehn wrote:
> How could the sheet have loaded for one node but not the other? This doesn't
> seem possible.

I see. I moved loadCompleted to CSSStyleSheet.
If CSSStyleSheet (client) is not loadCompleted, invoke sheetLoaded.

So I also changed StyleSheetContents::loadCompleted(). The method now checks all
clients.

https://codereview.chromium.org/28553005/diff/390001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/28553005/diff/390001/Source/core/dom/Document...
Source/core/dom/Document.cpp:499: StyleEngine::clearSheetCache();
On 2013/11/15 10:27:37, esprehn wrote:
> You shouldn't need to clear the sheet cache like this, the when the owner
nodes
> are freed they should drop their refs and the result should clear the cache.

Done.

https://codereview.chromium.org/28553005/diff/390001/Source/core/dom/Document...
Source/core/dom/Document.cpp:632: StyleEngine::clearSheetCache();
On 2013/11/15 10:27:37, esprehn wrote:
> Ditto.

I think, quirks document cannot share stylesheets with non-quirks document,
because CSSGrammar.y and BinsonCSSParser-in.cpp check whether parserContext mode
is quirksMode or not while parsing styles.

i.e. isQuirksModeBehavior(), inQuirksMode() and so on.

https://codereview.chromium.org/28553005/diff/390001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/28553005/diff/390001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:58: typedef HashMap<AtomicString,
StyleSheetContents*> TextToSheetCache;
On 2013/11/15 10:27:37, esprehn wrote:
> No need for the typedefs since they're inside functions.

I tried "DEFINE_STATIC_LOCAL((HashMap<...>), ..)". So I saw:
../../third_party/WebKit/Source/core/dom/StyleEngine.cpp:58:5: error: expected
primary-expression before 'static'
../../third_party/WebKit/Source/core/dom/StyleEngine.cpp:58:5: error: expected
';' before 'static'

Looking at other codes, they use typedef.

https://codereview.chromium.org/28553005/diff/390001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:475: sheetToTextCache().clear();
On 2013/11/15 10:27:37, esprehn wrote:
> Should not be needed. This is bad too, you're clearing the cache for _all_
> documents not just this one so unloading an iframe would nuke the cache across
> the whole process.

I'm still using this method when compatibility mode is changed.

[esprehn, 2014-01-09 10:05:32]

Looking much better.

https://codereview.chromium.org/28553005/diff/570001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/28553005/diff/570001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:401: // Because sheetLoaded() might
destroy CSSStyleSheet.
How does this happen?

https://codereview.chromium.org/28553005/diff/570001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/28553005/diff/570001/Source/core/dom/Document...
Source/core/dom/Document.cpp:628: StyleEngine::clearSheetCache();
You still can't do this, loading any quirks mode document would blow away the
cache for all the non-quirks ones. I'd suggest either having separate caches for
quirks vs non quirks or making the sharing standards mode only for now. I'd
start with standards mode only for now.

https://codereview.chromium.org/28553005/diff/570001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/28553005/diff/570001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:640: void StyleEngine::clearSheetCache()
This method can't exist.

[tasak, 2014-01-09 11:59:04]

Thank you for reviewing.

I found the following ASSERTION failure:
01:44:41.258 19602 worker/3 fast/dom/StyleSheet/gc-declaration-parent-rule.html
crashed, (stderr lines):
01:44:41.258 19602   ASSERTION FAILED: isCacheable()
01:44:41.258 19602  
../../third_party/WebKit/Source/core/css/StyleSheetContents.cpp(534) : void
WebCore::StyleSheetContents::removedFromMemoryCache()
01:44:41.258 19602   1   0x7f33d942321c

I fixed StyleSheetContents::loadComplete()'s mistake.
So I will check whether the new patch still has the same problem or not.

https://codereview.chromium.org/28553005/diff/570001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/28553005/diff/570001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:401: // Because sheetLoaded() might
destroy CSSStyleSheet.
On 2014/01/09 10:05:32, esprehn wrote:
> How does this happen?

This is the same reason above (i.e. "RefPtr<StyleSheetContents> protect(this)"'s
comment).

So I copied the same comment and modified to use "RefPtr<CSSStyleSheet>
protectClient(clients[i]);".

https://codereview.chromium.org/28553005/diff/570001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/28553005/diff/570001/Source/core/dom/Document...
Source/core/dom/Document.cpp:628: StyleEngine::clearSheetCache();
On 2014/01/09 10:05:32, esprehn wrote:
> You still can't do this, loading any quirks mode document would blow away the
> cache for all the non-quirks ones. I'd suggest either having separate caches
for
> quirks vs non quirks or making the sharing standards mode only for now. I'd
> start with standards mode only for now.

I see. I agree that it is much better to split cache.
Done.

https://codereview.chromium.org/28553005/diff/570001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/28553005/diff/570001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:640: void StyleEngine::clearSheetCache()
On 2014/01/09 10:05:32, esprehn wrote:
> This method can't exist.

Done.

[tasak, 2014-01-10 18:38:54]

On 2014/01/09 11:59:04, tasak wrote:

> I found the following ASSERTION failure:
> 01:44:41.258 19602 worker/3
fast/dom/StyleSheet/gc-declaration-parent-rule.html
> crashed, (stderr lines):
> 01:44:41.258 19602   ASSERTION FAILED: isCacheable()
> 01:44:41.258 19602  
> ../../third_party/WebKit/Source/core/css/StyleSheetContents.cpp(534) : void
> WebCore::StyleSheetContents::removedFromMemoryCache()
> 01:44:41.258 19602   1   0x7f33d942321c

The patch still causes the above ASSERTION failure.

So I'm thinking of the following:

(a) use ASSERT(mayCacheable()), or

(b) change loadCompleted(). It should return true if any client is
loadCompleted.

[tasak, 2014-01-11 02:04:33]

I found what causes the assertion failure.

void LinkStyle::setCSSStyleSheet() invokes LinkStyle::sheetLoaded() directly.
(not via StyleSheetContents::checkLoaded()).

So CSSStyleSheet's loadCompleted is not updated. ASSERT(!isCacheable()) fails.

I need to update the method in the following way:

(around line 399)
    if (RefPtr<StyleSheetContents> restoredSheet =
const_cast<CSSStyleSheetResource*>(cachedStyleSheet)->restoreParsedStyleSheet(parserContext))
{
...
        ASSERT(restoredSheet->isCacheable());
        ASSERT(!restoredSheet->isLoading());
        restoredSheet->checkLoaded();
        //sheetLoaded();                                                        
        //notifyLoadedSheetAndAllCriticalSubresources(false);  

or
        // Add setLoadCompleted to CSSStyleSheet.
        m_sheet->setLoadCompleted();

[tasak, 2014-01-14 04:06:31]

Would you review this CL?

I think, I fixed ASSERT(isCacheable) FAILURE.

[esprehn, 2014-01-14 05:09:42]

LGTM, can you expand the description to explain what you did?

https://codereview.chromium.org/28553005/diff/810001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/28553005/diff/810001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:365: return parentSheet->loadCompleted();
So only the root ever has clients?

[tasak, 2014-01-14 08:03:54]

Thank you for reviewing.

Sure.
I have just updated the description.

https://codereview.chromium.org/28553005/diff/810001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/28553005/diff/810001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:365: return parentSheet->loadCompleted();
On 2014/01/14 05:09:43, esprehn wrote:
> So only the root ever has clients?

Yes, I think so.
Because singleOwnerNode() always checks rootStyleSheet()'s clients.

[commit-bot: I haz the power, 2014-01-15 07:53:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tasak@google.com/28553005/830007

[commit-bot: I haz the power, 2014-01-15 15:57:49]

Change committed as 165139