Move markerlists to Oilpan heap and remove finalizer from DocumentMarkerController

Move markerlists to Oilpan heap and remove finalizer from DocumentMarkerController

R=ager@chromium.org, haraken@chromium.org, sigbjornf@opera.com
BUG=

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

[sof, 2014-05-19 07:05:23]

lgtm

https://codereview.chromium.org/289323003/diff/1/Source/core/dom/DocumentMark...
File Source/core/dom/DocumentMarker.h (right):

https://codereview.chromium.org/289323003/diff/1/Source/core/dom/DocumentMark...
Source/core/dom/DocumentMarker.h:30: 
nit: add #include "wtf/VectorTraits.h"?

https://codereview.chromium.org/289323003/diff/1/Source/core/dom/DocumentMark...
File Source/core/dom/DocumentMarkerController.h (right):

https://codereview.chromium.org/289323003/diff/1/Source/core/dom/DocumentMark...
Source/core/dom/DocumentMarkerController.h:91: typedef
WillBeHeapVector<RenderedDocumentMarker> MarkerList;
Clarifying Q: DocumentMarker objects contain a RefPtr<>, so they need to be
finalized. All controlled and taken care of by the vector traits for
DocumentMarker (used by the MarkerList HeapVector<>) ?

[haraken, 2014-05-19 07:13:20]

https://codereview.chromium.org/289323003/diff/1/Source/core/dom/DocumentMark...
File Source/core/dom/DocumentMarkerController.h (left):

https://codereview.chromium.org/289323003/diff/1/Source/core/dom/DocumentMark...
Source/core/dom/DocumentMarkerController.h:50: ~DocumentMarkerController();

You can use DEFINE_EMPTY_VIRTUAL_DESTRUCTOR_WILL_BE_REMOVED.

https://codereview.chromium.org/289323003/diff/1/Source/core/dom/DocumentMark...
File Source/core/dom/DocumentMarkerController.h (right):

https://codereview.chromium.org/289323003/diff/1/Source/core/dom/DocumentMark...
Source/core/dom/DocumentMarkerController.h:77: WillBeHeapVector<DocumentMarker*>
markersFor(Node*, DocumentMarker::MarkerTypes = DocumentMarker::AllMarkers());

Let's use WillBeHeapVector<Member<DocumentMarker>> (not for correctness but for
consistency of programming rules).

The same comment for other parts in this CL.

https://codereview.chromium.org/289323003/diff/1/Source/platform/geometry/Int...
File Source/platform/geometry/IntRect.h (right):

https://codereview.chromium.org/289323003/diff/1/Source/platform/geometry/Int...
Source/platform/geometry/IntRect.h:212: struct VectorTraits<WebCore::IntRect> :
SimpleClassVectorTraits<WebCore::IntRect> { };

Why do you need the trait for IntRect, but not need for FloatRect for example?
I'm not quite sure on which class we need to add the vector trait. (If we forget
to add the trait, are we asserted?)

[Erik Corry, 2014-05-19 07:52:10]

https://codereview.chromium.org/289323003/diff/1/Source/core/dom/DocumentMark...
File Source/core/dom/DocumentMarker.h (right):

https://codereview.chromium.org/289323003/diff/1/Source/core/dom/DocumentMark...
Source/core/dom/DocumentMarker.h:30: 
On 2014/05/19 07:05:24, sof wrote:
> nit: add #include "wtf/VectorTraits.h"?

Done.

https://codereview.chromium.org/289323003/diff/1/Source/core/dom/DocumentMark...
File Source/core/dom/DocumentMarkerController.h (left):

https://codereview.chromium.org/289323003/diff/1/Source/core/dom/DocumentMark...
Source/core/dom/DocumentMarkerController.h:50: ~DocumentMarkerController();
On 2014/05/19 07:13:20, haraken wrote:
> 
> You can use DEFINE_EMPTY_VIRTUAL_DESTRUCTOR_WILL_BE_REMOVED.

Not writing anything at all seems better though.

https://codereview.chromium.org/289323003/diff/1/Source/core/dom/DocumentMark...
File Source/core/dom/DocumentMarkerController.h (right):

https://codereview.chromium.org/289323003/diff/1/Source/core/dom/DocumentMark...
Source/core/dom/DocumentMarkerController.h:77: WillBeHeapVector<DocumentMarker*>
markersFor(Node*, DocumentMarker::MarkerTypes = DocumentMarker::AllMarkers());
On 2014/05/19 07:13:20, haraken wrote:
> 
> Let's use WillBeHeapVector<Member<DocumentMarker>> (not for correctness but
for
> consistency of programming rules).
> 
> The same comment for other parts in this CL.

DocumentMarker is not on the heap.

https://codereview.chromium.org/289323003/diff/1/Source/core/dom/DocumentMark...
Source/core/dom/DocumentMarkerController.h:91: typedef
WillBeHeapVector<RenderedDocumentMarker> MarkerList;
On 2014/05/19 07:05:24, sof wrote:
> Clarifying Q: DocumentMarker objects contain a RefPtr<>, so they need to be
> finalized. All controlled and taken care of by the vector traits for
> DocumentMarker (used by the MarkerList HeapVector<>) ?

