Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1779673003:
Added network components for blimp text input feature (Closed)
|
Created:
4 years, 9 months ago by shaktisahu Modified:
4 years, 9 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded network components for blimp text input feature
This is the third CL for enabling text input. Created protobuf ImeMessage containing text and IME related information to be sent over network. Added WebInputFeature and WebInputFeatureDelegate to handle these messages.
BUG=585646
Committed: https://crrev.com/ded7b6ba488f2163e9588573cbb5e705222b9205
Cr-Commit-Position: refs/heads/master@{#383242}
Patch Set 1 #
Total comments: 41
Patch Set 2 : Pulled out IME related engine code to EngineRenderWidgetFeature #Patch Set 3 : Added proto coverters for TextInputType #
Total comments: 49
Patch Set 4 : Added unit tests and addressed comments #
Total comments: 64
Patch Set 5 : code review comments #Patch Set 6 : Fixed initial values for tab id in ImeFeature #
Total comments: 25
Patch Set 7 : Code review comments from Wez #
Total comments: 6
Patch Set 8 : Cleanup #Patch Set 9 : Fixed DEPS file and a converter method #Patch Set 10 : Added text_input_types to BUILD.gn #
Total comments: 4
Patch Set 11 : More deps #Patch Set 12 : Merge origin/master #Messages
Total messages: 58 (14 generated)
Description was changed from ========== Added network components for blimp text input feature formatting Blimp text input - integration of client and engine Merge branch 'refs/heads/master' into input_showime Merge branch 'refs/heads/input_text' into input_native Merge branch 'refs/heads/master' into input_text Merge branch 'refs/heads/input_text' into input_native Merge branch 'refs/heads/master' into input_text Merge branch 'input_text' into input_native Merge branch 'master' into input_text formatting spaces more comments Added more comments Merge branch 'refs/heads/input_text' into input_native Merge branch 'refs/heads/master' into input_text Merge branch 'master' into input_native Added jni components for text input Code review comments Adding blimp text input ui Initial version BUG= ========== to ========== Added network components for blimp text input feature This is the third CL for enabling text input. Created protobuf ImeMessage containing text and IME related information to be sent over network. Added WebInputFeature and WebInputFeatureDelegate to handle these messages. BUG=585646 ==========
shaktisahu@chromium.org changed reviewers: + dtrainor@chromium.org, khushalsagar@chromium.org, nyquist@chromium.org
https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/ja... File blimp/client/app/android/java/src/org/chromium/blimp/input/WebInputBox.java (right): https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/ja... blimp/client/app/android/java/src/org/chromium/blimp/input/WebInputBox.java:35: if (actionId == EditorInfo.IME_ACTION_NEXT || actionId == EditorInfo.IME_ACTION_DONE Nit: This if-statement is getting rather large. Should we change it to become a switch with fall throughs instead? Something like: switch(actionId) { case EditorInfo...: // Intentional fall through. case EditorInfo...: // Intentional fall through. case EditorInfo...: // current code default: return false; } WDYT? https://codereview.chromium.org/1779673003/diff/1/blimp/client/feature/web_in... File blimp/client/feature/web_input_feature.cc (right): https://codereview.chromium.org/1779673003/diff/1/blimp/client/feature/web_in... blimp/client/feature/web_input_feature.cc:35: VLOG(0) << "Sending ime text to engine, "; Should this be VLOG(1) ? Nit: 'IME' and extra space after punctuation. (And I think you meant period, not comma) https://codereview.chromium.org/1779673003/diff/1/blimp/client/feature/web_in... File blimp/client/feature/web_input_feature.h (right): https://codereview.chromium.org/1779673003/diff/1/blimp/client/feature/web_in... blimp/client/feature/web_input_feature.h:20: // floating WebInputBox. I guess WebInputbox isn't know by this class, so maybe just refer to the WebInputFeatureDelegate here? https://codereview.chromium.org/1779673003/diff/1/blimp/client/feature/web_in... blimp/client/feature/web_input_feature.h:21: class BLIMP_CLIENT_EXPORT WebInputFeature : public BlimpMessageProcessor { Should there be a test for this class? It doesn't have a lot of external dependencies, so hopefully shouldn't be too crazy to add one. https://codereview.chromium.org/1779673003/diff/1/blimp/client/feature/web_in... blimp/client/feature/web_input_feature.h:42: // Sends text from Ime to the blimp engine. Nit: 'IME' https://codereview.chromium.org/1779673003/diff/1/blimp/common/create_blimp_m... File blimp/common/create_blimp_message.h (right): https://codereview.chromium.org/1779673003/diff/1/blimp/common/create_blimp_m... blimp/common/create_blimp_message.h:20: class ImeMessage; Could you make these adhere to alphabetical ordering? https://codereview.chromium.org/1779673003/diff/1/blimp/common/proto/ime.proto File blimp/common/proto/ime.proto (right): https://codereview.chromium.org/1779673003/diff/1/blimp/common/proto/ime.prot... blimp/common/proto/ime.proto:20: SHOW_TEXT = 3; SHOW_TEXT seems to be used to send text from the client to the Engine. And if we receive this on the client, we hit a NOTREACHED. Should this be under Client => Server below? https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:175: window_tree_host_->GetInputMethod()->AddObserver(this); Does this observer need to be removed? Also, this seems like a possible place for the hookup of the observer unless you of course (maybe better?) do it in the feature class instead. https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:213: thread_pipe_manager_->RegisterFeature(BlimpMessage::IME, this); I was hoping we wouldn't have to pass in |this| here. https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:317: if (type == ui::TEXT_INPUT_TYPE_NONE) { Nit: Braces are optional for single-line statements. https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.h:142: // ui::InputMethodObserver overrides. Could this either move to the EngineRenderWidgetFeature, or a new feature specific to IME? https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.h:152: void ShowIme(bool show); Can this be private?
https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:311: void BlimpEngineSession::OnTextInputStateChanged( And this should hoepfully be tested as well. https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:330: void BlimpEngineSession::ShowIme(bool show) { It feels like this should be tested. https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:395: } else if (message->type() == BlimpMessage::IME) { And this should hopefully be tested?
https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/ja... File blimp/client/app/android/java/src/org/chromium/blimp/input/WebInputBox.java (right): https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/ja... blimp/client/app/android/java/src/org/chromium/blimp/input/WebInputBox.java:81: public void showIme() { If this method is only called internally, like hideIme, does it need to be public? https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/ja... blimp/client/app/android/java/src/org/chromium/blimp/input/WebInputBox.java:155: hideIme(); Do you need to set all the Ime options if you're just hiding it? Also, may be put setting the Ime options into a separate function? if (show) { setImeOptions(int inputType); ... } else { hideIme(); } https://codereview.chromium.org/1779673003/diff/1/blimp/client/feature/web_in... File blimp/client/feature/web_input_feature.h (right): https://codereview.chromium.org/1779673003/diff/1/blimp/client/feature/web_in... blimp/client/feature/web_input_feature.h:31: WebInputFeature(); I think the WebInputFeature should be like the compositor and gesture input, since it is also tied to a render widget. If we are getting requests from an old widget, then we shouldn't process them. In fact now that I'm thinking about it, we should have gesture input and text input tied to a navigation. If we navigate within the same site-instance and the render widget remains the same, should we be dropping messages from the previous navigation? +dtrainor For now, we should add web input like other features in EngineRenderWidgetFeature on the engine. On the client, in the BlimpCompositorManager, when a new widget is initialized, in OnRenderWidgetInitialized, reset any state in the WebInputFeature and hide it. Make a note of the active render_widget_id so if you receive requests from the previous widget you can drop them. The BlimpCompositorManager will need the WebInputFeatureDelegate to send the text input events to. You can pass it to the BlimpCompositorManager when the BlimpView builds it. https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:312: const ui::TextInputClient* client) { The RenderWidgetHostView is also a text input client, and I think you should get this call when it becomes focused and should be the one sending these events. So perhaps, save a reference to the TextInputClient here. And when you get the OnShowImeIfNeeded call, make sure that it is the current RenderWidgetHostView, if (!client_) return; if (client_ != web_contents_->GetRenderWidgetHostView()->GetTextInputClient()) return; This way you can add the id for the RenderWidget generated in EngineRenderWidgetFeature to the ime messages sent to the client. When you are receiving messages from the client, discard the message if the RenderWidgetHost has been deleted or it is not the current RenderWidgetHost. And use the text input client from the RenderWidgetHostView to forward the ime event to.
https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/ja... File blimp/client/app/android/java/src/org/chromium/blimp/input/WebInputBox.java (right): https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/ja... blimp/client/app/android/java/src/org/chromium/blimp/input/WebInputBox.java:109: setImeOptions(EditorInfo.IME_FLAG_NO_FULLSCREEN | EditorInfo.IME_FLAG_NO_EXTRACT_UI); Do we want to do this if we're not showing the IME? https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/we... File blimp/client/app/android/web_input_box.cc (right): https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/we... blimp/client/app/android/web_input_box.cc:37: WebInputBox::~WebInputBox() {} Clear the delegate https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/we... blimp/client/app/android/web_input_box.cc:55: std::string str = base::android::ConvertJavaStringToUTF8(env, text); "str" feels a little too 'code lab' to me. Could we rename it? https://codereview.chromium.org/1779673003/diff/1/blimp/client/feature/web_in... File blimp/client/feature/web_input_feature.h (right): https://codereview.chromium.org/1779673003/diff/1/blimp/client/feature/web_in... blimp/client/feature/web_input_feature.h:26: virtual void OnImeRequested(bool show, Javadoc. Should we pull hide out to another method? It feels like input_type and text aren't relevant in that case, so it's a little weird to have both of these actions bundled together here. https://codereview.chromium.org/1779673003/diff/1/blimp/common/proto/ime.proto File blimp/common/proto/ime.proto (right): https://codereview.chromium.org/1779673003/diff/1/blimp/common/proto/ime.prot... blimp/common/proto/ime.proto:26: optional int32 text_input_type = 2; This probably shouldn't be an int. We should roll this up into a proto enum or something similar. https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:332: scoped_ptr<BlimpMessage> blimp_message = CreateBlimpMessage(&ime_message); We probably also want to attach the tab id and the render widget id? https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:344: ime_message->set_text_input_type(client->GetTextInputType()); We should pull this out to some protocol format and write some converters :).
https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/ja... File blimp/client/app/android/java/src/org/chromium/blimp/input/WebInputBox.java (right): https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/ja... blimp/client/app/android/java/src/org/chromium/blimp/input/WebInputBox.java:35: if (actionId == EditorInfo.IME_ACTION_NEXT || actionId == EditorInfo.IME_ACTION_DONE On 2016/03/10 00:57:13, nyquist wrote: > Nit: This if-statement is getting rather large. Should we change it to become a > switch with fall throughs instead? > > Something like: > > switch(actionId) { > case EditorInfo...: // Intentional fall through. > case EditorInfo...: // Intentional fall through. > case EditorInfo...: > // current code > default: > return false; > } > > WDYT? Done. https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/ja... blimp/client/app/android/java/src/org/chromium/blimp/input/WebInputBox.java:81: public void showIme() { On 2016/03/10 07:54:10, Khushal wrote: > If this method is only called internally, like hideIme, does it need to be > public? Done. https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/ja... blimp/client/app/android/java/src/org/chromium/blimp/input/WebInputBox.java:109: setImeOptions(EditorInfo.IME_FLAG_NO_FULLSCREEN | EditorInfo.IME_FLAG_NO_EXTRACT_UI); On 2016/03/14 17:59:41, David Trainor wrote: > Do we want to do this if we're not showing the IME? Done. https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/ja... blimp/client/app/android/java/src/org/chromium/blimp/input/WebInputBox.java:155: hideIme(); On 2016/03/10 07:54:10, Khushal wrote: > Do you need to set all the Ime options if you're just hiding it? > Also, may be put setting the Ime options into a separate function? > > if (show) { > setImeOptions(int inputType); > ... > } else { > hideIme(); > } Done. https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/we... File blimp/client/app/android/web_input_box.cc (right): https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/we... blimp/client/app/android/web_input_box.cc:37: WebInputBox::~WebInputBox() {} On 2016/03/14 17:59:41, David Trainor wrote: > Clear the delegate Done. https://codereview.chromium.org/1779673003/diff/1/blimp/client/app/android/we... blimp/client/app/android/web_input_box.cc:55: std::string str = base::android::ConvertJavaStringToUTF8(env, text); On 2016/03/14 17:59:41, David Trainor wrote: > "str" feels a little too 'code lab' to me. Could we rename it? Done. https://codereview.chromium.org/1779673003/diff/1/blimp/client/feature/web_in... File blimp/client/feature/web_input_feature.h (right): https://codereview.chromium.org/1779673003/diff/1/blimp/client/feature/web_in... blimp/client/feature/web_input_feature.h:20: // floating WebInputBox. On 2016/03/10 00:57:13, nyquist wrote: > I guess WebInputbox isn't know by this class, so maybe just refer to the > WebInputFeatureDelegate here? Done. https://codereview.chromium.org/1779673003/diff/1/blimp/client/feature/web_in... blimp/client/feature/web_input_feature.h:26: virtual void OnImeRequested(bool show, On 2016/03/14 17:59:41, David Trainor wrote: > Javadoc. Should we pull hide out to another method? It feels like input_type > and text aren't relevant in that case, so it's a little weird to have both of > these actions bundled together here. Done. https://codereview.chromium.org/1779673003/diff/1/blimp/client/feature/web_in... blimp/client/feature/web_input_feature.h:42: // Sends text from Ime to the blimp engine. On 2016/03/10 00:57:13, nyquist wrote: > Nit: 'IME' Done. https://codereview.chromium.org/1779673003/diff/1/blimp/common/proto/ime.proto File blimp/common/proto/ime.proto (right): https://codereview.chromium.org/1779673003/diff/1/blimp/common/proto/ime.prot... blimp/common/proto/ime.proto:20: SHOW_TEXT = 3; On 2016/03/10 00:57:13, nyquist wrote: > SHOW_TEXT seems to be used to send text from the client to the Engine. And if we > receive this on the client, we hit a NOTREACHED. > Should this be under Client => Server below? Done. https://codereview.chromium.org/1779673003/diff/1/blimp/common/proto/ime.prot... blimp/common/proto/ime.proto:26: optional int32 text_input_type = 2; On 2016/03/14 17:59:42, David Trainor wrote: > This probably shouldn't be an int. We should roll this up into a proto enum or > something similar. Done. https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:175: window_tree_host_->GetInputMethod()->AddObserver(this); On 2016/03/10 00:57:13, nyquist wrote: > Does this observer need to be removed? Also, this seems like a possible place > for the hookup of the observer unless you of course (maybe better?) do it in the > feature class instead. Done. https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:213: thread_pipe_manager_->RegisterFeature(BlimpMessage::IME, this); On 2016/03/10 00:57:13, nyquist wrote: > I was hoping we wouldn't have to pass in |this| here. Done. https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:332: scoped_ptr<BlimpMessage> blimp_message = CreateBlimpMessage(&ime_message); On 2016/03/14 17:59:42, David Trainor wrote: > We probably also want to attach the tab id and the render widget id? Done. https://codereview.chromium.org/1779673003/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:344: ime_message->set_text_input_type(client->GetTextInputType()); On 2016/03/14 17:59:42, David Trainor wrote: > We should pull this out to some protocol format and write some converters :). Done.
Looks great. May be add some tests for the EngineRenderWidgetFeature changes. https://codereview.chromium.org/1779673003/diff/40001/blimp/client/app/androi... File blimp/client/app/android/blimp_compositor_manager_android.h (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/client/app/androi... blimp/client/app/android/blimp_compositor_manager_android.h:52: void GenerateLayerTreeSettings(cc::LayerTreeSettings* settings) override; Thanks. :) https://codereview.chromium.org/1779673003/diff/40001/blimp/client/app/androi... File blimp/client/app/android/web_input_box.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/client/app/androi... blimp/client/app/android/web_input_box.cc:38: web_input_feature_->SetDelegate(NULL); nit: I think we use nullptr in the code now. https://codereview.chromium.org/1779673003/diff/40001/blimp/client/feature/we... File blimp/client/feature/web_input_feature.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/client/feature/we... blimp/client/feature/web_input_feature.cc:47: DCHECK(delegate_ != NULL) << "WebInputFeatureDelegate not set"; nit: nullptr. https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/blim... File blimp/common/proto/blimp_conversions.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/blim... blimp/common/proto/blimp_conversions.cc:80: default: Why add default here? The good thing about handling all the cases is that if someone adds a new value to the enum but doesn't add it to the conversions method it would give a compile time error. https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/blim... File blimp/common/proto/blimp_conversions.h (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/blim... blimp/common/proto/blimp_conversions.h:14: BLIMP_COMMON_EXPORT ui::TextInputType TextInputTypeFromProto( Could you add unit tests for these? You can take a look at the cc code for examples of tests for such conversions. https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... File blimp/engine/feature/engine_render_widget_feature.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature.cc:128: const ui::TextInputClient* client) { DCHECK the host and client, since you're going to access these later. https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature.cc:223: InsertTextFromIME(render_widget_host->GetView()->GetTextInputClient(), View can be null. Check that as well and drop the message in that case. https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:376: web_contents_->GetRenderWidgetHostView()->GetRenderWidgetHost(), RenderWidgetHostView can be null. Check that before proceeding further. https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:377: client); Do you need to pass the client in this method? Looks like EngineRenderWidgetFeature doesn't need it. https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:388: window_tree_host_->GetInputMethod()->GetTextInputClient()); The text input client can also be null. If the expectation is that it shouldn't be when we get this call, then DCHECK it. Otherwise, just return.
dtrainor@chromium.org changed reviewers: + haibinlu@chromium.org, wez@chromium.org
+wez and haibin for the network and engine reviews.
https://codereview.chromium.org/1779673003/diff/40001/blimp/client/feature/we... File blimp/client/feature/web_input_feature.h (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/client/feature/we... blimp/client/feature/web_input_feature.h:21: class BLIMP_CLIENT_EXPORT WebInputFeature : public BlimpMessageProcessor { ImeFeature to be consistent with proto nameing? https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/blim... File blimp/common/proto/blimp_conversions.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/blim... blimp/common/proto/blimp_conversions.cc:81: return ImeMessage_InputType_TEXT; second this. consider using NOTREACHED() << "Unknown type"? https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/blim... File blimp/common/proto/blimp_conversions.h (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/blim... blimp/common/proto/blimp_conversions.h:5: #ifndef BLIMP_COMMON_PROTO_BLIMP_CONVERSIONS_H_ we have a blimp/net/input_message_converter.h. Can this be merged into that file? If a separate file is needed, name it text_input_type_converter instead of a generic blimp_conversion. https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/ime.... File blimp/common/proto/ime.proto (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/ime.... blimp/common/proto/ime.proto:43: SHOW_TEXT = 3; Is this SHOW_TEXT or INSERT_TEXT as in the feature implementation? https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/ime.... blimp/common/proto/ime.proto:49: optional int32 render_widget_id = 4; nit. put render_widget_id as the first field. https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... File blimp/engine/feature/engine_render_widget_feature.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature.cc:125: void EngineRenderWidgetFeature::SendShowImeRequest( add cases to unit tests https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... File blimp/engine/feature/engine_render_widget_feature.h (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature.h:90: // Sends an ImeMessage to show/hide IME. Notifies the client to show/hide IME. https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature.h:149: void InsertTextFromIME(ui::TextInputClient* client, std::string text); add method document https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:373: if (type == ui::TEXT_INPUT_TYPE_NONE) does ui::TEXT_INPUT_TYPE_NONE mean text input is out of focus? https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:378: } what happens to other types? shall you just show the IME?
https://codereview.chromium.org/1779673003/diff/40001/blimp/client/feature/we... File blimp/client/feature/web_input_feature.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/client/feature/we... blimp/client/feature/web_input_feature.cc:50: tab_id_ = message->target_tab_id(); Should we ever clear these? If so, what's the contract between the calls to the delegates and the SendImeTextToEngine()? (ie: we should probably explain the underlying behavior of those methods in the header comments) https://codereview.chromium.org/1779673003/diff/40001/blimp/client/feature/we... File blimp/client/feature/web_input_feature.h (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/client/feature/we... blimp/client/feature/web_input_feature.h:26: virtual void OnShowImeRequested(int input_type, Change to be ui::TextInputType? https://codereview.chromium.org/1779673003/diff/40001/blimp/client/feature/we... blimp/client/feature/web_input_feature.h:52: int tab_id_; Add comments here. What are these and why do we need these in this class? When are they set/cleared? https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/blim... File blimp/common/proto/blimp_conversions.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/blim... blimp/common/proto/blimp_conversions.cc:43: } What about unknown? What happens if the client and server mismatch? https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/ime.... File blimp/common/proto/ime.proto (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/ime.... blimp/common/proto/ime.proto:43: SHOW_TEXT = 3; On 2016/03/17 18:59:46, haibinlu wrote: > Is this SHOW_TEXT or INSERT_TEXT as in the feature implementation? Maybe SET_TEXT? https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... File blimp/engine/feature/engine_render_widget_feature.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature.cc:237: // Clear out any existing text first and newline is too early https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... File blimp/engine/feature/engine_render_widget_feature.h (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature.h:96: const ui::TextInputClient* client); Do we need the client for hide? https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:366: // - the TextInputType of the TextInputClient changes Add a todo to propagate the new type to the client IMO. https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:372: // Hide IME if text input is out of focus . at the end.
shaktisahu@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/1779673003/diff/40001/blimp/client/app/androi... File blimp/client/app/android/web_input_box.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/client/app/androi... blimp/client/app/android/web_input_box.cc:38: web_input_feature_->SetDelegate(NULL); On 2016/03/17 10:02:45, Khushal wrote: > nit: I think we use nullptr in the code now. Done. https://codereview.chromium.org/1779673003/diff/40001/blimp/client/feature/we... File blimp/client/feature/web_input_feature.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/client/feature/we... blimp/client/feature/web_input_feature.cc:47: DCHECK(delegate_ != NULL) << "WebInputFeatureDelegate not set"; On 2016/03/17 10:02:45, Khushal wrote: > nit: nullptr. Done. https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/blim... File blimp/common/proto/blimp_conversions.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/blim... blimp/common/proto/blimp_conversions.cc:80: default: On 2016/03/17 10:02:45, Khushal wrote: > Why add default here? The good thing about handling all the cases is that if > someone adds a new value to the enum but doesn't add it to the conversions > method it would give a compile time error. Done. https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/blim... blimp/common/proto/blimp_conversions.cc:81: return ImeMessage_InputType_TEXT; On 2016/03/17 18:59:46, haibinlu wrote: > second this. consider using NOTREACHED() << "Unknown type"? Done. https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/blim... blimp/common/proto/blimp_conversions.cc:81: return ImeMessage_InputType_TEXT; On 2016/03/17 18:59:46, haibinlu wrote: > second this. consider using NOTREACHED() << "Unknown type"? Done. https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/blim... File blimp/common/proto/blimp_conversions.h (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/blim... blimp/common/proto/blimp_conversions.h:5: #ifndef BLIMP_COMMON_PROTO_BLIMP_CONVERSIONS_H_ On 2016/03/17 18:59:46, haibinlu wrote: > we have a blimp/net/input_message_converter.h. Can this be merged into that > file? If a separate file is needed, name it text_input_type_converter instead of > a generic blimp_conversion. Done. https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/blim... blimp/common/proto/blimp_conversions.h:14: BLIMP_COMMON_EXPORT ui::TextInputType TextInputTypeFromProto( On 2016/03/17 10:02:45, Khushal wrote: > Could you add unit tests for these? You can take a look at the cc code for > examples of tests for such conversions. Done. https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/ime.... File blimp/common/proto/ime.proto (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/ime.... blimp/common/proto/ime.proto:43: SHOW_TEXT = 3; On 2016/03/17 21:53:29, David Trainor wrote: > On 2016/03/17 18:59:46, haibinlu wrote: > > Is this SHOW_TEXT or INSERT_TEXT as in the feature implementation? > > Maybe SET_TEXT? Done. https://codereview.chromium.org/1779673003/diff/40001/blimp/common/proto/ime.... blimp/common/proto/ime.proto:43: SHOW_TEXT = 3; On 2016/03/17 18:59:46, haibinlu wrote: > Is this SHOW_TEXT or INSERT_TEXT as in the feature implementation? Acknowledged. https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... File blimp/engine/feature/engine_render_widget_feature.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature.cc:125: void EngineRenderWidgetFeature::SendShowImeRequest( On 2016/03/17 18:59:46, haibinlu wrote: > add cases to unit tests Done. https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature.cc:223: InsertTextFromIME(render_widget_host->GetView()->GetTextInputClient(), On 2016/03/17 10:02:45, Khushal wrote: > View can be null. Check that as well and drop the message in that case. Done. https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... File blimp/engine/feature/engine_render_widget_feature.h (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature.h:90: // Sends an ImeMessage to show/hide IME. On 2016/03/17 18:59:46, haibinlu wrote: > Notifies the client to show/hide IME. Done. https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature.h:90: // Sends an ImeMessage to show/hide IME. On 2016/03/17 18:59:46, haibinlu wrote: > Notifies the client to show/hide IME. Done. https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature.h:149: void InsertTextFromIME(ui::TextInputClient* client, std::string text); On 2016/03/17 18:59:46, haibinlu wrote: > add method document Done. https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature.h:149: void InsertTextFromIME(ui::TextInputClient* client, std::string text); On 2016/03/17 18:59:46, haibinlu wrote: > add method document Done. https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:373: if (type == ui::TEXT_INPUT_TYPE_NONE) On 2016/03/17 18:59:46, haibinlu wrote: > does ui::TEXT_INPUT_TYPE_NONE mean text input is out of focus? Yes, text input type changes to ui::TEXT_INPUT_TYPE_NONE when the text input goes out of focus. https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:378: } On 2016/03/17 18:59:46, haibinlu wrote: > what happens to other types? shall you just show the IME? For other text input types, I would receive a call to OnShowImeIFNeeded(), where I show the IME. And this is the only place to hide the IME, since I don't receive the calls OnFocus(), OnBlur() at all.
https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:373: if (type == ui::TEXT_INPUT_TYPE_NONE) On 2016/03/18 19:08:06, shaktisahu wrote: > On 2016/03/17 18:59:46, haibinlu wrote: > > does ui::TEXT_INPUT_TYPE_NONE mean text input is out of focus? > > Yes, text input type changes to ui::TEXT_INPUT_TYPE_NONE when the text input > goes out of focus. can you add this to the comment? https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:378: } On 2016/03/18 19:08:06, shaktisahu wrote: > On 2016/03/17 18:59:46, haibinlu wrote: > > what happens to other types? shall you just show the IME? > > For other text input types, I would receive a call to OnShowImeIFNeeded(), where > I show the IME. > And this is the only place to hide the IME, since I don't receive the calls > OnFocus(), OnBlur() at all. Does OnShowImeIFNeeded cover more cases for showing IME than other text input types here? Also add comment about why other types are not handled here. https://codereview.chromium.org/1779673003/diff/60001/blimp/engine/feature/en... File blimp/engine/feature/engine_render_widget_feature.cc (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature.cc:224: InsertTextFromIME(render_widget_host->GetView()->GetTextInputClient(), SetTextFromIME to be consistent with ImeMessage::SET_TEXT
Left a load of comments but they are mostly wording/naming & style nits - generally this is shaping up nicely. :) https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... File blimp/client/app/android/blimp_compositor_manager_android.h (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... blimp/client/app/android/blimp_compositor_manager_android.h:52: void GenerateLayerTreeSettings(cc::LayerTreeSettings* settings) override; nit: Is this inherited from some other CL..? https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... File blimp/client/app/android/web_input_box.cc (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... blimp/client/app/android/web_input_box.cc:21: return reinterpret_cast<intptr_t>( Why are you reinterpret_cast<>ing to intptr_t rather than to jlong, since that is the return type you actually need? https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... blimp/client/app/android/web_input_box.cc:50: env, java_obj_.obj(), input_type, nit: It looks a little strange to have |env| need to be supplied to the ctor, but then grabbed implicitly from the thread here - AFAICT the |env| is always per-thread, so can we avoid needing to pass it to the ctor at all..? Similarly, it is strange for OnShowImeRequested() to grab the |env| implicitly while OnImeTestEntered() requires it to be passed explicitly. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... File blimp/client/app/android/web_input_box.h (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... blimp/client/app/android/web_input_box.h:25: ImeFeature* ime_feature); nit: Need a comment to clarify the lifetime expectations of bare-pointer parameters - i.e. presumably the caller is expected to delete WebInputBox before tearing down the ImeFeature? https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... File blimp/client/feature/ime_feature.cc (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:16: ImeFeature::ImeFeature() {} As noted in the header, there are POD members of this class that need initializing; you can do it in the header directly, otherwise you have to do it here. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:22: outgoing_message_processor_ = std::move(processor); nit: Since this is a "simple" setter you could implement it in-line in the header. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:25: void ImeFeature::SetDelegate(ImeFeatureDelegate* delegate) { Similarly, since this is a simple setter, it could be implemented in the header directly. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:29: void ImeFeature::SendImeTextToEngine(const std::string& text) { I'd suggest DCHECK()ing that the tab & widget Ids are valid at this point. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:44: DCHECK(message->type() == BlimpMessage::IME); As noted below, if the Engine sends bad data, callback.Run(SOME_ERROR_CODE). https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:46: DCHECK(delegate_) << "ImeFeatureDelegate not set"; nit: The message following the DCHECK seems redundant? https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:50: render_widget_id_ = ime_message.render_widget_id(); When you receive a hide request you're basically switching off the IME, so instead of caching the tab & widget Ids, surely you should clear them back to invalid values? https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:53: case ImeMessage::SHOW_IME: Suggest adding a check that the tab & widget Ids are invalid if we reach here, before caching them and calling OnShowImeRequested(). The check should Run(SOME_ERROR_CODE) rather than DCHECK or CHECK, since we're processing protocol data from a remote Engine that has sent something bogus, implying it may be compromise - we therefore want to disconnect rather than continue! https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:60: delegate_->OnHideImeRequested(); Suggesting DCHECKing that the tab & widget Ids are valid here, before clearing them before calling on to delegate. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:62: case ImeMessage::SET_TEXT: nit: No need for a separate code block for this and UNKNOWN; you can just have case SET_TEXT: case UNKNOWN: NOTREACHED(); callback.Run(SOME_KIND_OF_ERROR); break; You want to return SOME_KIND_OF_ERROR in this case so that we'll terminate the connection to the Engine, since it has actually sent bogus protocol data, suggesting that it may be compromised. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... File blimp/client/feature/ime_feature.h (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.h:26: // |render_widget_id_|. nit: What happens if the Engine sends a new request while the previous one is outstanding? https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.h:27: class BLIMP_CLIENT_EXPORT ImeFeature : public BlimpMessageProcessor { This should be ImeClientFeature, since there will be a corresponding IME feature object for the Engine. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.h:29: // A delegate to be notified of text input events. nit: I think you mean "text input requests"? https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.h:46: void SetDelegate(ImeFeatureDelegate* delegate); Could/should this be a constructor parameter? nit: Be explicit re the lifetime expectation of the |delegate|, e.g. it must remain valid until after the last message has been passed to ImeFeature, or until ImeFeature is actually deleted? Also, since this is a simple setter it could be named in unix_style rather than CamelCase, and since the preceding setter is unix_style, this should also be, to match. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.h:49: void SendImeTextToEngine(const std::string& text); This class doesn't really have a concept of the "Engine", just of incoming and outgoing messages; you might rename this OnImeTextEntered() or just SendImeText(). https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.h:56: ImeFeatureDelegate* delegate_; nit: You can in-line initialize this to nullptr. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.h:65: int render_widget_id_; nit: Similarly, suggest inline initializing these to invalid values. https://codereview.chromium.org/1779673003/diff/60001/blimp/common/create_bli... File blimp/common/create_blimp_message.cc (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/common/create_bli... blimp/common/create_blimp_message.cc:58: DCHECK(ime_message); nit: Why do we DCHECK the out-parameter here & in the other CreateBlimpMessage impls? We de-reference it later in the same function, to assign to it, so DCHECK is redundant. https://codereview.chromium.org/1779673003/diff/60001/blimp/common/proto/blim... File blimp/common/proto/blimp_message.proto (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/common/proto/blim... blimp/common/proto/blimp_message.proto:45: IME = 7; What is the rationale for adding a top-level ImeMessage rather than folding these messages under INPUT? If we keep them separate then can we call it TEXT_INPUT/TextInputMessage? Without any surrounding context, IME's meaning is not immediately obvious!
Hi Wez and Haibin, Please review the suggested changes. Thanks Shakti https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1779673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:378: } On 2016/03/18 19:53:49, haibinlu wrote: > On 2016/03/18 19:08:06, shaktisahu wrote: > > On 2016/03/17 18:59:46, haibinlu wrote: > > > what happens to other types? shall you just show the IME? > > > > For other text input types, I would receive a call to OnShowImeIFNeeded(), > where > > I show the IME. > > And this is the only place to hide the IME, since I don't receive the calls > > OnFocus(), OnBlur() at all. > > Does OnShowImeIFNeeded cover more cases for showing IME than other text input > types here? Also add comment about why other types are not handled here. I would think so, although the api description for OnShowImeIFNeeded is not very clear. OnShowImeIFNeeded callback seems a more appropriate place to show IME. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... File blimp/client/app/android/blimp_compositor_manager_android.h (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... blimp/client/app/android/blimp_compositor_manager_android.h:52: void GenerateLayerTreeSettings(cc::LayerTreeSettings* settings) override; On 2016/03/18 20:51:27, Wez wrote: > nit: Is this inherited from some other CL..? Yes. It is related to Khushal's another change which gives a complilation error. I checked with him. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... File blimp/client/app/android/web_input_box.cc (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... blimp/client/app/android/web_input_box.cc:21: return reinterpret_cast<intptr_t>( On 2016/03/18 20:51:27, Wez wrote: > Why are you reinterpret_cast<>ing to intptr_t rather than to jlong, since that > is the return type you actually need? reinterpret_cast to intptr_t is quite standard in JNI and is used in many places in the code. I am not sure of the reason, but apparently complier might give a warning on 32 bit machines if jlong is used instead. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... blimp/client/app/android/web_input_box.cc:50: env, java_obj_.obj(), input_type, On 2016/03/18 20:51:27, Wez wrote: > nit: It looks a little strange to have |env| need to be supplied to the ctor, > but then grabbed implicitly from the thread here - AFAICT the |env| is always > per-thread, so can we avoid needing to pass it to the ctor at all..? > > Similarly, it is strange for OnShowImeRequested() to grab the |env| implicitly > while OnImeTestEntered() requires it to be passed explicitly. OnShowImeRequested() is a native-to-Java call whereas OnImeTextEntered() is a Java-to-native call. This is similar to the way most of the other native-to-Java calls in the other feature implementations. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... File blimp/client/app/android/web_input_box.h (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... blimp/client/app/android/web_input_box.h:25: ImeFeature* ime_feature); On 2016/03/18 20:51:27, Wez wrote: > nit: Need a comment to clarify the lifetime expectations of bare-pointer > parameters - i.e. presumably the caller is expected to delete WebInputBox before > tearing down the ImeFeature? Comments added to the header file. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... blimp/client/app/android/web_input_box.h:25: ImeFeature* ime_feature); On 2016/03/18 20:51:27, Wez wrote: > nit: Need a comment to clarify the lifetime expectations of bare-pointer > parameters - i.e. presumably the caller is expected to delete WebInputBox before > tearing down the ImeFeature? Acknowledged. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... File blimp/client/feature/ime_feature.cc (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:16: ImeFeature::ImeFeature() {} On 2016/03/18 20:51:28, Wez wrote: > As noted in the header, there are POD members of this class that need > initializing; you can do it in the header directly, otherwise you have to do it > here. Done. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:22: outgoing_message_processor_ = std::move(processor); On 2016/03/18 20:51:27, Wez wrote: > nit: Since this is a "simple" setter you could implement it in-line in the > header. Done. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:25: void ImeFeature::SetDelegate(ImeFeatureDelegate* delegate) { On 2016/03/18 20:51:28, Wez wrote: > Similarly, since this is a simple setter, it could be implemented in the header > directly. Done. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:29: void ImeFeature::SendImeTextToEngine(const std::string& text) { On 2016/03/18 20:51:28, Wez wrote: > I'd suggest DCHECK()ing that the tab & widget Ids are valid at this point. Done. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:44: DCHECK(message->type() == BlimpMessage::IME); On 2016/03/18 20:51:27, Wez wrote: > As noted below, if the Engine sends bad data, callback.Run(SOME_ERROR_CODE). Done. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:46: DCHECK(delegate_) << "ImeFeatureDelegate not set"; On 2016/03/18 20:51:27, Wez wrote: > nit: The message following the DCHECK seems redundant? Done. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:50: render_widget_id_ = ime_message.render_widget_id(); On 2016/03/18 20:51:28, Wez wrote: > When you receive a hide request you're basically switching off the IME, so > instead of caching the tab & widget Ids, surely you should clear them back to > invalid values? Done. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:53: case ImeMessage::SHOW_IME: On 2016/03/18 20:51:28, Wez wrote: > Suggest adding a check that the tab & widget Ids are invalid if we reach here, > before caching them and calling OnShowImeRequested(). The check should > Run(SOME_ERROR_CODE) rather than DCHECK or CHECK, since we're processing > protocol data from a remote Engine that has sent something bogus, implying it > may be compromise - we therefore want to disconnect rather than continue! Done. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:60: delegate_->OnHideImeRequested(); On 2016/03/18 20:51:27, Wez wrote: > Suggesting DCHECKing that the tab & widget Ids are valid here, before clearing > them before calling on to delegate. Actually, tab and widget ids can be invalid here. It happens when the page is loaded, and text input is out of focus. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:62: case ImeMessage::SET_TEXT: On 2016/03/18 20:51:27, Wez wrote: > nit: No need for a separate code block for this and UNKNOWN; you can just have > > case SET_TEXT: > case UNKNOWN: > NOTREACHED(); > callback.Run(SOME_KIND_OF_ERROR); > break; > > You want to return SOME_KIND_OF_ERROR in this case so that we'll terminate the > connection to the Engine, since it has actually sent bogus protocol data, > suggesting that it may be compromised. Done. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... File blimp/client/feature/ime_feature.h (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.h:27: class BLIMP_CLIENT_EXPORT ImeFeature : public BlimpMessageProcessor { On 2016/03/18 20:51:28, Wez wrote: > This should be ImeClientFeature, since there will be a corresponding IME feature > object for the Engine. Hmm, not sure if this sounds good. By the way, on the server side, IME requests are handled in EngineRenderWidgetFeature. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.h:29: // A delegate to be notified of text input events. On 2016/03/18 20:51:28, Wez wrote: > nit: I think you mean "text input requests"? Done. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.h:46: void SetDelegate(ImeFeatureDelegate* delegate); On 2016/03/18 20:51:28, Wez wrote: > Could/should this be a constructor parameter? > > nit: Be explicit re the lifetime expectation of the |delegate|, e.g. it must > remain valid until after the last message has been passed to ImeFeature, or > until ImeFeature is actually deleted? > > Also, since this is a simple setter it could be named in unix_style rather than > CamelCase, and since the preceding setter is unix_style, this should also be, to > match. Can't be a constructor param since ImeFeature outlives the |delegate|. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.h:49: void SendImeTextToEngine(const std::string& text); On 2016/03/18 20:51:28, Wez wrote: > This class doesn't really have a concept of the "Engine", just of incoming and > outgoing messages; you might rename this OnImeTextEntered() or just > SendImeText(). Done. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.h:56: ImeFeatureDelegate* delegate_; On 2016/03/18 20:51:28, Wez wrote: > nit: You can in-line initialize this to nullptr. Done. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.h:65: int render_widget_id_; On 2016/03/18 20:51:28, Wez wrote: > nit: Similarly, suggest inline initializing these to invalid values. Done. https://codereview.chromium.org/1779673003/diff/60001/blimp/common/proto/blim... File blimp/common/proto/blimp_message.proto (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/common/proto/blim... blimp/common/proto/blimp_message.proto:45: IME = 7; On 2016/03/18 20:51:28, Wez wrote: > What is the rationale for adding a top-level ImeMessage rather than folding > these messages under INPUT? > > If we keep them separate then can we call it TEXT_INPUT/TextInputMessage? > Without any surrounding context, IME's meaning is not immediately obvious! IME message is both way (client<-->server) whereas INPUT messages are one way. I can change the name. There are few names I have used so far : BlimpMessage::IME/ImeMessage, ImeFeature, WebInputBox. Please suggest what seems more appropriate. https://codereview.chromium.org/1779673003/diff/60001/blimp/engine/feature/en... File blimp/engine/feature/engine_render_widget_feature.cc (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature.cc:224: InsertTextFromIME(render_widget_host->GetView()->GetTextInputClient(), On 2016/03/18 19:53:49, haibinlu wrote: > SetTextFromIME to be consistent with ImeMessage::SET_TEXT Done.
Hi Wez and Haibin, Please review the suggested changes. Thanks Shakti
lgtm https://codereview.chromium.org/1779673003/diff/100001/blimp/net/input_messag... File blimp/net/input_message_converter.h (right): https://codereview.chromium.org/1779673003/diff/100001/blimp/net/input_messag... blimp/net/input_message_converter.h:5: #ifndef BLIMP_NET_INPUT_MESSAGE_CONVERTER_H_ this class, together with net/input_message_generator.h, shall be moved to blimp/common. not sure why they were put under "net" originally. you can do it a different cl.
https://codereview.chromium.org/1779673003/diff/100001/blimp/net/DEPS File blimp/net/DEPS (right): https://codereview.chromium.org/1779673003/diff/100001/blimp/net/DEPS#newcode5 blimp/net/DEPS:5: "+ui/base", once input_message_converter is moved to blimp/common, no need for blimp/net to depend on "ui/base".
https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... File blimp/client/app/android/web_input_box.cc (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... blimp/client/app/android/web_input_box.cc:21: return reinterpret_cast<intptr_t>( On 2016/03/22 19:44:19, shaktisahu wrote: > On 2016/03/18 20:51:27, Wez wrote: > > Why are you reinterpret_cast<>ing to intptr_t rather than to jlong, since that > > is the return type you actually need? > > reinterpret_cast to intptr_t is quite standard in JNI and is used in many places > in the code. I am not sure of the reason, but apparently complier might give a > warning on 32 bit machines if jlong is used instead. Huh... apparently it's about ensuring that the conversion is well-defined (since you're going from an effectively unsigned 32- or 64-bit pointer to a signed 64-bit value - in the 32-bit case I think it can be ambiguous whether to cast to signed and then sign-extend to 64-bit, versus extend to 64-bit and then switch to signed. WebRTC has a nice helper for this with a helpful comment; shame Chromium doesn't. :( Anyway... Ack! https://codereview.chromium.org/1779673003/diff/60001/blimp/client/app/androi... blimp/client/app/android/web_input_box.cc:50: env, java_obj_.obj(), input_type, On 2016/03/22 19:44:19, shaktisahu wrote: > On 2016/03/18 20:51:27, Wez wrote: > > nit: It looks a little strange to have |env| need to be supplied to the ctor, > > but then grabbed implicitly from the thread here - AFAICT the |env| is always > > per-thread, so can we avoid needing to pass it to the ctor at all..? > > > > Similarly, it is strange for OnShowImeRequested() to grab the |env| implicitly > > while OnImeTestEntered() requires it to be passed explicitly. > > OnShowImeRequested() is a native-to-Java call whereas OnImeTextEntered() is a > Java-to-native call. This is similar to the way most of the other native-to-Java > calls in the other feature implementations. Acknowledged. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... File blimp/client/feature/ime_feature.cc (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:44: DCHECK(message->type() == BlimpMessage::IME); On 2016/03/22 19:44:19, shaktisahu wrote: > On 2016/03/18 20:51:27, Wez wrote: > > As noted below, if the Engine sends bad data, callback.Run(SOME_ERROR_CODE). > > Done. Ah, sorry; forgot that the demultiplexer should never let us reach here! nit: use DCHECK_EQ(BlimpMessage::IME, message->type()) https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:53: case ImeMessage::SHOW_IME: On 2016/03/22 19:44:19, shaktisahu wrote: > On 2016/03/18 20:51:28, Wez wrote: > > Suggest adding a check that the tab & widget Ids are invalid if we reach here, > > before caching them and calling OnShowImeRequested(). The check should > > Run(SOME_ERROR_CODE) rather than DCHECK or CHECK, since we're processing > > protocol data from a remote Engine that has sent something bogus, implying it > > may be compromise - we therefore want to disconnect rather than continue! > > Done. Looks like you've added a check for the sender specifying valid-looking Ids, but I think you also want a check that the IME is not already shown, i.e. that |tab_id_| and |render_widget_id_| are not already set, here. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:60: delegate_->OnHideImeRequested(); On 2016/03/22 19:44:19, shaktisahu wrote: > On 2016/03/18 20:51:27, Wez wrote: > > Suggesting DCHECKing that the tab & widget Ids are valid here, before clearing > > them before calling on to delegate. > > Actually, tab and widget ids can be invalid here. It happens when the page is > loaded, and text input is out of focus. So if the tab_id_ and render_widget_id_ are invalid should we also skip calling out to the delegate, since it presumably isn't showing anything? https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... File blimp/client/feature/ime_feature.h (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.h:46: void SetDelegate(ImeFeatureDelegate* delegate); On 2016/03/22 19:44:20, shaktisahu wrote: > On 2016/03/18 20:51:28, Wez wrote: > > Could/should this be a constructor parameter? > > > > nit: Be explicit re the lifetime expectation of the |delegate|, e.g. it must > > remain valid until after the last message has been passed to ImeFeature, or > > until ImeFeature is actually deleted? > > > > Also, since this is a simple setter it could be named in unix_style rather > than > > CamelCase, and since the preceding setter is unix_style, this should also be, > to > > match. > > Can't be a constructor param since ImeFeature outlives the |delegate|. IIUC you're saying it's valid to pass a NULL |delegate| parameter - in which case best to mention that in the comment, e.g. "Passing a null |delegate| causes IME events to be ignored." https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.h:56: ImeFeatureDelegate* delegate_; On 2016/03/22 19:44:19, shaktisahu wrote: > On 2016/03/18 20:51:28, Wez wrote: > > nit: You can in-line initialize this to nullptr. > > Done. Doesn't look done! https://codereview.chromium.org/1779673003/diff/60001/blimp/common/create_bli... File blimp/common/create_blimp_message.cc (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/common/create_bli... blimp/common/create_blimp_message.cc:58: DCHECK(ime_message); On 2016/03/18 20:51:28, Wez wrote: > nit: Why do we DCHECK the out-parameter here & in the other CreateBlimpMessage > impls? We de-reference it later in the same function, to assign to it, so DCHECK > is redundant. Pingy. https://codereview.chromium.org/1779673003/diff/60001/blimp/common/proto/blim... File blimp/common/proto/blimp_message.proto (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/common/proto/blim... blimp/common/proto/blimp_message.proto:45: IME = 7; On 2016/03/22 19:44:20, shaktisahu wrote: > On 2016/03/18 20:51:28, Wez wrote: > > What is the rationale for adding a top-level ImeMessage rather than folding > > these messages under INPUT? > > > > If we keep them separate then can we call it TEXT_INPUT/TextInputMessage? > > Without any surrounding context, IME's meaning is not immediately obvious! > > IME message is both way (client<-->server) whereas INPUT messages are one way. I > can change the name. There are few names I have used so far : > BlimpMessage::IME/ImeMessage, ImeFeature, WebInputBox. Please suggest what seems > more appropriate. Let's stick w/ this for now & revisit naming if it proves troublesome. https://codereview.chromium.org/1779673003/diff/100001/blimp/client/app/andro... File blimp/client/app/android/web_input_box.h (right): https://codereview.chromium.org/1779673003/diff/100001/blimp/client/app/andro... blimp/client/app/android/web_input_box.h:51: // outlive the WebInputBox. nit: Lifetime comment belongs w/ the constructor, since that's the interface through which |ime_feature_| is received. https://codereview.chromium.org/1779673003/diff/100001/blimp/client/feature/i... File blimp/client/feature/ime_feature.cc (right): https://codereview.chromium.org/1779673003/diff/100001/blimp/client/feature/i... blimp/client/feature/ime_feature.cc:22: DCHECK(tab_id_ >= 0 && render_widget_id_ > 0); nit: Replace these w/ separate: DCHECK_LE(0, tab_id_); DCHECK_LT(0, render_widget_id_); which will print out the actual value if they fire - your current check will print the expression that failed, but not explain which parameter was bad. https://codereview.chromium.org/1779673003/diff/100001/blimp/client/feature/i... blimp/client/feature/ime_feature.cc:44: switch (ime_message.type()) { You haven't checked whether |ime_message| actually has a |type| field yet, so you may get the default value - IIUC that will be the first value listed in the enum, so it'll actually work, but for clarity you should either check has_type() explicitly here, and reject the message if it's missing, or update the ime.proto to have a [ default = ... ] tag on the field, so it's explicit that it'll be UNKNOWN if not present. https://codereview.chromium.org/1779673003/diff/100001/blimp/client/feature/i... blimp/client/feature/ime_feature.cc:46: if (message->target_tab_id() < 0 || ime_message.render_widget_id() <= 0) { Similarly, you haven't checked whether the sender actually provided values at all; integer fields will default to zero so if they only failed to provide target_tab_id then this test could still pass. https://codereview.chromium.org/1779673003/diff/100001/blimp/client/feature/i... File blimp/client/feature/ime_feature.h (right): https://codereview.chromium.org/1779673003/diff/100001/blimp/client/feature/i... blimp/client/feature/ime_feature.h:35: class ImeFeatureDelegate { nit: Either call this ImeFeatureDelegate and move it outside ImeFeature (since it already has a nicely unique name), or name it simply Delegate and leave it here. Having it declared outside ImeFeature would be better if there is any code that needs to pass around ImeFeatureDelegate pointers without knowing the details of the class, but I doubt we have those. https://codereview.chromium.org/1779673003/diff/100001/blimp/common/proto/ime... File blimp/common/proto/ime.proto (right): https://codereview.chromium.org/1779673003/diff/100001/blimp/common/proto/ime... blimp/common/proto/ime.proto:47: optional Type type = 2; See comments in ImeFeature re explicitly checking for presence of fields, OR explicitly specifying default values (e.g. type defaults to UNKNOWN) to make the intended behaviour clear. https://codereview.chromium.org/1779673003/diff/100001/blimp/engine/session/b... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1779673003/diff/100001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.cc:546: bool page_load_completed = progress == 1.0 ? true : false; nit: Why not just completed = (progress == 1.0) https://codereview.chromium.org/1779673003/diff/100001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.cc:554: NavigationMessage* navigation_message; nit: Initialize this, even though it's about to be overwritten. https://codereview.chromium.org/1779673003/diff/100001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.cc:558: NavigationStateChangeMessage* details = Indentation looks wrong here.
Hi Wez, Please review the changes. Thanks Shakti https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... File blimp/client/feature/ime_feature.cc (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:44: DCHECK(message->type() == BlimpMessage::IME); On 2016/03/22 21:43:37, Wez wrote: > On 2016/03/22 19:44:19, shaktisahu wrote: > > On 2016/03/18 20:51:27, Wez wrote: > > > As noted below, if the Engine sends bad data, callback.Run(SOME_ERROR_CODE). > > > > Done. > > Ah, sorry; forgot that the demultiplexer should never let us reach here! > > nit: use DCHECK_EQ(BlimpMessage::IME, message->type()) Done. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:44: DCHECK(message->type() == BlimpMessage::IME); On 2016/03/22 21:43:37, Wez wrote: > On 2016/03/22 19:44:19, shaktisahu wrote: > > On 2016/03/18 20:51:27, Wez wrote: > > > As noted below, if the Engine sends bad data, callback.Run(SOME_ERROR_CODE). > > > > Done. > > Ah, sorry; forgot that the demultiplexer should never let us reach here! > > nit: use DCHECK_EQ(BlimpMessage::IME, message->type()) Done. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:53: case ImeMessage::SHOW_IME: On 2016/03/22 21:43:37, Wez wrote: > On 2016/03/22 19:44:19, shaktisahu wrote: > > On 2016/03/18 20:51:28, Wez wrote: > > > Suggest adding a check that the tab & widget Ids are invalid if we reach > here, > > > before caching them and calling OnShowImeRequested(). The check should > > > Run(SOME_ERROR_CODE) rather than DCHECK or CHECK, since we're processing > > > protocol data from a remote Engine that has sent something bogus, implying > it > > > may be compromise - we therefore want to disconnect rather than continue! > > > > Done. > > Looks like you've added a check for the sender specifying valid-looking Ids, but > I think you also want a check that the IME is not already shown, i.e. that > |tab_id_| and |render_widget_id_| are not already set, here. Actually, there might be a case where we might have the ids already set, IME is showing, user presses back button (which hides the IME) and then tries again for text input. So we can have the valid ids at this point. The ids being set/unset is not a one-to-one mapping with showing the IME or hiding it. https://codereview.chromium.org/1779673003/diff/60001/blimp/client/feature/im... blimp/client/feature/ime_feature.cc:60: delegate_->OnHideImeRequested(); On 2016/03/22 21:43:37, Wez wrote: > On 2016/03/22 19:44:19, shaktisahu wrote: > > On 2016/03/18 20:51:27, Wez wrote: > > > Suggesting DCHECKing that the tab & widget Ids are valid here, before > clearing > > > them before calling on to delegate. > > > > Actually, tab and widget ids can be invalid here. It happens when the page is > > loaded, and text input is out of focus. > > So if the tab_id_ and render_widget_id_ are invalid should we also skip calling > out to the delegate, since it presumably isn't showing anything? Same as above comment. I would still prefer to make a call to hide IME. https://codereview.chromium.org/1779673003/diff/60001/blimp/common/create_bli... File blimp/common/create_blimp_message.cc (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/common/create_bli... blimp/common/create_blimp_message.cc:58: DCHECK(ime_message); On 2016/03/22 21:43:38, Wez wrote: > On 2016/03/18 20:51:28, Wez wrote: > > nit: Why do we DCHECK the out-parameter here & in the other CreateBlimpMessage > > impls? We de-reference it later in the same function, to assign to it, so > DCHECK > > is redundant. > > Pingy. Since ime_message is a pointer to pointer, shouldn't we do a null check before de-referencing its value? https://codereview.chromium.org/1779673003/diff/100001/blimp/client/app/andro... File blimp/client/app/android/web_input_box.h (right): https://codereview.chromium.org/1779673003/diff/100001/blimp/client/app/andro... blimp/client/app/android/web_input_box.h:51: // outlive the WebInputBox. On 2016/03/22 21:43:38, Wez wrote: > nit: Lifetime comment belongs w/ the constructor, since that's the interface > through which |ime_feature_| is received. Hmm.. I would prefer to keep it here, since it provides all the description of the |ime_feature_| at one place. Few other header files also do the same. https://codereview.chromium.org/1779673003/diff/100001/blimp/client/feature/i... File blimp/client/feature/ime_feature.cc (right): https://codereview.chromium.org/1779673003/diff/100001/blimp/client/feature/i... blimp/client/feature/ime_feature.cc:22: DCHECK(tab_id_ >= 0 && render_widget_id_ > 0); On 2016/03/22 21:43:38, Wez wrote: > nit: Replace these w/ separate: > > DCHECK_LE(0, tab_id_); > DCHECK_LT(0, render_widget_id_); > > which will print out the actual value if they fire - your current check will > print the expression that failed, but not explain which parameter was bad. Done. https://codereview.chromium.org/1779673003/diff/100001/blimp/client/feature/i... blimp/client/feature/ime_feature.cc:44: switch (ime_message.type()) { On 2016/03/22 21:43:38, Wez wrote: > You haven't checked whether |ime_message| actually has a |type| field yet, so > you may get the default value - IIUC that will be the first value listed in the > enum, so it'll actually work, but for clarity you should either check has_type() > explicitly here, and reject the message if it's missing, or update the ime.proto > to have a [ default = ... ] tag on the field, so it's explicit that it'll be > UNKNOWN if not present. Ok. I will explicitly add a [default = UNKNOWN] tag in the ime.proto https://codereview.chromium.org/1779673003/diff/100001/blimp/client/feature/i... blimp/client/feature/ime_feature.cc:46: if (message->target_tab_id() < 0 || ime_message.render_widget_id() <= 0) { On 2016/03/22 21:43:38, Wez wrote: > Similarly, you haven't checked whether the sender actually provided values at > all; integer fields will default to zero so if they only failed to provide > target_tab_id then this test could still pass. Done. https://codereview.chromium.org/1779673003/diff/100001/blimp/client/feature/i... File blimp/client/feature/ime_feature.h (right): https://codereview.chromium.org/1779673003/diff/100001/blimp/client/feature/i... blimp/client/feature/ime_feature.h:35: class ImeFeatureDelegate { On 2016/03/22 21:43:38, Wez wrote: > nit: Either call this ImeFeatureDelegate and move it outside ImeFeature (since > it already has a nicely unique name), or name it simply Delegate and leave it > here. > > Having it declared outside ImeFeature would be better if there is any code that > needs to pass around ImeFeatureDelegate pointers without knowing the details of > the class, but I doubt we have those. I think we can't just call it Delegate since it would be implemented by WebInputBox. Moving it to a separate file is a good idea, but I am holding on to it unless required. Also this is in line with other feature delegates such as NavigationFeatureDelegate etc. https://codereview.chromium.org/1779673003/diff/100001/blimp/common/proto/ime... File blimp/common/proto/ime.proto (right): https://codereview.chromium.org/1779673003/diff/100001/blimp/common/proto/ime... blimp/common/proto/ime.proto:47: optional Type type = 2; On 2016/03/22 21:43:38, Wez wrote: > See comments in ImeFeature re explicitly checking for presence of fields, OR > explicitly specifying default values (e.g. type defaults to UNKNOWN) to make the > intended behaviour clear. Done. https://codereview.chromium.org/1779673003/diff/100001/blimp/engine/session/b... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1779673003/diff/100001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.cc:546: bool page_load_completed = progress == 1.0 ? true : false; On 2016/03/22 21:43:38, Wez wrote: > nit: Why not just completed = (progress == 1.0) Not from my CL. https://codereview.chromium.org/1779673003/diff/100001/blimp/net/DEPS File blimp/net/DEPS (right): https://codereview.chromium.org/1779673003/diff/100001/blimp/net/DEPS#newcode5 blimp/net/DEPS:5: "+ui/base", On 2016/03/22 20:00:19, haibinlu wrote: > once input_message_converter is moved to blimp/common, no need for blimp/net to > depend on "ui/base". Acknowledged.
https://codereview.chromium.org/1779673003/diff/60001/blimp/common/create_bli... File blimp/common/create_blimp_message.cc (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/common/create_bli... blimp/common/create_blimp_message.cc:58: DCHECK(ime_message); On 2016/03/22 23:45:42, shaktisahu wrote: > On 2016/03/22 21:43:38, Wez wrote: > > On 2016/03/18 20:51:28, Wez wrote: > > > nit: Why do we DCHECK the out-parameter here & in the other > CreateBlimpMessage > > > impls? We de-reference it later in the same function, to assign to it, so > > DCHECK > > > is redundant. > > > > Pingy. > > Since ime_message is a pointer to pointer, shouldn't we do a null check before > de-referencing its value? DCHECK makes sense where you are given a pointer that you are not immediately going to dereference - then rather than crashing when you dereference it and not knowing why you got a null pointer, you will get a nice DCHECK at the point where the caller passed you something invalid. In this case you are dereferencing it on line #62 - if it's null then the app will crash, which is all that DCHECK will do - so all the DCHECK is doing is moving the crash earlier in this function, which isn't useful. https://codereview.chromium.org/1779673003/diff/100001/blimp/client/app/andro... File blimp/client/app/android/web_input_box.h (right): https://codereview.chromium.org/1779673003/diff/100001/blimp/client/app/andro... blimp/client/app/android/web_input_box.h:51: // outlive the WebInputBox. On 2016/03/22 23:45:42, shaktisahu wrote: > On 2016/03/22 21:43:38, Wez wrote: > > nit: Lifetime comment belongs w/ the constructor, since that's the interface > > through which |ime_feature_| is received. > > Hmm.. I would prefer to keep it here, since it provides all the description of > the |ime_feature_| at one place. Few other header files also do the same. I disagree; the point at which constraints need to be documented is the interface between the caller and the implementation, which in this case is the constructor. In general comments on private class members are there to help you understand the implementation (how it works), while the comments in the public section are on the interface (how to use it). I'd also note that the comment shouldn't refer to BlimpClientSession in talking about the lifetime, since WebInputBox doesn't really "know about" that class; all WebInputBox cares about is that the delegate outlives it. https://codereview.chromium.org/1779673003/diff/100001/blimp/client/feature/i... File blimp/client/feature/ime_feature.h (right): https://codereview.chromium.org/1779673003/diff/100001/blimp/client/feature/i... blimp/client/feature/ime_feature.h:35: class ImeFeatureDelegate { On 2016/03/22 23:45:42, shaktisahu wrote: > On 2016/03/22 21:43:38, Wez wrote: > > nit: Either call this ImeFeatureDelegate and move it outside ImeFeature (since > > it already has a nicely unique name), or name it simply Delegate and leave it > > here. > > > > Having it declared outside ImeFeature would be better if there is any code > that > > needs to pass around ImeFeatureDelegate pointers without knowing the details > of > > the class, but I doubt we have those. > > I think we can't just call it Delegate since it would be implemented by > WebInputBox. I don't understand why the name would make any difference there - it's already prefixed with ImeFeature:: > Moving it to a separate file is a good idea, but I am holding on > to it unless required. I'm not suggesting moving it to a different file, just moving it to be blimp::client::ImeFeatureDelegate - it would be declared immediately before ImeFeature in that case. (We don't have a one-class-per-header requirement). > Also this is in line with other feature delegates such as > NavigationFeatureDelegate etc. Those should be cleaned up - there's no need for the Foo prefix on FooDelegate if it's already declared inside class Foo - it can just be Foo::Delegate. https://codereview.chromium.org/1779673003/diff/100001/blimp/engine/session/b... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1779673003/diff/100001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.cc:546: bool page_load_completed = progress == 1.0 ? true : false; On 2016/03/22 23:45:43, shaktisahu wrote: > On 2016/03/22 21:43:38, Wez wrote: > > nit: Why not just completed = (progress == 1.0) > > Not from my CL. Acknowledged. https://codereview.chromium.org/1779673003/diff/100001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.cc:554: NavigationMessage* navigation_message; On 2016/03/22 21:43:38, Wez wrote: > nit: Initialize this, even though it's about to be overwritten. Ping https://codereview.chromium.org/1779673003/diff/120001/blimp/client/feature/i... File blimp/client/feature/ime_feature.cc (right): https://codereview.chromium.org/1779673003/diff/120001/blimp/client/feature/i... blimp/client/feature/ime_feature.cc:17: : delegate_(nullptr), tab_id_(-1), render_widget_id_(0) {} No need to initialize these here if you're doing it inline in the header file. https://codereview.chromium.org/1779673003/diff/120001/blimp/client/feature/i... blimp/client/feature/ime_feature.cc:39: DCHECK_EQ(message->type(), BlimpMessage::IME); nit: DCHECK_EQ(BlimpMessage::IME, message-type()) is equivalent but has the advantage of matching the EXPECT_EQ(expected, actual) ordering. https://codereview.chromium.org/1779673003/diff/120001/blimp/client/feature/i... File blimp/client/feature/ime_feature.h (right): https://codereview.chromium.org/1779673003/diff/120001/blimp/client/feature/i... blimp/client/feature/ime_feature.h:74: int render_widget_id_; What about these?
Hi Wez, Please review thesuggested changes. Thanks Shakti https://codereview.chromium.org/1779673003/diff/60001/blimp/common/create_bli... File blimp/common/create_blimp_message.cc (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/common/create_bli... blimp/common/create_blimp_message.cc:58: DCHECK(ime_message); On 2016/03/23 00:10:53, Wez wrote: > On 2016/03/22 23:45:42, shaktisahu wrote: > > On 2016/03/22 21:43:38, Wez wrote: > > > On 2016/03/18 20:51:28, Wez wrote: > > > > nit: Why do we DCHECK the out-parameter here & in the other > > CreateBlimpMessage > > > > impls? We de-reference it later in the same function, to assign to it, so > > > DCHECK > > > > is redundant. > > > > > > Pingy. > > > > Since ime_message is a pointer to pointer, shouldn't we do a null check before > > de-referencing its value? > > DCHECK makes sense where you are given a pointer that you are not immediately > going to dereference - then rather than crashing when you dereference it and not > knowing why you got a null pointer, you will get a nice DCHECK at the point > where the caller passed you something invalid. > > In this case you are dereferencing it on line #62 - if it's null then the app > will crash, which is all that DCHECK will do - so all the DCHECK is doing is > moving the crash earlier in this function, which isn't useful. I think DCHECK makes it easier for me to debug in the event of a crash. But if you feel strongly about it, I will remove this. https://codereview.chromium.org/1779673003/diff/100001/blimp/engine/session/b... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1779673003/diff/100001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.cc:554: NavigationMessage* navigation_message; On 2016/03/22 21:43:38, Wez wrote: > nit: Initialize this, even though it's about to be overwritten. Done. https://codereview.chromium.org/1779673003/diff/100001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.cc:558: NavigationStateChangeMessage* details = On 2016/03/22 21:43:38, Wez wrote: > Indentation looks wrong here. Done. https://codereview.chromium.org/1779673003/diff/120001/blimp/client/feature/i... File blimp/client/feature/ime_feature.cc (right): https://codereview.chromium.org/1779673003/diff/120001/blimp/client/feature/i... blimp/client/feature/ime_feature.cc:17: : delegate_(nullptr), tab_id_(-1), render_widget_id_(0) {} On 2016/03/23 00:10:53, Wez wrote: > No need to initialize these here if you're doing it inline in the header file. Done. https://codereview.chromium.org/1779673003/diff/120001/blimp/client/feature/i... blimp/client/feature/ime_feature.cc:39: DCHECK_EQ(message->type(), BlimpMessage::IME); On 2016/03/23 00:10:53, Wez wrote: > nit: DCHECK_EQ(BlimpMessage::IME, message-type()) is equivalent but has the > advantage of matching the EXPECT_EQ(expected, actual) ordering. Done. https://codereview.chromium.org/1779673003/diff/120001/blimp/client/feature/i... File blimp/client/feature/ime_feature.h (right): https://codereview.chromium.org/1779673003/diff/120001/blimp/client/feature/i... blimp/client/feature/ime_feature.h:74: int render_widget_id_; On 2016/03/23 00:10:53, Wez wrote: > What about these? Sorry, my bad.. Done.
LGTM! https://codereview.chromium.org/1779673003/diff/60001/blimp/common/create_bli... File blimp/common/create_blimp_message.cc (right): https://codereview.chromium.org/1779673003/diff/60001/blimp/common/create_bli... blimp/common/create_blimp_message.cc:58: DCHECK(ime_message); On 2016/03/23 01:44:13, shaktisahu wrote: > On 2016/03/23 00:10:53, Wez wrote: > > On 2016/03/22 23:45:42, shaktisahu wrote: > > > On 2016/03/22 21:43:38, Wez wrote: > > > > On 2016/03/18 20:51:28, Wez wrote: > > > > > nit: Why do we DCHECK the out-parameter here & in the other > > > CreateBlimpMessage > > > > > impls? We de-reference it later in the same function, to assign to it, > so > > > > DCHECK > > > > > is redundant. > > > > > > > > Pingy. > > > > > > Since ime_message is a pointer to pointer, shouldn't we do a null check > before > > > de-referencing its value? > > > > DCHECK makes sense where you are given a pointer that you are not immediately > > going to dereference - then rather than crashing when you dereference it and > not > > knowing why you got a null pointer, you will get a nice DCHECK at the point > > where the caller passed you something invalid. > > > > In this case you are dereferencing it on line #62 - if it's null then the app > > will crash, which is all that DCHECK will do - so all the DCHECK is doing is > > moving the crash earlier in this function, which isn't useful. > > I think DCHECK makes it easier for me to debug in the event of a crash. But if > you feel strongly about it, I will remove this. How does it make it easier?
On 2016/03/23 23:21:25, Wez wrote: > LGTM! > > https://codereview.chromium.org/1779673003/diff/60001/blimp/common/create_bli... > File blimp/common/create_blimp_message.cc (right): > > https://codereview.chromium.org/1779673003/diff/60001/blimp/common/create_bli... > blimp/common/create_blimp_message.cc:58: DCHECK(ime_message); > On 2016/03/23 01:44:13, shaktisahu wrote: > > On 2016/03/23 00:10:53, Wez wrote: > > > On 2016/03/22 23:45:42, shaktisahu wrote: > > > > On 2016/03/22 21:43:38, Wez wrote: > > > > > On 2016/03/18 20:51:28, Wez wrote: > > > > > > nit: Why do we DCHECK the out-parameter here & in the other > > > > CreateBlimpMessage > > > > > > impls? We de-reference it later in the same function, to assign to it, > > so > > > > > DCHECK > > > > > > is redundant. > > > > > > > > > > Pingy. > > > > > > > > Since ime_message is a pointer to pointer, shouldn't we do a null check > > before > > > > de-referencing its value? > > > > > > DCHECK makes sense where you are given a pointer that you are not > immediately > > > going to dereference - then rather than crashing when you dereference it and > > not > > > knowing why you got a null pointer, you will get a nice DCHECK at the point > > > where the caller passed you something invalid. > > > > > > In this case you are dereferencing it on line #62 - if it's null then the > app > > > will crash, which is all that DCHECK will do - so all the DCHECK is doing is > > > moving the crash earlier in this function, which isn't useful. > > > > I think DCHECK makes it easier for me to debug in the event of a crash. But if > > you feel strongly about it, I will remove this. > > How does it make it easier? I think it prints the exact line number of the crash. Otherwise I have to look at the core dump.
On 2016/03/23 23:21:25, Wez wrote: > LGTM! > > https://codereview.chromium.org/1779673003/diff/60001/blimp/common/create_bli... > File blimp/common/create_blimp_message.cc (right): > > https://codereview.chromium.org/1779673003/diff/60001/blimp/common/create_bli... > blimp/common/create_blimp_message.cc:58: DCHECK(ime_message); > On 2016/03/23 01:44:13, shaktisahu wrote: > > On 2016/03/23 00:10:53, Wez wrote: > > > On 2016/03/22 23:45:42, shaktisahu wrote: > > > > On 2016/03/22 21:43:38, Wez wrote: > > > > > On 2016/03/18 20:51:28, Wez wrote: > > > > > > nit: Why do we DCHECK the out-parameter here & in the other > > > > CreateBlimpMessage > > > > > > impls? We de-reference it later in the same function, to assign to it, > > so > > > > > DCHECK > > > > > > is redundant. > > > > > > > > > > Pingy. > > > > > > > > Since ime_message is a pointer to pointer, shouldn't we do a null check > > before > > > > de-referencing its value? > > > > > > DCHECK makes sense where you are given a pointer that you are not > immediately > > > going to dereference - then rather than crashing when you dereference it and > > not > > > knowing why you got a null pointer, you will get a nice DCHECK at the point > > > where the caller passed you something invalid. > > > > > > In this case you are dereferencing it on line #62 - if it's null then the > app > > > will crash, which is all that DCHECK will do - so all the DCHECK is doing is > > > moving the crash earlier in this function, which isn't useful. > > > > I think DCHECK makes it easier for me to debug in the event of a crash. But if > > you feel strongly about it, I will remove this. > > How does it make it easier? I think it prints the exact line number of the crash. Otherwise I have to look at the core dump.
The CQ bit was checked by shaktisahu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haibinlu@chromium.org Link to the patchset: https://codereview.chromium.org/1779673003/#ps140001 (title: "Cleanup")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1779673003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1779673003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/03/24 01:15:38, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) I see the new deps to ui/base, but AFAICT none of the BUILD.gn files actually have deps on ui/base. What am I missing? ui/base is a bit of a hodge-podge. What specifically do you need there? Can you add blimp to check_targets in .gn? Better do this earlier rather than later to catch missing dependencies.
Looks like we're just pulling in the text-input-type enum (in WebInputBox)? On 23 March 2016 at 19:59, <sky@chromium.org> wrote: > On 2016/03/24 01:15:38, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub... > ) > > I see the new deps to ui/base, but AFAICT none of the BUILD.gn files > actually > have deps on ui/base. What am I missing? ui/base is a bit of a > hodge-podge. What > specifically do you need there? > Can you add blimp to check_targets in .gn? Better do this earlier rather > than > later to catch missing dependencies. > > https://codereview.chromium.org/1779673003/ > -- 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.
On 2016/03/24 02:59:47, sky wrote: > On 2016/03/24 01:15:38, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > I see the new deps to ui/base, but AFAICT none of the BUILD.gn files actually > have deps on ui/base. What am I missing? ui/base is a bit of a hodge-podge. What > specifically do you need there? > Can you add blimp to check_targets in .gn? Better do this earlier rather than > later to catch missing dependencies. I fixed the DEPS files to include only ui/base/ime/text_input_type.h instead of whole ui/base. Adding blimp to check_targets in .gn is a good idea. I will try that in a separate CL.
On 2016/03/24 02:59:47, sky wrote: > On 2016/03/24 01:15:38, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > I see the new deps to ui/base, but AFAICT none of the BUILD.gn files actually > have deps on ui/base. What am I missing? ui/base is a bit of a hodge-podge. What > specifically do you need there? > Can you add blimp to check_targets in .gn? Better do this earlier rather than > later to catch missing dependencies. I fixed the DEPS files to include only ui/base/ime/text_input_type.h instead of whole ui/base. Adding blimp to check_targets in .gn is a good idea. I will try that in a separate CL.
I still don't see any BUILD.gn files dependening upon ui/base/ime:text_input_types. How come?
On 2016/03/24 19:15:24, sky wrote: > I still don't see any BUILD.gn files dependening upon > ui/base/ime:text_input_types. How come? May be it is just a header file, that's why it doesn't give error. I have added it to the BUILD.gn file now.
On 2016/03/24 19:15:24, sky wrote: > I still don't see any BUILD.gn files dependening upon > ui/base/ime:text_input_types. How come? May be it is just a header file, that's why it doesn't give error. I have added it to the BUILD.gn file now.
gn check would uncover all of these problems. https://codereview.chromium.org/1779673003/diff/180001/blimp/client/BUILD.gn File blimp/client/BUILD.gn (right): https://codereview.chromium.org/1779673003/diff/180001/blimp/client/BUILD.gn#... blimp/client/BUILD.gn:113: "feature/ime_feature.cc", These files include ui/base/ime, I would expect deps in here to ui/base/ime as well. That goes for any place you're including ui/base/ime. https://codereview.chromium.org/1779673003/diff/180001/blimp/common/BUILD.gn File blimp/common/BUILD.gn (right): https://codereview.chromium.org/1779673003/diff/180001/blimp/common/BUILD.gn#... blimp/common/BUILD.gn:36: "//ui/base/ime:text_input_types", Are any of the files in this target actually including ui/base/ime?
On 2016/03/24 22:10:58, sky wrote: > gn check would uncover all of these problems. > > https://codereview.chromium.org/1779673003/diff/180001/blimp/client/BUILD.gn > File blimp/client/BUILD.gn (right): > > https://codereview.chromium.org/1779673003/diff/180001/blimp/client/BUILD.gn#... > blimp/client/BUILD.gn:113: "feature/ime_feature.cc", > These files include ui/base/ime, I would expect deps in here to ui/base/ime as > well. That goes for any place you're including ui/base/ime. > > https://codereview.chromium.org/1779673003/diff/180001/blimp/common/BUILD.gn > File blimp/common/BUILD.gn (right): > > https://codereview.chromium.org/1779673003/diff/180001/blimp/common/BUILD.gn#... > blimp/common/BUILD.gn:36: "//ui/base/ime:text_input_types", > Are any of the files in this target actually including ui/base/ime? I think there are a lot of other similar problems already in blimp. I have filed a bug for this crbug/597830
Hi sky, Please review the last DEPS changes in BUILD.gn files Thanks Shakti https://codereview.chromium.org/1779673003/diff/180001/blimp/client/BUILD.gn File blimp/client/BUILD.gn (right): https://codereview.chromium.org/1779673003/diff/180001/blimp/client/BUILD.gn#... blimp/client/BUILD.gn:113: "feature/ime_feature.cc", On 2016/03/24 22:10:58, sky wrote: > These files include ui/base/ime, I would expect deps in here to ui/base/ime as > well. That goes for any place you're including ui/base/ime. Done. https://codereview.chromium.org/1779673003/diff/180001/blimp/common/BUILD.gn File blimp/common/BUILD.gn (right): https://codereview.chromium.org/1779673003/diff/180001/blimp/common/BUILD.gn#... blimp/common/BUILD.gn:36: "//ui/base/ime:text_input_types", On 2016/03/24 22:10:58, sky wrote: > Are any of the files in this target actually including ui/base/ime? Acknowledged. I have removed these and added deps at proper places.
sky: Thanks for pointing out gn check; we're aware of it but haven't got into the habit of using it yet! shaktisahu: Thanks for filing a bug for it. :D On 24 March 2016 at 16:13, <shaktisahu@chromium.org> wrote: > Hi sky, > > Please review the last DEPS changes in BUILD.gn files > > Thanks > Shakti > > > > https://codereview.chromium.org/1779673003/diff/180001/blimp/client/BUILD.gn > File blimp/client/BUILD.gn (right): > > > https://codereview.chromium.org/1779673003/diff/180001/blimp/client/BUILD.gn#... > blimp/client/BUILD.gn:113: "feature/ime_feature.cc", > On 2016/03/24 22:10:58, sky wrote: > > These files include ui/base/ime, I would expect deps in here to > ui/base/ime as > > well. That goes for any place you're including ui/base/ime. > > Done. > > > https://codereview.chromium.org/1779673003/diff/180001/blimp/common/BUILD.gn > File blimp/common/BUILD.gn (right): > > > https://codereview.chromium.org/1779673003/diff/180001/blimp/common/BUILD.gn#... > blimp/common/BUILD.gn:36: "//ui/base/ime:text_input_types", > On 2016/03/24 22:10:58, sky wrote: > > Are any of the files in this target actually including ui/base/ime? > > Acknowledged. I have removed these and added deps at proper places. > > https://codereview.chromium.org/1779673003/ > -- 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.
LGTM
The CQ bit was checked by shaktisahu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org, haibinlu@chromium.org Link to the patchset: https://codereview.chromium.org/1779673003/#ps200001 (title: "More deps")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1779673003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1779673003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by shaktisahu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, wez@chromium.org, haibinlu@chromium.org Link to the patchset: https://codereview.chromium.org/1779673003/#ps220001 (title: "Merge origin/master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1779673003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1779673003/220001
Message was sent while issue was closed.
Description was changed from ========== Added network components for blimp text input feature This is the third CL for enabling text input. Created protobuf ImeMessage containing text and IME related information to be sent over network. Added WebInputFeature and WebInputFeatureDelegate to handle these messages. BUG=585646 ========== to ========== Added network components for blimp text input feature This is the third CL for enabling text input. Created protobuf ImeMessage containing text and IME related information to be sent over network. Added WebInputFeature and WebInputFeatureDelegate to handle these messages. BUG=585646 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Added network components for blimp text input feature This is the third CL for enabling text input. Created protobuf ImeMessage containing text and IME related information to be sent over network. Added WebInputFeature and WebInputFeatureDelegate to handle these messages. BUG=585646 ========== to ========== Added network components for blimp text input feature This is the third CL for enabling text input. Created protobuf ImeMessage containing text and IME related information to be sent over network. Added WebInputFeature and WebInputFeatureDelegate to handle these messages. BUG=585646 Committed: https://crrev.com/ded7b6ba488f2163e9588573cbb5e705222b9205 Cr-Commit-Position: refs/heads/master@{#383242} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/ded7b6ba488f2163e9588573cbb5e705222b9205 Cr-Commit-Position: refs/heads/master@{#383242} |
