Move setWindowRect and windowRect calls from WebViewClient to WebWidgetClient.

Move setWindowRect and windowRect calls from WebViewClient
to WebWidgetClient.

This change also renames ChromeClient::windowRect to
ChromeClient::rootWindowRect to avoid confusion with
WebWidgetClient::windowRect, since they are used for
different purposes.

This change is part of WebView/WebWidget split refactor.

BUG=583347
Committed: https://crrev.com/5557376bb5137162c103b765680acc8567751e26
Cr-Commit-Position: refs/heads/master@{#412667}

[lfg, 2016-07-25 18:33:24]

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

[commit-bot: I haz the power, 2016-07-25 18:34:02]

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-07-25 20:00:39]

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

[commit-bot: I haz the power, 2016-07-25 20:00:40]

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...)

[lfg, 2016-07-25 20:39:44]

Description was changed from

==========
WIP
==========

to

==========
Move setWindowRect and windowRect calls from WebViewClient to WebWidgetClient.

This change also renames ChromeClient::windowRect to
ChromeClient::rootWindowRect to avoid confusion with
WebWidgetClient::windowRect, since they are used for different purposes.

This change is part of WebView/WebWidget split refactor.

BUG=583347
==========

[lfg, 2016-07-25 20:40:15]

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

[lfg, 2016-07-25 20:40:16]

+bokan, please take a look.

[bokan, 2016-07-26 23:17:05]

Please wrap description at 80 cols.

https://codereview.chromium.org/2177333002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2177333002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:190: CHECK(frame ==
m_webView->mainFrameImpl()->frame());
Wouldn't it be easier if ChromeClient implementations maintained a pointer to
their corresponding WebWidget instead? This seems more error prone and harder to
read at the call sites (plus, other impls like WebPagePopup don't use the
param). Or is this a temporary state while we untangle View from Widget?

[lfg, 2016-07-27 14:54:50]

Description was changed from

==========
Move setWindowRect and windowRect calls from WebViewClient to WebWidgetClient.

This change also renames ChromeClient::windowRect to
ChromeClient::rootWindowRect to avoid confusion with
WebWidgetClient::windowRect, since they are used for different purposes.

This change is part of WebView/WebWidget split refactor.

BUG=583347
==========

to

==========
Move setWindowRect and windowRect calls from WebViewClient
to WebWidgetClient.

This change also renames ChromeClient::windowRect to
ChromeClient::rootWindowRect to avoid confusion with
WebWidgetClient::windowRect, since they are used for
different purposes.

This change is part of WebView/WebWidget split refactor.

BUG=583347
==========

[lfg, 2016-07-27 15:06:10]

On 2016/07/26 23:17:05, bokan wrote:
> Please wrap description at 80 cols.

Done.

>
https://codereview.chromium.org/2177333002/diff/1/third_party/WebKit/Source/w...
> File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/2177333002/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/web/ChromeClientImpl.cpp:190: CHECK(frame ==
> m_webView->mainFrameImpl()->frame());
> Wouldn't it be easier if ChromeClient implementations maintained a pointer to
> their corresponding WebWidget instead? This seems more error prone and harder
to
> read at the call sites (plus, other impls like WebPagePopup don't use the
> param). Or is this a temporary state while we untangle View from Widget?

The popup is a special case since it is always local, so it can cache its own
WebWidgetClient. The problem of caching the widget in ChromeClientImpl is that
we can have multiple widgets, one per local root, so we would need a map between
frames<->WebWidgets. Local roots can also be created and destroyed whenever a
frame is created/destroyed, so we would also need a way to update/maintain this
map whenever this happens. I feel that the negatives outweight the benefits
here.

[bokan, 2016-07-27 15:43:53]

