Remember zoom on a per-host basis....

Remember zoom on a per-host basis.

BUG=567
TEST=Visit a page, zoom in or out, then navigate to a different host.  The new page should not be zoomed.  Go back, or restart, or open a new tab and navigate to the old page, and it should be zoomed.

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

[Peter Kasting, 2009-11-26 00:45:49]

This requires the patch on https://bugs.webkit.org/show_bug.cgi?id=31893 as
well.

There are a couple places in the renderer where I wasn't sure if I needed a
NULL-check.  I've noted them with comments.

[Evan Martin, 2009-11-26 06:33:51]

http://codereview.chromium.org/437077/diff/4001/4016
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/437077/diff/4001/4016#newcode1123
chrome/browser/browser.cc:1123: UserMetrics::RecordAction(kActions[zoom_function
- PageZoom::ZOOM_OUT],
see docs on this function for the weird usage instructions

[darin (slow to review), 2009-12-02 00:02:28]

http://codereview.chromium.org/437077/diff/7001/8010
File chrome/browser/host_zoom_map.cc (right):

http://codereview.chromium.org/437077/diff/7001/8010#newcode15
chrome/browser/host_zoom_map.cc:15:
profile_->GetPrefs()->GetDictionary(prefs::kPerHostZoomLevels);
is it true that the zoom levels seen by incognito mode would all be the default
value?  or, is it possible for incognito mode to inherit zoom levels set by the
normal profile?  maybe that is just a feature enhancement.

http://codereview.chromium.org/437077/diff/7001/8010#newcode36
chrome/browser/host_zoom_map.cc:36: AutoLock auto_lock(lock_);
please add an assertion using ChromeThread::CurrentlyOn to ensure that this is
called on the proper thread.

http://codereview.chromium.org/437077/diff/7001/8010#newcode42
chrome/browser/host_zoom_map.cc:42: {
please add an assertion using ChromeThread::CurrentlyOn to ensure that this is
called on the proper thread.

http://codereview.chromium.org/437077/diff/7001/8008
File chrome/browser/host_zoom_map.h (right):

http://codereview.chromium.org/437077/diff/7001/8008#newcode38
chrome/browser/host_zoom_map.h:38: // This will notify all renderers of the new
zoom level, in case they're
it seems like a "host zoom map" would only be concerned with storing (host,zoom)
pairs.  that it is also responsible for notifying renderers seems to morph it
into more of a zoom controller.

would you consider moving that duty out?

http://codereview.chromium.org/437077/diff/7001/8008#newcode45
chrome/browser/host_zoom_map.h:45: private:
please define a destructor for HostZoomMap in the private section and make
base::RefCountedThreadSafe<HostZoomMap> be a friend.  that way someone doesn't
accidentally delete a HostZoomMap instead of calling Release.  i think jam
posted to chromium-dev about this issue recently.

http://codereview.chromium.org/437077/diff/7001/8014
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/437077/diff/7001/8014#newcode10
chrome/browser/net/chrome_url_request_context.h:10: #include
"chrome/browser/host_zoom_map.h"
nit: "h" before "n"

http://codereview.chromium.org/437077/diff/7001/8017
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/437077/diff/7001/8017#newcode403
chrome/browser/renderer_host/resource_dispatcher_host.cc:403: if (context &&
(request_data.resource_type == ResourceType::MAIN_FRAME)) {
please add a note about the subtleties here.  your goal is to inform the
renderer of the zoom level before a response to this resource request is
received, and that is probably worth noting.

question:  maybe we should defer this until we send
ViewMsg_Resource_ReceivedResponse?  that way we avoid sending this in cases
where we aren't going to complete the network request (for whatever reason).

in fact, if you do this, then i think you won't need to worry about redirects
because they would have all been followed already.

http://codereview.chromium.org/437077/diff/7001/8016
File chrome/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/437077/diff/7001/8016#newcode45
chrome/browser/renderer_host/resource_message_filter.h:45: class SpellChecker;
it looks like you can delete this forward declaration as well.

http://codereview.chromium.org/437077/diff/7001/8022
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/437077/diff/7001/8022#newcode360
chrome/common/render_messages_internal.h:360:
IPC_MESSAGE_ROUTED2(ViewMsg_ZoomLevelForLoadingHost,
I think this should have the word Set in it.

ViewMsg_SetZoomLevelForLoadingHost
ViewMsg_SetZoomLevelForCurrentHost

http://codereview.chromium.org/437077/diff/7001/8025
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/437077/diff/7001/8025#newcode132
chrome/renderer/render_thread.cc:132: // Reviewers: Can mainFrame() be NULL
here?
you shouldn't have to worry about mainFrame being null here.

http://codereview.chromium.org/437077/diff/7001/8025#newcode133
chrome/renderer/render_thread.cc:133: if (webview->mainFrame()->url().operator
GURL().host() == host_)
please do not call operator GURL like this.  instead do
GURL(webview->mainFrame()->url()).host() == host_

that will produce equivalent code and is more readable.

http://codereview.chromium.org/437077/diff/7001/8025#newcode272
chrome/renderer/render_thread.cc:272: scoped_ptr<RenderViewZoomer> zoomer(new
RenderViewZoomer(host, zoom_level));
why 'new' this?  how about just allocating it on the stack?

http://codereview.chromium.org/437077/diff/7001/8027
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/437077/diff/7001/8027#newcode1048
chrome/renderer/render_view.cc:1048: // Set zoom level.
it would be good to document why you are able to erase this zoom level now.

http://codereview.chromium.org/437077/diff/7001/8027#newcode2153
chrome/renderer/render_view.cc:2153: const WebURLError& error) {
why are you changing the style here?  the old style was valid.

http://codereview.chromium.org/437077/diff/7001/8027#newcode2167
chrome/renderer/render_view.cc:2167:
host_zoom_levels_.erase(failed_request.url().operator GURL().host());
GURL(...).host() please

http://codereview.chromium.org/437077/diff/7001/8027#newcode2238
chrome/renderer/render_view.cc:2238: void
RenderView::didCommitProvisionalLoad(WebFrame* frame,
why change the style here?

http://codereview.chromium.org/437077/diff/7001/8027#newcode2982
chrome/renderer/render_view.cc:2982: // Reviewers: Can webview() be NULL here?
possibly.  perhaps when the RenderView is being shutdown?

http://codereview.chromium.org/437077/diff/7001/8027#newcode2983
chrome/renderer/render_view.cc:2983: WebView* _webview = webview();
please do not use leading underscores.  just use webview() everywhere you need
it.  it is lowercase to indicate that it is as cheap as a member variable
access, so no need to copy it to a local variable.

http://codereview.chromium.org/437077/diff/7001/8027#newcode2989
chrome/renderer/render_view.cc:2989: // Reviewers: Can mainFrame() be NULL here?
mainFrame won't be null if webview() is non-null

http://codereview.chromium.org/437077/diff/7001/8003
File net/url_request/url_request.h (right):

http://codereview.chromium.org/437077/diff/7001/8003#newcode456
net/url_request/url_request.h:456: GURL deferred_redirect_url() const;
nit: GetDeferredRedirectURL since this is not implemented inline.

http://codereview.chromium.org/437077/diff/7001/8001
File webkit/tools/test_shell/event_sending_controller.cc (right):

http://codereview.chromium.org/437077/diff/7001/8001#newcode619
webkit/tools/test_shell/event_sending_controller.cc:619: WebView* _webview =
webview();
nit: don't use leading underscores.  in this case, i would just use webview() in
both cases without a local variable.  there's no point in super optimizing this
code since it is just machinery for the layout test system.

[Peter Kasting, 2009-12-02 02:21:27]

http://codereview.chromium.org/437077/diff/7001/8010
File chrome/browser/host_zoom_map.cc (right):

http://codereview.chromium.org/437077/diff/7001/8010#newcode15
chrome/browser/host_zoom_map.cc:15:
profile_->GetPrefs()->GetDictionary(prefs::kPerHostZoomLevels);
On 2009/12/02 00:02:28, darin wrote:
> is it true that the zoom levels seen by incognito mode would all be the
default
> value?  or, is it possible for incognito mode to inherit zoom levels set by
the
> normal profile?  maybe that is just a feature enhancement.

Incognito will see zoom levels that were in the normal profile at the time the
incognito profile was created (i.e. when the incognito window was opened).  Zoom
levels set in either profile after that will be reflected to all windows within
the profile, but won't be mirrored to the other profile.  Closing all incognito
windows (i.e. destroying the profile) and then reopening one will mean it sees
updated values.

I think this is probably the ideal behavior.  Mirroring changes from the normal
profile to the incognito profile after incognito profile creation is fraught
with bad edges since we can't mirror things back the other way.  And it's
unlikely to matter in practice since you'd have to visit a site in the normal
window, zoom it, and then visit the same site in an already-open incognito
window to even notice-- a rare case.

http://codereview.chromium.org/437077/diff/7001/8010#newcode36
chrome/browser/host_zoom_map.cc:36: AutoLock auto_lock(lock_);
On 2009/12/02 00:02:28, darin wrote:
> please add an assertion using ChromeThread::CurrentlyOn to ensure that this is
> called on the proper thread.

But this can be called on any thread.

http://codereview.chromium.org/437077/diff/7001/8010#newcode42
chrome/browser/host_zoom_map.cc:42: {
On 2009/12/02 00:02:28, darin wrote:
> please add an assertion using ChromeThread::CurrentlyOn to ensure that this is
> called on the proper thread.

Done.

http://codereview.chromium.org/437077/diff/7001/8008
File chrome/browser/host_zoom_map.h (right):

http://codereview.chromium.org/437077/diff/7001/8008#newcode38
chrome/browser/host_zoom_map.h:38: // This will notify all renderers of the new
zoom level, in case they're
On 2009/12/02 00:02:28, darin wrote:
> it seems like a "host zoom map" would only be concerned with storing
(host,zoom)
> pairs.  that it is also responsible for notifying renderers seems to morph it
> into more of a zoom controller.
> 
> would you consider moving that duty out?

Moved to ResourceMessageFilter.

http://codereview.chromium.org/437077/diff/7001/8008#newcode45
chrome/browser/host_zoom_map.h:45: private:
On 2009/12/02 00:02:28, darin wrote:
> please define a destructor for HostZoomMap in the private section and make
> base::RefCountedThreadSafe<HostZoomMap> be a friend.  that way someone doesn't
> accidentally delete a HostZoomMap instead of calling Release.

Done.

http://codereview.chromium.org/437077/diff/7001/8014
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/437077/diff/7001/8014#newcode10
chrome/browser/net/chrome_url_request_context.h:10: #include
"chrome/browser/host_zoom_map.h"
On 2009/12/02 00:02:28, darin wrote:
> nit: "h" before "n"

Fixed.

http://codereview.chromium.org/437077/diff/7001/8017
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/437077/diff/7001/8017#newcode403
chrome/browser/renderer_host/resource_dispatcher_host.cc:403: if (context &&
(request_data.resource_type == ResourceType::MAIN_FRAME)) {
On 2009/12/02 00:02:28, darin wrote:
> please add a note about the subtleties here.  your goal is to inform the
> renderer of the zoom level before a response to this resource request is
> received, and that is probably worth noting.

Done.

> question:  maybe we should defer this until we send
> ViewMsg_Resource_ReceivedResponse?  that way we avoid sending this in cases
> where we aren't going to complete the network request (for whatever reason).
> 
> in fact, if you do this, then i think you won't need to worry about redirects
> because they would have all been followed already.

I tried to move this to CompleteResponseStarted(), but at that point we no
longer have a |receiver_|.  I'm also leery of what else that might effect.  For
now I'm sticking with my existing design because I know it works.  Is it
super-bad to send the zoom IPC in the case where the provisional load fails?  It
doesn't seem like it...

http://codereview.chromium.org/437077/diff/7001/8016
File chrome/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/437077/diff/7001/8016#newcode45
chrome/browser/renderer_host/resource_message_filter.h:45: class SpellChecker;
On 2009/12/02 00:02:28, darin wrote:
> it looks like you can delete this forward declaration as well.

Done.

http://codereview.chromium.org/437077/diff/7001/8022
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/437077/diff/7001/8022#newcode360
chrome/common/render_messages_internal.h:360:
IPC_MESSAGE_ROUTED2(ViewMsg_ZoomLevelForLoadingHost,
On 2009/12/02 00:02:28, darin wrote:
> I think this should have the word Set in it.
> 
> ViewMsg_SetZoomLevelForLoadingHost
> ViewMsg_SetZoomLevelForCurrentHost

Done.

http://codereview.chromium.org/437077/diff/7001/8025
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/437077/diff/7001/8025#newcode132
chrome/renderer/render_thread.cc:132: // Reviewers: Can mainFrame() be NULL
here?
On 2009/12/02 00:02:28, darin wrote:
> you shouldn't have to worry about mainFrame being null here.

Thanks.

http://codereview.chromium.org/437077/diff/7001/8025#newcode133
chrome/renderer/render_thread.cc:133: if (webview->mainFrame()->url().operator
GURL().host() == host_)
On 2009/12/02 00:02:28, darin wrote:
> please do not call operator GURL like this.  instead do
> GURL(webview->mainFrame()->url()).host() == host_
> 
> that will produce equivalent code and is more readable.

Done.

http://codereview.chromium.org/437077/diff/7001/8025#newcode272
chrome/renderer/render_thread.cc:272: scoped_ptr<RenderViewZoomer> zoomer(new
RenderViewZoomer(host, zoom_level));
On 2009/12/02 00:02:28, darin wrote:
> why 'new' this?  how about just allocating it on the stack?

Uh... dumbness?  :D

http://codereview.chromium.org/437077/diff/7001/8027
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/437077/diff/7001/8027#newcode1048
chrome/renderer/render_view.cc:1048: // Set zoom level.
On 2009/12/02 00:02:28, darin wrote:
> it would be good to document why you are able to erase this zoom level now.

Done.

http://codereview.chromium.org/437077/diff/7001/8027#newcode2153
chrome/renderer/render_view.cc:2153: const WebURLError& error) {
On 2009/12/02 00:02:28, darin wrote:
> why are you changing the style here?  the old style was valid.

From the style guide: "All parameters should be aligned if possible." i.e. this
style is preferred.

http://codereview.chromium.org/437077/diff/7001/8027#newcode2167
chrome/renderer/render_view.cc:2167:
host_zoom_levels_.erase(failed_request.url().operator GURL().host());
On 2009/12/02 00:02:28, darin wrote:
> GURL(...).host() please

Fixed.

http://codereview.chromium.org/437077/diff/7001/8027#newcode2238
chrome/renderer/render_view.cc:2238: void
RenderView::didCommitProvisionalLoad(WebFrame* frame,
On 2009/12/02 00:02:28, darin wrote:
> why change the style here?

See above.

http://codereview.chromium.org/437077/diff/7001/8027#newcode2982
chrome/renderer/render_view.cc:2982: // Reviewers: Can webview() be NULL here?
On 2009/12/02 00:02:28, darin wrote:
> possibly.  perhaps when the RenderView is being shutdown?

OK, added a check.

http://codereview.chromium.org/437077/diff/7001/8027#newcode2983
chrome/renderer/render_view.cc:2983: WebView* _webview = webview();
On 2009/12/02 00:02:28, darin wrote:
> please do not use leading underscores.  just use webview() everywhere you need
> it.  it is lowercase to indicate that it is as cheap as a member variable
> access, so no need to copy it to a local variable.

Done.

http://codereview.chromium.org/437077/diff/7001/8027#newcode2989
chrome/renderer/render_view.cc:2989: // Reviewers: Can mainFrame() be NULL here?
On 2009/12/02 00:02:28, darin wrote:
> mainFrame won't be null if webview() is non-null

Thanks.

http://codereview.chromium.org/437077/diff/7001/8003
File net/url_request/url_request.h (right):

http://codereview.chromium.org/437077/diff/7001/8003#newcode456
net/url_request/url_request.h:456: GURL deferred_redirect_url() const;
On 2009/12/02 00:02:28, darin wrote:
> nit: GetDeferredRedirectURL since this is not implemented inline.

Done.

http://codereview.chromium.org/437077/diff/7001/8001
File webkit/tools/test_shell/event_sending_controller.cc (right):

http://codereview.chromium.org/437077/diff/7001/8001#newcode619
webkit/tools/test_shell/event_sending_controller.cc:619: WebView* _webview =
webview();
On 2009/12/02 00:02:28, darin wrote:
> nit: don't use leading underscores.  in this case, i would just use webview()
in
> both cases without a local variable.  there's no point in super optimizing
this
> code since it is just machinery for the layout test system.

Done.

[darin (slow to review), 2009-12-03 21:52:24]

On 2009/12/02 02:21:27, Peter Kasting wrote:
> http://codereview.chromium.org/437077/diff/7001/8010
> File chrome/browser/host_zoom_map.cc (right):
> 
> http://codereview.chromium.org/437077/diff/7001/8010#newcode15
> chrome/browser/host_zoom_map.cc:15:
> profile_->GetPrefs()->GetDictionary(prefs::kPerHostZoomLevels);
> On 2009/12/02 00:02:28, darin wrote:
> > is it true that the zoom levels seen by incognito mode would all be the
> default
> > value?  or, is it possible for incognito mode to inherit zoom levels set by
> the
> > normal profile?  maybe that is just a feature enhancement.
> 
> Incognito will see zoom levels that were in the normal profile at the time the
> incognito profile was created (i.e. when the incognito window was opened). 
Zoom
> levels set in either profile after that will be reflected to all windows
within
> the profile, but won't be mirrored to the other profile.  Closing all
incognito
> windows (i.e. destroying the profile) and then reopening one will mean it sees
> updated values.
> 
> I think this is probably the ideal behavior.  Mirroring changes from the
normal
> profile to the incognito profile after incognito profile creation is fraught
> with bad edges since we can't mirror things back the other way.  And it's
> unlikely to matter in practice since you'd have to visit a site in the normal
> window, zoom it, and then visit the same site in an already-open incognito
> window to even notice-- a rare case.

OK, SGTM


> http://codereview.chromium.org/437077/diff/7001/8010#newcode36
> chrome/browser/host_zoom_map.cc:36: AutoLock auto_lock(lock_);
> On 2009/12/02 00:02:28, darin wrote:
> > please add an assertion using ChromeThread::CurrentlyOn to ensure that this
is
> > called on the proper thread.
> 
> But this can be called on any thread.

I think I got confused.  Sorry.


> http://codereview.chromium.org/437077/diff/7001/8008
> File chrome/browser/host_zoom_map.h (right):
> 
> http://codereview.chromium.org/437077/diff/7001/8008#newcode38
> chrome/browser/host_zoom_map.h:38: // This will notify all renderers of the
new
> zoom level, in case they're
> On 2009/12/02 00:02:28, darin wrote:
> > it seems like a "host zoom map" would only be concerned with storing
> (host,zoom)
> > pairs.  that it is also responsible for notifying renderers seems to morph
it
> > into more of a zoom controller.
> > 
> > would you consider moving that duty out?
> 
> Moved to ResourceMessageFilter.

Great, looks good.


> > question:  maybe we should defer this until we send
> > ViewMsg_Resource_ReceivedResponse?  that way we avoid sending this in cases
> > where we aren't going to complete the network request (for whatever reason).
> > 
> > in fact, if you do this, then i think you won't need to worry about
redirects
> > because they would have all been followed already.
> 
> I tried to move this to CompleteResponseStarted(), but at that point we no
> longer have a |receiver_|.  I'm also leery of what else that might effect. 
For
> now I'm sticking with my existing design because I know it works.  Is it
> super-bad to send the zoom IPC in the case where the provisional load fails? 
It
> doesn't seem like it...

Hmm, I see... I'm not happy about the way things are structured.  Notice that
AsyncResourceHandler does have a pointer to the ResourceDispatcherHost.  Are
you sure you can't move your logic into AsyncResourceHandler::OnResponseStarted?
I really think that would be cleaner.  Putting more things in RDH makes it
harder
to maintain.


> http://codereview.chromium.org/437077/diff/7001/8027#newcode2153
> chrome/renderer/render_view.cc:2153: const WebURLError& error) {
> On 2009/12/02 00:02:28, darin wrote:
> > why are you changing the style here?  the old style was valid.
> 
> From the style guide: "All parameters should be aligned if possible." i.e.
this
> style is preferred.

I see.  I misunderstood.

[Peter Kasting, 2009-12-03 23:19:41]

On 2009/12/03 21:52:24, darin wrote:
> Are
> you sure you can't move your logic into
AsyncResourceHandler::OnResponseStarted?
> I really think that would be cleaner.  Putting more things in RDH makes it
> harder
> to maintain.

Done.  Can we still reach DidFailProvisionalLoadWithError() after this point? 
If not, I can remove some more code from the renderer.

[Peter Kasting, 2009-12-03 23:52:26]

On 2009/12/03 23:19:41, Peter Kasting wrote:
> Can we still reach DidFailProvisionalLoadWithError() after this point? 
> If not, I can remove some more code from the renderer.

Tested a DNS lookup failure.  We reach didFailProvisionalLoad() without going
through OnResponseStarted().  Removed the code from didFailProvisionalLoad().

[darin (slow to review), 2009-12-04 23:40:02]

LGTM