Make LocalFrame::nodeImage() to take an image for the element with :-webkit-drag pseudo class

Make LocalFrame::nodeImage() to take an image for the element with :-webkit-drag pseudo class

This patch makes |LocalFrame::nodeImage()| to take image for element with
":-webkit-drag" pseudo class by re-factoring |DragImageBuilder|.

Before this patch, |nodeImage()| doesn't take ":-webkit-drag" and paints
dragged element with dirty layout tree, e.g. |LayoutObject::updateDragState()|
sets "Needs Style Recalc" for ":-webkit-drag". This causes layout tree update
in |BlockPainter::paint()| due by |LayoutObject::hasCursor()| which calls
|Document::updateStyleAndLayoutTreeIgnorePendingStylesheets()| via
|isEditingPosition()|.

This patch is a preparation of hoisting update layout for |isEditablePosition()|
, http://crrev.com/2089993003, which is called during layout tree building for
checking whether |LayoutBlock|
have caret or not.

BUG=623005
TEST=run-webkit-tests --gtest_filter=LocalFrameTest.nodeImage*

Committed: https://crrev.com/0aad49f542b1119bf3ef413ca9c1c08d2aecb699
Cr-Commit-Position: refs/heads/master@{#402100}

[yosin_UTC9, 2016-06-23 09:53:49]

Description was changed from

==========
2016-06-23T15:48:19

BUG=
==========

to

==========
Make LocalFrame::nodeImage() to take image for element with :drag pseudo class

This patch makes |LocalFrame::nodeImage()| to take image for element with
":drag" pseudo class by re-factoring |DragImageBuilder|.

This patch is a preparation of hoisting update layout for |isEditablePosition()|
which is called during layout tree building for checking whether |LayoutBlock|
have caret or not.

BUG=n/a
==========

[yosin_UTC9, 2016-06-23 09:55:02]

yosin@chromium.org changed reviewers:
+ pdr@chromium.org

[yosin_UTC9, 2016-06-23 09:55:03]

PTAL

[pdr., 2016-06-23 20:29:13]

Hey yosin, this patch looks reasonable but I don't quite understand the purpose.
Because LayoutObjects can get destroyed in many places, why refactor towards
passing LayoutObjects instead of Node which is a little more stable?

https://codereview.chromium.org/2094653002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2094653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:154: LayoutObject* const
m_draggedLayoutObject;
Why "LayoutObject* const" instead of "const LayoutObject*"?

https://codereview.chromium.org/2094653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:606: // TODO(yosin): We
should handle pseudo-class ":drag" as similar as
I was a little confused by ":drag", I think we use ":-webkit-drag"

https://codereview.chromium.org/2094653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:615:
layoutObject->updateDragState(true);
Previously, calling updateDragState was the responsibility of DragImageBuilder
but now it's split in two places. Can these be unified (either all in this
function or all in DragImageBuilder)? This will make it harder to forget to call
updateDragState.

One option would be to just update the return statement:
auto image = dragImageBuilder.createImage();
if (layoutObject)
    layoutObject->updateDragState(false);
return image;

[yosin_UTC9, 2016-06-24 05:51:46]

Description was changed from

==========
Make LocalFrame::nodeImage() to take image for element with :drag pseudo class

This patch makes |LocalFrame::nodeImage()| to take image for element with
":drag" pseudo class by re-factoring |DragImageBuilder|.

This patch is a preparation of hoisting update layout for |isEditablePosition()|
which is called during layout tree building for checking whether |LayoutBlock|
have caret or not.

BUG=n/a
==========

to

==========
Make LocalFrame::nodeImage() to take image for element with :-webkit-drag pseudo
class

This patch makes |LocalFrame::nodeImage()| to take image for element with
":-webkit-drag" pseudo class by re-factoring |DragImageBuilder|.

Before this patch, |nodeImage()| doesn't take ":-webkit-drag" and paints
dragged element with dirty layout tree, e.g. |LayoutObject::updateDragState()|
sets "Needs Style Recalc" for ":-webkit-drag". This causes layout tree update
in |BlockPainter::paint()| due by |LayoutObject::hasCursor()| which calls
|Document::updateStyleAndLayoutTreeIgnorePendingStylesheets()| via
|isEditingPosition()|.

This patch is a preparation of hoisting update layout for |isEditablePosition()|
which is called during layout tree building for checking whether |LayoutBlock|
have caret or not.

BUG=n/a
TEST=run-webkit-tests --gtest_filter=LocalFrameTest.nodeImage*
==========