On 2016/07/27 15:06:10, lfg wrote:
> On 2016/07/26 23:17:05, bokan wrote:
> > Please wrap description at 80 cols.
> 
> Done.
> 
> >
>
https://codereview.chromium.org/2177333002/diff/1/third_party/WebKit/Source/w...
> > File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2177333002/diff/1/third_party/WebKit/Source/w...
> > third_party/WebKit/Source/web/ChromeClientImpl.cpp:190: CHECK(frame ==
> > m_webView->mainFrameImpl()->frame());
> > Wouldn't it be easier if ChromeClient implementations maintained a pointer
to
> > their corresponding WebWidget instead? This seems more error prone and
harder
> to
> > read at the call sites (plus, other impls like WebPagePopup don't use the
> > param). Or is this a temporary state while we untangle View from Widget?
> 
> The popup is a special case since it is always local, so it can cache its own
> WebWidgetClient. The problem of caching the widget in ChromeClientImpl is that
> we can have multiple widgets, one per local root, so we would need a map
between
> frames<->WebWidgets. Local roots can also be created and destroyed whenever a
> frame is created/destroyed, so we would also need a way to update/maintain
this
> map whenever this happens. I feel that the negatives outweight the benefits
> here.

Ok, lgtm.

[lfg, 2016-07-27 16:28:23]

lfg@chromium.org changed reviewers:
+ avi@chromium.org, dcheng@chromium.org

[lfg, 2016-07-27 16:28:24]

dcheng@chromium.org: Please review changes in 

WebKit/public/

avi@chromium.org: Please review changes in 

content/

[Avi (use Gerrit), 2016-07-27 16:39:48]

content lgtm

[dcheng, 2016-07-29 08:31:11]

https://codereview.chromium.org/2177333002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2177333002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:190: CHECK(frame ==
m_webView->mainFrameImpl()->frame());
On 2016/07/26 23:17:05, bokan wrote:
> Wouldn't it be easier if ChromeClient implementations maintained a pointer to
> their corresponding WebWidget instead? This seems more error prone and harder
to
> read at the call sites (plus, other impls like WebPagePopup don't use the
> param). Or is this a temporary state while we untangle View from Widget?

Why CHECK()? In general, it's better to avoid CHECKs without a very good reason.

[lfg, 2016-07-29 15:34:55]

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

[commit-bot: I haz the power, 2016-07-29 15:35:15]

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

[lfg, 2016-07-29 15:36:06]

On 2016/07/29 08:31:11, dcheng (OOO Aug 1 - Aug 11) wrote:
> Why CHECK()? In general, it's better to avoid CHECKs without a very good
reason.

I switched it to a DCHECK(), it's just to avoid using it on anything other than
the main frame, since it's not implemented for other local roots.

[commit-bot: I haz the power, 2016-07-29 16:38:33]

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

[commit-bot: I haz the power, 2016-07-29 16:38:34]

Dry run: This issue passed the CQ dry run.

[lfg, 2016-08-12 18:41:17]

dcheng: ping

[dcheng, 2016-08-15 18:22:44]

https://codereview.chromium.org/2177333002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/2177333002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ChromeClient.h:88: void
setWindowRectWithAdjustment(const IntRect&, LocalFrame*);
Let's make this a LocalFrame&.

https://codereview.chromium.org/2177333002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ChromeClient.h:291: virtual void
setWindowRect(const IntRect&, LocalFrame*) = 0;
Ditto

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

https://codereview.chromium.org/2177333002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:190: DCHECK(frame ==
m_webView->mainFrameImpl()->frame());
Nit: DCHECK_EQ

https://codereview.chromium.org/2177333002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:193: if (client)
Do we need to nullcheck this? It looks like the calls to this are generally
guarded and make sure the FrameHost is still live.

[lfg, 2016-08-16 21:01:57]

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

[lfg, 2016-08-16 21:02:13]

https://codereview.chromium.org/2177333002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/2177333002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ChromeClient.h:88: void
setWindowRectWithAdjustment(const IntRect&, LocalFrame*);
On 2016/08/15 18:22:44, dcheng wrote:
> Let's make this a LocalFrame&.

I know this is blink, but this goes against the google style guide of not
passing non-const refs. Can you explain the reasoning for this?

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

https://codereview.chromium.org/2177333002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:190: DCHECK(frame ==
m_webView->mainFrameImpl()->frame());
On 2016/08/15 18:22:44, dcheng wrote:
> Nit: DCHECK_EQ

Done.

https://codereview.chromium.org/2177333002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:193: if (client)
On 2016/08/15 18:22:44, dcheng wrote:
> Do we need to nullcheck this? It looks like the calls to this are generally
> guarded and make sure the FrameHost is still live.

