Move IME related functions from WebFrame to WebLocalFrame

Move IME related functions from WebFrame to WebLocalFrame

BUG=604645
Committed: https://crrev.com/8204efcf337a42af04aebcee8827026b317071f0
Cr-Commit-Position: refs/heads/master@{#404345}

[yabinh, 2016-05-26 09:24:30]

yabinh@chromium.org changed reviewers:
+ aelias@chromium.org, changwan@chromium.org, dcheng@chromium.org

[yabinh, 2016-05-26 09:24:31]

changewan@, can you take a look at this patch?

[dcheng, 2016-05-27 01:40:50]

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

[dcheng, 2016-05-27 01:40:50]

+ekaramad as FYI

I also have a CL in progress to make focusedOrMainFrame() just return a
LocalFrame* directly, that might simplify things here a bit.

[Charlie Reis, 2016-05-27 04:38:21]

Description was changed from

==========
Move IME related functions from WebFrame to WebLocalFrame

BUG=604645
==========

to

==========
Move IME related functions from WebFrame to WebLocalFrame

BUG=604645
==========

[yabinh, 2016-06-13 08:10:01]

On 2016/05/27 01:40:50, dcheng wrote:
> +ekaramad as FYI
> 
> I also have a CL in progress to make focusedOrMainFrame() just return a
> LocalFrame* directly, that might simplify things here a bit.

dcheng@, can you take another look of this patch?
FYI, this patch is irrelevant to focusedOrMainFrame().

[dcheng, 2016-06-13 17:29:46]

I know you're just changing existing code, but I'm wondering how many of the
null checks are really necessary. You shouldn't ever get null unless you call it
on a detached webview... and it's not clear to me how that would happen. If it
can happen legitimately and we need to guard against it, that's fine... but it
seems out of place otherwise.

https://codereview.chromium.org/2012823003/diff/20001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_provider.cc (right):

https://codereview.chromium.org/2012823003/diff/20001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_provider.cc:294: if (!frame)
This seems like the code is doing the wrong thing: ideally, we should just not
call this in a webview that's not actually focused, i.e. the browser process
should be coordinating things. How is spelling check enabled today?

https://codereview.chromium.org/2012823003/diff/20001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2012823003/diff/20001/content/renderer/render...
content/renderer/render_view_impl.cc:1629: WebLocalFrame* frame =
webview()->focusedFrame();
Ditto: why is |frame| null here? I wonder if this is really the right thing to
be doing: higher up in the stack, in KeyboardEventManager, we actually have a
frame. Should we be plumbing that through here instead?

https://codereview.chromium.org/2012823003/diff/20001/content/renderer/render...
content/renderer/render_view_impl.cc:2715: if (WebLocalFrame* frame =
webview()->focusedFrame()) {
I'm not sure this can ever be null.

toWebLocalFrame() asserts if the frame is not local.
And if focusedFrame() is null, then toWebLocalFrame() is null.

So do we need this conditional at all?

https://codereview.chromium.org/2012823003/diff/20001/content/renderer/render...
content/renderer/render_view_impl.cc:2744: if (WebLocalFrame* frame =
webview()->focusedFrame()) {
Ditto.

https://codereview.chromium.org/2012823003/diff/20001/content/renderer/render...
content/renderer/render_view_impl.cc:2818: if (!frame)
Similar question: can this actually return null? It seems like it could if you
called on a detached webview... but what are we doing calling this on a detached
webview then? I would expect webview() to be cleared.

https://codereview.chromium.org/2012823003/diff/20001/content/renderer/text_i...
File content/renderer/text_input_client_observer.cc (right):

https://codereview.chromium.org/2012823003/diff/20001/content/renderer/text_i...
content/renderer/text_input_client_observer.cc:90: blink::WebLocalFrame* frame =
webview()->focusedFrame();
Same questions in this file about null |frame|.

https://codereview.chromium.org/2012823003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2012823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:2912: if (!frame ||
!frame->isLocalFrame())
Let's just assert that this is local. All the return paths claim to return a
local frame already.

[yabinh, 2016-06-16 07:12:57]

cc'd rouslan@ for the owner of spellcheck_provider.cc.

https://codereview.chromium.org/2012823003/diff/20001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_provider.cc (right):

https://codereview.chromium.org/2012823003/diff/20001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_provider.cc:294: if (!frame)
Most of the null checks is unnecessary, except for this one.

I found that spellcheck is always enabled, except on
iOS.(https://cs.corp.google.com/clankium/src/build/common.gypi?q=ENABLE_SPELLCHECK&sq=package:clankium&l=861&dr=C)

It might be the reason.

[dcheng, 2016-06-20 19:55:11]

https://codereview.chromium.org/2012823003/diff/40001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_provider.cc (right):

https://codereview.chromium.org/2012823003/diff/40001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_provider.cc:294: if (!frame)
I guess I'm still not sure why this is necessary. focusedFrame() ends up using
FocusController::focusedOrMainFrame(). While the return type makes it seem like
it won't always return a local frame, in practice, it always does:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu...

The only reason it won't return a non-null local frame is if there are no frames
in the page. Can you link a callstack that shows how we're calling it when
|frame| is null?

[yabinh, 2016-06-22 13:34:50]

https://codereview.chromium.org/2012823003/diff/40001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_provider.cc (right):

https://codereview.chromium.org/2012823003/diff/40001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_provider.cc:294: if (!frame)
On 2016/06/20 19:55:11, dcheng wrote:
> I guess I'm still not sure why this is necessary. focusedFrame() ends up using
> FocusController::focusedOrMainFrame(). While the return type makes it seem
like
> it won't always return a local frame, in practice, it always does:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu...
> 
> The only reason it won't return a non-null local frame is if there are no
frames
> in the page. Can you link a callstack that shows how we're calling it when
> |frame| is null?

Sometimes FocusController::focusedOrMainFrame() returns a remote frame. 
Please see patch set 5.

https://codereview.chromium.org/2012823003/diff/80001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_provider.cc (right):

https://codereview.chromium.org/2012823003/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_provider.cc:295: LOG(ERROR) << "hyb: not
local frame";
Remove the null check to see what will happen.

https://codereview.chromium.org/2012823003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2012823003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:800: 
Sometimes it returns a remote frame.
In the build:
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
many tests failed because of that.
Here is one of those tests:
WebViewInteractiveTests_WebViewNewWindowInteractiveTest.NewWindow_Close_0
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux...

[dcheng, 2016-07-01 06:53:16]

https://codereview.chromium.org/2012823003/diff/80001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_provider.cc (right):

https://codereview.chromium.org/2012823003/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_provider.cc:295: LOG(ERROR) << "hyb: not
local frame";
On 2016/06/22 13:34:49, yabinh wrote:
> Remove the null check to see what will happen.

OK, so I think this is because Spellcheck uses RenderViewVisitor, so it visits
all RenderViews, even ones with no local frames at all. =(

Let's keep this null check in for now then: do you mind filing a crbug.com for
fixing spellcheck to not use RenderViewVisitor like this?

[yabinh, 2016-07-01 07:58:46]

On 2016/07/01 06:53:16, dcheng wrote:
>
https://codereview.chromium.org/2012823003/diff/80001/chrome/renderer/spellch...
> File chrome/renderer/spellchecker/spellcheck_provider.cc (right):
> 
>
https://codereview.chromium.org/2012823003/diff/80001/chrome/renderer/spellch...
> chrome/renderer/spellchecker/spellcheck_provider.cc:295: LOG(ERROR) << "hyb:
not
> local frame";
> On 2016/06/22 13:34:49, yabinh wrote:
> > Remove the null check to see what will happen.
> 
> OK, so I think this is because Spellcheck uses RenderViewVisitor, so it visits
> all RenderViews, even ones with no local frames at all. =(
> 
> Let's keep this null check in for now then: do you mind filing a
http://crbug.com for
> fixing spellcheck to not use RenderViewVisitor like this?

Filed. Please add a comment if I didn't describe the bug clearly.
See:
crbug.com/625068

[dcheng, 2016-07-01 09:12:05]

https://codereview.chromium.org/2012823003/diff/80001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_provider.cc (right):

https://codereview.chromium.org/2012823003/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_provider.cc:295: LOG(ERROR) << "hyb: not
local frame";
On 2016/07/01 06:53:16, dcheng wrote:
> On 2016/06/22 13:34:49, yabinh wrote:
> > Remove the null check to see what will happen.
> 
> OK, so I think this is because Spellcheck uses RenderViewVisitor, so it visits
> all RenderViews, even ones with no local frames at all. =(
> 
> Let's keep this null check in for now then: do you mind filing a
http://crbug.com for
> fixing spellcheck to not use RenderViewVisitor like this?

Please clean up the logging here and in FocusController/WebViewImpl and I will
l-g-tm  =)

[yabinh, 2016-07-01 09:14:34]

On 2016/07/01 09:12:05, dcheng wrote:
>
https://codereview.chromium.org/2012823003/diff/80001/chrome/renderer/spellch...
> File chrome/renderer/spellchecker/spellcheck_provider.cc (right):
> 
>
https://codereview.chromium.org/2012823003/diff/80001/chrome/renderer/spellch...
> chrome/renderer/spellchecker/spellcheck_provider.cc:295: LOG(ERROR) << "hyb:
not
> local frame";
> On 2016/07/01 06:53:16, dcheng wrote:
> > On 2016/06/22 13:34:49, yabinh wrote:
> > > Remove the null check to see what will happen.
> > 
> > OK, so I think this is because Spellcheck uses RenderViewVisitor, so it
visits
> > all RenderViews, even ones with no local frames at all. =(
> > 
> > Let's keep this null check in for now then: do you mind filing a
> http://crbug.com for
> > fixing spellcheck to not use RenderViewVisitor like this?
> 
> Please clean up the logging here and in FocusController/WebViewImpl and I will
> l-g-tm  =)

Thanks. :)
Please see the latest patch. I added some todo in it.

[dcheng, 2016-07-01 09:22:40]

lgtm

[yabinh, 2016-07-07 07:59:18]

yabinh@chromium.org changed reviewers:
+ rouslan@chromium.org, sdefresne@chromium.org, sievers@chromium.org

[yabinh, 2016-07-07 07:59:19]

rouslan@chromium.org: Please review changes in 
spellcheck_provider.cc
sievers@chromium.org: Please review changes in 
content/
sdefresne@chromium.org: Please review changes in 
components/
Thanks.

[sdefresne, 2016-07-07 09:12:47]

On 2016/07/07 07:59:19, yabinh wrote:
> mailto:rouslan@chromium.org: Please review changes in 
> spellcheck_provider.cc
> mailto:sievers@chromium.org: Please review changes in 
> content/
> mailto:sdefresne@chromium.org: Please review changes in 
> components/
> Thanks.

The code lgtm but I don't know that code. It would probably be better to ask a
review from an owner of components/printing and components/test_runner (if the
change is more than just adapting client code usage of an updated API).

[yabinh, 2016-07-07 09:30:49]

On 2016/07/07 09:12:47, sdefresne wrote:
> On 2016/07/07 07:59:19, yabinh wrote:
> > mailto:rouslan@chromium.org: Please review changes in 
> > spellcheck_provider.cc
> > mailto:sievers@chromium.org: Please review changes in 
> > content/
> > mailto:sdefresne@chromium.org: Please review changes in 
> > components/
> > Thanks.
> 
> The code lgtm but I don't know that code. It would probably be better to ask a
> review from an owner of components/printing and components/test_runner (if the
> change is more than just adapting client code usage of an updated API).

Thanks, sdefresne@.

tkent@, can you take a look at components/test_runner? Thanks.

[yabinh, 2016-07-07 09:31:20]

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

[please use gerrit instead, 2016-07-07 13:00:50]

lgtm

spellcheck lgtm

[no sievers, 2016-07-07 17:43:58]

On 2016/07/07 07:59:19, yabinh wrote:
> mailto:rouslan@chromium.org: Please review changes in 
> spellcheck_provider.cc
> mailto:sievers@chromium.org: Please review changes in 
> content/

lgtm since dcheng@ said above that the null checks are probably not necessary.

[tkent, 2016-07-07 23:16:24]

components/test_runner lgtm.

Is WebView::mainFrame always WebLocalFrame?

[yabinh, 2016-07-08 01:40:58]

tkent@, can you take another look at the latest patch?

https://codereview.chromium.org/2012823003/diff/120001/components/test_runner...
File components/test_runner/test_runner_for_specific_view.cc (right):

https://codereview.chromium.org/2012823003/diff/120001/components/test_runner...
components/test_runner/test_runner_for_specific_view.cc:675: std::string
TestRunnerForSpecificView::SelectionAsMarkup() {
Same with TestRunnerForSpecificView::CallShouldCloseOnWebView().

https://codereview.chromium.org/2012823003/diff/120001/components/test_runner...
File components/test_runner/text_input_controller.cc (right):

https://codereview.chromium.org/2012823003/diff/120001/components/test_runner...
components/test_runner/text_input_controller.cc:168: void
TextInputController::DoCommand(const std::string& text) {
mainFrame() can be a remote frame because of OOPI, though the existing code
assumes that it is always a LocalFrame.

See the comment:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page...

[dcheng, 2016-07-08 01:55:15]

https://codereview.chromium.org/2012823003/diff/120001/components/test_runner...
File components/test_runner/test_runner_for_specific_view.cc (right):

https://codereview.chromium.org/2012823003/diff/120001/components/test_runner...
components/test_runner/test_runner_for_specific_view.cc:675: std::string
TestRunnerForSpecificView::SelectionAsMarkup() {
On 2016/07/08 01:40:58, yabinh wrote:
> Same with TestRunnerForSpecificView::CallShouldCloseOnWebView().

Note that a lot of existing code in test runner simply assumes everything is
local frames. I think it's better to do that for now: then we'll have visible
failures on the site isolation fyi bot, which we can use to update the test
runner.

CHECK() is OK, since it'll crash at least, so we know about the problem, but in
text_input_controller.cc, it's possible that a badly written test will somehow
still pass (and we'll miss this case)

[yabinh, 2016-07-08 02:42:53]

https://codereview.chromium.org/2012823003/diff/140001/components/test_runner...
File components/test_runner/text_input_controller.cc (right):

https://codereview.chromium.org/2012823003/diff/140001/components/test_runner...
components/test_runner/text_input_controller.cc:206: 
I think we don't need to CHECK here, because if it's a remote frame, it doesn't
have marked text. So it makes sense to return false.

[dcheng, 2016-07-08 03:31:09]

https://codereview.chromium.org/2012823003/diff/140001/components/test_runner...
File components/test_runner/text_input_controller.cc (right):

https://codereview.chromium.org/2012823003/diff/140001/components/test_runner...
components/test_runner/text_input_controller.cc:206: 
On 2016/07/08 02:42:53, yabinh wrote:
> I think we don't need to CHECK here, because if it's a remote frame, it
doesn't
> have marked text. So it makes sense to return false.

Right, but fundamentally, we shouldn't be calling this in on a remote frame at
all.

[yabinh, 2016-07-08 04:22:54]

https://codereview.chromium.org/2012823003/diff/140001/components/test_runner...
File components/test_runner/text_input_controller.cc (right):

https://codereview.chromium.org/2012823003/diff/140001/components/test_runner...
components/test_runner/text_input_controller.cc:206: 
On 2016/07/08 03:31:09, dcheng wrote:
> On 2016/07/08 02:42:53, yabinh wrote:
> > I think we don't need to CHECK here, because if it's a remote frame, it
> doesn't
> > have marked text. So it makes sense to return false.
> 
> Right, but fundamentally, we shouldn't be calling this in on a remote frame at
> all.

OK. It's been added to the latest patch. Please take another look.

[dcheng, 2016-07-08 04:30:54]

Still LGTM.

[yabinh, 2016-07-08 10:07:51]

The CQ bit was checked by yabinh@chromium.org

[yabinh, 2016-07-08 10:07:51]

The CQ bit was unchecked by yabinh@chromium.org

[yabinh, 2016-07-08 10:07:51]

The CQ bit was checked by yabinh@chromium.org

[yabinh, 2016-07-08 10:07:51]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
rouslan@chromium.org, sdefresne@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/2012823003/#ps160001
(title: "Add CHECK in TextInputController::HasMarkedText()")

[yabinh, 2016-07-08 10:07:52]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
rouslan@chromium.org, sdefresne@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/2012823003/#ps160001
(title: "Add CHECK in TextInputController::HasMarkedText()")

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

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-08 13:59:12]

Description was changed from

==========
Move IME related functions from WebFrame to WebLocalFrame

BUG=604645
==========

to

==========
Move IME related functions from WebFrame to WebLocalFrame

BUG=604645
==========

[commit-bot: I haz the power, 2016-07-08 13:59:15]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-08 13:59:46]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 14:00:33]

Description was changed from

==========
Move IME related functions from WebFrame to WebLocalFrame

BUG=604645
==========

to

==========
Move IME related functions from WebFrame to WebLocalFrame

BUG=604645

Committed: https://crrev.com/8204efcf337a42af04aebcee8827026b317071f0
Cr-Commit-Position: refs/heads/master@{#404345}
==========

[commit-bot: I haz the power, 2016-07-08 14:00:35]

Patchset 9 (id:??) landed as
https://crrev.com/8204efcf337a42af04aebcee8827026b317071f0
Cr-Commit-Position: refs/heads/master@{#404345}