Remove WebViewClient::showVirtualKeyboardOnElementFocus()

Remove WebViewClient::showVirtualKeyboardOnElementFocus()

When an <input> is focused by user gesture, ChildClientImpl calls this
method on RenderViewImpl to make sure the proper TextInputState to show
IME keyboard is sent to the browser (used for Android). However, for
OOPIFs it does not make sense to call this on RenderView and rather, the
call should be directed to the RenderWidget corresponding to the local
root of the focused element/frame.

This CL removes the current override from RenderViewImpl/WebViewClient
and directs the call to the frameWidget() of the focused frame instead.

This CL also adds a WebFrameTest.

BUG=578168, 682009
Review-Url: https://codereview.chromium.org/2709813007
Cr-Commit-Position: refs/heads/master@{#455820}
Committed: https://chromium.googlesource.com/chromium/src/+/1e2d08a7fc2db1297a7f2a28c40659af534c309e

[EhsanK, 2017-02-21 23:10:23]

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

[commit-bot: I haz the power, 2017-02-21 23:12:17]

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

[EhsanK, 2017-02-22 00:50:41]

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

[commit-bot: I haz the power, 2017-02-22 00:52:14]

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

[EhsanK, 2017-02-22 02:06:21]

ekaramad@chromium.org changed reviewers:
+ creis@chromium.org, dcheng@chromium.org

[EhsanK, 2017-02-22 02:06:22]

Please take a look! Thanks

dcheng@: blink changes.
creis@: content/ reviews.

[EhsanK, 2017-02-22 02:06:47]

Description was changed from

==========
Remove WebViewClient::showVirtualKeyboardOnElementFocus()

When an <input> is focused by user gesture, ChildClientImpl calls this
method on RenderViewImpl to make sure the proper TextInputState to show
IME keyboard is sent to the browser (used for Android). However, for
OOPIFs it does not make sense to call this on RenderView and rather, the
call should be directed to the RenderWidget corresponding to the local
root of the focused element/frame.

This CL removes the current override from RenderViewImpl/WebViewClient
and directs the call to the frameWidget() of the focused frame instead.

This CL also adds a WebFrameTest.

BUG=578168, 682009
==========

to

==========
Remove WebViewClient::showVirtualKeyboardOnElementFocus()

When an <input> is focused by user gesture, ChildClientImpl calls this
method on RenderViewImpl to make sure the proper TextInputState to show
IME keyboard is sent to the browser (used for Android). However, for
OOPIFs it does not make sense to call this on RenderView and rather, the
call should be directed to the RenderWidget corresponding to the local
root of the focused element/frame.

This CL removes the current override from RenderViewImpl/WebViewClient
and directs the call to the frameWidget() of the focused frame instead.

This CL also adds a WebFrameTest.

BUG=578168, 682009
==========

[commit-bot: I haz the power, 2017-02-22 02:54:57]

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

[commit-bot: I haz the power, 2017-02-22 02:54:57]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[dcheng, 2017-02-22 07:26:44]

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

https://codereview.chromium.org/2709813007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:1071: if (auto* frameWidget =
WebLocalFrameImpl::fromFrame(frame)->frameWidget())
Is this only null in tests? I would prefer not to null check this if it's not
necessary for production code.

[EhsanK, 2017-02-22 17:10:52]

Thanks Daniel! PTAL.

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

https://codereview.chromium.org/2709813007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:1071: if (auto* frameWidget =
WebLocalFrameImpl::fromFrame(frame)->frameWidget())
On 2017/02/22 07:26:44, dcheng wrote:
> Is this only null in tests? I would prefer not to null check this if it's not
> necessary for production code.