It's been a while, so I don't remember exactly, but I believe some tests don't
guarantee the existence of the client, but I'll give it a try.

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

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

[dcheng, 2016-08-16 21:03:46]

https://codereview.chromium.org/2177333002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/2177333002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ChromeClient.h:88: void
setWindowRectWithAdjustment(const IntRect&, LocalFrame*);
On 2016/08/16 21:02:13, lfg wrote:
> On 2016/08/15 18:22:44, dcheng wrote:
> > Let's make this a LocalFrame&.
> 
> I know this is blink, but this goes against the google style guide of not
> passing non-const refs. Can you explain the reasoning for this?

We have a strong convention of passing never-null pointers as mutable refs. Yes,
it is contrary to the Google style guide, but this is one of the Blink
exceptions in place to enable the overall style merge.

[commit-bot: I haz the power, 2016-08-16 22:38:07]

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

[commit-bot: I haz the power, 2016-08-16 22:38:09]

Dry run: This issue passed the CQ dry run.

[lfg, 2016-08-16 22:49:05]

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

[commit-bot: I haz the power, 2016-08-16 22:49:34]

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-17 01:18:22]

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

[commit-bot: I haz the power, 2016-08-17 01:18:23]

Dry run: Exceeded global retry quota

[lfg, 2016-08-17 15:43:28]

On 2016/08/16 21:03:46, dcheng wrote:
>
https://codereview.chromium.org/2177333002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/page/ChromeClient.h (right):
> 
>
https://codereview.chromium.org/2177333002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/page/ChromeClient.h:88: void
> setWindowRectWithAdjustment(const IntRect&, LocalFrame*);
> On 2016/08/16 21:02:13, lfg wrote:
> > On 2016/08/15 18:22:44, dcheng wrote:
> > > Let's make this a LocalFrame&.
> > 
> > I know this is blink, but this goes against the google style guide of not
> > passing non-const refs. Can you explain the reasoning for this?
> 
> We have a strong convention of passing never-null pointers as mutable refs.
Yes,
> it is contrary to the Google style guide, but this is one of the Blink
> exceptions in place to enable the overall style merge.

Done. Please take another look.

[dcheng, 2016-08-17 22:10:50]

LGTM based on the diff between PS4 and PS6.

[lfg, 2016-08-17 22:11:50]

The CQ bit was checked by lfg@chromium.org

[lfg, 2016-08-17 22:11:50]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2177333002/#ps100001
(title: "switch to refs")

[commit-bot: I haz the power, 2016-08-17 22:12:15]

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-17 22:18:22]

Description was changed from

==========
Move setWindowRect and windowRect calls from WebViewClient
to WebWidgetClient.

This change also renames ChromeClient::windowRect to
ChromeClient::rootWindowRect to avoid confusion with
WebWidgetClient::windowRect, since they are used for
different purposes.

This change is part of WebView/WebWidget split refactor.

BUG=583347
==========

to

==========
Move setWindowRect and windowRect calls from WebViewClient
to WebWidgetClient.

This change also renames ChromeClient::windowRect to
ChromeClient::rootWindowRect to avoid confusion with
WebWidgetClient::windowRect, since they are used for
different purposes.

This change is part of WebView/WebWidget split refactor.

BUG=583347
==========

[commit-bot: I haz the power, 2016-08-17 22:18:23]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-17 22:24:29]

Description was changed from

==========
Move setWindowRect and windowRect calls from WebViewClient
to WebWidgetClient.

This change also renames ChromeClient::windowRect to
ChromeClient::rootWindowRect to avoid confusion with
WebWidgetClient::windowRect, since they are used for
different purposes.

This change is part of WebView/WebWidget split refactor.

BUG=583347
==========

to

==========
Move setWindowRect and windowRect calls from WebViewClient
to WebWidgetClient.

This change also renames ChromeClient::windowRect to
ChromeClient::rootWindowRect to avoid confusion with
WebWidgetClient::windowRect, since they are used for
different purposes.

This change is part of WebView/WebWidget split refactor.

BUG=583347

Committed: https://crrev.com/5557376bb5137162c103b765680acc8567751e26
Cr-Commit-Position: refs/heads/master@{#412667}
==========

[commit-bot: I haz the power, 2016-08-17 22:24:31]

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