Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1948343002:
[reland] Browser Side Text Input State Tracking for OOPIF (Aura Only) (Closed)
|
Created:
4 years, 7 months ago by EhsanK Modified:
4 years, 6 months ago CC:
chromium-reviews, creis+watch_chromium.org, danakj+watch_chromium.org, darin-cc_chromium.org, jam, jbauman+watch_chromium.org, kalyank, nasko+codewatch_chromium.org, nona+watch_chromium.org, piman+watch_chromium.org, shuchen+watch_chromium.org, sievers+watch_chromium.org, site-isolation-reviews_chromium.org, James Su, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[reland] Browser Side Text Input State Tracking for OOPIF (Aura Only)
This CL relands the original issue https://codereview.chromium.org/1652483002/,
with modifications and for aura only (Linux, Windows, etc.). T
A TextInputManager is introduced which owned by the RenderWidgetHostDelegate
(mostly, WebContentsImpl). Each RWHV will observe the TextInputManager for its destruction
and lifetime while the TextInputManager observers all the RWHVs on the tab.
The role of the TextInputManager is to bookkeep and store TextInputState from all RWHV and
provide the correct TextInputState and the RWHV with active TextInputState at anytime. These
information are used by the tab-RWHV in handling IME related tasks.
This CL also adds several tests and some utility methods to help testing IME and TextInputState
for OOPIF.
BUG=578168, 602723
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223
Cr-Commit-Position: refs/heads/master@{#398559}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressing kenrb@ comments (Removing TextInputManager::Observer::OnTextInputManagerDestroyed) #
Total comments: 3
Patch Set 3 : Using the Destruction Observer for RWHVAura #
Total comments: 14
Patch Set 4 : Addressing dcheng@ Comments #Patch Set 5 : Using 'const ref' for arg of GetFrame() #Patch Set 6 : #Patch Set 7 : Directly initializing vector<size_t> before calling GetFrame #Patch Set 8 : Merged #
Total comments: 23
Patch Set 9 : Merged #
Total comments: 2
Patch Set 10 : Addressing shuchen@ and kenrb@ comments #
Total comments: 3
Patch Set 11 : Fixed a Compile Error #Patch Set 12 : Addressing kenrb@ Comments #
Total comments: 114
Patch Set 13 : Addressing creis@ Comments #Patch Set 14 : Added the missing test file #
Total comments: 2
Patch Set 15 : Changed Public Test API Methods to non-const #
Total comments: 93
Patch Set 16 : Addressing creis@ Comments #
Total comments: 5
Patch Set 17 : Rebase (major) #Patch Set 18 : Fixing Compile Errors #Patch Set 19 : Rebased #Patch Set 20 : Rebase #
Total comments: 28
Patch Set 21 : Addressing creis@ comments #Patch Set 22 : #
Total comments: 5
Patch Set 23 : Addressing sky@ comments #
Total comments: 18
Patch Set 24 : Addressing sky@ comments #Patch Set 25 : Rebase #Patch Set 26 : Rebase (Code changed during CQ Dry-run) #Messages
Total messages: 56 (14 generated)
Description was changed from ========== [reland] Browser Side Text Input State Tracking for OOPIF (Aura Only) This CL relands the original issue https://codereview.chromium.org/1652483002/, with modifications and for aura only (Linux, Windows, etc.). T A TextInputManager is introduced which owned by the RenderWidgetHostDelegate (mostly, WebContentsImpl). Each RWHV will observe the TextInputManager for its destruction and lifetime while the TextInputManager observers all the RWHVs on the tab. The role of the TextInputManager is to bookkeep and store TextInputState from all RWHV and provide the correct TextInputState and the RWHV with active TextInputState at anytime. These information are used by the tab-RWHV in handling IME related tasks. This CL also adds several tests and some utility methods to help testing IME and TextInputState for OOPIF. BUG=578168, 602723 ========== to ========== [reland] Browser Side Text Input State Tracking for OOPIF (Aura Only) This CL relands the original issue https://codereview.chromium.org/1652483002/, with modifications and for aura only (Linux, Windows, etc.). T A TextInputManager is introduced which owned by the RenderWidgetHostDelegate (mostly, WebContentsImpl). Each RWHV will observe the TextInputManager for its destruction and lifetime while the TextInputManager observers all the RWHVs on the tab. The role of the TextInputManager is to bookkeep and store TextInputState from all RWHV and provide the correct TextInputState and the RWHV with active TextInputState at anytime. These information are used by the tab-RWHV in handling IME related tasks. This CL also adds several tests and some utility methods to help testing IME and TextInputState for OOPIF. BUG=578168, 602723 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
ekaramad@chromium.org changed reviewers: + creis@chromium.org
Hello Charlie, Please take a look at this CL. Meanwhile, I am working on adding one more test and a branch of this to fix IME (for Aura for now). Thanks!
ekaramad@chromium.org changed reviewers: + kenrb@chromium.org
Adding kenrb@. Please, take a look.
I haven't looked closely at the tests but the TextInputManager class certainly makes things a lot cleaner. A few questions below. https://codereview.chromium.org/1948343002/diff/1/content/browser/frame_host/... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/1948343002/diff/1/content/browser/frame_host/... content/browser/frame_host/interstitial_page_impl.cc:935: TextInputManager* InterstitialPageImpl::GetTextInputManager() { Does this method get called? I don't quite understand what it means to have text input state on an interstitial page. https://codereview.chromium.org/1948343002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/1948343002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_base.h:392: // no reference is found, this method tries to obtain one from the RWHD. Expand RWHD to RenderWidgetHostDelegate. In this context it isn't obvious. https://codereview.chromium.org/1948343002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_base.h:442: TextInputManager* text_input_manager) override; Why is this public in the base class but private here? https://codereview.chromium.org/1948343002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/1948343002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/text_input_manager.h:36: TextInputManager* text_input_manager) {} To what extent is this necessary? Can RenderWidgetHostViews outlive the WebContents that owns the TextInputManager?
+site-isolation-reviews
Please, take another look. Thanks! https://codereview.chromium.org/1948343002/diff/1/content/browser/frame_host/... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/1948343002/diff/1/content/browser/frame_host/... content/browser/frame_host/interstitial_page_impl.cc:935: TextInputManager* InterstitialPageImpl::GetTextInputManager() { On 2016/05/05 20:26:28, kenrb wrote: > Does this method get called? I don't quite understand what it means to have text > input state on an interstitial page. Clicking on the page will cause a TextInputStateChanged call which will then call the RenderWidgetHostDelegate::GetTextInputManager(). In my earlier draft from months ago, I tested this (patch: https://codereview.chromium.org/1663013005/#ps160001) and a lot of tests actually failed because InterstitialPage where returning nullptr for TextInputState (bot: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_iso...). However, I am not sure if there is going to be any meaningful text input state, now or ever. I think if we can safely ignore those TextInputStateChanged updates, it is safe to return nullptr. We now do check for GetTextInputManager() != nullptr everywhere before asking for GetTextInputState(). https://codereview.chromium.org/1948343002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/1948343002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_base.h:392: // no reference is found, this method tries to obtain one from the RWHD. On 2016/05/05 20:26:28, kenrb wrote: > Expand RWHD to RenderWidgetHostDelegate. In this context it isn't obvious. Done. https://codereview.chromium.org/1948343002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_base.h:442: TextInputManager* text_input_manager) override; On 2016/05/05 20:26:28, kenrb wrote: > Why is this public in the base class but private here? Actually, no good reason. I noticed RenderWidgetHostViewAura implements DelegatedFrameHostClient in private and I figured maybe this is good to mask direct calls on these methods. I will move it back to public. https://codereview.chromium.org/1948343002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/1948343002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/text_input_manager.h:36: TextInputManager* text_input_manager) {} On 2016/05/05 20:26:28, kenrb wrote: > To what extent is this necessary? Can RenderWidgetHostViews outlive the > WebContents that owns the TextInputManager? As we discussed offline, the main reason behind it was the inner/outer WebContents case. It is not clear what TIM to pick during attach/detach process. But as that situation and <webview> is not addressed in this CL, I will remove this method for now (maybe add it back later after we resolve OOPIF <webview>). https://codereview.chromium.org/1948343002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1948343002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2387: GetTextInputManager()->RemoveObserver(this); We need to remove the observer here. If not, the RWHVCF destruction path (DeleteSoon) will cause an observer call to a dangling pointer to this view (closing the tab causes browser crash). This was perhaps the only complication in removing the TextInputManager::Observer::OnTextInputManagerDestroyed(). https://codereview.chromium.org/1948343002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/1948343002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.h:94: public TextInputManager::Observer, I guess there might be no need for RWHVBase to be the observer now. It should only be a tab-level RWHV thing. https://codereview.chromium.org/1948343002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/1948343002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.cc:404: manager->Register(const_cast<RenderWidgetHostViewBase*>(this)); I am not sure how OK this is, but the reason for this const_cast is that we need this method to be const. This is solely because of the getter methods in ui::TextInputClient which are defined const.
PTAL. kenrb@: Following our conversation offline offline, I have added a TODO to perhaps change the TextInputManager->RWHVAura communication from Observer style to Delegate style. I will add a bug no. for that when this CL is about to land.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
I didn't look over the entire CL, but some C++11 style comments. https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... chrome/browser/site_per_process_interactive_browsertest.cc:401: content::RenderFrameHost* GetFrame(std::initializer_list<size_t> list) { Just have this take a std::vector<size_t>. You can still use initializer lists: GetFrame({}); GetFrame({0, 1, 2}); etc https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... chrome/browser/site_per_process_interactive_browsertest.cc:404: for (size_t i = 0; i < indices.size(); ++i) for (int index : indices) https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... chrome/browser/site_per_process_interactive_browsertest.cc:421: std::vector<content::RenderFrameHost*> frames( As far as I can tell, Google style is: auto frames{GetFrame({}), GetFrame({0}), GetFrame({1}), ...}; The extra parens is generally used for an actual function call. https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... File content/public/test/text_input_test_utils.cc (right): https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... content/public/test/text_input_test_utils.cc:197: static_cast<TestTextInputManagerObserver*>( return base::WrapUnique(new TextInputManagerObserver(...)) Static cast should not be needed. https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... content/public/test/text_input_test_utils.cc:220: static_cast<RenderWidgetHostViewDestructionObserver*>( return base::WrapUnique(new RenderWidgetHostViewBaseObserverImpl(...)); The static cast to the base class should not be needed. https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... content/public/test/text_input_test_utils.cc:296: new InputMethodObserverAura(view->GetInputMethod()))); This static cast should be unnecessary. IF it is required, something weird is happening.
Thansk. Please, take another look. @dcheng: I believe the initializer_list was necessary as this patch failed compile on some bots. I think it was the reason I used initializer_list originally as apparently it was failing in my draft CL as well (https://codereview.chromium.org/1934423002/). https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... chrome/browser/site_per_process_interactive_browsertest.cc:401: content::RenderFrameHost* GetFrame(std::initializer_list<size_t> list) { On 2016/05/10 20:33:41, dcheng wrote: > Just have this take a std::vector<size_t>. You can still use initializer lists: > > GetFrame({}); > > GetFrame({0, 1, 2}); > > etc Done. https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... chrome/browser/site_per_process_interactive_browsertest.cc:404: for (size_t i = 0; i < indices.size(); ++i) On 2016/05/10 20:33:41, dcheng wrote: > for (int index : indices) Should I still use size_t or just plain int? https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... chrome/browser/site_per_process_interactive_browsertest.cc:421: std::vector<content::RenderFrameHost*> frames( On 2016/05/10 20:33:41, dcheng wrote: > As far as I can tell, Google style is: > auto frames{GetFrame({}), GetFrame({0}), GetFrame({1}), ...}; > > The extra parens is generally used for an actual function call. Done. https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... File content/public/test/text_input_test_utils.cc (right): https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... content/public/test/text_input_test_utils.cc:197: static_cast<TestTextInputManagerObserver*>( On 2016/05/10 20:33:41, dcheng wrote: > return base::WrapUnique(new TextInputManagerObserver(...)) > > Static cast should not be needed. Thanks! This is great! https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... content/public/test/text_input_test_utils.cc:220: static_cast<RenderWidgetHostViewDestructionObserver*>( On 2016/05/10 20:33:41, dcheng wrote: > return base::WrapUnique(new RenderWidgetHostViewBaseObserverImpl(...)); > > The static cast to the base class should not be needed. Done. https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... content/public/test/text_input_test_utils.cc:296: new InputMethodObserverAura(view->GetInputMethod()))); On 2016/05/10 20:33:41, dcheng wrote: > This static cast should be unnecessary. IF it is required, something weird is > happening. I think I was getting some compile error no a bot. But I might be mistaken. It seems fine now. Thanks!
On 2016/05/11 at 01:54:03, ekaramad wrote: > Thansk. Please, take another look. > > @dcheng: I believe the initializer_list was necessary as this patch failed compile on some bots. I think it was the reason I used initializer_list originally as apparently it was failing in my draft CL as well (https://codereview.chromium.org/1934423002/). Does it still fail if you pass it by const ref instead of by copy? > > https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... > File chrome/browser/site_per_process_interactive_browsertest.cc (right): > > https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... > chrome/browser/site_per_process_interactive_browsertest.cc:401: content::RenderFrameHost* GetFrame(std::initializer_list<size_t> list) { > On 2016/05/10 20:33:41, dcheng wrote: > > Just have this take a std::vector<size_t>. You can still use initializer lists: > > > > GetFrame({}); > > > > GetFrame({0, 1, 2}); > > > > etc > > Done. > > https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... > chrome/browser/site_per_process_interactive_browsertest.cc:404: for (size_t i = 0; i < indices.size(); ++i) > On 2016/05/10 20:33:41, dcheng wrote: > > for (int index : indices) > > Should I still use size_t or just plain int? > > https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... > chrome/browser/site_per_process_interactive_browsertest.cc:421: std::vector<content::RenderFrameHost*> frames( > On 2016/05/10 20:33:41, dcheng wrote: > > As far as I can tell, Google style is: > > auto frames{GetFrame({}), GetFrame({0}), GetFrame({1}), ...}; > > > > The extra parens is generally used for an actual function call. > > Done. > > https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... > File content/public/test/text_input_test_utils.cc (right): > > https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... > content/public/test/text_input_test_utils.cc:197: static_cast<TestTextInputManagerObserver*>( > On 2016/05/10 20:33:41, dcheng wrote: > > return base::WrapUnique(new TextInputManagerObserver(...)) > > > > Static cast should not be needed. > > Thanks! This is great! > > https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... > content/public/test/text_input_test_utils.cc:220: static_cast<RenderWidgetHostViewDestructionObserver*>( > On 2016/05/10 20:33:41, dcheng wrote: > > return base::WrapUnique(new RenderWidgetHostViewBaseObserverImpl(...)); > > > > The static cast to the base class should not be needed. > > Done. > > https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... > content/public/test/text_input_test_utils.cc:296: new InputMethodObserverAura(view->GetInputMethod()))); > On 2016/05/10 20:33:41, dcheng wrote: > > This static cast should be unnecessary. IF it is required, something weird is > > happening. > > I think I was getting some compile error no a bot. But I might be mistaken. It seems fine now. Thanks!
On 2016/05/11 02:26:31, dcheng wrote: > On 2016/05/11 at 01:54:03, ekaramad wrote: > > Thansk. Please, take another look. > > > > @dcheng: I believe the initializer_list was necessary as this patch failed > compile on some bots. I think it was the reason I used initializer_list > originally as apparently it was failing in my draft CL as well > (https://codereview.chromium.org/1934423002/). > > Does it still fail if you pass it by const ref instead of by copy? > > > > > > https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... > > File chrome/browser/site_per_process_interactive_browsertest.cc (right): > > > > > https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... > > chrome/browser/site_per_process_interactive_browsertest.cc:401: > content::RenderFrameHost* GetFrame(std::initializer_list<size_t> list) { > > On 2016/05/10 20:33:41, dcheng wrote: > > > Just have this take a std::vector<size_t>. You can still use initializer > lists: > > > > > > GetFrame({}); > > > > > > GetFrame({0, 1, 2}); > > > > > > etc > > > > Done. > > > > > https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... > > chrome/browser/site_per_process_interactive_browsertest.cc:404: for (size_t i > = 0; i < indices.size(); ++i) > > On 2016/05/10 20:33:41, dcheng wrote: > > > for (int index : indices) > > > > Should I still use size_t or just plain int? > > > > > https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... > > chrome/browser/site_per_process_interactive_browsertest.cc:421: > std::vector<content::RenderFrameHost*> frames( > > On 2016/05/10 20:33:41, dcheng wrote: > > > As far as I can tell, Google style is: > > > auto frames{GetFrame({}), GetFrame({0}), GetFrame({1}), ...}; > > > > > > The extra parens is generally used for an actual function call. > > > > Done. > > > > > https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... > > File content/public/test/text_input_test_utils.cc (right): > > > > > https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... > > content/public/test/text_input_test_utils.cc:197: > static_cast<TestTextInputManagerObserver*>( > > On 2016/05/10 20:33:41, dcheng wrote: > > > return base::WrapUnique(new TextInputManagerObserver(...)) > > > > > > Static cast should not be needed. > > > > Thanks! This is great! > > > > > https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... > > content/public/test/text_input_test_utils.cc:220: > static_cast<RenderWidgetHostViewDestructionObserver*>( > > On 2016/05/10 20:33:41, dcheng wrote: > > > return base::WrapUnique(new RenderWidgetHostViewBaseObserverImpl(...)); > > > > > > The static cast to the base class should not be needed. > > > > Done. > > > > > https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... > > content/public/test/text_input_test_utils.cc:296: new > InputMethodObserverAura(view->GetInputMethod()))); > > On 2016/05/10 20:33:41, dcheng wrote: > > > This static cast should be unnecessary. IF it is required, something weird > is > > > happening. > > > > I think I was getting some compile error no a bot. But I might be mistaken. It > seems fine now. Thanks! In my draft CL at patch 12 I decided to use initializer_list due to compile errors. My guess is that I was actually using const vector&. I found this thread which says it might be a clang bug prior to 3.3 (http://permalink.gmane.org/gmane.comp.compilers.clang.devel/27173). I will do this again with const ref. Either works or we will have it on the records as a definitive reference. Thanks.
On 2016/05/11 14:05:50, ehsaaan wrote: > On 2016/05/11 02:26:31, dcheng wrote: > > On 2016/05/11 at 01:54:03, ekaramad wrote: > > > Thansk. Please, take another look. > > > > > > @dcheng: I believe the initializer_list was necessary as this patch failed > > compile on some bots. I think it was the reason I used initializer_list > > originally as apparently it was failing in my draft CL as well > > (https://codereview.chromium.org/1934423002/). > > > > Does it still fail if you pass it by const ref instead of by copy? > > > > > > > > > > > https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... > > > File chrome/browser/site_per_process_interactive_browsertest.cc (right): > > > > > > > > > https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... > > > chrome/browser/site_per_process_interactive_browsertest.cc:401: > > content::RenderFrameHost* GetFrame(std::initializer_list<size_t> list) { > > > On 2016/05/10 20:33:41, dcheng wrote: > > > > Just have this take a std::vector<size_t>. You can still use initializer > > lists: > > > > > > > > GetFrame({}); > > > > > > > > GetFrame({0, 1, 2}); > > > > > > > > etc > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... > > > chrome/browser/site_per_process_interactive_browsertest.cc:404: for (size_t > i > > = 0; i < indices.size(); ++i) > > > On 2016/05/10 20:33:41, dcheng wrote: > > > > for (int index : indices) > > > > > > Should I still use size_t or just plain int? > > > > > > > > > https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... > > > chrome/browser/site_per_process_interactive_browsertest.cc:421: > > std::vector<content::RenderFrameHost*> frames( > > > On 2016/05/10 20:33:41, dcheng wrote: > > > > As far as I can tell, Google style is: > > > > auto frames{GetFrame({}), GetFrame({0}), GetFrame({1}), ...}; > > > > > > > > The extra parens is generally used for an actual function call. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... > > > File content/public/test/text_input_test_utils.cc (right): > > > > > > > > > https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... > > > content/public/test/text_input_test_utils.cc:197: > > static_cast<TestTextInputManagerObserver*>( > > > On 2016/05/10 20:33:41, dcheng wrote: > > > > return base::WrapUnique(new TextInputManagerObserver(...)) > > > > > > > > Static cast should not be needed. > > > > > > Thanks! This is great! > > > > > > > > > https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... > > > content/public/test/text_input_test_utils.cc:220: > > static_cast<RenderWidgetHostViewDestructionObserver*>( > > > On 2016/05/10 20:33:41, dcheng wrote: > > > > return base::WrapUnique(new RenderWidgetHostViewBaseObserverImpl(...)); > > > > > > > > The static cast to the base class should not be needed. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... > > > content/public/test/text_input_test_utils.cc:296: new > > InputMethodObserverAura(view->GetInputMethod()))); > > > On 2016/05/10 20:33:41, dcheng wrote: > > > > This static cast should be unnecessary. IF it is required, something weird > > is > > > > happening. > > > > > > I think I was getting some compile error no a bot. But I might be mistaken. > It > > seems fine now. Thanks! > > In my draft CL at patch 12 I decided to use initializer_list due to compile > errors. My guess is that I was actually using const vector&. > > I found this thread which says it might be a clang bug prior to 3.3 > (http://permalink.gmane.org/gmane.comp.compilers.clang.devel/27173). > > I will do this again with const ref. Either works or we will have it on the > records as a definitive reference. Thanks. @dcheng: Should I revert back to having an initializer_list argument?
On 2016/05/11 at 14:58:41, ekaramad wrote: > On 2016/05/11 14:05:50, ehsaaan wrote: > > On 2016/05/11 02:26:31, dcheng wrote: > > > On 2016/05/11 at 01:54:03, ekaramad wrote: > > > > Thansk. Please, take another look. > > > > > > > > @dcheng: I believe the initializer_list was necessary as this patch failed > > > compile on some bots. I think it was the reason I used initializer_list > > > originally as apparently it was failing in my draft CL as well > > > (https://codereview.chromium.org/1934423002/). > > > > > > Does it still fail if you pass it by const ref instead of by copy? > > > > > > > > > > > > > > > > https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... > > > > File chrome/browser/site_per_process_interactive_browsertest.cc (right): > > > > > > > > > > > > > https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... > > > > chrome/browser/site_per_process_interactive_browsertest.cc:401: > > > content::RenderFrameHost* GetFrame(std::initializer_list<size_t> list) { > > > > On 2016/05/10 20:33:41, dcheng wrote: > > > > > Just have this take a std::vector<size_t>. You can still use initializer > > > lists: > > > > > > > > > > GetFrame({}); > > > > > > > > > > GetFrame({0, 1, 2}); > > > > > > > > > > etc > > > > > > > > Done. > > > > > > > > > > > > > https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... > > > > chrome/browser/site_per_process_interactive_browsertest.cc:404: for (size_t > > i > > > = 0; i < indices.size(); ++i) > > > > On 2016/05/10 20:33:41, dcheng wrote: > > > > > for (int index : indices) > > > > > > > > Should I still use size_t or just plain int? > > > > > > > > > > > > > https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... > > > > chrome/browser/site_per_process_interactive_browsertest.cc:421: > > > std::vector<content::RenderFrameHost*> frames( > > > > On 2016/05/10 20:33:41, dcheng wrote: > > > > > As far as I can tell, Google style is: > > > > > auto frames{GetFrame({}), GetFrame({0}), GetFrame({1}), ...}; > > > > > > > > > > The extra parens is generally used for an actual function call. > > > > > > > > Done. > > > > > > > > > > > > > https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... > > > > File content/public/test/text_input_test_utils.cc (right): > > > > > > > > > > > > > https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... > > > > content/public/test/text_input_test_utils.cc:197: > > > static_cast<TestTextInputManagerObserver*>( > > > > On 2016/05/10 20:33:41, dcheng wrote: > > > > > return base::WrapUnique(new TextInputManagerObserver(...)) > > > > > > > > > > Static cast should not be needed. > > > > > > > > Thanks! This is great! > > > > > > > > > > > > > https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... > > > > content/public/test/text_input_test_utils.cc:220: > > > static_cast<RenderWidgetHostViewDestructionObserver*>( > > > > On 2016/05/10 20:33:41, dcheng wrote: > > > > > return base::WrapUnique(new RenderWidgetHostViewBaseObserverImpl(...)); > > > > > > > > > > The static cast to the base class should not be needed. > > > > > > > > Done. > > > > > > > > > > > > > https://codereview.chromium.org/1948343002/diff/40001/content/public/test/tex... > > > > content/public/test/text_input_test_utils.cc:296: new > > > InputMethodObserverAura(view->GetInputMethod()))); > > > > On 2016/05/10 20:33:41, dcheng wrote: > > > > > This static cast should be unnecessary. IF it is required, something weird > > > is > > > > > happening. > > > > > > > > I think I was getting some compile error no a bot. But I might be mistaken. > > It > > > seems fine now. Thanks! > > > > In my draft CL at patch 12 I decided to use initializer_list due to compile > > errors. My guess is that I was actually using const vector&. > > > > I found this thread which says it might be a clang bug prior to 3.3 > > (http://permalink.gmane.org/gmane.comp.compilers.clang.devel/27173). > > > > I will do this again with const ref. Either works or we will have it on the > > records as a definitive reference. Thanks. > > @dcheng: Should I revert back to having an initializer_list argument? Not sure. Passing around an initializer list as an argument is kind of weird thing to write outside a constructor. So I would prefer to avoid writing it in this context at all if possible.
https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... chrome/browser/site_per_process_interactive_browsertest.cc:404: for (size_t i = 0; i < indices.size(); ++i) On 2016/05/11 at 01:54:03, ehsaaan wrote: > On 2016/05/10 20:33:41, dcheng wrote: > > for (int index : indices) > > Should I still use size_t or just plain int? Ah, sorry, use the appropriate type.
PTAL. https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/40001/chrome/browser/site_per... chrome/browser/site_per_process_interactive_browsertest.cc:404: for (size_t i = 0; i < indices.size(); ++i) On 2016/05/11 17:12:33, dcheng wrote: > On 2016/05/11 at 01:54:03, ehsaaan wrote: > > On 2016/05/10 20:33:41, dcheng wrote: > > > for (int index : indices) > > > > Should I still use size_t or just plain int? > > Ah, sorry, use the appropriate type. Acknowledged.
ekaramad@chromium.org changed reviewers: + shuchen@chromium.org
Adding shuchen@ for IME reviews. shuchen@: Please, take a look at the patch.
Most of my comments are questions and nits, but I have one significant concern remaining regarding how this works with webview and nested WebContents. The potential state of having a live page under a WebContents that can be temporarily disconnected from a parent WebContents makes some code difficult to reason about, so it is hard to be sure the right things are happening here. https://codereview.chromium.org/1948343002/diff/1/content/browser/frame_host/... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/1948343002/diff/1/content/browser/frame_host/... content/browser/frame_host/interstitial_page_impl.cc:935: TextInputManager* InterstitialPageImpl::GetTextInputManager() { On 2016/05/06 18:23:43, ehsaaan wrote: > On 2016/05/05 20:26:28, kenrb wrote: > > Does this method get called? I don't quite understand what it means to have > text > > input state on an interstitial page. > > Clicking on the page will cause a TextInputStateChanged call which will then > call the RenderWidgetHostDelegate::GetTextInputManager(). In my earlier draft > from months ago, I tested this (patch: > https://codereview.chromium.org/1663013005/#ps160001) and a lot of tests > actually failed because InterstitialPage where returning nullptr for > TextInputState (bot: > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_iso...). > > However, I am not sure if there is going to be any meaningful text input state, > now or ever. I think if we can safely ignore those TextInputStateChanged > updates, it is safe to return nullptr. We now do check for GetTextInputManager() > != nullptr everywhere before asking for GetTextInputState(). Well, it isn't a good thing to create an object that will never be used for anything, so returning nullptr here sounds likely to be right. If it needs a lot of test reworking to make that happen then a follow up patch would be appropriate. https://codereview.chromium.org/1948343002/diff/140001/chrome/browser/site_pe... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/140001/chrome/browser/site_pe... chrome/browser/site_per_process_interactive_browsertest.cc:300: // This class observers the TextInputManager for updated |TextInputState.value|. nit: s/observers/observes https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:522: // TextInputManager::Observer implementations. nit: 'implementation' singular not plural https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:528: // as in some cases, RWHV goes away after WebContentsImpl's destruction. I don't think you need this comment. The comment on the base class method declaration should be enough, since the override here doesn't have any special significance. Also you can remove the blank line above this comment. https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:434: // with. Explicitly state that this can be nullptr, with a brief explanation of when that will be. The comment on GetTextInputManager() mentions it can return nullptr but derived RWHV classes use the pointer directly so it should be here also. https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... content/browser/renderer_host/text_input_manager.h:31: // Called when the text input state for |view| has been updated. If the nit: "If the the update" https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... content/browser/renderer_host/text_input_manager.h:33: // is defined differently for each platform), then |changed| will be true. This could use some clarification. First, is it necessary to notify observers if the state hasn't changed? 'has been updated' kind of implies something has changed, but it could be clearer here that an update has been received but possibly it hasn't changed the input state. https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... content/browser/renderer_host/text_input_manager.h:73: // before nit: Unneeded line break. https://codereview.chromium.org/1948343002/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1948343002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2374: // TODO(ekaramad): Is this the right way to for IME handoff? I have concerns about this, there were problems with the similar pattern for accessing RenderWidgetHostInputEventRouter. If there is a RenderWidgetHostView registered on the inner WebContents, how does it re-register with the outer WebContents? This gets especially hard to reason about in the case of RenderWidgetHostViewGuest which proxies calls to a platform RenderWidgetHostView that it owns. At the very least you should ensure your changes are tested with WebView in both OOPIF and non-OOPIF modes. https://codereview.chromium.org/1948343002/diff/140001/content/public/test/te... File content/public/test/text_input_test_utils.cc (right): https://codereview.chromium.org/1948343002/diff/140001/content/public/test/te... content/public/test/text_input_test_utils.cc:30: public TextInputManager::Observer { Why are you using multiple inheritance, rather than having TextInputManagerObserver inherit TextInputManager::Observer directly? (Likewise for the other observers below) https://codereview.chromium.org/1948343002/diff/140001/content/public/test/te... content/public/test/text_input_test_utils.cc:120: if (message_loop_runner_ && message_loop_runner_->loop_running()) I don't think the loop_running() check does anything. It should always be true if message_loop_runner_ exists.
lgtm for the overall text input state updating logic. https://codereview.chromium.org/1948343002/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1948343002/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2879: void RenderWidgetHostViewAura::OnTextInputManagerDestroyed( nit: maybe clearer to rename this as OnDestroyingTextInputManager().
Please, take another look. https://codereview.chromium.org/1948343002/diff/1/content/browser/frame_host/... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/1948343002/diff/1/content/browser/frame_host/... content/browser/frame_host/interstitial_page_impl.cc:935: TextInputManager* InterstitialPageImpl::GetTextInputManager() { On 2016/05/12 16:16:36, kenrb wrote: > On 2016/05/06 18:23:43, ehsaaan wrote: > > On 2016/05/05 20:26:28, kenrb wrote: > > > Does this method get called? I don't quite understand what it means to have > > text > > > input state on an interstitial page. > > > > Clicking on the page will cause a TextInputStateChanged call which will then > > call the RenderWidgetHostDelegate::GetTextInputManager(). In my earlier draft > > from months ago, I tested this (patch: > > https://codereview.chromium.org/1663013005/#ps160001) and a lot of tests > > actually failed because InterstitialPage where returning nullptr for > > TextInputState (bot: > > > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_iso...). > > > > However, I am not sure if there is going to be any meaningful text input > state, > > now or ever. I think if we can safely ignore those TextInputStateChanged > > updates, it is safe to return nullptr. We now do check for > GetTextInputManager() > > != nullptr everywhere before asking for GetTextInputState(). > > Well, it isn't a good thing to create an object that will never be used for > anything, so returning nullptr here sounds likely to be right. If it needs a lot > of test reworking to make that happen then a follow up patch would be > appropriate. Makes sense. I don't think it does need a lot of work. If it returns nullptr, then TextInputState does not get updated. But we should not have text input inside those pages. https://codereview.chromium.org/1948343002/diff/140001/chrome/browser/site_pe... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/140001/chrome/browser/site_pe... chrome/browser/site_per_process_interactive_browsertest.cc:300: // This class observers the TextInputManager for updated |TextInputState.value|. On 2016/05/12 16:16:36, kenrb wrote: > nit: s/observers/observes Done. https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:522: // TextInputManager::Observer implementations. On 2016/05/12 16:16:36, kenrb wrote: > nit: 'implementation' singular not plural Done. https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:528: // as in some cases, RWHV goes away after WebContentsImpl's destruction. On 2016/05/12 16:16:36, kenrb wrote: > I don't think you need this comment. The comment on the base class method > declaration should be enough, since the override here doesn't have any special > significance. Also you can remove the blank line above this comment. Done. https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:434: // with. On 2016/05/12 16:16:36, kenrb wrote: > Explicitly state that this can be nullptr, with a brief explanation of when that > will be. The comment on GetTextInputManager() mentions it can return nullptr but > derived RWHV classes use the pointer directly so it should be here also. Acknowledged. https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... content/browser/renderer_host/text_input_manager.h:31: // Called when the text input state for |view| has been updated. If the On 2016/05/12 16:16:37, kenrb wrote: > nit: "If the the update" Changed the comment altogether since the method signature changed. https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... content/browser/renderer_host/text_input_manager.h:33: // is defined differently for each platform), then |changed| will be true. The issue, which as of now only exists in Aura, is that we have a concept of change in TextInputState, which will lead to a call to InputMethod->OnTextInputTypeChanged(), as well as a need to call for IME, (InputMethod->ShowImeIfNeeded) when TextInpuState.show_ime_if_needed is true and TextInputState.type is not none. To differentiate the two cases, I used this boolean. To make it clear as it is confusing as it is, I broke this method into two smaller methods. One is only called when there is a change, and the other one is called all the time (for Aura). https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... content/browser/renderer_host/text_input_manager.h:73: // before On 2016/05/12 16:16:37, kenrb wrote: > nit: Unneeded line break. I think git cl format did this somehow. Done! https://codereview.chromium.org/1948343002/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1948343002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2374: // TODO(ekaramad): Is this the right way to for IME handoff? On 2016/05/12 16:16:37, kenrb wrote: > I have concerns about this, there were problems with the similar pattern for > accessing RenderWidgetHostInputEventRouter. If there is a RenderWidgetHostView > registered on the inner WebContents, how does it re-register with the outer > WebContents? This gets especially hard to reason about in the case of > RenderWidgetHostViewGuest which proxies calls to a platform RenderWidgetHostView > that it owns. At the very least you should ensure your changes are tested with > WebView in both OOPIF and non-OOPIF modes. Indeed, the case with <webview> is very tricky. As for this patch, non-OOPIF-<webview> is not affected as I am keeping the old TextInputState mechanism for RenderWidgetHostViewGuest. RWHVGuest normally reports its TextInputState to embedder (outer) tab-RWHV. So, from a tab-RWHV's perspective, the TextInputState has come from its own RenderWidget. This is racy but has been like this for a while now (crbug.com/546645). For OOPIF-<webview> this will not work as is. I am intending to fix that after landing this and IME for OOPIF. The solution I had in mind was to make all RWHV observer TextInputManager's destruction. This way, when attaching, all RWHV in inner WebContents will lose their reference to TextInputManager and then try to reinitialize and end up getting the one from outer WebContents. The complication is that we have to restore the current state after attachment. Although this only happens if there has been a TextInputState before attaching to start with. All in all, I think we should look at <webview> after this. https://codereview.chromium.org/1948343002/diff/140001/content/public/test/te... File content/public/test/text_input_test_utils.cc (right): https://codereview.chromium.org/1948343002/diff/140001/content/public/test/te... content/public/test/text_input_test_utils.cc:30: public TextInputManager::Observer { On 2016/05/12 16:16:37, kenrb wrote: > Why are you using multiple inheritance, rather than having > TextInputManagerObserver inherit TextInputManager::Observer directly? (Likewise > for the other observers below) I really wanted to avoid this. But I think as I understand, we are not allowed to expose anything from content except for content/public. So I cannot have a direct inheritence from TextInputManager::Observer since it lives in content/browser/renderer_host (Same with TextInputState in content/common). https://codereview.chromium.org/1948343002/diff/140001/content/public/test/te... content/public/test/text_input_test_utils.cc:120: if (message_loop_runner_ && message_loop_runner_->loop_running()) On 2016/05/12 16:16:37, kenrb wrote: > I don't think the loop_running() check does anything. It should always be true > if message_loop_runner_ exists. I was trying to avoid calling Quit twice. But I guess Quit() already checks for loop_running. Done! https://codereview.chromium.org/1948343002/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1948343002/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2879: void RenderWidgetHostViewAura::OnTextInputManagerDestroyed( On 2016/05/13 07:20:41, Shu Chen wrote: > nit: maybe clearer to rename this as OnDestroyingTextInputManager(). Acknowledged. https://codereview.chromium.org/1948343002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/1948343002/diff/180001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:57: text_input_manager_ = GetTextInputManager(); Actually, I am not sure if this is really necessary. https://codereview.chromium.org/1948343002/diff/180001/content/browser/render... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/1948343002/diff/180001/content/browser/render... content/browser/renderer_host/text_input_manager.h:42: virtual void OnTextInputStateUpdateCalled( shuchen@, kenrb@: To differentiate between a 'change' and a case where we only call 'ShowImeIfNeeded' on InputMethod, I separated the observer calls. As of now, this is only required by RenderWidgetHostViewAura. I am not sure if I should enclose it with #ifdef? https://codereview.chromium.org/1948343002/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1948343002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1424: text_input_manager_.reset(nullptr); kenrb@: This will notify all the RWHV (if any) and they will all reset their pointer to null. Upon next TextInputStateChanged IPC, they will acquire the new reference to TextInputManager from outer WebContents.
lgtm (with one remaining nit). It looks like there is still some work to do in some areas but the current state is okay for now. I think you just need a content owner's review at this point. https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:434: // with. On 2016/05/13 16:00:56, ehsaaan wrote: > On 2016/05/12 16:16:36, kenrb wrote: > > Explicitly state that this can be nullptr, with a brief explanation of when > that > > will be. The comment on GetTextInputManager() mentions it can return nullptr > but > > derived RWHV classes use the pointer directly so it should be here also. > > Acknowledged. Can you please adjust the comment to that affect? https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... content/browser/renderer_host/text_input_manager.h:73: // before On 2016/05/13 16:00:56, ehsaaan wrote: > On 2016/05/12 16:16:37, kenrb wrote: > > nit: Unneeded line break. > > I think git cl format did this somehow. Done! Yes it wraps comments wrong.
On 2016/05/13 20:37:42, kenrb wrote: > I think you just need a content owner's review at this point. > Sorry it's taking me a while. I'm still working my way through it, but I should be able to finish tomorrow.
Thanks for moving forward in a smaller chunk (Aura-only), and for including tests! Sorry for the long list of comments-- some are asking for clarification, some are suggestions to simplify, and many are just simple nits. (The CL is still a bit large, but I don't see an easy way to split it further.) For the tests, are they in an interactive test to avoid flakiness? I know that was important for the focus tests, so I can see that it might be worthwhile. (I also see that it requires a bunch of public test utility functions/classes, so I wanted to verify that it's needed.) https://codereview.chromium.org/1948343002/diff/220001/chrome/browser/site_pe... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/220001/chrome/browser/site_pe... chrome/browser/site_per_process_interactive_browsertest.cc:248: // TextInputManager and IME Tests Given the number of helper classes (and the expectation that this might grow), I think this belongs more in a test file of its own. For that matter, should it be a SitePerProcess-specific test class, or do these tests provide any meaningful coverage for default modes as well? Our FYI bots will run them with --site-per-process and the equivalent of --isolate-extensions, so if they pass the same way in default Chrome, maybe we can make them normal tests for IME. https://codereview.chromium.org/1948343002/diff/220001/chrome/browser/site_pe... chrome/browser/site_per_process_interactive_browsertest.cc:438: // selected for each frame. The test then creates a sequence of tab presses and What do you mean by distinctly selected? https://codereview.chromium.org/1948343002/diff/220001/chrome/browser/site_pe... chrome/browser/site_per_process_interactive_browsertest.cc:439: // verifies that after eahc key press, the right TextInputState is observed. nit: each https://codereview.chromium.org/1948343002/diff/220001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl.cc:945: return RenderWidgetHostDelegate::GetTextInputManager(); I think it's clearer to just return nullptr in this case. (I'm not sure if we would ever put a default implementation in the delegate class.) https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:1406: if (text_input_manager_ && text_input_manager_->GetTextInputState()) I'm curious-- how hard would it be to eliminate the case that GetTextInputState() returns null? It makes this a bit awkward and introduces a failure state we didn't have before. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2862: DCHECK(text_input_manager_ == text_input_manager); nit: DCHECK_EQ (here and below) https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:407: text_input_manager_->AddObserver(this); Would it make sense to call TextInputManager::AddObserver from within Register, or are there cases when we want to register without observing? https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:437: text_input_manager_->RemoveObserver(this); Seems a bit unusual to have the observer reach back into the TextInputManager to clear this state, rather than having the TextInputManager handle it directly in its destructor. Maybe it would be better to just do clear text_input_manager_ here (with the DCHECK above) and clear the rest in ~TextInputManager? https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:378: // sure to remove the observer before they go away. Maybe you can add a similar comment to the add/remove observer methods in TextInputManager? https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:388: TextInputManager* text_input_manager) override; nit: This belongs near the top, under "IPC::Listener implementation." https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:397: // no reference is found, this method tries to obtain one from the nit: "no no" https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:399: // can be obtained. Unless not possible, this method should be used internally I don't follow the "Unless not possible" part. Is there a case when it can't be used? (You can also rephrase by ending the sentence with "when possible," which would flow a little better, though I'm still curious about when it wouldn't be.) https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.cc:20: // TODO(ekaramad): Implement this logic for other platforms. Maybe add a NOTREACHED()? We don't want to forget about this when adding more platforms. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.cc:47: bool changed = AreDifferentTextInputStates(text_input_state_map_[view], What if text_input_state_map_[view] were null? AreDifferentTextInputStates would crash. Seems like many of the other uses of the map include null checks, so I'm not sure if there's a reason this is safe. (If so, please document.) https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.cc:103: if (active_view_ == view) { Didn't we just do this block in Unregister on line 98? I don't see how we could hit it again. RemoveObserver doesn't run observer code, does it? https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:5: #ifndef CONTENT_BROWSER_TEXT_INPUT_MANAGER_H__ nit: Missing RENDERER_HOST https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:18: class TestTextInputManagerObserver; This shouldn't be needed. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:20: // A class which receives updates of Text Input State from multiple sources and nit: Remove extra space. Also, let's be consistent about whether to capitalize Text Input State (this line and next one). Can you give some extra context about why this is needed (e.g., that the multiple sources may be out-of-process iframes, and this class is deciding what to use for a WebContents?). https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:24: : public RenderWidgetHostViewBaseObserver { It's a bit unfortunate that these two classes need to observe each other, since it creates a circular dependency. Can we have RWHVB call a method on TextInputManager when it's destroyed so that we don't need TextInputManager to observe it? That will lead to less overhead on the other observer methods in RWHVB and break the circular dependency. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:26: // The top-level tab RWHV should be an observer of TextInputManager to get nit: The tab's top-level RWHV https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:35: virtual void OnTextInputStateUpdated(TextInputManager* text_input_manager, I'm curious why the |text_input_manager| parameter is needed. Do the observers listen to multiple managers? Looks like we just DCHECK it today. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:39: // regardless the call having lead to change in TextInputState. In some nit: regardless of whether the call led to https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:42: virtual void OnTextInputStateUpdateCalled( Why do we need both this and OnTextInputStateUpdated? Feels like it might be simpler (and more efficient) to just have this one with a bool did_update_state. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:45: // Notifies all the observers that |text_input_manager| is being destroyed. Let's be consistent with how the other observer methods are phrased: // Called when the |text_input_manager| is being destroyed. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:54: explicit TextInputManager(); No need for explicit unless you have a single parameter. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:57: // Returns the current text input state corresponding to |active_source_|. nit: TextInputState (here and below) nit: s/|active_source_|/the currently active view/ https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:65: RenderWidgetHostViewBase* GetActiveView() const; Can this ever be null? Looks like active_view_ is initialized to null, so we may want to mention when it becomes valid, etc. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:77: // before destruction of RWHV or TextInputManager. "or TextInputManager" -> Should we add a DCHECK that all views have been unregistered in ~TextInputManager? Also, there is a risk of UaF if we get this wrong. Should active_view_ be a weak pointer, or are we confident that it will be cleared any time the view goes away? https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:81: bool IsRegisteredView(RenderWidgetHostViewBase* view) const; nit: IsRegistered (to be consistent with Register and Unregister). https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:89: // RenderWidgetHostViewObserver implementation. nit: Missing "Base" from name. https://codereview.chromium.org/1948343002/diff/220001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1420: // the RWHV in this WebContents. What is this accomplishing for the inner WebContents? I'm not sure I follow this. https://codereview.chromium.org/1948343002/diff/220001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1421: // TODO(ekaramad): Is this the right way to for IME handoff? Is it possible to Not sure I understand this TODO. (Right way to what?) https://codereview.chromium.org/1948343002/diff/220001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1380: std::unique_ptr<TextInputManager> text_input_manager_; Please add a comment here, mentioning something about how this tracks IME-related state for all widgets in this tab. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... File content/public/test/text_input_test_utils.cc (right): https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:29: class TextInputManagerObserver : public TestTextInputManagerObserver, Why is this a subclass of TestTextInputManagerObserver? I don't see any other implementations, and unlike InputMethodObserverAura, this doesn't seem to be platform-specific. I think it would be much simpler and easier to follow if we just had one implementation of TestTextInputManagerObserver. That also means most of these methods don't need to be virtual. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:32: // TestTextInputManagerObserver implementations. nit: Move this comment down to SetUpdateCallback, since it doesn't really apply to the constructor/destructor. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:45: void SetUpdateCallback(const content::TestTextInputManagerObserver::Callback& nit: We're already in the content namespace, so you shouldn't need content:: here or below. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:48: new content::TestTextInputManagerObserver::Callback(callback)); You shouldn't need to wrap/copy this. Just pass callback. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:58: bool GetTextInputValue(std::string& value) const override { We don't use non-const refs. This should be const std::string&, or std::string* if you're treating it as an out parameter (in which case it must be documented in the interface). https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:115: class RenderWidgetHostViewBaseObserverImpl I'm unclear about why this one is a subclass, too. Can we just move all this into RenderWidgetHostViewDestructionObserver directly? https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:134: DCHECK(view_ == view); nit: DCHECK_EQ https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:168: void SetOnTextInputTypeChangedCallback( nit: Let's be consistent on whether there's a blank line between these methods or not. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:170: on_text_input_type_changed_callback_.reset(new base::Closure(callback)); Again, no need to make a copy of callback. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:223: result[pair.first] = pair.second.type; Is there a reason we need to make a copy of the whole map to return, rather than just returning a reference to it? It's test code, so I'm not concerned about the caller mutating the map. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:265: *text_input_state_ = Shouldn't this be a .reset call for a unique pointer? (I may be wrong.) https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... File content/public/test/text_input_test_utils.h (right): https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.h:37: virtual void SetUpdateCallback(const Callback& callback) {} These all need comments. That's especially true for things like Update vs UpdateCalled, which isn't clear at first glance. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.h:39: virtual ui::TextInputType GetTextInputType() const = 0; nit: We don't put const in content/ public interfaces: http://www.chromium.org/developers/content-module/content-api "don't add the const identifier to interfaces. For interfaces implemented by the embedder, we can't make assumptions about what the embedder needs to implement it. For interfaces implemented by content, the implementation details doesn't have to be exposed." https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.h:56: ui::TextInputType GetTextInputTypeFromWebContents(WebContents* web_contents); Please add comments for all classes and functions in here, since it's a public interface. Also, let's maybe put the raw functions near the top of the file, rather than mixed in between the test classes. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.h:61: // to the |view|. |view| is also on the browser side. Maybe you can rephrase? (I'm guessing this class is only needed because TextInputState isn't public?) https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.h:71: // Adding the required setter methods. nit: Drop "Adding" and explain what these are required for. (e.g., Setters for TextInputState members?) https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.h:86: // This class is intended to observe the InputMethod. I think it's worth mentioning that the Create method returns a platform-specific subclass of this interface, since it's a bit unclear how this works and why there's no constructor at first glance. (It might also be worth adding a protected constructor, to make it explicit that you need to call Create to get one.)
ekaramad@chromium.org changed reviewers: + lazyboy@chromium.org
Thank you Charlie for the reviews. The part I am a bit unsure is the abstract classes for the public test file. As I understand, we cannot directly subclass from TextInputManager::Observer in the header file since it will violate DEPS rules (text_input_manager.* are in content/browser/renderer_host). I am no sure what the better option is, subclass the way I did it in this patch, or make a wrapper class, or perhaps something else? Also, +lazyboy as he was a reviewer from the original attempt. PTAL. https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/1948343002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:434: // with. On 2016/05/13 20:37:41, kenrb wrote: > On 2016/05/13 16:00:56, ehsaaan wrote: > > On 2016/05/12 16:16:36, kenrb wrote: > > > Explicitly state that this can be nullptr, with a brief explanation of when > > that > > > will be. The comment on GetTextInputManager() mentions it can return nullptr > > but > > > derived RWHV classes use the pointer directly so it should be here also. > > > > Acknowledged. > > Can you please adjust the comment to that affect? Sorry I missed this one. Done! https://codereview.chromium.org/1948343002/diff/220001/chrome/browser/site_pe... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/220001/chrome/browser/site_pe... chrome/browser/site_per_process_interactive_browsertest.cc:248: // TextInputManager and IME Tests On 2016/05/18 20:46:03, Charlie Reis (slow to reply) wrote: > Given the number of helper classes (and the expectation that this might grow), I > think this belongs more in a test file of its own. > Yes, I was thinking within the same lines. I have now moved it to "chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc". > For that matter, should it be a SitePerProcess-specific test class, or do these > tests provide any meaningful coverage for default modes as well? Our FYI bots > will run them with --site-per-process and the equivalent of > --isolate-extensions, so if they pass the same way in default Chrome, maybe we > can make them normal tests for IME. I am not sure if they all can be treated as normal tests. Except for one test about crashing the top-level RWHV, the rest rely on OOPIF-ness. https://codereview.chromium.org/1948343002/diff/220001/chrome/browser/site_pe... chrome/browser/site_per_process_interactive_browsertest.cc:438: // selected for each frame. The test then creates a sequence of tab presses and On 2016/05/18 20:46:03, Charlie Reis (slow to reply) wrote: > What do you mean by distinctly selected? What I was trying to explain here was that each <input> has a unique value and this test monitors the TextInputState.value to make sure that TextInputState is properly tracked. I revised this to reflect exactly what I explained here. https://codereview.chromium.org/1948343002/diff/220001/chrome/browser/site_pe... chrome/browser/site_per_process_interactive_browsertest.cc:439: // verifies that after eahc key press, the right TextInputState is observed. On 2016/05/18 20:46:03, Charlie Reis (slow to reply) wrote: > nit: each Done. https://codereview.chromium.org/1948343002/diff/220001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl.cc:945: return RenderWidgetHostDelegate::GetTextInputManager(); On 2016/05/18 20:46:03, Charlie Reis (slow to reply) wrote: > I think it's clearer to just return nullptr in this case. (I'm not sure if we > would ever put a default implementation in the delegate class.) Done. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:1406: if (text_input_manager_ && text_input_manager_->GetTextInputState()) On 2016/05/18 20:46:03, Charlie Reis (slow to reply) wrote: > I'm curious-- how hard would it be to eliminate the case that > GetTextInputState() returns null? It makes this a bit awkward and introduces a > failure state we didn't have before. It is not hard at all. But, are you suggesting to 1) pass the struct, i.e., if there is no state return TextInputState()? This yields to copying the state around though. 2) pass const ref? Then I need to have a fallback empty TextInputState() to return reference of when we do not have state. Returning a pointer is good and it should always be checked for nullptr. nullptr means there is no state, i.e., no focused <input>. However, alternatively, checking for GetActiveView() also determines if there is focused <input>. GetActiveView() returns nullptr if and only if GetTextInputState() returns nullptr. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2862: DCHECK(text_input_manager_ == text_input_manager); On 2016/05/18 20:46:03, Charlie Reis (slow to reply) wrote: > nit: DCHECK_EQ (here and below) Done. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:407: text_input_manager_->AddObserver(this); On 2016/05/18 20:46:03, Charlie Reis (slow to reply) wrote: > Would it make sense to call TextInputManager::AddObserver from within Register, > or are there cases when we want to register without observing? For testing purposes, we just want to observe and not register. Also, we register to update TextInputManager with state, but we observe it to get updates about state change, ime stuff, etc. So observing and registering should be done separately. Nonetheless, following the comment on line 24 of 'text_input_manager.h', I have now removed the inheritance of RWHVBase from TextInputManager::Observer. So now we only call Register here and the observing part is left with RWHVAura. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:437: text_input_manager_->RemoveObserver(this); On 2016/05/18 20:46:03, Charlie Reis (slow to reply) wrote: > Seems a bit unusual to have the observer reach back into the TextInputManager to > clear this state, rather than having the TextInputManager handle it directly in > its destructor. Maybe it would be better to just do clear text_input_manager_ > here (with the DCHECK above) and clear the rest in ~TextInputManager? Acknowledged. However, I removed this method and added a new method which basically does the same thing. However, now the RWHVBase does not need to observer TextInputManager which is good since most of its methods (in future) will be IME related and only tab's RWHV should observe them (also tests). The new method is called 'DidUnregisterFromTextInputManager'. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:378: // sure to remove the observer before they go away. On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > Maybe you can add a similar comment to the add/remove observer methods in > TextInputManager? Done. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:388: TextInputManager* text_input_manager) override; On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > nit: This belongs near the top, under "IPC::Listener implementation." Done. This is removed from this class since RenderWidgetHostViewBase is no longer an observer of TextInputManager (breaking the observer loop as suggested in your comment on line 24 of "text_input_manager.h"). https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:397: // no reference is found, this method tries to obtain one from the On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > nit: "no no" Done. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:399: // can be obtained. Unless not possible, this method should be used internally What I meant to say is that, we should always use GetTextInputManager() since it tries to initialize everything upon the first call. Basically, |text_input_manager_| is nullptr at first. However, in RWHVAura we need to dereference |text_input_manager_| in some const methods. GetTextInputManager() cannot be const, unless we make it and then initialize |text_input_manager_| by const casting |this|; which would seem weird. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.cc:20: // TODO(ekaramad): Implement this logic for other platforms. On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > Maybe add a NOTREACHED()? We don't want to forget about this when adding more > platforms. Done. Also, added #ifdef USE_AURA in RenderWidgetHostViewBase::TextInputStateChanged. Before, we were hitting this line but it was not affecting IME on Android and Mac since they still override TextInputManager. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.cc:47: bool changed = AreDifferentTextInputStates(text_input_state_map_[view], On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > What if text_input_state_map_[view] were null? AreDifferentTextInputStates > would crash. Seems like many of the other uses of the map include null checks, > so I'm not sure if there's a reason this is safe. (If so, please document.) On line 45, we verify that the view is registered, i.e., the key exists in the map. The value is also a struct (not a pointer). Also, when registering the view, the struct is initialized to TextInputState() (line 67). So I believe we are fine. I added a comment re Registering. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.cc:103: if (active_view_ == view) { On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > Didn't we just do this block in Unregister on line 98? I don't see how we could > hit it again. You are right and the DCHECK was impossible to hit. However, I have now removed this method as TextInputManager is no longer observing RenderWidgetHostViewBase. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:5: #ifndef CONTENT_BROWSER_TEXT_INPUT_MANAGER_H__ On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > nit: Missing RENDERER_HOST Uh yes...I moved this file around a lot. Sorry. :) https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:18: class TestTextInputManagerObserver; On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > This shouldn't be needed. Done. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:20: // A class which receives updates of Text Input State from multiple sources and On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > nit: Remove extra space. > Also, let's be consistent about whether to capitalize Text Input State (this > line and next one). > > Can you give some extra context about why this is needed (e.g., that the > multiple sources may be out-of-process iframes, and this class is deciding what > to use for a WebContents?). Acknowledged. I think to be consistent I should have used TextInputState as it is the name of the struct. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:24: : public RenderWidgetHostViewBaseObserver { On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > It's a bit unfortunate that these two classes need to observe each other, since > it creates a circular dependency. Can we have RWHVB call a method on > TextInputManager when it's destroyed so that we don't need TextInputManager to > observe it? That will lead to less overhead on the other observer methods in > RWHVB and break the circular dependency. I was worried about the observing loop to become confusing to follow and was not sure if I should use a direct API call on RWHVBase. Following your suggestion, the only observers of TextInputManager are RWHV for tab and test observers. RWHVBase gets a new method which does the job of OnDestroyingTextInputManager. I am still keeping OnDestroyingTextInputManager since I believe it will be used when fixing the issue for <webview> (and testing). https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:26: // The top-level tab RWHV should be an observer of TextInputManager to get On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > nit: The tab's top-level RWHV Done. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:35: virtual void OnTextInputStateUpdated(TextInputManager* text_input_manager, On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > I'm curious why the |text_input_manager| parameter is needed. Do the observers > listen to multiple managers? Looks like we just DCHECK it today. Yes that is all we do for now. The only possible case to have two TextInputManager's involed is when there are inner WebContents. Then a view might be observing the inner WebContents at first, and then when attached to outer WebContents, stops observing that as starts observing the outer WebContents' TextInputManager. Also, to be honest, I saw this pattern in base/obsever_list.h comments and presumed that this is part of the style ;). https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:39: // regardless the call having lead to change in TextInputState. In some On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > nit: regardless of whether the call led to Done. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:42: virtual void OnTextInputStateUpdateCalled( On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > Why do we need both this and OnTextInputStateUpdated? Feels like it might be > simpler (and more efficient) to just have this one with a bool did_update_state. Hmm. I think I took the less efficient solution to resolve kenrb@'s comment in patch 8. I will follow your suggestion as it is simpler and looks better. I should also mention that |did_update_state| is only used on Aura. Later, for Mac and Android I could enclose this in #ifdef since |did_update_state| is always true on Android. Also on Mac, even though it is not always true, we only inform RenderWidgetHostViewMac when it is. So it is pointless to pass the argument in those two platforms. But I am not sure if this tiny optimization would be in line with good design/coding practices. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:45: // Notifies all the observers that |text_input_manager| is being destroyed. On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > Let's be consistent with how the other observer methods are phrased: > // Called when the |text_input_manager| is being destroyed. Acknowledged. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:54: explicit TextInputManager(); On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > No need for explicit unless you have a single parameter. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:57: // Returns the current text input state corresponding to |active_source_|. On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > nit: TextInputState (here and below) > nit: s/|active_source_|/the currently active view/ Done. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:65: RenderWidgetHostViewBase* GetActiveView() const; On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > Can this ever be null? Looks like active_view_ is initialized to null, so we > may want to mention when it becomes valid, etc. It is nullptr initially, and whenever we lose focus on <input>. When it is null, there should not be any TextInputState. I revised the comment. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:77: // before destruction of RWHV or TextInputManager. On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > "or TextInputManager" -> Should we add a DCHECK that all views have been > unregistered in ~TextInputManager? > > Also, there is a risk of UaF if we get this wrong. Should active_view_ be a > weak pointer, or are we confident that it will be cleared any time the view goes > away? It depends whether we want the un-registering to happen in TextInputManager, or in the view. I think your comment on RenderWidgetHostViewBase::OnDestroyingTextInputManager suggests that we should only set |text_input_manager_| to null and let TextInputManager take care of un-registering in its destructor. In the current implementation, RWHVs will unregister from TextInputManager (using their member variable |text_input_manager_|) in the destructor of RWHVBase. If the |view| is active, then |active_view_| becomes nullptr. I do not readily see a scenario where |active_view_| is stale. The unregistering part is similar to an observer call. The issue we had before (original patch) was that we were obtaining WebContentsImpl through RenderWidgetHostImpl. That does not always work out fine in the destruction paths as we lose the reference to |host_| early on before the dtor of RenderWidgetHostViewBase is called. Now that we have a reference to |text_input_manager_| in every view, we should be fine with the un-registration process. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:81: bool IsRegisteredView(RenderWidgetHostViewBase* view) const; On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > nit: IsRegistered (to be consistent with Register and Unregister). Done. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:89: // RenderWidgetHostViewObserver implementation. On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > nit: Missing "Base" from name. Yes sorry. But I removed this method following the suggestion on line 24. https://codereview.chromium.org/1948343002/diff/220001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1420: // the RWHV in this WebContents. On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > What is this accomplishing for the inner WebContents? I'm not sure I follow > this. On line 2376, we return the outer WebContents TextInputManager. If a WebContents is attached to outer WebContents, its own TextInputManager should be destroyed. https://codereview.chromium.org/1948343002/diff/220001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1421: // TODO(ekaramad): Is this the right way to for IME handoff? Is it possible to On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > Not sure I understand this TODO. (Right way to what?) Me neither yet. This only affects OOPIF-<webview> and should be dealt with soon after OOPIF IME. Traditionally, in BrowserPluginGuest-based <webview>, we call TextInputStateChanged() on RWHVGuest in some specific occasions. For example, here when <webview> is focused: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br... I am not yet quite certain if the same logic is necessary for OOPIF <webview>. Two simple scenarios I can think of (not confirm either): 1- We will never have meaningful TextInputState inside inner WebContents before it is attached. In that case, destroying the TextInputManager is enough. 2- We might have meaningful state, OR, we need to update the state when <webview> gets focused. I don't know the answer to this yet. This should be resolved later (soon after OOPIF) when dealing with <webview>-IME. Also to clarify, this does not affect non-OOPIF <webview> so current <webview> IME should be fine as long as OOPIF IME is fine. https://codereview.chromium.org/1948343002/diff/220001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1380: std::unique_ptr<TextInputManager> text_input_manager_; On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > Please add a comment here, mentioning something about how this tracks > IME-related state for all widgets in this tab. Done. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... File content/public/test/text_input_test_utils.cc (right): https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:29: class TextInputManagerObserver : public TestTextInputManagerObserver, > Why is this a subclass of TestTextInputManagerObserver? I don't see any other implementations, and unlike InputMethodObserverAura, this doesn't seem to be platform-specific. The reason is that TextInputManager is not publicly visible. The only two solutions I could think of were this, or, creating a wrapper class which entails an implementation TextInputManager::Observer. I went this way. I did not change this for now to get your suggestions on the best way to do this. > I think it would be much simpler and easier to follow if we just had one implementation of TestTextInputManagerObserver. That also means most of these methods don't need to be virtual. I think that would be the case were TestTextInputManagerObserver has a memeber |observer_| which implements TextInputManager::Observer, right? https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:32: // TestTextInputManagerObserver implementations. On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > nit: Move this comment down to SetUpdateCallback, since it doesn't really apply > to the constructor/destructor. Done. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:45: void SetUpdateCallback(const content::TestTextInputManagerObserver::Callback& On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > nit: We're already in the content namespace, so you shouldn't need content:: > here or below. Acknowledged. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:48: new content::TestTextInputManagerObserver::Callback(callback)); On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > You shouldn't need to wrap/copy this. Just pass callback. Acknowledged. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:58: bool GetTextInputValue(std::string& value) const override { On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > We don't use non-const refs. This should be const std::string&, or std::string* > if you're treating it as an out parameter (in which case it must be documented > in the interface). Done. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:115: class RenderWidgetHostViewBaseObserverImpl On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > I'm unclear about why this one is a subclass, too. Can we just move all this > into RenderWidgetHostViewDestructionObserver directly? Same reason as above. What is the best approach? https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:134: DCHECK(view_ == view); On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > nit: DCHECK_EQ Acknowledged. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:168: void SetOnTextInputTypeChangedCallback( On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > nit: Let's be consistent on whether there's a blank line between these methods > or not. Acknowledged. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:170: on_text_input_type_changed_callback_.reset(new base::Closure(callback)); On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > Again, no need to make a copy of callback. Acknowledged. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:223: result[pair.first] = pair.second.type; On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > Is there a reason we need to make a copy of the whole map to return, rather than > just returning a reference to it? It's test code, so I'm not concerned about > the caller mutating the map. The key in the internal map is RenderWidgetHostViewBase. Unless, I change that one into RenderWidgetHostView, this might not work since RenderWidgetHostViewBase is not in content/public. To clarify, I am running browser_tests (and I guess) I cannot include content/ code. At least for extension tests (later for <webview>) I believe this is the case? https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:265: *text_input_state_ = On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > Shouldn't this be a .reset call for a unique pointer? (I may be wrong.) I think we should not. Two reasons I can think of: 1- The TextInputManager::GetTextInputState() returns a const pointer which cannot be owned. 2- We do not want to take ownership of TextInputManager::GetTextInputState() anyway since it will be the actual struct TextInputState for the specific view. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... File content/public/test/text_input_test_utils.h (right): https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.h:34: static std::unordered_map<const RenderWidgetHostView*, ui::TextInputType> This is now a raw function and a friend of TextInputManager. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.h:37: virtual void SetUpdateCallback(const Callback& callback) {} On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > These all need comments. That's especially true for things like Update vs > UpdateCalled, which isn't clear at first glance. Acknowledged. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.h:39: virtual ui::TextInputType GetTextInputType() const = 0; On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > nit: We don't put const in content/ public interfaces: > > http://www.chromium.org/developers/content-module/content-api > "don't add the const identifier to interfaces. For interfaces implemented by the > embedder, we can't make assumptions about what the embedder needs to implement > it. For interfaces implemented by content, the implementation details doesn't > have to be exposed." Acknowledged. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.h:56: ui::TextInputType GetTextInputTypeFromWebContents(WebContents* web_contents); On 2016/05/18 20:46:05, Charlie Reis wrote: > Please add comments for all classes and functions in here, since it's a public > interface. > > Also, let's maybe put the raw functions near the top of the file, rather than > mixed in between the test classes. Acknowledged. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.h:61: // to the |view|. On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > |view| is also on the browser side. Maybe you can rephrase? TextInputState is normally instantiated on the renderer side. As far as I know, only BrowserPluginGuest creates it on the browser side. For testing purposes, we need to create it on the browser side. > (I'm guessing this class is only needed because TextInputState isn't public?) Yes. The same with TextInputManager::Observer which is implemented behind the TestTextInputManagerObserver. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.h:71: // Adding the required setter methods. On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > nit: Drop "Adding" and explain what these are required for. (e.g., Setters for > TextInputState members?) Acknowledged. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.h:86: // This class is intended to observe the InputMethod. On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > I think it's worth mentioning that the Create method returns a platform-specific > subclass of this interface, since it's a bit unclear how this works and why > there's no constructor at first glance. (It might also be worth adding a > protected constructor, to make it explicit that you need to call Create to get > one.) Acknowledged. https://codereview.chromium.org/1948343002/diff/260001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/1948343002/diff/260001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:237: void RenderWidgetHostViewBase::DidUnregisterFromTextInputManager( This method is replacing the observer method OnDestroyingTextInputManager. I also followed the comment and now we only reset the pointer to nullptr.
https://codereview.chromium.org/1948343002/diff/260001/content/public/test/te... File content/public/test/text_input_test_utils.h (right): https://codereview.chromium.org/1948343002/diff/260001/content/public/test/te... content/public/test/text_input_test_utils.h:65: virtual const RenderWidgetHostView* GetUpdatedView() const = 0; I missed these consts. I will remove them soon in a new patch.
Great, thanks. We're getting closer, but sometimes it takes a while to get through very large (1000+ line) CLs. Another round of feedback below, largely on the tests. In general, it helps to run a full "git cl try" before requesting another round of review, since the try jobs communicate to the reviewer that the patch is working. (In CLs like this for OOPIFs, it also helps to manually kick off a "git cl try -b linux_site_isolation" job.) On 2016/05/24 20:42:47, ehsaaan wrote: > Thank you Charlie for the reviews. The part I am a bit unsure is the abstract > classes for the public test file. As I understand, we cannot directly subclass > from TextInputManager::Observer in the header file since it will violate DEPS > rules (text_input_manager.* are in content/browser/renderer_host). I am no sure > what the better option is, subclass the way I did it in this patch, or make a > wrapper class, or perhaps something else? > Thanks for pointing out the non-public inheritance issue. Let's use an inner class to hide the observer implementation. https://codereview.chromium.org/1948343002/diff/220001/chrome/browser/site_pe... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/220001/chrome/browser/site_pe... chrome/browser/site_per_process_interactive_browsertest.cc:248: // TextInputManager and IME Tests On 2016/05/24 20:42:44, ehsaaan wrote: > On 2016/05/18 20:46:03, Charlie Reis (slow to reply) wrote: > > Given the number of helper classes (and the expectation that this might grow), > I > > think this belongs more in a test file of its own. > > > Yes, I was thinking within the same lines. I have now moved it to > "chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc". > > For that matter, should it be a SitePerProcess-specific test class, or do > these > > tests provide any meaningful coverage for default modes as well? Our FYI bots > > will run them with --site-per-process and the equivalent of > > --isolate-extensions, so if they pass the same way in default Chrome, maybe we > > can make them normal tests for IME. > I am not sure if they all can be treated as normal tests. Except for one test > about crashing the top-level RWHV, the rest rely on OOPIF-ness. Ok, a site_per_process style test will do. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:1406: if (text_input_manager_ && text_input_manager_->GetTextInputState()) On 2016/05/24 20:42:44, ehsaaan wrote: > On 2016/05/18 20:46:03, Charlie Reis (slow to reply) wrote: > > I'm curious-- how hard would it be to eliminate the case that > > GetTextInputState() returns null? It makes this a bit awkward and introduces > a > > failure state we didn't have before. > It is not hard at all. But, are you suggesting to > > 1) pass the struct, i.e., if there is no state return TextInputState()? This > yields to copying the state around though. > > 2) pass const ref? Then I need to have a fallback empty TextInputState() to > return reference of when we do not have state. > > Returning a pointer is good and it should always be checked for nullptr. nullptr > means there is no state, i.e., no focused <input>. > > However, alternatively, checking for GetActiveView() also determines if there is > focused <input>. GetActiveView() returns nullptr if and only if > GetTextInputState() returns nullptr. Ok, let's leave it as a possibly null pointer, with the documentation in TextInputManager about when it's expected to be null. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:407: text_input_manager_->AddObserver(this); On 2016/05/24 20:42:44, ehsaaan wrote: > On 2016/05/18 20:46:03, Charlie Reis (slow to reply) wrote: > > Would it make sense to call TextInputManager::AddObserver from within > Register, > > or are there cases when we want to register without observing? > > For testing purposes, we just want to observe and not register. > > Also, we register to update TextInputManager with state, but we observe it to > get updates about state change, ime stuff, etc. So observing and registering > should be done separately. > > Nonetheless, following the comment on line 24 of 'text_input_manager.h', I have > now removed the inheritance of RWHVBase from TextInputManager::Observer. So now > we only call Register here and the observing part is left with RWHVAura. Acknowledged. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:437: text_input_manager_->RemoveObserver(this); On 2016/05/24 20:42:44, ehsaaan wrote: > On 2016/05/18 20:46:03, Charlie Reis (slow to reply) wrote: > > Seems a bit unusual to have the observer reach back into the TextInputManager > to > > clear this state, rather than having the TextInputManager handle it directly > in > > its destructor. Maybe it would be better to just do clear text_input_manager_ > > here (with the DCHECK above) and clear the rest in ~TextInputManager? > > Acknowledged. However, I removed this method and added a new method which > basically does the same thing. However, now the RWHVBase does not need to > observer TextInputManager which is good since most of its methods (in future) > will be IME related and only tab's RWHV should observe them (also tests). > > The new method is called 'DidUnregisterFromTextInputManager'. Acknowledged. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:388: TextInputManager* text_input_manager) override; On 2016/05/24 20:42:45, ehsaaan wrote: > On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > > nit: This belongs near the top, under "IPC::Listener implementation." > > Done. This is removed from this class since RenderWidgetHostViewBase is no > longer an observer of TextInputManager (breaking the observer loop as suggested > in your comment on line 24 of "text_input_manager.h"). Thanks, though that isn't what my line 24 comment was about. I wanted to avoid having TextInputManager observe RWHV, which is the other direction. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.cc:47: bool changed = AreDifferentTextInputStates(text_input_state_map_[view], On 2016/05/24 20:42:45, ehsaaan wrote: > On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > > What if text_input_state_map_[view] were null? AreDifferentTextInputStates > > would crash. Seems like many of the other uses of the map include null > checks, > > so I'm not sure if there's a reason this is safe. (If so, please document.) > > On line 45, we verify that the view is registered, i.e., the key exists in the > map. The value is also a struct (not a pointer). Also, when registering the > view, the struct is initialized to TextInputState() (line 67). > > So I believe we are fine. I added a comment re Registering. Hopefully so. The IsRegistered check is a DCHECK, which means it isn't present in release builds. I suppose it'll be easily diagnosed if we see crashes in AreDifferentTextInputStates, though. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:24: : public RenderWidgetHostViewBaseObserver { On 2016/05/24 20:42:45, ehsaaan wrote: > On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > > It's a bit unfortunate that these two classes need to observe each other, > since > > it creates a circular dependency. Can we have RWHVB call a method on > > TextInputManager when it's destroyed so that we don't need TextInputManager to > > observe it? That will lead to less overhead on the other observer methods in > > RWHVB and break the circular dependency. > > I was worried about the observing loop to become confusing to follow and was not > sure if I should use a direct API call on RWHVBase. > > Following your suggestion, the only observers of TextInputManager are RWHV for > tab and test observers. RWHVBase gets a new method which does the job of > OnDestroyingTextInputManager. Thanks-- it's much easier to follow with the observing only going in one direction. > > I am still keeping OnDestroyingTextInputManager since I believe it will be used > when fixing the issue for <webview> (and testing). We shouldn't add it until it's used. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:35: virtual void OnTextInputStateUpdated(TextInputManager* text_input_manager, On 2016/05/24 20:42:45, ehsaaan wrote: > On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > > I'm curious why the |text_input_manager| parameter is needed. Do the > observers > > listen to multiple managers? Looks like we just DCHECK it today. > > Yes that is all we do for now. The only possible case to have two > TextInputManager's involed is when there are inner WebContents. Then a view > might be observing the inner WebContents at first, and then when attached to > outer WebContents, stops observing that as starts observing the outer > WebContents' TextInputManager. > > Also, to be honest, I saw this pattern in base/obsever_list.h comments and > presumed that this is part of the style ;). I think that only matters when you listen to more than one, but it sounds like that will be possible here. Fine to keep it. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:42: virtual void OnTextInputStateUpdateCalled( On 2016/05/24 20:42:45, ehsaaan wrote: > On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > > Why do we need both this and OnTextInputStateUpdated? Feels like it might be > > simpler (and more efficient) to just have this one with a bool > did_update_state. > > Hmm. I think I took the less efficient solution to resolve kenrb@'s comment in > patch 8. I will follow your suggestion as it is simpler and looks better. > > I should also mention that |did_update_state| is only used on Aura. Later, for > Mac and Android I could enclose this in #ifdef since |did_update_state| is > always true on Android. Also on Mac, even though it is not always true, we only > inform RenderWidgetHostViewMac when it is. So it is pointless to pass the > argument in those two platforms. But I am not sure if this tiny optimization > would be in line with good design/coding practices. I wouldn't worry about it unless there's a substantial difference in frequency. https://codereview.chromium.org/1948343002/diff/220001/content/browser/render... content/browser/renderer_host/text_input_manager.h:77: // before destruction of RWHV or TextInputManager. On 2016/05/24 20:42:45, ehsaaan wrote: > On 2016/05/18 20:46:04, Charlie Reis (slow to reply) wrote: > > "or TextInputManager" -> Should we add a DCHECK that all views have been > > unregistered in ~TextInputManager? > > > > Also, there is a risk of UaF if we get this wrong. Should active_view_ be a > > weak pointer, or are we confident that it will be cleared any time the view > goes > > away? > > It depends whether we want the un-registering to happen in TextInputManager, or > in the view. I think your comment on > RenderWidgetHostViewBase::OnDestroyingTextInputManager suggests that we should > only set |text_input_manager_| to null and let TextInputManager take care of > un-registering in its destructor. > > In the current implementation, RWHVs will unregister from TextInputManager > (using their member variable |text_input_manager_|) in the destructor of > RWHVBase. If the |view| is active, then |active_view_| becomes nullptr. I do not > readily see a scenario where |active_view_| is stale. The unregistering part is > similar to an observer call. > > The issue we had before (original patch) was that we were obtaining > WebContentsImpl through RenderWidgetHostImpl. That does not always work out fine > in the destruction paths as we lose the reference to |host_| early on before the > dtor of RenderWidgetHostViewBase is called. Now that we have a reference to > |text_input_manager_| in every view, we should be fine with the un-registration > process. Acknowledged. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... File content/public/test/text_input_test_utils.cc (right): https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:29: class TextInputManagerObserver : public TestTextInputManagerObserver, On 2016/05/24 20:42:47, ehsaaan wrote: > > Why is this a subclass of TestTextInputManagerObserver? I don't see any other > implementations, and unlike InputMethodObserverAura, this doesn't seem to be > platform-specific. > > The reason is that TextInputManager is not publicly visible. The only two > solutions I could think of were this, or, creating a wrapper class which entails > an implementation TextInputManager::Observer. I went this way. I did not change > this for now to get your suggestions on the best way to do this. > > > I think it would be much simpler and easier to follow if we just had one > implementation of TestTextInputManagerObserver. That also means most of these > methods don't need to be virtual. > > I think that would be the case were TestTextInputManagerObserver has a memeber > |observer_| which implements TextInputManager::Observer, right? Ah, I see. Yes, let's just have an inner class to do the observing of the non-public TextInputManager. That avoids unnecessary inheritance, keeps the interface simpler (by having a constructor) and still hides the implementation details. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:223: result[pair.first] = pair.second.type; On 2016/05/24 20:42:47, ehsaaan wrote: > On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > > Is there a reason we need to make a copy of the whole map to return, rather > than > > just returning a reference to it? It's test code, so I'm not concerned about > > the caller mutating the map. > > The key in the internal map is RenderWidgetHostViewBase. Unless, I change that > one into RenderWidgetHostView, this might not work since > RenderWidgetHostViewBase is not in content/public. > > To clarify, I am running browser_tests (and I guess) I cannot include content/ > code. At least for extension tests (later for <webview>) I believe this is the > case? Ah, that's really subtle. Please add a comment explaining that we need to convert the map from RWHVB to the public RWHV type. Or even better, let's replace this API with GetTextInputTypeForView, as I mention on line 157 of the test file. https://codereview.chromium.org/1948343002/diff/220001/content/public/test/te... content/public/test/text_input_test_utils.cc:265: *text_input_state_ = On 2016/05/24 20:42:46, ehsaaan wrote: > On 2016/05/18 20:46:05, Charlie Reis (slow to reply) wrote: > > Shouldn't this be a .reset call for a unique pointer? (I may be wrong.) > > I think we should not. > > Two reasons I can think of: > > 1- The TextInputManager::GetTextInputState() returns a const pointer which > cannot be owned. > > 2- We do not want to take ownership of TextInputManager::GetTextInputState() > anyway since it will be the actual struct TextInputState for the specific view. Acknowledged. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:27: #ifdef USE_AURA Having the whole file ifdef'd out on other platforms looks a bit weird. We could restrict it to Aura in the build files, but it's probably better to just put a TODO here to enable it on more platforms (since we intend to do that soon). I think you had one before that you could put here. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:37: // TextInputManager Observers nit: Blank line before and after. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:38: // Observing the |TextInputState.value|. Please put a more thorough comment here. This is a base class for several types of observers below, each of which may wait for a different TextInputState-related event. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:49: void Wait() { // Wait for the derived class's definition of success, at which point it will run on_success(). https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:59: base::Closure on_success() { Why does this return a closure, rather than just making OnSuccess() protected and calling it directly? If there's a reason, please document it, but hopefully we can avoid it to simplify this. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:85: explicit TextInputManagerValueObserver(content::WebContents* web_contents, No explicit. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:140: // The following class observers |TextInputState.type| for a specific RWHV. nit: observes https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:157: type_map = content::GetTextInputTypeMapFromWebContents(web_contents_); Can we replace this API with a GetTextInputTypeForView(web_contents, view) (and optionally a HasInputTypeForView if you want to check that as well)? That would avoid copying the whole map and exposing it, and it seems to meet the needs of this test file. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:205: "if (%s && !!document.body.firstChild) {" Maybe "document.body.%s", with the third param being append_as_first_child ? "insertBefore(...)" : "appendChild(...)". https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:223: // Recrusively uses ChildFrameAt(frame, i) to get the i-th child frame nit: Recursively Actually, Iteratively, since this isn't recursive. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:252: using vector = std::vector<size_t>; nit: Blank line after, since it looks like the TODO applies to the whole block. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:272: // both frames and tab key is faked until the one in the second OOPIF is s/faked/pressed/? (I think it's implicit that we're simulating.) https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:273: // focused. Then, the renderer process for both frames are crashed. The test nit: processes https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:274: // verifies that the TextInputManager stop tracking the RWHVs as well as nit: stops https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:275: // properly resetting the TextInputState after the second (active) RWHV goes nit: resets https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:289: // TextInputState. For the second tab two IPC's arrive one from the first nit: arrive: https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:321: // Verifying that the TextInputManager has smaller cleaned the memory Typo? https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:338: // The following test loads a page with two child frames; one in process and one nit: Colon, not semicolon. (The second phrase would need to be a complete sentence of its own to use a semicolon.) https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:488: // |TextInputState.show_ime_if_needed| is true. This should happen even. Even if what? https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:1383: DCHECK(GetTextInputType() != ui::TEXT_INPUT_TYPE_NONE); nit: DCHECK_NE https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:95: public TextInputManager::Observer, Nice. I like that we can be more specific about which RWHV classes observe TextInputManager. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:523: // TextInputManager::Observer implementation. Style nit: Override declarations always go near the top of the section they're in. This should probably go after the DelegatedFrameHostClient overrides above, around line 457. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:419: text_input_manager_ = host->delegate()->GetTextInputManager(); Is there a reason we do this lazily, rather than when the RWHV is constructed? https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:397: // reference is obtained (, which is not nullptr), its value is cached in nit: Commas go before the open paren. Also, you can just say "a non-null reference" to flow more smoothly. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:399: // (,i.e., the instance |text_input_manager_| referes to). The RWHV will nit: No comma before i.e. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:404: // It is safer to use this method when required rather than dereferencing nit: Drop "when required" https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:448: // RenderWidgetHostDelegate owning the TextInputManager is destroyed. Isn't that the WebContents? When does a RenderWidgetHostView outlive the WebContents? https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/text_input_manager.cc:30: // If there is an acitve view, we should unregister it first so that the nit: active https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/text_input_manager.cc:61: // Since |view| is registgered, then we already have a previous value for its nit: registered nit: Drop "then" https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/text_input_manager.h:20: // A class which receives updates of TextInputState from multiple sources and nit: Remove extra space between "from" and "multiple." https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/text_input_manager.h:40: virtual void OnDestroyingTextInputManager( Please remove this for now. We can add it later when there's a use for it. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/text_input_manager.h:47: // Returns the currently active view (,i.e., the RWHV with a focused <input> nit: No comma after open paren. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/text_input_manager.h:73: // Add and remove observers for lifetime event notifications, as well as nit: There's no lifetime event notifications in these observers (for the time being) once we remove the OnDestroying one. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/text_input_manager.h:81: GetTextInputTypeMapFromWebContents(WebContents* web_contents); We won't need this if we expose a GetTextInputTypeForView instead. https://codereview.chromium.org/1948343002/diff/280001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1948343002/diff/280001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1380: // The TextInputManager instance for WebContents. The manager will track the nit: TextInputManager tracks the IME-related... (Drop the first part, which is clear from the context.) https://codereview.chromium.org/1948343002/diff/280001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1382: // outer WebContents and is automatically destroyed when a WebContents nit: outer-most https://codereview.chromium.org/1948343002/diff/280001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1383: // becomes inner WebContents by attaching to an outer WebContents. Then the nit: an inner https://codereview.chromium.org/1948343002/diff/280001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1384: // IME-related state for RenderWidgetHost on the inner WebContents are tracked nit: RenderWidgetHosts nit: s/are/is/ (state ... is tracked) https://codereview.chromium.org/1948343002/diff/280001/content/public/test/te... File content/public/test/text_input_test_utils.cc (right): https://codereview.chromium.org/1948343002/diff/280001/content/public/test/te... content/public/test/text_input_test_utils.cc:203: // TestTextInputManagerObserver implementations. This comment isn't needed. (That's only needed for override blocks in the class declaration.) Same with the similar comments below. https://codereview.chromium.org/1948343002/diff/280001/content/public/test/te... File content/public/test/text_input_test_utils.h (right): https://codereview.chromium.org/1948343002/diff/280001/content/public/test/te... content/public/test/text_input_test_utils.h:45: // Sets a callback which is invoked when a RWHV calls UpdateTextInputManager UpdateTextInputState? https://codereview.chromium.org/1948343002/diff/280001/content/public/test/te... content/public/test/text_input_test_utils.h:76: class RenderWidgetHostViewDestructionObserver { nit: Add comment. (This is a requirement for public APIs.) https://codereview.chromium.org/1948343002/diff/280001/content/public/test/te... content/public/test/text_input_test_utils.h:86: RenderWidgetHostView* GetActiveViewFromWebContents(WebContents* web_contents); nit: Add comment, and maybe move up with the other raw functions above? https://codereview.chromium.org/1948343002/diff/280001/content/public/test/te... content/public/test/text_input_test_utils.h:122: // static nit: Move static comment above the real comment, as done elsewhere. https://codereview.chromium.org/1948343002/diff/280001/content/public/test/te... content/public/test/text_input_test_utils.h:128: virtual ui::TextInputType GetTextInputTypeFromClient() const = 0; No const.
PTAL. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:27: #ifdef USE_AURA On 2016/05/26 06:22:04, Charlie Reis wrote: > Having the whole file ifdef'd out on other platforms looks a bit weird. We > could restrict it to Aura in the build files, but it's probably better to just > put a TODO here to enable it on more platforms (since we intend to do that > soon). I think you had one before that you could put here. Yes. I think I had that TODO in the original file but missed it when moved the tests to this one. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:37: // TextInputManager Observers On 2016/05/26 06:22:04, Charlie Reis wrote: > nit: Blank line before and after. Done. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:38: // Observing the |TextInputState.value|. On 2016/05/26 06:22:04, Charlie Reis wrote: > Please put a more thorough comment here. This is a base class for several types > of observers below, each of which may wait for a different > TextInputState-related event. Done. This comment was wrong to begin with. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:49: void Wait() { On 2016/05/26 06:22:04, Charlie Reis wrote: > // Wait for the derived class's definition of success, at which point it will > run on_success(). I am a bit unclear on the second statement in this comment. What Wait() does is to wait for the derived class's definition of success. It does not run on_success(). https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:59: base::Closure on_success() { On 2016/05/26 06:22:04, Charlie Reis wrote: > Why does this return a closure, rather than just making OnSuccess() protected > and calling it directly? > > If there's a reason, please document it, but hopefully we can avoid it to > simplify this. No reasons and calling OnSuccess() is better. Initially, I was using external functions and statics to verify conditions and passing a closure was the way to go. Now, as you pointed out, it is not the case so it better to just make it protected and call it directly in subclasses' verification methods. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:85: explicit TextInputManagerValueObserver(content::WebContents* web_contents, On 2016/05/26 06:22:03, Charlie Reis wrote: > No explicit. Done. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:140: // The following class observers |TextInputState.type| for a specific RWHV. On 2016/05/26 06:22:04, Charlie Reis wrote: > nit: observes Done. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:157: type_map = content::GetTextInputTypeMapFromWebContents(web_contents_); On 2016/05/26 06:22:03, Charlie Reis wrote: > Can we replace this API with a GetTextInputTypeForView(web_contents, view) (and > optionally a HasInputTypeForView if you want to check that as well)? That would > avoid copying the whole map and exposing it, and it seems to meet the needs of > this test file. Done. Replacing the API with a simple function seems like a much better idea. I am only adding a GetTextInputTypeForView(view, &type) which will return false if we cannot access view's TextInputState (e.g., not registered or host or host->delegate() are missing). https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:205: "if (%s && !!document.body.firstChild) {" On 2016/05/26 06:22:04, Charlie Reis wrote: > Maybe "document.body.%s", with the third param being append_as_first_child ? > "insertBefore(...)" : "appendChild(...)". Agreed. Looks confusing the way it was. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:223: // Recrusively uses ChildFrameAt(frame, i) to get the i-th child frame On 2016/05/26 06:22:04, Charlie Reis wrote: > nit: Recursively > Actually, Iteratively, since this isn't recursive. Done. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:252: using vector = std::vector<size_t>; On 2016/05/26 06:22:04, Charlie Reis wrote: > nit: Blank line after, since it looks like the TODO applies to the whole block. Done. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:272: // both frames and tab key is faked until the one in the second OOPIF is On 2016/05/26 06:22:04, Charlie Reis wrote: > s/faked/pressed/? (I think it's implicit that we're simulating.) Done. Don't we do manual testing of chrome though? TEST=Press tab key until... (just kidding ;)) https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:273: // focused. Then, the renderer process for both frames are crashed. The test On 2016/05/26 06:22:04, Charlie Reis wrote: > nit: processes Done. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:274: // verifies that the TextInputManager stop tracking the RWHVs as well as On 2016/05/26 06:22:03, Charlie Reis wrote: > nit: stops Done. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:275: // properly resetting the TextInputState after the second (active) RWHV goes On 2016/05/26 06:22:04, Charlie Reis wrote: > nit: resets Done. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:289: // TextInputState. For the second tab two IPC's arrive one from the first On 2016/05/26 06:22:04, Charlie Reis wrote: > nit: arrive: Done. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:321: // Verifying that the TextInputManager has smaller cleaned the memory On 2016/05/26 06:22:03, Charlie Reis wrote: > Typo? Sorry...typo seems to be an understatement in this case. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:338: // The following test loads a page with two child frames; one in process and one On 2016/05/26 06:22:04, Charlie Reis wrote: > nit: Colon, not semicolon. (The second phrase would need to be a complete > sentence of its own to use a semicolon.) Acknowledged. https://codereview.chromium.org/1948343002/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:488: // |TextInputState.show_ime_if_needed| is true. This should happen even. On 2016/05/26 06:22:04, Charlie Reis wrote: > Even if what? Done. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:1383: DCHECK(GetTextInputType() != ui::TEXT_INPUT_TYPE_NONE); On 2016/05/26 06:22:04, Charlie Reis wrote: > nit: DCHECK_NE Done. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:95: public TextInputManager::Observer, On 2016/05/26 06:22:04, Charlie Reis wrote: > Nice. I like that we can be more specific about which RWHV classes observe > TextInputManager. My initial intention was to have two classes of observers, or maybe an observer and a delegate where the observers only track the lifetime and the delegate does the IME tasks. But this way with proper API to track lifetime and having only specific RWHV observing this is better since it is more testable than having a delegate. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:523: // TextInputManager::Observer implementation. On 2016/05/26 06:22:04, Charlie Reis wrote: > Style nit: Override declarations always go near the top of the section they're > in. This should probably go after the DelegatedFrameHostClient overrides above, > around line 457. Acknowledged. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:419: text_input_manager_ = host->delegate()->GetTextInputManager(); On 2016/05/26 06:22:04, Charlie Reis wrote: > Is there a reason we do this lazily, rather than when the RWHV is constructed? I do not see any critical reasons against proper initialization. We do this for tab's RWHV; but it is necessary for tab's RWHV since it must observe TextInputManager as soon as and as long as it can. The reason for lazy initialization was that a RWHV might not even have any <input> inside it. Then why bother with tracking IME when it can not even have it. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:397: // reference is obtained (, which is not nullptr), its value is cached in On 2016/05/26 06:22:04, Charlie Reis wrote: > nit: Commas go before the open paren. Also, you can just say "a non-null > reference" to flow more smoothly. Acknowledged. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:399: // (,i.e., the instance |text_input_manager_| referes to). The RWHV will On 2016/05/26 06:22:05, Charlie Reis wrote: > nit: No comma before i.e. Done. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:404: // It is safer to use this method when required rather than dereferencing On 2016/05/26 06:22:04, Charlie Reis wrote: > nit: Drop "when required" Done. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:448: // RenderWidgetHostDelegate owning the TextInputManager is destroyed. On 2016/05/26 06:22:04, Charlie Reis wrote: > Isn't that the WebContents? As far as I can tell, yes it is. > When does a RenderWidgetHostView outlive the > WebContents? Not really. It is just we cannot guarantee the order of destruction. When WebContentsImpl is deleted, we delete the frame tree and frame tree node for the root. That is when we delete RenderWidgetHostViewAura. In fact, if I do not reset text_input_manager_ to null when TextInputManager is destroyed, we get a UaF when closing the window. Also, for RenderWidgetHostViewChildFrame we use a DeleteSoon which I think could actually make it go away after WebContents. All this being said, I modified the comment to mention only TextInputManager not the delegate. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/text_input_manager.cc:30: // If there is an acitve view, we should unregister it first so that the On 2016/05/26 06:22:05, Charlie Reis wrote: > nit: active Done. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/text_input_manager.cc:61: // Since |view| is registgered, then we already have a previous value for its On 2016/05/26 06:22:05, Charlie Reis wrote: > nit: registered > nit: Drop "then" Done. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/text_input_manager.h:20: // A class which receives updates of TextInputState from multiple sources and On 2016/05/26 06:22:05, Charlie Reis wrote: > nit: Remove extra space between "from" and "multiple." Done. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/text_input_manager.h:40: virtual void OnDestroyingTextInputManager( On 2016/05/26 06:22:05, Charlie Reis wrote: > Please remove this for now. We can add it later when there's a use for it. Done. I removed it, but there is something about not having such a call which does not add up for me. RWHVs register with TextInputManager to make it track their state. Those who use these states and updates, i.e., tab's RWHV, will add themselves as observers. So we have two categories of entities here: registered views, and observers. Now, when TextInputManager goes away it will unregister all RWHVs, where during the process, they all lose their reference to TextInputManager. But the observers will never know this. As for tab's RWHV it is fine because we also register the RWHV even if there are no <input> (in the ctor of RWHVAura). So we are fine; but I think not for the right reason. In other words, RWHVAura is on the clear because it is a view, not because it is an observer. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/text_input_manager.h:47: // Returns the currently active view (,i.e., the RWHV with a focused <input> On 2016/05/26 06:22:05, Charlie Reis wrote: > nit: No comma after open paren. Done. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/text_input_manager.h:73: // Add and remove observers for lifetime event notifications, as well as On 2016/05/26 06:22:05, Charlie Reis wrote: > nit: There's no lifetime event notifications in these observers (for the time > being) once we remove the OnDestroying one. Acknowledged. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/text_input_manager.h:81: GetTextInputTypeMapFromWebContents(WebContents* web_contents); On 2016/05/26 06:22:05, Charlie Reis wrote: > We won't need this if we expose a GetTextInputTypeForView instead. I am not sure if we need a GetTextInputTypeForView() in TextInputManager. I have added the method in test utils, but other than that, I don't quite see application of it anywhere else. That being said, we might later on have to do such a thing, if not for Type, maybe for another IME related state which is kept in TextInputManager. If you think we should expose it nonetheless, I could modify the definition of TextInputManager::GetTextInputState() to: TextInputManager::GetTextInputState(RenderWidgetHostViewBase* view = nullptr) { if (!view) view = active_view_; if (!IsRegistered(view)) return nullptr; return !!view ? &text_input_state_map_[view] : nullptr; } What do you think? https://codereview.chromium.org/1948343002/diff/280001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1948343002/diff/280001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1380: // The TextInputManager instance for WebContents. The manager will track the On 2016/05/26 06:22:05, Charlie Reis wrote: > nit: TextInputManager tracks the IME-related... > (Drop the first part, which is clear from the context.) Acknowledged. https://codereview.chromium.org/1948343002/diff/280001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1382: // outer WebContents and is automatically destroyed when a WebContents On 2016/05/26 06:22:05, Charlie Reis wrote: > nit: outer-most Done. Put 'outermost' rather than 'outer-most' as the former is a word with the intended meaning; unless we use outer-most for another reason? (search in content/ 443 instances of outermost vs 22 for outer-most; spooky btw since both relate to port numbers with secure connections - and you work on Chrome security! #texassharpshooterfallacy). https://codereview.chromium.org/1948343002/diff/280001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1383: // becomes inner WebContents by attaching to an outer WebContents. Then the On 2016/05/26 06:22:05, Charlie Reis wrote: > nit: an inner Done. https://codereview.chromium.org/1948343002/diff/280001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1384: // IME-related state for RenderWidgetHost on the inner WebContents are tracked On 2016/05/26 06:22:05, Charlie Reis wrote: > nit: RenderWidgetHosts > > nit: s/are/is/ > (state ... is tracked) Acknowledged. https://codereview.chromium.org/1948343002/diff/280001/content/public/test/te... File content/public/test/text_input_test_utils.cc (right): https://codereview.chromium.org/1948343002/diff/280001/content/public/test/te... content/public/test/text_input_test_utils.cc:187: const TextInputState* state = static_cast<WebContentsImpl*>(web_contents) I removed the namespace but forgot to move the raw functions to before the class definitions. I will do that in the next patch. https://codereview.chromium.org/1948343002/diff/280001/content/public/test/te... content/public/test/text_input_test_utils.cc:203: // TestTextInputManagerObserver implementations. On 2016/05/26 06:22:05, Charlie Reis wrote: > This comment isn't needed. (That's only needed for override blocks in the class > declaration.) > > Same with the similar comments below. Acknowledged. https://codereview.chromium.org/1948343002/diff/280001/content/public/test/te... File content/public/test/text_input_test_utils.h (right): https://codereview.chromium.org/1948343002/diff/280001/content/public/test/te... content/public/test/text_input_test_utils.h:45: // Sets a callback which is invoked when a RWHV calls UpdateTextInputManager On 2016/05/26 06:22:05, Charlie Reis wrote: > UpdateTextInputState? Acknowledged. https://codereview.chromium.org/1948343002/diff/280001/content/public/test/te... content/public/test/text_input_test_utils.h:76: class RenderWidgetHostViewDestructionObserver { On 2016/05/26 06:22:05, Charlie Reis wrote: > nit: Add comment. (This is a requirement for public APIs.) Acknowledged. https://codereview.chromium.org/1948343002/diff/280001/content/public/test/te... content/public/test/text_input_test_utils.h:86: RenderWidgetHostView* GetActiveViewFromWebContents(WebContents* web_contents); On 2016/05/26 06:22:05, Charlie Reis wrote: > nit: Add comment, and maybe move up with the other raw functions above? Acknowledged. https://codereview.chromium.org/1948343002/diff/280001/content/public/test/te... content/public/test/text_input_test_utils.h:122: // static On 2016/05/26 06:22:05, Charlie Reis wrote: > nit: Move static comment above the real comment, as done elsewhere. Acknowledged. https://codereview.chromium.org/1948343002/diff/280001/content/public/test/te... content/public/test/text_input_test_utils.h:128: virtual ui::TextInputType GetTextInputTypeFromClient() const = 0; On 2016/05/26 06:22:05, Charlie Reis wrote: > No const. Done.
Whew! I think we've made it. LGTM with nits. Thanks for pushing through. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:448: // RenderWidgetHostDelegate owning the TextInputManager is destroyed. On 2016/05/30 15:06:08, ehsaaan wrote: > On 2016/05/26 06:22:04, Charlie Reis wrote: > > Isn't that the WebContents? > As far as I can tell, yes it is. > > > When does a RenderWidgetHostView outlive the > > WebContents? > Not really. It is just we cannot guarantee the order of destruction. When > WebContentsImpl is deleted, we delete the frame tree and frame tree node for the > root. That is when we delete RenderWidgetHostViewAura. In fact, if I do not > reset text_input_manager_ to null when TextInputManager is destroyed, we get a > UaF when closing the window. > > Also, for RenderWidgetHostViewChildFrame we use a DeleteSoon which I think could > actually make it go away after WebContents. > > All this being said, I modified the comment to mention only TextInputManager not > the delegate. > Acknowledged. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/text_input_manager.h:40: virtual void OnDestroyingTextInputManager( On 2016/05/30 15:06:08, ehsaaan wrote: > On 2016/05/26 06:22:05, Charlie Reis wrote: > > Please remove this for now. We can add it later when there's a use for it. > > Done. > > I removed it, but there is something about not having such a call which does not > add up for me. RWHVs register with TextInputManager to make it track their > state. Those who use these states and updates, i.e., tab's RWHV, will add > themselves as observers. So we have two categories of entities here: registered > views, and observers. > > Now, when TextInputManager goes away it will unregister all RWHVs, where during > the process, they all lose their reference to TextInputManager. > > But the observers will never know this. As for tab's RWHV it is fine because we > also register the RWHV even if there are no <input> (in the ctor of RWHVAura). > So we are fine; but I think not for the right reason. In other words, RWHVAura > is on the clear because it is a view, not because it is an observer. I'm not sure I follow that in depth, but my intention here is to not add code until it's used. I'm fine with either of these two options: 1) Include OnDestroyingTextInputManager and a place that uses it (e.g., in a test). 2) Put a TODO to add OnDestroyingTextInputManager when it's needed for observers. https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... content/browser/renderer_host/text_input_manager.h:81: GetTextInputTypeMapFromWebContents(WebContents* web_contents); On 2016/05/30 15:06:08, ehsaaan wrote: > On 2016/05/26 06:22:05, Charlie Reis wrote: > > We won't need this if we expose a GetTextInputTypeForView instead. > > I am not sure if we need a GetTextInputTypeForView() in TextInputManager. I have > added the method in test utils, but other than that, I don't quite see > application of it anywhere else. > > That being said, we might later on have to do such a thing, if not for Type, > maybe for another IME related state which is kept in TextInputManager. > > If you think we should expose it nonetheless, I could modify the definition of > TextInputManager::GetTextInputState() > to: > > TextInputManager::GetTextInputState(RenderWidgetHostViewBase* view = nullptr) { > if (!view) > view = active_view_; > if (!IsRegistered(view)) > return nullptr; > > return !!view ? &text_input_state_map_[view] : nullptr; > } > > What do you think? Sorry, that's not what I meant. I just meant we wouldn't need to friend GetTextInputTypeMapFromWebContents. I agree with keeping GetTextInputTypeForView as a test method. It would still be nice to avoid the friend declaration if possible. Maybe we can have a public GetTextInputTypeForTesting(view) on TextInputManager, and call it from the GetTextInputTypeForView test method? https://codereview.chromium.org/1948343002/diff/280001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1948343002/diff/280001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1382: // outer WebContents and is automatically destroyed when a WebContents On 2016/05/30 15:06:09, ehsaaan wrote: > On 2016/05/26 06:22:05, Charlie Reis wrote: > > nit: outer-most > > Done. Put 'outermost' rather than 'outer-most' as the former is a word with the > intended meaning; unless we use outer-most for another reason? > (search in content/ 443 instances of outermost vs 22 for outer-most; spooky btw > since both relate to port numbers with secure connections - and you work on > Chrome security! #texassharpshooterfallacy). Outermost is fine, thanks. https://codereview.chromium.org/1948343002/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/1948343002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:397: // null reference is obtained , its value is cached in |text_input_manager_|. nit: No space before comma. https://codereview.chromium.org/1948343002/diff/300001/content/browser/render... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/1948343002/diff/300001/content/browser/render... content/browser/renderer_host/text_input_manager.cc:58: // Since |view| is registgered, we already have a previous value for its nit: registered https://codereview.chromium.org/1948343002/diff/300001/content/public/test/te... File content/public/test/text_input_test_utils.cc (right): https://codereview.chromium.org/1948343002/diff/300001/content/public/test/te... content/public/test/text_input_test_utils.cc:83: // This class observers the lifetime of a RenderWidgetHostView. An instance of nit: observes https://codereview.chromium.org/1948343002/diff/300001/content/public/test/te... File content/public/test/text_input_test_utils.h (right): https://codereview.chromium.org/1948343002/diff/300001/content/public/test/te... content/public/test/text_input_test_utils.h:27: // This method returns true of |view| is registered in the TextInputManager that nit: s/of/if/ https://codereview.chromium.org/1948343002/diff/300001/content/public/test/te... content/public/test/text_input_test_utils.h:29: // the |TextInputState.type| corresponding to the |view|. Returns false if|view| nit: Space after "if" https://codereview.chromium.org/1948343002/diff/380001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/380001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:34: // The following tests verify the correctness of TextInputState tracking on the Can you add something about how this test is outside content/ because it relies on being part of the interactive UI test framework (to avoid flakiness)? I think the only breadcrumb we have for that is in the build file, now that the test name doesn't include "interactive." https://codereview.chromium.org/1948343002/diff/380001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:37: // legacy implementation in RWHV's other than RWHVCF.). nit: No period before the close paren. https://codereview.chromium.org/1948343002/diff/380001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:45: // changes. The class wraps a public test observer which accepts callbacks that nit: public tester https://codereview.chromium.org/1948343002/diff/380001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/1948343002/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:253: // Updates the state of the input method attached to the view. Please clarify why this would be overridden. (It's not clear to me why it's virtual but has a default implementation with no overrides.) https://codereview.chromium.org/1948343002/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:400: // null reference is obtained , its value is cached in |text_input_manager_|. nit: No space before comma. https://codereview.chromium.org/1948343002/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:401: // Then this view is registered with TextInputManage (i.e., the instance nit: is registered with |text_input_manager_|. Or even more simply: its value is cached in |text_input_manager_| and this view is registered with it. https://codereview.chromium.org/1948343002/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:406: // TextInputManager::Unregister. It is safer to use this method rather than nit: New paragraph for "It is safer..." https://codereview.chromium.org/1948343002/diff/380001/content/browser/render... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/1948343002/diff/380001/content/browser/render... content/browser/renderer_host/text_input_manager.h:58: // TextInputManager will observe the lifetime of the |view|. This is expected to be called for all views (including children), right? Can we add that to the comment? https://codereview.chromium.org/1948343002/diff/380001/content/browser/render... content/browser/renderer_host/text_input_manager.h:72: void AddObserver(Observer* observer); In contrast, I think we only expect this to be called for the top-level / native view, right? (Do we only have multiple observers in the case of tests? That's fine, but might be worth mentioning.) https://codereview.chromium.org/1948343002/diff/380001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/380001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:66: #include "ui/base/ime/text_input_type.h" These are all stale and can be removed. https://codereview.chromium.org/1948343002/diff/380001/content/public/test/te... File content/public/test/text_input_test_utils.cc (right): https://codereview.chromium.org/1948343002/diff/380001/content/public/test/te... content/public/test/text_input_test_utils.cc:84: // This class observers the lifetime of a RenderWidgetHostView. An instance of nit: observes https://codereview.chromium.org/1948343002/diff/380001/content/public/test/te... content/public/test/text_input_test_utils.cc:263: // TextInputStateSender implementations. nit: This comment isn't needed. https://codereview.chromium.org/1948343002/diff/380001/content/public/test/te... File content/public/test/text_input_test_utils.h (right): https://codereview.chromium.org/1948343002/diff/380001/content/public/test/te... content/public/test/text_input_test_utils.h:27: // This method returns true of |view| is registered in the TextInputManager that nit: s/of/if/ https://codereview.chromium.org/1948343002/diff/380001/content/public/test/te... content/public/test/text_input_test_utils.h:29: // the |TextInputState.type| corresponding to the |view|. Returns false if|view| nit: Space after if
Thank you Charlie for the detailed reviews. Adding sky@ for owner's LGTM on the "chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc". Please, take a look. Thanks. https://codereview.chromium.org/1948343002/diff/380001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/380001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:34: // The following tests verify the correctness of TextInputState tracking on the On 2016/06/02 22:03:29, Charlie Reis wrote: > Can you add something about how this test is outside content/ because it relies > on being part of the interactive UI test framework (to avoid flakiness)? I > think the only breadcrumb we have for that is in the build file, now that the > test name doesn't include "interactive." Acknowledged. https://codereview.chromium.org/1948343002/diff/380001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:37: // legacy implementation in RWHV's other than RWHVCF.). On 2016/06/02 22:03:29, Charlie Reis wrote: > nit: No period before the close paren. Done. https://codereview.chromium.org/1948343002/diff/380001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:45: // changes. The class wraps a public test observer which accepts callbacks that On 2016/06/02 22:03:29, Charlie Reis wrote: > nit: public tester Done. https://codereview.chromium.org/1948343002/diff/380001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/1948343002/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:253: // Updates the state of the input method attached to the view. On 2016/06/02 22:03:29, Charlie Reis wrote: > Please clarify why this would be overridden. (It's not clear to me why it's > virtual but has a default implementation with no overrides.) Yes. This should be temporary and resolved after we fix the TextInputState for other RWHV*. After that this method does not need to be virtual anymore and will be moved up top with the rest of non-virtuals. https://codereview.chromium.org/1948343002/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:400: // null reference is obtained , its value is cached in |text_input_manager_|. On 2016/06/02 22:03:29, Charlie Reis wrote: > nit: No space before comma. Done. https://codereview.chromium.org/1948343002/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:401: // Then this view is registered with TextInputManage (i.e., the instance On 2016/06/02 22:03:29, Charlie Reis wrote: > nit: is registered with |text_input_manager_|. > > Or even more simply: > its value is cached in |text_input_manager_| and this view is registered with > it. Done. Second suggestion applied. https://codereview.chromium.org/1948343002/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:406: // TextInputManager::Unregister. It is safer to use this method rather than On 2016/06/02 22:03:29, Charlie Reis wrote: > nit: New paragraph for "It is safer..." Done. https://codereview.chromium.org/1948343002/diff/380001/content/browser/render... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/1948343002/diff/380001/content/browser/render... content/browser/renderer_host/text_input_manager.h:58: // TextInputManager will observe the lifetime of the |view|. On 2016/06/02 22:03:29, Charlie Reis wrote: > This is expected to be called for all views (including children), right? Can we > add that to the comment? Done. Also modified a bit as TextInputManager is not observing view but rather view unregisters from TextInputManager when it is destroyed. https://codereview.chromium.org/1948343002/diff/380001/content/browser/render... content/browser/renderer_host/text_input_manager.h:72: void AddObserver(Observer* observer); On 2016/06/02 22:03:29, Charlie Reis wrote: > In contrast, I think we only expect this to be called for the top-level / native > view, right? (Do we only have multiple observers in the case of tests? That's > fine, but might be worth mentioning.) At any point in time, we might have more than 2. The observers remove themselves from the list when destroyed. But I might be using (now or later) multiple test observers within the same test. But apart from the tests, tab's RWHV must be the only one. https://codereview.chromium.org/1948343002/diff/380001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/380001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:66: #include "ui/base/ime/text_input_type.h" On 2016/06/02 22:03:29, Charlie Reis wrote: > These are all stale and can be removed. Yes. Sorry I forgot. https://codereview.chromium.org/1948343002/diff/380001/content/public/test/te... File content/public/test/text_input_test_utils.cc (right): https://codereview.chromium.org/1948343002/diff/380001/content/public/test/te... content/public/test/text_input_test_utils.cc:84: // This class observers the lifetime of a RenderWidgetHostView. An instance of On 2016/06/02 22:03:30, Charlie Reis wrote: > nit: observes Thanks. This is now officially my signature typo. https://codereview.chromium.org/1948343002/diff/380001/content/public/test/te... content/public/test/text_input_test_utils.cc:263: // TextInputStateSender implementations. On 2016/06/02 22:03:30, Charlie Reis wrote: > nit: This comment isn't needed. Acknowledged. https://codereview.chromium.org/1948343002/diff/380001/content/public/test/te... File content/public/test/text_input_test_utils.h (right): https://codereview.chromium.org/1948343002/diff/380001/content/public/test/te... content/public/test/text_input_test_utils.h:27: // This method returns true of |view| is registered in the TextInputManager that On 2016/06/02 22:03:30, Charlie Reis wrote: > nit: s/of/if/ Done. https://codereview.chromium.org/1948343002/diff/380001/content/public/test/te... content/public/test/text_input_test_utils.h:29: // the |TextInputState.type| corresponding to the |view|. Returns false if|view| On 2016/06/02 22:03:30, Charlie Reis wrote: > nit: Space after if Done.
ekaramad@chromium.org changed reviewers: + sky@chromium.org
sky@chromium.org: Please review changes in + sky@ for owner's LGTM on the test file in chrome/browser/renderer_host/ (whom I forgot to add before). Please take a look. Thanks!
https://codereview.chromium.org/1948343002/diff/420001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/420001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:30: #ifdef USE_AURA That bug is close to 3 months old. Who knows how long it will take to fix. In the mean time only compile for aura and remove the ifdef. https://codereview.chromium.org/1948343002/diff/420001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:38: // These tests live outside content/ because they rely on being part of the Why not have interactive ui tests include this file and move to content?
https://codereview.chromium.org/1948343002/diff/420001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/420001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:38: // These tests live outside content/ because they rely on being part of the On 2016/06/03 22:38:49, sky wrote: > Why not have interactive ui tests include this file and move to content? I didn't realize that was possible. Interactive UI tests can just pull in test files from content/ and they'll be treated the same way as if they were in chrome/? If so, we should move site_per_process_interactive_browsertest.cc as well. (General comment, not for this CL.)
On Fri, Jun 3, 2016 at 4:03 PM, <creis@chromium.org> wrote: > > https://codereview.chromium.org/1948343002/diff/420001/chrome/browser/rendere... > File > chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc > (right): > > https://codereview.chromium.org/1948343002/diff/420001/chrome/browser/rendere... > chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:38: > // These tests live outside content/ because they rely on being part of > the > On 2016/06/03 22:38:49, sky wrote: >> Why not have interactive ui tests include this file and move to > content? > > I didn't realize that was possible. Interactive UI tests can just pull > in test files from content/ and they'll be treated the same way as if > they were in chrome/? I believe that is the case. You'll have to verify that is true for isolates. > If so, we should move site_per_process_interactive_browsertest.cc as > well. (General comment, not for this CL.) > https://codereview.chromium.org/1948343002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks for the reviews. PTAL. https://codereview.chromium.org/1948343002/diff/420001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/420001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:30: #ifdef USE_AURA On 2016/06/03 22:38:49, sky wrote: > That bug is close to 3 months old. Who knows how long it will take to fix. In > the mean time only compile for aura and remove the ifdef. The intention was to activate the tests on Mac/Android within a week as these are all parts of the same bug fixes. Either way, point taken, and the test only compiles for Aura now. I will change that once we have fixed things for other platforms. https://codereview.chromium.org/1948343002/diff/420001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:38: // These tests live outside content/ because they rely on being part of the On 2016/06/03 22:38:49, sky wrote: > Why not have interactive ui tests include this file and move to content? As per our offline discussion and the fact that I am including chrome/ files, I am keeping the test file in chrome/ as well.
LGTM https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:24: nit: remove newline. https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:104: std::string expected_value_; nit const https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:105: }; nit: DISALLOW... https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:127: ui::TextInputType expected_type_; const and DISALLOW... https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:146: }; DISALLOW.. https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:173: ui::TextInputType expected_type_; const and DISALLOW https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:259: using vector = std::vector<size_t>; nit: vector -> Vector https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:288: using vector = std::vector<size_t>; Vector. And as you have this in each test put it at the top of the file. Also, Vector is way to vague, maybe IdVector? https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:492: }; private: DISALLOW..
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/patch-status/1948343002/480001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks for the reviews! https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:24: On 2016/06/07 19:44:45, sky wrote: > nit: remove newline. Done. https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:104: std::string expected_value_; On 2016/06/07 19:44:45, sky wrote: > nit const Done. https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:105: }; On 2016/06/07 19:44:44, sky wrote: > nit: DISALLOW... Done. https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:127: ui::TextInputType expected_type_; On 2016/06/07 19:44:45, sky wrote: > const and DISALLOW... Done. https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:146: }; On 2016/06/07 19:44:45, sky wrote: > DISALLOW.. Done. https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:173: ui::TextInputType expected_type_; On 2016/06/07 19:44:44, sky wrote: > const and DISALLOW Done. https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:259: using vector = std::vector<size_t>; On 2016/06/07 19:44:45, sky wrote: > nit: vector -> Vector Acknowledged. https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:288: using vector = std::vector<size_t>; On 2016/06/07 19:44:45, sky wrote: > Vector. And as you have this in each test put it at the top of the file. Also, > Vector is way to vague, maybe IdVector? Acknowledged. https://codereview.chromium.org/1948343002/diff/440001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:492: }; On 2016/06/07 19:44:45, sky wrote: > private: DISALLOW.. Done.
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/patch-status/1948343002/500001
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 shuchen@chromium.org, kenrb@chromium.org, creis@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1948343002/#ps500001 (title: "Rebase (Code changed during CQ Dry-run)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1948343002/500001
Message was sent while issue was closed.
Committed patchset #26 (id:500001)
Message was sent while issue was closed.
Description was changed from ========== [reland] Browser Side Text Input State Tracking for OOPIF (Aura Only) This CL relands the original issue https://codereview.chromium.org/1652483002/, with modifications and for aura only (Linux, Windows, etc.). T A TextInputManager is introduced which owned by the RenderWidgetHostDelegate (mostly, WebContentsImpl). Each RWHV will observe the TextInputManager for its destruction and lifetime while the TextInputManager observers all the RWHVs on the tab. The role of the TextInputManager is to bookkeep and store TextInputState from all RWHV and provide the correct TextInputState and the RWHV with active TextInputState at anytime. These information are used by the tab-RWHV in handling IME related tasks. This CL also adds several tests and some utility methods to help testing IME and TextInputState for OOPIF. BUG=578168, 602723 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== [reland] Browser Side Text Input State Tracking for OOPIF (Aura Only) This CL relands the original issue https://codereview.chromium.org/1652483002/, with modifications and for aura only (Linux, Windows, etc.). T A TextInputManager is introduced which owned by the RenderWidgetHostDelegate (mostly, WebContentsImpl). Each RWHV will observe the TextInputManager for its destruction and lifetime while the TextInputManager observers all the RWHVs on the tab. The role of the TextInputManager is to bookkeep and store TextInputState from all RWHV and provide the correct TextInputState and the RWHV with active TextInputState at anytime. These information are used by the tab-RWHV in handling IME related tasks. This CL also adds several tests and some utility methods to help testing IME and TextInputState for OOPIF. BUG=578168, 602723 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223 Cr-Commit-Position: refs/heads/master@{#398559} ==========
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223 Cr-Commit-Position: refs/heads/master@{#398559}
Message was sent while issue was closed.
FYI: this CL broke 'CFI Linux' buildbot, see https://crbug.com/619147 for more details. |
