Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10307004:
Support datalist UI for <input type=email multiple> (Closed)
|
Created:
8 years, 7 months ago by keishi Modified:
8 years, 7 months ago Reviewers:
Ilya Sherman CC:
chromium-reviews, dhollowa+watch_chromium.org, brettw-cc_chromium.org, Ilya Sherman, dyu1, darin-cc_chromium.org, tkent Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupport datalist UI for <input type=email multiple>
BUG=126459
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137838
Patch Set 1 #Patch Set 2 : Updated #
Total comments: 11
Patch Set 3 : fix indentation and include #
Total comments: 1
Patch Set 4 : Using newly added WebInputType::setEditingValue #
Total comments: 9
Patch Set 5 : Fixed nits #Patch Set 6 : Fixed nits #
Total comments: 4
Patch Set 7 : Fixed nits #Patch Set 8 : fix varialbe name #
Messages
Total messages: 20 (0 generated)
I am adding the necessary WebKit API in these patches https://bugs.webkit.org/show_bug.cgi?id=85353 https://bugs.webkit.org/show_bug.cgi?id=85355 Here is a chromium Mac build with the patch applied https://docs.google.com/open?id=0BwRi59l_ri74ZHVKVnhaYjN3SjA
https://chromiumcodereview.appspot.com/10307004/diff/2001/chrome/renderer/aut... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/10307004/diff/2001/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:48: #include <iostream> nit: Did you mean to add this #include, or was it for debugging purposes? If you meant to add it, this should go on line 7 (preceding the other #includes), and should be followed by a blank line (separating it from the #includes of Chromium code, as opposed to system library code). https://chromiumcodereview.appspot.com/10307004/diff/2001/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:64: string16 prefix = element.editingValue(); What is the difference between element.value() and element.editingValue()? https://chromiumcodereview.appspot.com/10307004/diff/2001/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:73: !option.isNull(); option = options.nextItem().to<WebOptionElement>()) { nit: This line should be indented one more space. (Not related to this CL, but might as well fix it while you're here.) https://chromiumcodereview.appspot.com/10307004/diff/2001/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:629: substring); Is there really no more direct way to achieve the desired effect? Passing strings as method calls feels very off for C++ code. https://chromiumcodereview.appspot.com/10307004/diff/2001/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:633: weak_ptr_factory_.GetWeakPtr())); Hmm, I don't understand the motivation for the change to this method. What's wrong with calling into node->setValue()?
http://codereview.chromium.org/10307004/diff/2001/chrome/renderer/autofill/au... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/10307004/diff/2001/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:48: #include <iostream> On 2012/05/07 23:34:52, Ilya Sherman wrote: > nit: Did you mean to add this #include, or was it for debugging purposes? If > you meant to add it, this should go on line 7 (preceding the other #includes), > and should be followed by a blank line (separating it from the #includes of > Chromium code, as opposed to system library code). Sorry my mistake. http://codereview.chromium.org/10307004/diff/2001/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:64: string16 prefix = element.editingValue(); On 2012/05/07 23:34:52, Ilya Sherman wrote: > What is the difference between element.value() and element.editingValue()? There is no difference for <input type=text>. But for others types like email, element.value() returns the sanitized value and editingValue() will return the raw visible value. For example, if you type in "a@a.com, b" (separated with comma and space) element.value() returns "a@a.com,b" without the extra space element.selectionEnd() returns the position with the extra space so you can't use element.value().length() and element.selectionEnd() to tell if the caret is at the end or not. http://codereview.chromium.org/10307004/diff/2001/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:73: !option.isNull(); option = options.nextItem().to<WebOptionElement>()) { On 2012/05/07 23:34:52, Ilya Sherman wrote: > nit: This line should be indented one more space. (Not related to this CL, but > might as well fix it while you're here.) ok. http://codereview.chromium.org/10307004/diff/2001/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:629: substring); On 2012/05/07 23:34:52, Ilya Sherman wrote: > Is there really no more direct way to achieve the desired effect? Passing > strings as method calls feels very off for C++ code. There is an inner text field that's being edited but we don't want to expose that in the WebKit API. http://codereview.chromium.org/10307004/diff/2001/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:633: weak_ptr_factory_.GetWeakPtr())); On 2012/05/07 23:34:52, Ilya Sherman wrote: > Hmm, I don't understand the motivation for the change to this method. What's > wrong with calling into node->setValue()? if you do setValue("a@.com, b@b.com"), the text field will show "a@a.com,b@b.com". I did this because it bugged me that the space kept vanishing.
http://codereview.chromium.org/10307004/diff/2001/chrome/renderer/autofill/au... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/10307004/diff/2001/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:629: substring); On 2012/05/09 01:38:21, keishi1 wrote: > On 2012/05/07 23:34:52, Ilya Sherman wrote: > > Is there really no more direct way to achieve the desired effect? Passing > > strings as method calls feels very off for C++ code. > > There is an inner text field that's being edited but we don't want to expose > that in the WebKit API. If we are exposing |editingValue()|, it makes sense to me to expose |setEditingValue()| as well. What is the motivation for not wanting to expose it, and instead making clients jump through hoops for this sort of functionality? http://codereview.chromium.org/10307004/diff/7001/chrome/renderer/autofill/au... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/10307004/diff/7001/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:625: return; nit: This is no longer used...
On 2012/05/09 02:38:58, Ilya Sherman wrote: > http://codereview.chromium.org/10307004/diff/2001/chrome/renderer/autofill/au... > File chrome/renderer/autofill/autofill_agent.cc (right): > > http://codereview.chromium.org/10307004/diff/2001/chrome/renderer/autofill/au... > chrome/renderer/autofill/autofill_agent.cc:629: substring); > On 2012/05/09 01:38:21, keishi1 wrote: > > On 2012/05/07 23:34:52, Ilya Sherman wrote: > > > Is there really no more direct way to achieve the desired effect? Passing > > > strings as method calls feels very off for C++ code. > > > > There is an inner text field that's being edited but we don't want to expose > > that in the WebKit API. > > If we are exposing |editingValue()|, it makes sense to me to expose > |setEditingValue()| as well. What is the motivation for not wanting to expose > it, and instead making clients jump through hoops for this sort of > functionality? I have added WebInputType::setEditingValue
LGTM, thanks https://chromiumcodereview.appspot.com/10307004/diff/10001/chrome/renderer/au... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/10307004/diff/10001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:11: #include "base/string_split.h" nit: These should precede the time.h include. https://chromiumcodereview.appspot.com/10307004/diff/10001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:74: if (!StartsWith(option.value(), prefix, false) || option.value() == prefix) nit: Do you really need the "option.value() == prefix" check? It seems like StartsWith() should already cover that. https://chromiumcodereview.appspot.com/10307004/diff/10001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:252: } nit: Please decompose this block to a helper method. https://chromiumcodereview.appspot.com/10307004/diff/10001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:627: return; nit: Is this code still needed? If so, please add a comment describing why it's necessary -- it's currently not clear to me whether this is here because setEditingValue() somehow implicitly depends on there being a focused frame...
http://codereview.chromium.org/10307004/diff/10001/chrome/renderer/autofill/a... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/10307004/diff/10001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:11: #include "base/string_split.h" On 2012/05/15 21:12:39, Ilya Sherman wrote: > nit: These should precede the time.h include. Done. http://codereview.chromium.org/10307004/diff/10001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:74: if (!StartsWith(option.value(), prefix, false) || option.value() == prefix) On 2012/05/15 21:12:39, Ilya Sherman wrote: > nit: Do you really need the "option.value() == prefix" check? It seems like > StartsWith() should already cover that. This removes suggestions that completely matches the input from the list. http://codereview.chromium.org/10307004/diff/10001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:252: } On 2012/05/15 21:12:39, Ilya Sherman wrote: > nit: Please decompose this block to a helper method. Done. http://codereview.chromium.org/10307004/diff/10001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:627: return; On 2012/05/15 21:12:39, Ilya Sherman wrote: > nit: Is this code still needed? If so, please add a comment describing why it's > necessary -- it's currently not clear to me whether this is here because > setEditingValue() somehow implicitly depends on there being a focused frame... Done.
(Still LGTM) https://chromiumcodereview.appspot.com/10307004/diff/10001/chrome/renderer/au... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/10307004/diff/10001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:74: if (!StartsWith(option.value(), prefix, false) || option.value() == prefix) On 2012/05/16 11:44:07, keishi1 wrote: > On 2012/05/15 21:12:39, Ilya Sherman wrote: > > nit: Do you really need the "option.value() == prefix" check? It seems like > > StartsWith() should already cover that. > > This removes suggestions that completely matches the input from the list. Ah, I see, that makes sense. Thanks. https://chromiumcodereview.appspot.com/10307004/diff/14003/chrome/renderer/au... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/10307004/diff/14003/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:632: return; nit: I still don't see why we're checking for the WebView -- will setEditingValue() fail if GetWebView() returns false? If so, please add a comment mentioning this. https://chromiumcodereview.appspot.com/10307004/diff/14003/chrome/renderer/au... File chrome/renderer/autofill/autofill_agent.h (right): https://chromiumcodereview.appspot.com/10307004/diff/14003/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.h:152: void ApplyDataListSuggestionToValue(const string16& value); nit: Perhaps "AcceptDataListSuggestion(const string16& suggested_value)" or "ApplyDataListSuggestion(const string16& suggested_value)" or something like that?
Thanks! http://codereview.chromium.org/10307004/diff/14003/chrome/renderer/autofill/a... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/10307004/diff/14003/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:632: return; On 2012/05/16 18:05:31, Ilya Sherman wrote: > nit: I still don't see why we're checking for the WebView -- will > setEditingValue() fail if GetWebView() returns false? If so, please add a > comment mentioning this. Yes. Sorry I wasn't paying attention. Done. http://codereview.chromium.org/10307004/diff/14003/chrome/renderer/autofill/a... File chrome/renderer/autofill/autofill_agent.h (right): http://codereview.chromium.org/10307004/diff/14003/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.h:152: void ApplyDataListSuggestionToValue(const string16& value); On 2012/05/16 18:05:31, Ilya Sherman wrote: > nit: Perhaps "AcceptDataListSuggestion(const string16& suggested_value)" or > "ApplyDataListSuggestion(const string16& suggested_value)" or something like > that? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/10307004/6005
Try job failure for 10307004-6005 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/10307004/6005
Try job failure for 10307004-6005 (retry) on mac_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/10307004/6005
Try job failure for 10307004-6005 (retry) on mac_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/10307004/10004
Try job failure for 10307004-10004 on win for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/10307004/10004
Change committed as 137838 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
