Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2929543002:
Request Composition Range Updates for Focused GuestViews based on BrowserPlugins (Closed)
DescriptionRequest Composition Range Updates for Focused GuestViews based on BrowserPlugins
Since the patch https://codereview.chromium.org/2121953002/ landed, composition
range information are only requested for RenderWidgets which have a focused
text input box, that is, their TextInputState.type != ui::TEXT_INPUT_STATE_NONE.
This change however is not implemented for guest views which are based on
BrowserPlugin and therefore, some IME features in Japanese IME have regressed
for <webview>s. While this is not a bug in OOPIF-based guests, it is still an
important bug to fix because:
1- PDFs will still use BrowserPlugin for a while,
2- OOPIF-based <webview>'s might not launch in M60.
This patch will add the support by sending the request after BrowserPluginGuest
forwards a TextInputStateChanged call to RenderWidgetHostViewGuest.
BUG=714771
Review-Url: https://codereview.chromium.org/2929543002
Cr-Commit-Position: refs/heads/master@{#479698}
Committed: https://chromium.googlesource.com/chromium/src/+/2c3c487604c90c9ce96fefe0a32f660050f9fe55
Patch Set 1 #
Total comments: 2
Patch Set 2 : Adding a comment #Patch Set 3 : Adding a test #
Total comments: 4
Patch Set 4 : Formatting #Patch Set 5 : Added a comment #
Total comments: 8
Patch Set 6 : Addressing nits and fixing a link error on Windows bots #Patch Set 7 : Added a comment #
Total comments: 2
Patch Set 8 : Rebased #Patch Set 9 : Addressing comments #Patch Set 10 : Rebased #Patch Set 11 : Using numeric_limits::max instead of -1 #
Messages
Total messages: 39 (23 generated)
ekaramad@chromium.org changed reviewers: + creis@chromium.org
Hello Charlie, PTAL. This bug is not important if OOPIF-webview ships. But given that it might be delayed I think this fix might be worth landing and merging.
On 2017/06/06 21:51:14, EhsanK wrote: > Hello Charlie, PTAL. > > This bug is not important if OOPIF-webview ships. But given that it might be > delayed I think this fix might be worth landing and merging. Thanks for the fix! Basically looks good, but is there a way to add a test for it, which might have caught the regression on the previous CL? https://codereview.chromium.org/2929543002/diff/1/content/browser/browser_plu... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2929543002/diff/1/content/browser/browser_plu... content/browser/browser_plugin/browser_plugin_guest.cc:657: rwh->RequestCompositionUpdates( Can you add a comment for why this is needed? I don't immediately understand from context.
On 2017/06/06 22:52:13, Charlie Reis wrote: > On 2017/06/06 21:51:14, EhsanK wrote: > > Hello Charlie, PTAL. > > > > This bug is not important if OOPIF-webview ships. But given that it might be > > delayed I think this fix might be worth landing and merging. > > Thanks for the fix! Basically looks good, but is there a way to add a test for > it, which might have caught the regression on the previous CL? > > https://codereview.chromium.org/2929543002/diff/1/content/browser/browser_plu... > File content/browser/browser_plugin/browser_plugin_guest.cc (right): > > https://codereview.chromium.org/2929543002/diff/1/content/browser/browser_plu... > content/browser/browser_plugin/browser_plugin_guest.cc:657: > rwh->RequestCompositionUpdates( > Can you add a comment for why this is needed? I don't immediately understand > from context. Thanks Charlie and sorry it took a bit to update the patch. I was not quite sure what the right test could be. I think OOPIF <webview> should be properly covered in terms of testing; though a direct test like this does not exist atm. This this test is basically for non-OOPIF case which will be comes useless the moment OOPIF-webview launches on all channels. I think a similar test for MimeHandlerView should also be helpful but that is not quite within the scope of this bug/patch. WDYT? Also, PTAL. Thanks!
https://codereview.chromium.org/2929543002/diff/1/content/browser/browser_plu... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2929543002/diff/1/content/browser/browser_plu... content/browser/browser_plugin/browser_plugin_guest.cc:657: rwh->RequestCompositionUpdates( On 2017/06/06 22:52:13, Charlie Reis wrote: > Can you add a comment for why this is needed? I don't immediately understand > from context. Yes I should have put a comment here. Done!
Thanks. Seems ok that the test is BrowserPlugin-specific if the bug is. (Obviously a general test would be better, but that's ok.) LGTM with nits. https://codereview.chromium.org/2929543002/diff/40001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2929543002/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:571: // This class observers all the composition range updates associated with the nit: observes :) https://codereview.chromium.org/2929543002/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1763: // Verify the text inside the <input> is "A B X D". I don't see anything verifying this. Is the comment stale?
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ekaramad@chromium.org changed reviewers: + lazyboy@chromium.org
Thanks Charlie! Adding lazyboy@ for test file reviews. Istiaque, PTAL! https://codereview.chromium.org/2929543002/diff/40001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2929543002/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:571: // This class observers all the composition range updates associated with the On 2017/06/09 21:38:17, Charlie Reis wrote: > nit: observes :) Thanks! Sorry! My signature typo again. https://codereview.chromium.org/2929543002/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1763: // Verify the text inside the <input> is "A B X D". On 2017/06/09 21:38:17, Charlie Reis wrote: > I don't see anything verifying this. Is the comment stale? Thanks! It should be empty "" instead.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
web_view_interactive_browsertest.cc lgtm with nits. https://codereview.chromium.org/2929543002/diff/80001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2929543002/diff/80001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:583: if (did_update_composition_range_ && nit: base::Optional is better suited for this kind of optional values. (Don't feel strongly to change this). https://codereview.chromium.org/2929543002/diff/80001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1758: content::WebContents* target_web_contents = Add a note why there is a difference in WebContents for oopif/non-oopif mode https://codereview.chromium.org/2929543002/diff/80001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1761: content::SimulateMouseClickAt(target_web_contents, 0, It always helps to point out in comments why (50, 50) is significant. https://codereview.chromium.org/2929543002/diff/80001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1773: "window.domAutomationController." super nit: the line breaking is hard to read may be: "window.domAutomationController.send( "document.querySelector('input').value)"
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
Thanks! Charlie: I had to make some changes to text_input_manager.* and text_input_test_utils.* files to fix a linker error on Windows. Please take another look. Thanks! https://codereview.chromium.org/2929543002/diff/80001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2929543002/diff/80001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:583: if (did_update_composition_range_ && On 2017/06/12 21:35:08, lazyboy wrote: > nit: base::Optional is better suited for this kind of optional values. (Don't > feel strongly to change this). Done. Thanks for the suggestions! https://codereview.chromium.org/2929543002/diff/80001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1758: content::WebContents* target_web_contents = On 2017/06/12 21:35:08, lazyboy wrote: > Add a note why there is a difference in WebContents for oopif/non-oopif mode Done. https://codereview.chromium.org/2929543002/diff/80001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1761: content::SimulateMouseClickAt(target_web_contents, 0, On 2017/06/12 21:35:08, lazyboy wrote: > It always helps to point out in comments why (50, 50) is significant. Done. https://codereview.chromium.org/2929543002/diff/80001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1773: "window.domAutomationController." On 2017/06/12 21:35:07, lazyboy wrote: > super nit: the line breaking is hard to read > may be: > "window.domAutomationController.send( > "document.querySelector('input').value)" Done. Added some indentation as well!
Thanks, LGTM. https://codereview.chromium.org/2929543002/diff/120001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2929543002/diff/120001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:593: uint32_t length; nit: Let's initialize this to something like -1.
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you all! I will try to CQ soon and hopefully merge this to M60 as well (given that <webview> might not make it to M60). Thanks! https://codereview.chromium.org/2929543002/diff/120001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2929543002/diff/120001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:593: uint32_t length; On 2017/06/14 19:57:22, Charlie Reis (slow) wrote: > nit: Let's initialize this to something like -1. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ekaramad@chromium.org
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2929543002/#ps200001 (title: "Using numeric_limits::max instead of -1")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1497534772182760,
"parent_rev": "6ddb7ce90b926412d667f71e782942408033a9f6", "commit_rev":
"2c3c487604c90c9ce96fefe0a32f660050f9fe55"}
Message was sent while issue was closed.
Description was changed from ========== Request Composition Range Updates for Focused GuestViews based on BrowserPlugins Since the patch https://codereview.chromium.org/2121953002/ landed, composition range information are only requested for RenderWidgets which have a focused text input box, that is, their TextInputState.type != ui::TEXT_INPUT_STATE_NONE. This change however is not implemented for guest views which are based on BrowserPlugin and therefore, some IME features in Japanese IME have regressed for <webview>s. While this is not a bug in OOPIF-based guests, it is still an important bug to fix because: 1- PDFs will still use BrowserPlugin for a while, 2- OOPIF-based <webview>'s might not launch in M60. This patch will add the support by sending the request after BrowserPluginGuest forwards a TextInputStateChanged call to RenderWidgetHostViewGuest. BUG=714771 ========== to ========== Request Composition Range Updates for Focused GuestViews based on BrowserPlugins Since the patch https://codereview.chromium.org/2121953002/ landed, composition range information are only requested for RenderWidgets which have a focused text input box, that is, their TextInputState.type != ui::TEXT_INPUT_STATE_NONE. This change however is not implemented for guest views which are based on BrowserPlugin and therefore, some IME features in Japanese IME have regressed for <webview>s. While this is not a bug in OOPIF-based guests, it is still an important bug to fix because: 1- PDFs will still use BrowserPlugin for a while, 2- OOPIF-based <webview>'s might not launch in M60. This patch will add the support by sending the request after BrowserPluginGuest forwards a TextInputStateChanged call to RenderWidgetHostViewGuest. BUG=714771 Review-Url: https://codereview.chromium.org/2929543002 Cr-Commit-Position: refs/heads/master@{#479698} Committed: https://chromium.googlesource.com/chromium/src/+/2c3c487604c90c9ce96fefe0a32f... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/2c3c487604c90c9ce96fefe0a32f...
Message was sent while issue was closed.
On 2017/06/15 14:58:25, commit-bot: I haz the power wrote: > Committed patchset #11 (id:200001) as > https://chromium.googlesource.com/chromium/src/+/2c3c487604c90c9ce96fefe0a32f... Findit suggests this CL introduced a flaky test "WebViewInteractiveTests/WebViewImeInteractiveTest.CompositionRangeUpdates/0 " according to an analysis at https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV... Can someone please help verify? Thanks, Jeff on behalf of Findit team
Message was sent while issue was closed.
On 2017/06/20 20:56:45, lijeffrey1 wrote: > On 2017/06/15 14:58:25, commit-bot: I haz the power wrote: > > Committed patchset #11 (id:200001) as > > > https://chromium.googlesource.com/chromium/src/+/2c3c487604c90c9ce96fefe0a32f... > > Findit suggests this CL introduced a flaky test > "WebViewInteractiveTests/WebViewImeInteractiveTest.CompositionRangeUpdates/0 " > according to an analysis at > https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV... > > Can someone please help verify? > > Thanks, > Jeff on behalf of Findit team That test (CompositionRangeUpdates) was added by this CL. ekaramad@, can you take a look at the flake?
Message was sent while issue was closed.
On 2017/06/20 22:23:58, lazyboy wrote: > On 2017/06/20 20:56:45, lijeffrey1 wrote: > > On 2017/06/15 14:58:25, commit-bot: I haz the power wrote: > > > Committed patchset #11 (id:200001) as > > > > > > https://chromium.googlesource.com/chromium/src/+/2c3c487604c90c9ce96fefe0a32f... > > > > Findit suggests this CL introduced a flaky test > > "WebViewInteractiveTests/WebViewImeInteractiveTest.CompositionRangeUpdates/0 " > > according to an analysis at > > > https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV... > > > > Can someone please help verify? > > > > Thanks, > > Jeff on behalf of Findit team > > That test (CompositionRangeUpdates) was added by this CL. ekaramad@, can > you take a look at the flake? Sorry for the delay. I am currently in chrome bootcamp. I can take a look tomorrow afternoon.
Message was sent while issue was closed.
On 2017/06/22 20:14:28, EhsanK wrote: > On 2017/06/20 22:23:58, lazyboy wrote: > > On 2017/06/20 20:56:45, lijeffrey1 wrote: > > > On 2017/06/15 14:58:25, commit-bot: I haz the power wrote: > > > > Committed patchset #11 (id:200001) as > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/2c3c487604c90c9ce96fefe0a32f... > > > > > > Findit suggests this CL introduced a flaky test > > > "WebViewInteractiveTests/WebViewImeInteractiveTest.CompositionRangeUpdates/0 > " > > > according to an analysis at > > > > > > https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV... > > > > > > Can someone please help verify? > > > > > > Thanks, > > > Jeff on behalf of Findit team > > > > That test (CompositionRangeUpdates) was added by this CL. ekaramad@, can > > you take a look at the flake? > > Sorry for the delay. I am currently in chrome bootcamp. I can take a look > tomorrow afternoon. I took a stab at this and apparently the problem is with the internals of the text_input_test_utils.cc. I will submit a followup CL for this bug. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
