Abstracted AutofillMsg_SetNodeText IPC out of core autofill code.

Abstracted AutofillMsg_SetNodeText IPC out of core autofill code.

BUG=302476
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235656

[jif-google, 2013-11-13 08:24:45]

ptal

https://codereview.chromium.org/69293007/diff/10008/components/autofill/conte...
File components/autofill/content/browser/autofill_driver_impl_unittest.cc
(right):

https://codereview.chromium.org/69293007/diff/10008/components/autofill/conte...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:151: bool
GetSetNodeText(base::string16* value) {
GetSetNodeText could be factored with GetAcceptDataListSuggestion in a method
that in addition to |value| takes a |msgID| and a function pointer to read the
message, but I don't think the added complexity is worth it.
WDYT?

[blundell, 2013-11-13 11:55:45]

https://codereview.chromium.org/69293007/diff/10008/components/autofill/conte...
File components/autofill/content/browser/autofill_driver_impl_unittest.cc
(right):

https://codereview.chromium.org/69293007/diff/10008/components/autofill/conte...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:151: bool
GetSetNodeText(base::string16* value) {
I think it would be worth doing a |GetValueFromMessageWithID(uint32 msgID,
base::string16* value)| and then does a switch on the message ID to read
|autofill_param|. Seems like a good middle ground between avoiding code
duplication and not wanting to deal with function pointers.

On 2013/11/13 08:24:45, jif wrote:
> GetSetNodeText could be factored with GetAcceptDataListSuggestion in a method
> that in addition to |value| takes a |msgID| and a function pointer to read the
> message, but I don't think the added complexity is worth it.
> WDYT?

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
File components/autofill/core/browser/autofill_driver.h (right):

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
components/autofill/core/browser/autofill_driver.h:80: // Tells the renderer to
complete entry.
I would say "Tells the renderer to set the node text" unless Ilya has a better
suggestion.

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
File components/autofill/core/browser/autofill_external_delegate.cc (left):

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
components/autofill/core/browser/autofill_external_delegate.cc:27: :
web_contents_(web_contents),
Yay!

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
components/autofill/core/browser/autofill_external_delegate.cc:11: #include
"components/autofill/core/common/autofill_messages.h"
You should be able to remove this now.

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
components/autofill/core/browser/autofill_external_delegate.cc:12: #include
"content/public/browser/render_view_host.h"
You should be be able to remove this now.

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
components/autofill/core/browser/autofill_external_delegate.cc:18: using
content::RenderViewHost;
You should be able to remove this now.

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
File components/autofill/core/browser/autofill_external_delegate.h (left):

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
components/autofill/core/browser/autofill_external_delegate.h:144:
AutofillDriver* autofill_driver_;  // weak
It's the AutofillDriver that owns the AutofillExternalDelegate instance now, but
I don't think it makes sense to put that comment here, as the core code doesn't
have that knowledge.

[jif-google, 2013-11-13 14:20:25]

https://codereview.chromium.org/69293007/diff/10008/components/autofill/conte...
File components/autofill/content/browser/autofill_driver_impl_unittest.cc
(right):

https://codereview.chromium.org/69293007/diff/10008/components/autofill/conte...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:151: bool
GetSetNodeText(base::string16* value) {
On 2013/11/13 11:55:45, blundell wrote:
> I think it would be worth doing a |GetValueFromMessageWithID(uint32 msgID,
> base::string16* value)| and then does a switch on the message ID to read
> |autofill_param|. Seems like a good middle ground between avoiding code
> duplication and not wanting to deal with function pointers.
> 
> On 2013/11/13 08:24:45, jif wrote:
> > GetSetNodeText could be factored with GetAcceptDataListSuggestion in a
method
> > that in addition to |value| takes a |msgID| and a function pointer to read
the
> > message, but I don't think the added complexity is worth it.
> > WDYT?
> 

Done.

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
File components/autofill/core/browser/autofill_driver.h (right):

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
components/autofill/core/browser/autofill_driver.h:80: // Tells the renderer to
complete entry.
On 2013/11/13 11:55:45, blundell wrote:
> I would say "Tells the renderer to set the node text" unless Ilya has a better
> suggestion.

I agree.

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
components/autofill/core/browser/autofill_external_delegate.cc:11: #include
"components/autofill/core/common/autofill_messages.h"
On 2013/11/13 11:55:45, blundell wrote:
> You should be able to remove this now.

Done.

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
components/autofill/core/browser/autofill_external_delegate.cc:12: #include
"content/public/browser/render_view_host.h"
On 2013/11/13 11:55:45, blundell wrote:
> You should be be able to remove this now.

Done.

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
components/autofill/core/browser/autofill_external_delegate.cc:18: using
content::RenderViewHost;
On 2013/11/13 11:55:45, blundell wrote:
> You should be able to remove this now.

Done.

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
File components/autofill/core/browser/autofill_external_delegate.h (left):

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
components/autofill/core/browser/autofill_external_delegate.h:144:
AutofillDriver* autofill_driver_;  // weak
On 2013/11/13 11:55:45, blundell wrote:
> It's the AutofillDriver that owns the AutofillExternalDelegate instance now,
but
> I don't think it makes sense to put that comment here, as the core code
doesn't
> have that knowledge.

I've removed the "Must outlive this object." part.

[jif-google, 2013-11-13 15:01:54]

On 2013/11/13 14:20:25, jif wrote:
>
https://codereview.chromium.org/69293007/diff/10008/components/autofill/conte...
> File components/autofill/content/browser/autofill_driver_impl_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/69293007/diff/10008/components/autofill/conte...
> components/autofill/content/browser/autofill_driver_impl_unittest.cc:151: bool
> GetSetNodeText(base::string16* value) {
> On 2013/11/13 11:55:45, blundell wrote:
> > I think it would be worth doing a |GetValueFromMessageWithID(uint32 msgID,
> > base::string16* value)| and then does a switch on the message ID to read
> > |autofill_param|. Seems like a good middle ground between avoiding code
> > duplication and not wanting to deal with function pointers.
> > 
> > On 2013/11/13 08:24:45, jif wrote:
> > > GetSetNodeText could be factored with GetAcceptDataListSuggestion in a
> method
> > > that in addition to |value| takes a |msgID| and a function pointer to read
> the
> > > message, but I don't think the added complexity is worth it.
> > > WDYT?
> > 
> 
> Done.
> 
>
https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
> File components/autofill/core/browser/autofill_driver.h (right):
> 
>
https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
> components/autofill/core/browser/autofill_driver.h:80: // Tells the renderer
to
> complete entry.
> On 2013/11/13 11:55:45, blundell wrote:
> > I would say "Tells the renderer to set the node text" unless Ilya has a
better
> > suggestion.
> 
> I agree.
> 
>
https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
> File components/autofill/core/browser/autofill_external_delegate.cc (right):
> 
>
https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
> components/autofill/core/browser/autofill_external_delegate.cc:11: #include
> "components/autofill/core/common/autofill_messages.h"
> On 2013/11/13 11:55:45, blundell wrote:
> > You should be able to remove this now.
> 
> Done.
> 
>
https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
> components/autofill/core/browser/autofill_external_delegate.cc:12: #include
> "content/public/browser/render_view_host.h"
> On 2013/11/13 11:55:45, blundell wrote:
> > You should be be able to remove this now.
> 
> Done.
> 
>
https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
> components/autofill/core/browser/autofill_external_delegate.cc:18: using
> content::RenderViewHost;
> On 2013/11/13 11:55:45, blundell wrote:
> > You should be able to remove this now.
> 
> Done.
> 
>
https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
> File components/autofill/core/browser/autofill_external_delegate.h (left):
> 
>
https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
> components/autofill/core/browser/autofill_external_delegate.h:144:
> AutofillDriver* autofill_driver_;  // weak
> On 2013/11/13 11:55:45, blundell wrote:
> > It's the AutofillDriver that owns the AutofillExternalDelegate instance now,
> but
> > I don't think it makes sense to put that comment here, as the core code
> doesn't
> > have that knowledge.
> 
> I've removed the "Must outlive this object." part.

+isherman for OWNERS

[blundell, 2013-11-13 15:26:48]

LGTM with nits.

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
File components/autofill/core/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
components/autofill/core/browser/autofill_external_delegate.h:134: // Provides
driver-level context to the shared code of the component. Must
This actually should stay, sorry. It *is* an assumption made by this class that
the driver outlives it.

https://codereview.chromium.org/69293007/diff/250001/components/autofill/cont...
File components/autofill/content/browser/autofill_driver_impl_unittest.cc
(right):

https://codereview.chromium.org/69293007/diff/250001/components/autofill/cont...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:130: //
message's parameter, and clears the queue of sent messages.
Mention that this is specific to the parameter being a string16 somewhere.

https://codereview.chromium.org/69293007/diff/250001/components/autofill/cont...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:131: bool
GetValueFromMessageWithID(uint32 messageID, base::string16* value) {
Actually I guess this should be something like
|GetString16ValueFromMessageWithID|.

[jif-google, 2013-11-13 16:43:02]

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
File components/autofill/core/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/69293007/diff/10008/components/autofill/core/...
components/autofill/core/browser/autofill_external_delegate.h:134: // Provides
driver-level context to the shared code of the component. Must
On 2013/11/13 15:26:49, blundell wrote:
> This actually should stay, sorry. It *is* an assumption made by this class
that
> the driver outlives it.

Ok. I removed it because I felt like it was redundant because of the "//  weak".

[Ilya Sherman, 2013-11-14 01:26:14]

Please address Colin's nits, as well as a couple more from me.  Then LG. 
Thanks!

https://codereview.chromium.org/69293007/diff/250001/components/autofill/cont...
File components/autofill/content/browser/autofill_driver_impl_unittest.cc
(right):

https://codereview.chromium.org/69293007/diff/250001/components/autofill/cont...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:261:
base::string16 inputValue(UTF8ToUTF16("barqux"));
nit: ASCIIToUTF16 (slightly cheaper)

https://codereview.chromium.org/69293007/diff/250001/components/autofill/core...
File components/autofill/core/browser/autofill_external_delegate_unittest.cc
(right):

https://codereview.chromium.org/69293007/diff/250001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate_unittest.cc:490:
base::string16 dummyString(ASCIIToUTF16("baz foo"));
nit: "dummyString" -> "dummy_string" (i.e. use hacker case)

[jif-google, 2013-11-14 13:52:03]

Thanks. ptal.

https://codereview.chromium.org/69293007/diff/250001/components/autofill/cont...
File components/autofill/content/browser/autofill_driver_impl_unittest.cc
(right):

https://codereview.chromium.org/69293007/diff/250001/components/autofill/cont...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:130: //
message's parameter, and clears the queue of sent messages.
On 2013/11/13 15:26:49, blundell wrote:
> Mention that this is specific to the parameter being a string16 somewhere.

Done.

https://codereview.chromium.org/69293007/diff/250001/components/autofill/cont...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:131: bool
GetValueFromMessageWithID(uint32 messageID, base::string16* value) {
On 2013/11/13 15:26:49, blundell wrote:
> Actually I guess this should be something like
> |GetString16ValueFromMessageWithID|. 

Done.

https://codereview.chromium.org/69293007/diff/250001/components/autofill/cont...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:261:
base::string16 inputValue(UTF8ToUTF16("barqux"));
On 2013/11/14 01:26:14, Ilya Sherman wrote:
> nit: ASCIIToUTF16 (slightly cheaper)

Done.

https://codereview.chromium.org/69293007/diff/250001/components/autofill/core...
File components/autofill/core/browser/autofill_external_delegate_unittest.cc
(right):

https://codereview.chromium.org/69293007/diff/250001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate_unittest.cc:490:
base::string16 dummyString(ASCIIToUTF16("baz foo"));
On 2013/11/14 01:26:14, Ilya Sherman wrote:
> nit: "dummyString" -> "dummy_string" (i.e. use hacker case)

Done.

[blundell, 2013-11-14 14:22:04]

SLGTM

[Ilya Sherman, 2013-11-14 22:44:15]

LGTM, thanks.

[commit-bot: I haz the power, 2013-11-14 23:48:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeanfrancoisg@chromium.org/69293007/42...

[commit-bot: I haz the power, 2013-11-15 02:08:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeanfrancoisg@chromium.org/69293007/42...

[commit-bot: I haz the power, 2013-11-15 04:11:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeanfrancoisg@chromium.org/69293007/42...

[commit-bot: I haz the power, 2013-11-15 07:50:48]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-15 07:57:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeanfrancoisg@chromium.org/69293007/42...

[commit-bot: I haz the power, 2013-11-15 09:58:36]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 10:07:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeanfrancoisg@chromium.org/69293007/42...

[commit-bot: I haz the power, 2013-11-15 12:19:17]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 12:22:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeanfrancoisg@chromium.org/69293007/42...

[commit-bot: I haz the power, 2013-11-15 14:32:43]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 15:46:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeanfrancoisg@chromium.org/69293007/42...

[commit-bot: I haz the power, 2013-11-15 22:13:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeanfrancoisg@chromium.org/69293007/42...

[commit-bot: I haz the power, 2013-11-16 00:46:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeanfrancoisg@chromium.org/69293007/42...

[commit-bot: I haz the power, 2013-11-18 08:09:31]

Change committed as 235656