Allow zooming error pages.

Allow zooming error pages.

This CL operates by providing a mechanism in HostZoomMap to convert urls
into the error-page url when an error page is loaded. The error-page
url is used to track the zoom-levels for error pages. Callers to
HostZoomMap::SetZoomLevelForHost*() functions are expected to convert
URLs obtained from NavigationEntrys before setting zoom levels.

HostZoomMap is also modified to add a function to transmit the zoom
level for a newly loaded error page, since it doesn't follow the usual
loading pathway. The transmission of the zoom level is triggered
when an error page is detected in
NavigationControllerImpl::RendererDidNavigateToNewPage().

The CL also creates a localization string to represent error pages in
the Content Settings page list of hosts with non-default zooms, this
since the internal url representations for error pages are not suitable
to be displayed to a user.

BUG=417699
Committed: https://crrev.com/de29ed5dcdeed9400020ee333deda779bd825bda
Cr-Commit-Position: refs/heads/master@{#301707}

[wjmaclean, 2014-10-27 20:26:35]

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

[wjmaclean, 2014-10-27 20:26:36]

creis@ here's the approach based on converting the URLs in ZoomController and
HostZoomMap (I relialized we need to include the latter after I started this.
Changes to HZM would have been necessary even if we converted the URLs inside
NavigationEntry). I put this in a separate code-review since it has little in
common with the other CL. If this looks better, we'll close out the other one.

[wjmaclean, 2014-10-28 13:56:40]

wjmaclean@chromium.org changed reviewers:
+ bauerb@chromium.org

[wjmaclean, 2014-10-28 13:56:41]

bauerb@chromium.org: Please review changes in 

content_settings_handler

This is the same URL as you already lgtm'd, with the addition of a conversion
from the IDS string back to the unreachable URL for removing the zoom exception.

[Bernhard Bauer, 2014-10-28 14:55:23]

content_settings LGTM

https://codereview.chromium.org/678963003/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/678963003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:1217:
l10n_util::GetStringUTF8(IDS_ZOOMLEVELS_CHROME_ERROR_PAGES_LABEL)) {
This feels kind of hacky, but it's probably not worth having additional code in
the Web UI to do this instead of doing it here.

https://codereview.chromium.org/678963003/diff/1/content/browser/host_zoom_ma...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/678963003/diff/1/content/browser/host_zoom_ma...
content/browser/host_zoom_map_impl.cc:413: SendZoomLevelChange("", host,
error_page_zoom_level);
Using an empty std::string() constructor instead of a char* literal saves some
instructions.

[Charlie Reis, 2014-10-28 16:36:22]

Thanks, this seems like a better approach than changing NavigationEntry::GetURL.

https://codereview.chromium.org/678963003/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/678963003/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1066:
HostZoomMap::SendErrorPageZoomLevelRefresh(GetWebContents());
This doesn't seem to belong here, since zoom is unrelated to the rest of this
code.  ZoomController is a WebContentsObserver, so perhaps it belongs there?

https://codereview.chromium.org/678963003/diff/1/content/browser/host_zoom_ma...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/678963003/diff/1/content/browser/host_zoom_ma...
content/browser/host_zoom_map_impl.cc:52: GURL
HostZoomMap::GetURLFromEntry(const NavigationEntry* entry) {
I'm confused why we're defining HostZoomMap methods in host_zoom_map_impl.cc. 
Shouldn't they go in host_zoom_map.cc, like the other .cc files in
content/public/browser?

https://codereview.chromium.org/678963003/diff/1/content/browser/host_zoom_ma...
content/browser/host_zoom_map_impl.cc:56: // In future, give interstitial pages
special treatment as well.
nit: In future -> TODO(wjmaclean):

https://codereview.chromium.org/678963003/diff/1/content/browser/host_zoom_ma...
content/browser/host_zoom_map_impl.cc:410: if (error_page_zoom_level ==
default_zoom_level_)
What happens if the renderer had a different zoom level and the user changed it
to match the default?  Does it always start at default after every commit, or is
it possible to start a different value?

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

https://codereview.chromium.org/678963003/diff/1/content/public/browser/host_...
content/public/browser/host_zoom_map.h:57: CONTENT_EXPORT static GURL
GetURLFromEntry(const NavigationEntry*);
nit: Always name parameters.

Also add a comment hinting at why this is needed, rather than just using
NavigationEntry::GetURL.

https://codereview.chromium.org/678963003/diff/1/content/public/browser/host_...
content/public/browser/host_zoom_map.h:73: CONTENT_EXPORT static void
SendErrorPageZoomLevelRefresh(
This doesn't appear to be called outside of content/, so it should be in the
impl file rather than the public interface.

From 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."

[wjmaclean, 2014-10-28 17:37:04]

creis@ ptal?

https://codereview.chromium.org/678963003/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/678963003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:1217:
l10n_util::GetStringUTF8(IDS_ZOOMLEVELS_CHROME_ERROR_PAGES_LABEL)) {
On 2014/10/28 14:55:22, Bernhard Bauer wrote:
> This feels kind of hacky, but it's probably not worth having additional code
in
> the Web UI to do this instead of doing it here.

Acknowledged.

https://codereview.chromium.org/678963003/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/678963003/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1066:
HostZoomMap::SendErrorPageZoomLevelRefresh(GetWebContents());
On 2014/10/28 16:36:21, Charlie Reis wrote:
> This doesn't seem to belong here, since zoom is unrelated to the rest of this
> code.  ZoomController is a WebContentsObserver, so perhaps it belongs there?

Done.

https://codereview.chromium.org/678963003/diff/1/content/browser/host_zoom_ma...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/678963003/diff/1/content/browser/host_zoom_ma...
content/browser/host_zoom_map_impl.cc:52: GURL
HostZoomMap::GetURLFromEntry(const NavigationEntry* entry) {
On 2014/10/28 16:36:21, Charlie Reis wrote:
> I'm confused why we're defining HostZoomMap methods in host_zoom_map_impl.cc. 
> Shouldn't they go in host_zoom_map.cc, like the other .cc files in
> content/public/browser?

I think the idea was to create convenience functions for clients that don't have
access to the HostZoomMapImpl*, just HostZoomMap*, so we don't have static casts
all over the place. In many call sites, the caller would have to retrieve a
HostZoomMap* using browser_context or web_contents, and then convert it ...
these were meant to simplify those calls.

I think HostZoomMap was meant to be a pure-interface, but with some static
convenience functions. Instead of moving these into host_zoom_map.cc (which
would need to be created), would we instead want to make them virtual functions?

I'm happy to clean all these up in a re-factoring CL.

https://codereview.chromium.org/678963003/diff/1/content/browser/host_zoom_ma...
content/browser/host_zoom_map_impl.cc:56: // In future, give interstitial pages
special treatment as well.
On 2014/10/28 16:36:21, Charlie Reis wrote:
> nit: In future -> TODO(wjmaclean):

Done.

https://codereview.chromium.org/678963003/diff/1/content/browser/host_zoom_ma...
content/browser/host_zoom_map_impl.cc:410: if (error_page_zoom_level ==
default_zoom_level_)
On 2014/10/28 16:36:21, Charlie Reis wrote:
> What happens if the renderer had a different zoom level and the user changed
it
> to match the default?  Does it always start at default after every commit, or
is
> it possible to start a different value?

Good catch ... we should probably always just send it.

Done.

https://codereview.chromium.org/678963003/diff/1/content/browser/host_zoom_ma...
content/browser/host_zoom_map_impl.cc:413: SendZoomLevelChange("", host,
error_page_zoom_level);
On 2014/10/28 14:55:22, Bernhard Bauer wrote:
> Using an empty std::string() constructor instead of a char* literal saves some
> instructions.

I wondered if that might be better; I'll add it in the next version, thanks!

Done.

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

https://codereview.chromium.org/678963003/diff/1/content/public/browser/host_...
content/public/browser/host_zoom_map.h:57: CONTENT_EXPORT static GURL
GetURLFromEntry(const NavigationEntry*);
On 2014/10/28 16:36:22, Charlie Reis wrote:
> nit: Always name parameters.
> 
> Also add a comment hinting at why this is needed, rather than just using
> NavigationEntry::GetURL.

Done.

https://codereview.chromium.org/678963003/diff/1/content/public/browser/host_...
content/public/browser/host_zoom_map.h:73: CONTENT_EXPORT static void
SendErrorPageZoomLevelRefresh(
On 2014/10/28 16:36:22, Charlie Reis wrote:
> This doesn't appear to be called outside of content/, so it should be in the
> impl file rather than the public interface.
> 
> From 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."

With the moving of the code out of NavigationControllerImpl, this is now called
outside content/.

[Charlie Reis, 2014-10-28 19:09:45]

Thanks.  content/ LGTM.

https://codereview.chromium.org/678963003/diff/1/content/browser/host_zoom_ma...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/678963003/diff/1/content/browser/host_zoom_ma...
content/browser/host_zoom_map_impl.cc:52: GURL
HostZoomMap::GetURLFromEntry(const NavigationEntry* entry) {
On 2014/10/28 17:37:04, wjmaclean wrote:
> On 2014/10/28 16:36:21, Charlie Reis wrote:
> > I'm confused why we're defining HostZoomMap methods in
host_zoom_map_impl.cc. 
> > Shouldn't they go in host_zoom_map.cc, like the other .cc files in
> > content/public/browser?
> 
> I think the idea was to create convenience functions for clients that don't
have
> access to the HostZoomMapImpl*, just HostZoomMap*, so we don't have static
casts
> all over the place. In many call sites, the caller would have to retrieve a
> HostZoomMap* using browser_context or web_contents, and then convert it ...
> these were meant to simplify those calls.
> 
> I think HostZoomMap was meant to be a pure-interface, but with some static
> convenience functions. Instead of moving these into host_zoom_map.cc (which
> would need to be created), would we instead want to make them virtual
functions?
> 
> I'm happy to clean all these up in a re-factoring CL.

Thanks.  I realized there's precedent for this in other places (e.g.,
RenderFrameHost::FromID), so we can follow up on this separately.  (I'm getting
mixed opinions from people I talk with, so I'll check in with John.)

[wjmaclean, 2014-10-28 19:20:39]

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

[Fady Samuel, 2014-10-28 19:33:09]

lgtm

[wjmaclean, 2014-10-28 19:54:30]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-10-28 19:55:44]

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

[commit-bot: I haz the power, 2014-10-28 21:09:14]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-10-28 21:10:03]

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