[Autofill]Enable mojo AutofillDriver for android_webview.

[Autofill]Enable mojo AutofillDriver for android_webview.

BUG=662040
Committed: https://crrev.com/e404109db1d011479ea0fee59c5d415f44d73b22
Cr-Commit-Position: refs/heads/master@{#433418}

[leonhsl(Using Gerrit), 2016-11-10 05:32:41]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 05:32:56]

Dry run: 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-11-10 07:13:17]

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

[commit-bot: I haz the power, 2016-11-10 07:13:18]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-11-10 09:38:43]

leon.han@intel.com changed reviewers:
+ boliu@chromium.org, vabr@chromium.org

[leonhsl(Using Gerrit), 2016-11-10 09:38:43]

Hi, would you PTAL at this? Seems red bots are not caused by changes of this CL.

AwContents::InitAutofillIfNecessary() creates
autofill::ContentAutofillDriverFactory, so we need to register mojo
AutofillDriver interface for every render frame, just like what is being done
for chrome content browser client:
https://cs.chromium.org/chromium/src/chrome/browser/chrome_content_browser_cl...

[vabr (Chromium), 2016-11-10 10:13:22]

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

[vabr (Chromium), 2016-11-10 10:13:22]

Hi Han Leon;

I am not completely sure whether the current situation is working as intended,
therefore adding Mathieu to help me double-check:

Is autofill supposed to work in WebView? If so, which parts exactly
(autocomplete, address autofill, credit card autofill)? I know that we don't
allow password manager in WebView on purpose: limited usefulness due to the
separated profile (unlike Custom Tabs), security issues with the embedding app
having control over the WebView profile.

Let's get this clarified before changing the code.

Cheers,
Vaclav

[boliu, 2016-11-10 18:00:21]

boliu@chromium.org changed reviewers:
+ sgurun@chromium.org

[boliu, 2016-11-10 18:00:21]

On 2016/11/10 10:13:22, vabr (Chromium) wrote:
> Hi Han Leon;
> 
> I am not completely sure whether the current situation is working as intended,
> therefore adding Mathieu to help me double-check:
> 
> Is autofill supposed to work in WebView? If so, which parts exactly
> (autocomplete, address autofill, credit card autofill)? I know that we don't
> allow password manager in WebView on purpose: limited usefulness due to the
> separated profile (unlike Custom Tabs), security issues with the embedding app
> having control over the WebView profile.
> 
> Let's get this clarified before changing the code.
> 
> Cheers,
> Vaclav

+sgurun to answer which parts of autofill is supposed to work in webview

[Mathieu, 2016-11-10 18:11:35]

On 2016/11/10 18:00:21, boliu wrote:
> On 2016/11/10 10:13:22, vabr (Chromium) wrote:
> > Hi Han Leon;
> > 
> > I am not completely sure whether the current situation is working as
intended,
> > therefore adding Mathieu to help me double-check:
> > 
> > Is autofill supposed to work in WebView? If so, which parts exactly
> > (autocomplete, address autofill, credit card autofill)? I know that we don't
> > allow password manager in WebView on purpose: limited usefulness due to the
> > separated profile (unlike Custom Tabs), security issues with the embedding
app
> > having control over the WebView profile.
> > 
> > Let's get this clarified before changing the code.
> > 
> > Cheers,
> > Vaclav
> 
> +sgurun to answer which parts of autofill is supposed to work in webview

Only Autocomplete is supposed to work.

[leonhsl(Using Gerrit), 2016-11-11 03:17:26]

On 2016/11/10 18:11:35, Mathieu Perreault wrote:
> On 2016/11/10 18:00:21, boliu wrote:
> > On 2016/11/10 10:13:22, vabr (Chromium) wrote:
> > > Hi Han Leon;
> > > 
> > > I am not completely sure whether the current situation is working as
> intended,
> > > therefore adding Mathieu to help me double-check:
> > > 
> > > Is autofill supposed to work in WebView? If so, which parts exactly
> > > (autocomplete, address autofill, credit card autofill)? I know that we
don't
> > > allow password manager in WebView on purpose: limited usefulness due to
the
> > > separated profile (unlike Custom Tabs), security issues with the embedding
> app
> > > having control over the WebView profile.
> > > 
> > > Let's get this clarified before changing the code.
> > > 
> > > Cheers,
> > > Vaclav
> > 
> > +sgurun to answer which parts of autofill is supposed to work in webview
> 
> Only Autocomplete is supposed to work.

