Oilpan: move editing objects to the heap.

Oilpan: move editing objects to the heap.

Move the editing commands and style objects to the Oilpan heap, along
with having them use transition types throughout.

R=zerny@chromium.org,haraken@chromium.org,tkent@chromium.org
BUG=357163
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175079

[sof, 2014-05-25 16:33:17]

Please take a look.

I apologize for the length of this, but once you flip the switch for UndoStep
and EditingStyle, most other objects in editing/ needs to be updated. I couldn't
see a way to split this up other than introducing temporary persistents.

[haraken, 2014-05-26 02:36:11]

Here is a first round of comments!

I'm OK with handling this CL, but it would be great if we could split this kind
of huge CLs into pieces :) (e.g., Changes to Position objects could be landed
separately.)

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

https://codereview.chromium.org/299353004/diff/1/Source/core/dom/Position.h#n...
Source/core/dom/Position.h:218: RefPtr<Node> m_anchorNode;

This should be RefPtrWillBeMember.

Ian: It'd be nice if the plugin could detects this kind of error when
non-Member/non-RawPtr is traced.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ApplySty...
File Source/core/editing/ApplyStyleCommand.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ApplySty...
Source/core/editing/ApplyStyleCommand.cpp:764:
WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS(WebCore::InlineRunToApplyStyle);

I'm not sure if we can set canMoveWithMemcpy.

InlineRunToApplyStyle holds StyleChange which holds Strings. Given that Strings
need destructors to keep the reference counting of the underlying StringImpl
objects, I don't think canMoveWithMemcpy works correctly for
InlineRunToApplyStyle.

Also see my comment in markup.cpp.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ApplySty...
Source/core/editing/ApplyStyleCommand.cpp:1132: RefPtrWillBeRawPtr<Node> next;

= nullptr

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ApplySty...
Source/core/editing/ApplyStyleCommand.cpp:1409: RefPtrWillBeMember<Node>
dummyElement = nullptr;

This should be RefPtrWillBeRawPtr.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/BreakBlo...
File Source/core/editing/BreakBlockquoteCommand.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/BreakBlo...
Source/core/editing/BreakBlockquoteCommand.cpp:169: RefPtrWillBeRawPtr<Element>
clonedParent;