Good point. But unfortunately (and I don't quite know why yet) it happens in
content shell as well.
To repro:
1- I put a DCHECK(false) when frameWidget is nullptr.
2- Created an html page with <input> which in its onload handler creates and
attaches another <iframe> to its window.location.href.
3- In content shell, focused address bar with the URL to the page explaiend and
spammed Enter (to navigate) + tab.

Sample Stack Trace:
#0 0x7ff8d3f784a7 base::debug::StackTrace::StackTrace()
#1 0x7ff8d3f7801f base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7ff8d5d5e330 <unknown>
#3 0x7ff8ccf23c37 gsignal
#4 0x7ff8ccf27028 abort
#5 0x7ff8d3f762b2 base::debug::BreakDebugger()
#6 0x7ff8d3f9cb0d logging::LogMessage::~LogMessage()
#7 0x7ff8d0d18704 blink::ChromeClientImpl::showVirtualKeyboardOnElementFocus()
#8 0x7ff8cf9b4157 blink::Element::focus()
#9 0x7ff8d0150b4e blink::FocusController::advanceFocusInDocumentOrder()
#10 0x7ff8d015060d blink::FocusController::advanceFocus()
#11 0x7ff8cfe42f0e blink::KeyboardEventManager::defaultTabEventHandler()
#12 0x7ff8cfe42d22 blink::KeyboardEventManager::defaultKeyboardEventHandler()
#13 0x7ff8cfa02112 blink::Node::defaultEventHandler()
#14 0x7ff8cfca8496 blink::HTMLElement::defaultEventHandler()
#15 0x7ff8cfb9ac53 blink::EventDispatcher::dispatchEventPostProcess()
#16 0x7ff8cfb9a10d blink::EventDispatcher::dispatch()
#17 0x7ff8cfb995f1 blink::EventDispatchMediator::dispatchEvent()
#18 0x7ff8cfb996e1 blink::EventDispatcher::dispatchEvent()
#19 0x7ff8cfe428fd blink::KeyboardEventManager::keyEvent()
#20 0x7ff8d0de02b3 blink::WebViewImpl::handleKeyEvent()
#21 0x7ff8d0d47454 blink::PageWidgetDelegate::handleInputEvent()
#22 0x7ff8d0ddee23 blink::WebViewImpl::handleInputEvent()
#23 0x7ff8d5095969 content::RenderWidgetInputHandler::HandleInputEvent()
#24 0x7ff8d5134554 content::RenderWidget::OnHandleInputEvent()

Also, I tried focusing the <input> in the window.onload handler which added the
following variety of stack trace to the mix:

Sample Stack Trace #2:
#0 0x7ff8d3f784a7 base::debug::StackTrace::StackTrace()
#1 0x7ff8d3f7801f base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7ff8d5d5e330 <unknown>
#3 0x7ff8ccf23c37 gsignal
#4 0x7ff8ccf27028 abort
#5 0x7ff8d3f762b2 base::debug::BreakDebugger()
#6 0x7ff8d3f9cb0d logging::LogMessage::~LogMessage()
#7 0x7ff8d0d18704 blink::ChromeClientImpl::showVirtualKeyboardOnElementFocus()
#8 0x7ff8cf9b4157 blink::Element::focus()
#9 0x7ff8d045fff4 blink::V8HTMLElement::focusMethodCallback()

So I believe it is safe to check for frameWidget. That being said, we were (and
still are in other places) checking for |m_webView|. Is it possible that
m_webView does not exist but we still have a |frameWidget|?

[Charlie Reis, 2017-02-22 21:36:29]

content/ LGTM

[dcheng, 2017-02-23 05:42:29]

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

https://codereview.chromium.org/2709813007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:1071: if (auto* frameWidget =
WebLocalFrameImpl::fromFrame(frame)->frameWidget())
On 2017/02/22 17:10:52, EhsanK wrote:
> On 2017/02/22 07:26:44, dcheng wrote:
> > Is this only null in tests? I would prefer not to null check this if it's
not
> > necessary for production code.
> 
> Good point. But unfortunately (and I don't quite know why yet) it happens in
> content shell as well.
> To repro:
> 1- I put a DCHECK(false) when frameWidget is nullptr.
> 2- Created an html page with <input> which in its onload handler creates and
> attaches another <iframe> to its window.location.href.
> 3- In content shell, focused address bar with the URL to the page explaiend
and
> spammed Enter (to navigate) + tab.
> 
> Sample Stack Trace:
> #0 0x7ff8d3f784a7 base::debug::StackTrace::StackTrace()
> #1 0x7ff8d3f7801f base::debug::(anonymous
namespace)::StackDumpSignalHandler()
> #2 0x7ff8d5d5e330 <unknown>
> #3 0x7ff8ccf23c37 gsignal
> #4 0x7ff8ccf27028 abort
> #5 0x7ff8d3f762b2 base::debug::BreakDebugger()
> #6 0x7ff8d3f9cb0d logging::LogMessage::~LogMessage()
> #7 0x7ff8d0d18704
blink::ChromeClientImpl::showVirtualKeyboardOnElementFocus()
> #8 0x7ff8cf9b4157 blink::Element::focus()
> #9 0x7ff8d0150b4e blink::FocusController::advanceFocusInDocumentOrder()
> #10 0x7ff8d015060d blink::FocusController::advanceFocus()
> #11 0x7ff8cfe42f0e blink::KeyboardEventManager::defaultTabEventHandler()
> #12 0x7ff8cfe42d22 blink::KeyboardEventManager::defaultKeyboardEventHandler()
> #13 0x7ff8cfa02112 blink::Node::defaultEventHandler()
> #14 0x7ff8cfca8496 blink::HTMLElement::defaultEventHandler()
> #15 0x7ff8cfb9ac53 blink::EventDispatcher::dispatchEventPostProcess()
> #16 0x7ff8cfb9a10d blink::EventDispatcher::dispatch()
> #17 0x7ff8cfb995f1 blink::EventDispatchMediator::dispatchEvent()
> #18 0x7ff8cfb996e1 blink::EventDispatcher::dispatchEvent()
> #19 0x7ff8cfe428fd blink::KeyboardEventManager::keyEvent()
> #20 0x7ff8d0de02b3 blink::WebViewImpl::handleKeyEvent()
> #21 0x7ff8d0d47454 blink::PageWidgetDelegate::handleInputEvent()
> #22 0x7ff8d0ddee23 blink::WebViewImpl::handleInputEvent()
> #23 0x7ff8d5095969 content::RenderWidgetInputHandler::HandleInputEvent()
> #24 0x7ff8d5134554 content::RenderWidget::OnHandleInputEvent()
> 
> Also, I tried focusing the <input> in the window.onload handler which added
the
> following variety of stack trace to the mix:
> 
> Sample Stack Trace #2:
> #0 0x7ff8d3f784a7 base::debug::StackTrace::StackTrace()
> #1 0x7ff8d3f7801f base::debug::(anonymous
namespace)::StackDumpSignalHandler()
> #2 0x7ff8d5d5e330 <unknown>
> #3 0x7ff8ccf23c37 gsignal
> #4 0x7ff8ccf27028 abort
> #5 0x7ff8d3f762b2 base::debug::BreakDebugger()
> #6 0x7ff8d3f9cb0d logging::LogMessage::~LogMessage()
> #7 0x7ff8d0d18704
blink::ChromeClientImpl::showVirtualKeyboardOnElementFocus()
> #8 0x7ff8cf9b4157 blink::Element::focus()
> #9 0x7ff8d045fff4 blink::V8HTMLElement::focusMethodCallback()
> 
> So I believe it is safe to check for frameWidget. That being said, we were
(and
> still are in other places) checking for |m_webView|. Is it possible that
> m_webView does not exist but we still have a |frameWidget|?

