|
|
Created:
4 years, 5 months ago by EhsanK Modified:
4 years, 4 months 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTrack TextInputState from multiple RenderWidgets in TextInputManager (Mac)
This CL implements the logic to track TextInputState in TextInputManager
for OOPIF structure on Mac. A similar implementation was landed on Aura
here: https://codereview.chromium.org/1948343002.
The CL also activates the corresponding TextInputManager tests on Mac.
BUG=578168, 602723, 602427
Committed: https://crrev.com/c3ef49d3c300e954f631acfe601b6276a3b5b89f
Cr-Commit-Position: refs/heads/master@{#408199}
Patch Set 1 #Patch Set 2 : Enabled Interactive Tests on Mac #Patch Set 3 : Rebased + Made GetTextInputType() private. #Patch Set 4 : GetTextInputType() must be public to be accessed by the Cocoa view #
Total comments: 12
Patch Set 5 : Addressing kenrb@'s and creis@'s comments #Patch Set 6 : Rebased #Patch Set 7 : Disabled SitePerProcessTextInputManagerTest.TrackTextSelectionForAllFrames on Mac since the corresp… #
Total comments: 6
Patch Set 8 : Addressing creis@'s comments #
Messages
Total messages: 46 (30 generated)
Description was changed from ========== wip-Track TextInputState from multiple RenderWidgets in TextInputManager (Mac) This CL implements the logic to track TextInputState in TextInputManager for OOPIF structure on Mac. A similar implementation was landed on Aura here: https://codereview.chromium.org/1948343002. The CL also activates the corresponding TextInputManager tests on Mac. BUG=578168,602723,602427 ========== to ========== Track TextInputState from multiple RenderWidgets in TextInputManager (Mac) This CL implements the logic to track TextInputState in TextInputManager for OOPIF structure on Mac. A similar implementation was landed on Aura here: https://codereview.chromium.org/1948343002. The CL also activates the corresponding TextInputManager tests on Mac. BUG=578168,602723,602427 ==========
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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) 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: 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...
ekaramad@chromium.org changed reviewers: + creis@chromium.org, kenrb@chromium.org
Ken, Charlie: PTAL. I will continue adding more browser side code for Mac and enable tests as features are implemented.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2166573003/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2166573003/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:510: // corresponding feature is implemented (http://crbug.com/578168). Is it much extra to implement all of it in this CL? It appears to be just a few more changes to RenderWidgetHostViewMac. https://codereview.chromium.org/2166573003/diff/60001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2166573003/diff/60001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:485: # TODO(ekaramad): These tests are activated for aura only. They should nit: Comment needs update. https://codereview.chromium.org/2166573003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2166573003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.cc:406: #if defined(USE_AURA) || defined(OS_MACOSX) Should this be replaced with #if !defined(OS_ANDROID) ? https://codereview.chromium.org/2166573003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2166573003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:914: } This closing brace is indented too far.
Exciting to see Mac moving over as well! LGTM once Ken's happy. https://codereview.chromium.org/2166573003/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2166573003/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:29: // TODO(ekaramad): The following tests are only active on aura platforms. After This looks stale now. https://codereview.chromium.org/2166573003/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:510: // corresponding feature is implemented (http://crbug.com/578168). On 2016/07/21 21:48:16, kenrb wrote: > Is it much extra to implement all of it in this CL? It appears to be just a few > more changes to RenderWidgetHostViewMac. I'm ok either way, but I have a slight preference to do it incrementally like this. The smaller CLs have been easy to review when they have less going on (assuming the intermediate stages are safe).
Patchset #6 (id:100001) has been deleted
Thanks for the reviews! PTAL. https://codereview.chromium.org/2166573003/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2166573003/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:29: // TODO(ekaramad): The following tests are only active on aura platforms. After On 2016/07/22 20:59:35, Charlie Reis wrote: > This looks stale now. Acknowledged. https://codereview.chromium.org/2166573003/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:510: // corresponding feature is implemented (http://crbug.com/578168). On 2016/07/22 20:59:35, Charlie Reis wrote: > On 2016/07/21 21:48:16, kenrb wrote: > > Is it much extra to implement all of it in this CL? It appears to be just a > few > > more changes to RenderWidgetHostViewMac. > > I'm ok either way, but I have a slight preference to do it incrementally like > this. The smaller CLs have been easy to review when they have less going on > (assuming the intermediate stages are safe). I have fixes for some of the other IME-related tracking in Mac but not all. I figured 1) make CLs small and 2) land changes asap so that I can catch regressions sooner. My own suggestion is to have at least TextInputState tracking on a different CL. I guess I will be able to manage to fit all the other state trackings in one CL. That being said I guess I should be able to have one CL for all of Mac by the end of the week if that is the direction we are going. https://codereview.chromium.org/2166573003/diff/60001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2166573003/diff/60001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:485: # TODO(ekaramad): These tests are activated for aura only. They should On 2016/07/21 21:48:16, kenrb wrote: > nit: Comment needs update. Acknowledged. https://codereview.chromium.org/2166573003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2166573003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.cc:406: #if defined(USE_AURA) || defined(OS_MACOSX) On 2016/07/21 21:48:16, kenrb wrote: > Should this be replaced with #if !defined(OS_ANDROID) ? Done. I guess I was just being very specific. https://codereview.chromium.org/2166573003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2166573003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:914: } On 2016/07/21 21:48:16, kenrb wrote: > This closing brace is indented too far. Acknowledged.
Thanks! https://codereview.chromium.org/2166573003/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2166573003/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:510: // corresponding feature is implemented (http://crbug.com/578168). On 2016/07/25 17:12:02, EhsanK wrote: > On 2016/07/22 20:59:35, Charlie Reis wrote: > > On 2016/07/21 21:48:16, kenrb wrote: > > > Is it much extra to implement all of it in this CL? It appears to be just a > > few > > > more changes to RenderWidgetHostViewMac. > > > > I'm ok either way, but I have a slight preference to do it incrementally like > > this. The smaller CLs have been easy to review when they have less going on > > (assuming the intermediate stages are safe). > > I have fixes for some of the other IME-related tracking in Mac but not all. I > figured 1) make CLs small and 2) land changes asap so that I can catch > regressions sooner. My own suggestion is to have at least TextInputState > tracking on a different CL. I guess I will be able to manage to fit all the > other state trackings in one CL. > > That being said I guess I should be able to have one CL for all of Mac by the > end of the week if that is the direction we are going. Let's keep the CLs small and proceed incrementally if possible.
lgtm
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Thanks for the reviews! I will commit after 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...
On 2016/07/26 20:49:22, EhsanK wrote: > Thanks for the reviews! I will commit after dry-run. Why is disabling the test on Mac in patch 7 the right thing? Please don't land yet.
On 2016/07/26 23:56:21, Charlie Reis wrote: > On 2016/07/26 20:49:22, EhsanK wrote: > > Thanks for the reviews! I will commit after dry-run. > > Why is disabling the test on Mac in patch 7 the right thing? Please don't land > yet. I landed the patch for TextSelection tracking for aura and failed to include it inside the #ifdef for Aura only (during rebase here). The test cannot be active since I have not implemented the feature for Mac.
On 2016/07/27 00:08:57, EhsanK wrote: > On 2016/07/26 23:56:21, Charlie Reis wrote: > > On 2016/07/26 20:49:22, EhsanK wrote: > > > Thanks for the reviews! I will commit after dry-run. > > > > Why is disabling the test on Mac in patch 7 the right thing? Please don't > land > > yet. > > I landed the patch for TextSelection tracking for aura and failed to include it > inside the #ifdef for Aura only (during rebase here). The test cannot be active > since I have not implemented the feature for Mac. PTAL@ patch 6 (Rebased) and the diff to patch 5 on site_per_process_text_input_browsertest.cc.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for clarifying. From the diff between patches, I first couldn't tell if we were turning off the only Mac test coverage, but I'm glad to see there are still several tests. LGTM, with one minor question about the two separate ifdef blocks. https://codereview.chromium.org/2166573003/diff/140001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2166573003/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:523: ResetStateAfterBrowserNavigation) { Ok, glad to see the tests here and above are running on Mac now. https://codereview.chromium.org/2166573003/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:657: // (crbug.com/602723). This part confuses me a bit. Is there a meaningful difference between the Aura-only block above and the Aura-only block below? Both seem to have TODOs to make them work on other platforms, so I'm wondering if it's simpler to combine them. (Or do the InputMethodObserver-specific tests deserve a block of their own? I'm ok with that if so.)
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...
Thanks Charlie! Updated the comments and added a few notes. https://codereview.chromium.org/2166573003/diff/140001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2166573003/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:523: ResetStateAfterBrowserNavigation) { On 2016/07/27 16:14:18, Charlie Reis wrote: > Ok, glad to see the tests here and above are running on Mac now. Yes, and these should also run on Android when we implement IME for android. https://codereview.chromium.org/2166573003/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:542: #if defined(USE_AURA) I guess all of the following will eventually be active run on Mac as well. But some, if not all, will not run on Android as the selection related state is updated differently on Android. So we will have to deal with some #ifdefs for life. https://codereview.chromium.org/2166573003/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:657: // (crbug.com/602723). On 2016/07/27 16:14:18, Charlie Reis wrote: > This part confuses me a bit. Is there a meaningful difference between the > Aura-only block above and the Aura-only block below? Both seem to have TODOs to > make them work on other platforms, so I'm wondering if it's simpler to combine > them. > > (Or do the InputMethodObserver-specific tests deserve a block of their own? I'm > ok with that if so.) Sorry for the confusion. The block above is Aura only because I have not implemented the features. I expect them to move to Mac + Aura 1by1 as I finish IME for Mac. The ones below were originally intended to be InputMethodObserver tests as you suggest. But as of now I have the only one below for aura which is actually a unit test. I guess I should be able to move it to unit tests and get rid of this block for good. It will be ideal though to have proper end-to-end tests for IME which will only use IME extension (which only works on ChromeOS for now). I will try to revisit this issue after I investigate the IME api in more depths. Although, the IME API has its own tests for ChromeOS. https://codereview.chromium.org/2166573003/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:718: // test. We should move it to somewhere more relevant. I might remove this test and put it with other unit tests as soon as I figure out the window focus issue I had before. I have added the bug number to this TODO btw.
LGTM
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 kenrb@chromium.org Link to the patchset: https://codereview.chromium.org/2166573003/#ps160001 (title: "Addressing creis@'s comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Track TextInputState from multiple RenderWidgets in TextInputManager (Mac) This CL implements the logic to track TextInputState in TextInputManager for OOPIF structure on Mac. A similar implementation was landed on Aura here: https://codereview.chromium.org/1948343002. The CL also activates the corresponding TextInputManager tests on Mac. BUG=578168,602723,602427 ========== to ========== Track TextInputState from multiple RenderWidgets in TextInputManager (Mac) This CL implements the logic to track TextInputState in TextInputManager for OOPIF structure on Mac. A similar implementation was landed on Aura here: https://codereview.chromium.org/1948343002. The CL also activates the corresponding TextInputManager tests on Mac. BUG=578168,602723,602427 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Track TextInputState from multiple RenderWidgets in TextInputManager (Mac) This CL implements the logic to track TextInputState in TextInputManager for OOPIF structure on Mac. A similar implementation was landed on Aura here: https://codereview.chromium.org/1948343002. The CL also activates the corresponding TextInputManager tests on Mac. BUG=578168,602723,602427 ========== to ========== Track TextInputState from multiple RenderWidgets in TextInputManager (Mac) This CL implements the logic to track TextInputState in TextInputManager for OOPIF structure on Mac. A similar implementation was landed on Aura here: https://codereview.chromium.org/1948343002. The CL also activates the corresponding TextInputManager tests on Mac. BUG=578168,602723,602427 Committed: https://crrev.com/c3ef49d3c300e954f631acfe601b6276a3b5b89f Cr-Commit-Position: refs/heads/master@{#408199} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c3ef49d3c300e954f631acfe601b6276a3b5b89f Cr-Commit-Position: refs/heads/master@{#408199} |