Page zoom improvements

Page zoom improvements:
1. Change from using zoom levels to a range of preset zoom factors (percentages). Zoom levels are the values that WebKit uses for page zoom. While zoom levels are simple to implement, they result in undesirable percentages like 57%, 144%, 207%, etc. Changing to zoom factors gives us user-friendly zoom percentages.
2. Increase the range of supported zoom values. A user can now zoom between 25% to 500%.
3. Use an epsilon value when comparing zoom floating point values. The previous version was doing an equality comparison of double values which may not always work as expected.

BUG=71484
TEST=Exercise zoom in/out via the wrench menu; exercise default page zoom setting in options window
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111087

[csilv, 2011-11-12 19:57:43]

+jhawkins for review

James, I'm going to ask you to review this when you have time.  This touches
lots of different parts so I'll probably need additional reviewers (owners,
etc), but a lot of this CL is webui so you're an ideal person to start with.

I think this is a straight-forward CL, but please let me know if anything isn't
clear.

[James Hawkins, 2011-11-14 18:09:04]

You're gonna cringe, but I'm going to have to ask you to write some tests.

http://codereview.chromium.org/8528011/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/8528011/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:1973: RenderViewHost* host =
GetSelectedTabContentsWrapper()->render_view_host();
When is |host| NULL?

http://codereview.chromium.org/8528011/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:2021:
UserMetrics::RecordAction(UserMetricsAction("ZoomMinus"));
For the sake of posterity, it would be interesting to record when the user tries
to zoom minus when already at the lowest (?) level.  Same for zoom plus.

http://codereview.chromium.org/8528011/diff/1/chrome/browser/ui/webui/options...
File chrome/browser/ui/webui/options/advanced_options_handler.cc (right):

http://codereview.chromium.org/8528011/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/advanced_options_handler.cc:264:
web_ui()->RegisterMessageCallback("selectDownloadLocation",
Ehhh I'm not really a fan of continuously calling a getter method; I'd rather
pull it into a local var if you don't want it stored on the object.

http://codereview.chromium.org/8528011/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/advanced_options_handler.cc:564: // Generate a
vector of zoom factors from an array of known preset values.
:-/ This is very similar if not exactly like the code in browser.cc. Can we
refactor somehow?

http://codereview.chromium.org/8528011/diff/1/content/browser/host_zoom_map.cc
File content/browser/host_zoom_map.cc (right):

http://codereview.chromium.org/8528011/diff/1/content/browser/host_zoom_map.c...
content/browser/host_zoom_map.cc:61: if (std::fabs(level - default_zoom_level_)
<= content::kPageZoomEpsilon)
I'm seeing this a lot; can we refactor this somewhere?

http://codereview.chromium.org/8528011/diff/1/content/browser/tab_contents/ta...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/8528011/diff/1/content/browser/tab_contents/ta...
content/browser/tab_contents/tab_contents.cc:866:
WebKit::WebView::zoomLevelToZoomFactor(GetZoomLevel()) * 100 + 0.5);
What's the 0.5 about?

http://codereview.chromium.org/8528011/diff/1/content/public/common/page_zoom.cc
File content/public/common/page_zoom.cc (right):

http://codereview.chromium.org/8528011/diff/1/content/public/common/page_zoom...
content/public/common/page_zoom.cc:17: const int kPresetZoomFactorsCount = 16;
Isn't there a macro that does this for you?  Like arraysize() or something. Then
you can remove the comment above.

http://codereview.chromium.org/8528011/diff/1/content/public/common/page_zoom...
content/public/common/page_zoom.cc:19: const double kPageZoomEpsilon = 0.005;
Document var.

http://codereview.chromium.org/8528011/diff/1/content/public/common/page_zoom.h
File content/public/common/page_zoom.h (right):

http://codereview.chromium.org/8528011/diff/1/content/public/common/page_zoom...
content/public/common/page_zoom.h:22: // The minimum zoom factor permitted for a
page. This is an alternative to
What do you mean by 'alternative'?

