Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1080693002:
Implementation of Smart GO NEXT feature in Android Chrome (Closed)
|
Created:
5 years, 8 months ago by AKV Modified:
3 years, 8 months ago CC:
aelias_OOO_until_Jul13, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, kochi, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, yosin_UTC9 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplementation of Smart GO NEXT feature in Android Chrome
This change takes care of providing easy navigation among text
input elements inside a form.
Dependent blink side changes can be found at
https://codereview.chromium.org/939603002/
BUG=410785
Patch Set 1 #
Total comments: 7
Patch Set 2 : Changed the direction variable to forward. #Patch Set 3 : Kept ACTION_GO when there is no next/prev/enter Key. #
Total comments: 6
Patch Set 4 : Enabling GO button even if we have Previous element available. #
Total comments: 4
Patch Set 5 : Moved overridden function to correct location in header. #
Total comments: 6
Patch Set 6 : Fixed indentation and naming convention issues. #
Total comments: 4
Patch Set 7 : Removed duplicate comments and formatted the comments. #Patch Set 8 : Updated based on blink side reviews. #
Total comments: 1
Messages
Total messages: 35 (7 generated)
ajith.v@chromium.org changed reviewers: + bcwhite@chromium.org
@bcwhite PTAL
https://codereview.chromium.org/1080693002/diff/1/content/common/input_messag... File content/common/input_messages.h (right): https://codereview.chromium.org/1080693002/diff/1/content/common/input_messag... content/common/input_messages.h:190: IPC_MESSAGE_ROUTED1(InputMsg_AdvanceFocusToNextInputField, bool /* direction */) "forward" instead of "direction" so we know which way "true" represents. https://codereview.chromium.org/1080693002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (right): https://codereview.chromium.org/1080693002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:125: } I don't see any way to show the GO button. Should that appear if there is no NEXT or PREVIOUS? https://codereview.chromium.org/1080693002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1080693002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:620: public void advanceFocusToNextInputField(boolean direction) { direction -> forward (here and elsewhere)
@bcwhite PTAL https://codereview.chromium.org/1080693002/diff/1/content/common/input_messag... File content/common/input_messages.h (right): https://codereview.chromium.org/1080693002/diff/1/content/common/input_messag... content/common/input_messages.h:190: IPC_MESSAGE_ROUTED1(InputMsg_AdvanceFocusToNextInputField, bool /* direction */) On 2015/04/13 16:51:34, bcwhite wrote: > "forward" instead of "direction" so we know which way "true" represents. Done. https://codereview.chromium.org/1080693002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (right): https://codereview.chromium.org/1080693002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:125: } On 2015/04/13 16:51:34, bcwhite wrote: > I don't see any way to show the GO button. Should that appear if there is no > NEXT or PREVIOUS? Mostly this use case can happen when the element is last text child of a form. In reality, there will be Enter key listener to it to submit the form when we press the enter key on last input element. But for a test page it may not be necessary. But our code currently assumes and on Keyboard we can see "Enter Key" available by default. If we observe the code at line 117, I am trying to remove the Enter key from the keyboard when there is no enter key listener for that field. So by default enter key is available. "Enter Key" is better than "GO" I feel. But I wanted to remove it when there is no listener. In that case there will be no "Enter Key", nor PREVIOUS/NEXT key. I will decide based on your opinion. https://codereview.chromium.org/1080693002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1080693002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:620: public void advanceFocusToNextInputField(boolean direction) { On 2015/04/13 16:51:34, bcwhite wrote: > direction -> forward (here and elsewhere) Done.
https://codereview.chromium.org/1080693002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (right): https://codereview.chromium.org/1080693002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:125: } On 2015/04/13 18:35:46, AKV wrote: > On 2015/04/13 16:51:34, bcwhite wrote: > > I don't see any way to show the GO button. Should that appear if there is no > > NEXT or PREVIOUS? > Mostly this use case can happen when the element is last text child of a form. > In reality, there will be Enter key listener to it to submit the form when we > press the enter key on last input element. But for a test page it may not be > necessary. But our code currently assumes and on Keyboard we can see "Enter Key" > available by default. If we observe the code at line 117, I am trying to remove > the Enter key from the keyboard when there is no enter key listener for that > field. So by default enter key is available. "Enter Key" is better than "GO" I > feel. But I wanted to remove it when there is no listener. In that case there > will be no "Enter Key", nor PREVIOUS/NEXT key. I will decide based on your > opinion. > The existing functionality is to show GO so we need to follow that. Specify GO if there is no enter-key handler and Enter if there is. NEXT should take precedence over GO, though.
@bcwhite - PTAL. I made the changes related to GO button status.
https://codereview.chromium.org/1080693002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (right): https://codereview.chromium.org/1080693002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:126: if (mSingleLine && (inputFlags & WebTextInputFlags.WantEnterEvents) == 0) { Should this be || (or)? https://codereview.chromium.org/1080693002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:128: if (!haveNextInput) outAttrs.imeOptions |= EditorInfo.IME_ACTION_GO; This would mean no GO button if there was a PREVIOUS button. We want GO any time there is no NEXT button.
@bcwhite - PTAL https://codereview.chromium.org/1080693002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (right): https://codereview.chromium.org/1080693002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:126: if (mSingleLine && (inputFlags & WebTextInputFlags.WantEnterEvents) == 0) { On 2015/04/14 18:22:08, bcwhite wrote: > Should this be || (or)? For textarea and content editable, which has to support multi line, can have no enter key listeners. But we should show enter key still to enter multi line data. GO should not be shown. So this case is specific to Single line input with no enter key listeners. https://codereview.chromium.org/1080693002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:128: if (!haveNextInput) outAttrs.imeOptions |= EditorInfo.IME_ACTION_GO; On 2015/04/14 18:22:08, bcwhite wrote: > This would mean no GO button if there was a PREVIOUS button. We want GO any > time there is no NEXT button. Done. But I feel we need to discuss with Keyboard team to give us an option to display both NEXT and PREVIOUS at same time to provide better user experience, along with option to disable the enter key from the keyboard whenever required.
LGTM https://codereview.chromium.org/1080693002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (right): https://codereview.chromium.org/1080693002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:126: if (mSingleLine && (inputFlags & WebTextInputFlags.WantEnterEvents) == 0) { On 2015/04/14 18:41:26, AKV wrote: > On 2015/04/14 18:22:08, bcwhite wrote: > > Should this be || (or)? > For textarea and content editable, which has to support multi line, can have no > enter key listeners. But we should show enter key still to enter multi line > data. GO should not be shown. So this case is specific to Single line input with > no enter key listeners. > Understood. https://codereview.chromium.org/1080693002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:128: if (!haveNextInput) outAttrs.imeOptions |= EditorInfo.IME_ACTION_GO; On 2015/04/14 18:41:26, AKV wrote: > On 2015/04/14 18:22:08, bcwhite wrote: > > This would mean no GO button if there was a PREVIOUS button. We want GO any > > time there is no NEXT button. > > Done. But I feel we need to discuss with Keyboard team to give us an option to > display both NEXT and PREVIOUS at same time to provide better user experience, > along with option to disable the enter key from the keyboard whenever required. Do NEXT and PREVIOUS ever both appear in your current testing? When, in your experience, is ENTER appearing that it should not? Adding a way to disable ENTER would required a new Android SDK that defines the value. I'll mention that.
On 2015/04/14 18:56:59, bcwhite wrote: > LGTM > > https://codereview.chromium.org/1080693002/diff/40001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java > (right): > > https://codereview.chromium.org/1080693002/diff/40001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:126: > if (mSingleLine && (inputFlags & WebTextInputFlags.WantEnterEvents) == 0) { > On 2015/04/14 18:41:26, AKV wrote: > > On 2015/04/14 18:22:08, bcwhite wrote: > > > Should this be || (or)? > > For textarea and content editable, which has to support multi line, can have > no > > enter key listeners. But we should show enter key still to enter multi line > > data. GO should not be shown. So this case is specific to Single line input > with > > no enter key listeners. > > > > Understood. > > https://codereview.chromium.org/1080693002/diff/40001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:128: > if (!haveNextInput) outAttrs.imeOptions |= EditorInfo.IME_ACTION_GO; > On 2015/04/14 18:41:26, AKV wrote: > > On 2015/04/14 18:22:08, bcwhite wrote: > > > This would mean no GO button if there was a PREVIOUS button. We want GO any > > > time there is no NEXT button. > > > > Done. But I feel we need to discuss with Keyboard team to give us an option to > > display both NEXT and PREVIOUS at same time to provide better user experience, > > along with option to disable the enter key from the keyboard whenever > required. > > Do NEXT and PREVIOUS ever both appear in your current testing? > > When, in your experience, is ENTER appearing that it should not? Adding a way > to disable ENTER would required a new Android SDK that defines the value. I'll > mention that. @bcwhite - Thanks for your review comments. In my testing NEXT and PREVIOUS never appeared together. Infact PREVIOUS itself not coming correctly. As I remember I saw PREV key when I tested the initial patch on a KK device. But in L device couldn't see PREV at all. It was always "<-|" when I OR with ACTION_PREVIOUS. In my opinion, Enter Key is no need to appear when there is no action associated with that element in response to Enter Key. So do we need to wait until next Android SDK release(5.XXX) to land this patch ?
ajith.v@chromium.org changed reviewers: + avi@chromium.org, yfriedman@chromium.org
@Avi & Yaron PTAL this change.
It would be nice to see some trybots. https://codereview.chromium.org/1080693002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1080693002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:982: void AdvanceFocusToNextInputField(bool forward); If this is overriding a method on WebContents, then move it to the section with all its friends, and mark it with override.
On 2015/04/14 19:05:37, AKV wrote: > On 2015/04/14 18:56:59, bcwhite wrote: > > LGTM > > > > > https://codereview.chromium.org/1080693002/diff/40001/content/public/android/... > > File > > > content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java > > (right): > > > > > https://codereview.chromium.org/1080693002/diff/40001/content/public/android/... > > > content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:126: > > if (mSingleLine && (inputFlags & WebTextInputFlags.WantEnterEvents) == 0) { > > On 2015/04/14 18:41:26, AKV wrote: > > > On 2015/04/14 18:22:08, bcwhite wrote: > > > > Should this be || (or)? > > > For textarea and content editable, which has to support multi line, can have > > no > > > enter key listeners. But we should show enter key still to enter multi line > > > data. GO should not be shown. So this case is specific to Single line input > > with > > > no enter key listeners. > > > > > > > Understood. > > > > > https://codereview.chromium.org/1080693002/diff/40001/content/public/android/... > > > content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:128: > > if (!haveNextInput) outAttrs.imeOptions |= EditorInfo.IME_ACTION_GO; > > On 2015/04/14 18:41:26, AKV wrote: > > > On 2015/04/14 18:22:08, bcwhite wrote: > > > > This would mean no GO button if there was a PREVIOUS button. We want GO > any > > > > time there is no NEXT button. > > > > > > Done. But I feel we need to discuss with Keyboard team to give us an option > to > > > display both NEXT and PREVIOUS at same time to provide better user > experience, > > > along with option to disable the enter key from the keyboard whenever > > required. > > > > Do NEXT and PREVIOUS ever both appear in your current testing? > > > > When, in your experience, is ENTER appearing that it should not? Adding a way > > to disable ENTER would required a new Android SDK that defines the value. > I'll > > mention that. > > @bcwhite - Thanks for your review comments. > In my testing NEXT and PREVIOUS never appeared together. Infact PREVIOUS itself > not coming correctly. As I remember I saw PREV key when I tested the initial > patch on a KK device. But in L device couldn't see PREV at all. It was always > "<-|" when I OR with ACTION_PREVIOUS. In my opinion, Enter Key is no need to > appear when there is no action associated with that element in response to Enter > Key. > So do we need to wait until next Android SDK release(5.XXX) to land this patch ? Okay. Create a publicly visible test page with several fields that will be present. Document (on that page) what buttons you feel should appear when that field is in focus and what you are observing instead. I'll take this to the keyboard team to get their input.
jdduke@chromium.org changed reviewers: + jdduke@chromium.org
This is not ready for landing. Playing with a toy example form here, http://jsbin.com/kenozu/1/quiet, I don't get consistent behavior. Just a few issues I noticed in the first few moments of testing: 1) If I tap the second field in a form, I always get the "previous" button, never the "next" button. 2) Are we really OK with never being able to submit a form with the keyboard when there are multiple fields? 3) If I use the previous button repeatedly, once I return to the first field it doesn't expose the "next" button unless I clear focus and tap the first field again. 4) Pressing return in textareas within a multi-field form will now take me next/previous instead of creating a newline. https://codereview.chromium.org/1080693002/diff/60001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1080693002/diff/60001/content/renderer/render... content/renderer/render_view_impl.cc:230: using blink::WebFocusType; We only use this once, don't think it merits a using declaration.
On 2015/04/15 18:42:46, jdduke wrote: > This is not ready for landing. > > Playing with a toy example form here, http://jsbin.com/kenozu/1/quiet, I don't > get consistent behavior. Just a few issues I noticed in the first few moments of > testing: > > 1) If I tap the second field in a form, I always get the "previous" button, > never the "next" button. This is a problem from the Keyboard, if we OR with IME_ACTION_PREVIOUS irrespective of previous values, it shows PREVIOUS always. I had raised this concern to bcwhite earlier. > 2) Are we really OK with never being able to submit a form with the keyboard > when there are multiple fields? If the field is not responding to enter key, then we don't have to allow submit the form with enter key. > 3) If I use the previous button repeatedly, once I return to the first field it > doesn't expose the "next" button unless I clear focus and tap the first field > again. This I will check, I saw during my sanity check yesterday. > 4) Pressing return in textareas within a multi-field form will now take me > next/previous instead of creating a newline. > If we have a NEXT/PREVIOUS an > https://codereview.chromium.org/1080693002/diff/60001/content/renderer/render... > File content/renderer/render_view_impl.cc (right): > > https://codereview.chromium.org/1080693002/diff/60001/content/renderer/render... > content/renderer/render_view_impl.cc:230: using blink::WebFocusType; > We only use this once, don't think it merits a using declaration.
On 2015/04/15 18:49:14, AKV wrote: > On 2015/04/15 18:42:46, jdduke wrote: > > This is not ready for landing. > > > > Playing with a toy example form here, http://jsbin.com/kenozu/1/quiet, I don't > > get consistent behavior. Just a few issues I noticed in the first few moments > of > > testing: > > > > 1) If I tap the second field in a form, I always get the "previous" button, > > never the "next" button. > > This is a problem from the Keyboard, if we OR with IME_ACTION_PREVIOUS > irrespective of previous values, it shows PREVIOUS always. I had raised this > concern to bcwhite earlier. > > > 2) Are we really OK with never being able to submit a form with the keyboard > > when there are multiple fields? > > If the field is not responding to enter key, then we don't have to allow submit > the form with enter key. > > > 3) If I use the previous button repeatedly, once I return to the first field > it > > doesn't expose the "next" button unless I clear focus and tap the first field > > again. > > This I will check, I saw during my sanity check yesterday. > > > 4) Pressing return in textareas within a multi-field form will now take me > > next/previous instead of creating a newline. > > > If we have a NEXT/PREVIOUS and ENTER, then only ENTER is getting appeared on the keyboard. This is Keyboard issue, and we can see even though it shows ENTER, the event comes in PREV, so this also I raised to bcwhite to in document. > > > https://codereview.chromium.org/1080693002/diff/60001/content/renderer/render... > > File content/renderer/render_view_impl.cc (right): > > > > > https://codereview.chromium.org/1080693002/diff/60001/content/renderer/render... > > content/renderer/render_view_impl.cc:230: using blink::WebFocusType; > > We only use this once, don't think it merits a using declaration. Thank you for comments and suggestions. I will address these issues while fixing the review comments.
I also noticed that with the stock keyboard on Android L, once I get to the last form field the "next" button doesn't switch to the "previous" button. I can only get the "previous" button if I initially focus a field that is not the first field. We'll definitely want to test this with a few of the more popular keyboard types, as well as on different Android versions.
On 2015/04/14 20:42:04, Avi wrote: > It would be nice to see some trybots. > > https://codereview.chromium.org/1080693002/diff/60001/content/browser/web_con... > File content/browser/web_contents/web_contents_impl.h (right): > > https://codereview.chromium.org/1080693002/diff/60001/content/browser/web_con... > content/browser/web_contents/web_contents_impl.h:982: void > AdvanceFocusToNextInputField(bool forward); > If this is overriding a method on WebContents, then move it to the section with > all its friends, and mark it with override. @Avi this patch directly depends on blink side patch https://codereview.chromium.org/939603002/. So until blink side patch lands, we can't make trybots happy. I will fix other comments and make this patch in ready to land state. Thanks for your review comments.
@Avi & jdduke - PTAL! https://codereview.chromium.org/1080693002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1080693002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:982: void AdvanceFocusToNextInputField(bool forward); On 2015/04/14 20:42:04, Avi wrote: > If this is overriding a method on WebContents, then move it to the section with > all its friends, and mark it with override. Done. https://codereview.chromium.org/1080693002/diff/60001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1080693002/diff/60001/content/renderer/render... content/renderer/render_view_impl.cc:230: using blink::WebFocusType; On 2015/04/15 18:42:46, jdduke wrote: > We only use this once, don't think it merits a using declaration. Done.
https://codereview.chromium.org/1080693002/diff/80001/content/browser/rendere... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/1080693002/diff/80001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:304: jboolean forward) { fix indentation https://codereview.chromium.org/1080693002/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1080693002/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:676: No. In web_contents.h, you have it with its friends, located right after Unselect. Put this ALSO right after Unselect, and in the .cc file, put this ALSO right after Unselect. Also, put the comment in web_contents.h. https://codereview.chromium.org/1080693002/diff/80001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1080693002/diff/80001/content/renderer/render... content/renderer/render_view_impl.cc:1457: focusType = blink::WebFocusTypeBackward; Two comments before discarding this entire block. First, if (forward == false) is crazy; do if (!forward). Second, use_underscores_in_chromium_code, notCamelCase. But do instead: blink::WebFocusType focus_type = forward ? blink::WebFocusTypeForward : blink::WebFocusTypeBackward;
@Avi PTAL. https://codereview.chromium.org/1080693002/diff/80001/content/browser/rendere... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/1080693002/diff/80001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:304: jboolean forward) { On 2015/04/17 16:26:38, Avi wrote: > fix indentation Done. Sorry for it, recently I changed the name of function, So it happened. https://codereview.chromium.org/1080693002/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1080693002/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:676: On 2015/04/17 16:26:38, Avi wrote: > No. > > In web_contents.h, you have it with its friends, located right after Unselect. > Put this ALSO right after Unselect, and in the .cc file, put this ALSO right > after Unselect. > > Also, put the comment in web_contents.h. Done. Thanks. https://codereview.chromium.org/1080693002/diff/80001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1080693002/diff/80001/content/renderer/render... content/renderer/render_view_impl.cc:1457: focusType = blink::WebFocusTypeBackward; On 2015/04/17 16:26:38, Avi wrote: > Two comments before discarding this entire block. > > First, if (forward == false) is crazy; do if (!forward). > Second, use_underscores_in_chromium_code, notCamelCase. > > But do instead: > > blink::WebFocusType focus_type = forward ? blink::WebFocusTypeForward : > blink::WebFocusTypeBackward; > Done. Thanks
LGTM with fixed nits. https://codereview.chromium.org/1080693002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1080693002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:297: void AdvanceFocusToNextFormControl(bool forward) override; Drop the comment here; having it in the inherited header is enough. https://codereview.chromium.org/1080693002/diff/100001/content/public/browser... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/1080693002/diff/100001/content/public/browser... content/public/browser/web_contents.h:394: // Called to advance the focus to next form control element in Blank line before this one.
@Avi Thanks for your review. https://codereview.chromium.org/1080693002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1080693002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:297: void AdvanceFocusToNextFormControl(bool forward) override; On 2015/04/17 16:46:27, Avi wrote: > Drop the comment here; having it in the inherited header is enough. Done. https://codereview.chromium.org/1080693002/diff/100001/content/public/browser... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/1080693002/diff/100001/content/public/browser... content/public/browser/web_contents.h:394: // Called to advance the focus to next form control element in On 2015/04/17 16:46:27, Avi wrote: > Blank line before this one. Done.
@bcwhite & avi Could you please take final look. I have updated based on blink side reviews.
On 2015/04/28 14:53:49, AKV wrote: > @bcwhite & avi Could you please take final look. I have updated based on blink > side reviews. Still lgtm. Note that you will need a reviewer for content/common/input_messages.h as that's IPC.
ajith.v@chromium.org changed reviewers: + dcheng@chromium.org
@Avi Thank you. @dcheng - PTAL IPC related changes.
ipc changes lgtm https://codereview.chromium.org/1080693002/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1080693002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:295: void AdvanceFocusInForm(bool forward) override; Nit: I would suggest using an enum for this as well, to match what Blink does (it makes the callsite clearer).
LGTM (java) sorry for the delay
bcwhite@chromium.org changed reviewers: - bcwhite@chromium.org
ajith.v@samsung.com changed reviewers: - jdduke@chromium.org
ajith.v@samsung.com changed reviewers: - avi@chromium.org
On 2015/05/13 15:16:45, bcwhite wrote: > LGTM (java) > sorry for the delay Closing this issue. As there is another review is active which includes both Chromium and blink changes together @ https://codereview.chromium.org/2839993002/ |