I suppose, at content level, for both legacy IPC and current mojo world, all
autofill IPCs(between render frame host and render frame) should just happen the
same way , no matter the embedder is Chrome or Android WebView. But, the
embedder can decide how to handle them, for example, WebView will handle
Autocomplete related only, and just ignore others. # I noticed several
NOTIMPLEMENTED functions in android_webview/native/aw_autofill_client.cc.

This CL is to enable content level autofill IPC communications for the embedder
Android WebView, like what we did for the embedder Chrome.

[boliu, 2016-11-11 05:13:40]

On 2016/11/11 03:17:26, leonhsl wrote:
> On 2016/11/10 18:11:35, Mathieu Perreault wrote:
> > On 2016/11/10 18:00:21, boliu wrote:
> > > On 2016/11/10 10:13:22, vabr (Chromium) wrote:
> > > > Hi Han Leon;
> > > > 
> > > > I am not completely sure whether the current situation is working as
> > intended,
> > > > therefore adding Mathieu to help me double-check:
> > > > 
> > > > Is autofill supposed to work in WebView? If so, which parts exactly
> > > > (autocomplete, address autofill, credit card autofill)? I know that we
> don't
> > > > allow password manager in WebView on purpose: limited usefulness due to
> the
> > > > separated profile (unlike Custom Tabs), security issues with the
embedding
> > app
> > > > having control over the WebView profile.
> > > > 
> > > > Let's get this clarified before changing the code.
> > > > 
> > > > Cheers,
> > > > Vaclav
> > > 
> > > +sgurun to answer which parts of autofill is supposed to work in webview
> > 
> > Only Autocomplete is supposed to work.
> 
> I suppose, at content level, for both legacy IPC and current mojo world, all
> autofill IPCs(between render frame host and render frame) should just happen
the
> same way , no matter the embedder is Chrome or Android WebView. But, the
> embedder can decide how to handle them, for example, WebView will handle
> Autocomplete related only, and just ignore others. # I noticed several
> NOTIMPLEMENTED functions in android_webview/native/aw_autofill_client.cc.
> 
> This CL is to enable content level autofill IPC communications for the
embedder
> Android WebView, like what we did for the embedder Chrome.

That makes sense to me. But I don't know the autofill code at all. Any autofill
owner want to approve this first?

[vabr (Chromium), 2016-11-11 09:07:42]

Thanks for the explanations. I asked one more thing below to understand better.

