[Oilpan]: Move CSSFontSelectorClient to the oilpan heap using transition types.

[Oilpan]: Move CSSFontSelectorClient to the oilpan heap using transition types.

@jochen or tkent: Could you take a look at the wtf change. I needed to change the trait's Pass[In|Out]Type and Peek[In|Out]Type to make it compile on both gcc and clang for both oilpan and non-oilpan.

Also change the StyleResolver's ViewportStyleResolver to an OwnPtrWillBeMember and removed the call to the clearDocument method.
Added DISALLOW_ALLOCATION to TreeBoundaryCrossingRules since it is only allocated as a part object and traced it from StyleResolver.
Had to change CanvasRenderingContext2D's vector of State to a Vector of pointers to State since we don't yet support vectors of objects with vtables.

R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, oilpan-reviews@chromium.org, tkent@chromium.org, vegorov@chromium.org, zerny@chromium.org
BUG=341815
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170040

[haraken, 2014-03-25 13:19:30]

https://codereview.chromium.org/211073002/diff/1/Source/core/css/CSSFontSelec...
File Source/core/css/CSSFontSelector.cpp (right):

https://codereview.chromium.org/211073002/diff/1/Source/core/css/CSSFontSelec...
Source/core/css/CSSFontSelector.cpp:132: void
CSSFontSelector::unregisterForInvalidationCallbacks(CSSFontSelectorClient*
client)

I'd add #if !ENABLE_OILPAN to this method to make sure that it's not called in
the oilpan build.

https://codereview.chromium.org/211073002/diff/1/Source/core/css/resolver/Vie...
File Source/core/css/resolver/ViewportStyleResolver.h (left):

https://codereview.chromium.org/211073002/diff/1/Source/core/css/resolver/Vie...
Source/core/css/resolver/ViewportStyleResolver.h:57: void clearDocument();

Removing clearDocument() looks wrong to me. When a document is cleared,
ViewportStyleResolver::m_document should be cleared.

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

https://codereview.chromium.org/211073002/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:131: if (!m_pushedStyleResolver)

Is it safe to touch m_pushedStyleResolver in a destructor of stack-allocated
objects?

https://codereview.chromium.org/211073002/diff/1/Source/core/html/canvas/Canv...
File Source/core/html/canvas/CanvasRenderingContext2D.h (right):

