Make ChromeAutofillClient a ZoomObserver.

Make ChromeAutofillClient a ZoomObserver.

In r273854 we may have removed an indirect pathway used to close open
autofill popups when the associated web_contents undergoes a zoom
change. This CL replaces that by directly registering the
ChromeAutofillClient as a ZoomObserver, allowing it to directly
react to zoom events.

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

[wjmaclean, 2014-07-09 20:18:31]

avi@chromium.org: Please review changes in tab_helpers.cc

pkasting@chromium.org: Please review changes in chrome_autofill_client.*,
tab_helpers.cc

This is sort of an experimental patch, so I appreciate your feedback about
whether this is the right way to do this. In particular, is the induced ordering
dependence in the tab helpers a problem?

[Avi (use Gerrit), 2014-07-09 20:24:40]

On 2014/07/09 20:18:31, wjmaclean wrote:
> mailto:avi@chromium.org: Please review changes in tab_helpers.cc
> 
> mailto:pkasting@chromium.org: Please review changes in
chrome_autofill_client.*,
> tab_helpers.cc
> 
> This is sort of an experimental patch, so I appreciate your feedback about
> whether this is the right way to do this. In particular, is the induced
ordering
> dependence in the tab helpers a problem?

Right now there's no real dependency control among the tab helpers, and they're
destroyed in an arbitrary order. We've tried to keep them independent because
that helps with code sanity, and complexity shows up at that point (e.g.
KeyedServices).

I'm not sure if it is a good thing or not. I've tried to avoid this for a long
while...

[wjmaclean, 2014-07-09 20:36:49]

On 2014/07/09 20:24:40, Avi (OOO July 11-27) wrote:
> On 2014/07/09 20:18:31, wjmaclean wrote:
> > mailto:avi@chromium.org: Please review changes in tab_helpers.cc
> > 
> > mailto:pkasting@chromium.org: Please review changes in
> chrome_autofill_client.*,
> > tab_helpers.cc
> > 
> > This is sort of an experimental patch, so I appreciate your feedback about
> > whether this is the right way to do this. In particular, is the induced
> ordering
> > dependence in the tab helpers a problem?
> 
> Right now there's no real dependency control among the tab helpers, and
they're
> destroyed in an arbitrary order. We've tried to keep them independent because
> that helps with code sanity, and complexity shows up at that point (e.g.
> KeyedServices).
> 
> I'm not sure if it is a good thing or not. I've tried to avoid this for a long
> while...

I hadn't realized they might be destroyed in arbitrary order, so that's good to
know ...

I just realized I never removed the ChromeAutofillClient as an observer, but we
can do the following:

1) If the ChromeAutofillClient's web_contents is destroyed, un-register as an
observer at that time, else

2) when the ChromeAutofillClient is destroyed, call
ZoomController::FromWebContents() and if the zoom controller still exists,
un-register at that time.

Actually, despite the existence of ChromeAutofillClient::WebContentsDestroyed(),
it seems like the web contents itself must outlive its user data, so perhaps (1)
isn't an issue?

In (2), as long as querying the WebContentsUserData for the ZoomController while
the ChromeAutofillClient is being destroyed isn't a problem, then that could
work.

Another alternative would be to find another place to install the ZoomObserver,
though offhand I'm not familiar enough with the autofill framework to know where
that would be.

[Avi (use Gerrit), 2014-07-09 20:43:30]

On 2014/07/09 20:36:49, wjmaclean wrote:
> I hadn't realized they might be destroyed in arbitrary order, so that's good
to
> know ...

SupportsUserData keeps them in a std::map<const void*, linked_ptr<Data>>, so the
order in which they're destroyed is dependent on the addresses of the keys,
which is an accident of the link.

> 1) If the ChromeAutofillClient's web_contents is destroyed, un-register as an
> observer at that time, else
> 
> 2) when the ChromeAutofillClient is destroyed, call
> ZoomController::FromWebContents() and if the zoom controller still exists,
> un-register at that time.

I would lean towards (1); unlink the dependency as soon as possible.

> it seems like the web contents itself must outlive its user data

That's untrue. WebContents has SupportsUserData as a base class, and it's the
SupportsUserData that owns the tab helpers. ~WebContents() is called long before
the destructors of any of the tab helpers.

> In (2), as long as querying the WebContentsUserData for the ZoomController
while
> the ChromeAutofillClient is being destroyed isn't a problem, then that could
> work.