[yosin_UTC9, 2016-06-24 05:58:24]

Description was changed from

==========
Make LocalFrame::nodeImage() to take image for element with :-webkit-drag pseudo
class

This patch makes |LocalFrame::nodeImage()| to take image for element with
":-webkit-drag" pseudo class by re-factoring |DragImageBuilder|.

Before this patch, |nodeImage()| doesn't take ":-webkit-drag" and paints
dragged element with dirty layout tree, e.g. |LayoutObject::updateDragState()|
sets "Needs Style Recalc" for ":-webkit-drag". This causes layout tree update
in |BlockPainter::paint()| due by |LayoutObject::hasCursor()| which calls
|Document::updateStyleAndLayoutTreeIgnorePendingStylesheets()| via
|isEditingPosition()|.

This patch is a preparation of hoisting update layout for |isEditablePosition()|
which is called during layout tree building for checking whether |LayoutBlock|
have caret or not.

BUG=n/a
TEST=run-webkit-tests --gtest_filter=LocalFrameTest.nodeImage*
==========

to

==========
Make LocalFrame::nodeImage() to take image for element with :-webkit-drag pseudo
class

This patch makes |LocalFrame::nodeImage()| to take image for element with
":-webkit-drag" pseudo class by re-factoring |DragImageBuilder|.

Before this patch, |nodeImage()| doesn't take ":-webkit-drag" and paints
dragged element with dirty layout tree, e.g. |LayoutObject::updateDragState()|
sets "Needs Style Recalc" for ":-webkit-drag". This causes layout tree update
in |BlockPainter::paint()| due by |LayoutObject::hasCursor()| which calls
|Document::updateStyleAndLayoutTreeIgnorePendingStylesheets()| via
|isEditingPosition()|.

This patch is a preparation of hoisting update layout for |isEditablePosition()|
, http://crrev.com/2089993003, which is called during layout tree building for
checking whether |LayoutBlock|
have caret or not.

BUG=n/a
TEST=run-webkit-tests --gtest_filter=LocalFrameTest.nodeImage*
==========

[yosin_UTC9, 2016-06-24 06:02:09]

Description was changed from

==========
Make LocalFrame::nodeImage() to take image for element with :-webkit-drag pseudo
class

This patch makes |LocalFrame::nodeImage()| to take image for element with
":-webkit-drag" pseudo class by re-factoring |DragImageBuilder|.

Before this patch, |nodeImage()| doesn't take ":-webkit-drag" and paints
dragged element with dirty layout tree, e.g. |LayoutObject::updateDragState()|
sets "Needs Style Recalc" for ":-webkit-drag". This causes layout tree update
in |BlockPainter::paint()| due by |LayoutObject::hasCursor()| which calls
|Document::updateStyleAndLayoutTreeIgnorePendingStylesheets()| via
|isEditingPosition()|.

This patch is a preparation of hoisting update layout for |isEditablePosition()|
, http://crrev.com/2089993003, which is called during layout tree building for
checking whether |LayoutBlock|
have caret or not.

BUG=n/a
TEST=run-webkit-tests --gtest_filter=LocalFrameTest.nodeImage*
==========

to

==========
Make LocalFrame::nodeImage() to take an image for the element with :-webkit-drag
pseudo class

This patch makes |LocalFrame::nodeImage()| to take image for element with
":-webkit-drag" pseudo class by re-factoring |DragImageBuilder|.

Before this patch, |nodeImage()| doesn't take ":-webkit-drag" and paints
dragged element with dirty layout tree, e.g. |LayoutObject::updateDragState()|
sets "Needs Style Recalc" for ":-webkit-drag". This causes layout tree update
in |BlockPainter::paint()| due by |LayoutObject::hasCursor()| which calls
|Document::updateStyleAndLayoutTreeIgnorePendingStylesheets()| via
|isEditingPosition()|.

This patch is a preparation of hoisting update layout for |isEditablePosition()|
, http://crrev.com/2089993003, which is called during layout tree building for
checking whether |LayoutBlock|
have caret or not.

BUG=n/a
TEST=run-webkit-tests --gtest_filter=LocalFrameTest.nodeImage*
==========

[yosin_UTC9, 2016-06-24 06:02:57]

PTAL

- Update description
- Introduce NodeImageBuilder class to encapsulate |updateDragState()|
- Introduce gTest for regression and ":-webkit-drag" pseudo class

>LayoutObjects can get destroyed in many places,

