Move filtering logic in GetDataListSuggestions() to Blink. (issue 2174303002 by tkent@chromium.org)

0 views
Skip to first unread message

tk...@chromium.org

unread,
Jul 25, 2016, 9:19:36 AM7/25/16
to isherman...@chromium.org, vabr+r...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, browser-comp...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, estade...@chromium.org, j...@chromium.org, jdonnelly+a...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, rouslan+...@chromium.org, vabr+watchl...@chromium.org
Reviewers: Ilya Sherman, vabr (Chromium)
CL: https://codereview.chromium.org/2174303002/

Message:
isherman@, or vabr@, would you review this please?
This is a preparation for crbug.com/153991 and crbug.com/607097.


Description:
Move filtering logic in GetDataListSuggestions() to Blink.

Add blink::WebInputElement::filteredDataListOptions(), and it implements the
filtering logic in GetDataListSuggestions().
The logic should be in Blink because it affects a web-platform behavior.

This CL doesn't have behavior changes.

BUG=153991

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+104, -30 lines):
M components/autofill/content/renderer/autofill_agent.cc
M third_party/WebKit/Source/web/WebInputElement.cpp
A third_party/WebKit/Source/web/WebInputElementTest.cpp
M third_party/WebKit/Source/web/web.gypi
M third_party/WebKit/public/web/WebInputElement.h


va...@chromium.org

unread,
Jul 25, 2016, 9:59:54 AM7/25/16
to tk...@chromium.org, isherman...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, browser-comp...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, estade...@chromium.org, j...@chromium.org, jdonnelly+a...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, rouslan+...@chromium.org, vabr+watchl...@chromium.org
LGTM, thanks!
Vaclav


https://codereview.chromium.org/2174303002/diff/1/third_party/WebKit/Source/web/WebInputElementTest.cpp
File third_party/WebKit/Source/web/WebInputElementTest.cpp (right):

https://codereview.chromium.org/2174303002/diff/1/third_party/WebKit/Source/web/WebInputElementTest.cpp#newcode30
third_party/WebKit/Source/web/WebInputElementTest.cpp:30:
std::unique_ptr<DummyPageHolder> m_pageHolder;
nit: #include <memory> for the unique_ptr?

https://codereview.chromium.org/2174303002/

dgla...@chromium.org

unread,
Jul 25, 2016, 10:26:41 AM7/25/16
to tk...@chromium.org, isherman...@chromium.org, vabr+r...@chromium.org, esp...@chromium.org, har...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, browser-comp...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, estade...@chromium.org, j...@chromium.org, jdonnelly+a...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, rouslan+...@chromium.org, vabr+watchl...@chromium.org
I think this move is great start, but WebInputElement is not the final resting
place. Ideally, this logic should just go into HTMLInputElement and be plumbed
through to the WebInputElement. One day, we are going to get rid of the
WebElement* \o/

https://codereview.chromium.org/2174303002/

tk...@chromium.org

unread,
Jul 25, 2016, 9:53:40 PM7/25/16
to isherman...@chromium.org, vabr+r...@chromium.org, esp...@chromium.org, har...@chromium.org, dgla...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, browser-comp...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, estade...@chromium.org, j...@chromium.org, jdonnelly+a...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, rouslan+...@chromium.org, vabr+watchl...@chromium.org
On 2016/07/25 at 14:26:41, dglazkov wrote:
> I think this move is great start, but WebInputElement is not the final resting
place. Ideally, this logic should just go into HTMLInputElement and be plumbed
through to the WebInputElement. One day, we are going to get rid of the
WebElement* \o/

Yes. I implemented it in WebInputElement because I thought implementation in
HTMLInputElement needed an extra vector copy. However, I found such extra copy
wasn't necessary. Patch Set 3 moved the implementation to HTMLInputElement.


