[Autofill] Remove datalist flash when there is an existing entry.

[Autofill] Remove datalist flash when there is an existing entry.

When clicking the datalist arrow the full list would briefly flash before it was filtered down according to the existing entry.

We now filter in all cases on the existing entry. A separate bug will be filed for autofill/datalist interaction when clicking the arrow.

BUG=501161
TEST=manual

Committed: https://crrev.com/523de48cdc8b3e2e1be7a941960b9c168e67b882
Cr-Commit-Position: refs/heads/master@{#360626}

[Mathieu, 2015-11-19 15:41:09]

Description was changed from

==========
[Autofill] Remove datalist flash when there is an existing entry.

As described in the bug, the full list would briefly flash before it was
filtered
down according to the existing entry.

BUG=501161
==========

to

==========
[Autofill] Remove datalist flash when there is an existing entry.

When clicking the datalist arrow the full list would briefly flash before it was
filtered down according to the existing entry.

We now filter in all cases on the existing entry. A separate bug will be filed
for autofill/datalist interaction when clicking the arrow.

BUG=501161
TEST=manual
==========

[Mathieu, 2015-11-19 15:41:09]

mathp@chromium.org changed reviewers:
+ estade@chromium.org

[Mathieu, 2015-11-19 15:41:29]

Patchset #1 (id:1) has been deleted

[Mathieu, 2015-11-19 15:41:52]

PTAL.

[vabr (Chromium), 2015-11-19 16:31:19]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2015-11-19 16:31:20]

LGTM, this seems to match the conclusion from the bug.

2 comments, they are optional, because it is an old code. But since you are
touching those areas anyway, I thought I would mention it.

Cheers,
Vaclav

https://codereview.chromium.org/1455093004/diff/20001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/1455093004/diff/20001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:92: prefix, comma,
base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
optional:
comma -> ASCIITOUTF16(",")

https://codereview.chromium.org/1455093004/diff/20001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:93: if (parts.size() > 0)
{
optional: !parts.empty()

(This is similar to using .count() for checking whether there is an element,
which I mentioned in the other CL.)

[Evan Stade, 2015-11-19 16:45:12]

lgtm

https://codereview.chromium.org/1455093004/diff/20001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/1455093004/diff/20001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:93: if (parts.size() > 0)
{
On 2015/11/19 16:31:20, vabr (Chromium) wrote:
> optional: !parts.empty()
> 
> (This is similar to using .count() for checking whether there is an element,
> which I mentioned in the other CL.)

I don't see how it's similar to that one, because there's no extra work being
done here. But I agree .empty is more readable.

[vabr (Chromium), 2015-11-19 16:50:24]

https://codereview.chromium.org/1455093004/diff/20001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/1455093004/diff/20001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:93: if (parts.size() > 0)
{
On 2015/11/19 16:45:12, Evan Stade wrote:
> On 2015/11/19 16:31:20, vabr (Chromium) wrote:
> > optional: !parts.empty()
> > 
> > (This is similar to using .count() for checking whether there is an element,
> > which I mentioned in the other CL.)
> 
> I don't see how it's similar to that one, because there's no extra work being
> done here. But I agree .empty is more readable.

Sorry that I was not clear. I meant using .empty() instead of .size() for
emptiness it as a general practice: There is no extra work for this particular
container (vector), but were it a linked list, for example, there would be extra
work. And .empty() will never be more work then .size().
But the readability yu point out is for me even more important. Good that
performance and readabilty are here aligned. :)

[Mathieu, 2015-11-19 16:59:43]

Thanks both.

https://codereview.chromium.org/1455093004/diff/20001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/1455093004/diff/20001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:92: prefix, comma,
base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
On 2015/11/19 16:31:20, vabr (Chromium) wrote:
> optional:
> comma -> ASCIITOUTF16(",")

Done.

https://codereview.chromium.org/1455093004/diff/20001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:93: if (parts.size() > 0)
{
On 2015/11/19 16:31:20, vabr (Chromium) wrote:
> optional: !parts.empty()
> 
> (This is similar to using .count() for checking whether there is an element,
> which I mentioned in the other CL.)

Done.

[Mathieu, 2015-11-19 17:00:05]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2015-11-19 17:00:05]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
estade@chromium.org
Link to the patchset: https://codereview.chromium.org/1455093004/#ps40001
(title: "addressed comments")

[commit-bot: I haz the power, 2015-11-19 17:00:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1455093004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1455093004/40001

[commit-bot: I haz the power, 2015-11-19 18:08:53]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-11-19 18:10:02]

Patchset 2 (id:??) landed as
https://crrev.com/523de48cdc8b3e2e1be7a941960b9c168e67b882
Cr-Commit-Position: refs/heads/master@{#360626}