Show Zoom Levels as content settings

Show Zoom Levels as content settings

This CL shows page zoom levels as content settings in chrome://settings/content
and allows clearing them.

BUG=137412
TEST=Open a site example.com, zoom using Ctrl + +, observe how settings change in chrome://settings/content -> Zoom levels; reset some zoom levels.
R=dbeam@chromium.org, joi@chromium.org, markusheintz@chromium.org, sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257730

[battre, 2014-03-12 14:54:12]

Hi Markus,

could you please review this CL?

Thanks,
Dominic

[markusheintz_, 2014-03-12 16:31:05]

https://codereview.chromium.org/197523002/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/197523002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:8323: +      <message
name="IDS_ZOOMLEVELS_TAB_LABEL" desc="Label for Zoom Level tab on Content
Settings dialog (zoom levels indicate that a page should be shown for example at
110% magnification).">
Do we really need two separate resources strings yet?

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

https://codereview.chromium.org/197523002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:1024:
exception->SetString(kZoom, base::IntToString(percent) + "%");
hm ... I guess the string concatenation should work for RTL as well. Maybe we
should try it out.

https://codereview.chromium.org/197523002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:1246:
std::string::size_type scheme_separator_position = pattern.find("://");
In src/content/public/common/url_constants.cc there is a const for the standard
scheme separator (content::kStandardSchemeSeparator). Please use it.

https://codereview.chromium.org/197523002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:1251: std::string
host = pattern.substr(scheme_separator_position + 3);
Please replace the '3' with the length of the content::kStandardSchemeSeparator.

