Move PseudoElements storage to their own object.

Move PseudoElements storage to their own object.

BUG=479745
Committed: https://crrev.com/c7d5ebe0a8549633679ace6426a72434c0c8511f
Cr-Commit-Position: refs/heads/master@{#392026}

[ramya.v, 2016-05-04 07:11:51]

Patchset #1 (id:1) has been deleted

[ramya.v, 2016-05-04 07:12:25]

ramya.v@samsung.com changed reviewers:
+ timloh@chromium.org, tkent@chromium.org

[ramya.v, 2016-05-04 07:12:25]

PTAL!

Thanks

[Timothy Loh, 2016-05-05 06:39:33]

sof: It seems unfortunate that NodeRareData, and now PseudoElementData also, are
finalized even though the finalizers only exist for a couple of assertions,
since I assume this adds some overhead. Is there a simple way to not have the
finalization in release builds?

https://codereview.chromium.org/1943803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/PseudoElement.h (right):

https://codereview.chromium.org/1943803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PseudoElement.h:64: class PseudoElementData :
public GarbageCollectedFinalized<PseudoElementData> {
This should be in a separate file.

https://codereview.chromium.org/1943803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PseudoElement.h:81: PseudoElementData();
You could write "PseudoElementData() = default;" instead of explicitly defining
it.

https://codereview.chromium.org/1943803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PseudoElement.h:158: return 0;
nullptr

[sof, 2016-05-05 06:54:41]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2016-05-05 06:54:41]

https://codereview.chromium.org/1943803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/1943803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ElementRareData.h:196: if
(m_pseudoElementData)
How about (unconditionally) doing m_pseudoElementData.clear() ? i.e., the
purpose of this clearing operation is to detach the ElementRareData from its
pseudo elements.

[sof, 2016-05-05 07:07:13]

On 2016/05/05 06:39:33, Timothy Loh wrote:
> sof: It seems unfortunate that NodeRareData, and now PseudoElementData also,
are
> finalized even though the finalizers only exist for a couple of assertions,
> since I assume this adds some overhead. Is there a simple way to not have the
> finalization in release builds?
> 

The overhead isn't excessive, so we shouldn't be too worried about using
*Finalized<>, but preferably avoided for objects that are plentiful.
 
PseudoElementData can be GarbageCollected<> and not have an asserting
destructor; I think we should do that here. And DCHECK the reference to
PseudoElementData from ~ElementRareData -- that destructor is needed regardless
as some unmanaged RefPtr<> and OwnPtr<> pointers need to be released.

[ramya.v, 2016-05-05 08:57:10]

Updated as per review comments. PTAL!

Thanks.

https://codereview.chromium.org/1943803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/1943803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ElementRareData.h:196: if
(m_pseudoElementData)
On 2016/05/05 06:54:41, sof wrote:
> How about (unconditionally) doing m_pseudoElementData.clear() ? i.e., the
> purpose of this clearing operation is to detach the ElementRareData from its
> pseudo elements.

If we do unconditionally, if clearPseudoElements is called before calling
setPseudoElement, it would result in a crash because m_pseudoElementData won't
be initialized by then.

https://codereview.chromium.org/1943803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/PseudoElement.h (right):

https://codereview.chromium.org/1943803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PseudoElement.h:64: class PseudoElementData :
public GarbageCollectedFinalized<PseudoElementData> {
On 2016/05/05 06:39:32, Timothy Loh wrote:
> This should be in a separate file.

Done.

https://codereview.chromium.org/1943803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PseudoElement.h:81: PseudoElementData();
On 2016/05/05 06:39:33, Timothy Loh wrote:
> You could write "PseudoElementData() = default;" instead of explicitly
defining
> it.

Done.

https://codereview.chromium.org/1943803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PseudoElement.h:158: return 0;
On 2016/05/05 06:39:33, Timothy Loh wrote:
> nullptr

Done.

[sof, 2016-05-05 13:21:25]

https://codereview.chromium.org/1943803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/1943803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ElementRareData.h:196: if
(m_pseudoElementData)
On 2016/05/05 08:57:09, ramya.v wrote:
> On 2016/05/05 06:54:41, sof wrote:
> > How about (unconditionally) doing m_pseudoElementData.clear() ? i.e., the
> > purpose of this clearing operation is to detach the ElementRareData from its
> > pseudo elements.
> 
> If we do unconditionally, if clearPseudoElements is called before calling
> setPseudoElement, it would result in a crash because m_pseudoElementData won't
> be initialized by then. 

m_pseudoElementClearData.clear() would work just fine and is sufficient.

[ramya.v, 2016-05-06 04:07:14]

https://codereview.chromium.org/1943803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/1943803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ElementRareData.h:196: if
(m_pseudoElementData)
On 2016/05/05 13:21:25, sof wrote:
> On 2016/05/05 08:57:09, ramya.v wrote:
> > On 2016/05/05 06:54:41, sof wrote:
> > > How about (unconditionally) doing m_pseudoElementData.clear() ? i.e., the
> > > purpose of this clearing operation is to detach the ElementRareData from
its
> > > pseudo elements.
> > 
> > If we do unconditionally, if clearPseudoElements is called before calling
> > setPseudoElement, it would result in a crash because m_pseudoElementData
won't
> > be initialized by then. 
> 
> m_pseudoElementClearData.clear() would work just fine and is sufficient.

Hi added suggested changes. Few tests are failing with this. Could you please
take a look? Thanks
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

[sof, 2016-05-06 05:22:15]

https://codereview.chromium.org/1943803002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/1943803002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ElementRareData.h:198:
m_pseudoElementData.clear();
Make this

  if (m_pseudoElementData) {
      m_pseudoElementData->clearPseudoElements();
      m_pseudoElementData.clear();
  }

i.e., we additionally need to force dispose()ing in the other direction, i
overlooked that, but just having the assert in ~ElementRareData is sufficient.

https://codereview.chromium.org/1943803002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/PseudoElementData.h (right):

https://codereview.chromium.org/1943803002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PseudoElementData.h:12: class
PseudoElementData : public GarbageCollectedFinalized<PseudoElementData> {
You can use GarbageCollected<> now + make the class 'final'.

https://codereview.chromium.org/1943803002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PseudoElementData.h:76: return
m_generatedBefore.get();
The uses of get() are redundant, let's remove them while here.

https://codereview.chromium.org/1943803002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PseudoElementData.h:90: #endif
// PseudoElementData_h

[ramya.v, 2016-05-06 06:16:06]

PTAL!

Thanks

https://codereview.chromium.org/1943803002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/1943803002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ElementRareData.h:198:
m_pseudoElementData.clear();
On 2016/05/06 05:22:15, sof wrote:
> Make this
> 
>   if (m_pseudoElementData) {
>       m_pseudoElementData->clearPseudoElements();
>       m_pseudoElementData.clear();
>   }
> 
> i.e., we additionally need to force dispose()ing in the other direction, i
> overlooked that, but just having the assert in ~ElementRareData is sufficient.

Done.

https://codereview.chromium.org/1943803002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/PseudoElementData.h (right):

https://codereview.chromium.org/1943803002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PseudoElementData.h:12: class
PseudoElementData : public GarbageCollectedFinalized<PseudoElementData> {
On 2016/05/06 05:22:15, sof wrote:
> You can use GarbageCollected<> now + make the class 'final'.

Done.

https://codereview.chromium.org/1943803002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PseudoElementData.h:76: return
m_generatedBefore.get();
On 2016/05/06 05:22:15, sof wrote:
> The uses of get() are redundant, let's remove them while here.

Done.

https://codereview.chromium.org/1943803002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PseudoElementData.h:90: #endif
On 2016/05/06 05:22:15, sof wrote:
> // PseudoElementData_h

Done.

[sof, 2016-05-06 06:18:24]

lgtm

[ramya.v, 2016-05-06 06:19:55]

The CQ bit was checked by ramya.v@samsung.com

[commit-bot: I haz the power, 2016-05-06 06:20:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1943803002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1943803002/80001

[commit-bot: I haz the power, 2016-05-06 09:21:36]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-05-06 09:23:11]

Description was changed from

==========
Move PseudoElements storage to their own object.

BUG=479745
==========

to

==========
Move PseudoElements storage to their own object.

BUG=479745

Committed: https://crrev.com/c7d5ebe0a8549633679ace6426a72434c0c8511f
Cr-Commit-Position: refs/heads/master@{#392026}
==========

[commit-bot: I haz the power, 2016-05-06 09:23:12]

Patchset 4 (id:??) landed as
https://crrev.com/c7d5ebe0a8549633679ace6426a72434c0c8511f
Cr-Commit-Position: refs/heads/master@{#392026}