Adds notification of when zoom level changes. I'm going to need this

Adds notification of when zoom level changes. I'm going to need this
for the merged menu so that I can update the zoom level in the menu
appropriately.

BUG=45734
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=49253

[Peter Kasting, 2010-06-08 00:22:48]

Perhaps the existing machinery that is notified of zoom changes should be
converted to use your notification, so we don't have multiple separate systems.

http://codereview.chromium.org/2637006/diff/1/2
File chrome/browser/host_zoom_map.cc (right):

http://codereview.chromium.org/2637006/diff/1/2#newcode82
chrome/browser/host_zoom_map.cc:82: Details<const GURL> details(&url);
You should notify with |host| (as a std::string), not |url|, as that will
accurately tell you what's been zoomed (otherwise you'd need to reconstruct this
elsewhere to realize that the zoom change for foo.com/1 actually applies to
foo.com/2 as well).

http://codereview.chromium.org/2637006/diff/1/2#newcode83
chrome/browser/host_zoom_map.cc:83: NotificationService::current()->Notify(
Nit: I checked, and this would fit in 80 columns:

NotificationService::current()->Notify(NotificationType::ZOOM_LEVEL_CHANGED,
                                       Source<Profile>(profile_), details);

[sky, 2010-06-08 00:27:41]

On Mon, Jun 7, 2010 at 5:22 PM,  <pkasting@chromium.org> wrote:
> Perhaps the existing machinery that is notified of zoom changes should be
> converted to use your notification, so we don't have multiple separate
> systems.

Good idea. What machinery are you referring to? Are you suggesting I
change ResourceMessageFilter::UpdateHostZoomLevelsOnUIThread to send
the notification and have HostZoomMap observer that?

  -Scott

[Peter Kasting, 2010-06-08 00:31:21]

On Mon, Jun 7, 2010 at 5:27 PM, Scott Violet <sky@chromium.org> wrote:

> On Mon, Jun 7, 2010 at 5:22 PM,  <pkasting@chromium.org> wrote:
> > Perhaps the existing machinery that is notified of zoom changes should be
> > converted to use your notification, so we don't have multiple separate
> > systems.
>
> Good idea. What machinery are you referring to? Are you suggesting I
> change ResourceMessageFilter::UpdateHostZoomLevelsOnUIThread to send
> the notification and have HostZoomMap observer that?


I was thinking the other way, that when the zoom map changes a page's zoom
level, instead of directly telling renderer hosts to update relevant
connected renderers, those hosts should just get this notification.

I'm a little shaky on the plumbing, I haven't glanced at it recently.

PK

[sky, 2010-06-08 17:13:07]

On Mon, Jun 7, 2010 at 5:31 PM, Peter Kasting <pkasting@chromium.org> wrote:
> On Mon, Jun 7, 2010 at 5:27 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> On Mon, Jun 7, 2010 at 5:22 PM,  <pkasting@chromium.org> wrote:
>> > Perhaps the existing machinery that is notified of zoom changes should
>> > be
>> > converted to use your notification, so we don't have multiple separate
>> > systems.
>>
>> Good idea. What machinery are you referring to? Are you suggesting I
>> change ResourceMessageFilter::UpdateHostZoomLevelsOnUIThread to send
>> the notification and have HostZoomMap observer that?
>
> I was thinking the other way, that when the zoom map changes a page's zoom
> level, instead of directly telling renderer hosts to update relevant
> connected renderers, those hosts should just get this notification.
> I'm a little shaky on the plumbing, I haven't glanced at it recently.

The current routing is a message is sent to the renderer, the renderer
then sends a message back and the HostZoomMap is updated.
AsyncResourceHandler::OnResponseStarted also sets the zoom level by
querying the HostZoomMap.

We could likely change setting the zoom to be routed through the
HostZoomMap and RenderViewHost listen for changes and potentially send
a zoom message down. It would give different ordering than we know
get, but I supsect it would be ok. None-the-less I would prefer to
keep this cl around sending the notification and handle the wiring
changes separately. What do you think?

  -Scott

[sky, 2010-06-08 17:45:29]

Updated

[Peter Kasting, 2010-06-08 17:54:00]

LGTM

http://codereview.chromium.org/2637006/diff/8001/9001
File chrome/browser/host_zoom_map.cc (right):

http://codereview.chromium.org/2637006/diff/8001/9001#newcode85
chrome/browser/host_zoom_map.cc:85: details);
Nit: You can fit this on two lines

http://codereview.chromium.org/2637006/diff/8001/9002
File chrome/common/notification_type.h (right):

http://codereview.chromium.org/2637006/diff/8001/9002#newcode1022
chrome/common/notification_type.h:1022: // host as a std::string.
Nit: You should probably give more detail here as the "host" may be a spec for
URLs that don't have a host (e.g. file: URLs).  I'd make a reference to some
comments on this elsewhere (since we note this in a few places already).