This is the goal of this patch. During painting, |layoutObjects| should not be
destroyed.
I expect painting phase doesn't destroy layout object. Is my assumption false?

If you see layout object destruction during paint phase, one of reasons is in
editing about
caret management. |BlockPainter::paint()| calls |LayoutObject::hasCaret()| and
it can update
layout tree due by |isEditingPosition()| which calls
|Document::updateStyleAndLayoutTreeIgnorePendingStylesheets()| and layout tree
is updated. This can be happened in |nodeImage()| since
|updateDragState()| sets layout tree dirty.

This patch blocks http://crrev.com/2089993003

https://codereview.chromium.org/2094653002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2094653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:154: LayoutObject* const
m_draggedLayoutObject;
On 2016/06/23 at 20:29:13, pdr. wrote:
> Why "LayoutObject* const" instead of "const LayoutObject*"?

|m_layoutObject| is read-only member and |LayoutObject::updateDragState()| is
non-const.

https://codereview.chromium.org/2094653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:606: // TODO(yosin): We
should handle pseudo-class ":drag" as similar as
On 2016/06/23 at 20:29:13, pdr. wrote:
> I was a little confused by ":drag", I think we use ":-webkit-drag"

Done.

https://codereview.chromium.org/2094653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:615:
layoutObject->updateDragState(true);
On 2016/06/23 at 20:29:13, pdr. wrote:
> Previously, calling updateDragState was the responsibility of DragImageBuilder
but now it's split in two places. Can these be unified (either all in this
function or all in DragImageBuilder)? This will make it harder to forget to call
updateDragState.
> 
> One option would be to just update the return statement:
> auto image = dragImageBuilder.createImage();
> if (layoutObject)
>     layoutObject->updateDragState(false);
> return image;

I introduce |NodeImageBuilder| class and make its ctor/dtor manage
|updateDragState|.

[yosin_UTC9, 2016-06-24 07:47:55]

Description was changed from

==========
Make LocalFrame::nodeImage() to take an image for the element with :-webkit-drag
pseudo class

This patch makes |LocalFrame::nodeImage()| to take image for element with
":-webkit-drag" pseudo class by re-factoring |DragImageBuilder|.

Before this patch, |nodeImage()| doesn't take ":-webkit-drag" and paints
dragged element with dirty layout tree, e.g. |LayoutObject::updateDragState()|
sets "Needs Style Recalc" for ":-webkit-drag". This causes layout tree update
in |BlockPainter::paint()| due by |LayoutObject::hasCursor()| which calls
|Document::updateStyleAndLayoutTreeIgnorePendingStylesheets()| via
|isEditingPosition()|.

This patch is a preparation of hoisting update layout for |isEditablePosition()|
, http://crrev.com/2089993003, which is called during layout tree building for
checking whether |LayoutBlock|
have caret or not.

BUG=n/a
TEST=run-webkit-tests --gtest_filter=LocalFrameTest.nodeImage*
==========

to

==========
Make LocalFrame::nodeImage() to take an image for the element with :-webkit-drag
pseudo class

This patch makes |LocalFrame::nodeImage()| to take image for element with
":-webkit-drag" pseudo class by re-factoring |DragImageBuilder|.

Before this patch, |nodeImage()| doesn't take ":-webkit-drag" and paints
dragged element with dirty layout tree, e.g. |LayoutObject::updateDragState()|
sets "Needs Style Recalc" for ":-webkit-drag". This causes layout tree update
in |BlockPainter::paint()| due by |LayoutObject::hasCursor()| which calls
|Document::updateStyleAndLayoutTreeIgnorePendingStylesheets()| via
|isEditingPosition()|.

This patch is a preparation of hoisting update layout for |isEditablePosition()|
, http://crrev.com/2089993003, which is called during layout tree building for
checking whether |LayoutBlock|
have caret or not.

BUG=623005
TEST=run-webkit-tests --gtest_filter=LocalFrameTest.nodeImage*
==========

[pdr., 2016-06-24 17:53:55]

