Refactor composited selection code out of WebViewImpl.

Refactor composited selection code out of WebViewImpl.

This CL moves the code for setting and clearing composited selections
out of WebViewImpl, thus making it frame-based. This refactor is also
needed for touch selection editing for the site-isolation iframe work.

BUG=583347, 470662
Committed: https://crrev.com/fad8f6972241f294d6b31a33ded6ce1aa5c2f72b
Cr-Commit-Position: refs/heads/master@{#411340}

[wjmaclean, 2016-08-10 13:45:34]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 13:45:53]

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, 2016-08-10 14:34:17]

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

[commit-bot: I haz the power, 2016-08-10 14:34:18]

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

[wjmaclean, 2016-08-10 18:03:49]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 18:04:20]

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, 2016-08-10 19:07:33]

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

[commit-bot: I haz the power, 2016-08-10 19:07:34]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[wjmaclean, 2016-08-10 20:51:18]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 20:52:12]

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, 2016-08-11 01:10:33]

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

[commit-bot: I haz the power, 2016-08-11 01:10:35]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bokan, 2016-08-11 13:21:36]

Description was changed from

==========
Refactor composited selection code out of WebViewImpl.

This CL moves the code for setting and clearing composited selections
out of WebViewImpl, thus making it frame-based. This refactor is also
needed for touch selection editing for the site-isolation iframe work.

BUG=583347, 470662
==========

to

==========
Refactor composited selection code out of WebViewImpl.

This CL moves the code for setting and clearing composited selections
out of WebViewImpl, thus making it frame-based. This refactor is also
needed for touch selection editing for the site-isolation iframe work.

BUG=583347, 470662
==========

[bokan, 2016-08-11 13:21:36]

bokan@chromium.org changed reviewers:
+ bokan@chromium.org

[bokan, 2016-08-11 14:14:07]

Change looks fine, just some style nits

