Site Settings Desktop: Implement Zoom Levels category.

Site Settings Desktop: Implement Zoom Levels category.

BUG=635846, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/1fbf113bd0f643517fbb0e6e5a7ac832605e2ccb
Cr-Commit-Position: refs/heads/master@{#417554}

[Finnur, 2016-09-08 09:43:23]

Description was changed from

==========
Site Settings Desktop: Implement Zoom Levels category.

BUG=635846, 614277
==========

to

==========
Site Settings Desktop: Implement Zoom Levels category.

BUG=635846, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Finnur, 2016-09-08 09:43:28]

The CQ bit was checked by finnur@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 09:43:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 10:01:03]

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

[commit-bot: I haz the power, 2016-09-08 10:01:04]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Finnur, 2016-09-08 10:46:59]

The CQ bit was checked by finnur@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 10:47:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 10:58:32]

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

[commit-bot: I haz the power, 2016-09-08 10:58:33]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Finnur, 2016-09-08 11:40:27]

The CQ bit was checked by finnur@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 11:40:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Finnur, 2016-09-08 15:36:33]

The CQ bit was checked by finnur@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 15:36:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 16:33:53]

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

[commit-bot: I haz the power, 2016-09-08 16:33:54]

Dry run: This issue passed the CQ dry run.

[Finnur, 2016-09-08 20:45:52]

finnur@chromium.org changed reviewers:
+ dschuyler@chromium.org

[Finnur, 2016-09-08 20:45:53]

https://codereview.chromium.org/2323693002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js
(left):

https://codereview.chromium.org/2323693002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:65: //
    can be ripped out.
Already done.

[dschuyler, 2016-09-08 22:14:32]

LGTM with nits.

https://codereview.chromium.org/2323693002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/zoom_levels.html (right):

https://codereview.chromium.org/2323693002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/zoom_levels.html:37: </div>
nit: I think this can be written without the outer div.
The class="middle" div should push the icon to the start
and the other divs to the end.

          <div class="favicon-image"
              style$="[[computeSiteIcon(item.origin)]]">
          </div>
          <div class="middle">
            <div>[[item.origin]]</div>
          </div>
          <div>[[item.zoom]]</div>
          <div>
            <paper-icon-button icon="settings:delete"
                on-tap="removeZoomLevel_">
          </div>

https://codereview.chromium.org/2323693002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/2323693002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:593: std::string host
= i->host;
nit: the two lines above could be swapped and the local used 
in the function call.

        std::string host = i->host;
        exception->SetString(site_settings::kOrigin, host);

[dschuyler, 2016-09-08 22:17:00]

Is the trashcan icon is the correct icon for 
removing a zoom level? I thought it may be the X icon
or something else.

[Finnur, 2016-09-09 09:56:18]

The trashcan is not the right icon. I'll check this in as-is and do a follow-up.

https://codereview.chromium.org/2323693002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/zoom_levels.html (right):

https://codereview.chromium.org/2323693002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/zoom_levels.html:37: </div>
On 2016/09/08 22:14:32, dschuyler wrote:
> nit: I think this can be written without the outer div.
> The class="middle" div should push the icon to the start
> and the other divs to the end.
> 
>           <div class="favicon-image"
>               style$="[[computeSiteIcon(item.origin)]]">
>           </div>
>           <div class="middle">
>             <div>[[item.origin]]</div>
>           </div>
>           <div>[[item.zoom]]</div>
>           <div>
>             <paper-icon-button icon="settings:delete"
>                 on-tap="removeZoomLevel_">
>           </div>

Done.

https://codereview.chromium.org/2323693002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/2323693002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:593: std::string host
= i->host;
This was copied verbatim from content_settings_handler.cc, but now that I look
at it again I notice that line 592 is redundant (due to line 598). So, I removed
it.

[Finnur, 2016-09-09 09:56:55]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2016-09-09 09:56:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2323693002/#ps100001
(title: "Address feedback")

[commit-bot: I haz the power, 2016-09-09 09:57:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 10:50:27]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-09 10:51:52]

Description was changed from

==========
Site Settings Desktop: Implement Zoom Levels category.

BUG=635846, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Site Settings Desktop: Implement Zoom Levels category.

BUG=635846, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/1fbf113bd0f643517fbb0e6e5a7ac832605e2ccb
Cr-Commit-Position: refs/heads/master@{#417554}
==========

[commit-bot: I haz the power, 2016-09-09 10:51:53]

Patchset 6 (id:??) landed as
https://crrev.com/1fbf113bd0f643517fbb0e6e5a7ac832605e2ccb
Cr-Commit-Position: refs/heads/master@{#417554}