Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
133443011:
Add text field change handling for autofill preview in textarea element. (Closed)
|
Created:
6 years, 10 months ago by ziran.sun Modified:
6 years, 9 months ago CC:
blink-reviews, dglazkov+blink, adamk+blink_chromium.org, abarth-chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAdd text field change handling for autofill preview in textarea element.
Extend textarea setSuggestedValue() function to handle the dispaly when
initiate autofill from textarea element.
R=isherman@chromium.org, tkent@chromium.org
BUG=332557
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168720
Patch Set 1 #
Total comments: 11
Patch Set 2 : Update code as per review comments #Patch Set 3 : Wrap selectionStart, selectionEnd and editingValue for TextArea element #Patch Set 4 : Alphabetical order correction for include files #Patch Set 5 : Move selectionStart(), selectionEnd() and editingValue methods to WebFormControlElement interface. #Patch Set 6 : Wrap text #
Total comments: 12
Patch Set 7 : Update code as per review comments from tkent and jamesr #
Total comments: 8
Patch Set 8 : Update code as per tkent's comments #
Total comments: 6
Patch Set 9 : Update documentation as per tkent's review comments #
Messages
Total messages: 42 (0 generated)
Relevant review link - https://codereview.chromium.org/140093005 Thanks!
https://codereview.chromium.org/133443011/diff/1/public/web/WebTextAreaElement.h File public/web/WebTextAreaElement.h (right): https://codereview.chromium.org/133443011/diff/1/public/web/WebTextAreaElemen... public/web/WebTextAreaElement.h:59: BLINK_EXPORT void setSelectionRange(int, int); Can you give these parameters some names? It's not clear from "int" what they mean. Maybe the first one is the start and the second one is the end? Maybe the second one is the length?
https://codereview.chromium.org/133443011/diff/1/Source/core/html/HTMLTextAre... File Source/core/html/HTMLTextAreaElement.cpp (right): https://codereview.chromium.org/133443011/diff/1/Source/core/html/HTMLTextAre... Source/core/html/HTMLTextAreaElement.cpp:451: updatePlaceholderVisibility(false); nit: we can merge updatePlaceholderVisibility calls into one. https://codereview.chromium.org/133443011/diff/1/Source/web/ChromeClientImpl.cpp File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/133443011/diff/1/Source/web/ChromeClientImpl.... Source/web/ChromeClientImpl.cpp:1003: HTMLFormControlElement& element) You don't need to wrap the line. https://codereview.chromium.org/133443011/diff/1/Source/web/ChromeClientImpl.... Source/web/ChromeClientImpl.cpp:1007: m_webView->autofillClient()->textFieldDidChange( Ditto. https://codereview.chromium.org/133443011/diff/1/public/web/WebAutofillClient.h File public/web/WebAutofillClient.h (right): https://codereview.chromium.org/133443011/diff/1/public/web/WebAutofillClient... public/web/WebAutofillClient.h:62: virtual void textFieldDidChange(const WebFormControlElement&) { } Is the WebAutofillClient implementation in Chromium ready for this change? If this CL is landed and the WebAutofillClient still implements the old signature, the WebAutofillClient doesn't receive textFieldDidChange notification.
Updated code as per review comments. https://codereview.chromium.org/133443011/diff/1/Source/core/html/HTMLTextAre... File Source/core/html/HTMLTextAreaElement.cpp (right): https://codereview.chromium.org/133443011/diff/1/Source/core/html/HTMLTextAre... Source/core/html/HTMLTextAreaElement.cpp:451: updatePlaceholderVisibility(false); On 2014/02/06 00:05:45, tkent wrote: > nit: we can merge updatePlaceholderVisibility calls into one. Done. https://codereview.chromium.org/133443011/diff/1/Source/web/ChromeClientImpl.cpp File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/133443011/diff/1/Source/web/ChromeClientImpl.... Source/web/ChromeClientImpl.cpp:1003: HTMLFormControlElement& element) On 2014/02/06 00:05:45, tkent wrote: > You don't need to wrap the line. Done. https://codereview.chromium.org/133443011/diff/1/Source/web/ChromeClientImpl.... Source/web/ChromeClientImpl.cpp:1007: m_webView->autofillClient()->textFieldDidChange( On 2014/02/06 00:05:45, tkent wrote: > Ditto. Done. https://codereview.chromium.org/133443011/diff/1/public/web/WebAutofillClient.h File public/web/WebAutofillClient.h (right): https://codereview.chromium.org/133443011/diff/1/public/web/WebAutofillClient... public/web/WebAutofillClient.h:62: virtual void textFieldDidChange(const WebFormControlElement&) { } On 2014/02/06 00:05:45, tkent wrote: > Is the WebAutofillClient implementation in Chromium ready for this change? > If this CL is landed and the WebAutofillClient still implements the old > signature, the WebAutofillClient doesn't receive textFieldDidChange > notification. Yes. Check line 93 https://codereview.chromium.org/140093005/diff/1/components/autofill/content/... I guess this CL needs to land together with the CL in the link? I've added you as reviewer for above commit. Hope it's okay. https://codereview.chromium.org/133443011/diff/1/public/web/WebTextAreaElement.h File public/web/WebTextAreaElement.h (right): https://codereview.chromium.org/133443011/diff/1/public/web/WebTextAreaElemen... public/web/WebTextAreaElement.h:59: BLINK_EXPORT void setSelectionRange(int, int); On 2014/02/05 19:08:13, abarth wrote: > Can you give these parameters some names? It's not clear from "int" what they > mean. Maybe the first one is the start and the second one is the end? Maybe > the second one is the length? Done.
https://codereview.chromium.org/133443011/diff/1/public/web/WebAutofillClient.h File public/web/WebAutofillClient.h (right): https://codereview.chromium.org/133443011/diff/1/public/web/WebAutofillClient... public/web/WebAutofillClient.h:62: virtual void textFieldDidChange(const WebFormControlElement&) { } On 2014/02/06 10:32:17, ziran.sun wrote: > On 2014/02/06 00:05:45, tkent wrote: > > Is the WebAutofillClient implementation in Chromium ready for this change? > > If this CL is landed and the WebAutofillClient still implements the old > > signature, the WebAutofillClient doesn't receive textFieldDidChange > > notification. > > Yes. Check line 93 > https://codereview.chromium.org/140093005/diff/1/components/autofill/content/... > > I guess this CL needs to land together with the CL in the link? I've added you > as reviewer for above commit. Hope it's okay. It's almost impossible to land a Chromium CL and a Blink CL at the same time because of Blink roll delay. The process should be: 1. Chromium: Remove OVERRIDE from textFieldDidChange(const WebInputElement&), and add textFieldDidChange(const WebFormControlElement&) implementation without OVERRIDE. 2. Blink: Land this CL 3. Wait until next Blink roll 4. Chromium: Remove textFieldDidChange(const WebInputElement&), and add OVERRIDE to textFieldDidChange(const WebFormControlElement&)
On 2014/02/07 06:04:08, tkent wrote: > https://codereview.chromium.org/133443011/diff/1/public/web/WebAutofillClient.h > File public/web/WebAutofillClient.h (right): > > https://codereview.chromium.org/133443011/diff/1/public/web/WebAutofillClient... > public/web/WebAutofillClient.h:62: virtual void textFieldDidChange(const > WebFormControlElement&) { } > On 2014/02/06 10:32:17, ziran.sun wrote: > > On 2014/02/06 00:05:45, tkent wrote: > > > Is the WebAutofillClient implementation in Chromium ready for this change? > > > If this CL is landed and the WebAutofillClient still implements the old > > > signature, the WebAutofillClient doesn't receive textFieldDidChange > > > notification. > > > > Yes. Check line 93 > > > https://codereview.chromium.org/140093005/diff/1/components/autofill/content/... > > > > I guess this CL needs to land together with the CL in the link? I've added you > > as reviewer for above commit. Hope it's okay. > > It's almost impossible to land a Chromium CL and a Blink CL at the same time > because of Blink roll delay. > > The process should be: > 1. Chromium: Remove OVERRIDE from textFieldDidChange(const WebInputElement&), > and add textFieldDidChange(const WebFormControlElement&) implementation without > OVERRIDE. > 2. Blink: Land this CL > 3. Wait until next Blink roll > 4. Chromium: Remove textFieldDidChange(const WebInputElement&), and add OVERRIDE > to textFieldDidChange(const WebFormControlElement&) Updated https://codereview.chromium.org/140093005 The patch temporarily keeps textFieldDidChange(const WebInputElement&) function before the Blink roll. Thanks!
lgtm
Ziran, is this ready to land? Should we check the CQ box?
On 2014/02/14 02:12:32, Ilya Sherman wrote: > Ziran, is this ready to land? Should we check the CQ box? Ilya, from tkent's comment above, my understanding is to land the following patch first. https://codereview.chromium.org/140093005/ I've create a new patch. Please check my comment in this review request about the CL landing sequence is correct or not. https://codereview.chromium.org/167383002/ Thanks!
Code updated. Please review. Thanks!
On 2014/02/25 16:10:46, ziran.sun wrote: > Code updated. Please review. What's changed, other than rebasing your code, since tkent's last review? If nothing, then there shouldn't be a need to re-review this CL.
On 2014/02/25 22:52:04, Ilya Sherman wrote: > On 2014/02/25 16:10:46, ziran.sun wrote: > > Code updated. Please review. > > What's changed, other than rebasing your code, since tkent's last review? If > nothing, then there shouldn't be a need to re-review this CL. Ilya, I added wrapper functions for selectionStart(), selectionEnd(), editingValue() for TextArea (see Patch set 3).
On 2014/02/26 10:46:36, ziran.sun wrote: > On 2014/02/25 22:52:04, Ilya Sherman wrote: > > On 2014/02/25 16:10:46, ziran.sun wrote: > > > Code updated. Please review. > > > > What's changed, other than rebasing your code, since tkent's last review? If > > nothing, then there shouldn't be a need to re-review this CL. > > Ilya, I added wrapper functions for selectionStart(), selectionEnd(), > editingValue() for TextArea (see Patch set 3). I see. Perhaps it would make more sense to instead create a WebTextFormControlElement class, in parallel to the existing HTMLTextFormControlElement class, that both WebInputElement and WebTextAreaElement could inherit from? This would allow the Chromium code to simply write "element.value()" rather than "is_input_element ? as_input_element.value() : as_text_area.value()". tkent@, WDYT?
> I see. Perhaps it would make more sense to instead create a > WebTextFormControlElement class, in parallel to the existing > HTMLTextFormControlElement class, that both WebInputElement and > WebTextAreaElement could inherit from? This would allow the Chromium code to > simply write "element.value()" rather than "is_input_element ? > as_input_element.value() : as_text_area.value()". tkent@, WDYT? It sounds reasonable. However I don't want to add another public class, and would like to add them to existing WebFormControlElement.
On 2014/02/27 22:44:07, tkent wrote: > > I see. Perhaps it would make more sense to instead create a > > WebTextFormControlElement class, in parallel to the existing > > HTMLTextFormControlElement class, that both WebInputElement and > > WebTextAreaElement could inherit from? This would allow the Chromium code to > > simply write "element.value()" rather than "is_input_element ? > > as_input_element.value() : as_text_area.value()". tkent@, WDYT? > > It sounds reasonable. However I don't want to add another public class, and > would like to add them to existing WebFormControlElement. Hmm, a WebFormControlElement wraps an HTMLFormControlElement, which (I think) has subclasses that are not of the HTMLTextFormControlElement type. How do you recommend adding these methods to WebFormControlElement -- asserting that the wrapped pointer is of the correct type?
On 2014/02/27 22:53:02, Ilya Sherman wrote: > On 2014/02/27 22:44:07, tkent wrote: > > > I see. Perhaps it would make more sense to instead create a > > > WebTextFormControlElement class, in parallel to the existing > > > HTMLTextFormControlElement class, that both WebInputElement and > > > WebTextAreaElement could inherit from? This would allow the Chromium code > to > > > simply write "element.value()" rather than "is_input_element ? > > > as_input_element.value() : as_text_area.value()". tkent@, WDYT? > > > > It sounds reasonable. However I don't want to add another public class, and > > would like to add them to existing WebFormControlElement. > > Hmm, a WebFormControlElement wraps an HTMLFormControlElement, which (I think) > has subclasses that are not of the HTMLTextFormControlElement type. How do you > recommend adding these methods to WebFormControlElement -- asserting that the > wrapped pointer is of the correct type? Yeah, asserting control types, or returning meaningless values. WeNode has some examples. See WebNode::isFocusable, WebNode::getElementsByTagName, etc.
On 2014/02/27 23:29:47, tkent wrote: > On 2014/02/27 22:53:02, Ilya Sherman wrote: > > On 2014/02/27 22:44:07, tkent wrote: > > > > I see. Perhaps it would make more sense to instead create a > > > > WebTextFormControlElement class, in parallel to the existing > > > > HTMLTextFormControlElement class, that both WebInputElement and > > > > WebTextAreaElement could inherit from? This would allow the Chromium code > > to > > > > simply write "element.value()" rather than "is_input_element ? > > > > as_input_element.value() : as_text_area.value()". tkent@, WDYT? > > > > > > It sounds reasonable. However I don't want to add another public class, and > > > would like to add them to existing WebFormControlElement. > > > > Hmm, a WebFormControlElement wraps an HTMLFormControlElement, which (I think) > > has subclasses that are not of the HTMLTextFormControlElement type. How do > you > > recommend adding these methods to WebFormControlElement -- asserting that the > > wrapped pointer is of the correct type? > > Yeah, asserting control types, or returning meaningless values. > WeNode has some examples. See WebNode::isFocusable, > WebNode::getElementsByTagName, etc. Ok, that makes sense. IMO asserting is slightly more developer-friendly, so I'd lean toward that. Ziran, could you please update this CL to move these methods to the WebFormControlElement interface, so that they can be shared by textarea and input elements? Thanks.
On 2014/02/27 23:34:12, Ilya Sherman wrote: > On 2014/02/27 23:29:47, tkent wrote: > > On 2014/02/27 22:53:02, Ilya Sherman wrote: > > > On 2014/02/27 22:44:07, tkent wrote: > > > > > I see. Perhaps it would make more sense to instead create a > > > > > WebTextFormControlElement class, in parallel to the existing > > > > > HTMLTextFormControlElement class, that both WebInputElement and > > > > > WebTextAreaElement could inherit from? This would allow the Chromium > code > > > to > > > > > simply write "element.value()" rather than "is_input_element ? > > > > > as_input_element.value() : as_text_area.value()". tkent@, WDYT? > > > > > > > > It sounds reasonable. However I don't want to add another public class, > and > > > > would like to add them to existing WebFormControlElement. > > > > > > Hmm, a WebFormControlElement wraps an HTMLFormControlElement, which (I > think) > > > has subclasses that are not of the HTMLTextFormControlElement type. How do > > you > > > recommend adding these methods to WebFormControlElement -- asserting that > the > > > wrapped pointer is of the correct type? > > > > Yeah, asserting control types, or returning meaningless values. > > WeNode has some examples. See WebNode::isFocusable, > > WebNode::getElementsByTagName, etc. > > Ok, that makes sense. IMO asserting is slightly more developer-friendly, so I'd > lean toward that. Ziran, could you please update this CL to move these methods > to the WebFormControlElement interface, so that they can be shared by textarea > and input elements? Thanks. CL updated. Only the methods that touched in this CL are moved to WebFormControlElement. Is it okay that I create a new issue to move all the rest common methods to WebFormControlElement and clean Chromium side code? Thanks!
Updated code. Please review. Thanks!
https://codereview.chromium.org/133443011/diff/360001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextAreaElement.cpp (left): https://codereview.chromium.org/133443011/diff/360001/Source/core/html/HTMLTe... Source/core/html/HTMLTextAreaElement.cpp:443: setFormControlValueMatchesRenderer(true); Why do you remove this? https://codereview.chromium.org/133443011/diff/360001/Source/web/WebFormContr... File Source/web/WebFormControlElement.cpp (right): https://codereview.chromium.org/133443011/diff/360001/Source/web/WebFormContr... Source/web/WebFormControlElement.cpp:89: return constUnwrap<HTMLTextAreaElement>()->innerTextValue(); Need to check hasTagName(textareaTag) too. https://codereview.chromium.org/133443011/diff/360001/Source/web/WebFormContr... Source/web/WebFormControlElement.cpp:96: return constUnwrap<HTMLTextAreaElement>()->selectionStart(); ditto. https://codereview.chromium.org/133443011/diff/360001/Source/web/WebFormContr... Source/web/WebFormControlElement.cpp:103: return constUnwrap<HTMLTextAreaElement>()->selectionEnd(); ditto. https://codereview.chromium.org/133443011/diff/360001/public/web/WebAutofillC... File public/web/WebAutofillClient.h (right): https://codereview.chromium.org/133443011/diff/360001/public/web/WebAutofillC... public/web/WebAutofillClient.h:52: virtual void textFieldDidChange(const WebFormControlElement&) { } This will break build because Chromium has "textFieldDidChange(const WebInputElement&) OVERRIDE". Please *add* textFieldDidChange(const WebFormControlElement&).
https://codereview.chromium.org/133443011/diff/360001/public/web/WebAutofillC... File public/web/WebAutofillClient.h (right): https://codereview.chromium.org/133443011/diff/360001/public/web/WebAutofillC... public/web/WebAutofillClient.h:52: virtual void textFieldDidChange(const WebFormControlElement&) { } If so then the bug is the OVERRIDE annotation on the chromium side. Implementations of cross-repo APIs should *never* have OVERRIDE annotations.
Update code as per review comments from tkent and jamesr. Please review. Thanks! https://codereview.chromium.org/133443011/diff/360001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextAreaElement.cpp (left): https://codereview.chromium.org/133443011/diff/360001/Source/core/html/HTMLTe... Source/core/html/HTMLTextAreaElement.cpp:443: setFormControlValueMatchesRenderer(true); On 2014/03/04 00:15:56, tkent wrote: > Why do you remove this? "setFormControlValueMatchesRenderer(true);" is included in "setInnerTextValue()". I wonder if we need to call it again here? "setNeedsStyleRecalc(SubtreeStyleChange);" was moved right after the setting of m_suggestedValue, I've just move back down. https://codereview.chromium.org/133443011/diff/360001/Source/web/WebFormContr... File Source/web/WebFormControlElement.cpp (right): https://codereview.chromium.org/133443011/diff/360001/Source/web/WebFormContr... Source/web/WebFormControlElement.cpp:89: return constUnwrap<HTMLTextAreaElement>()->innerTextValue(); On 2014/03/04 00:15:56, tkent wrote: > Need to check hasTagName(textareaTag) too. Done. https://codereview.chromium.org/133443011/diff/360001/Source/web/WebFormContr... Source/web/WebFormControlElement.cpp:96: return constUnwrap<HTMLTextAreaElement>()->selectionStart(); On 2014/03/04 00:15:56, tkent wrote: > ditto. Done. https://codereview.chromium.org/133443011/diff/360001/Source/web/WebFormContr... Source/web/WebFormControlElement.cpp:103: return constUnwrap<HTMLTextAreaElement>()->selectionEnd(); On 2014/03/04 00:15:56, tkent wrote: > ditto. Done. https://codereview.chromium.org/133443011/diff/360001/public/web/WebAutofillC... File public/web/WebAutofillClient.h (right): https://codereview.chromium.org/133443011/diff/360001/public/web/WebAutofillC... public/web/WebAutofillClient.h:52: virtual void textFieldDidChange(const WebFormControlElement&) { } On 2014/03/04 00:15:56, tkent wrote: > This will break build because Chromium has "textFieldDidChange(const > WebInputElement&) OVERRIDE". Please *add* textFieldDidChange(const > WebFormControlElement&). Do you mean add "textFieldDidChange(const WebInputElement&)" here? https://codereview.chromium.org/133443011/diff/360001/public/web/WebAutofillC... public/web/WebAutofillClient.h:52: virtual void textFieldDidChange(const WebFormControlElement&) { } On 2014/03/04 00:26:33, jamesr wrote: > If so then the bug is the OVERRIDE annotation on the chromium side. > Implementations of cross-repo APIs should *never* have OVERRIDE annotations. In the chromium side code for this bug, we removed OVERRIDE for both textFieldDidChange(const WebInputElement&) and textFieldDidChange(const WebFormControlElement&) The end target though is to replace textFieldDidChange(const WebInputElement&) with textFieldDidChange(const WebInputElement&). Reason for keeping both for the moment is due to the Blink roll delay. So once both Blink and Chromium side code are merged in, we'll remove textFieldDidChange(const WebInputElement&). Then we put OVERRIDE on textFieldDidChange(const WebFormControlElement&) - this is the stage I'm not sure. If I understand correctly, your mean OVERRIDE shouldn't be used here at all?
On 2014/03/04 12:12:39, ziran.sun wrote: > So once both Blink and Chromium side code are merged in, we'll remove > textFieldDidChange(const WebInputElement&). Then we put OVERRIDE on > textFieldDidChange(const WebFormControlElement&) - this is the stage I'm not > sure. If I understand correctly, your mean OVERRIDE shouldn't be used here at > all? That's right. If an API is defined in Blink and implemented in Chromium, you should never use OVERRIDE annotations. It makes mutating the API too difficult (as you've seen here) because the interface and the implementation cannot be atomically updated to match each other.
https://codereview.chromium.org/133443011/diff/370001/Source/web/ChromeClient... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/133443011/diff/370001/Source/web/ChromeClient... Source/web/ChromeClientImpl.cpp:917: m_webView->autofillClient()->textFieldDidChange(WebFormControlElement(&element)); Please call textFieldDidChange(WebInputElement(...)) too if |element| is <input>. Without it, this CL will break autofill feature temporarily. https://codereview.chromium.org/133443011/diff/370001/Source/web/WebFormContr... File Source/web/WebFormControlElement.cpp (right): https://codereview.chromium.org/133443011/diff/370001/Source/web/WebFormContr... Source/web/WebFormControlElement.cpp:91: return WebString(); Please document this behavior in WebFormControlElement.h. https://codereview.chromium.org/133443011/diff/370001/Source/web/WebFormContr... Source/web/WebFormControlElement.cpp:100: return 0; Please document this behavior in WebFormControlElement.h. https://codereview.chromium.org/133443011/diff/370001/Source/web/WebFormContr... Source/web/WebFormControlElement.cpp:109: return 0; Please document this behavior in WebFormControlElement.h.
Update code as per tkent's comments. All review comments have been addressed. Please review. Thanks! https://codereview.chromium.org/133443011/diff/370001/Source/web/ChromeClient... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/133443011/diff/370001/Source/web/ChromeClient... Source/web/ChromeClientImpl.cpp:917: m_webView->autofillClient()->textFieldDidChange(WebFormControlElement(&element)); On 2014/03/04 23:43:58, tkent wrote: > Please call textFieldDidChange(WebInputElement(...)) too if |element| is > <input>. Without it, this CL will break autofill feature temporarily. Done. https://codereview.chromium.org/133443011/diff/370001/Source/web/WebFormContr... File Source/web/WebFormControlElement.cpp (right): https://codereview.chromium.org/133443011/diff/370001/Source/web/WebFormContr... Source/web/WebFormControlElement.cpp:91: return WebString(); On 2014/03/04 23:43:58, tkent wrote: > Please document this behavior in WebFormControlElement.h. Done. https://codereview.chromium.org/133443011/diff/370001/Source/web/WebFormContr... Source/web/WebFormControlElement.cpp:100: return 0; On 2014/03/04 23:43:58, tkent wrote: > Please document this behavior in WebFormControlElement.h. Done. https://codereview.chromium.org/133443011/diff/370001/Source/web/WebFormContr... Source/web/WebFormControlElement.cpp:109: return 0; On 2014/03/04 23:43:58, tkent wrote: > Please document this behavior in WebFormControlElement.h. Done.
https://codereview.chromium.org/133443011/diff/390001/Source/core/page/Chrome... File Source/core/page/ChromeClient.h (right): https://codereview.chromium.org/133443011/diff/390001/Source/core/page/Chrome... Source/core/page/ChromeClient.h:246: // TODO(ziran.sun): This function is to be removed once both chromium and blink changes TODO(name) is not a Blink style. This should be just FIXME:. https://codereview.chromium.org/133443011/diff/390001/Source/web/ChromeClient... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/133443011/diff/390001/Source/web/ChromeClient... Source/web/ChromeClientImpl.cpp:906: // TODO(ziran.sun): This function is to be removed once both chromium and blink changes TODO() -> FIXME https://codereview.chromium.org/133443011/diff/390001/Source/web/ChromeClient... File Source/web/ChromeClientImpl.h (right): https://codereview.chromium.org/133443011/diff/390001/Source/web/ChromeClient... Source/web/ChromeClientImpl.h:180: // TODO(ziran.sun): This function is to be removed once both chromium and blink changes TODO() -> FIXME: https://codereview.chromium.org/133443011/diff/390001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/133443011/diff/390001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:1132: // TODO(ziran.sun): This function is to be removed once both chromium and blink changes TODO() -> FIXME https://codereview.chromium.org/133443011/diff/390001/public/web/WebAutofillC... File public/web/WebAutofillClient.h (right): https://codereview.chromium.org/133443011/diff/390001/public/web/WebAutofillC... public/web/WebAutofillClient.h:52: // TODO(ziran.sun): This function is to be removed once both chromium and blink changes TODO() -> FIXME https://codereview.chromium.org/133443011/diff/390001/public/web/WebFormContr... File public/web/WebFormControlElement.h (right): https://codereview.chromium.org/133443011/diff/390001/public/web/WebFormContr... public/web/WebFormControlElement.h:69: // an empty string is returned. Actually, *null* string is returned, not *empty* string.
Updated documentations as per tkent's review comments. Please review. Thanks!
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/133443011/410001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/133443011/410001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink
The CQ bit was checked by tkent@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/133443011/410001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink
The CQ bit was checked by tkent@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/133443011/410001
Message was sent while issue was closed.
Change committed as 168720 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