https://codereview.chromium.org/2230993002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2230993002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1721: if (!localFrame ||
!computeCompositedSelection(*localFrame, selection)) {
These `if` statements can be simplified to:

if (localFrame && computeCompositedSelection) {
    updateCompositedSelection
} else {
    // Clearing the mainframe...
    if (!localFrame)
        localFrame = m_frame->localFrameRoot()

    if (localFrame)
        clearCompositedSelection(localFrame)
}

https://codereview.chromium.org/2230993002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1731: auto mainLocalFrame =
m_frame->localFrameRoot();
IMO, readability is a bit better with s/auto/LocalFrame*

https://codereview.chromium.org/2230993002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2230993002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:831: auto client =
WebLocalFrameImpl::fromFrame(localRoot)->frameWidget()->client();
I don't think `auto` helps in these cases. My read on Chromium policy is that,
while not explicitly disallowed for these cases, we should be judicious with
auto - mainly limited to range for loops and template types where the type is
just boiler plate. Please just use the real types.

[wjmaclean, 2016-08-11 14:35:07]

https://codereview.chromium.org/2230993002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2230993002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1721: if (!localFrame ||
!computeCompositedSelection(*localFrame, selection)) {
On 2016/08/11 14:14:06, bokan wrote:
> These `if` statements can be simplified to:
> 
> if (localFrame && computeCompositedSelection) {
>     updateCompositedSelection
> } else {
>     // Clearing the mainframe...
>     if (!localFrame)
>         localFrame = m_frame->localFrameRoot()
> 
>     if (localFrame)
>         clearCompositedSelection(localFrame)
> }

Done.

Sorry, I had thought about doing this, then forgot about it as I made sure that
all the tests were passing.

https://codereview.chromium.org/2230993002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1731: auto mainLocalFrame =
m_frame->localFrameRoot();
On 2016/08/11 14:14:06, bokan wrote:
> IMO, readability is a bit better with s/auto/LocalFrame*

Done.

https://codereview.chromium.org/2230993002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2230993002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:831: auto client =
WebLocalFrameImpl::fromFrame(localRoot)->frameWidget()->client();
On 2016/08/11 14:14:06, bokan wrote:
> I don't think `auto` helps in these cases. My read on Chromium policy is that,
> while not explicitly disallowed for these cases, we should be judicious with
> auto - mainly limited to range for loops and template types where the type is
> just boiler plate. Please just use the real types.

Done.

[wjmaclean, 2016-08-11 14:35:34]

On 2016/08/11 14:35:07, wjmaclean wrote:
>
https://codereview.chromium.org/2230993002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/2230993002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/FrameView.cpp:1721: if (!localFrame ||
> !computeCompositedSelection(*localFrame, selection)) {
> On 2016/08/11 14:14:06, bokan wrote:
> > These `if` statements can be simplified to:
> > 
> > if (localFrame && computeCompositedSelection) {
> >     updateCompositedSelection
> > } else {
> >     // Clearing the mainframe...
> >     if (!localFrame)
> >         localFrame = m_frame->localFrameRoot()
> > 
> >     if (localFrame)
> >         clearCompositedSelection(localFrame)
> > }
> 
> Done.
> 
> Sorry, I had thought about doing this, then forgot about it as I made sure
that
> all the tests were passing.
> 
>
https://codereview.chromium.org/2230993002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/FrameView.cpp:1731: auto mainLocalFrame =
> m_frame->localFrameRoot();
> On 2016/08/11 14:14:06, bokan wrote:
> > IMO, readability is a bit better with s/auto/LocalFrame*
> 
> Done.
> 
>
https://codereview.chromium.org/2230993002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/2230993002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/ChromeClientImpl.cpp:831: auto client =
> WebLocalFrameImpl::fromFrame(localRoot)->frameWidget()->client();
> On 2016/08/11 14:14:06, bokan wrote:
> > I don't think `auto` helps in these cases. My read on Chromium policy is
that,
> > while not explicitly disallowed for these cases, we should be judicious with
> > auto - mainly limited to range for loops and template types where the type
is
> > just boiler plate. Please just use the real types.
> 
> Done.

PTAL?

[bokan, 2016-08-11 14:40:09]

thanks, lgtm.

https://codereview.chromium.org/2230993002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2230993002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1736:
page->chromeClient().clearCompositedSelection(mainLocalFrame);
IMO, it's a little clearer if there's just one clearCompositedSelection call
below and you set localFrame based on the conditions, but this is fine if you
prefer it.

[wjmaclean, 2016-08-11 14:42:24]

On 2016/08/11 14:40:09, bokan wrote:
> thanks, lgtm.
> 
>
https://codereview.chromium.org/2230993002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/2230993002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/FrameView.cpp:1736:
> page->chromeClient().clearCompositedSelection(mainLocalFrame);
> IMO, it's a little clearer if there's just one clearCompositedSelection call
> below and you set localFrame based on the conditions, but this is fine if you
> prefer it.

No worries, I can change that ...

[wjmaclean, 2016-08-11 14:45:55]

The CQ bit was checked by wjmaclean@chromium.org

[wjmaclean, 2016-08-11 14:45:55]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2230993002/#ps100001
(title: "Use single localFrame clearing.")

[commit-bot: I haz the power, 2016-08-11 14:46:08]

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-08-11 15:58:58]

Description was changed from

==========
Refactor composited selection code out of WebViewImpl.

This CL moves the code for setting and clearing composited selections
out of WebViewImpl, thus making it frame-based. This refactor is also
needed for touch selection editing for the site-isolation iframe work.

BUG=583347, 470662
==========

to

==========
Refactor composited selection code out of WebViewImpl.

This CL moves the code for setting and clearing composited selections
out of WebViewImpl, thus making it frame-based. This refactor is also
needed for touch selection editing for the site-isolation iframe work.

BUG=583347, 470662
==========

[commit-bot: I haz the power, 2016-08-11 15:58:59]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-11 16:02:15]

Description was changed from

==========
Refactor composited selection code out of WebViewImpl.

This CL moves the code for setting and clearing composited selections
out of WebViewImpl, thus making it frame-based. This refactor is also
needed for touch selection editing for the site-isolation iframe work.

BUG=583347, 470662
==========

to

==========
Refactor composited selection code out of WebViewImpl.

This CL moves the code for setting and clearing composited selections
out of WebViewImpl, thus making it frame-based. This refactor is also
needed for touch selection editing for the site-isolation iframe work.

BUG=583347, 470662

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

[commit-bot: I haz the power, 2016-08-11 16:02:16]

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