Hmm, I think you might need to get the local root first, right? Otherwise, I
think the null deref is expected.

[EhsanK, 2017-02-23 16:06:45]

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

[commit-bot: I haz the power, 2017-02-23 16:07:04]

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-02-23 16:09:52]

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

[commit-bot: I haz the power, 2017-02-23 16:09:53]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[EhsanK, 2017-02-23 16:25:59]

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

[commit-bot: I haz the power, 2017-02-23 16:26:27]

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

[EhsanK, 2017-02-23 17:24:16]

Thanks! PTAL.

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

https://codereview.chromium.org/2709813007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:1071: if (auto* frameWidget =
WebLocalFrameImpl::fromFrame(frame)->frameWidget())
On 2017/02/23 05:42:29, dcheng wrote:
> On 2017/02/22 17:10:52, EhsanK wrote:
> > On 2017/02/22 07:26:44, dcheng wrote:
> > > Is this only null in tests? I would prefer not to null check this if it's
> not
> > > necessary for production code.
> > 
> > Good point. But unfortunately (and I don't quite know why yet) it happens in
> > content shell as well.
> > To repro:
> > 1- I put a DCHECK(false) when frameWidget is nullptr.
> > 2- Created an html page with <input> which in its onload handler creates and
> > attaches another <iframe> to its window.location.href.
> > 3- In content shell, focused address bar with the URL to the page explaiend
> and
> > spammed Enter (to navigate) + tab.
> > 
> > Sample Stack Trace:
> > #0 0x7ff8d3f784a7 base::debug::StackTrace::StackTrace()
> > #1 0x7ff8d3f7801f base::debug::(anonymous
namespace)::StackDumpSignalHandler()
> > #2 0x7ff8d5d5e330 <unknown>
> > #3 0x7ff8ccf23c37 gsignal
> > #4 0x7ff8ccf27028 abort
> > #5 0x7ff8d3f762b2 base::debug::BreakDebugger()
> > #6 0x7ff8d3f9cb0d logging::LogMessage::~LogMessage()
> > #7 0x7ff8d0d18704
blink::ChromeClientImpl::showVirtualKeyboardOnElementFocus()
> > #8 0x7ff8cf9b4157 blink::Element::focus()
> > #9 0x7ff8d0150b4e blink::FocusController::advanceFocusInDocumentOrder()
> > #10 0x7ff8d015060d blink::FocusController::advanceFocus()
> > #11 0x7ff8cfe42f0e blink::KeyboardEventManager::defaultTabEventHandler()
> > #12 0x7ff8cfe42d22
blink::KeyboardEventManager::defaultKeyboardEventHandler()
> > #13 0x7ff8cfa02112 blink::Node::defaultEventHandler()
> > #14 0x7ff8cfca8496 blink::HTMLElement::defaultEventHandler()
> > #15 0x7ff8cfb9ac53 blink::EventDispatcher::dispatchEventPostProcess()
> > #16 0x7ff8cfb9a10d blink::EventDispatcher::dispatch()
> > #17 0x7ff8cfb995f1 blink::EventDispatchMediator::dispatchEvent()
> > #18 0x7ff8cfb996e1 blink::EventDispatcher::dispatchEvent()
> > #19 0x7ff8cfe428fd blink::KeyboardEventManager::keyEvent()
> > #20 0x7ff8d0de02b3 blink::WebViewImpl::handleKeyEvent()
> > #21 0x7ff8d0d47454 blink::PageWidgetDelegate::handleInputEvent()
> > #22 0x7ff8d0ddee23 blink::WebViewImpl::handleInputEvent()
> > #23 0x7ff8d5095969 content::RenderWidgetInputHandler::HandleInputEvent()
> > #24 0x7ff8d5134554 content::RenderWidget::OnHandleInputEvent()
> > 
> > Also, I tried focusing the <input> in the window.onload handler which added
> the
> > following variety of stack trace to the mix:
> > 
> > Sample Stack Trace #2:
> > #0 0x7ff8d3f784a7 base::debug::StackTrace::StackTrace()
> > #1 0x7ff8d3f7801f base::debug::(anonymous
namespace)::StackDumpSignalHandler()
> > #2 0x7ff8d5d5e330 <unknown>
> > #3 0x7ff8ccf23c37 gsignal
> > #4 0x7ff8ccf27028 abort
> > #5 0x7ff8d3f762b2 base::debug::BreakDebugger()
> > #6 0x7ff8d3f9cb0d logging::LogMessage::~LogMessage()
> > #7 0x7ff8d0d18704
blink::ChromeClientImpl::showVirtualKeyboardOnElementFocus()
> > #8 0x7ff8cf9b4157 blink::Element::focus()
> > #9 0x7ff8d045fff4 blink::V8HTMLElement::focusMethodCallback()
> > 
> > So I believe it is safe to check for frameWidget. That being said, we were
> (and
> > still are in other places) checking for |m_webView|. Is it possible that
> > m_webView does not exist but we still have a |frameWidget|?
> 
> Hmm, I think you might need to get the local root first, right? Otherwise, I
> think the null deref is expected.


