In-place modification of drag related functionality present in LocalFrame

In-place modification of drag related functionality present in LocalFrame

In-Place modification of drag functionality present in LocalFrame.
NodeImage() and DragImageForSelection() are irrelevant to LocalFrame,
and will be moved out from LocalFrame to drag releated files.
This CL does the in-place changes to the above functions to
make movement of code independent of C++ core changes.

BUG=727981
Review-Url: https://codereview.chromium.org/2934233002
Cr-Commit-Position: refs/heads/master@{#480392}
Committed: https://chromium.googlesource.com/chromium/src/+/c9cb177ab255f3978bcb203f0c0867d7eed1a80d

[tanvir, 2017-06-14 13:15:59]

The CQ bit was checked by tanvir.rizvi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 13:16:09]

Dry run: 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, 2017-06-14 13:16:11]

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

[commit-bot: I haz the power, 2017-06-14 13:16:12]

Dry run: No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[tanvir, 2017-06-14 14:19:04]

tanvir.rizvi@samsung.com changed reviewers:
+ xiaochengh@chromium.org

[tanvir, 2017-06-14 14:19:05]

Hi Xiaochengh,
Updated the patch with the core C++ changes.
PTAL!!!

Thanks

[Xiaocheng, 2017-06-14 18:15:31]

Btw, what's the time zone you work in?

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

