Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18750003:
Require ACK for editor-related changes not originating from browser. (Closed)
|
Created:
7 years, 5 months ago by nyquist Modified:
7 years, 4 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, penghuang+watch_chromium.org, joi+watch-content_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, James Su, jochen+watch_chromium.org, palmer Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRequire ACK for editor-related changes not originating from browser.
Whenever a change happens in the renderer regarding editing and it
did not originate from the browser (for example JavaScript, autofill),
this CL now requires the browser side to acknowledge the change before
the renderer handles any more IME events. If there are IME events in
flight before the browser sends the acknowledgement, the renderer drops
the event.
BUG=145521, 230848, 172845, 248544, 235704
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215698
Patch Set 1 #
Total comments: 23
Patch Set 2 : Rebased #Patch Set 3 : Added ifdefs instead of depending on flag value, addressed comments #Patch Set 4 : Fixed compile warning on windows #
Total comments: 5
Patch Set 5 : Removed flag, simplified implementation #Patch Set 6 : Rebased #
Total comments: 2
Patch Set 7 : Rebased #Patch Set 8 : Change from using enums to bools for UpdateTextInputState #
Total comments: 2
Patch Set 9 : Fixed naming nit #Patch Set 10 : Rebased #
Messages
Total messages: 29 (0 generated)
PTAL
I'll take closer look but how about using "ordered IME processing" instead of "strict IME processing" for switches etc.?
First round of comments from me. Do you have a minimal reproduction case for confirming what this CL fixes? https://codereview.chromium.org/18750003/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/18750003/diff/1/chrome/browser/about_flags.cc... chrome/browser/about_flags.cc:676: kOsAndroid, A general question: how can we pass an argument on Android? https://codereview.chromium.org/18750003/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/18750003/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_android.h:228: private: nit: not necessary to remove a space. https://codereview.chromium.org/18750003/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_android.h:251: bool has_strict_ime_processing() { return strict_ime_processing_; } Usually we use the same name for a simple accessor - probably rename the member to bool has_strict_ime_processing_? https://codereview.chromium.org/18750003/diff/1/content/renderer/render_widge... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/18750003/diff/1/content/renderer/render_widge... content/renderer/render_widget.cc:1781: return; Can you add #if defined(OS_ANDROID) around this? https://codereview.chromium.org/18750003/diff/1/content/renderer/render_widge... content/renderer/render_widget.cc:1799: return; Ditto. https://codereview.chromium.org/18750003/diff/1/content/renderer/render_widge... content/renderer/render_widget.cc:1976: #endif Can you move this line below the following added section? https://codereview.chromium.org/18750003/diff/1/content/renderer/render_widge... content/renderer/render_widget.cc:1980: outstanding_ime_acknowledgements_++; I thought we use pre-increment style in general, even though it doesn't make any difference on primitive integer types here - but looks like post-increment/decrement style is used elsewhere in this file except for loop counters... https://codereview.chromium.org/18750003/diff/1/content/renderer/render_widge... content/renderer/render_widget.cc:1985: outstanding_ime_acknowledgements_--; Ditto. https://codereview.chromium.org/18750003/diff/1/content/renderer/render_widget.h File content/renderer/render_widget.h (right): https://codereview.chromium.org/18750003/diff/1/content/renderer/render_widge... content/renderer/render_widget.h:732: bool strict_ime_processing_; Same here. rename the member as has_strict_ime_processing_?
https://chromiumcodereview.appspot.com/18750003/diff/1/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://chromiumcodereview.appspot.com/18750003/diff/1/chrome/browser/about_f... chrome/browser/about_flags.cc:676: kOsAndroid, On 2013/07/08 04:28:43, Takayoshi Kochi wrote: > A general question: how can we pass an argument on Android? adb shell 'echo "chrome <flags>" > /data/local/chrome-command-line' https://chromiumcodereview.appspot.com/18750003/diff/1/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://chromiumcodereview.appspot.com/18750003/diff/1/content/renderer/rende... content/renderer/render_view_impl.cc:1713: if (RenderWidget::HasOutstandingImeEventAcknowledgements()) Maybe we should create a call shouldProcessImeEvent() that has this check? https://chromiumcodereview.appspot.com/18750003/diff/1/content/renderer/rende... File content/renderer/render_widget.cc (right): https://chromiumcodereview.appspot.com/18750003/diff/1/content/renderer/rende... content/renderer/render_widget.cc:348: IPC_MESSAGE_HANDLER(ViewMsg_AcknowledgeImeEvent, OnImeAcknowledgeImeEvent) This Android specific so it should be moved to the ifdef above.
addressed all comments. PTAL. Sadly, the browser tests for RenderViewImplTest are disabled on android since data: URLs are loaded through the browser process, which fails. See https://code.google.com/p/chromium/issues/detail?id=187500 for bug about that. https://codereview.chromium.org/18750003/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/18750003/diff/1/chrome/browser/about_flags.cc... chrome/browser/about_flags.cc:676: kOsAndroid, On 2013/07/08 16:04:49, aurimas wrote: > On 2013/07/08 04:28:43, Takayoshi Kochi wrote: > > A general question: how can we pass an argument on Android? > > adb shell 'echo "chrome <flags>" > /data/local/chrome-command-line' For content-shell: build/android/adb_content_shell_command_line <arguments> For chromium testshell: build/android/adb_chromium_testshell_command_line <arguments> https://codereview.chromium.org/18750003/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/18750003/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_android.h:228: private: On 2013/07/08 04:28:43, Takayoshi Kochi wrote: > nit: not necessary to remove a space. Done. https://codereview.chromium.org/18750003/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_android.h:251: bool has_strict_ime_processing() { return strict_ime_processing_; } On 2013/07/08 04:28:43, Takayoshi Kochi wrote: > Usually we use the same name for a simple accessor - probably rename the member > to bool has_strict_ime_processing_? Done. https://codereview.chromium.org/18750003/diff/1/content/renderer/render_view_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/18750003/diff/1/content/renderer/render_view_... content/renderer/render_view_impl.cc:1713: if (RenderWidget::HasOutstandingImeEventAcknowledgements()) On 2013/07/08 16:04:49, aurimas wrote: > Maybe we should create a call shouldProcessImeEvent() that has this check? Done. https://codereview.chromium.org/18750003/diff/1/content/renderer/render_widge... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/18750003/diff/1/content/renderer/render_widge... content/renderer/render_widget.cc:348: IPC_MESSAGE_HANDLER(ViewMsg_AcknowledgeImeEvent, OnImeAcknowledgeImeEvent) On 2013/07/08 16:04:49, aurimas wrote: > This Android specific so it should be moved to the ifdef above. Done. https://codereview.chromium.org/18750003/diff/1/content/renderer/render_widge... content/renderer/render_widget.cc:1781: return; On 2013/07/08 04:28:43, Takayoshi Kochi wrote: > Can you add #if defined(OS_ANDROID) around this? Done. https://codereview.chromium.org/18750003/diff/1/content/renderer/render_widge... content/renderer/render_widget.cc:1799: return; On 2013/07/08 04:28:43, Takayoshi Kochi wrote: > Ditto. Done. https://codereview.chromium.org/18750003/diff/1/content/renderer/render_widge... content/renderer/render_widget.cc:1976: #endif On 2013/07/08 04:28:43, Takayoshi Kochi wrote: > Can you move this line below the following added section? Done. https://codereview.chromium.org/18750003/diff/1/content/renderer/render_widge... content/renderer/render_widget.cc:1980: outstanding_ime_acknowledgements_++; On 2013/07/08 04:28:43, Takayoshi Kochi wrote: > I thought we use pre-increment style in general, even though it doesn't make > any difference on primitive integer types here - but looks like > post-increment/decrement style is used elsewhere in this file except for > loop counters... Done. https://codereview.chromium.org/18750003/diff/1/content/renderer/render_widge... content/renderer/render_widget.cc:1985: outstanding_ime_acknowledgements_--; On 2013/07/08 04:28:43, Takayoshi Kochi wrote: > Ditto. Done. https://codereview.chromium.org/18750003/diff/1/content/renderer/render_widget.h File content/renderer/render_widget.h (right): https://codereview.chromium.org/18750003/diff/1/content/renderer/render_widge... content/renderer/render_widget.h:732: bool strict_ime_processing_; On 2013/07/08 04:28:43, Takayoshi Kochi wrote: > Same here. rename the member as has_strict_ime_processing_? Done.
https://chromiumcodereview.appspot.com/18750003/diff/25002/content/renderer/r... File content/renderer/render_view_impl.cc (right): https://chromiumcodereview.appspot.com/18750003/diff/25002/content/renderer/r... content/renderer/render_view_impl.cc:1738: if (!RenderWidget::ShouldHandleImeEvent()) Do you need to specify RenderWidget:: even though RenderViewImpl does not override the call?
lgtm nona, could you also take a look?
lgtm https://codereview.chromium.org/18750003/diff/25002/content/renderer/render_w... File content/renderer/render_widget.h (right): https://codereview.chromium.org/18750003/diff/25002/content/renderer/render_w... content/renderer/render_widget.h:334: // Returns whether ordered IME processing has been enabled. nit: newline between comment and next function?
jamesr: PTAL
I don't quite follow the flow here. From the patch description I expect to see something limiting sending of message from browser->renderer until the renderer acknowledges that it has complete processing of an IME event, but from the code it looks like there's an ack sent the other way. We already have an ACK system set up for input events, why is this separate from that mechanism? Why is this configurable? Are there platforms where we don't want this behavior? https://codereview.chromium.org/18750003/diff/25002/chrome/browser/about_flag... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/18750003/diff/25002/chrome/browser/about_flag... chrome/browser/about_flags.cc:688: "enable-ordered-ime-processing", why do you need an about:flags entry for this? when would you want out-of-order IME processing?
PTAL. Addressed comments, clarified CL description, removed flag, reduced the number of changes to when we send UpdateTextInputState. https://codereview.chromium.org/18750003/diff/25002/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/18750003/diff/25002/content/renderer/render_v... content/renderer/render_view_impl.cc:1738: if (!RenderWidget::ShouldHandleImeEvent()) On 2013/07/09 14:34:03, aurimas wrote: > Do you need to specify RenderWidget:: even though RenderViewImpl does not > override the call? Done. https://codereview.chromium.org/18750003/diff/25002/content/renderer/render_w... File content/renderer/render_widget.h (right): https://codereview.chromium.org/18750003/diff/25002/content/renderer/render_w... content/renderer/render_widget.h:334: // Returns whether ordered IME processing has been enabled. On 2013/07/10 02:22:17, Seigo Nonaka wrote: > nit: newline between comment and next function? Done.
What's the status of this CL?
On 2013/07/10 20:26:19, jamesr wrote: > I don't quite follow the flow here. From the patch description I expect to see > something limiting sending of message from browser->renderer until the renderer > acknowledges that it has complete processing of an IME event, but from the code > it looks like there's an ack sent the other way. We already have an ACK system > set up for input events, why is this separate from that mechanism? > > Why is this configurable? Are there platforms where we don't want this > behavior? > > https://codereview.chromium.org/18750003/diff/25002/chrome/browser/about_flag... > File chrome/browser/about_flags.cc (right): > > https://codereview.chromium.org/18750003/diff/25002/chrome/browser/about_flag... > chrome/browser/about_flags.cc:688: "enable-ordered-ime-processing", > why do you need an about:flags entry for this? when would you want out-of-order > IME processing? jamesr: this CL is part of a larger Android IME fixup. The overall issues and our proposed fixes are outlined in https://docs.google.com/a/google.com/document/d/1481evREyW88EQRijW_OzruAy13Ou... This CL is the first step towards that change, it will enable renderer to ignore messages the messages from the browser after there was a change that was triggered by JavaScript, Autofill, or some other way outside IME. The messages that browser sent during that period are based on a wrong state, thus would lead to more brokenness, hence we will ignore them until we get an ACK back from the browser to know that the browser has the latest state. Let me know if you have additional questions.
jamesr: PTAL
Thanks for the description update and context link. This lgtm, but I'm not sure why this needs to be android-specific. Is this something that will be generalized in the future to all platforms?
brettw: PTAL for OWNERS review
jam: PTAL for OWNERS stamp
https://codereview.chromium.org/18750003/diff/61001/content/renderer/render_w... File content/renderer/render_widget.h (right): https://codereview.chromium.org/18750003/diff/61001/content/renderer/render_w... content/renderer/render_widget.h:209: NO_IME_ACK i realize this is the convention in blink, but in chromium the most common thing is just to use bools.. i'd even prefer we switch ShowIme to bool as well
On 2013/07/31 20:57:09, jam wrote: > https://codereview.chromium.org/18750003/diff/61001/content/renderer/render_w... > File content/renderer/render_widget.h (right): > > https://codereview.chromium.org/18750003/diff/61001/content/renderer/render_w... > content/renderer/render_widget.h:209: NO_IME_ACK > i realize this is the convention in blink, but in chromium the most common thing > is just to use bools.. i'd even prefer we switch ShowIme to bool as well I believe that since there are several bool-type values that are passed in to the same method, it becomes confusing what each one stands for. Personally I think it is easier to understand: UpdateTextInputState(SHOW_IME_IF_NEEDED, SEND_IME_ACK) than: UpdateTextInputState(true, true) However, I fully agree that consistency in our codebase is important, so if you feel strongly about it I will of course change it to be bools.
On 2013/07/31 21:53:17, nyquist wrote: > On 2013/07/31 20:57:09, jam wrote: > > > https://codereview.chromium.org/18750003/diff/61001/content/renderer/render_w... > > File content/renderer/render_widget.h (right): > > > > > https://codereview.chromium.org/18750003/diff/61001/content/renderer/render_w... > > content/renderer/render_widget.h:209: NO_IME_ACK > > i realize this is the convention in blink, but in chromium the most common > thing > > is just to use bools.. i'd even prefer we switch ShowIme to bool as well > > I believe that since there are several bool-type values that are passed in to > the same method, it becomes confusing what each one stands for. > > Personally I think it is easier to understand: > UpdateTextInputState(SHOW_IME_IF_NEEDED, SEND_IME_ACK) > than: > UpdateTextInputState(true, true) right, that's the argument that's given for blink. i personally don't buy it that much, and i think the chrome code with (true, false) is readable because when someone wants to know they can look it it up easily. i feel it's the same argument for hungarian notation, and instead we have IDEs that show this. > > However, I fully agree that consistency in our codebase is important, so if you > feel strongly about it I will of course change it to be bools.
On 2013/08/01 00:12:32, jam wrote: > On 2013/07/31 21:53:17, nyquist wrote: > > On 2013/07/31 20:57:09, jam wrote: > > > > > > https://codereview.chromium.org/18750003/diff/61001/content/renderer/render_w... > > > File content/renderer/render_widget.h (right): > > > > > > > > > https://codereview.chromium.org/18750003/diff/61001/content/renderer/render_w... > > > content/renderer/render_widget.h:209: NO_IME_ACK > > > i realize this is the convention in blink, but in chromium the most common > > thing > > > is just to use bools.. i'd even prefer we switch ShowIme to bool as well > > > > I believe that since there are several bool-type values that are passed in to > > the same method, it becomes confusing what each one stands for. > > > > Personally I think it is easier to understand: > > UpdateTextInputState(SHOW_IME_IF_NEEDED, SEND_IME_ACK) > > than: > > UpdateTextInputState(true, true) > > right, that's the argument that's given for blink. i personally don't buy it > that much, and i think the chrome code with (true, false) is readable because > when someone wants to know they can look it it up easily. i feel it's the same > argument for hungarian notation, and instead we have IDEs that show this. Okay. Changed both ShowIme and ImeAck now. Also added explanatory comment for the new bool this CL adds. PTAL
lgtm https://codereview.chromium.org/18750003/diff/77001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/18750003/diff/77001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:410: base::ScopedClosureRunner ackCaller(base::Bind(&SendImeEventAck, host_)); nit: ack_caller per style guide btw never saw ScopedClosureRunner before, cool!
palmer: PTAL for OWNERS review of content/common/view_messages.h https://codereview.chromium.org/18750003/diff/61001/content/renderer/render_w... File content/renderer/render_widget.h (right): https://codereview.chromium.org/18750003/diff/61001/content/renderer/render_w... content/renderer/render_widget.h:209: NO_IME_ACK On 2013/07/31 20:57:09, jam wrote: > i realize this is the convention in blink, but in chromium the most common thing > is just to use bools.. i'd even prefer we switch ShowIme to bool as well Done. https://codereview.chromium.org/18750003/diff/77001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/18750003/diff/77001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:410: base::ScopedClosureRunner ackCaller(base::Bind(&SendImeEventAck, host_)); On 2013/08/01 22:34:07, jam wrote: > nit: ack_caller per style guide > > btw never saw ScopedClosureRunner before, cool! Done. Yeah, the ScopedClosureRunner is neat.
palmer: PTAL for OWNERS review of content/common/view_messages.h. Forgot to add you yesterday.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nyquist@chromium.org/18750003/86001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nyquist@chromium.org/18750003/86001
Message was sent while issue was closed.
Change committed as 215698 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