Yes thanks! I was confusing frameWidget() with GetRenderWidget() in content/
which
automatically gets it from the local root. I believe everything should be green
now.

[commit-bot: I haz the power, 2017-02-23 17:55:53]

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

[commit-bot: I haz the power, 2017-02-23 17:55:54]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-23 19:28:48]

https://codereview.chromium.org/2709813007/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2709813007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11451: view->close();
FWIW, I don't think this unit test adds much and we should just remove it. If we
do want to test this, I think we should be loading some actual DOM into the page
and then calling HTMLInputElement::focus() and checking that it calls out
correctly. WDYT?

[EhsanK, 2017-02-23 21:06:09]

Thanks! PTAL.

https://codereview.chromium.org/2709813007/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2709813007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11451: view->close();
On 2017/02/23 19:28:48, dcheng wrote:
> FWIW, I don't think this unit test adds much and we should just remove it. If
we
> do want to test this, I think we should be loading some actual DOM into the
page
> and then calling HTMLInputElement::focus() and checking that it calls out
> correctly. WDYT?

I agree with not having the test. I intended to load a page initially but was
unable to find a way to have a test WebWidgetClient. I will remove the test if
that is OK.

[dcheng, 2017-02-23 21:10:21]

https://codereview.chromium.org/2709813007/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2709813007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11451: view->close();
On 2017/02/23 21:06:09, EhsanK wrote:
> On 2017/02/23 19:28:48, dcheng wrote:
> > FWIW, I don't think this unit test adds much and we should just remove it.
If
> we
> > do want to test this, I think we should be loading some actual DOM into the
> page
> > and then calling HTMLInputElement::focus() and checking that it calls out
> > correctly. WDYT?
> 
> I agree with not having the test. I intended to load a page initially but was
> unable to find a way to have a test WebWidgetClient. I will remove the test if
> that is OK.

