content: convert zoom notifications to observer usage

content: convert zoom notifications to callbacks

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

[jam, 2013-01-24 01:28:35]

https://codereview.chromium.org/12039058/diff/1/content/public/browser/host_z...
File content/public/browser/host_zoom_map.h (right):

https://codereview.chromium.org/12039058/diff/1/content/public/browser/host_z...
content/public/browser/host_zoom_map.h:29: class Observer {
nit: per c++ style guide, and convention, prefer to put every class into a
separate file. this avoids having to include this header in other headers that
just want to implement the interface (so this would be host_zoom_map_observer.h,
and the class would be HostZoomMapObserver)

[tfarina, 2013-01-24 01:31:46]

On Wed, Jan 23, 2013 at 11:28 PM,  <jam@chromium.org> wrote:
>
>
https://codereview.chromium.org/12039058/diff/1/content/public/browser/host_z...
> File content/public/browser/host_zoom_map.h (right):
>
>
https://codereview.chromium.org/12039058/diff/1/content/public/browser/host_z...
> content/public/browser/host_zoom_map.h:29: class Observer {
> nit: per c++ style guide, and convention, prefer to put every class into
> a separate file. this avoids having to include this header in other
> headers that just want to implement the interface (so this would be
> host_zoom_map_observer.h, and the class would be HostZoomMapObserver)
>
I wish that was a rule that everyone in chrome followed.

But many reviewers don't know that and don't enforce it through out. :(

> https://codereview.chromium.org/12039058/



-- 
Thiago

[Paweł Hajdan Jr., 2013-01-24 19:42:38]

https://codereview.chromium.org/12039058/diff/1/content/public/browser/host_z...
File content/public/browser/host_zoom_map.h (right):

https://codereview.chromium.org/12039058/diff/1/content/public/browser/host_z...
content/public/browser/host_zoom_map.h:29: class Observer {
On 2013/01/24 01:28:35, John Abd-El-Malek wrote:
> nit: per c++ style guide, and convention, prefer to put every class into a
> separate file. this avoids having to include this header in other headers that
> just want to implement the interface (so this would be
host_zoom_map_observer.h,
> and the class would be HostZoomMapObserver)

Done.

[jam, 2013-01-28 18:10:51]

looking at this more: since there's only one method in the interface, it seems
this can be replaced by a callback. there can be a
AddZoomLevelChangedObserver(const callback&);
RemoveZoomLevelChangedObserver(const callback&);

wdyt?

[Paweł Hajdan Jr., 2013-01-28 19:49:44]

On 2013/01/28 18:10:51, John Abd-El-Malek wrote:
> looking at this more: since there's only one method in the interface, it seems
> this can be replaced by a callback. there can be a
> AddZoomLevelChangedObserver(const callback&);
> RemoveZoomLevelChangedObserver(const callback&);
> 
> wdyt?

Well, this would be creating yet another convention for this type of thing, and
if we ever extend the number of callbacks we would either convert things back to
an observer class, or keep adding callback-specific registration and
de-registration functions, at which point cost of conversion increases.

If we want to convert all such observers to callbacks like that, I'm fine.

I'm just unenthusiastic about seemingly inconsistent application of that, and
also no mention of that in the first round of review (which sort of implied to
me the only thing needed is extracting the observer to its own file).

[Jói, 2013-01-30 11:12:02]

On 2013/01/28 19:49:44, Paweł Hajdan Jr. wrote:
> On 2013/01/28 18:10:51, John Abd-El-Malek wrote:
> > looking at this more: since there's only one method in the interface, it
seems
> > this can be replaced by a callback. there can be a
> > AddZoomLevelChangedObserver(const callback&);
> > RemoveZoomLevelChangedObserver(const callback&);
> > 
> > wdyt?
> 
> Well, this would be creating yet another convention for this type of thing,
and
> if we ever extend the number of callbacks we would either convert things back
to
> an observer class, or keep adding callback-specific registration and
> de-registration functions, at which point cost of conversion increases.
> 
> If we want to convert all such observers to callbacks like that, I'm fine.
> 
> I'm just unenthusiastic about seemingly inconsistent application of that, and
> also no mention of that in the first round of review (which sort of implied to
> me the only thing needed is extracting the observer to its own file).

Just to chime in, I think it makes sense to prefer callbacks when there is just
one callback to make, and delegates when there are more.  

I shared the same misgivings as Paweł when I worked on switching Prefs over to
using callbacks, but along the way I saw how powerful they are and how much
flexibility they give clients of an interface.

Cheers,
Jói

[Paweł Hajdan Jr., 2013-01-30 13:57:12]

PTAL

[jam, 2013-01-30 17:50:44]

https://codereview.chromium.org/12039058/diff/12001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/12039058/diff/12001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.h:133: void OnZoomLevelChanged(const
std::string& host);
nit: private if you can. also in other files

https://codereview.chromium.org/12039058/diff/12001/chrome/browser/ui/zoom/zo...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/12039058/diff/12001/chrome/browser/ui/zoom/zo...
chrome/browser/ui/zoom/zoom_controller.cc:50: base::Unretained(this)));
this doesn't work because the new callback you create here won't be equal to the
one that was used earlier. this class needs to store its callback at
registration, so it can use the same one during unregistration (callbacks are
really just smart pointers, so it's fine to do this)

this comment applies to the other files too

https://codereview.chromium.org/12039058/diff/12001/chrome/browser/ui/zoom/zo...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/12039058/diff/12001/chrome/browser/ui/zoom/zo...
chrome/browser/ui/zoom/zoom_controller.h:36: void OnZoomLevelChanged(const
std::string& host);
nit: make this private if it doesn't need to be public

https://codereview.chromium.org/12039058/diff/12001/chrome/browser/ui/zoom/zo...
File chrome/browser/ui/zoom/zoom_controller_unittest.cc (left):

https://codereview.chromium.org/12039058/diff/12001/chrome/browser/ui/zoom/zo...
chrome/browser/ui/zoom/zoom_controller_unittest.cc:80:
notification_observer.Wait();
can you update the test to verify that callbacks are called?

https://codereview.chromium.org/12039058/diff/12001/content/browser/host_zoom...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/12039058/diff/12001/content/browser/host_zoom...
content/browser/host_zoom_map_impl.cc:93: for
(std::vector<ZoomLevelChangedCallback>::iterator i =
nit: here and below, using 

for (size_t i = 0; i < vector_class_.size(); ++i)

is much more readable

https://codereview.chromium.org/12039058/diff/12001/content/public/browser/ho...
File content/public/browser/host_zoom_map.h (right):

https://codereview.chromium.org/12039058/diff/12001/content/public/browser/ho...
content/public/browser/host_zoom_map.h:19: class HostZoomMapImpl;
nit: not needed anymore

https://codereview.chromium.org/12039058/diff/12001/content/public/browser/ho...
content/public/browser/host_zoom_map.h:27: typedef base::Callback<void(const
std::string&)> ZoomLevelChangedCallback;
nit: the convention in the content api is to declare the callbacks right above
the method that uses them

please document what the string parameter in the callback is used for

https://codereview.chromium.org/12039058/diff/12001/content/public/browser/ho...
content/public/browser/host_zoom_map.h:59: ZoomLevelChangedCallback callback) =
0;
nit: callbacks, like other complex types, are passed as const references

[Paweł Hajdan Jr., 2013-01-31 16:02:15]

Thank you for review. PTAL.

https://codereview.chromium.org/12039058/diff/12001/chrome/browser/ui/zoom/zo...
File chrome/browser/ui/zoom/zoom_controller_unittest.cc (left):

https://codereview.chromium.org/12039058/diff/12001/chrome/browser/ui/zoom/zo...
chrome/browser/ui/zoom/zoom_controller_unittest.cc:80:
notification_observer.Wait();
On 2013/01/30 17:50:44, jam wrote:
> can you update the test to verify that callbacks are called?

It already does - see EXPECT_CALL above. The call is synchronous, so there is no
need to spin a message loop or anything like that.

https://codereview.chromium.org/12039058/diff/12001/content/browser/host_zoom...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/12039058/diff/12001/content/browser/host_zoom...
content/browser/host_zoom_map_impl.cc:93: for
(std::vector<ZoomLevelChangedCallback>::iterator i =
On 2013/01/30 17:50:44, jam wrote:
> nit: here and below, using 
> 
> for (size_t i = 0; i < vector_class_.size(); ++i)
> 
> is much more readable

Done. For deletion I'm still using iterators - I can convert to using indexes,
I'd use an explicit temporary std::vector then.

[jam, 2013-01-31 17:59:15]

lgtm with nit

https://codereview.chromium.org/12039058/diff/12001/content/browser/host_zoom...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/12039058/diff/12001/content/browser/host_zoom...
content/browser/host_zoom_map_impl.cc:93: for
(std::vector<ZoomLevelChangedCallback>::iterator i =
On 2013/01/31 16:02:15, Paweł Hajdan Jr. wrote:
> On 2013/01/30 17:50:44, jam wrote:
> > nit: here and below, using 
> > 
> > for (size_t i = 0; i < vector_class_.size(); ++i)
> > 
> > is much more readable
> 
> Done. For deletion I'm still using iterators - I can convert to using indexes,
> I'd use an explicit temporary std::vector then.

not sure why you'd use a temporary std::vector?  a callback should only be in
there once. so

for (size_t i = 0; i < vector_class_.size(); ++i) {
  if (zoom_level_changed_callbacks_[i] == callback) {
    zoom_level_changed_callbacks_.erase(zoom_level_changed_callbacks_.begin() +
i);
    return;
  }
}

should be enough

[Avi (use Gerrit), 2013-01-31 18:46:55]

NAK, NAK.

https://codereview.chromium.org/12039058/diff/18001/chrome/browser/ui/zoom/zo...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/12039058/diff/18001/chrome/browser/ui/zoom/zo...
chrome/browser/ui/zoom/zoom_controller.cc:46: return;
This looks like a very, very bad idea.

Yes, as a WebContentsUserData the ZoomController is owned by a WebContents, but
during destruction the WebContents is destroyed first. web_contents() will never
be non null here. If the HostZoomMap outlives the WebContents, you're almost
guaranteeing a call through a busted callback.

If you need to deregister a callback, ZoomController is a WebContentsObserver.
Override WebContentsDestroyed.

[Paweł Hajdan Jr., 2013-02-01 12:36:11]

https://codereview.chromium.org/12039058/diff/18001/chrome/browser/ui/zoom/zo...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/12039058/diff/18001/chrome/browser/ui/zoom/zo...
chrome/browser/ui/zoom/zoom_controller.cc:46: return;
On 2013/01/31 18:46:55, Avi wrote:
> This looks like a very, very bad idea.
> 
> Yes, as a WebContentsUserData the ZoomController is owned by a WebContents,
but
> during destruction the WebContents is destroyed first. web_contents() will
never
> be non null here. If the HostZoomMap outlives the WebContents, you're almost
> guaranteeing a call through a busted callback.

Thanks for catching that!

> If you need to deregister a callback, ZoomController is a WebContentsObserver.
> Override WebContentsDestroyed.

I used a slightly different solution, just saved pointer to BrowserContext. IMHO
it's easier to verify the symmetry of adding callback in ctor and removing in
dtor.

[Avi (use Gerrit), 2013-02-01 15:45:37]

Nice! I'll go away now :) lgtm