Yes, SimpleClassVectorTraits are allowed RefPtrs and are destructed.  They can't
have virtual methods though because they have canInitializeWithMemset

https://codereview.chromium.org/289323003/diff/1/Source/platform/geometry/Int...
File Source/platform/geometry/IntRect.h (right):

https://codereview.chromium.org/289323003/diff/1/Source/platform/geometry/Int...
Source/platform/geometry/IntRect.h:212: struct VectorTraits<WebCore::IntRect> :
SimpleClassVectorTraits<WebCore::IntRect> { };
On 2014/05/19 07:13:20, haraken wrote:
> 
> Why do you need the trait for IntRect, but not need for FloatRect for example?
> I'm not quite sure on which class we need to add the vector trait. (If we
forget
> to add the trait, are we asserted?)

We could add it for FloatRect too.  It makes all vectors potentially more
efficient because it can use memset and memcpy.  For HeapVectors you get an
assert if it is missing.

[Mads Ager (chromium), 2014-05-19 09:23:18]

LGTM, lots of room for follow-up changes to move DocumentMarker and friends to
the oilpan heap. :)

[jochen (gone - plz use gerrit), 2014-05-19 09:34:05]

Source/web lgtm

[haraken, 2014-05-19 09:54:24]

LGTM but we should consider moving DocumentMarker and DocumentMarkerDetails to
the heap in a follow-up. It looks a bit confusing to keep the change in without
moving them.

> We could add it for FloatRect too.  It makes all vectors potentially more
> efficient because it can use memset and memcpy.  For HeapVectors you get an
> assert if it is missing.

- I understand you need to add the vector trait for DocumentMarker and
RenderedDocumentMarker, but why do you need to add the trait for IntRect and
IntPoint? We might want to be consistent about the programming rule.

- Are you planning to remove the vector trait from DocumentMarker and
RenderedDocumentMarker, once you move DocumentMarkerDetails and DocumentMarker
to the heap? Then let's add a FIXME.

[Erik Corry, 2014-05-19 10:27:51]

On 2014/05/19 09:54:24, haraken wrote:
> LGTM but we should consider moving DocumentMarker and DocumentMarkerDetails to
> the heap in a follow-up. It looks a bit confusing to keep the change in
without
> moving them.

Agreed

> > We could add it for FloatRect too.  It makes all vectors potentially more
> > efficient because it can use memset and memcpy.  For HeapVectors you get an
> > assert if it is missing.
> 
> - I understand you need to add the vector trait for DocumentMarker and
> RenderedDocumentMarker, but why do you need to add the trait for IntRect and
> IntPoint? We might want to be consistent about the programming rule.

I tried changing the renderedRectsForMarkers method to return a HeapVector, but
I had to back off that for now because of non-heap classes that are required to
have methods of the same type.  I left the traits for IntRect and IntPoint since
they make the regular vectors more efficient too, and we will need them later.
 
> - Are you planning to remove the vector trait from DocumentMarker and
> RenderedDocumentMarker, once you move DocumentMarkerDetails and DocumentMarker
> to the heap? Then let's add a FIXME.

No, we will need the vector trait for them even if they exist as on-heap
independent objects because it's a vector of actual objects, not a vector of
members pointing to the objects.

[Erik Corry, 2014-05-19 10:28:17]

The CQ bit was checked by erik.corry@gmail.com

[commit-bot: I haz the power, 2014-05-19 10:28:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erik.corry@gmail.com/289323003/20001

[commit-bot: I haz the power, 2014-05-19 13:46:38]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/8844)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8671)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/8139)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/8244)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-05-19 14:04:40]

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

[commit-bot: I haz the power, 2014-05-19 14:04:40]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[Erik Corry, 2014-05-20 08:40:37]

https://codereview.chromium.org/289323003/diff/1/Source/core/dom/DocumentMark...
File Source/core/dom/DocumentMarkerController.h (left):

https://codereview.chromium.org/289323003/diff/1/Source/core/dom/DocumentMark...
Source/core/dom/DocumentMarkerController.h:50: ~DocumentMarkerController();
On 2014/05/19 07:52:11, Erik Corry wrote:
> On 2014/05/19 07:13:20, haraken wrote:
> > 
> > You can use DEFINE_EMPTY_VIRTUAL_DESTRUCTOR_WILL_BE_REMOVED.
> 
> Not writing anything at all seems better though.

I put this back after our offline discussion this morning.

[Erik Corry, 2014-05-20 08:40:47]

The CQ bit was checked by erik.corry@gmail.com

[Erik Corry, 2014-05-20 08:40:54]

The CQ bit was unchecked by erik.corry@gmail.com

[commit-bot: I haz the power, 2014-05-20 08:40:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erik.corry@gmail.com/289323003/40001

[Erik Corry, 2014-05-20 08:43:41]

The CQ bit was checked by erik.corry@gmail.com

[commit-bot: I haz the power, 2014-05-20 08:43:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erik.corry@gmail.com/289323003/60001

[commit-bot: I haz the power, 2014-05-20 10:02:21]

Change committed as 174362