= nullptr;

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Caret.h
File Source/core/editing/Caret.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Caret.h#...
Source/core/editing/Caret.h:41: class CaretBase {

CaretBase needs to be a GC mix-in once we move VisiblePosition to the heap
(DragCaretController has VisiblePosition as a part of object), but it looks OK
to keep CaretBase off-heap in this CL.

(I hope the plugin catches the error if we forget to make CaretBase a GC mix-in
when moving VisiblePosition to the heap.)

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Composit...
File Source/core/editing/CompositeEditCommand.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Composit...
Source/core/editing/CompositeEditCommand.cpp:981: RefPtrWillBeRawPtr<Node>
lastNode;

= nullptr;

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Composit...
Source/core/editing/CompositeEditCommand.cpp:1467: RefPtrWillBeRawPtr<Node>
node;

= nullptr;

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/DeleteSe...
File Source/core/editing/DeleteSelectionCommand.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/DeleteSe...
Source/core/editing/DeleteSelectionCommand.h:90: Position m_trailingWhitespace;

You need to trace these Position objects? (The plugin didn't detect this error
because you used a RefPtr instead of a Member for Position::m_anchorNode.)

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/EditComm...
File Source/core/editing/EditCommand.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/EditComm...
Source/core/editing/EditCommand.h:77: CompositeEditCommand* m_parent;

Shouldn't this be a Member, since CompositeEditCommand is now on the heap?

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/EditComm...
Source/core/editing/EditCommand.h:85: class SimpleEditCommand : public
EditCommand {

Just to confirm: You don't need to define SimpleEditCommand::trace(), because
SimpleEditCommand doesn't have any members, and SimpleEditCommand::trace() is
redirected to EditCommand::trace(). Right?

(Ian was discussing this programming rule before, and I think your CL is
following Ian's conclusion.)

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/EditingS...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/EditingS...
Source/core/editing/EditingStyle.cpp:1056: RefPtrWillBeRawPtr<EditingStyle>
wrappingStyle;

= nullptr;

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Editor.cpp
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Editor.c...
Source/core/editing/Editor.cpp:1086: RefPtrWillBeRawPtr<EditingStyle>
typingStyle;

= nullptr;

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Editor.h
File Source/core/editing/Editor.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Editor.h...
Source/core/editing/Editor.h:240: LocalFrame& m_frame;

Are you sure that this raw pointer is safe?

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/FrameSel...
File Source/core/editing/FrameSelection.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/FrameSel...
Source/core/editing/FrameSelection.h:257: LocalFrame* m_frame;

This raw pointer looks safe because the FrameSelection is owned by the
LocalFrame.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/InsertLi...
File Source/core/editing/InsertLineBreakCommand.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/InsertLi...
Source/core/editing/InsertLineBreakCommand.cpp:84: RefPtrWillBeRawPtr<Node>
nodeToInsert;

= nullptr;

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/InsertPa...
File Source/core/editing/InsertParagraphSeparatorCommand.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/InsertPa...
Source/core/editing/InsertParagraphSeparatorCommand.cpp:204:
RefPtrWillBeRawPtr<Element> blockToInsert;

= nullptr;

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/MarkupAc...
File Source/core/editing/MarkupAccumulator.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/MarkupAc...
Source/core/editing/MarkupAccumulator.h:106:
WillBeHeapVector<RawPtrWillBeMember<Node> >* const m_nodes;

Shall we use RawPtrWillBeMember<WillBeHeapVector<RawPtrWillBeMember<Node> >> ?

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/MoveSele...
File Source/core/editing/MoveSelectionCommand.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/MoveSele...
Source/core/editing/MoveSelectionCommand.h:51: Position m_position;

You need to trace m_position.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ReplaceN...
File Source/core/editing/ReplaceNodeWithSpanCommand.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ReplaceN...
Source/core/editing/ReplaceNodeWithSpanCommand.cpp:54:
RefPtrWillBeMember<ContainerNode> parentNode = nodeToReplace.parentNode();

RefPtrWillBeRawPtr.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ReplaceS...
File Source/core/editing/ReplaceSelectionCommand.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ReplaceS...
Source/core/editing/ReplaceSelectionCommand.cpp:466: RefPtrWillBeRawPtr<Node>
next;

= nullptr;

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ReplaceS...
Source/core/editing/ReplaceSelectionCommand.cpp:616: RefPtrWillBeRawPtr<Node>
next;

= nullptr;

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ReplaceS...
File Source/core/editing/ReplaceSelectionCommand.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ReplaceS...
Source/core/editing/ReplaceSelectionCommand.h:109: Position
m_endOfInsertedContent;

These Positions should be traced.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/TypingCo...
File Source/core/editing/TypingCommand.cpp (left):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/TypingCo...
Source/core/editing/TypingCommand.cpp:428: FrameSelection selection;

Just to confirm: Does the plugin detect the error if you forget to change this
line?

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/UndoStac...
File Source/core/editing/UndoStack.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/UndoStac...
Source/core/editing/UndoStack.cpp:79: UndoStepStack newStack;

This shouldn't be a persistent handle.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/UndoStack.h
File Source/core/editing/UndoStack.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/UndoStac...
Source/core/editing/UndoStack.h:62: typedef
WillBePersistentHeapDeque<RefPtrWillBeMember<UndoStep> > UndoStepStack;

Shall we avoid using typedef for persistent collections? It's easy to use it in
places where persistent handles shouldn't be used (e.g., see my comment in
UndoStack.cpp.)

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/markup.cpp
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/markup.c...
Source/core/editing/markup.cpp:102:
WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS(WebCore::AttributeChange);

Is it OK to set canMoveWithMemcpy? I'm concerned that it will confuse reference
counting of a StringImpl behind AttributeChange::m_value.

I think we should make a macro that only sets canInitializeWithMemset and use
the macro by default for ALLOW_ONLY_INLINE_ALLOCATION objects. If we're sure
that the object is movable by memcpy, we can use other macros as a way of
performance optimization.

[haraken, 2014-05-26 05:56:24]

+yosin, +yoichio

[yosin_UTC9, 2014-05-26 07:06:42]

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Editor.h
File Source/core/editing/Editor.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Editor.h...
Source/core/editing/Editor.h:240: LocalFrame& m_frame;
On 2014/05/26 02:36:12, haraken wrote:
> 
> Are you sure that this raw pointer is safe?

FYI: The reason we use reference is life time of Editor object is as same as
LocalFrame.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/FrameSel...
File Source/core/editing/FrameSelection.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/FrameSel...
Source/core/editing/FrameSelection.h:257: LocalFrame* m_frame;
On 2014/05/26 02:36:12, haraken wrote:
> 
> This raw pointer looks safe because the FrameSelection is owned by the
> LocalFrame.

FYI: m_frame can be null when FrameSelection is allocated on stack.

[zerny-chromium, 2014-05-26 07:51:07]

oilpan lgtm with Haraken's comments addressed.

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

https://codereview.chromium.org/299353004/diff/1/Source/core/dom/Position.h#n...
Source/core/dom/Position.h:218: RefPtr<Node> m_anchorNode;
On 2014/05/26 02:36:12, haraken wrote:
> 
> This should be RefPtrWillBeMember.
> 
> Ian: It'd be nice if the plugin could detects this kind of error when
> non-Member/non-RawPtr is traced.

That might be a good check to have during the transition. After transition this
would be caught since it is disallowed to have Ref/Raw/Own ptrs to GC allocated
objects. I'll add it to the list.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ApplySty...
File Source/core/editing/ApplyStyleCommand.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ApplySty...
Source/core/editing/ApplyStyleCommand.cpp:1409: RefPtrWillBeMember<Node>
dummyElement = nullptr;
On 2014/05/26 02:36:12, haraken wrote:
> 
> This should be RefPtrWillBeRawPtr.

While I agree that a raw pointer would be more apt here, but the real/other use
is that the dummyElement in an output parameter to a slot allocated in the
collection. I'm not sure what the best solution is here.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/DeleteSe...
File Source/core/editing/DeleteSelectionCommand.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/DeleteSe...
Source/core/editing/DeleteSelectionCommand.h:90: Position m_trailingWhitespace;
On 2014/05/26 02:36:12, haraken wrote:
> 
> You need to trace these Position objects? (The plugin didn't detect this error
> because you used a RefPtr instead of a Member for Position::m_anchorNode.)

That is a good reason to add the check for tracing RefPtrs. However, the check
we really want is to disallow a GC allocated type to have Ref/Raw pointers to
another GC allocated type. That is explicitly disabled during transition because
it hinders incremental changes :-(

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/MarkupAc...
File Source/core/editing/MarkupAccumulator.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/MarkupAc...
Source/core/editing/MarkupAccumulator.h:69:
MarkupAccumulator(WillBeHeapVector<RawPtrWillBeMember<Node> >*, EAbsoluteURLs,
RawPtr<const Range> = nullptr, SerializationType = AsOwnerDocument);
Nit: Do we need to change Range* to RawPtr<Range>?

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/TypingCo...
File Source/core/editing/TypingCommand.cpp (left):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/TypingCo...
Source/core/editing/TypingCommand.cpp:428: FrameSelection selection;
On 2014/05/26 02:36:12, haraken wrote:
> 
> Just to confirm: Does the plugin detect the error if you forget to change this
> line?

The plugin cannot check against the use of OwnPtr here (after the change) since
we don't check method bodies, only declaration structure. To assert againt on
stack uses of OwnPtr we would need to add static asserts to the OwnPtr
constructor (which requires referencing heap structures in wtf).

In any case, this would result in a compile error since FrameSelection now has
an explict create method.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/UndoStack.h
File Source/core/editing/UndoStack.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/UndoStac...
Source/core/editing/UndoStack.h:62: typedef
WillBePersistentHeapDeque<RefPtrWillBeMember<UndoStep> > UndoStepStack;
On 2014/05/26 02:36:12, haraken wrote:
> 
> Shall we avoid using typedef for persistent collections? It's easy to use it
in
> places where persistent handles shouldn't be used (e.g., see my comment in
> UndoStack.cpp.)

In other places we have used a typedef pair to avoid hidden persistents:

typedef WillBePersistentHeapFoo PersistentFoo
typedef WillBeHeapFoo Foo

[sof, 2014-05-28 08:31:35]

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

https://codereview.chromium.org/299353004/diff/1/Source/core/dom/Position.h#n...
Source/core/dom/Position.h:218: RefPtr<Node> m_anchorNode;
On 2014/05/26 07:51:08, zerny-chromium wrote:
> On 2014/05/26 02:36:12, haraken wrote:
> > 
> > This should be RefPtrWillBeMember.
> > 
> > Ian: It'd be nice if the plugin could detects this kind of error when
> > non-Member/non-RawPtr is traced.
> 
> That might be a good check to have during the transition. After transition
this
> would be caught since it is disallowed to have Ref/Raw/Own ptrs to GC
allocated
> objects. I'll add it to the list.

Fixed.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ApplySty...
File Source/core/editing/ApplyStyleCommand.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ApplySty...
Source/core/editing/ApplyStyleCommand.cpp:764:
WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS(WebCore::InlineRunToApplyStyle);
On 2014/05/26 02:36:12, haraken wrote:
> 
> I'm not sure if we can set canMoveWithMemcpy.
> 
> InlineRunToApplyStyle holds StyleChange which holds Strings. Given that
Strings
> need destructors to keep the reference counting of the underlying StringImpl
> objects, I don't think canMoveWithMemcpy works correctly for
> InlineRunToApplyStyle.
> 
> Also see my comment in markup.cpp.

Yes, keeping this as a inline object won't work for that reason. Thanks for
checking all the fields here, which I didn't do.

Moved InlineRunToApplyStyle to the heap as a consequence.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ApplySty...
Source/core/editing/ApplyStyleCommand.cpp:1132: RefPtrWillBeRawPtr<Node> next;
On 2014/05/26 02:36:12, haraken wrote:
> 
> = nullptr

Complied.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ApplySty...
Source/core/editing/ApplyStyleCommand.cpp:1409: RefPtrWillBeMember<Node>
dummyElement = nullptr;
On 2014/05/26 07:51:08, zerny-chromium wrote:
> On 2014/05/26 02:36:12, haraken wrote:
> > 
> > This should be RefPtrWillBeRawPtr.
> 
> While I agree that a raw pointer would be more apt here, but the real/other
use
> is that the dummyElement in an output parameter to a slot allocated in the
> collection. I'm not sure what the best solution is here.

Yes, that's what brings about the Member use here.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/BreakBlo...
File Source/core/editing/BreakBlockquoteCommand.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/BreakBlo...
Source/core/editing/BreakBlockquoteCommand.cpp:169: RefPtrWillBeRawPtr<Element>
clonedParent;
On 2014/05/26 02:36:12, haraken wrote:
> 
> = nullptr;

Done.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Caret.h
File Source/core/editing/Caret.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Caret.h#...
Source/core/editing/Caret.h:41: class CaretBase {
On 2014/05/26 02:36:12, haraken wrote:
> 
> CaretBase needs to be a GC mix-in once we move VisiblePosition to the heap
> (DragCaretController has VisiblePosition as a part of object), but it looks OK
> to keep CaretBase off-heap in this CL.
> 
> (I hope the plugin catches the error if we forget to make CaretBase a GC
mix-in
> when moving VisiblePosition to the heap.)

Hmm, do we declare these kinds of private-inherited 'base' classes as GC mixins
even if derived objects of CaretBase are never stored as CaretBases?

Left it as-is.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Composit...
File Source/core/editing/CompositeEditCommand.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Composit...
Source/core/editing/CompositeEditCommand.cpp:981: RefPtrWillBeRawPtr<Node>
lastNode;
On 2014/05/26 02:36:12, haraken wrote:
> 
> = nullptr;

Done.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Composit...
Source/core/editing/CompositeEditCommand.cpp:1467: RefPtrWillBeRawPtr<Node>
node;
On 2014/05/26 02:36:12, haraken wrote:
> 
> = nullptr;

Done.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/DeleteSe...
File Source/core/editing/DeleteSelectionCommand.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/DeleteSe...
Source/core/editing/DeleteSelectionCommand.h:90: Position m_trailingWhitespace;
On 2014/05/26 07:51:08, zerny-chromium wrote:
> On 2014/05/26 02:36:12, haraken wrote:
> > 
> > You need to trace these Position objects? (The plugin didn't detect this
error
> > because you used a RefPtr instead of a Member for Position::m_anchorNode.)
> 
> That is a good reason to add the check for tracing RefPtrs. However, the check
> we really want is to disallow a GC allocated type to have Ref/Raw pointers to
> another GC allocated type. That is explicitly disabled during transition
because
> it hinders incremental changes :-(

Position (and VisiblePosition) objects are now traced throughout.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/EditComm...
File Source/core/editing/EditCommand.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/EditComm...
Source/core/editing/EditCommand.h:77: CompositeEditCommand* m_parent;
On 2014/05/26 02:36:12, haraken wrote:
> 
> Shouldn't this be a Member, since CompositeEditCommand is now on the heap?

Yes, now a Member. afaict, the command holding this parent object reference is
always stored in the command sequence on the parent itself, so it seems safe to
keep it raw pointer. But Member is preferable.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/EditComm...
Source/core/editing/EditCommand.h:85: class SimpleEditCommand : public
EditCommand {
On 2014/05/26 02:36:12, haraken wrote:
> 
> Just to confirm: You don't need to define SimpleEditCommand::trace(), because
> SimpleEditCommand doesn't have any members, and SimpleEditCommand::trace() is
> redirected to EditCommand::trace(). Right?
> 
> (Ian was discussing this programming rule before, and I think your CL is
> following Ian's conclusion.)

That's the pattern I've tried to follow in this directory -- don't define
trace()s that merely redirect up the inheritance chain.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/EditingS...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/EditingS...
Source/core/editing/EditingStyle.cpp:1056: RefPtrWillBeRawPtr<EditingStyle>
wrappingStyle;
On 2014/05/26 02:36:12, haraken wrote:
> 
> = nullptr;

Done.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Editor.cpp
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Editor.c...
Source/core/editing/Editor.cpp:1086: RefPtrWillBeRawPtr<EditingStyle>
typingStyle;
On 2014/05/26 02:36:12, haraken wrote:
> 
> = nullptr;

Done.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Editor.h
File Source/core/editing/Editor.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/Editor.h...
Source/core/editing/Editor.h:240: LocalFrame& m_frame;
On 2014/05/26 07:06:42, yosi wrote:
> On 2014/05/26 02:36:12, haraken wrote:
> > 
> > Are you sure that this raw pointer is safe?
> 
> FYI: The reason we use reference is life time of Editor object is as same as
> LocalFrame.

Thanks for confirming, that's what I was inferring from LocalFrame use.
LocalFrame is not on the heap.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/InsertLi...
File Source/core/editing/InsertLineBreakCommand.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/InsertLi...
Source/core/editing/InsertLineBreakCommand.cpp:84: RefPtrWillBeRawPtr<Node>
nodeToInsert;
On 2014/05/26 02:36:12, haraken wrote:
> 
> = nullptr;

Complied.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/InsertPa...
File Source/core/editing/InsertParagraphSeparatorCommand.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/InsertPa...
Source/core/editing/InsertParagraphSeparatorCommand.cpp:204:
RefPtrWillBeRawPtr<Element> blockToInsert;
On 2014/05/26 02:36:12, haraken wrote:
> 
> = nullptr;

Complied.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/MarkupAc...
File Source/core/editing/MarkupAccumulator.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/MarkupAc...
Source/core/editing/MarkupAccumulator.h:69:
MarkupAccumulator(WillBeHeapVector<RawPtrWillBeMember<Node> >*, EAbsoluteURLs,
RawPtr<const Range> = nullptr, SerializationType = AsOwnerDocument);
On 2014/05/26 07:51:08, zerny-chromium wrote:
> Nit: Do we need to change Range* to RawPtr<Range>?

Because it'll soon be a Range* again? :) No, no good reason beyond trying to
consistently use RawPtr<>.

Switched back.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/MarkupAc...
Source/core/editing/MarkupAccumulator.h:106:
WillBeHeapVector<RawPtrWillBeMember<Node> >* const m_nodes;
On 2014/05/26 02:36:12, haraken wrote:
> 
> Shall we use RawPtrWillBeMember<WillBeHeapVector<RawPtrWillBeMember<Node> >> ?

It shouldn't make a difference wrt stack scanning, but no good reason not to.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/MoveSele...
File Source/core/editing/MoveSelectionCommand.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/MoveSele...
Source/core/editing/MoveSelectionCommand.h:51: Position m_position;
On 2014/05/26 02:36:12, haraken wrote:
> 
> You need to trace m_position.

Done.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ReplaceN...
File Source/core/editing/ReplaceNodeWithSpanCommand.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ReplaceN...
Source/core/editing/ReplaceNodeWithSpanCommand.cpp:54:
RefPtrWillBeMember<ContainerNode> parentNode = nodeToReplace.parentNode();
On 2014/05/26 02:36:12, haraken wrote:
> 
> RefPtrWillBeRawPtr.

Done.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ReplaceS...
File Source/core/editing/ReplaceSelectionCommand.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ReplaceS...
Source/core/editing/ReplaceSelectionCommand.cpp:466: RefPtrWillBeRawPtr<Node>
next;
On 2014/05/26 02:36:12, haraken wrote:
> 
> = nullptr;

Complied.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ReplaceS...
Source/core/editing/ReplaceSelectionCommand.cpp:616: RefPtrWillBeRawPtr<Node>
next;
On 2014/05/26 02:36:12, haraken wrote:
> 
> = nullptr;

Complied.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ReplaceS...
File Source/core/editing/ReplaceSelectionCommand.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/ReplaceS...
Source/core/editing/ReplaceSelectionCommand.h:109: Position
m_endOfInsertedContent;
On 2014/05/26 02:36:12, haraken wrote:
> 
> These Positions should be traced.

All done now.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/UndoStac...
File Source/core/editing/UndoStack.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/UndoStac...
Source/core/editing/UndoStack.cpp:79: UndoStepStack newStack;
On 2014/05/26 02:36:12, haraken wrote:
> 
> This shouldn't be a persistent handle.

Tidied.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/UndoStack.h
File Source/core/editing/UndoStack.h (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/UndoStac...
Source/core/editing/UndoStack.h:62: typedef
WillBePersistentHeapDeque<RefPtrWillBeMember<UndoStep> > UndoStepStack;
On 2014/05/26 07:51:08, zerny-chromium wrote:
> On 2014/05/26 02:36:12, haraken wrote:
> > 
> > Shall we avoid using typedef for persistent collections? It's easy to use it
> in
> > places where persistent handles shouldn't be used (e.g., see my comment in
> > UndoStack.cpp.)
> 
> In other places we have used a typedef pair to avoid hidden persistents:
> 
> typedef WillBePersistentHeapFoo PersistentFoo
> typedef WillBeHeapFoo Foo

Followed that.

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/markup.cpp
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/299353004/diff/1/Source/core/editing/markup.c...
Source/core/editing/markup.cpp:102:
WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS(WebCore::AttributeChange);
On 2014/05/26 02:36:12, haraken wrote:
> 
> Is it OK to set canMoveWithMemcpy? I'm concerned that it will confuse
reference
> counting of a StringImpl behind AttributeChange::m_value.
> 
> I think we should make a macro that only sets canInitializeWithMemset and use
> the macro by default for ALLOW_ONLY_INLINE_ALLOCATION objects. If we're sure
> that the object is movable by memcpy, we can use other macros as a way of
> performance optimization.

This object needs to be on the heap and finalized also (to handle vector
resizes.)

[haraken, 2014-05-28 15:35:24]

Thanks for the update! Here is a final round of comments.

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Appl...
File Source/core/editing/ApplyStyleCommand.cpp (right):

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Appl...
Source/core/editing/ApplyStyleCommand.cpp:112: return fontNode.release();

Nit: return createHTMLElement(document, fontTag);

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Appl...
Source/core/editing/ApplyStyleCommand.cpp:729: class InlineRunToApplyStyle :
public NoBaseWillBeGarbageCollectedFinalized<InlineRunToApplyStyle> {

I think InlineRunToApplyStyle can be ALLOW_ONLY_INLINE_ALLOCATION() by setting
canInitializeWithMemset to true.

(What I pointed out in the previous review is that you cannot use
WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS because it specifies
canMoveWithMemcpy as well.)

I think we can create a new macro  WTF_ALLOW_INIT_WITH_MEM_FUNCTION, which
specifies canInitializeWithMemset only. Then the programming rule is:

- You have to specify WTF_ALLOW_INIT_WITH_MEM_FUNCTION for
ALLOW_ONLY_INLINE_ALLOCATION() objects.
- You can specify WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS if the object is
movable with memcpy. This is just for performance optimization.

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Fram...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Fram...
Source/core/editing/FrameSelection.cpp:110:
stopObservingVisibleSelectionChangeIfNecessary();

This destructor touches m_selection, which is an on-heap object.

I think you need to make VisibleSelection::m_changeObserver a WeakMember, and
remove the above code from the destructor.

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Remo...
File Source/core/editing/RemoveNodePreservingChildrenCommand.h (right):

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Remo...
Source/core/editing/RemoveNodePreservingChildrenCommand.h:43: explicit
RemoveNodePreservingChildrenCommand(PassRefPtrWillBeRawPtr<Node>,
ShouldAssumeContentIsAlwaysEditable);

Drop explicit.

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Visi...
File Source/core/editing/VisiblePosition.h (right):

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Visi...
Source/core/editing/VisiblePosition.h:53: ALLOW_ONLY_INLINE_ALLOCATION();

Who uses VisiblePosition in collections? Can this probably be
DISALLOW_ALLOCATION ?

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Visi...
File Source/core/editing/VisibleSelection.cpp (right):

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Visi...
Source/core/editing/VisibleSelection.cpp:139: didChange();

This touches another on-heap object. If you make m_changeObserver a WeakMember,
I think you can just remove didChange() from here.

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Visi...
File Source/core/editing/VisibleSelection.h (right):

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Visi...
Source/core/editing/VisibleSelection.h:42: ALLOW_ONLY_INLINE_ALLOCATION();

Just help me understand: Who uses VisibleSelection in collections? Can this
probably be DISALLOW_ALLOCATION() ?

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Visi...
Source/core/editing/VisibleSelection.h:162: ChangeObserver* m_changeObserver;

I think this should be a WeakMember.

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/mark...
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/mark...
Source/core/editing/markup.cpp:154: RefPtrWillBeMember<EditingStyle>
m_wrappingStyle;

Who traces these Members?

Ian: It'd be great if the plugin could catch this :)

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/mark...
Source/core/editing/markup.cpp:296: RefPtrWillBeRawPtr<EditingStyle>
newInlineStyle;

= nullptr;

Mads prefers always-initialization even if it compiles.

[Mads Ager (chromium), 2014-05-28 16:09:43]

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/mark...
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/mark...
Source/core/editing/markup.cpp:296: RefPtrWillBeRawPtr<EditingStyle>
newInlineStyle;
On 2014/05/28 15:35:25, haraken wrote:
> 
> = nullptr;
> 
> Mads prefers always-initialization even if it compiles.

I do prefer that. :-)

On the other hand I don't have much of a problem with not doing it if we are
sure that compilers catch this. When I changed RawPtr to not zero-initialize
there were bugs that were not caught by clang where we were missing nullptr
initialization of these. If we are sure that compilers will catch uninitialized
RawPtrs I'm fine with leaving it out as well.

[sof, 2014-05-28 22:06:05]

Thanks haraken. I'm all for doing smaller CLs by now :)

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Appl...
File Source/core/editing/ApplyStyleCommand.cpp (right):

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Appl...
Source/core/editing/ApplyStyleCommand.cpp:112: return fontNode.release();
On 2014/05/28 15:35:25, haraken wrote:
> 
> Nit: return createHTMLElement(document, fontTag);

Done (same for the helper below.)

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Appl...
Source/core/editing/ApplyStyleCommand.cpp:729: class InlineRunToApplyStyle :
public NoBaseWillBeGarbageCollectedFinalized<InlineRunToApplyStyle> {
On 2014/05/28 15:35:25, haraken wrote:
> 
> I think InlineRunToApplyStyle can be ALLOW_ONLY_INLINE_ALLOCATION() by setting
> canInitializeWithMemset to true.
> 
> (What I pointed out in the previous review is that you cannot use
> WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS because it specifies
> canMoveWithMemcpy as well.)
> 
> I think we can create a new macro  WTF_ALLOW_INIT_WITH_MEM_FUNCTION, which
> specifies canInitializeWithMemset only. Then the programming rule is:
> 
> - You have to specify WTF_ALLOW_INIT_WITH_MEM_FUNCTION for
> ALLOW_ONLY_INLINE_ALLOCATION() objects.
> - You can specify WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS if the object is
> movable with memcpy. This is just for performance optimization.

Aha, that works out -- I read the COMPILE_ASSERT in Vector() wrong. Added the
macro to VectorTraits and used it; thanks.

(Similar change made for markup.cpp:AttributeChange )

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Fram...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Fram...
Source/core/editing/FrameSelection.cpp:110:
stopObservingVisibleSelectionChangeIfNecessary();
On 2014/05/28 15:35:25, haraken wrote:
> 
> This destructor touches m_selection, which is an on-heap object.
> 
> I think you need to make VisibleSelection::m_changeObserver a WeakMember, and
> remove the above code from the destructor.

Yes, the observer clearing is not needed at all with Oilpan; made the destructor
be empty. No need to weakly reference a part object.

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Remo...
File Source/core/editing/RemoveNodePreservingChildrenCommand.h (right):

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Remo...
Source/core/editing/RemoveNodePreservingChildrenCommand.h:43: explicit
RemoveNodePreservingChildrenCommand(PassRefPtrWillBeRawPtr<Node>,
ShouldAssumeContentIsAlwaysEditable);
On 2014/05/28 15:35:25, haraken wrote:
> 
> Drop explicit.

Done.

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Visi...
File Source/core/editing/VisiblePosition.h (right):

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Visi...
Source/core/editing/VisiblePosition.h:53: ALLOW_ONLY_INLINE_ALLOCATION();
On 2014/05/28 15:35:25, haraken wrote:
> 
> Who uses VisiblePosition in collections? Can this probably be
> DISALLOW_ALLOCATION ?

No one is it turns out; don't need more than DISALLOW_ALLOCATION().

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Visi...
File Source/core/editing/VisibleSelection.cpp (right):

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Visi...
Source/core/editing/VisibleSelection.cpp:139: didChange();
On 2014/05/28 15:35:25, haraken wrote:
> 
> This touches another on-heap object. If you make m_changeObserver a
WeakMember,
> I think you can just remove didChange() from here.

I'm not sure the generality of a WeakMember is warranted here -- the only place
a ChangeObserver is set is for FrameSelection::m_selection, which is a part
object of the FrameSelection observer. I think we can wait and see a bit before
generalizing..?

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Visi...
File Source/core/editing/VisibleSelection.h (right):

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/Visi...
Source/core/editing/VisibleSelection.h:42: ALLOW_ONLY_INLINE_ALLOCATION();
On 2014/05/28 15:35:25, haraken wrote:
> 
> Just help me understand: Who uses VisibleSelection in collections? Can this
> probably be DISALLOW_ALLOCATION() ?

Yes, no more than DISALLOW_ALLOCATION() is needed.

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/mark...
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/mark...
Source/core/editing/markup.cpp:296: RefPtrWillBeRawPtr<EditingStyle>
newInlineStyle;
On 2014/05/28 16:09:44, Mads Ager (chromium) wrote:
> On 2014/05/28 15:35:25, haraken wrote:
> > 
> > = nullptr;
> > 
> > Mads prefers always-initialization even if it compiles.
> 
> I do prefer that. :-)
> 
> On the other hand I don't have much of a problem with not doing it if we are
> sure that compilers catch this. When I changed RawPtr to not zero-initialize
> there were bugs that were not caught by clang where we were missing nullptr
> initialization of these. If we are sure that compilers will catch
uninitialized
> RawPtrs I'm fine with leaving it out as well. 

Complied.

[haraken, 2014-05-29 01:14:14]

LGTM.

tkent-san: Would you approve a change to wtf/ ?

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/mark...
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/299353004/diff/20001/Source/core/editing/mark...
Source/core/editing/markup.cpp:154: RefPtrWillBeMember<EditingStyle>
m_wrappingStyle;
On 2014/05/28 15:35:25, haraken wrote:
> 
> Who traces these Members?
> 
> Ian: It'd be great if the plugin could catch this :)

Ignore this comment :) I found that this object is stack-allocated.

https://codereview.chromium.org/299353004/diff/40001/Source/core/editing/Visi...
File Source/core/editing/VisibleSelection.h (right):

https://codereview.chromium.org/299353004/diff/40001/Source/core/editing/Visi...
Source/core/editing/VisibleSelection.h:164: ChangeObserver* m_changeObserver;

I agree that no weak processing is needed for the observer.

Given that the only derived class of ChangeObserver is FrameSelection (which is
on heap), can we change this to a Member and trace it? It's not really nice to
leave a raw pointer in on-heap objects.

[tkent, 2014-05-29 01:19:22]

wtf LGTM.

Please fix VisibleSelectionTest.cpp before landing.

https://codereview.chromium.org/299353004/diff/40001/Source/wtf/VectorTraits.h
File Source/wtf/VectorTraits.h (right):

https://codereview.chromium.org/299353004/diff/40001/Source/wtf/VectorTraits....
Source/wtf/VectorTraits.h:113: template<> \
nit: We don't need to indent the content of namespace {}.

[sof, 2014-05-29 10:03:52]

https://codereview.chromium.org/299353004/diff/40001/Source/core/editing/Visi...
File Source/core/editing/VisibleSelection.h (right):

https://codereview.chromium.org/299353004/diff/40001/Source/core/editing/Visi...
Source/core/editing/VisibleSelection.h:164: ChangeObserver* m_changeObserver;
On 2014/05/29 01:14:15, haraken wrote:
> 
> I agree that no weak processing is needed for the observer.
> 
> Given that the only derived class of ChangeObserver is FrameSelection (which
is
> on heap), can we change this to a Member and trace it? It's not really nice to
> leave a raw pointer in on-heap objects.

Done, made ChangeObserver a GC mixin.

https://codereview.chromium.org/299353004/diff/40001/Source/wtf/VectorTraits.h
File Source/wtf/VectorTraits.h (right):

https://codereview.chromium.org/299353004/diff/40001/Source/wtf/VectorTraits....
Source/wtf/VectorTraits.h:113: template<> \
On 2014/05/29 01:19:23, tkent wrote:
> nit: We don't need to indent the content of namespace {}.

yes, indentation is used for this entire file, so i kept it consistent in this
CL. Hope that's ok.

[sof, 2014-05-29 10:06:08]

On 2014/05/29 01:19:22, tkent wrote:
> wtf LGTM.
> 
> Please fix VisibleSelectionTest.cpp before landing.

Done & verified locally (oilpan trybots are a bit slow atm)

Having part objects that need tracing in a (g)test class is uncommon. Added a
wrapper class to handle it; if someone has a better solution, will followup
later.

[sof, 2014-05-29 10:06:35]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-05-29 10:07:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/299353004/100001

[commit-bot: I haz the power, 2014-05-29 12:42:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9665)

[commit-bot: I haz the power, 2014-05-29 13:31:19]

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

[commit-bot: I haz the power, 2014-05-29 13:31:19]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9665)

[sof, 2014-05-29 19:03:32]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-05-29 19:04:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/299353004/100001

[commit-bot: I haz the power, 2014-05-29 20:03:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9665)

[commit-bot: I haz the power, 2014-05-30 00:32:01]

Change committed as 175079