My concern is that the filtering of which kinds of autofill will happen should
be done in the browser process, instead of sending all the data to the renderer
and hoping it is not compromised and will pick just what it should pick. If in
doubt, we could ask the security team (e.g., anybody from
//ipc/SECURITY_OWNERS).

Cheers,
Vaclav

https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
android_webview/browser/aw_content_browser_client.cc:548:
base::Bind(&autofill::ContentAutofillDriverFactory::BindAutofillDriver,
When you bind this, does it mean that the browser-side will work the same in
WebView as it does in normal Chrome? In other words, would it be now left with
the renderer process to drop information it should not have access to?

[sgurun-gerrit only, 2016-11-11 22:04:31]

On 2016/11/11 09:07:42, vabr (Chromium) wrote:
> Thanks for the explanations. I asked one more thing below to understand
better.
> 
> My concern is that the filtering of which kinds of autofill will happen should
> be done in the browser process, instead of sending all the data to the
renderer
> and hoping it is not compromised and will pick just what it should pick. If in
> doubt, we could ask the security team (e.g., anybody from
> //ipc/SECURITY_OWNERS).

That actually was not a concern until recently, but now with Multiprocess
WebView becoming a goal, it should be done at the browser side.

Also the comment about which piece we support is correct. WebView has only
autocomplete. No credit card, password. Most methods are not implemented.


> 
> Cheers,
> Vaclav
> 
>
https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
> File android_webview/browser/aw_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
> android_webview/browser/aw_content_browser_client.cc:548:
> base::Bind(&autofill::ContentAutofillDriverFactory::BindAutofillDriver,
> When you bind this, does it mean that the browser-side will work the same in
> WebView as it does in normal Chrome? In other words, would it be now left with
> the renderer process to drop information it should not have access to?

[sgurun-gerrit only, 2016-11-11 22:12:56]

On 2016/11/11 22:04:31, sgurun wrote:
> On 2016/11/11 09:07:42, vabr (Chromium) wrote:
> > Thanks for the explanations. I asked one more thing below to understand
> better.
> > 
> > My concern is that the filtering of which kinds of autofill will happen
should
> > be done in the browser process, instead of sending all the data to the
> renderer
> > and hoping it is not compromised and will pick just what it should pick. If
in
> > doubt, we could ask the security team (e.g., anybody from
> > //ipc/SECURITY_OWNERS).
> 
> That actually was not a concern until recently, but now with Multiprocess
> WebView becoming a goal, it should be done at the browser side.
> 
> Also the comment about which piece we support is correct. WebView has only
> autocomplete. No credit card, password. Most methods are not implemented.
> 
> 
> > 
> > Cheers,
> > Vaclav
> > 
> >
>
https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
> > File android_webview/browser/aw_content_browser_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
> > android_webview/browser/aw_content_browser_client.cc:548:
> > base::Bind(&autofill::ContentAutofillDriverFactory::BindAutofillDriver,
> > When you bind this, does it mean that the browser-side will work the same in
> > WebView as it does in normal Chrome? In other words, would it be now left
with
> > the renderer process to drop information it should not have access to?

I will look at this monday

[leonhsl(Using Gerrit), 2016-11-17 03:24:46]

On 2016/11/11 22:12:56, sgurun wrote:
> On 2016/11/11 22:04:31, sgurun wrote:
> > On 2016/11/11 09:07:42, vabr (Chromium) wrote:
> > > Thanks for the explanations. I asked one more thing below to understand
> > better.
> > > 
> > > My concern is that the filtering of which kinds of autofill will happen
> should
> > > be done in the browser process, instead of sending all the data to the
> > renderer
> > > and hoping it is not compromised and will pick just what it should pick.
If
> in
> > > doubt, we could ask the security team (e.g., anybody from
> > > //ipc/SECURITY_OWNERS).
> > 
> > That actually was not a concern until recently, but now with Multiprocess
> > WebView becoming a goal, it should be done at the browser side.
> > 
> > Also the comment about which piece we support is correct. WebView has only
> > autocomplete. No credit card, password. Most methods are not implemented.
> > 
> > 
> > > 
> > > Cheers,
> > > Vaclav
> > > 
> > >
> >
>
https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
> > > File android_webview/browser/aw_content_browser_client.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
> > > android_webview/browser/aw_content_browser_client.cc:548:
> > > base::Bind(&autofill::ContentAutofillDriverFactory::BindAutofillDriver,
> > > When you bind this, does it mean that the browser-side will work the same
in
> > > WebView as it does in normal Chrome? In other words, would it be now left
> with
> > > the renderer process to drop information it should not have access to?
> 
> I will look at this monday

Friendly ping sgurun@, Thanks.

[sgurun-gerrit only, 2016-11-17 19:46:57]

On 2016/11/17 03:24:46, leonhsl wrote:
> On 2016/11/11 22:12:56, sgurun wrote:
> > On 2016/11/11 22:04:31, sgurun wrote:
> > > On 2016/11/11 09:07:42, vabr (Chromium) wrote:
> > > > Thanks for the explanations. I asked one more thing below to understand
> > > better.
> > > > 
> > > > My concern is that the filtering of which kinds of autofill will happen
> > should
> > > > be done in the browser process, instead of sending all the data to the
> > > renderer
> > > > and hoping it is not compromised and will pick just what it should pick.
> If
> > in
> > > > doubt, we could ask the security team (e.g., anybody from
> > > > //ipc/SECURITY_OWNERS).
> > > 
> > > That actually was not a concern until recently, but now with Multiprocess
> > > WebView becoming a goal, it should be done at the browser side.
> > > 
> > > Also the comment about which piece we support is correct. WebView has only
> > > autocomplete. No credit card, password. Most methods are not implemented.
> > > 
> > > 
> > > > 
> > > > Cheers,
> > > > Vaclav
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
> > > > File android_webview/browser/aw_content_browser_client.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
> > > > android_webview/browser/aw_content_browser_client.cc:548:
> > > > base::Bind(&autofill::ContentAutofillDriverFactory::BindAutofillDriver,
> > > > When you bind this, does it mean that the browser-side will work the
same
> in
> > > > WebView as it does in normal Chrome? In other words, would it be now
left
> > with
> > > > the renderer process to drop information it should not have access to?
> > 
> > I will look at this monday
> 
> Friendly ping sgurun@, Thanks.

sorry, was a very busy week. working on it today.

[sgurun-gerrit only, 2016-11-18 07:34:23]

On 2016/11/17 19:46:57, sgurun wrote:
> On 2016/11/17 03:24:46, leonhsl wrote:
> > On 2016/11/11 22:12:56, sgurun wrote:
> > > On 2016/11/11 22:04:31, sgurun wrote:
> > > > On 2016/11/11 09:07:42, vabr (Chromium) wrote:
> > > > > Thanks for the explanations. I asked one more thing below to
understand
> > > > better.
> > > > > 
> > > > > My concern is that the filtering of which kinds of autofill will
happen
> > > should
> > > > > be done in the browser process, instead of sending all the data to the
> > > > renderer
> > > > > and hoping it is not compromised and will pick just what it should
pick.
> > If
> > > in
> > > > > doubt, we could ask the security team (e.g., anybody from
> > > > > //ipc/SECURITY_OWNERS).
> > > > 
> > > > That actually was not a concern until recently, but now with
Multiprocess
> > > > WebView becoming a goal, it should be done at the browser side.
> > > > 
> > > > Also the comment about which piece we support is correct. WebView has
only
> > > > autocomplete. No credit card, password. Most methods are not
implemented.
> > > > 
> > > > 
> > > > > 
> > > > > Cheers,
> > > > > Vaclav
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
> > > > > File android_webview/browser/aw_content_browser_client.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
> > > > > android_webview/browser/aw_content_browser_client.cc:548:
> > > > >
base::Bind(&autofill::ContentAutofillDriverFactory::BindAutofillDriver,
> > > > > When you bind this, does it mean that the browser-side will work the
> same
> > in
> > > > > WebView as it does in normal Chrome? In other words, would it be now
> left
> > > with
> > > > > the renderer process to drop information it should not have access to?
> > > 
> > > I will look at this monday
> > 
> > Friendly ping sgurun@, Thanks.
> 
> sorry, was a very busy week. working on it today.

This change fixes the recent breaks in  saveformdata cts test and other breaks
in autocomplete. I also don't see any behavior that would indicate webview is
differnt than chrome. But I am not super familiar with mojo so I will keep on
looking a little more.

[sgurun-gerrit only, 2016-11-18 23:51:28]

lgtm

I will add a bug number and CQ

https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
android_webview/browser/aw_content_browser_client.cc:548:
base::Bind(&autofill::ContentAutofillDriverFactory::BindAutofillDriver,
On 2016/11/11 09:07:42, vabr (travelling until 21 Nov) wrote:
> When you bind this, does it mean that the browser-side will work the same in
> WebView as it does in normal Chrome? In other words, would it be now left with
> the renderer process to drop information it should not have access to?

yes it works the same way as chrome. Specifically, when a form is seen, the
renderer sends a mojo message for forms seen:
https://cs.chromium.org/chromium/src/out/Debug/gen/components/autofill/conten...

which I think is the most important piece for webview.

[sgurun-gerrit only, 2016-11-18 23:52:19]

On 2016/11/18 23:51:28, sgurun wrote:
> lgtm
> 
> I will add a bug number and CQ

n/m I will dupe bugs against the existing bug here.

> 
>
https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
> File android_webview/browser/aw_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
> android_webview/browser/aw_content_browser_client.cc:548:
> base::Bind(&autofill::ContentAutofillDriverFactory::BindAutofillDriver,
> On 2016/11/11 09:07:42, vabr (travelling until 21 Nov) wrote:
> > When you bind this, does it mean that the browser-side will work the same in
> > WebView as it does in normal Chrome? In other words, would it be now left
with
> > the renderer process to drop information it should not have access to?
> 
> yes it works the same way as chrome. Specifically, when a form is seen, the
> renderer sends a mojo message for forms seen:
>
https://cs.chromium.org/chromium/src/out/Debug/gen/components/autofill/conten...
> 
> which I think is the most important piece for webview.

[sgurun-gerrit only, 2016-11-18 23:52:25]

On 2016/11/18 23:51:28, sgurun wrote:
> lgtm
> 
> I will add a bug number and CQ

n/m I will dupe bugs against the existing bug here.

> 
>
https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
> File android_webview/browser/aw_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
> android_webview/browser/aw_content_browser_client.cc:548:
> base::Bind(&autofill::ContentAutofillDriverFactory::BindAutofillDriver,
> On 2016/11/11 09:07:42, vabr (travelling until 21 Nov) wrote:
> > When you bind this, does it mean that the browser-side will work the same in
> > WebView as it does in normal Chrome? In other words, would it be now left
with
> > the renderer process to drop information it should not have access to?
> 
> yes it works the same way as chrome. Specifically, when a form is seen, the
> renderer sends a mojo message for forms seen:
>
https://cs.chromium.org/chromium/src/out/Debug/gen/components/autofill/conten...
> 
> which I think is the most important piece for webview.

[sgurun-gerrit only, 2016-11-18 23:52:34]

The CQ bit was checked by sgurun@chromium.org

[commit-bot: I haz the power, 2016-11-18 23:53:20]

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-11-19 01:22:46]

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

[commit-bot: I haz the power, 2016-11-19 01:22:47]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[leonhsl(Using Gerrit), 2016-11-19 11:32:42]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-19 11:32:52]

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

[leonhsl(Using Gerrit), 2016-11-19 12:05:31]

https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/2491103003/diff/1/android_webview/browser/aw_...
android_webview/browser/aw_content_browser_client.cc:548:
base::Bind(&autofill::ContentAutofillDriverFactory::BindAutofillDriver,
On 2016/11/18 23:51:28, sgurun wrote:
> On 2016/11/11 09:07:42, vabr (travelling until 21 Nov) wrote:
> > When you bind this, does it mean that the browser-side will work the same in
> > WebView as it does in normal Chrome? In other words, would it be now left
with
> > the renderer process to drop information it should not have access to?
> 
> yes it works the same way as chrome. Specifically, when a form is seen, the
> renderer sends a mojo message for forms seen:
>
https://cs.chromium.org/chromium/src/out/Debug/gen/components/autofill/conten...
> 
> which I think is the most important piece for webview.

Thanks sgurun@ a lot for the clarifications. As for the concern about renderer
process getting unnecessary information, I think:
  1, At least autofill mojo IPC is keeping the same logic with legacy chromium
IPC before, mojo is not making things worse. 
  2, It's true that chrome and webview have the same capability of sending
autofill mojo IPC messages, but, comparing the implementation of webview
autofill client with chrome autofill client, we can see that webview autofill
client is handling only those interested features, such as autocomplete, and is
ignoring others by empty or NOTIMPLEMENTED() functions, this means that actually
many mojo messages(except autocomplete) will never be triggered to be sent to
renderer process.

[commit-bot: I haz the power, 2016-11-19 12:06:21]

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

[commit-bot: I haz the power, 2016-11-19 12:06:22]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-11-19 12:12:21]

The CQ bit was checked by leon.han@intel.com

[commit-bot: I haz the power, 2016-11-19 12:12:31]

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-11-19 12:16:48]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-19 12:18:17]

Description was changed from

==========
[Autofill]Enable mojo AutofillDriver for android_webview.

BUG=662040
==========

to

==========
[Autofill]Enable mojo AutofillDriver for android_webview.

BUG=662040

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

[commit-bot: I haz the power, 2016-11-19 12:18:21]

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

[vabr (Chromium), 2016-11-21 10:14:08]

Thanks for the explanation, Han Leon.

I'm afraid I need to understand a bit more though. Reading your response:

>   2, It's true that chrome and webview have the same capability of sending
> autofill mojo IPC messages, but, comparing the implementation of webview
> autofill client with chrome autofill client, we can see that webview autofill
> client is handling only those interested features, such as autocomplete, and
is
> ignoring others by empty or NOTIMPLEMENTED() functions, this means that
actually
> many mojo messages(except autocomplete) will never be triggered to be sent to
> renderer process.

I have looked into android_webview/native/aw_autofill_client.cc. I have the
following questions:
 * Does aw_autofill_client.cc already run outside of Chrome's browser process?
(I guess yes, because the normal autofill client runs in the renderer.)
 * Are the NOTIMPLEMENTED() calls actually triggered now that your CL landed?
(That would be bad, because they should not be triggered and will issue a
warning.)
 * When you say: "this means that actually many mojo messages(except
autocomplete) will never be triggered to be sent to renderer process," how is
that achieved?

Thanks,
Vaclav

[leonhsl(Using Gerrit), 2016-11-21 14:00:56]

On 2016/11/21 10:14:08, vabr (Chromium) wrote:
> Thanks for the explanation, Han Leon.
> 
> I'm afraid I need to understand a bit more though. Reading your response:
> 
> >   2, It's true that chrome and webview have the same capability of sending
> > autofill mojo IPC messages, but, comparing the implementation of webview
> > autofill client with chrome autofill client, we can see that webview
autofill
> > client is handling only those interested features, such as autocomplete, and
> is
> > ignoring others by empty or NOTIMPLEMENTED() functions, this means that
> actually
> > many mojo messages(except autocomplete) will never be triggered to be sent
to
> > renderer process.
> 
> I have looked into android_webview/native/aw_autofill_client.cc. I have the
> following questions:
>  * Does aw_autofill_client.cc already run outside of Chrome's browser process?
> (I guess yes, because the normal autofill client runs in the renderer.)
I think it runs in Chromium browser process, because autofill::AutofillClient is
supposed to run in browser process.
Am I misunderstanding this?...

>  * Are the NOTIMPLEMENTED() calls actually triggered now that your CL landed?
> (That would be bad, because they should not be triggered and will issue a
> warning.)
OK I'm not aware that NOTIMPLEMENTED() would raise warning, I thought it's just
indicating no any operation.
But I suppose that if they had never been triggered in the legacy IPC world
before, 
they won't be triggered after this CL landed.
Hi, sgurun@, are there any test cases to check these NOTIMPLEMENTED() should not
be triggered?

>  * When you say: "this means that actually many mojo messages(except
> autocomplete) will never be triggered to be sent to renderer process," how is
> that achieved?
> 
An example would be autofill::AutofillClient::PropagateAutofillPredictions, 
it's implementation in ChromeAutofillClient could trigger [1] and [2],
but AwAutofillClient provides an empty implementation so it will trigger nothing
to be sent to renderer process.

[1]
https://cs.chromium.org/chromium/src/components/password_manager/content/brow...
[2]
https://cs.chromium.org/chromium/src/components/password_manager/content/brow...

> Thanks,
> Vaclav

[vabr (Chromium), 2016-11-21 16:59:15]

On 2016/11/21 14:00:56, leonhsl wrote:
> On 2016/11/21 10:14:08, vabr (Chromium) wrote:
> > Thanks for the explanation, Han Leon.
> > 
> > I'm afraid I need to understand a bit more though. Reading your response:
> > 
> > >   2, It's true that chrome and webview have the same capability of sending
> > > autofill mojo IPC messages, but, comparing the implementation of webview
> > > autofill client with chrome autofill client, we can see that webview
> autofill
> > > client is handling only those interested features, such as autocomplete,
and
> > is
> > > ignoring others by empty or NOTIMPLEMENTED() functions, this means that
> > actually
> > > many mojo messages(except autocomplete) will never be triggered to be sent
> to
> > > renderer process.
> > 
> > I have looked into android_webview/native/aw_autofill_client.cc. I have the
> > following questions:
> >  * Does aw_autofill_client.cc already run outside of Chrome's browser
process?
> > (I guess yes, because the normal autofill client runs in the renderer.)
> I think it runs in Chromium browser process, because autofill::AutofillClient
is
> supposed to run in browser process.
> Am I misunderstanding this?...

You are right. This is awkward, I mixed AutofillClient with AutofillAgent... :)
So the whole concern about renderer getting to decide what to drop seems to be
void, that's good news.

> 
> >  * Are the NOTIMPLEMENTED() calls actually triggered now that your CL
landed?
> > (That would be bad, because they should not be triggered and will issue a
> > warning.)
> OK I'm not aware that NOTIMPLEMENTED() would raise warning, I thought it's
just
> indicating no any operation.
> But I suppose that if they had never been triggered in the legacy IPC world
> before, 
> they won't be triggered after this CL landed.
> Hi, sgurun@, are there any test cases to check these NOTIMPLEMENTED() should
not
> be triggered?

If there is an easy answer, this would be good to know, but as explained above,
the main concern is gone, it was my mistake.

> 
> >  * When you say: "this means that actually many mojo messages(except
> > autocomplete) will never be triggered to be sent to renderer process," how
is
> > that achieved?
> > 
> An example would be autofill::AutofillClient::PropagateAutofillPredictions, 
> it's implementation in ChromeAutofillClient could trigger [1] and [2],
> but AwAutofillClient provides an empty implementation so it will trigger
nothing
> to be sent to renderer process.

Thanks for all the responses!

Vaclav
> 
> [1]
>
https://cs.chromium.org/chromium/src/components/password_manager/content/brow...
> [2]
>
https://cs.chromium.org/chromium/src/components/password_manager/content/brow...
> 
> > Thanks,
> > Vaclav

[sgurun-gerrit only, 2016-11-21 21:53:56]

On 2016/11/21 16:59:15, vabr (Chromium) wrote:
> On 2016/11/21 14:00:56, leonhsl wrote:
> > On 2016/11/21 10:14:08, vabr (Chromium) wrote:
> > > Thanks for the explanation, Han Leon.
> > > 
> > > I'm afraid I need to understand a bit more though. Reading your response:
> > > 
> > > >   2, It's true that chrome and webview have the same capability of
sending
> > > > autofill mojo IPC messages, but, comparing the implementation of webview
> > > > autofill client with chrome autofill client, we can see that webview
> > autofill
> > > > client is handling only those interested features, such as autocomplete,
> and
> > > is
> > > > ignoring others by empty or NOTIMPLEMENTED() functions, this means that
> > > actually
> > > > many mojo messages(except autocomplete) will never be triggered to be
sent
> > to
> > > > renderer process.
> > > 
> > > I have looked into android_webview/native/aw_autofill_client.cc. I have
the
> > > following questions:
> > >  * Does aw_autofill_client.cc already run outside of Chrome's browser
> process?
> > > (I guess yes, because the normal autofill client runs in the renderer.)
> > I think it runs in Chromium browser process, because
autofill::AutofillClient
> is
> > supposed to run in browser process.
> > Am I misunderstanding this?...
> 
> You are right. This is awkward, I mixed AutofillClient with AutofillAgent...
:)
> So the whole concern about renderer getting to decide what to drop seems to be
> void, that's good news.

And also WebView's AutofillClient also runs in the browser process (but webview
multiprocess is still a developer's feature).

> 
> > 
> > >  * Are the NOTIMPLEMENTED() calls actually triggered now that your CL
> landed?
> > > (That would be bad, because they should not be triggered and will issue a
> > > warning.)
> > OK I'm not aware that NOTIMPLEMENTED() would raise warning, I thought it's
> just
> > indicating no any operation.
> > But I suppose that if they had never been triggered in the legacy IPC world
> > before, 
> > they won't be triggered after this CL landed.
> > Hi, sgurun@, are there any test cases to check these NOTIMPLEMENTED() should
> not
> > be triggered?
> 
> If there is an easy answer, this would be good to know, but as explained
above,
> the main concern is gone, it was my mistake.

No we don't have test cases, unfortunately. 

> 
> > 
> > >  * When you say: "this means that actually many mojo messages(except
> > > autocomplete) will never be triggered to be sent to renderer process," how
> is
> > > that achieved?
> > > 
> > An example would be autofill::AutofillClient::PropagateAutofillPredictions, 
> > it's implementation in ChromeAutofillClient could trigger [1] and [2],
> > but AwAutofillClient provides an empty implementation so it will trigger
> nothing
> > to be sent to renderer process.
> 
> Thanks for all the responses!
> 
> Vaclav
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/components/password_manager/content/brow...
> > [2]
> >
>
https://cs.chromium.org/chromium/src/components/password_manager/content/brow...
> > 
> > > Thanks,
> > > Vaclav