Move convertViewportToWindow and convertWindowToViewport from

Move convertViewportToWindow and convertWindowToViewport from
WebViewClient to WebWidgetClient.

This is part of a refactor to split WebView/WebWidget.

BUG=583347

[lfg, 2016-08-31 19:50:18]

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

[commit-bot: I haz the power, 2016-08-31 19:50:45]

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

[lfg, 2016-08-31 19:51:03]

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

[lfg, 2016-08-31 19:51:04]

+dcheng, please take a look.

[commit-bot: I haz the power, 2016-08-31 22:41:19]

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

[commit-bot: I haz the power, 2016-08-31 22:41:20]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-09-02 08:57:39]

So looking at this and the other CLs, I wonder if we should just move
WebWidgetClient into public/platform? The main problem is the things that expose
WEbNode, but maybe we can just shove those onto WebView and ignore the problem
for now. That will make it easier to plumb things through, since LocalFrame can
expose a widgetClient() directly to Blink...

https://codereview.chromium.org/2290233007/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2290233007/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2108: void
RenderViewImpl::ConvertViewportToWindowViaWidget(blink::WebRect* rect) {
These all need to move eventually as well, right?

https://codereview.chromium.org/2290233007/diff/1/content/shell/renderer/layo...
File content/shell/renderer/layout_test/blink_test_runner.cc (right):

https://codereview.chromium.org/2290233007/diff/1/content/shell/renderer/layo...
content/shell/renderer/layout_test/blink_test_runner.cc:497: return
content::GetWindowToViewportScale(render_view()->GetMainRenderFrame());
And these would also need to be frame-associated as well, right?

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

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:143: m_webFrameWidget =
WebFrameWidget::create(nullptr, m_webView, m_mainFrame);
Shared workers do layout?

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8114:
WebFrameWidget::create(&webWidgetClient, localRoot);
Would it be better to just make FrameTestHelpers do this automatically?

[lfg, 2016-09-02 17:36:38]

https://codereview.chromium.org/2290233007/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2290233007/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2108: void
RenderViewImpl::ConvertViewportToWindowViaWidget(blink::WebRect* rect) {
On 2016/09/02 08:57:39, dcheng wrote:
> These all need to move eventually as well, right?

Yes, that's right. However, drag and drop depends on these, so this is not
happening soon.

https://codereview.chromium.org/2290233007/diff/1/content/shell/renderer/layo...
File content/shell/renderer/layout_test/blink_test_runner.cc (right):

https://codereview.chromium.org/2290233007/diff/1/content/shell/renderer/layo...
content/shell/renderer/layout_test/blink_test_runner.cc:497: return
content::GetWindowToViewportScale(render_view()->GetMainRenderFrame());
On 2016/09/02 08:57:39, dcheng wrote:
> And these would also need to be frame-associated as well, right?

Yes, I spent a bit of time looking at that, but the callers don't have the
associated frame. For now, I'm just deferring this work to the future, getting
the viewport of the main frame is identical to the old behavior.

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

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:143: m_webFrameWidget =
WebFrameWidget::create(nullptr, m_webView, m_mainFrame);
On 2016/09/02 08:57:39, dcheng wrote:
> Shared workers do layout?

Yes, they do.

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8114:
WebFrameWidget::create(&webWidgetClient, localRoot);
On 2016/09/02 08:57:39, dcheng wrote:
> Would it be better to just make FrameTestHelpers do this automatically?

In this case I didn't think it was worth the effort. FrameTestHelpers already
create the widget when using the helper functions to create child frames, but in
this case we are swapping the main frame. I think this is a singular case, that
won't be reused often.

[dcheng, 2016-09-08 05:25:46]

dcheng@chromium.org changed reviewers:
+ falken@chromium.org

[dcheng, 2016-09-08 05:25:48]

+falken for worker question

https://codereview.chromium.org/2290233007/diff/1/content/shell/renderer/layo...
File content/shell/renderer/layout_test/blink_test_runner.cc (right):

https://codereview.chromium.org/2290233007/diff/1/content/shell/renderer/layo...
content/shell/renderer/layout_test/blink_test_runner.cc:497: return
content::GetWindowToViewportScale(render_view()->GetMainRenderFrame());
On 2016/09/02 17:36:37, lfg wrote:
> On 2016/09/02 08:57:39, dcheng wrote:
> > And these would also need to be frame-associated as well, right?
> 
> Yes, I spent a bit of time looking at that, but the callers don't have the
> associated frame. For now, I'm just deferring this work to the future, getting
> the viewport of the main frame is identical to the old behavior.

Just to confirm: this will explode horribly at some point if we forget to come
back and update this, so we know we need to fix it?

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorHighlight.cpp (right):

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorHighlight.cpp:245: m_scale =
1.f / frameView->getHostWindow()->windowToViewportScalar(1.f, frameView);
This might be a dumb question, but why isn't this just plumbed through
ChromeClient itself anyway? It feels kind of random when we need to pass a
LocalFrame and when we need to pass a Widget.

(Not necessary to fix in this CL, but it doesn't seem great)

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:137: float
ScrollableArea::scrollStep(ScrollGranularity granularity, ScrollbarOrientation
orientation)
Argh const-correctness.

Let's just mark getWidget() const =(

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollableArea.h:63: static int
pixelsPerLineStep(HostWindow*, Widget*);
Oh... I guess it's because of this. So I guess we need to move the frame tree /
WebWidgetClient into public/platform first.

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

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:299: m_webFrameWidget =
WebFrameWidget::create(nullptr, m_webView, m_mainFrame);
I would /really/ like to disallow null clients, because that just means
complexity. +falken, are you OK with just making WebEmbeddedWorkerImpl a no-op
WebWidgetClient?

[falken, 2016-09-08 05:50:39]

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

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:299: m_webFrameWidget =
WebFrameWidget::create(nullptr, m_webView, m_mainFrame);
On 2016/09/08 05:25:47, dcheng wrote:
> I would /really/ like to disallow null clients, because that just means
> complexity. +falken, are you OK with just making WebEmbeddedWorkerImpl a no-op
> WebWidgetClient?

I don't totally understand why webFrameWidget is needed for service or shared
worker, they shouldn't need to be "doing layout" as commented above. But maybe
that's what's happening today as a result of the shadow page hack, and it means
we need webFrameWidget to keep making it work (eventually it should be removed
altogether: https://bugs.chromium.org/p/chromium/issues/detail?id=538751).

If we need a webFrameWidget, I don't have an objection with making
WebEmbeddedWorkerImpl a no-op WebWidgetClient to support it.

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

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:143: m_webFrameWidget =
WebFrameWidget::create(nullptr, m_webView, m_mainFrame);
On 2016/09/02 17:36:38, lfg wrote:
> On 2016/09/02 08:57:39, dcheng wrote:
> > Shared workers do layout?
> 
> Yes, they do.

Can you explain more? They don't need to be doing layout, but maybe it's
happening as a result of the "shadow page" hack. Does something break if we
don't have this webFrameWidget?

[lfg, 2016-09-08 21:40:43]

https://codereview.chromium.org/2290233007/diff/1/content/shell/renderer/layo...
File content/shell/renderer/layout_test/blink_test_runner.cc (right):

https://codereview.chromium.org/2290233007/diff/1/content/shell/renderer/layo...
content/shell/renderer/layout_test/blink_test_runner.cc:497: return
content::GetWindowToViewportScale(render_view()->GetMainRenderFrame());
On 2016/09/08 05:25:47, dcheng wrote:
> 
> Just to confirm: this will explode horribly at some point if we forget to come
> back and update this, so we know we need to fix it?

Yes, if the main_render_frame_ is null this will cause a nullptr dereference (it
should happen on subframe processes).

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorHighlight.cpp (right):

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorHighlight.cpp:245: m_scale =
1.f / frameView->getHostWindow()->windowToViewportScalar(1.f, frameView);
On 2016/09/08 05:25:47, dcheng wrote:
> This might be a dumb question, but why isn't this just plumbed through
> ChromeClient itself anyway? It feels kind of random when we need to pass a
> LocalFrame and when we need to pass a Widget.
> 
> (Not necessary to fix in this CL, but it doesn't seem great)

This is also used in Source/platform/scroll/Scrollbar.cpp, so it can't be
plumbed through ChromeClient with a LocalFrame -- unless we have 2 ways of
plumbing this data, which seems worse.

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:137: float
ScrollableArea::scrollStep(ScrollGranularity granularity, ScrollbarOrientation
orientation)
On 2016/09/08 05:25:47, dcheng wrote:
> Argh const-correctness.
> 
> Let's just mark getWidget() const =(

I tried that before, but FrameView::getWidget returns "this", so it can't be
const.

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollableArea.h:63: static int
pixelsPerLineStep(HostWindow*, Widget*);
On 2016/09/08 05:25:47, dcheng wrote:
> Oh... I guess it's because of this. So I guess we need to move the frame tree
/
> WebWidgetClient into public/platform first.

Acknowledged.

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

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:299: m_webFrameWidget =
WebFrameWidget::create(nullptr, m_webView, m_mainFrame);
On 2016/09/08 05:50:39, falken wrote:
> On 2016/09/08 05:25:47, dcheng wrote:
> > I would /really/ like to disallow null clients, because that just means
> > complexity. +falken, are you OK with just making WebEmbeddedWorkerImpl a
no-op
> > WebWidgetClient?
> 
> I don't totally understand why webFrameWidget is needed for service or shared
> worker, they shouldn't need to be "doing layout" as commented above. But maybe
> that's what's happening today as a result of the shadow page hack, and it
means
> we need webFrameWidget to keep making it work (eventually it should be removed
> altogether: https://bugs.chromium.org/p/chromium/issues/detail?id=538751).
> 
> If we need a webFrameWidget, I don't have an objection with making
> WebEmbeddedWorkerImpl a no-op WebWidgetClient to support it.

I think for now we can use a WebViewFrameWidget, but that's obviously not an
option long term as we continue to remove widget functionality from WebView. I
think we can reevaluate this as we move along to remove WebViewFrameWidget,
perhaps issue 538751 would be fixed by then.

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

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:143: m_webFrameWidget =
WebFrameWidget::create(nullptr, m_webView, m_mainFrame);
On 2016/09/08 05:50:39, falken wrote:
> On 2016/09/02 17:36:38, lfg wrote:
> > On 2016/09/02 08:57:39, dcheng wrote:
> > > Shared workers do layout?
> > 
> > Yes, they do.
> 
> Can you explain more? They don't need to be doing layout, but maybe it's
> happening as a result of the "shadow page" hack. Does something break if we
> don't have this webFrameWidget?

Yes, several tests fail. For example, here's a callstack when running
WorkerTest.SingleSharedWorker:

blink::ChromeClientImpl::windowToViewportScalar
blink::Scrollbar::Scrollbar
blink::Scrollbar::create
blink::FrameView::createScrollbar
blink::FrameView::setHasVerticalScrollbar
blink::FrameView::adjustScrollbarExistence
blink::FrameView::updateScrollbars
blink::FrameView::setScrollbarModes
blink::FrameView::setVerticalScrollbarMode
blink::FrameView::layout
blink::Document::implicitClose
blink::FrameLoader::checkCompleted
blink::FrameLoader::finishedParsing
blink::Document::finishedParsing
blink::HTMLConstructionSite::finishedParsing
blink::HTMLTreeBuilder::finished
blink::HTMLDocumentParser::end
blink::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd
blink::HTMLDocumentParser::prepareToStopParsing
blink::HTMLDocumentParser::attemptToEnd
blink::HTMLDocumentParser::finish
blink::DocumentWriter::end
blink::DocumentLoader::endWriting
blink::DocumentLoader::finishedLoading
blink::DocumentLoader::notifyFinished
blink::Resource::didAddClient
blink::RawResource::didAddClient
blink::Resource::finishPendingClients
blink::Resource::ResourceCallback::runTask

frame->document()->url() is
"file:///.../src/content/test/data/workers/worker_common.js

[falken, 2016-09-09 05:04:28]

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

https://codereview.chromium.org/2290233007/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:143: m_webFrameWidget =
WebFrameWidget::create(nullptr, m_webView, m_mainFrame);
On 2016/09/08 21:40:43, lfg wrote:
> On 2016/09/08 05:50:39, falken wrote:
> > On 2016/09/02 17:36:38, lfg wrote:
> > > On 2016/09/02 08:57:39, dcheng wrote:
> > > > Shared workers do layout?
> > > 
> > > Yes, they do.
> > 
> > Can you explain more? They don't need to be doing layout, but maybe it's
> > happening as a result of the "shadow page" hack. Does something break if we
> > don't have this webFrameWidget?
> 
> Yes, several tests fail. For example, here's a callstack when running
> WorkerTest.SingleSharedWorker:
> 

I see. Yikes! I'll update the shadow page bug to mention FrameView::layout is
for workers because of it.

[lfg, 2016-09-23 16:24:45]

Daniel: ping.

[dcheng, 2016-09-23 17:29:22]

On 2016/09/23 16:24:45, lfg wrote:
> Daniel: ping.

My comments regarding a null client haven't been addressed.

[lfg, 2016-09-23 19:22:23]

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

[commit-bot: I haz the power, 2016-09-23 19:23:03]

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

[lfg, 2016-09-23 19:24:43]

On 2016/09/23 17:29:22, dcheng wrote:
> On 2016/09/23 16:24:45, lfg wrote:
> > Daniel: ping.
> 
> My comments regarding a null client haven't been addressed.

Ah, sorry, I missed that. Done, please take another look.

I've also switched Widget* to Widget&, to be more in line with what we've been
doing on other patches. I'll follow up eventually cleaning up the places that
are different.

[lfg, 2016-09-23 19:26:36]

The CQ bit was unchecked by lfg@chromium.org

[lfg, 2016-09-23 19:27:08]

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

[lfg, 2016-09-23 19:27:17]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-09-23 19:27:46]

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

[lfg, 2017-09-22 06:28:50]

Description was changed from

==========
Move convertViewportToWindow and convertWindowToViewport from
WebViewClient to WebWidgetClient.

This is part of a refactor to split WebView/WebWidget.

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

to

==========
Move convertViewportToWindow and convertWindowToViewport from
WebViewClient to WebWidgetClient.

This is part of a refactor to split WebView/WebWidget.

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