One option is to use the helpers from FrameTestHelpers: the createLocalChild()
helper takes a WebWidgetClient that you can control. Then you can load HTML into
it using FrameTestHelpers::loadFrame or the like, and verify that the methods
are called.

[EhsanK, 2017-02-27 16:57:13]

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

[commit-bot: I haz the power, 2017-02-27 16:57:41]

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

[EhsanK, 2017-02-27 16:58:17]

Patchset #9 (id:160001) has been deleted

[EhsanK, 2017-02-27 16:59:56]

Thanks Daniel! PTAL.

https://codereview.chromium.org/2709813007/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2709813007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11451: view->close();
On 2017/02/23 21:10:21, dcheng wrote:
> On 2017/02/23 21:06:09, EhsanK wrote:
> > On 2017/02/23 19:28:48, dcheng wrote:
> > > FWIW, I don't think this unit test adds much and we should just remove it.
> If
> > we
> > > do want to test this, I think we should be loading some actual DOM into
the
> > page
> > > and then calling HTMLInputElement::focus() and checking that it calls out
> > > correctly. WDYT?
> > 
> > I agree with not having the test. I intended to load a page initially but
was
> > unable to find a way to have a test WebWidgetClient. I will remove the test
if
> > that is OK.
> 
> One option is to use the helpers from FrameTestHelpers: the createLocalChild()
> helper takes a WebWidgetClient that you can control. Then you can load HTML
into
> it using FrameTestHelpers::loadFrame or the like, and verify that the methods
> are called.

Thanks! I also had to make it a proper WebFrameSwapTest to work.

[commit-bot: I haz the power, 2017-02-27 17:00:27]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[EhsanK, 2017-02-27 17:03:31]

The CQ bit was unchecked by ekaramad@chromium.org

[dcheng, 2017-02-27 23:13:29]

