[Password Manager] Change signature of PasswordAutofillAgent.FillSuggestion

[Password Manager] Change signature of PasswordAutofillAgent.FillSuggestion

Replace WebString with base::string16. AutofillAgent sends to PasswordAutofillAgent base::string16, but they are wrapped into WebString and and unwrapped to base::string16. That doesn't make sense.

Committed: https://crrev.com/c23924b8001ada9e2fccd6973a9dd32d49617ecb
Cr-Commit-Position: refs/heads/master@{#408792}

[kolos1, 2016-07-28 09:52:17]

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

[kolos1, 2016-07-28 09:52:18]

Hi Vaclav,

Please review this small CL.

Best regards,
Maxim

[vabr (Chromium), 2016-07-28 10:02:38]

I'm not sure I follow, Maxim.
In the body of FillSuggestions, the username and password arguments are passed
to setValue and nonscript_modified_values_, both of which expect WebString.

So changing the signature does not get rid of wrapping into WebString, does it?

Cheers,
Vaclav

[kolos1, 2016-07-28 10:07:16]

On 2016/07/28 10:02:38, vabr (Chromium) wrote:
> I'm not sure I follow, Maxim.
> In the body of FillSuggestions, the username and password arguments are passed
> to setValue and nonscript_modified_values_, both of which expect WebString.
> 
> So changing the signature does not get rid of wrapping into WebString, does
it?
> 
> Cheers,
> Vaclav

ah, yes, we will have to wrap string16 in setValue(). So, this change doesn't
get rid of wrapping. So, I will just left as it is. Sorry for spam. I needed
this change for another CL.

[vabr (Chromium), 2016-07-28 11:38:30]

On 2016/07/28 10:07:16, kolos1 wrote:
> On 2016/07/28 10:02:38, vabr (Chromium) wrote:
> > I'm not sure I follow, Maxim.
> > In the body of FillSuggestions, the username and password arguments are
passed
> > to setValue and nonscript_modified_values_, both of which expect WebString.
> > 
> > So changing the signature does not get rid of wrapping into WebString, does
> it?
> > 
> > Cheers,
> > Vaclav
> 
> ah, yes, we will have to wrap string16 in setValue(). So, this change doesn't
> get rid of wrapping. So, I will just left as it is. Sorry for spam. I needed
> this change for another CL.

Maxim,

Now that I read https://codereview.chromium.org/2148303005/, I think this
current CL would make sense if you rebased it on
https://codereview.chromium.org/2148303005/, because you do create a string16
out of the WebString inside FillSuggestion again.
Happy to re-review if you do the rebase, just let me know.

Cheers,
Vaclav

[kolos1, 2016-07-28 15:30:23]

Patchset #2 (id:20001) has been deleted

[kolos1, 2016-07-28 15:34:35]

Patchset #2 (id:40001) has been deleted

[kolos1, 2016-07-28 15:35:12]

Hi Vaclav,

I rebased this CL on top of https://codereview.chromium.org/2148303005/. Please
review.

Regards,
Maxim

[vabr (Chromium), 2016-07-28 17:01:03]

LGTM, thank you!
Vaclav

[kolos1, 2016-07-29 21:21:43]

The CQ bit was checked by kolos@chromium.org

[commit-bot: I haz the power, 2016-07-29 21:21:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 22:48:29]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-07-29 22:51:39]

Description was changed from

==========
[Password Manager] Change signature of PasswordAutofillAgent.FillSuggestion

Replace WebString with base::string16. AutofillAgent sends to
PasswordAutofillAgent base::string16, but they are wrapped into WebString and
and unwrapped to base::string16. That doesn't make sense.
==========

to

==========
[Password Manager] Change signature of PasswordAutofillAgent.FillSuggestion

Replace WebString with base::string16. AutofillAgent sends to
PasswordAutofillAgent base::string16, but they are wrapped into WebString and
and unwrapped to base::string16. That doesn't make sense.

Committed: https://crrev.com/c23924b8001ada9e2fccd6973a9dd32d49617ecb
Cr-Commit-Position: refs/heads/master@{#408792}
==========

[commit-bot: I haz the power, 2016-07-29 22:51:40]

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