Check temporary zoom status when sending zoom level in navigation.

Check temporary zoom status when sending zoom level in navigation.

This CL causes the AsyncResourceHandler code to check for temporary zoom
levels for the current view before sending the new zoom level during
navigation. This will allow tabs with per-tab scope zoom settings to
retain their current (temporary) zoom levels during same-origin
navigations.

Committed: https://crrev.com/c62490491303cf2650bc3b44c2a4c630ed615831
Cr-Commit-Position: refs/heads/master@{#316301}

[wjmaclean, 2014-12-30 20:07:37]

wjmaclean@chromium.org changed reviewers:
+ creis@chromium.org, fsamuel@chromium.org

[wjmaclean, 2014-12-30 20:07:38]

wjmaclean@chromium.org changed required reviewers:
+ creis@chromium.org

[wjmaclean, 2014-12-30 20:07:38]

[WIP] This CL will need some discussion about desired behaviour when navigating
tabs with temporary zoom levels. This patch can serve as a starting point for
that discussion.

[Fady Samuel, 2015-01-05 16:24:28]

This seems reasonable to me. lgtm.

[wjmaclean, 2015-01-06 16:24:54]

Closing this issue, as we're taking a different approach.

[wjmaclean, 2015-02-11 15:08:13]

Re-opening this issues, as we will need it to provide a sensible solution to
virtual keyboard zoom issues (isolating from default page zoom in the default
storage partition).

And also, because AsyncResourceHandler shouldn't be ignoring temporary zoom
levels when are set.

[Charlie Reis, 2015-02-11 18:34:32]

Sorry for the delay-- a few thoughts below.

https://codereview.chromium.org/809223006/diff/1/content/browser/loader/async...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/809223006/diff/1/content/browser/loader/async...
content/browser/loader/async_resource_handler.cc:202: // this logic could move
into HostZoomMap, in a function that is given
+1 to moving this into a HostZoomMapImpl function.  As it stands, we're making
calls into two separate AutoLock functions, which means we're releasing the lock
and having to reacquire it (on every OnResponseStarted).  That seems
unnecessary.

https://codereview.chromium.org/809223006/diff/1/content/browser/loader/async...
content/browser/loader/async_resource_handler.cc:204: // WebContents* for the
navigating page from here, though that may change
We can't pass a WebContents here, since this is on the IO thread.

https://codereview.chromium.org/809223006/diff/1/content/browser/loader/async...
content/browser/loader/async_resource_handler.cc:209: double zoom_level;
nit: Initialize this.

https://codereview.chromium.org/809223006/diff/1/content/browser/loader/async...
content/browser/loader/async_resource_handler.cc:212: zoom_level =
host_zoom_map->GetTemporaryZoomLevel(render_process_id,
Can we cast this to HostZoomMapImpl so that we don't expose this (or the
alternative method discussed above) in the public API?

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

https://codereview.chromium.org/809223006/diff/1/content/public/browser/host_...
content/public/browser/host_zoom_map.h:145: virtual double
GetTemporaryZoomLevel(int render_process_id,
From the Content API guidelines:
http://www.chromium.org/developers/content-module/content-api

"only expose methods in the public API that embedders need. If a method is only
used by other code in content, it belongs in foo_impl.h and not foo.h."

https://codereview.chromium.org/809223006/diff/1/content/public/browser/host_...
content/public/browser/host_zoom_map.h:146: int render_view_id) const = 0;
From the Content API guidelines:
http://www.chromium.org/developers/content-module/content-api

"don't add the const identifier to interfaces. For interfaces implemented by the
embedder, we can't make assumptions about what the embedder needs to implement
it. For interfaces implemented by content, the implementation details doesn't
have to be exposed."

[wjmaclean, 2015-02-11 20:29:34]

PTAL?

https://codereview.chromium.org/809223006/diff/1/content/browser/loader/async...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/809223006/diff/1/content/browser/loader/async...
content/browser/loader/async_resource_handler.cc:202: // this logic could move
into HostZoomMap, in a function that is given
On 2015/02/11 18:34:31, Charlie Reis wrote:
> +1 to moving this into a HostZoomMapImpl function.  As it stands, we're making
> calls into two separate AutoLock functions, which means we're releasing the
lock
> and having to reacquire it (on every OnResponseStarted).  That seems
> unnecessary.

Certainly. I wasn't sure about how much willingness there would be to add
interfaces for this one case, but that's a better solution.

Done.

https://codereview.chromium.org/809223006/diff/1/content/browser/loader/async...
content/browser/loader/async_resource_handler.cc:204: // WebContents* for the
navigating page from here, though that may change
On 2015/02/11 18:34:31, Charlie Reis wrote:
> We can't pass a WebContents here, since this is on the IO thread.

Hadn't thought of that, so thanks for pointing it out. This comment is going
away in any case, so no worries (or web_contentses either ...).

https://codereview.chromium.org/809223006/diff/1/content/browser/loader/async...
content/browser/loader/async_resource_handler.cc:209: double zoom_level;
On 2015/02/11 18:34:31, Charlie Reis wrote:
> nit: Initialize this.

Done.

https://codereview.chromium.org/809223006/diff/1/content/browser/loader/async...
content/browser/loader/async_resource_handler.cc:212: zoom_level =
host_zoom_map->GetTemporaryZoomLevel(render_process_id,
On 2015/02/11 18:34:31, Charlie Reis wrote:
> Can we cast this to HostZoomMapImpl so that we don't expose this (or the
> alternative method discussed above) in the public API?

Done.

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

https://codereview.chromium.org/809223006/diff/1/content/public/browser/host_...
content/public/browser/host_zoom_map.h:145: virtual double
GetTemporaryZoomLevel(int render_process_id,
On 2015/02/11 18:34:31, Charlie Reis wrote:
> From the Content API guidelines:
> http://www.chromium.org/developers/content-module/content-api
> 
> "only expose methods in the public API that embedders need. If a method is
only
> used by other code in content, it belongs in foo_impl.h and not foo.h."

Done.

https://codereview.chromium.org/809223006/diff/1/content/public/browser/host_...
content/public/browser/host_zoom_map.h:146: int render_view_id) const = 0;
On 2015/02/11 18:34:31, Charlie Reis wrote:
> From the Content API guidelines:
> http://www.chromium.org/developers/content-module/content-api
> 
> "don't add the const identifier to interfaces. For interfaces implemented by
the
> embedder, we can't make assumptions about what the embedder needs to implement
> it. For interfaces implemented by content, the implementation details doesn't
> have to be exposed."

Done.

[Charlie Reis, 2015-02-11 20:56:34]

Thanks, that's better.  Some nits and a question below.  Also, can you add tests
for GetZoomLevelForView?

https://codereview.chromium.org/809223006/diff/20001/content/browser/host_zoo...
File content/browser/host_zoom_map_impl.h (right):

https://codereview.chromium.org/809223006/diff/20001/content/browser/host_zoo...
content/browser/host_zoom_map_impl.h:118: double
GetZoomLevelForHostAndSchemeInternal(const std::string& scheme,
nit: This should be declared in the same order in the .h and .cc file.  Maybe it
makes sense to move this below GetZoomLevelForHost and have the comment apply to
both of the *Internal functions.

https://codereview.chromium.org/809223006/diff/20001/content/browser/host_zoo...
content/browser/host_zoom_map_impl.h:120: double GetZoomLevelForHost(const
std::string& host) const;
nit: Put a blank line before this to clarify that the comment doesn't apply to
it.  (Unnecessary if you fix the first comment.)

https://codereview.chromium.org/809223006/diff/20001/content/browser/loader/a...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/809223006/diff/20001/content/browser/loader/a...
content/browser/loader/async_resource_handler.cc:205: // an empty scheme as we
used to) doesn't break anything.
This doesn't instill confidence.  :)  Can we test this?

Also, I'm not sure I understand the comment, since you do call .scheme() in the
implementation.  What empty scheme are you referring to?

[wjmaclean, 2015-02-12 15:59:36]

Updated with a test ... PTAL?

https://codereview.chromium.org/809223006/diff/20001/content/browser/host_zoo...
File content/browser/host_zoom_map_impl.h (right):

https://codereview.chromium.org/809223006/diff/20001/content/browser/host_zoo...
content/browser/host_zoom_map_impl.h:118: double
GetZoomLevelForHostAndSchemeInternal(const std::string& scheme,
On 2015/02/11 20:56:34, Charlie Reis wrote:
> nit: This should be declared in the same order in the .h and .cc file.  Maybe
it
> makes sense to move this below GetZoomLevelForHost and have the comment apply
to
> both of the *Internal functions.

Done.

https://codereview.chromium.org/809223006/diff/20001/content/browser/host_zoo...
content/browser/host_zoom_map_impl.h:120: double GetZoomLevelForHost(const
std::string& host) const;
On 2015/02/11 20:56:34, Charlie Reis wrote:
> nit: Put a blank line before this to clarify that the comment doesn't apply to
> it.  (Unnecessary if you fix the first comment.)

Done.

https://codereview.chromium.org/809223006/diff/20001/content/browser/loader/a...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/809223006/diff/20001/content/browser/loader/a...
content/browser/loader/async_resource_handler.cc:205: // an empty scheme as we
used to) doesn't break anything.
On 2015/02/11 20:56:34, Charlie Reis wrote:
> This doesn't instill confidence.  :)  Can we test this?
> 
> Also, I'm not sure I understand the comment, since you do call .scheme() in
the
> implementation.  What empty scheme are you referring to?

Gah, just realized we're not changing anything! *Many* places just put in an
empty string for scheme, but ARH is one of the few that don't.

Nothing will break here.

[Charlie Reis, 2015-02-12 21:51:41]

Great-- I appreciate the test.  A few comments on readability and it should be
good to go.  Thanks!

https://codereview.chromium.org/809223006/diff/40001/content/browser/host_zoo...
File content/browser/host_zoom_map_content_browsertest.cc (right):

https://codereview.chromium.org/809223006/diff/40001/content/browser/host_zoo...
content/browser/host_zoom_map_content_browsertest.cc:16: class
HostZoomMapContentBrowserTest : public ContentBrowserTest {
nit: We should probably name this file host_zoom_map_impl_browsertest.cc,
similar to host_zoom_map_impl_unittest.cc.

https://codereview.chromium.org/809223006/diff/40001/content/browser/host_zoo...
content/browser/host_zoom_map_content_browsertest.cc:44:
IN_PROC_BROWSER_TEST_F(HostZoomMapContentBrowserTest, GetZoomForView) {
This test does seem to be covering a lot of the interesting cases, but it's
pretty cryptic to read without stepping through it line-by-line.  Tests are a
form of documentation for how we expect the code to behave, so I think it's
worth restructuring this to make it easier to read.

I'd suggest things like:
 - Splitting it into two tests (one for SetZoomLevelForHost, one for
SZLFHostAndScheme)
 - Renaming zoom_level_2 to temp_zoom_level (and maybe zoom_level_1 to
host_zoom_level)
 - Adding test-level comments (etc) to guide the reader

https://codereview.chromium.org/809223006/diff/40001/content/browser/host_zoo...
File content/browser/host_zoom_map_impl.h (right):

https://codereview.chromium.org/809223006/diff/40001/content/browser/host_zoo...
content/browser/host_zoom_map_impl.h:119: // a scope where a lock has been
acquired.
nit: versions
nit: These

[wjmaclean, 2015-02-13 15:57:53]

creis@ I think I've addressed all your comments, PTAL?

https://codereview.chromium.org/809223006/diff/40001/content/browser/host_zoo...
File content/browser/host_zoom_map_content_browsertest.cc (right):

https://codereview.chromium.org/809223006/diff/40001/content/browser/host_zoo...
content/browser/host_zoom_map_content_browsertest.cc:16: class
HostZoomMapContentBrowserTest : public ContentBrowserTest {
On 2015/02/12 21:51:41, Charlie Reis wrote:
> nit: We should probably name this file host_zoom_map_impl_browsertest.cc,
> similar to host_zoom_map_impl_unittest.cc.

Done.

https://codereview.chromium.org/809223006/diff/40001/content/browser/host_zoo...
content/browser/host_zoom_map_content_browsertest.cc:44:
IN_PROC_BROWSER_TEST_F(HostZoomMapContentBrowserTest, GetZoomForView) {
On 2015/02/12 21:51:41, Charlie Reis wrote:
> This test does seem to be covering a lot of the interesting cases, but it's
> pretty cryptic to read without stepping through it line-by-line.  Tests are a
> form of documentation for how we expect the code to behave, so I think it's
> worth restructuring this to make it easier to read.
> 
> I'd suggest things like:
>  - Splitting it into two tests (one for SetZoomLevelForHost, one for
> SZLFHostAndScheme)
>  - Renaming zoom_level_2 to temp_zoom_level (and maybe zoom_level_1 to
> host_zoom_level)
>  - Adding test-level comments (etc) to guide the reader

Done.

https://codereview.chromium.org/809223006/diff/40001/content/browser/host_zoo...
File content/browser/host_zoom_map_impl.h (right):

https://codereview.chromium.org/809223006/diff/40001/content/browser/host_zoo...
content/browser/host_zoom_map_impl.h:119: // a scope where a lock has been
acquired.
On 2015/02/12 21:51:41, Charlie Reis wrote:
> nit: versions
> nit: These

Done.

[Charlie Reis, 2015-02-13 20:47:08]

Thanks.  LGTM with nits.

https://codereview.chromium.org/809223006/diff/60001/content/browser/host_zoo...
File content/browser/host_zoom_map_impl_browsertest.cc (right):

https://codereview.chromium.org/809223006/diff/60001/content/browser/host_zoo...
content/browser/host_zoom_map_impl_browsertest.cc:36: url, render_process_id,
view_id));
nit: Wrong indent.

https://codereview.chromium.org/809223006/diff/60001/content/browser/host_zoo...
content/browser/host_zoom_map_impl_browsertest.cc:42: url, render_process_id,
view_id));
nit: Wrong indent.

https://codereview.chromium.org/809223006/diff/60001/content/browser/host_zoo...
content/browser/host_zoom_map_impl_browsertest.cc:48:
IN_PROC_BROWSER_TEST_F(HostZoomMapImplBrowserTest, GetZoomForView_Host) {
nit: Comment.

https://codereview.chromium.org/809223006/diff/60001/content/browser/host_zoo...
content/browser/host_zoom_map_impl_browsertest.cc:63:
IN_PROC_BROWSER_TEST_F(HostZoomMapImplBrowserTest,
nit: Comment.

[wjmaclean, 2015-02-13 21:00:04]

New patchsets have been uploaded after l-g-t-m from creis@chromium.org

[wjmaclean, 2015-02-13 21:01:15]

https://codereview.chromium.org/809223006/diff/60001/content/browser/host_zoo...
File content/browser/host_zoom_map_impl_browsertest.cc (right):

https://codereview.chromium.org/809223006/diff/60001/content/browser/host_zoo...
content/browser/host_zoom_map_impl_browsertest.cc:36: url, render_process_id,
view_id));
On 2015/02/13 20:47:08, Charlie Reis wrote:
> nit: Wrong indent.

Done.

https://codereview.chromium.org/809223006/diff/60001/content/browser/host_zoo...
content/browser/host_zoom_map_impl_browsertest.cc:42: url, render_process_id,
view_id));
On 2015/02/13 20:47:08, Charlie Reis wrote:
> nit: Wrong indent.

Done.

https://codereview.chromium.org/809223006/diff/60001/content/browser/host_zoo...
content/browser/host_zoom_map_impl_browsertest.cc:48:
IN_PROC_BROWSER_TEST_F(HostZoomMapImplBrowserTest, GetZoomForView_Host) {
On 2015/02/13 20:47:08, Charlie Reis wrote:
> nit: Comment.

Done.

https://codereview.chromium.org/809223006/diff/60001/content/browser/host_zoo...
content/browser/host_zoom_map_impl_browsertest.cc:63:
IN_PROC_BROWSER_TEST_F(HostZoomMapImplBrowserTest,
On 2015/02/13 20:47:08, Charlie Reis wrote:
> nit: Comment.

Done.

[wjmaclean, 2015-02-13 21:01:21]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2015-02-13 21:02:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/809223006/80001

[commit-bot: I haz the power, 2015-02-13 22:02:15]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-13 22:02:59]

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