https://codereview.chromium.org/2709813007/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2709813007/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:1070: void
ChromeClientImpl::showVirtualKeyboardOnElementFocus(LocalFrame* frame) {
Nit: pass as mutable reference, as this should never be null

https://codereview.chromium.org/2709813007/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2709813007/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11390:
mainFrame()->swap(remoteFrame);
Is it possible to create a WebView directly and set the remote frame as the main
frame, instead of using WebFrameSwapTest?

https://codereview.chromium.org/2709813007/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11397:
remoteFrame->swap(localFrame);
Is it possible to use FrameTestHelpers::createLocalChild() directly instead of
swapping?

[EhsanK, 2017-03-08 23:38:28]

Thanks Daniel and sorry it took me a while. Please take another look.

https://codereview.chromium.org/2709813007/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2709813007/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:1070: void
ChromeClientImpl::showVirtualKeyboardOnElementFocus(LocalFrame* frame) {
On 2017/02/27 23:13:29, dcheng wrote:
> Nit: pass as mutable reference, as this should never be null

Thanks. You mean LocalFrame&?

https://codereview.chromium.org/2709813007/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2709813007/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11390:
mainFrame()->swap(remoteFrame);
On 2017/02/27 23:13:29, dcheng wrote:
> Is it possible to create a WebView directly and set the remote frame as the
main
> frame, instead of using WebFrameSwapTest?

Done.

https://codereview.chromium.org/2709813007/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11397:
remoteFrame->swap(localFrame);
On 2017/02/27 23:13:29, dcheng wrote:
> Is it possible to use FrameTestHelpers::createLocalChild() directly instead of
> swapping?

Done.

https://codereview.chromium.org/2709813007/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2709813007/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11439:
RefPtr<SecurityOrigin> origin =
If I don't set this, it will be null for the frame and I will get a crash when
trying to load the local frame.

[dcheng, 2017-03-09 02:00:25]

LGTM with nits addressed

https://codereview.chromium.org/2709813007/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2709813007/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11437:
WebRemoteFrame::create(WebTreeScopeType::Document, nullptr));
Nit: WebRemoteFrameImpl::create will avoid the need for a static cast.

https://codereview.chromium.org/2709813007/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11439:
RefPtr<SecurityOrigin> origin =
On 2017/03/08 23:38:28, EhsanK wrote:
> If I don't set this, it will be null for the frame and I will get a crash when
> trying to load the local frame.

Nit: we usually we just create a unique origin here.

[EhsanK, 2017-03-09 17:19:49]

Thanks Daniel! I addressed the comments and will try to CQ soon.

https://codereview.chromium.org/2709813007/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2709813007/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11437:
WebRemoteFrame::create(WebTreeScopeType::Document, nullptr));
On 2017/03/09 02:00:25, dcheng wrote:
> Nit: WebRemoteFrameImpl::create will avoid the need for a static cast.

Acknowledged.

https://codereview.chromium.org/2709813007/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11439:
RefPtr<SecurityOrigin> origin =
On 2017/03/09 02:00:25, dcheng wrote:
> On 2017/03/08 23:38:28, EhsanK wrote:
> > If I don't set this, it will be null for the frame and I will get a crash
when
> > trying to load the local frame.
> 
> Nit: we usually we just create a unique origin here.

Acknowledged.

[EhsanK, 2017-03-09 17:20:28]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2017-03-09 17:20:28]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2709813007/#ps260001
(title: "Addressing dcheng@'s comments")

[commit-bot: I haz the power, 2017-03-09 17:20:47]

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-03-09 19:06:53]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1489080028055200,
"parent_rev": "5cec5a9e7fe1f7e1ed5a6b8a9a45f3b539b43eb7", "commit_rev":
"1e2d08a7fc2db1297a7f2a28c40659af534c309e"}

[commit-bot: I haz the power, 2017-03-09 19:07:38]

Description was changed from

==========
Remove WebViewClient::showVirtualKeyboardOnElementFocus()

When an <input> is focused by user gesture, ChildClientImpl calls this
method on RenderViewImpl to make sure the proper TextInputState to show
IME keyboard is sent to the browser (used for Android). However, for
OOPIFs it does not make sense to call this on RenderView and rather, the
call should be directed to the RenderWidget corresponding to the local
root of the focused element/frame.

This CL removes the current override from RenderViewImpl/WebViewClient
and directs the call to the frameWidget() of the focused frame instead.

This CL also adds a WebFrameTest.

BUG=578168, 682009
==========

to

==========
Remove WebViewClient::showVirtualKeyboardOnElementFocus()

When an <input> is focused by user gesture, ChildClientImpl calls this
method on RenderViewImpl to make sure the proper TextInputState to show
IME keyboard is sent to the browser (used for Android). However, for
OOPIFs it does not make sense to call this on RenderView and rather, the
call should be directed to the RenderWidget corresponding to the local
root of the focused element/frame.

This CL removes the current override from RenderViewImpl/WebViewClient
and directs the call to the frameWidget() of the focused frame instead.

This CL also adds a WebFrameTest.

BUG=578168, 682009

Review-Url: https://codereview.chromium.org/2709813007
Cr-Commit-Position: refs/heads/master@{#455820}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e2d08a7fc2db1297a7f2a28c406...
==========

[commit-bot: I haz the power, 2017-03-09 19:07:40]

Committed patchset #13 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/1e2d08a7fc2db1297a7f2a28c406...