[csilv, 2011-11-14 23:10:29]

Partial response to review comments.  (Answering questions, asking for
clarifications.)

http://codereview.chromium.org/8528011/diff/1/content/browser/tab_contents/ta...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/8528011/diff/1/content/browser/tab_contents/ta...
content/browser/tab_contents/tab_contents.cc:866:
WebKit::WebView::zoomLevelToZoomFactor(GetZoomLevel()) * 100 + 0.5);
On 2011/11/14 18:09:05, James Hawkins wrote:
> What's the 0.5 about?

Rouding a fractional value to the nearest integer.  This is important because
zoom levels are saved as levels... and when converted to an factor can result in
values like 299.99999%... and we really want that to be shown as 300%, not 299%.

http://codereview.chromium.org/8528011/diff/1/content/public/common/page_zoom.cc
File content/public/common/page_zoom.cc (right):

http://codereview.chromium.org/8528011/diff/1/content/public/common/page_zoom...
content/public/common/page_zoom.cc:17: const int kPresetZoomFactorsCount = 16;
On 2011/11/14 18:09:05, James Hawkins wrote:
> Isn't there a macro that does this for you?  Like arraysize() or something.
Then
> you can remove the comment above.

I wish this were the case.  Yes, there is an arraysize macro, but it doesn't
work for cases like this.  arraysize only works when you define the array in the
same implementation.  When the array is an extern, the size is unknown so
attempting to use arraysize just leads to a compiler error.

I could put the size in the header, but that seems riskier than having it
defined here.

This might all be a moot point.  You asked me to see if I can refactor the two
places this array is used... so if I succeed in doing that, this array can be
moved to the implementation and then I will be able to use arraysize().  Let me
proceed in that direction.

http://codereview.chromium.org/8528011/diff/1/content/public/common/page_zoom...
content/public/common/page_zoom.cc:19: const double kPageZoomEpsilon = 0.005;
On 2011/11/14 18:09:05, James Hawkins wrote:
> Document var.

It's documented in the header already.  The style guidelines say that we
shouldn't duplicate comments in both the header and implementation.  Are you
asking for redundant documentation, or more detailed documentation?

http://codereview.chromium.org/8528011/diff/1/content/public/common/page_zoom.h
File content/public/common/page_zoom.h (right):

http://codereview.chromium.org/8528011/diff/1/content/public/common/page_zoom...
content/public/common/page_zoom.h:22: // The minimum zoom factor permitted for a
page. This is an alternative to
On 2011/11/14 18:09:05, James Hawkins wrote:
> What do you mean by 'alternative'?

These are the values that we are going to use in Chromium, rather than WebKit's
default values.  The webkit minimum and maximum values are too narrow for
Chromium.  So we're defining our own min/max values and using those in the
appropriate places.

[csilv, 2011-11-15 02:20:30]

James,

If you have time please take a look at this revised CL.  I believe I've
addressed all of your concerns, except for adding tests.  (I will tackle adding
tests on Tuesday).  If you prefer waiting for the tests that's fine, I'll ping
again when those are ready.  thanks!

[csilv, 2011-11-15 02:26:49]

http://codereview.chromium.org/8528011/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/8528011/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:1973: RenderViewHost* host =
GetSelectedTabContentsWrapper()->render_view_host();
On 2011/11/14 18:09:05, James Hawkins wrote:
> When is |host| NULL?

Shouldn't happen, removed.

http://codereview.chromium.org/8528011/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:2021:
UserMetrics::RecordAction(UserMetricsAction("ZoomMinus"));
On 2011/11/14 18:09:05, James Hawkins wrote:
> For the sake of posterity, it would be interesting to record when the user
tries
> to zoom minus when already at the lowest (?) level.  Same for zoom plus.

Done.

http://codereview.chromium.org/8528011/diff/1/chrome/browser/ui/webui/options...
File chrome/browser/ui/webui/options/advanced_options_handler.cc (right):

