Datalist UI (WebKit part)

Datalist UI (WebKit part)


BUG=
TEST=

[Ilya Sherman, 2012-04-11 00:27:52]

Just a handful of nits...

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/feature...
File Source/WebKit/chromium/features.gypi (right):

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/feature...
Source/WebKit/chromium/features.gypi:42: 'ENABLE_DATALIST=1',
Why do we need a flag for this, rather than having it be always on?

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Aut...
File Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp (left):

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Aut...
Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp:56: , m_selectedIndex(-1)
nit: We should still initialize m_selectedIndex.

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Aut...
File Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp (right):

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Aut...
Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp:70: ASSERT(listIndex >= 0
&& static_cast<size_t>(listIndex) < m_names.size());
nit: No need to check nonnegativity nor to cast -- the listIndex is already
unsigned.

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Aut...
Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp:76: ASSERT(listIndex >= 0
&& static_cast<size_t>(listIndex) < m_labels.size());
nit: Ditto

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Aut...
Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp:82: ASSERT(listIndex >= 0
&& static_cast<size_t>(listIndex) < m_icons.size());
nit: Ditto

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Aut...
Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp:91:
ASSERT(static_cast<unsigned>(listIndex) < m_names.size());
nit: No need to cast, the listIndex is already unsigned.

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Aut...
Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp:101: return
m_itemIDs[listIndex] > 0 || // Is autofill entry?
It's not currently possible to remove Autofill entries.  I'm also not sure about
password suggestions.

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Web...
File Source/WebKit/chromium/src/WebDataListElement.cpp (right):

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Web...
Source/WebKit/chromium/src/WebDataListElement.cpp:2: * Copyright (C) 2009 Google
Inc. All rights reserved.
nit: 2012

[keishi, 2012-04-11 12:13:11]

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/feature...
File Source/WebKit/chromium/features.gypi (right):

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/feature...
Source/WebKit/chromium/features.gypi:42: 'ENABLE_DATALIST=1',
On 2012/04/11 00:27:52, Ilya Sherman wrote:
> Why do we need a flag for this, rather than having it be always on?

This flag is already used in WebCore.
WebKit ports that do not support datalist need to disable it because we don't
want to trigger feature detection.

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Aut...
File Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp (left):

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Aut...
Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp:56: , m_selectedIndex(-1)
On 2012/04/11 00:27:52, Ilya Sherman wrote:
> nit: We should still initialize m_selectedIndex.

Done.

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Aut...
File Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp (right):

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Aut...
Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp:70: ASSERT(listIndex >= 0
&& static_cast<size_t>(listIndex) < m_names.size());
On 2012/04/11 00:27:52, Ilya Sherman wrote:
> nit: No need to check nonnegativity nor to cast -- the listIndex is already
> unsigned.

Done.

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Aut...
Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp:76: ASSERT(listIndex >= 0
&& static_cast<size_t>(listIndex) < m_labels.size());
On 2012/04/11 00:27:52, Ilya Sherman wrote:
> nit: Ditto

Done.

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Aut...
Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp:82: ASSERT(listIndex >= 0
&& static_cast<size_t>(listIndex) < m_icons.size());
On 2012/04/11 00:27:52, Ilya Sherman wrote:
> nit: Ditto

Done.

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Aut...
Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp:91:
ASSERT(static_cast<unsigned>(listIndex) < m_names.size());
On 2012/04/11 00:27:52, Ilya Sherman wrote:
> nit: No need to cast, the listIndex is already unsigned.

Done.

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Aut...
Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp:101: return
m_itemIDs[listIndex] > 0 || // Is autofill entry?
On 2012/04/11 00:27:52, Ilya Sherman wrote:
> It's not currently possible to remove Autofill entries.  I'm also not sure
about
> password suggestions.

Done. I'll match the current behavior.

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Web...
File Source/WebKit/chromium/src/WebDataListElement.cpp (right):

http://codereview.chromium.org/10037002/diff/1/Source/WebKit/chromium/src/Web...
Source/WebKit/chromium/src/WebDataListElement.cpp:2: * Copyright (C) 2009 Google
Inc. All rights reserved.
On 2012/04/11 00:27:52, Ilya Sherman wrote:
> nit: 2012

Done.

[Ilya Sherman, 2012-04-11 18:20:31]

LG, though you'll want to actually upload this to https://bugs.webkit.org and
get it reviewed by Darin Fisher.  Thanks :)