On 2016/06/24 at 06:02:57, yosin wrote:
> PTAL
> 
> - Update description
> - Introduce NodeImageBuilder class to encapsulate |updateDragState()|
> - Introduce gTest for regression and ":-webkit-drag" pseudo class
> 
> >LayoutObjects can get destroyed in many places,
> 
> This is the goal of this patch. During painting, |layoutObjects| should not be
destroyed.
> I expect painting phase doesn't destroy layout object. Is my assumption false?
> 
> If you see layout object destruction during paint phase, one of reasons is in
editing about
> caret management. |BlockPainter::paint()| calls |LayoutObject::hasCaret()| and
it can update
> layout tree due by |isEditingPosition()| which calls
|Document::updateStyleAndLayoutTreeIgnorePendingStylesheets()| and layout tree
is updated. This can be happened in |nodeImage()| since
> |updateDragState()| sets layout tree dirty.
> 
> This patch blocks http://crrev.com/2089993003
> 
>
https://codereview.chromium.org/2094653002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):
> 
>
https://codereview.chromium.org/2094653002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/LocalFrame.cpp:154: LayoutObject* const
m_draggedLayoutObject;
> On 2016/06/23 at 20:29:13, pdr. wrote:
> > Why "LayoutObject* const" instead of "const LayoutObject*"?
> 
> |m_layoutObject| is read-only member and |LayoutObject::updateDragState()| is
non-const.
> 
>
https://codereview.chromium.org/2094653002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/LocalFrame.cpp:606: // TODO(yosin): We
should handle pseudo-class ":drag" as similar as
> On 2016/06/23 at 20:29:13, pdr. wrote:
> > I was a little confused by ":drag", I think we use ":-webkit-drag"
> 
> Done.
> 
>
https://codereview.chromium.org/2094653002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/LocalFrame.cpp:615:
layoutObject->updateDragState(true);
> On 2016/06/23 at 20:29:13, pdr. wrote:
> > Previously, calling updateDragState was the responsibility of
DragImageBuilder but now it's split in two places. Can these be unified (either
all in this function or all in DragImageBuilder)? This will make it harder to
forget to call updateDragState.
> > 
> > One option would be to just update the return statement:
> > auto image = dragImageBuilder.createImage();
> > if (layoutObject)
> >     layoutObject->updateDragState(false);
> > return image;
> 
> I introduce |NodeImageBuilder| class and make its ctor/dtor manage
|updateDragState|.

Thanks, LGTM

[yosin_UTC9, 2016-06-27 01:42:20]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2016-06-27 01:42:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 02:54:30]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-27 02:56:38]

Description was changed from

==========
Make LocalFrame::nodeImage() to take an image for the element with :-webkit-drag
pseudo class

This patch makes |LocalFrame::nodeImage()| to take image for element with
":-webkit-drag" pseudo class by re-factoring |DragImageBuilder|.

Before this patch, |nodeImage()| doesn't take ":-webkit-drag" and paints
dragged element with dirty layout tree, e.g. |LayoutObject::updateDragState()|
sets "Needs Style Recalc" for ":-webkit-drag". This causes layout tree update
in |BlockPainter::paint()| due by |LayoutObject::hasCursor()| which calls
|Document::updateStyleAndLayoutTreeIgnorePendingStylesheets()| via
|isEditingPosition()|.

This patch is a preparation of hoisting update layout for |isEditablePosition()|
, http://crrev.com/2089993003, which is called during layout tree building for
checking whether |LayoutBlock|
have caret or not.

BUG=623005
TEST=run-webkit-tests --gtest_filter=LocalFrameTest.nodeImage*
==========

to

==========
Make LocalFrame::nodeImage() to take an image for the element with :-webkit-drag
pseudo class

This patch makes |LocalFrame::nodeImage()| to take image for element with
":-webkit-drag" pseudo class by re-factoring |DragImageBuilder|.

Before this patch, |nodeImage()| doesn't take ":-webkit-drag" and paints
dragged element with dirty layout tree, e.g. |LayoutObject::updateDragState()|
sets "Needs Style Recalc" for ":-webkit-drag". This causes layout tree update
in |BlockPainter::paint()| due by |LayoutObject::hasCursor()| which calls
|Document::updateStyleAndLayoutTreeIgnorePendingStylesheets()| via
|isEditingPosition()|.

This patch is a preparation of hoisting update layout for |isEditablePosition()|
, http://crrev.com/2089993003, which is called during layout tree building for
checking whether |LayoutBlock|
have caret or not.

BUG=623005
TEST=run-webkit-tests --gtest_filter=LocalFrameTest.nodeImage*

Committed: https://crrev.com/0aad49f542b1119bf3ef413ca9c1c08d2aecb699
Cr-Commit-Position: refs/heads/master@{#402100}
==========

[commit-bot: I haz the power, 2016-06-27 02:56:39]

Patchset 2 (id:??) landed as
https://crrev.com/0aad49f542b1119bf3ef413ca9c1c08d2aecb699
Cr-Commit-Position: refs/heads/master@{#402100}