https://codereview.chromium.org/211073002/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasRenderingContext2D.h:237: struct State FINAL :
CSSFontSelectorClient {

Nit: I would change this struct to a class. It looks weird that a struct
inherits from a class.

[wibling-chromium, 2014-03-25 13:27:16]

Thanks for the review!

https://codereview.chromium.org/211073002/diff/1/Source/core/css/CSSFontSelec...
File Source/core/css/CSSFontSelector.cpp (right):

https://codereview.chromium.org/211073002/diff/1/Source/core/css/CSSFontSelec...
Source/core/css/CSSFontSelector.cpp:132: void
CSSFontSelector::unregisterForInvalidationCallbacks(CSSFontSelectorClient*
client)
On 2014/03/25 13:19:30, haraken wrote:
> 
> I'd add #if !ENABLE_OILPAN to this method to make sure that it's not called in
> the oilpan build.

Done.

https://codereview.chromium.org/211073002/diff/1/Source/core/css/resolver/Vie...
File Source/core/css/resolver/ViewportStyleResolver.h (left):

https://codereview.chromium.org/211073002/diff/1/Source/core/css/resolver/Vie...
Source/core/css/resolver/ViewportStyleResolver.h:57: void clearDocument();
On 2014/03/25 13:19:30, haraken wrote:
> 
> Removing clearDocument() looks wrong to me. When a document is cleared,
> ViewportStyleResolver::m_document should be cleared.

It wasn't cleared previously. The only caller of clearDocument was the
StyleResolver's destructor and since I changed the StyleResolver's
RefPtr<ViewportStyleResolver> to an OwnPtr<ViewportStyleResolver> it should not
be necessary to clear the document pointer since it is dead in the same cycle.

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

https://codereview.chromium.org/211073002/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:131: if (!m_pushedStyleResolver)
On 2014/03/25 13:19:30, haraken wrote:
> 
> Is it safe to touch m_pushedStyleResolver in a destructor of stack-allocated
> objects?

I would think so since the destructor will never be called by the garbage
collector, but only when the object goes out of scope. 
Even if there is a GC done as part of the destructor this stack allocated object
will never die as part of that GC and given that is has a member to the
StyleResolver the resolver will also not die and can be referenced.

https://codereview.chromium.org/211073002/diff/1/Source/core/html/canvas/Canv...
File Source/core/html/canvas/CanvasRenderingContext2D.h (right):

https://codereview.chromium.org/211073002/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasRenderingContext2D.h:237: struct State FINAL :
CSSFontSelectorClient {
On 2014/03/25 13:19:30, haraken wrote:
> 
> Nit: I would change this struct to a class. It looks weird that a struct
> inherits from a class.

Done.

[haraken, 2014-03-25 13:50:20]

https://codereview.chromium.org/211073002/diff/1/Source/core/css/resolver/Vie...
File Source/core/css/resolver/ViewportStyleResolver.h (left):

https://codereview.chromium.org/211073002/diff/1/Source/core/css/resolver/Vie...
Source/core/css/resolver/ViewportStyleResolver.h:57: void clearDocument();
On 2014/03/25 13:27:17, wibling-chromium wrote:
> On 2014/03/25 13:19:30, haraken wrote:
> > 
> > Removing clearDocument() looks wrong to me. When a document is cleared,
> > ViewportStyleResolver::m_document should be cleared.
> 
> It wasn't cleared previously. The only caller of clearDocument was the
> StyleResolver's destructor and since I changed the StyleResolver's
> RefPtr<ViewportStyleResolver> to an OwnPtr<ViewportStyleResolver> it should
not
> be necessary to clear the document pointer since it is dead in the same cycle.

Correct me if I'm wrong:
StyleResolver and ViewportStyleResolver are on the heap and die together.
Document is not on the heap. Thus it's possible that the ViewportStyleResolver
outlives the Document. Then it's possible that ViewportStyleResolver::m_document
becomes a stale pointer, which we should avoid.

[wibling-chromium, 2014-03-25 13:59:24]

https://codereview.chromium.org/211073002/diff/1/Source/core/css/resolver/Vie...
File Source/core/css/resolver/ViewportStyleResolver.h (left):

https://codereview.chromium.org/211073002/diff/1/Source/core/css/resolver/Vie...
Source/core/css/resolver/ViewportStyleResolver.h:57: void clearDocument();
On 2014/03/25 13:50:21, haraken wrote:
> On 2014/03/25 13:27:17, wibling-chromium wrote:
> > On 2014/03/25 13:19:30, haraken wrote:
> > > 
> > > Removing clearDocument() looks wrong to me. When a document is cleared,
> > > ViewportStyleResolver::m_document should be cleared.
> > 
> > It wasn't cleared previously. The only caller of clearDocument was the
> > StyleResolver's destructor and since I changed the StyleResolver's
> > RefPtr<ViewportStyleResolver> to an OwnPtr<ViewportStyleResolver> it should
> not
> > be necessary to clear the document pointer since it is dead in the same
cycle.
> 
> Correct me if I'm wrong:
> StyleResolver and ViewportStyleResolver are on the heap and die together.
> Document is not on the heap. Thus it's possible that the ViewportStyleResolver
> outlives the Document. Then it's possible that
ViewportStyleResolver::m_document
> becomes a stale pointer, which we should avoid.

My point is that before my change what you said was also true since there was
nothing that prevented the Document to die before the ViewportStyleResolver
before my change. I was merely assuming that the code before was correct and the
person who wrote it knew the Document would live at least as long as the
ViewportStyleResolver since there was no call to clearDocument from Document
code.
Looking at the Document code a bit closer it seems there is an
OwnPtr<StyleEngine> which in turn has an OwnPtr<StyleResolver> which now has an
OwnPtr<ViewportStyleResolver> so from what I can tell the code is correct and
Document will not die before the ViewportStyleResolver.

[haraken, 2014-03-25 14:16:36]

On 2014/03/25 13:59:24, wibling-chromium wrote:
>
https://codereview.chromium.org/211073002/diff/1/Source/core/css/resolver/Vie...
> File Source/core/css/resolver/ViewportStyleResolver.h (left):
> 
>
https://codereview.chromium.org/211073002/diff/1/Source/core/css/resolver/Vie...
> Source/core/css/resolver/ViewportStyleResolver.h:57: void clearDocument();
> On 2014/03/25 13:50:21, haraken wrote:
> > On 2014/03/25 13:27:17, wibling-chromium wrote:
> > > On 2014/03/25 13:19:30, haraken wrote:
> > > > 
> > > > Removing clearDocument() looks wrong to me. When a document is cleared,
> > > > ViewportStyleResolver::m_document should be cleared.
> > > 
> > > It wasn't cleared previously. The only caller of clearDocument was the
> > > StyleResolver's destructor and since I changed the StyleResolver's
> > > RefPtr<ViewportStyleResolver> to an OwnPtr<ViewportStyleResolver> it
should
> > not
> > > be necessary to clear the document pointer since it is dead in the same
> cycle.
> > 
> > Correct me if I'm wrong:
> > StyleResolver and ViewportStyleResolver are on the heap and die together.
> > Document is not on the heap. Thus it's possible that the
ViewportStyleResolver
> > outlives the Document. Then it's possible that
> ViewportStyleResolver::m_document
> > becomes a stale pointer, which we should avoid.
> 
> My point is that before my change what you said was also true since there was
> nothing that prevented the Document to die before the ViewportStyleResolver
> before my change. I was merely assuming that the code before was correct and
the
> person who wrote it knew the Document would live at least as long as the
> ViewportStyleResolver since there was no call to clearDocument from Document
> code.
> Looking at the Document code a bit closer it seems there is an
> OwnPtr<StyleEngine> which in turn has an OwnPtr<StyleResolver> which now has
an
> OwnPtr<ViewportStyleResolver> so from what I can tell the code is correct and
> Document will not die before the ViewportStyleResolver.

I agree that you're not introducing any new pitfalls, but I don't fully
understand why the code is correct.

StyleResolver and ViewportStyleResolver don't die at the point where all the
references to them are gone. They die in the next GC after all the references to
them are gone. On the other hand, Document dies once all the reference to the
Document is gone. Thus in theory it's possible that StyleResolver and
ViewportStyleResolver outlive the Document. Am I missing something?

[wibling-chromium, 2014-03-25 14:35:31]

On 2014/03/25 14:16:36, haraken wrote:
> On 2014/03/25 13:59:24, wibling-chromium wrote:
> >
>
https://codereview.chromium.org/211073002/diff/1/Source/core/css/resolver/Vie...
> > File Source/core/css/resolver/ViewportStyleResolver.h (left):
> > 
> >
>
https://codereview.chromium.org/211073002/diff/1/Source/core/css/resolver/Vie...
> > Source/core/css/resolver/ViewportStyleResolver.h:57: void clearDocument();
> > On 2014/03/25 13:50:21, haraken wrote:
> > > On 2014/03/25 13:27:17, wibling-chromium wrote:
> > > > On 2014/03/25 13:19:30, haraken wrote:
> > > > > 
> > > > > Removing clearDocument() looks wrong to me. When a document is
cleared,
> > > > > ViewportStyleResolver::m_document should be cleared.
> > > > 
> > > > It wasn't cleared previously. The only caller of clearDocument was the
> > > > StyleResolver's destructor and since I changed the StyleResolver's
> > > > RefPtr<ViewportStyleResolver> to an OwnPtr<ViewportStyleResolver> it
> should
> > > not
> > > > be necessary to clear the document pointer since it is dead in the same
> > cycle.
> > > 
> > > Correct me if I'm wrong:
> > > StyleResolver and ViewportStyleResolver are on the heap and die together.
> > > Document is not on the heap. Thus it's possible that the
> ViewportStyleResolver
> > > outlives the Document. Then it's possible that
> > ViewportStyleResolver::m_document
> > > becomes a stale pointer, which we should avoid.
> > 
> > My point is that before my change what you said was also true since there
was
> > nothing that prevented the Document to die before the ViewportStyleResolver
> > before my change. I was merely assuming that the code before was correct and
> the
> > person who wrote it knew the Document would live at least as long as the
> > ViewportStyleResolver since there was no call to clearDocument from Document
> > code.
> > Looking at the Document code a bit closer it seems there is an
> > OwnPtr<StyleEngine> which in turn has an OwnPtr<StyleResolver> which now has
> an
> > OwnPtr<ViewportStyleResolver> so from what I can tell the code is correct
and
> > Document will not die before the ViewportStyleResolver.
> 
> I agree that you're not introducing any new pitfalls, but I don't fully
> understand why the code is correct.
> 
> StyleResolver and ViewportStyleResolver don't die at the point where all the
> references to them are gone. They die in the next GC after all the references
to
> them are gone. On the other hand, Document dies once all the reference to the
> Document is gone. Thus in theory it's possible that StyleResolver and
> ViewportStyleResolver outlive the Document. Am I missing something?

From what I can tell the code in the non-oilpan version is correct given the
above mentioned OwnPtr chain which means the Document dying will cause the
ViewportStyleResolver to die.

In the non-oilpan version I am assuming that if all references to StyleResolver
and ViewportStyleResolver are gone then the only code that could touch the
m_document field would be the destructor of ViewportStyleResolver since it is
the only code that should get executed after the references are gone, right?
Since ViewportStyleDestructor has an empty destructor it should be okay.

One thing that I just realized writing this is that there could be a stack
pointer to a StyleResolver keeping it alive after the document dies, however I
don't think that is the case since then the non-oilpan version would also risk
accessing a destructed object as it will not keep anything alive as the
accessors are passing our raw pointers and not PassRefPtrs.

[haraken, 2014-03-25 15:30:42]

On 2014/03/25 14:35:31, wibling-chromium wrote:
> On 2014/03/25 14:16:36, haraken wrote:
> > On 2014/03/25 13:59:24, wibling-chromium wrote:
> > >
> >
>
https://codereview.chromium.org/211073002/diff/1/Source/core/css/resolver/Vie...
> > > File Source/core/css/resolver/ViewportStyleResolver.h (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/211073002/diff/1/Source/core/css/resolver/Vie...
> > > Source/core/css/resolver/ViewportStyleResolver.h:57: void clearDocument();
> > > On 2014/03/25 13:50:21, haraken wrote:
> > > > On 2014/03/25 13:27:17, wibling-chromium wrote:
> > > > > On 2014/03/25 13:19:30, haraken wrote:
> > > > > > 
> > > > > > Removing clearDocument() looks wrong to me. When a document is
> cleared,
> > > > > > ViewportStyleResolver::m_document should be cleared.
> > > > > 
> > > > > It wasn't cleared previously. The only caller of clearDocument was the
> > > > > StyleResolver's destructor and since I changed the StyleResolver's
> > > > > RefPtr<ViewportStyleResolver> to an OwnPtr<ViewportStyleResolver> it
> > should
> > > > not
> > > > > be necessary to clear the document pointer since it is dead in the
same
> > > cycle.
> > > > 
> > > > Correct me if I'm wrong:
> > > > StyleResolver and ViewportStyleResolver are on the heap and die
together.
> > > > Document is not on the heap. Thus it's possible that the
> > ViewportStyleResolver
> > > > outlives the Document. Then it's possible that
> > > ViewportStyleResolver::m_document
> > > > becomes a stale pointer, which we should avoid.
> > > 
> > > My point is that before my change what you said was also true since there
> was
> > > nothing that prevented the Document to die before the
ViewportStyleResolver
> > > before my change. I was merely assuming that the code before was correct
and
> > the
> > > person who wrote it knew the Document would live at least as long as the
> > > ViewportStyleResolver since there was no call to clearDocument from
Document
> > > code.
> > > Looking at the Document code a bit closer it seems there is an
> > > OwnPtr<StyleEngine> which in turn has an OwnPtr<StyleResolver> which now
has
> > an
> > > OwnPtr<ViewportStyleResolver> so from what I can tell the code is correct
> and
> > > Document will not die before the ViewportStyleResolver.
> > 
> > I agree that you're not introducing any new pitfalls, but I don't fully
> > understand why the code is correct.
> > 
> > StyleResolver and ViewportStyleResolver don't die at the point where all the
> > references to them are gone. They die in the next GC after all the
references
> to
> > them are gone. On the other hand, Document dies once all the reference to
the
> > Document is gone. Thus in theory it's possible that StyleResolver and
> > ViewportStyleResolver outlive the Document. Am I missing something?
> 
> From what I can tell the code in the non-oilpan version is correct given the
> above mentioned OwnPtr chain which means the Document dying will cause the
> ViewportStyleResolver to die.
> 
> In the non-oilpan version I am assuming that if all references to
StyleResolver
> and ViewportStyleResolver are gone then the only code that could touch the
> m_document field would be the destructor of ViewportStyleResolver since it is
> the only code that should get executed after the references are gone, right?
> Since ViewportStyleDestructor has an empty destructor it should be okay.

Sorry about the iterative comments. What I'm concerned about is not the
non-oilpan version but the oilpan version. In the oilpan version, as I mentioned
above, ViewportStyleDestructor can outlive Document. So I'm guessing that we'll
need to add code to clear ViewportStyleDestructor::m_document when the Document
dies.

[wibling-chromium, 2014-03-25 15:54:32]

On 2014/03/25 15:30:42, haraken wrote:
> On 2014/03/25 14:35:31, wibling-chromium wrote:
> > On 2014/03/25 14:16:36, haraken wrote:
> > > On 2014/03/25 13:59:24, wibling-chromium wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/211073002/diff/1/Source/core/css/resolver/Vie...
> > > > File Source/core/css/resolver/ViewportStyleResolver.h (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/211073002/diff/1/Source/core/css/resolver/Vie...
> > > > Source/core/css/resolver/ViewportStyleResolver.h:57: void
clearDocument();
> > > > On 2014/03/25 13:50:21, haraken wrote:
> > > > > On 2014/03/25 13:27:17, wibling-chromium wrote:
> > > > > > On 2014/03/25 13:19:30, haraken wrote:
> > > > > > > 
> > > > > > > Removing clearDocument() looks wrong to me. When a document is
> > cleared,
> > > > > > > ViewportStyleResolver::m_document should be cleared.
> > > > > > 
> > > > > > It wasn't cleared previously. The only caller of clearDocument was
the
> > > > > > StyleResolver's destructor and since I changed the StyleResolver's
> > > > > > RefPtr<ViewportStyleResolver> to an OwnPtr<ViewportStyleResolver> it
> > > should
> > > > > not
> > > > > > be necessary to clear the document pointer since it is dead in the
> same
> > > > cycle.
> > > > > 
> > > > > Correct me if I'm wrong:
> > > > > StyleResolver and ViewportStyleResolver are on the heap and die
> together.
> > > > > Document is not on the heap. Thus it's possible that the
> > > ViewportStyleResolver
> > > > > outlives the Document. Then it's possible that
> > > > ViewportStyleResolver::m_document
> > > > > becomes a stale pointer, which we should avoid.
> > > > 
> > > > My point is that before my change what you said was also true since
there
> > was
> > > > nothing that prevented the Document to die before the
> ViewportStyleResolver
> > > > before my change. I was merely assuming that the code before was correct
> and
> > > the
> > > > person who wrote it knew the Document would live at least as long as the
> > > > ViewportStyleResolver since there was no call to clearDocument from
> Document
> > > > code.
> > > > Looking at the Document code a bit closer it seems there is an
> > > > OwnPtr<StyleEngine> which in turn has an OwnPtr<StyleResolver> which now
> has
> > > an
> > > > OwnPtr<ViewportStyleResolver> so from what I can tell the code is
correct
> > and
> > > > Document will not die before the ViewportStyleResolver.
> > > 
> > > I agree that you're not introducing any new pitfalls, but I don't fully
> > > understand why the code is correct.
> > > 
> > > StyleResolver and ViewportStyleResolver don't die at the point where all
the
> > > references to them are gone. They die in the next GC after all the
> references
> > to
> > > them are gone. On the other hand, Document dies once all the reference to
> the
> > > Document is gone. Thus in theory it's possible that StyleResolver and
> > > ViewportStyleResolver outlive the Document. Am I missing something?
> > 
> > From what I can tell the code in the non-oilpan version is correct given the
> > above mentioned OwnPtr chain which means the Document dying will cause the
> > ViewportStyleResolver to die.
> > 
> > In the non-oilpan version I am assuming that if all references to
> StyleResolver
> > and ViewportStyleResolver are gone then the only code that could touch the
> > m_document field would be the destructor of ViewportStyleResolver since it
is
> > the only code that should get executed after the references are gone, right?
> > Since ViewportStyleDestructor has an empty destructor it should be okay.
> 
> Sorry about the iterative comments. What I'm concerned about is not the
> non-oilpan version but the oilpan version. In the oilpan version, as I
mentioned
> above, ViewportStyleDestructor can outlive Document. So I'm guessing that
we'll
> need to add code to clear ViewportStyleDestructor::m_document when the
Document
> dies.

I miswrote in my previous reply. 

The comment 

> > In the non-oilpan version I am assuming that if all references to
> StyleResolver
> > and ViewportStyleResolver are gone then the only code that could touch the
> > m_document field would be the destructor of ViewportStyleResolver since it
is
> > the only code that should get executed after the references are gone, right?
> > Since ViewportStyleDestructor has an empty destructor it should be okay.

actually refers to the oilpan version.

In the oilpan version the structure is

Document: Member<StyleEngine> -> StyleEngine: Member<StyleResolver> ->
StyleResolver: Member<ViewportStyleResolver>

We also know that there are no other members since this used to be OwnPtrs. When
Document dies the StyleEngine is no longer referred from any other objects since
Document had the only member to it. This is transitively true all the way to
ViewportStyleResolver. The only other thing that could keep any of the
StyleEngine, StyleResolver, ViewportStyleResolver objects alive are any pointers
we have on the stack. These pointers could have been obtained via the object's
getter methods.
However if we had any such pointers on the stack then we would have risked
accessing destructed objects in the non-oilpan version since there are no
getters which return a PassRefPtr. All getters return a raw pointer (T*). 

We could add a clearDocument call in the Document's destructor, but since it was
not needed before I am arguing it is not needed now. We are at least as good in
the oilpan version since we guarantee you will not access the StyleEngine,
StyleResolver, or ViewportStyleResolver objects after they are destructed. We
might keep them alive a bit longer, but given that existing code is not crashing
with access to destructed objects there should be no calls to them and hence no
accesses to ViewportStyleResolver's m_document.

When document is moved to the heap this will be even better since we can change
the Document* to a Member<Document> and they will all die together.


However since the document is dead there should be no references to the
StyleEngine
reference variables we have to the StyleEngine -> StyleResolver ->
ViewportStyleResolver

[wibling-chromium, 2014-03-25 15:56:57]

Please disregard the below comment. It was a leftover I forgot to remove:)

> However since the document is dead there should be no references to the
> StyleEngine
> reference variables we have to the StyleEngine -> StyleResolver ->
> ViewportStyleResolver

[haraken, 2014-03-25 16:15:55]

> We could add a clearDocument call in the Document's destructor, but since it
was
> not needed before I am arguing it is not needed now. We are at least as good
in
> the oilpan version since we guarantee you will not access the StyleEngine,
> StyleResolver, or ViewportStyleResolver objects after they are destructed. We
> might keep them alive a bit longer, but given that existing code is not
crashing
> with access to destructed objects there should be no calls to them and hence
no
> accesses to ViewportStyleResolver's m_document.

Thanks for the detailed clarification. Makes sense.

> When document is moved to the heap this will be even better since we can
change
> the Document* to a Member<Document> and they will all die together.

Yeah, moving Document to the heap will resolve the issue.

LGTM. But this CL is a bit complex, so I might want to have one more oilpan
reviewer.

[Mads Ager (chromium), 2014-03-25 19:21:20]

LGTM

https://codereview.chromium.org/211073002/diff/170001/Source/core/dom/StyleEn...
File Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/211073002/diff/170001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.h:250: WillBeHeapHashMap<AtomicString,
RawPtrWillBeMember<StyleSheetContents> > m_textToSheetCache;
Thanks! These were moved recently and never updated for Oilpan. These used to be
weak mappings, but that is not the case any more which is nice! :)

[wibling-chromium, 2014-03-26 07:40:48]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-03-26 07:40:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wibling@chromium.org/211073002/170001

[commit-bot: I haz the power, 2014-03-26 08:05:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-26 08:05:41]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[wibling-chromium, 2014-03-26 08:13:55]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-03-26 08:14:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wibling@chromium.org/211073002/170001

[wibling-chromium, 2014-03-26 08:49:58]

The CQ bit was unchecked by wibling@chromium.org

[wibling-chromium, 2014-03-26 09:35:39]

On 2014/03/26 09:30:01, wibling-chromium wrote:

jochen@, tkent@: could I get you to take a look at the wtf change?

[jochen (gone - plz use gerrit), 2014-03-26 09:36:27]

wtf lgtm

[wibling-chromium, 2014-03-26 09:37:05]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-03-26 09:37:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wibling@chromium.org/211073002/320001

[wibling-chromium, 2014-03-26 09:37:18]

On 2014/03/26 09:36:27, jochen wrote:
> wtf lgtm

Thanks!

[wibling-chromium, 2014-03-26 09:39:10]

The CQ bit was unchecked by wibling@chromium.org

[wibling-chromium, 2014-03-26 09:40:48]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-03-26 09:40:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wibling@chromium.org/211073002/430001

[commit-bot: I haz the power, 2014-03-26 10:42:28]

Change committed as 170040

[zerny-chromium, 2014-03-26 14:57:49]

lgtm with a few clean-up comments

https://codereview.chromium.org/211073002/diff/430001/Source/core/css/resolve...
File Source/core/css/resolver/ViewportStyleResolver.cpp (right):

https://codereview.chromium.org/211073002/diff/430001/Source/core/css/resolve...
Source/core/css/resolver/ViewportStyleResolver.cpp:45:
DEFINE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED(ViewportStyleResolver);
Ditto

https://codereview.chromium.org/211073002/diff/430001/Source/core/css/resolve...
File Source/core/css/resolver/ViewportStyleResolver.h (right):

https://codereview.chromium.org/211073002/diff/430001/Source/core/css/resolve...
Source/core/css/resolver/ViewportStyleResolver.h:46:
DECLARE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED(ViewportStyleResolver);
This can be removed now that the RefCounted base is gone.

[wibling-chromium, 2014-03-26 15:21:34]

Thanks for the review. I will do the cleanup in a subsequent change.

https://codereview.chromium.org/211073002/diff/430001/Source/core/css/resolve...
File Source/core/css/resolver/ViewportStyleResolver.cpp (right):

https://codereview.chromium.org/211073002/diff/430001/Source/core/css/resolve...
Source/core/css/resolver/ViewportStyleResolver.cpp:45:
DEFINE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED(ViewportStyleResolver);
On 2014/03/26 14:57:49, zerny-chromium wrote:
> Ditto

Done.

https://codereview.chromium.org/211073002/diff/430001/Source/core/css/resolve...
File Source/core/css/resolver/ViewportStyleResolver.h (right):

https://codereview.chromium.org/211073002/diff/430001/Source/core/css/resolve...
Source/core/css/resolver/ViewportStyleResolver.h:46:
DECLARE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED(ViewportStyleResolver);
On 2014/03/26 14:57:49, zerny-chromium wrote:
> This can be removed now that the RefCounted base is gone.

Done.

[wibling-chromium, 2014-03-26 16:06:11]

On 2014/03/26 15:21:34, wibling-chromium wrote:
> Thanks for the review. I will do the cleanup in a subsequent change.
> 
>
https://codereview.chromium.org/211073002/diff/430001/Source/core/css/resolve...
> File Source/core/css/resolver/ViewportStyleResolver.cpp (right):
> 
>
https://codereview.chromium.org/211073002/diff/430001/Source/core/css/resolve...
> Source/core/css/resolver/ViewportStyleResolver.cpp:45:
> DEFINE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED(ViewportStyleResolver);
> On 2014/03/26 14:57:49, zerny-chromium wrote:
> > Ditto
> 
> Done.
> 
>
https://codereview.chromium.org/211073002/diff/430001/Source/core/css/resolve...
> File Source/core/css/resolver/ViewportStyleResolver.h (right):
> 
>
https://codereview.chromium.org/211073002/diff/430001/Source/core/css/resolve...
> Source/core/css/resolver/ViewportStyleResolver.h:46:
> DECLARE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED(ViewportStyleResolver);
> On 2014/03/26 14:57:49, zerny-chromium wrote:
> > This can be removed now that the RefCounted base is gone.
> 
> Done.

I tried removing the above, but it caused the below error. I will leave it
around for now.

...
In file included from ../../third_party/WebKit/Source/wtf/HashFunctions.h:24:
In file included from ../../third_party/WebKit/Source/wtf/RefPtr.h:28:
../../third_party/WebKit/Source/wtf/PassRefPtr.h:57:16: error: member access
into incomplete type 'WebCore::MutableStylePropertySet'
            ptr->deref();
               ^
../../third_party/WebKit/Source/wtf/RefPtr.h:54:35: note: in instantiation of
function template specialization
'WTF::derefIfNotNull<WebCore::MutableStylePropertySet>' requested here
        ALWAYS_INLINE ~RefPtr() { derefIfNotNull(m_ptr); }
                                  ^
../../third_party/WebKit/Source/core/css/resolver/ViewportStyleResolver.h:45:7:
note: in instantiation of member function
'WTF::RefPtr<WebCore::MutableStylePropertySet>::~RefPtr' requested here
class ViewportStyleResolver : public
NoBaseWillBeGarbageCollected<ViewportStyleResolver> {
      ^
../../third_party/WebKit/Source/wtf/PassOwnPtr.h:55:45: note: in instantiation
of member function
'WTF::OwnedPtrDeleter<WebCore::ViewportStyleResolver>::deletePtr' requested here
        ~PassOwnPtr() { OwnedPtrDeleter<T>::deletePtr(m_ptr); }
                                            ^
../../third_party/WebKit/Source/core/css/resolver/ViewportStyleResolver.h:50:16:
note: in instantiation of member function
'WTF::PassOwnPtr<WebCore::ViewportStyleResolver>::~PassOwnPtr' requested here
        return adoptPtrWillBeNoop(new ViewportStyleResolver(document));
               ^
../../third_party/WebKit/Source/core/dom/Element.h:56:7: note: forward
declaration of 'WebCore::MutableStylePropertySet'
class MutableStylePropertySet;
      ^
1 error generated.