Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2694543002:
[refactor] Cleanup IME State in RenderWidgetHostViewMac which is already tracked by TextInputManager (Closed)
|
Created:
3 years, 10 months ago by EhsanK Modified:
3 years, 9 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, mac-reviews_chromium.org, jam, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org, James Su, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[refactor] Cleanup IME State in RenderWidgetHostViewMac which is already tracked by TextInputManager
This CL will cleanup some text selection and composition information related
variables inside RenderWidgetHostViewMac. These values are already tracked by
TextInputManager so keeping them inside RenderWidgetHostViewMac is costly. In
line with this, some changes to TextInputManager::TextSelection led to small
changes to both RenderWidgetHostViewAura and RenderWidgetHostViewAndroid.
BUG=602427
Review-Url: https://codereview.chromium.org/2694543002
Cr-Commit-Position: refs/heads/master@{#454367}
Committed: https://chromium.googlesource.com/chromium/src/+/374b2669b0af7cd88882cff28555f5aaa04c52d7
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : Fixing some compile errors #Patch Set 4 : Fixed a compile error on Aura #Patch Set 5 : Fixed a compile error on Mac #
Total comments: 33
Patch Set 6 : Rebased #Patch Set 7 : Addressing creis@'s comments #Patch Set 8 : Rebased #Patch Set 9 : Fixed compile errors #Patch Set 10 : Caching |selected_text_| again. #Patch Set 11 : Fixed a compile error #Patch Set 12 : Fixed another compile error for Android test #Messages
Total messages: 66 (54 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: + avi@chromium.org, creis@chromium.org
Hello Charlie, Avi, This is the refactor promised after : https://codereview.chromium.org/2659433002/#msg12 Could you please take a look? avi@: most of the change is in mac code creis@: other platforms Thanks!
Looks reasonable. LGTM https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:1129: // TODO: This will not work with OOPIFs (https://crbug.com/659753). Is this a regression or a note of something that is already the case?
Thanks Avi! https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:1129: // TODO: This will not work with OOPIFs (https://crbug.com/659753). On 2017/02/13 17:51:32, Avi wrote: > Is this a regression or a note of something that is already the case? This is just a note. This is a feature which currently does not work with Mac. That being said I am not sure if it is worth fixing (the solution seems complicated). There is already a voice over feature (Cmd + F5) which I guess works on top of OOPIFs and everything.
Thanks! Mostly looks good-- a few clarifying questions and nits below. https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:785: content_view_core_->OnSelectionChanged( Is it ok that this is now called even when the selection is empty (i.e., when GetSelectedText returned false in the past)? Hopefully so, but I remember some differences between platforms based on whether methods like this were called selectively or not. https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2370: GetTextInputManager()->GetTextSelection(focused_view); Sanity check: Hopefully this won't return null since focused_view is non-null? (I suppose we would have had a crash before if so.) https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.cc:171: return GetTextInputManager()->GetTextSelection(this)->selected_text(); Sanity check: As before, are we safe from GetTextSelection returning null since we're passing in a non-null RWHV? https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:480: // |is_guest_view_hack_| is true, then it will return the TextSelection nit: Remove double space between "then it" https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:331: // Updating the selected text. nit: Update https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:338: << "The text can not fully cover range (selection's end point " nit: s/can not/cannot/ https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:343: LOG(WARNING) << "The text ca not cover range (selection range's starting " nit: s/ca not/cannot/ https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:346: selected_text_.append(text.substr(pos, n)); Sanity check: substr won't go past the end of the string if n is too large, right? (It's documented that way, but always worth thinking about.) https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:95: // This struct is used to store text selection related information for views. nit: class https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:109: const base::string16& text() const { return text_; } I don't understand the difference between selected_text() and text(). Please elaborate in a comment, and maybe clarify on the declaration of |text_| below as well (since it's easy to think that it would hold the selected text inside a class called TextSelection). https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:120: // either of |text| or |range| are empty, or when there values are nit: s/there/their/ https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:124: // The text inside and around the current selection range. As noted above, please clarify how this differs from selected_text_. https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:125: base::string16 text_; Is it inefficient to store these two strings separately?
Description was changed from ========== [refactor] Cleanup IME State in RenderWidgetHostViewMac which is already tracked by TextInputManager This CL will cleanup some text selection and composition information related variables inside RenderWidgetHostViewMac. These values are already tracked by TextInputManager so keeping them inside RenderWidgetHostViewMac is costly. In line with this, some changes to TextInputManager::TextSelection led to small changes to both RenderWidgetHostViewAura and RenderWidgetHostViewAndroid. BUG=602427 ========== to ========== [refactor] Cleanup IME State in RenderWidgetHostViewMac which is already tracked by TextInputManager This CL will cleanup some text selection and composition information related variables inside RenderWidgetHostViewMac. These values are already tracked by TextInputManager so keeping them inside RenderWidgetHostViewMac is costly. In line with this, some changes to TextInputManager::TextSelection led to small changes to both RenderWidgetHostViewAura and RenderWidgetHostViewAndroid. BUG=602427 ==========
Thanks Charlie, PTAL. https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:785: content_view_core_->OnSelectionChanged( On 2017/02/14 00:46:42, Charlie Reis wrote: > Is it ok that this is now called even when the selection is empty (i.e., when > GetSelectedText returned false in the past)? Hopefully so, but I remember some > differences between platforms based on whether methods like this were called > selectively or not. A couple of things that come to mind: 1- We were indeed calling this method with empty string before as well since GetSelectedText() would only return false if the range would not cover the text (i.e., bad values from renderer). 2- Looking at this again line 780 seems redundant since selected_text() is empty when |selection.range| is empty. 3- The usage of this call is apparently in SelectionPopupController and ContextualSearchClient for both of which I guess passing an empty string is quite normal. 4- Before, when selection.range would not intersect with selection.text, we were not calling this method. Now we are calling it with empty string. But FWIW, range not covering text is an error somewhere and is not even supposed to happen (and I don't think it does necessarily). I can revert this back to not calling OnSelectionChanged when range is invalid (does not cover text), but that seems like extra checks for something very rare and also not checking for it does not seem to have an repercussions. WDYT? https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2370: GetTextInputManager()->GetTextSelection(focused_view); On 2017/02/14 00:46:42, Charlie Reis wrote: > Sanity check: Hopefully this won't return null since focused_view is non-null? > (I suppose we would have had a crash before if so.) Thanks for this question (I spent some time reviewing how we do things here). We do not return null. The potential issue in calling GetTextSelection or methods of this sort which receive the pointer to view as an argument is that we then call std::map<ViewPtr, Struct>::at(ViewPtr) internally, it might throw out_of_range exception (I don't actually know if we throw exceptions at all). We not register all views with TextInputManager in their ctor. We also unregister in the dtor of RWHVBase (last line). Since we get focused view from focused widget, and given that by that time RWH has lost its reference to RWHV we are quite safe, as in an unregistered view can never be a focused view in this context. https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.cc:171: return GetTextInputManager()->GetTextSelection(this)->selected_text(); On 2017/02/14 00:46:42, Charlie Reis wrote: > Sanity check: As before, are we safe from GetTextSelection returning null since > we're passing in a non-null RWHV? In line 169 if we cannot register with TextInputManager we will early return. But since we got here the view is registered so there is already a struct memory for it (selection can never be nullptr). https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:480: // |is_guest_view_hack_| is true, then it will return the TextSelection On 2017/02/14 00:46:42, Charlie Reis wrote: > nit: Remove double space between "then it" Done. https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:331: // Updating the selected text. On 2017/02/14 00:46:42, Charlie Reis wrote: > nit: Update Done. https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:338: << "The text can not fully cover range (selection's end point " On 2017/02/14 00:46:42, Charlie Reis wrote: > nit: s/can not/cannot/ Done. https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:343: LOG(WARNING) << "The text ca not cover range (selection range's starting " On 2017/02/14 00:46:42, Charlie Reis wrote: > nit: s/ca not/cannot/ Funny I have copied around this text and the one above multiple times and never found it fit to fix the typo in the constant. Thanks! https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:346: selected_text_.append(text.substr(pos, n)); On 2017/02/14 00:46:42, Charlie Reis wrote: > Sanity check: substr won't go past the end of the string if n is too large, > right? (It's documented that way, but always worth thinking about.) If pos is past the length we will get an out_of_range exception but line 342 stops that. If n is longer than the remainder of the string we are fine as apparently this behavior is defined and the returned string will be limited to [pos, size()) http://en.cppreference.com/w/cpp/string/basic_string/substr https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:95: // This struct is used to store text selection related information for views. On 2017/02/14 00:46:43, Charlie Reis wrote: > nit: class Acknowledged. https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:109: const base::string16& text() const { return text_; } On 2017/02/14 00:46:43, Charlie Reis wrote: > I don't understand the difference between selected_text() and text(). Please > elaborate in a comment, and maybe clarify on the declaration of |text_| below as > well (since it's easy to think that it would hold the selected text inside a > class called TextSelection). I found it a bit confusing myself. |text_| is supposed to be a super set of |selected_text_|. See the comment here: https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rc... Apparently, if |text.length()| could be as many as 200 + |selected_text_.length()| or so characters. It includes 100 chars before and after selection. Range is also supposed to (along with offset) identify the selected text inside text. https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:120: // either of |text| or |range| are empty, or when there values are On 2017/02/14 00:46:43, Charlie Reis wrote: > nit: s/there/their/ Done. https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:124: // The text inside and around the current selection range. On 2017/02/14 00:46:43, Charlie Reis wrote: > As noted above, please clarify how this differs from selected_text_. |selected_text_|, as I understand, is the highlighted text. |text_| on the other hand is supposed to include |selected_text_| and beyond. So if I were to select the word inside quotation "here", selected text would be "here" and |text| would be perhaps most of this paragraph. https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:125: base::string16 text_; On 2017/02/14 00:46:43, Charlie Reis wrote: > Is it ineficient to store these two strings separately? Here is my analysis: Cons (of caching): 1- Extra memory for |selected_text_|. a- If caret or selection is inside <input>, |text_| could be much larger than |selected_text_| (200s chars as opposed to single digit number of chars). In this case there is practically no extra memory usage. b- If selection is in a <div> or some noneditable content, |text_| seems to be equal to |selected_text_|. So the cost is doubling memory usage (which might not be much considering the fact that most practical selections are several words long). 2- Finding |selected_text_| for any selection update (regardless of whether or not this will be used later) a- On Android we do this at least once. b- On Mac we do this at least once. If we end up right clicking on any selected text, we call selected_text() exactly 30 times! (on my machine and I am not sure why 30). c- On linux as well we call this at least once. So in general I guess it is not a clear call on whether it is more efficient or less. I think on Mac caching makes more sense since we usually select by clicking on text which pops up context menu (And the 30 consecutive calls!). I am also happy to remove this and make a method to calculate selected text every time. WDYT?
Sorry for the delay-- LGTM with one question about the efficiency of text_ vs selected_text_. https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:785: content_view_core_->OnSelectionChanged( On 2017/02/14 18:48:58, EhsanK wrote: > On 2017/02/14 00:46:42, Charlie Reis wrote: > > Is it ok that this is now called even when the selection is empty (i.e., when > > GetSelectedText returned false in the past)? Hopefully so, but I remember > some > > differences between platforms based on whether methods like this were called > > selectively or not. > A couple of things that come to mind: > 1- We were indeed calling this method with empty string before as well since > GetSelectedText() would only return false if the range would not cover the text > (i.e., bad values from renderer). > 2- Looking at this again line 780 seems redundant since selected_text() is empty > when |selection.range| is empty. > 3- The usage of this call is apparently in SelectionPopupController and > ContextualSearchClient for both of which I guess passing an empty string is > quite normal. > 4- Before, when selection.range would not intersect with selection.text, we were > not calling this method. Now we are calling it with empty string. But FWIW, > range not covering text is an error somewhere and is not even supposed to happen > (and I don't think it does necessarily). > > I can revert this back to not calling OnSelectionChanged when range is invalid > (does not cover text), but that seems like extra checks for something very rare > and also not checking for it does not seem to have an repercussions. WDYT? Yeah, no need-- your version sounds safe. Thanks for the analysis. https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2370: GetTextInputManager()->GetTextSelection(focused_view); On 2017/02/14 18:48:58, EhsanK wrote: > On 2017/02/14 00:46:42, Charlie Reis wrote: > > Sanity check: Hopefully this won't return null since focused_view is non-null? > > > (I suppose we would have had a crash before if so.) > > Thanks for this question (I spent some time reviewing how we do things here). > > We do not return null. The potential issue in calling GetTextSelection or > methods of this sort which receive the pointer to view as an argument is that we > then call std::map<ViewPtr, Struct>::at(ViewPtr) internally, it might throw > out_of_range exception (I don't actually know if we throw exceptions at all). We don't use exceptions, but we would crash in that case. > > We not register all views with TextInputManager in their ctor. We also > unregister in the dtor of RWHVBase (last line). Since we get focused view from > focused widget, and given that by that time RWH has lost its reference to RWHV > we are quite safe, as in an unregistered view can never be a focused view in > this context. Sounds good (assuming you meant s/not/now/ in the first sentence). :) https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.cc:171: return GetTextInputManager()->GetTextSelection(this)->selected_text(); On 2017/02/14 18:48:58, EhsanK wrote: > On 2017/02/14 00:46:42, Charlie Reis wrote: > > Sanity check: As before, are we safe from GetTextSelection returning null > since > > we're passing in a non-null RWHV? > > In line 169 if we cannot register with TextInputManager we will early return. > But since we got here the view is registered so there is already a struct memory > for it (selection can never be nullptr). Acknowledged. https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:125: base::string16 text_; On 2017/02/14 18:48:59, EhsanK wrote: > On 2017/02/14 00:46:43, Charlie Reis wrote: > > Is it ineficient to store these two strings separately? > Here is my analysis: > Cons (of caching): > 1- Extra memory for |selected_text_|. > a- If caret or selection is inside <input>, |text_| could be much larger than > |selected_text_| (200s chars as opposed to single digit number of chars). In > this case there is practically no extra memory usage. > b- If selection is in a <div> or some noneditable content, |text_| seems to be > equal to |selected_text_|. So the cost is doubling memory usage (which might not > be much considering the fact that most practical selections are several words > long). > 2- Finding |selected_text_| for any selection update (regardless of whether or > not this will be used later) > a- On Android we do this at least once. > b- On Mac we do this at least once. If we end up right clicking on any > selected text, we call selected_text() exactly 30 times! (on my machine and I am > not sure why 30). > c- On linux as well we call this at least once. > > So in general I guess it is not a clear call on whether it is more efficient or > less. I think on Mac caching makes more sense since we usually select by > clicking on text which pops up context menu (And the 30 consecutive calls!). > > I am also happy to remove this and make a method to calculate selected text > every time. WDYT? I'm mainly worried about the Ctrl+A on a large page case, where we're doubling a possibly very large string. But maybe that's uncommon-- do you know if we happen to have stats? Given the 30 calls on Mac, though, it's probably worthwhile to have it cached. One thought: have you considered storing a max-100-char prefix and suffix instead of duplicating the middle? I don't want to put too much effort into premature optimization, but it does seem like there could be realistic cases where we might use a lot more memory than needed here.
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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
Patchset #8 (id:140001) has been deleted
Patchset #8 (id:160001) has been deleted
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_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) 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...
Patchset #10 (id:220001) has been deleted
Patchset #8 (id:180001) has been deleted
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 Charlie! In short, I am not caching in the final patch. Please let me know if you'd prefer the latest patch over patch 7? https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:125: base::string16 text_; On 2017/02/21 22:57:38, Charlie Reis wrote: > On 2017/02/14 18:48:59, EhsanK wrote: > > On 2017/02/14 00:46:43, Charlie Reis wrote: > > > Is it ineficient to store these two strings separately? > > Here is my analysis: > > Cons (of caching): > > 1- Extra memory for |selected_text_|. > > a- If caret or selection is inside <input>, |text_| could be much larger > than > > |selected_text_| (200s chars as opposed to single digit number of chars). In > > this case there is practically no extra memory usage. > > b- If selection is in a <div> or some noneditable content, |text_| seems to > be > > equal to |selected_text_|. So the cost is doubling memory usage (which might > not > > be much considering the fact that most practical selections are several words > > long). > > 2- Finding |selected_text_| for any selection update (regardless of whether or > > not this will be used later) > > a- On Android we do this at least once. > > b- On Mac we do this at least once. If we end up right clicking on any > > selected text, we call selected_text() exactly 30 times! (on my machine and I > am > > not sure why 30). > > c- On linux as well we call this at least once. > > > > So in general I guess it is not a clear call on whether it is more efficient > or > > less. I think on Mac caching makes more sense since we usually select by > > clicking on text which pops up context menu (And the 30 consecutive calls!). > > > > I am also happy to remove this and make a method to calculate selected text > > every time. WDYT? > > I'm mainly worried about the Ctrl+A on a large page case, where we're doubling a > possibly very large string. But maybe that's uncommon-- do you know if we > happen to have stats? Yes it will be consuming much more (2x) in those cases. I don't know if we do (not sure how I can track that down), but I guess Ctrl/Command+A could be quite common (at least I do when copying text in between docs). > Given the 30 calls on Mac, though, it's probably worthwhile to have it cached. > Yes it used to be cached inside RWHVMac (so we were having the 2x memory issue before). > One thought: have you considered storing a max-100-char prefix and suffix > instead of duplicating the middle? I don't want to put too much effort into > premature optimization, but it does seem like there could be realistic cases > where we might use a lot more memory than needed here. I am not sure. It will work just fine with Mac since it only cares about selected text. On Aura (and I guess android) it will be problematic again since methods such as RWHVAura::GetTextRange(range) will require getting a range from |text_| which will be a bit hard when we have it stored in 3 pieces (and I am not sure how often we call that method yet). All in all, I suppose not caching should not be too bad (yes 30 calls but that is only for right-clicking on Mac which should not cause major problems...I guess). I suggest moving ahead with not caching and I will followup on perhaps a better design around this issue (we could for instance cache only when length is less than a certain number of bytes). If you think caching might be better (given that it used to be the case on Mac before), we could proceed with patch 7. WDYT?
On 2017/02/24 18:34:40, EhsanK wrote: > Thanks Charlie! > > In short, I am not caching in the final patch. Please let me know if you'd > prefer the latest patch over patch 7? > > https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... > File content/browser/renderer_host/text_input_manager.h (right): > > https://codereview.chromium.org/2694543002/diff/80001/content/browser/rendere... > content/browser/renderer_host/text_input_manager.h:125: base::string16 text_; > On 2017/02/21 22:57:38, Charlie Reis wrote: > > On 2017/02/14 18:48:59, EhsanK wrote: > > > On 2017/02/14 00:46:43, Charlie Reis wrote: > > > > Is it ineficient to store these two strings separately? > > > Here is my analysis: > > > Cons (of caching): > > > 1- Extra memory for |selected_text_|. > > > a- If caret or selection is inside <input>, |text_| could be much larger > > than > > > |selected_text_| (200s chars as opposed to single digit number of chars). > In > > > this case there is practically no extra memory usage. > > > b- If selection is in a <div> or some noneditable content, |text_| seems > to > > be > > > equal to |selected_text_|. So the cost is doubling memory usage (which might > > not > > > be much considering the fact that most practical selections are several > words > > > long). > > > 2- Finding |selected_text_| for any selection update (regardless of whether > or > > > not this will be used later) > > > a- On Android we do this at least once. > > > b- On Mac we do this at least once. If we end up right clicking on any > > > selected text, we call selected_text() exactly 30 times! (on my machine and > I > > am > > > not sure why 30). > > > c- On linux as well we call this at least once. > > > > > > So in general I guess it is not a clear call on whether it is more efficient > > or > > > less. I think on Mac caching makes more sense since we usually select by > > > clicking on text which pops up context menu (And the 30 consecutive calls!). > > > > > > > I am also happy to remove this and make a method to calculate selected text > > > every time. WDYT? > > > > I'm mainly worried about the Ctrl+A on a large page case, where we're doubling > a > > possibly very large string. But maybe that's uncommon-- do you know if we > > happen to have stats? > > Yes it will be consuming much more (2x) in those cases. I don't know if we do > (not sure how I can track that down), but I guess Ctrl/Command+A could be quite > common (at least I do when copying text in between docs). > > > Given the 30 calls on Mac, though, it's probably worthwhile to have it cached. > > > > Yes it used to be cached inside RWHVMac (so we were having the 2x memory issue > before). Ah, I didn't realize this. If we were caching it and doubling the memory use in ctrl+A before, then we should leave that part alone for now and continue caching. If we want to optimize it later that's fine, but no sense in changing it now. Sorry about the hassle, and the caching version LGTM. > > > One thought: have you considered storing a max-100-char prefix and suffix > > instead of duplicating the middle? I don't want to put too much effort into > > premature optimization, but it does seem like there could be realistic cases > > where we might use a lot more memory than needed here. > I am not sure. It will work just fine with Mac since it only cares about > selected text. On Aura (and I guess android) it will be problematic again since > methods such as RWHVAura::GetTextRange(range) will require getting a range from > |text_| which will be a bit hard when we have it stored in 3 pieces (and I am > not sure how often we call that method yet). (Good point-- that could get complex.) > > All in all, I suppose not caching should not be too bad (yes 30 calls but that > is only for right-clicking on Mac which should not cause major problems...I > guess). > > I suggest moving ahead with not caching and I will followup on perhaps a better > design around this issue (we could for instance cache only when length is less > than a certain number of bytes). If you think caching might be better (given > that it used to be the case on Mac before), we could proceed with patch 7. WDYT?
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Thanks Charlie! I am going ahead with the cached version then.
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2694543002/#ps300001 (title: "Fixed another compile error for Android test")
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": 300001, "attempt_start_ts": 1488482604356310,
"parent_rev": "5936323a6285a2b50d39099d72cef7aa6a171888", "commit_rev":
"374b2669b0af7cd88882cff28555f5aaa04c52d7"}
Message was sent while issue was closed.
Description was changed from ========== [refactor] Cleanup IME State in RenderWidgetHostViewMac which is already tracked by TextInputManager This CL will cleanup some text selection and composition information related variables inside RenderWidgetHostViewMac. These values are already tracked by TextInputManager so keeping them inside RenderWidgetHostViewMac is costly. In line with this, some changes to TextInputManager::TextSelection led to small changes to both RenderWidgetHostViewAura and RenderWidgetHostViewAndroid. BUG=602427 ========== to ========== [refactor] Cleanup IME State in RenderWidgetHostViewMac which is already tracked by TextInputManager This CL will cleanup some text selection and composition information related variables inside RenderWidgetHostViewMac. These values are already tracked by TextInputManager so keeping them inside RenderWidgetHostViewMac is costly. In line with this, some changes to TextInputManager::TextSelection led to small changes to both RenderWidgetHostViewAura and RenderWidgetHostViewAndroid. BUG=602427 Review-Url: https://codereview.chromium.org/2694543002 Cr-Commit-Position: refs/heads/master@{#454367} Committed: https://chromium.googlesource.com/chromium/src/+/374b2669b0af7cd88882cff28555... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:300001) as https://chromium.googlesource.com/chromium/src/+/374b2669b0af7cd88882cff28555... |
