Refactor Autofill for OOPIF, step 1: add autofillClient to WebLocalFrame

Refactor Autofill for OOPIF, step 1: add autofillClient to WebLocalFrame

Chromium side: https://codereview.chromium.org/707173004/
Goal state for Blink: https://codereview.chromium.org/707723002/

BUG=425756, 400186
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185686

[Evan Stade, 2014-11-18 20:35:12]

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

[Mike West, 2014-11-19 10:27:45]

Looking at this patch, I think I gave you bad advice on the first go. It might
make more sense to add the new path on the Chromium side, then adjust the Blink
side to use the new path (deleting the old path at the same time), and then
deleting the old path from Chromium once Blink rolls in.

That would avoid the "Does X exist? Ok, do Y. Oh, it doesn't? Ok, do Z." bits
that you've ended up adding to the ChromeClientImpl.

https://codereview.chromium.org/737853002/diff/1/Source/web/ChromeClientImpl.cpp
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/737853002/diff/1/Source/web/ChromeClientImpl....
Source/web/ChromeClientImpl.cpp:819:
webframe->autofillClient()->didAssociateFormControls(elements);
If you add an `else if (m_webView->autofillClient())` here, I believe you can
get rid of the existing method entirely. Same thing you do below.

https://codereview.chromium.org/737853002/diff/1/Source/web/ChromeClientImpl....
Source/web/ChromeClientImpl.cpp:848: // FIXME: remove.
Can you add a link to the bug here and elsewhere so that it's clear when and why
this should be removed?

[Evan Stade, 2014-11-19 22:16:41]

> Looking at this patch, I think I gave you bad advice on the first go. It might
> make more sense to add the new path on the Chromium side, then adjust the
Blink
> side to use the new path (deleting the old path at the same time), and then
> deleting the old path from Chromium once Blink rolls in.
> 
> That would avoid the "Does X exist? Ok, do Y. Oh, it doesn't? Ok, do Z." bits
> that you've ended up adding to the ChromeClientImpl.

I think the amount of temporary cruft present here is far less than the amount
of wrangling and duplication that would be necessary to get the Chrome side to
simultaneously work with both APIs. Note that the Chrome refactor is thousands
of lines, and the changes to blink are about 100 lines.

https://codereview.chromium.org/737853002/diff/1/Source/web/ChromeClientImpl.cpp
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/737853002/diff/1/Source/web/ChromeClientImpl....
Source/web/ChromeClientImpl.cpp:819:
webframe->autofillClient()->didAssociateFormControls(elements);
On 2014/11/19 10:27:45, Mike West wrote:
> If you add an `else if (m_webView->autofillClient())` here, I believe you can
> get rid of the existing method entirely. Same thing you do below.

Done.

https://codereview.chromium.org/737853002/diff/1/Source/web/ChromeClientImpl....
Source/web/ChromeClientImpl.cpp:848: // FIXME: remove.
On 2014/11/19 10:27:45, Mike West wrote:
> Can you add a link to the bug here and elsewhere so that it's clear when and
why
> this should be removed?

Done.

[Mike West, 2014-11-20 08:36:21]

Alright. LGTM, thanks for going another round.

[Evan Stade, 2014-11-20 15:14:11]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-11-20 15:15:05]

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

[commit-bot: I haz the power, 2014-11-20 15:15:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-20 15:15:32]

Failed to apply patch for Source/web/ChromeClientImpl.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/web/ChromeClientImpl.h
  Hunk #1 succeeded at 126 (offset 1 line).
  Hunk #2 FAILED at 163.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/web/ChromeClientImpl.h.rej

Patch:       Source/web/ChromeClientImpl.h
Index: Source/web/ChromeClientImpl.h
diff --git a/Source/web/ChromeClientImpl.h b/Source/web/ChromeClientImpl.h
index
f230cabb3a30cd4354fbcf9cb61cb86742191276..260fac7067bcffc949e1b0843a882fdf260a2e8b
100644
--- a/Source/web/ChromeClientImpl.h
+++ b/Source/web/ChromeClientImpl.h
@@ -125,7 +125,6 @@ public:
     virtual bool paintCustomOverhangArea(GraphicsContext*, const IntRect&,
const IntRect&, const IntRect&) override;
     virtual PassOwnPtrWillBeRawPtr<ColorChooser>
createColorChooser(LocalFrame*, ColorChooserClient*, const Color&) override;
     virtual PassRefPtr<DateTimeChooser>
openDateTimeChooser(DateTimeChooserClient*, const DateTimeChooserParameters&)
override;
-    virtual void openTextDataListChooser(HTMLInputElement&) override;
     virtual void runOpenPanel(LocalFrame*, PassRefPtr<FileChooser>) override;
     virtual void enumerateChosenDirectory(FileChooser*) override;
     virtual void setCursor(const Cursor&) override;
@@ -164,10 +163,12 @@ public:
     virtual bool requestPointerLock() override;
     virtual void requestPointerUnlock() override;
 
-    virtual void didAssociateFormControls(const
WillBeHeapVector<RefPtrWillBeMember<Element> >&) override;
+    // AutofillClient pass throughs:
+    virtual void didAssociateFormControls(const
WillBeHeapVector<RefPtrWillBeMember<Element> >&, LocalFrame*) override;
+    virtual void handleKeyboardEventOnTextField(HTMLInputElement&,
KeyboardEvent&) override;
     virtual void didChangeValueInTextField(HTMLFormControlElement&) override;
     virtual void didEndEditingOnTextField(HTMLInputElement&) override;
-    virtual void handleKeyboardEventOnTextField(HTMLInputElement&,
KeyboardEvent&) override;
+    virtual void openTextDataListChooser(HTMLInputElement&) override;
 
     virtual void didCancelCompositionOnSelectionChange() override;
     virtual void willSetInputMethodState() override;

[Evan Stade, 2014-11-20 16:29:30]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-11-20 16:29:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/737853002/80001

[commit-bot: I haz the power, 2014-11-20 18:13:45]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185686