I'd really prefer if you weren't making reentrant calls into an object that is
partially destructed.

> Another alternative would be to find another place to install the
ZoomObserver,
> though offhand I'm not familiar enough with the autofill framework to know
where
> that would be.

That would be much preferable to me, though I'm not the expert in that so I
can't say.

[Peter Kasting, 2014-07-09 20:51:51]

I feel like I'm out of my depth trying to review this.  I'm going to bow out in
favor of Avi.  If you need someone for the autofill stuff, I suggest isherman or
one of the other autofill owners.

[wjmaclean, 2014-07-09 21:00:15]

On 2014/07/09 20:43:30, Avi (OOO July 11-27) wrote:
> On 2014/07/09 20:36:49, wjmaclean wrote:
> > I hadn't realized they might be destroyed in arbitrary order, so that's good
> to
> > know ...
> 
> SupportsUserData keeps them in a std::map<const void*, linked_ptr<Data>>, so
the
> order in which they're destroyed is dependent on the addresses of the keys,
> which is an accident of the link.
> 
> > 1) If the ChromeAutofillClient's web_contents is destroyed, un-register as
an
> > observer at that time, else
> > 
> > 2) when the ChromeAutofillClient is destroyed, call
> > ZoomController::FromWebContents() and if the zoom controller still exists,
> > un-register at that time.
> 
> I would lean towards (1); unlink the dependency as soon as possible.

Sure, this can be made to work (see my comment below).
 
> > it seems like the web contents itself must outlive its user data
> 
> That's untrue. WebContents has SupportsUserData as a base class, and it's the
> SupportsUserData that owns the tab helpers. ~WebContents() is called long
before
> the destructors of any of the tab helpers.

Ok, that makes sense ... I haven't read through enough of that code yet :-)

> > In (2), as long as querying the WebContentsUserData for the ZoomController
> while
> > the ChromeAutofillClient is being destroyed isn't a problem, then that could
> > work.
> 
> I'd really prefer if you weren't making reentrant calls into an object that is
> partially destructed.

Yeah, thought that might be a concern.

> > Another alternative would be to find another place to install the
> ZoomObserver,
> > though offhand I'm not familiar enough with the autofill framework to know
> where
> > that would be.
> 
> That would be much preferable to me, though I'm not the expert in that so I
> can't say.

I'll look and see what the possibilities are here (wrt to making some other
class a ZoomObserver), but otherwise I can make it so that during
WebContentsDestroyed() that the ZoomController() just dumps its list of
ZoomObservers, thus de-linking it from ChromeAutofillClient.

[wjmaclean, 2014-07-09 21:00:51]

On 2014/07/09 20:51:51, Peter Kasting wrote:
> I feel like I'm out of my depth trying to review this.  I'm going to bow out
in
> favor of Avi.  If you need someone for the autofill stuff, I suggest isherman
or
> one of the other autofill owners.

No problem ... thanks for looking.

[wjmaclean, 2014-07-09 21:01:22]

isherman@chromium.org: Please review changes in the autofill-related parts of
this cl?

[Ilya Sherman, 2014-07-09 21:52:43]

Autofill changes LGTM -- thanks!

https://codereview.chromium.org/377423002/diff/1/chrome/browser/ui/autofill/c...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/377423002/diff/1/chrome/browser/ui/autofill/c...
chrome/browser/ui/autofill/chrome_autofill_client.cc:183: // just zoom events
for web_contents_.
What other zoom events are possible?

[Ilya Sherman, 2014-07-09 22:04:23]

BTW, is RenderViewObserver::ZoomLevelChanged obsolete now?  Should it be removed
entirely?

[wjmaclean, 2014-07-10 14:47:36]

dbeam@chromium.org: Please review changes in zoom_controller.*

On 2014/07/09 22:04:23, Ilya Sherman wrote:
> BTW, is RenderViewObserver::ZoomLevelChanged obsolete now?  Should it be
removed
> entirely?

It appears to be obsolete. I'll file a bug to verify this and remove it, and
assign it to myself.

avi@ - I've simplified the dependance of ChromeAutofillClient on ZoomController
to be restricted to the ZoomObserver interface definition only. The only other
place it looks reasonable to hang this would be the autofill manager, which is
also a WebContentsObserver. The previous notification chain notified the
autofill manager via IPC.