http://codereview.chromium.org/8528011/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/advanced_options_handler.cc:264:
web_ui()->RegisterMessageCallback("selectDownloadLocation",
I reverted this change here and below.

On 2011/11/14 18:09:05, James Hawkins wrote:
> Ehhh I'm not really a fan of continuously calling a getter method; I'd rather
> pull it into a local var if you don't want it stored on the object.

http://codereview.chromium.org/8528011/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/advanced_options_handler.cc:564: // Generate a
vector of zoom factors from an array of known preset values.
On 2011/11/14 18:09:05, James Hawkins wrote:
> :-/ This is very similar if not exactly like the code in browser.cc. Can we
> refactor somehow?

Done.

http://codereview.chromium.org/8528011/diff/1/content/browser/host_zoom_map.cc
File content/browser/host_zoom_map.cc (right):

http://codereview.chromium.org/8528011/diff/1/content/browser/host_zoom_map.c...
content/browser/host_zoom_map.cc:61: if (std::fabs(level - default_zoom_level_)
<= content::kPageZoomEpsilon)
On 2011/11/14 18:09:05, James Hawkins wrote:
> I'm seeing this a lot; can we refactor this somewhere?

Done.

http://codereview.chromium.org/8528011/diff/1/content/public/common/page_zoom.cc
File content/public/common/page_zoom.cc (right):

http://codereview.chromium.org/8528011/diff/1/content/public/common/page_zoom...
content/public/common/page_zoom.cc:17: const int kPresetZoomFactorsCount = 16;
On 2011/11/14 23:10:29, csilv wrote:
> On 2011/11/14 18:09:05, James Hawkins wrote:
> > Isn't there a macro that does this for you?  Like arraysize() or something.
> Then
> > you can remove the comment above.
> 
> I wish this were the case.  Yes, there is an arraysize macro, but it doesn't
> work for cases like this.  arraysize only works when you define the array in
the
> same implementation.  When the array is an extern, the size is unknown so
> attempting to use arraysize just leads to a compiler error.
> 
> I could put the size in the header, but that seems riskier than having it
> defined here.
> 
> This might all be a moot point.  You asked me to see if I can refactor the two
> places this array is used... so if I succeed in doing that, this array can be
> moved to the implementation and then I will be able to use arraysize().  Let
me
> proceed in that direction.

Done.

http://codereview.chromium.org/8528011/diff/1/content/public/common/page_zoom...
content/public/common/page_zoom.cc:19: const double kPageZoomEpsilon = 0.005;
Improved documentation.  This is an internal value now.

On 2011/11/14 23:10:29, csilv wrote:
> On 2011/11/14 18:09:05, James Hawkins wrote:
> > Document var.
> 
> It's documented in the header already.  The style guidelines say that we
> shouldn't duplicate comments in both the header and implementation.  Are you
> asking for redundant documentation, or more detailed documentation?

[James Hawkins, 2011-11-15 18:35:50]

http://codereview.chromium.org/8528011/diff/12004/chrome/browser/resources/op...
File chrome/browser/resources/options/advanced_options.html (right):

http://codereview.chromium.org/8528011/diff/12004/chrome/browser/resources/op...
chrome/browser/resources/options/advanced_options.html:95: <select
id="defaultZoomFactor" dataType="double"></select>
s/defaultZoomFactor/default-zoom-factor/

To make this mod would cause inconsistency though :-(

http://codereview.chromium.org/8528011/diff/12004/chrome/browser/resources/op...
File chrome/browser/resources/options/advanced_options.js (right):

http://codereview.chromium.org/8528011/diff/12004/chrome/browser/resources/op...
chrome/browser/resources/options/advanced_options.js:179:
AdvancedOptions.SetupPageZoomSelector = function(items) {
Document the method and |items|.

http://codereview.chromium.org/8528011/diff/12004/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/8528011/diff/12004/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:866:
WebKit::WebView::zoomLevelToZoomFactor(GetZoomLevel()) * 100 + 0.5);
SG on the explanation, please add it as a comment in the code.

http://codereview.chromium.org/8528011/diff/12004/content/public/common/page_...
File content/public/common/page_zoom.h (right):

http://codereview.chromium.org/8528011/diff/12004/content/public/common/page_...
content/public/common/page_zoom.h:33: class CONTENT_EXPORT PageZoomHelpers {
Consider using a named namespace instead of a class with static methods.

[csilv, 2011-11-19 00:17:45]

Updated per reviews, added test cases at different levels.  PTAL.

http://codereview.chromium.org/8528011/diff/12004/chrome/browser/resources/op...
File chrome/browser/resources/options/advanced_options.html (right):

http://codereview.chromium.org/8528011/diff/12004/chrome/browser/resources/op...
chrome/browser/resources/options/advanced_options.html:95: <select
id="defaultZoomFactor" dataType="double"></select>
I decided not to do this for consistency, but perhaps I can come back and do a
full-file cleanup as a separate CL.

On 2011/11/15 18:35:51, James Hawkins wrote:
> s/defaultZoomFactor/default-zoom-factor/
> 
> To make this mod would cause inconsistency though :-(

http://codereview.chromium.org/8528011/diff/12004/chrome/browser/resources/op...
File chrome/browser/resources/options/advanced_options.js (right):

http://codereview.chromium.org/8528011/diff/12004/chrome/browser/resources/op...
chrome/browser/resources/options/advanced_options.js:179:
AdvancedOptions.SetupPageZoomSelector = function(items) {
On 2011/11/15 18:35:51, James Hawkins wrote:
> Document the method and |items|.

Done.

http://codereview.chromium.org/8528011/diff/12004/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/8528011/diff/12004/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:866:
WebKit::WebView::zoomLevelToZoomFactor(GetZoomLevel()) * 100 + 0.5);
On 2011/11/15 18:35:51, James Hawkins wrote:
> SG on the explanation, please add it as a comment in the code.

Done.

http://codereview.chromium.org/8528011/diff/12004/content/public/common/page_...
File content/public/common/page_zoom.h (right):

http://codereview.chromium.org/8528011/diff/12004/content/public/common/page_...
content/public/common/page_zoom.h:33: class CONTENT_EXPORT PageZoomHelpers {
On 2011/11/15 18:35:51, James Hawkins wrote:
> Consider using a named namespace instead of a class with static methods.

I addressed this by getting rid of the class (and using the content namespace). 
This approach seems consistent with other similar methods in content.

[James Hawkins, 2011-11-19 00:54:32]

lgtm

[csilv, 2011-11-19 02:42:34]

+jam for OWNERS for review/approval.

Primarily these files:
  content/browser/host_zoom_map.cc
  content/browser/host_zoom_map_unittest.cc
  content/browser/tab_contents/tab_contents.cc
  content/browser/webui/web_ui.cc [.h]
  content/public/common/page_zoom.cc [.h]
  content/public/common/page_zoom_unittest.cc
  content/renderer/render_view_impl.cc

[jam, 2011-11-21 18:24:39]

http://codereview.chromium.org/8528011/diff/25001/content/public/common/page_...
File content/public/common/page_zoom.cc (right):

http://codereview.chromium.org/8528011/diff/25001/content/public/common/page_...
content/public/common/page_zoom.cc:20: const double kPresetZoomFactors[] = {
0.25, 0.333, 0.5, 0.666, 0.75, 0.9, 1.0,
these preset values are all chrome-specific. other embedders might want
different values. since it's related to UI, it really doesn't belong in content
(http://www.chromium.org/developers/content-module), and seems like it belongs
in chrome code. same applies for the functions below.

[csilv, 2011-11-21 21:44:58]

http://codereview.chromium.org/8528011/diff/25001/content/public/common/page_...
File content/public/common/page_zoom.cc (right):

http://codereview.chromium.org/8528011/diff/25001/content/public/common/page_...
content/public/common/page_zoom.cc:20: const double kPresetZoomFactors[] = {
0.25, 0.333, 0.5, 0.666, 0.75, 0.9, 1.0,
On 2011/11/21 18:24:39, John Abd-El-Malek wrote:
> these preset values are all chrome-specific. other embedders might want
> different values. since it's related to UI, it really doesn't belong in
content
> (http://www.chromium.org/developers/content-module), and seems like it belongs
> in chrome code. same applies for the functions below.

Done (relocated to chrome/browser as recommended).

[jam, 2011-11-21 22:20:54]

http://codereview.chromium.org/8528011/diff/34004/chrome/browser/page_zoom.h
File chrome/browser/page_zoom.h (right):

http://codereview.chromium.org/8528011/diff/34004/chrome/browser/page_zoom.h#...
chrome/browser/page_zoom.h:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
for these three files, can we avoid filenames that match files in content? that
makes it hard to open them in IDEs and is generally confusing.

can you prefix the ones in chrome with "chrome_"? that's been the usual
convention for files that extend content's files

http://codereview.chromium.org/8528011/diff/34004/content/public/common/page_...
File content/public/common/page_zoom.cc (right):

http://codereview.chromium.org/8528011/diff/34004/content/public/common/page_...
content/public/common/page_zoom.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: sorry i should have said this earlier, wasn't sure if you were going to
have a cc file here after the comments. if you are, please move this to
content/common/page_zoom.cc. we only leave cc files in content/public that have
constructor/destructors etc

http://codereview.chromium.org/8528011/diff/34004/content/public/common/page_...
File content/public/common/page_zoom_unittest.cc (right):

http://codereview.chromium.org/8528011/diff/34004/content/public/common/page_...
content/public/common/page_zoom_unittest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
unit tests belong in content/common as well

[csilv, 2011-11-22 00:10:58]

http://codereview.chromium.org/8528011/diff/34004/chrome/browser/page_zoom.h
File chrome/browser/page_zoom.h (right):

http://codereview.chromium.org/8528011/diff/34004/chrome/browser/page_zoom.h#...
chrome/browser/page_zoom.h:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
On 2011/11/21 22:20:54, John Abd-El-Malek wrote:
> for these three files, can we avoid filenames that match files in content?
that
> makes it hard to open them in IDEs and is generally confusing.
> 
> can you prefix the ones in chrome with "chrome_"? that's been the usual
> convention for files that extend content's files

Done.

http://codereview.chromium.org/8528011/diff/34004/content/public/common/page_...
File content/public/common/page_zoom.cc (right):

http://codereview.chromium.org/8528011/diff/34004/content/public/common/page_...
content/public/common/page_zoom.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2011/11/21 22:20:54, John Abd-El-Malek wrote:
> nit: sorry i should have said this earlier, wasn't sure if you were going to
> have a cc file here after the comments. if you are, please move this to
> content/common/page_zoom.cc. we only leave cc files in content/public that
have
> constructor/destructors etc

Done.

http://codereview.chromium.org/8528011/diff/34004/content/public/common/page_...
File content/public/common/page_zoom_unittest.cc (right):

http://codereview.chromium.org/8528011/diff/34004/content/public/common/page_...
content/public/common/page_zoom_unittest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2011/11/21 22:20:54, John Abd-El-Malek wrote:
> unit tests belong in content/common as well

Done.

[jam, 2011-11-22 01:22:43]

lgtm

http://codereview.chromium.org/8528011/diff/38004/chrome/browser/chrome_page_...
File chrome/browser/chrome_page_zoom_unittest.cc (right):

http://codereview.chromium.org/8528011/diff/38004/chrome/browser/chrome_page_...
chrome/browser/chrome_page_zoom_unittest.cc:7: 
nit: not needed

[jam, 2012-01-19 20:09:35]

hi, I just noticed that this change added a web_ui_unittest.cc but didn't add it
the list of tests to run. they also don't pass.

[csilv, 2012-01-19 21:23:15]

On 2012/01/19 20:09:35, John Abd-El-Malek wrote:
> hi, I just noticed that this change added a web_ui_unittest.cc but didn't add
it
> the list of tests to run. they also don't pass.

Thanks for the heads up.  Reported as http://crbug.com/110784