https://codereview.chromium.org/197523002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:1383: if
(type_string == kZoomContentType) {
Please add a comment why this needs to go before the following code. So that
people that are touching the code later know that they need to keep it where it
is.

kZoomContentType can not be converted by "ContentSettingsTypeFromGroupName".

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

https://codereview.chromium.org/197523002/diff/1/content/browser/host_zoom_ma...
content/browser/host_zoom_map_impl.cc:88:
std::vector<HostZoomMap::ZoomLevelChange> result;
Can't  you use HostZoomMap::ZoomLevelVector as type here too?

[battre, 2014-03-13 09:22:14]

Thanks! PTAL.

Dominic

https://codereview.chromium.org/197523002/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/197523002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:8323: +      <message
name="IDS_ZOOMLEVELS_TAB_LABEL" desc="Label for Zoom Level tab on Content
Settings dialog (zoom levels indicate that a page should be shown for example at
110% magnification).">
On 2014/03/12 16:31:06, markusheintz_ wrote:
> Do we really need two separate resources strings yet?

I don't know whether tabs hand headers require different translations but I am
ok with removing one.

Done.

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

https://codereview.chromium.org/197523002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:1024:
exception->SetString(kZoom, base::IntToString(percent) + "%");
On 2014/03/12 16:31:06, markusheintz_ wrote:
> hm ... I guess the string concatenation should work for RTL as well. Maybe we
> should try it out.

Done.

https://codereview.chromium.org/197523002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:1246:
std::string::size_type scheme_separator_position = pattern.find("://");
On 2014/03/12 16:31:06, markusheintz_ wrote:
> In src/content/public/common/url_constants.cc there is a const for the
standard
> scheme separator (content::kStandardSchemeSeparator). Please use it.

Done.

https://codereview.chromium.org/197523002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:1246:
std::string::size_type scheme_separator_position = pattern.find("://");
On 2014/03/12 16:31:06, markusheintz_ wrote:
> In src/content/public/common/url_constants.cc there is a const for the
standard
> scheme separator (content::kStandardSchemeSeparator). Please use it.

Done.

https://codereview.chromium.org/197523002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:1251: std::string
host = pattern.substr(scheme_separator_position + 3);
On 2014/03/12 16:31:06, markusheintz_ wrote:
> Please replace the '3' with the length of the
content::kStandardSchemeSeparator.

Done.

https://codereview.chromium.org/197523002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:1383: if
(type_string == kZoomContentType) {
On 2014/03/12 16:31:06, markusheintz_ wrote:
> Please add a comment why this needs to go before the following code. So that
> people that are touching the code later know that they need to keep it where
it
> is.
> 
> kZoomContentType can not be converted by "ContentSettingsTypeFromGroupName". 

Done.

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

https://codereview.chromium.org/197523002/diff/1/content/browser/host_zoom_ma...
content/browser/host_zoom_map_impl.cc:88:
std::vector<HostZoomMap::ZoomLevelChange> result;
On 2014/03/12 16:31:06, markusheintz_ wrote:
> Can't  you use HostZoomMap::ZoomLevelVector as type here too?

Done.

[markusheintz_, 2014-03-13 09:37:14]

One last comment.

LGTM otherwise

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/content_settings_exceptions_area.html
(right):

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/content_settings_exceptions_area.html:11: <div
id="exception-zoom-column"
Do we really need to add an additional column because of the resource strings?
Or would it possible to just set the string of the second column.

Do you happen to know how this is done by the protocol handler dialog?

[battre, 2014-03-13 14:04:28]

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/content_settings_exceptions_area.html
(right):

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/content_settings_exceptions_area.html:11: <div
id="exception-zoom-column"
On 2014/03/13 09:37:14, markusheintz_ wrote:
> Do we really need to add an additional column because of the resource strings?
> Or would it possible to just set the string of the second column.
> 
> Do you happen to know how this is done by the protocol handler dialog?

They have implemented their own handler at chrome://settings/handlers. I think
this is strictly more complicated and worse.

My feeling is that the current approach is fine but I will eave that decision to
the owners.

[battre, 2014-03-13 14:07:38]

Dear OWNERS,

could you please review this CL?

dbeam for chrome/browser/**/options/*
joi for content/public/host_zoom_map.h
sky for content/browser/*

Thanks,
Dominic

[Jói, 2014-03-13 14:09:05]

//content/public LGTM

[sky, 2014-03-13 16:03:14]

LGTM

[Dan Beam, 2014-03-15 15:28:50]

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/content_settings.css (right):

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/content_settings.css:46: #exception-zoom-column
{
Any reason you didn't combine this with the rule above?

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/content_settings_exceptions_area.js:130:
this.editable = false;
nit: \n

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:263: // (a.com <
http://a.com < https://a.com < b.com)
nit: punctuation

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:264: bool
ZoomLevelChangeLessThan(const content::HostZoomMap::ZoomLevelChange& a,
nit: ZoomHostSort

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:270: return a.scheme
< b.scheme;
nit:

return a.host == b.host ? a.scheme < b.scheme : a.host < b.host;

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:713: // separately.
nit: are not actually a content type,

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:1009:
base::DictionaryValue* exception = new base::DictionaryValue();
nit: scoped_ptr + release() later

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:1027:
content::ZoomLevelToZoomFactor(i->zoom_level) * 100 + 0.5);
This is mildly cryptic but I didn't know std::round() is c++11

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:1242: bool rv =
args->GetString(arg_index++, &mode);
Can you just make this arg_index?

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:1246: rv =
args->GetString(arg_index++, &pattern);
and arg_index + 1

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/content_settings_handler.h (right):

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.h:137: void
RemoveZoomLevelException(const base::ListValue* args, size_t arg_index);
s/excpetion/exception

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.h:137: void
RemoveZoomLevelException(const base::ListValue* args, size_t arg_index);
What is in |args|?

[battre, 2014-03-17 23:00:19]

Thanks a lot, Dan. Please take another look.

Best regards,
Dominic

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/content_settings.css (right):

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/content_settings.css:46: #exception-zoom-column
{
On 2014/03/15 15:28:51, Dan Beam wrote:
> Any reason you didn't combine this with the rule above?

Done

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/content_settings_exceptions_area.js:130:
this.editable = false;
On 2014/03/15 15:28:51, Dan Beam wrote:
> nit: \n

Done.

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:263: // (a.com <
http://a.com < https://a.com < b.com)
On 2014/03/15 15:28:51, Dan Beam wrote:
> nit: punctuation

Done.

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:264: bool
ZoomLevelChangeLessThan(const content::HostZoomMap::ZoomLevelChange& a,
On 2014/03/15 15:28:51, Dan Beam wrote:
> nit: ZoomHostSort

Done.

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:270: return a.scheme
< b.scheme;
On 2014/03/15 15:28:51, Dan Beam wrote:
> nit:
> 
> return a.host == b.host ? a.scheme < b.scheme : a.host < b.host;

Done.

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:713: // separately.
On 2014/03/15 15:28:51, Dan Beam wrote:
> nit: are not actually a content type,

Done.

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:1009:
base::DictionaryValue* exception = new base::DictionaryValue();
On 2014/03/15 15:28:51, Dan Beam wrote:
> nit: scoped_ptr + release() later

Done.

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:1027:
content::ZoomLevelToZoomFactor(i->zoom_level) * 100 + 0.5);
On 2014/03/15 15:28:51, Dan Beam wrote:
> This is mildly cryptic but I didn't know std::round() is c++11

Ok, I'll leave it as it is.

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:1242: bool rv =
args->GetString(arg_index++, &mode);
On 2014/03/15 15:28:51, Dan Beam wrote:
> Can you just make this arg_index?

I could, but I think I would rather keep consistency with all the other
Remove...() functions here and actually I find this pretty readable.

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:1246: rv =
args->GetString(arg_index++, &pattern);
On 2014/03/15 15:28:51, Dan Beam wrote:
> and arg_index + 1

see above.

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/content_settings_handler.h (right):

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.h:137: void
RemoveZoomLevelException(const base::ListValue* args, size_t arg_index);
On 2014/03/15 15:28:51, Dan Beam wrote:
> s/excpetion/exception

Done.

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.h:137: void
RemoveZoomLevelException(const base::ListValue* args, size_t arg_index);
On 2014/03/15 15:28:51, Dan Beam wrote:
> What is in |args|?

I have done this in the first function that uses the args, arg_index pair above.

Done.

[Dan Beam, 2014-03-18 00:10:34]

lgtm w/comment

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/197523002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:1242: bool rv =
args->GetString(arg_index++, &mode);
On 2014/03/17 23:00:19, battre wrote:
> On 2014/03/15 15:28:51, Dan Beam wrote:
> > Can you just make this arg_index?
> 
> I could, but I think I would rather keep consistency with all the other
> Remove...() functions here and actually I find this pretty readable.

the existing code is:
1) silly
2) local to this file (afaict), and probably copy pasted from an original
example
3) continuing to result in more copy pasta of silly code

I don't like to ask people to update unrelated code but I don't think you should
copy this.

[Dan Beam, 2014-03-18 00:13:23]

https://codereview.chromium.org/197523002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/content_settings_handler.h (right):

https://codereview.chromium.org/197523002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.h:126: //
RemoveException().
i have little to no idea what this means, though...

|arg_index| is the arg to be removed?

[battre, 2014-03-18 12:38:53]

I'll land this as is and create a second CL that removes all occurrences of
arg_index.

Best regards,
Dominic

[battre, 2014-03-18 12:39:01]

The CQ bit was checked by battre@chromium.org

[commit-bot: I haz the power, 2014-03-18 12:39:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/197523002/40001

[commit-bot: I haz the power, 2014-03-18 18:45:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-18 18:45:14]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[battre, 2014-03-18 20:37:29]

Committed patchset #4 manually as r257730 (presubmit successful).