https://codereview.chromium.org/377423002/diff/1/chrome/browser/ui/autofill/c...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/377423002/diff/1/chrome/browser/ui/autofill/c...
chrome/browser/ui/autofill/chrome_autofill_client.cc:183: // just zoom events
for web_contents_.
On 2014/07/09 21:52:43, Ilya Sherman wrote:
> What other zoom events are possible?

ZoomObservers are notified of the following:
1) changes in zoom level
2) changes in zoom mode (as defined in ZoomController)

(2) & (1) sometimes occur together.

Thinking some more on this, because the ChromeAutofillClient is tied to a single
WebContents, this check will always be true (some ZoomObservers listen to
multiple ZoomControllers, thus the reason the event data carries with
web_contents pointer with it.

[Avi (use Gerrit), 2014-07-10 14:59:53]

https://codereview.chromium.org/377423002/diff/20001/chrome/browser/ui/tab_he...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/377423002/diff/20001/chrome/browser/ui/tab_he...
chrome/browser/ui/tab_helpers.cc:174:
autofill::ChromeAutofillClient::FromWebContents(web_contents));
:(

I would much rather move ZoomController before ChromeAutofillClient, and note
with a comment the dependency, than start calling code here.

The job of this file is to know what tab helpers need to be instantiated (and
unfortunately in what order). I'd prefer to keep the knowledge that the
ChromeAutofillClient needs to be a ZoomObserver inside ChromeAutofillClient.

[wjmaclean, 2014-07-10 15:42:09]

On 2014/07/10 14:59:53, Avi (OOO July 11-27) wrote:
>
https://codereview.chromium.org/377423002/diff/20001/chrome/browser/ui/tab_he...
> File chrome/browser/ui/tab_helpers.cc (right):
> 
>
https://codereview.chromium.org/377423002/diff/20001/chrome/browser/ui/tab_he...
> chrome/browser/ui/tab_helpers.cc:174:
> autofill::ChromeAutofillClient::FromWebContents(web_contents));
> :(
> 
> I would much rather move ZoomController before ChromeAutofillClient, and note
> with a comment the dependency, than start calling code here.
> 
> The job of this file is to know what tab helpers need to be instantiated (and
> unfortunately in what order). I'd prefer to keep the knowledge that the
> ChromeAutofillClient needs to be a ZoomObserver inside ChromeAutofillClient.

Np ... I did this mainly because I though you might prefer it, so if that's not
the case we can go back to the other model :-) I'll put a new patch up shortly.

[Avi (use Gerrit), 2014-07-10 15:44:12]

On 2014/07/10 15:42:09, wjmaclean wrote:
> On 2014/07/10 14:59:53, Avi (OOO July 11-27) wrote:
> >
>
https://codereview.chromium.org/377423002/diff/20001/chrome/browser/ui/tab_he...
> > File chrome/browser/ui/tab_helpers.cc (right):
> > 
> >
>
https://codereview.chromium.org/377423002/diff/20001/chrome/browser/ui/tab_he...
> > chrome/browser/ui/tab_helpers.cc:174:
> > autofill::ChromeAutofillClient::FromWebContents(web_contents));
> > :(
> > 
> > I would much rather move ZoomController before ChromeAutofillClient, and
note
> > with a comment the dependency, than start calling code here.
> > 
> > The job of this file is to know what tab helpers need to be instantiated
(and
> > unfortunately in what order). I'd prefer to keep the knowledge that the
> > ChromeAutofillClient needs to be a ZoomObserver inside ChromeAutofillClient.
> 
> Np ... I did this mainly because I though you might prefer it, so if that's
not
> the case we can go back to the other model :-) I'll put a new patch up
shortly.

Yeah. I'd much rather avoid a dependency at all but if not let's just call it
out.

[wjmaclean, 2014-07-10 17:24:50]

Revised as per avi@'s suggestion, PTAL.

[Avi (use Gerrit), 2014-07-10 17:39:26]

lgtm

Unfortunate, but OK.

https://codereview.chromium.org/377423002/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/377423002/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/chrome_autofill_client.cc:41: // Since ZoomController
is also a WebContentsObservers, we need to be careful
s/WebContentsObservers/WebContentsObserver/

[Dan Beam, 2014-07-10 20:34:06]

lgtm

[wjmaclean, 2014-07-10 20:37:28]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-07-10 20:39:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/377423002/60001

[commit-bot: I haz the power, 2014-07-10 22:59:52]

Change committed as 282453