https://codereview.chromium.org/2174303002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 25, 2016, 11:07:34 PM7/25/16
to tk...@chromium.org, isherman...@chromium.org, vabr+r...@chromium.org, esp...@chromium.org, har...@chromium.org, dgla...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, browser-comp...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, estade...@chromium.org, j...@chromium.org, jdonnelly+a...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, rouslan+...@chromium.org, vabr+watchl...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 26, 2016, 12:39:24 AM7/26/16
to tk...@chromium.org, isherman...@chromium.org, vabr+r...@chromium.org, esp...@chromium.org, har...@chromium.org, dgla...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, browser-comp...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, estade...@chromium.org, j...@chromium.org, jdonnelly+a...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, rouslan+...@chromium.org, vabr+watchl...@chromium.org
Committed patchset #3 (id:40001)

https://codereview.chromium.org/2174303002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 26, 2016, 12:40:39 AM7/26/16
to tk...@chromium.org, isherman...@chromium.org, vabr+r...@chromium.org, esp...@chromium.org, har...@chromium.org, dgla...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, browser-comp...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, estade...@chromium.org, j...@chromium.org, jdonnelly+a...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, rouslan+...@chromium.org, vabr+watchl...@chromium.org
Patchset 3 (id:??) landed as
https://crrev.com/7b8a38bd4485221cc71355c22e86c3e4c5b2a970
Cr-Commit-Position: refs/heads/master@{#407709}

https://codereview.chromium.org/2174303002/

dgla...@chromium.org

unread,
Jul 26, 2016, 12:51:22 AM7/26/16
to tk...@chromium.org, isherman...@chromium.org, vabr+r...@chromium.org, esp...@chromium.org, har...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, browser-comp...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, estade...@chromium.org, j...@chromium.org, jdonnelly+a...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, rouslan+...@chromium.org, vabr+watchl...@chromium.org
On 2016/07/26 at 04:40:39, commit-bot wrote:
> Patchset 3 (id:??) landed as
https://crrev.com/7b8a38bd4485221cc71355c22e86c3e4c5b2a970
> Cr-Commit-Position: refs/heads/master@{#407709}

va...@chromium.org

unread,
Jul 26, 2016, 8:05:03 AM7/26/16
to tk...@chromium.org, isherman...@chromium.org, esp...@chromium.org, har...@chromium.org, dgla...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, browser-comp...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, estade...@chromium.org, j...@chromium.org, jdonnelly+a...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, rouslan+...@chromium.org, vabr+watchl...@chromium.org
Still LGTM, just 2 post-landing optional nits.
Cheers,
Vaclav


https://codereview.chromium.org/2174303002/diff/40001/third_party/WebKit/Source/core/html/HTMLInputElement.h
File third_party/WebKit/Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/2174303002/diff/40001/third_party/WebKit/Source/core/html/HTMLInputElement.h#newcode211
third_party/WebKit/Source/core/html/HTMLInputElement.h:211: //
Associated <datalsit> options which match to the current INPUT value.
typo: datalsit -> datalist

https://codereview.chromium.org/2174303002/diff/40001/third_party/WebKit/public/web/WebInputElement.h
File third_party/WebKit/public/web/WebInputElement.h (right):

https://codereview.chromium.org/2174303002/diff/40001/third_party/WebKit/public/web/WebInputElement.h#newcode80
third_party/WebKit/public/web/WebInputElement.h:80: // Associated
<datalsit> options which match to the current INPUT value.
typo: datalsit -> datalist

https://codereview.chromium.org/2174303002/

tk...@chromium.org

unread,
Jul 26, 2016, 11:55:14 PM7/26/16
to isherman...@chromium.org, vabr+r...@chromium.org, esp...@chromium.org, har...@chromium.org, dgla...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, browser-comp...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, estade...@chromium.org, j...@chromium.org, jdonnelly+a...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, rouslan+...@chromium.org, vabr+watchl...@chromium.org
Reply all
Reply to author
Forward
0 new messages