https://codereview.chromium.org/2934233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:765:
std::unique_ptr<DragImage> LocalFrame::NodeImage(const LocalFrame& frame,
This is different from our common workflow. What we used to do is to change this
function to

std::unique_ptr<DragImage> DataTransfer::NodeImage(const LocalFrame& frame,
Node& node)

but still keep the function impl in LocalFrame.cpp. In this way, the second
patch does absolutely no change to the chunks of code moved to other files.

Besides, you don't need to modify the unit tests again in the second patch.

[tanvir, 2017-06-15 05:08:16]

On 2017/06/14 18:15:31, Xiaocheng wrote:
> Btw, what's the time zone you work in?

Hi, I am working at IST(UTC+5.30).

> 
>
https://codereview.chromium.org/2934233002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):
> 
>
https://codereview.chromium.org/2934233002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/LocalFrame.cpp:765:
> std::unique_ptr<DragImage> LocalFrame::NodeImage(const LocalFrame& frame,
> This is different from our common workflow. What we used to do is to change
this
> function to
> 
> std::unique_ptr<DragImage> DataTransfer::NodeImage(const LocalFrame& frame,
> Node& node)
> 
> but still keep the function impl in LocalFrame.cpp. In this way, the second
> patch does absolutely no change to the chunks of code moved to other files.
> 
> Besides, you don't need to modify the unit tests again in the second patch.

Ohh ok.. Sure.. I will do the changes in this manner. Thanks.

[Xiaocheng, 2017-06-15 18:18:43]

On 2017/06/15 at 05:08:16, tanvir.rizvi wrote:
> On 2017/06/14 18:15:31, Xiaocheng wrote:
> > Btw, what's the time zone you work in?
> 
> Hi, I am working at IST(UTC+5.30).

I'm working in PDT (UTC-7). It seems hard to work together...

If you would like to speed up the iterations, please add tkent@ directly when
you update the patch.

> 
> > 
> >
https://codereview.chromium.org/2934233002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):
> > 
> >
https://codereview.chromium.org/2934233002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/frame/LocalFrame.cpp:765:
> > std::unique_ptr<DragImage> LocalFrame::NodeImage(const LocalFrame& frame,
> > This is different from our common workflow. What we used to do is to change
this
> > function to
> > 
> > std::unique_ptr<DragImage> DataTransfer::NodeImage(const LocalFrame& frame,
> > Node& node)
> > 
> > but still keep the function impl in LocalFrame.cpp. In this way, the second
> > patch does absolutely no change to the chunks of code moved to other files.
> > 
> > Besides, you don't need to modify the unit tests again in the second patch.
> 
> Ohh ok.. Sure.. I will do the changes in this manner. Thanks.

[tanvir, 2017-06-16 10:30:18]

The CQ bit was checked by tanvir.rizvi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 10:30:28]

Dry run: 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, 2017-06-16 10:30:28]

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

[commit-bot: I haz the power, 2017-06-16 10:30:29]

Dry run: No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[tanvir, 2017-06-16 14:00:08]

On 2017/06/15 18:18:43, Xiaocheng wrote:
> On 2017/06/15 at 05:08:16, tanvir.rizvi wrote:
> > On 2017/06/14 18:15:31, Xiaocheng wrote:
> > > Btw, what's the time zone you work in?
> > 
> > Hi, I am working at IST(UTC+5.30).
> 
> I'm working in PDT (UTC-7). It seems hard to work together...
Hi, I will try to make the iterations little faster from my side in the future 
patches and keep in mind of your time zone also. 
> If you would like to speed up the iterations, please add tkent@ directly when
> you update the patch.
> 
> > 
> > > 
> > >
>
https://codereview.chromium.org/2934233002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/2934233002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/frame/LocalFrame.cpp:765:
> > > std::unique_ptr<DragImage> LocalFrame::NodeImage(const LocalFrame& frame,
> > > This is different from our common workflow. What we used to do is to
change
> this
> > > function to
> > > 
> > > std::unique_ptr<DragImage> DataTransfer::NodeImage(const LocalFrame&
frame,
> > > Node& node)
> > > 
> > > but still keep the function impl in LocalFrame.cpp. In this way, the
second
> > > patch does absolutely no change to the chunks of code moved to other
files.
> > > 
> > > Besides, you don't need to modify the unit tests again in the second
patch.
> > 
> > Ohh ok.. Sure.. I will do the changes in this manner. Thanks.

PTAL!!!
Thanks

[tanvir, 2017-06-16 14:00:08]

On 2017/06/15 18:18:43, Xiaocheng wrote:
> On 2017/06/15 at 05:08:16, tanvir.rizvi wrote:
> > On 2017/06/14 18:15:31, Xiaocheng wrote:
> > > Btw, what's the time zone you work in?
> > 
> > Hi, I am working at IST(UTC+5.30).
> 
> I'm working in PDT (UTC-7). It seems hard to work together...
Hi, I will try to make the iterations little faster from my side in the future 
patches and keep in mind of your time zone also. 
> If you would like to speed up the iterations, please add tkent@ directly when
> you update the patch.
> 
> > 
> > > 
> > >
>
https://codereview.chromium.org/2934233002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/2934233002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/frame/LocalFrame.cpp:765:
> > > std::unique_ptr<DragImage> LocalFrame::NodeImage(const LocalFrame& frame,
> > > This is different from our common workflow. What we used to do is to
change
> this
> > > function to
> > > 
> > > std::unique_ptr<DragImage> DataTransfer::NodeImage(const LocalFrame&
frame,
> > > Node& node)
> > > 
> > > but still keep the function impl in LocalFrame.cpp. In this way, the
second
> > > patch does absolutely no change to the chunks of code moved to other
files.
> > > 
> > > Besides, you don't need to modify the unit tests again in the second
patch.
> > 
> > Ohh ok.. Sure.. I will do the changes in this manner. Thanks.

PTAL!!!
Thanks

[tanvir, 2017-06-16 14:02:05]

Description was changed from

==========
In-place modification of drag related functionality present in LocalFrame

In-Place modification of drag functionality present in LocalFrame.
NodeImage() and DragImageForSelection() are irrelevant to LocalFrame,
and will be moved out from LocalFrame to drag releated files.
This CL does the in-place changes to the above functions to
make movement of code independent of C++ core changes.

BUG=727981
==========

to

==========
In-place modification of drag related functionality present in LocalFrame

In-Place modification of drag functionality present in LocalFrame.
NodeImage() and DragImageForSelection() are irrelevant to LocalFrame,
and will be moved out from LocalFrame to drag releated files.
This CL does the in-place changes to the above functions to
make movement of code independent of C++ core changes.

BUG=727981
==========

[tanvir, 2017-06-16 14:02:06]

tanvir.rizvi@samsung.com changed reviewers:
+ shanmuga.m@samsung.com

[Shanmuga Pandi, 2017-06-16 14:03:35]

The CQ bit was checked by shanmuga.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 14:03:49]

Dry run: 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, 2017-06-16 16:38:32]

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

[commit-bot: I haz the power, 2017-06-16 16:38:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xiaocheng, 2017-06-16 18:10:47]

xiaochengh@chromium.org changed reviewers:
+ tkent@chromium.org

[Xiaocheng, 2017-06-16 18:10:48]

+tkent

lgtm with nits

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:117: // Converts from bounds
in CSS space to device space based on the given
nit: It's better to add a note here to indicate this function is static:

// static
// Converts from bounds in ...
// ...
// TODO(tanvir.rizvi): ...
// ...
FloatRect DataTransfer::DeviceSpaceBounds(..) {
...

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:139:
std::unique_ptr<DragImage> DataTransfer::CreateDragImageForFrame(
Ditto.

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:767:
std::unique_ptr<DragImage> DataTransfer::NodeImage(const LocalFrame& frame,
Ditto.

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:776:
std::unique_ptr<DragImage> DragController::DragImageForSelection(
Ditto.

[tkent, 2017-06-19 00:12:45]

lgtm

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/DataTransferTest.cpp (right):

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/DataTransferTest.cpp:1: // Copyright
2017 The Chromium Authors. All rights reserved.
You don't need to update the year.

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> $YEAR should be set to the current year at the time a file is created, and not
changed thereafter.

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/DragControllerTest.cpp (right):

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragControllerTest.cpp:1: // Copyright 2017
The Chromium Authors. All rights reserved.
Though git failed to detect file copy, this file is a copy of
LocalFrameTest.cpp.  So you should not update the year.

[tanvir, 2017-06-19 07:01:33]

Updated for comments. Thanks for review.

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/DataTransferTest.cpp (right):

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/DataTransferTest.cpp:1: // Copyright
2017 The Chromium Authors. All rights reserved.
On 2017/06/19 00:12:45, tkent wrote:
> You don't need to update the year.
> 
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> > $YEAR should be set to the current year at the time a file is created, and
not
> changed thereafter.

Done.

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:117: // Converts from bounds
in CSS space to device space based on the given
On 2017/06/16 18:10:48, Xiaocheng wrote:
> nit: It's better to add a note here to indicate this function is static:
> 
> // static
> // Converts from bounds in ...
> // ...
> // TODO(tanvir.rizvi): ...
> // ...
> FloatRect DataTransfer::DeviceSpaceBounds(..) {
> ...

Done.

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:139:
std::unique_ptr<DragImage> DataTransfer::CreateDragImageForFrame(
On 2017/06/16 18:10:48, Xiaocheng wrote:
> Ditto.

Done.

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:767:
std::unique_ptr<DragImage> DataTransfer::NodeImage(const LocalFrame& frame,
On 2017/06/16 18:10:47, Xiaocheng wrote:
> Ditto.

Done.

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:767:
std::unique_ptr<DragImage> DataTransfer::NodeImage(const LocalFrame& frame,
On 2017/06/16 18:10:47, Xiaocheng wrote:
> Ditto.

Done.

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:776:
std::unique_ptr<DragImage> DragController::DragImageForSelection(
On 2017/06/16 18:10:47, Xiaocheng wrote:
> Ditto.

Done.

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/DragControllerTest.cpp (right):

https://codereview.chromium.org/2934233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragControllerTest.cpp:1: // Copyright 2017
The Chromium Authors. All rights reserved.
On 2017/06/19 00:12:45, tkent wrote:
> Though git failed to detect file copy, this file is a copy of
> LocalFrameTest.cpp.  So you should not update the year.

Done.

[tanvir, 2017-06-19 08:56:40]

The CQ bit was checked by tanvir.rizvi@samsung.com

[tanvir, 2017-06-19 08:56:40]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
xiaochengh@chromium.org
Link to the patchset: https://codereview.chromium.org/2934233002/#ps40001
(title: "updatedPS")

[commit-bot: I haz the power, 2017-06-19 08:56:58]

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, 2017-06-19 10:40:45]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497862600352950,
"parent_rev": "2ad506ac98a4b768f90187af5e35cf51815be6a8", "commit_rev":
"c9cb177ab255f3978bcb203f0c0867d7eed1a80d"}

[commit-bot: I haz the power, 2017-06-19 10:40:58]

Description was changed from

==========
In-place modification of drag related functionality present in LocalFrame

In-Place modification of drag functionality present in LocalFrame.
NodeImage() and DragImageForSelection() are irrelevant to LocalFrame,
and will be moved out from LocalFrame to drag releated files.
This CL does the in-place changes to the above functions to
make movement of code independent of C++ core changes.

BUG=727981
==========

to

==========
In-place modification of drag related functionality present in LocalFrame

In-Place modification of drag functionality present in LocalFrame.
NodeImage() and DragImageForSelection() are irrelevant to LocalFrame,
and will be moved out from LocalFrame to drag releated files.
This CL does the in-place changes to the above functions to
make movement of code independent of C++ core changes.

BUG=727981

Review-Url: https://codereview.chromium.org/2934233002
Cr-Commit-Position: refs/heads/master@{#480392}
Committed:
https://chromium.googlesource.com/chromium/src/+/c9cb177ab255f3978bcb203f0c08...
==========

[commit-bot: I haz the power, 2017-06-19 10:41:01]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/c9cb177ab255f3978bcb203f0c08...