Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2354793003:
Browser Side TextInputState Tracking for Android (Closed)
|
Created:
4 years, 3 months ago by EhsanK Modified:
4 years ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, nona+watch_chromium.org, darin-cc_chromium.org, James Su, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBrowser Side TextInputState Tracking for Android
To enable OOPIF and Top Document Isolation for Android, we need to track
the text input state from multiple RenderWidgetHosts. To this end, the
TextInputStateChanged updates should be routed through TextInputManager
which keeps a map of all RenderWidgetHost(View) and their corresponding
TextInputState.
This CL will also enable some site-per-process tests on android.
BUG=578168, 602723
Committed: https://crrev.com/06b277144e9e48f754e82ec5224076eeeac347b5
Cr-Commit-Position: refs/heads/master@{#434683}
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : Rebased #Patch Set 4 : Rebased #
Total comments: 4
Patch Set 5 : Addressing kenrb@'s comments #
Total comments: 11
Patch Set 6 : Rebased #Patch Set 7 : Addressing creis@'s comments #
Total comments: 4
Patch Set 8 : Addressing shuchen@'s comment #Patch Set 9 : Rebased + Removed unused Forward Dec #
Messages
Total messages: 54 (41 generated)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
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: This issue passed the CQ dry run.
ekaramad@chromium.org changed reviewers: + creis@chromium.org, kenrb@chromium.org, shuchen@chromium.org
Please take a look at this CL. cries@: general IME and content/ permissions. kenrb@: general IME. shuchen@: Android IME changes. Thanks!
Looks good, just some comments on the test file. https://codereview.chromium.org/2354793003/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (left): https://codereview.chromium.org/2354793003/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:1045: LookUpStringForRangeRoutesToFocusedWidget) { Why is this test being removed? https://codereview.chromium.org/2354793003/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2354793003/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:621: #if !defined(OS_ANDROID) You removed the comment with the bug reference for enabling tests on Android, but there are still many disabled tests. A comment should be preserved here explaining it.
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: This issue passed the CQ dry run.
Thanks Ken! PTAL. https://codereview.chromium.org/2354793003/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (left): https://codereview.chromium.org/2354793003/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:1045: LookUpStringForRangeRoutesToFocusedWidget) { On 2016/11/02 17:22:48, kenrb wrote: > Why is this test being removed? I lost this part in the rebase (the conflict was in this file only). I have added this back in the new version. Thanks for noticing this. https://codereview.chromium.org/2354793003/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2354793003/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:621: #if !defined(OS_ANDROID) On 2016/11/02 17:22:48, kenrb wrote: > You removed the comment with the bug reference for enabling tests on Android, > but there are still many disabled tests. A comment should be preserved here > explaining it. I added it back to where !defined(OS_ANDROID) will be.
[+site-isolation-reviews] LGTM with nits, and one question about AreDifferentTextInputStates. https://codereview.chromium.org/2354793003/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2354793003/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:1131: #endif nit: Can you add a comment here about which ifdef this is closing? https://codereview.chromium.org/2354793003/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2354793003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:770: TextInputManager* text_input_manager, RenderWidgetHostViewAura's version of this method starts with: DCHECK_EQ(text_input_manager_, text_input_manager); Should we be doing that here, given the GetTextInputManager() call below? https://codereview.chromium.org/2354793003/diff/80001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2354793003/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:29: // the state. So the new state is always different. nit: Comma, not period. (Second sentence is not a sentence.) Still, this behavior seems a bit unexpected. Wouldn't this be confusing if future code added a call to AreDifferentTextInputStates and it returns true for two identical states? https://codereview.chromium.org/2354793003/diff/80001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/2354793003/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:127: // Returns the currently stored TextInputState for |view|. An state of nullptr nit: A state https://codereview.chromium.org/2354793003/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:129: // the view. If |view| is null, and there is an |active_view_|, the state for nit: No comma after null
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...
Sorry it took me a while to update this CL. Thank you Charlie for reviews! PTAL. shuchen@: Could you please a look at this change esp. RenderWidgetHostViewAndroid changes. Thanks! https://codereview.chromium.org/2354793003/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2354793003/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:1131: #endif On 2016/11/02 22:23:13, Charlie Reis (away) wrote: > nit: Can you add a comment here about which ifdef this is closing? Done. https://codereview.chromium.org/2354793003/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2354793003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:770: TextInputManager* text_input_manager, On 2016/11/02 22:23:13, Charlie Reis (away) wrote: > RenderWidgetHostViewAura's version of this method starts with: > DCHECK_EQ(text_input_manager_, text_input_manager); > > Should we be doing that here, given the GetTextInputManager() call below? I added it here for now but my mindeset behind this check was for GuestViews since with nested WebContents there could be more than one of these at some point. We do not have guests here on Android. However, it might still be worth putting it here in case someone assigns a TIM to another tab's TIM (or for future testing). WDYT? https://codereview.chromium.org/2354793003/diff/80001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2354793003/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:29: // the state. So the new state is always different. On 2016/11/02 22:23:13, Charlie Reis (away) wrote: > nit: Comma, not period. (Second sentence is not a sentence.) > > Still, this behavior seems a bit unexpected. Wouldn't this be confusing if > future code added a call to AreDifferentTextInputStates and it returns true for > two identical states? Acknowledged...and good point. I believe what needs to be changed is the method name. We only use this to determine if TextInputState should be updated (given the old and new state). https://codereview.chromium.org/2354793003/diff/80001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/2354793003/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:127: // Returns the currently stored TextInputState for |view|. An state of nullptr On 2016/11/02 22:23:13, Charlie Reis (away) wrote: > nit: A state Done. https://codereview.chromium.org/2354793003/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:129: // the view. If |view| is null, and there is an |active_view_|, the state for On 2016/11/02 22:23:13, Charlie Reis (away) wrote: > nit: No comma after null Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2354793003/diff/120001/content/browser/render... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2354793003/diff/120001/content/browser/render... content/browser/renderer_host/text_input_manager.cc:27: #else Can you please add #elif defined(OS_ANDROID) for this? And "NOTREACHED();" for #else for other platforms?
Thanks for the reviews. PTAL with one Question regarding remaining platform for either shuchen@ or creis@. https://codereview.chromium.org/2354793003/diff/120001/content/browser/render... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2354793003/diff/120001/content/browser/render... content/browser/renderer_host/text_input_manager.cc:27: #else On 2016/11/21 02:10:53, Shu Chen wrote: > Can you please add #elif defined(OS_ANDROID) for this? > And "NOTREACHED();" for #else for other platforms? Acknowledged and done. Although I am not sure what platform would be neither Aura, Mac, or Android and for that platform, what is the best fallback behavior (return false or true?).
https://codereview.chromium.org/2354793003/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2354793003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:770: TextInputManager* text_input_manager, On 2016/11/18 19:55:59, EhsanK wrote: > On 2016/11/02 22:23:13, Charlie Reis (away) wrote: > > RenderWidgetHostViewAura's version of this method starts with: > > DCHECK_EQ(text_input_manager_, text_input_manager); > > > > Should we be doing that here, given the GetTextInputManager() call below? > > I added it here for now but my mindeset behind this check was for GuestViews > since with nested WebContents there could be more than one of these at some > point. We do not have guests here on Android. However, it might still be worth > putting it here in case someone assigns a TIM to another tab's TIM (or for > future testing). WDYT? Thanks-- I think it's worth keeping. https://codereview.chromium.org/2354793003/diff/120001/content/browser/render... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2354793003/diff/120001/content/browser/render... content/browser/renderer_host/text_input_manager.cc:27: #else On 2016/11/21 20:44:21, EhsanK wrote: > On 2016/11/21 02:10:53, Shu Chen wrote: > > Can you please add #elif defined(OS_ANDROID) for this? > > And "NOTREACHED();" for #else for other platforms? > > Acknowledged and done. Although I am not sure what platform would be neither > Aura, Mac, or Android and for that platform, what is the best fallback behavior > (return false or true?). If you really don't want it to be handled, you can put #error so it won't compile. But I think there will be other corner case platforms that don't fall into these buckets. (I don't know much about Ozone, for example.) I think it's fine to just pick a value for now, since people on that platform will have to decide when they hit your NOTREACHED.
lgtm
Thanks for the reviews. I will try to CQ soon. https://codereview.chromium.org/2354793003/diff/120001/content/browser/render... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2354793003/diff/120001/content/browser/render... content/browser/renderer_host/text_input_manager.cc:27: #else On 2016/11/23 23:27:21, Charlie Reis (slow) wrote: > On 2016/11/21 20:44:21, EhsanK wrote: > > On 2016/11/21 02:10:53, Shu Chen wrote: > > > Can you please add #elif defined(OS_ANDROID) for this? > > > And "NOTREACHED();" for #else for other platforms? > > > > Acknowledged and done. Although I am not sure what platform would be neither > > Aura, Mac, or Android and for that platform, what is the best fallback > behavior > > (return false or true?). > > If you really don't want it to be handled, you can put #error so it won't > compile. But I think there will be other corner case platforms that don't fall > into these buckets. (I don't know much about Ozone, for example.) I think it's > fine to just pick a value for now, since people on that platform will have to > decide when they hit your NOTREACHED. Acknowledged.
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: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) 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) chromeos_x86-generic_chromium_compile_only_ng 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_compile_dbg_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)
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_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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: This issue passed the CQ dry run.
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, shuchen@chromium.org Link to the patchset: https://codereview.chromium.org/2354793003/#ps160001 (title: "Rebased + Removed unused Forward Dec")
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": 160001, "attempt_start_ts": 1480355421179880,
"parent_rev": "f3c244462d430c397c437ca36343ed0575c03788", "commit_rev":
"0282f508807f90ee48ba84c859bd54f235f27830"}
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Browser Side TextInputState Tracking for Android To enable OOPIF and Top Document Isolation for Android, we need to track the text input state from multiple RenderWidgetHosts. To this end, the TextInputStateChanged updates should be routed through TextInputManager which keeps a map of all RenderWidgetHost(View) and their corresponding TextInputState. This CL will also enable some site-per-process tests on android. BUG=578168,602723 ========== to ========== Browser Side TextInputState Tracking for Android To enable OOPIF and Top Document Isolation for Android, we need to track the text input state from multiple RenderWidgetHosts. To this end, the TextInputStateChanged updates should be routed through TextInputManager which keeps a map of all RenderWidgetHost(View) and their corresponding TextInputState. This CL will also enable some site-per-process tests on android. BUG=578168,602723 Committed: https://crrev.com/06b277144e9e48f754e82ec5224076eeeac347b5 Cr-Commit-Position: refs/heads/master@{#434683} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/06b277144e9e48f754e82ec5224076eeeac347b5 Cr-Commit-Position: refs/heads/master@{#434683} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
