Site Settings Desktop: Implement USB devices section.

Site Settings Desktop: Implement USB devices section.

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

Committed: https://crrev.com/0d9862f8d67ee97a5b6ec1cbdd19395f5720d0c0
Cr-Commit-Position: refs/heads/master@{#412212}

[Finnur, 2016-08-11 14:29:23]

Description was changed from

==========
Site Settings Desktop: Implement USB devices section.

BUG=635845, 614277
==========

to

==========
Site Settings Desktop: Implement USB devices section.

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

[Finnur, 2016-08-11 14:29:26]

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

[commit-bot: I haz the power, 2016-08-11 14:29: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-08-11 14:38:29]

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

[commit-bot: I haz the power, 2016-08-11 14:38:29]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[Finnur, 2016-08-11 14:52:32]

Patchset #1 (id:1) has been deleted

[Finnur, 2016-08-11 14:52:40]

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

[commit-bot: I haz the power, 2016-08-11 14:53:22]

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

[Finnur, 2016-08-11 14:56:28]

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

[commit-bot: I haz the power, 2016-08-11 15:43:28]

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

[commit-bot: I haz the power, 2016-08-11 15:43:29]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-08-11 23:28:07]

It looks like this is affecting the old options 
code as well. Could that be avoided?

If we do end up changing the old options code,
what should be tested to assure that the old
options is not damaged?

Also, would it be reasonable to break this CL
into smaller pieces?

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_resources.grd:895: type="chrome_html"
/>
nit: the indent is off here.

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:351: }
If this is only used in site settings, please move it to 
where it is used. If it's useful to more than site settings,
please give it a name that is more general.
(This file is intended for classes that are used in several
sections).

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
(right):

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:187:
* @param {UsbDeviceDetails} object The USB device object to revoke
nit: how about the name |usbDevice| instead of |object| for the parameter?

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:288:
removeUsbDevice: function(origin, embeddingOrigin, object) {
Nit: is there another param name we can use other than |object|
it's too generic imo.

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/usb_devices.html (right):

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/usb_devices.html:9: <link
rel="import" href="/site_settings/site_settings_prefs_browser_proxy.html"-->
It looks like there is the end of a comment on this import.
"-->"

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/usb_devices.html:27: 
This blank line seems unnecessary.

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/usb_devices.js (right):

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/usb_devices.js:8: * USB Devices
category under Site Settings.
nit: more of this line could go on the line above.

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/usb_devices.js:45:
console.log(item);
Please remove the console.log.

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/site_settings_helper.h (right):

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/site_settings_helper.h:70: // This struct is declared
early so that it can used by functions below.
I'd rather this comment said something about what a 
ChooserTypeNameEntry is. Those are four common
words strung together that don't convey much context.

[Finnur, 2016-08-12 11:32:27]

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

[commit-bot: I haz the power, 2016-08-12 11:32:47]

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-08-12 11:34:50]

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

[commit-bot: I haz the power, 2016-08-12 11:34:51]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Finnur, 2016-08-12 12:42:38]

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

[commit-bot: I haz the power, 2016-08-12 12:42:48]

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

[Finnur, 2016-08-12 13:11:26]

> It looks like this is affecting the old options 
> code as well. Could that be avoided?

I now realize I meant to add two comments before sending this out for review.
One was something along these lines (the other will be added as a question
elsewhere): 

So... the refactor touches the old options code, correct. But there's also no
material change to the options code, I'm just moving the code to a central
location to avoid copying it (avoiding future maintenance headaches if someone
wants to change the old code). The site_settings_helper files include other such
code that has been moved to a central location (that's why it exists in the
first place, in fact).

> If we do end up changing the old options code,
> what should be tested to assure that the old
> options is not damaged?

Generally speaking: That's what the testing framework should catch (assuming the
test coverage is good enough). Beyond that, I'm not sure what is reasonable to
do, except perhaps manual testing to see the functionality still works. Since
I'm not the original implementor my testing would (unintentionally) not be as
thorough as the owner would (who has the best knowledge of the code). But, I
think it is kind of a moot point, because the old code shouldn't, and I believe
isn't changing in this CL so I have reasonable confidence that it isn't
affected.

> Also, would it be reasonable to break this CL
> into smaller pieces?

That is always good to keep in mind. But I don't know... I think this CL looks a
lot bigger than it actually is, and a logical split would be to take the
code-move and do that in a separate CL. But since the code isn't changing, it
would only serve as an indicator of what is purely a code-move and what is new
implementation and that should already be fairly obvious (code is moving
wholesale from content_settings_handler to site_settings_helper). I feel
splitting it up into two CL wouldn't add much to our understanding of it. Does
that sound reasonable?

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_resources.grd:895: type="chrome_html"
/>
On 2016/08/11 23:28:07, dschuyler wrote:
> nit: the indent is off here.

Done.

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:351: }
Sorry, I interpreted your words at our encounter in LA as an indication that if
a css rule was used in more than one place, it should belong centrally (which I
thought was the settings_shared css). Moved this back.

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
(right):

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:187:
* @param {UsbDeviceDetails} object The USB device object to revoke
I like that idea.

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:288:
removeUsbDevice: function(origin, embeddingOrigin, object) {
On 2016/08/11 23:28:07, dschuyler wrote:
> Nit: is there another param name we can use other than |object|
> it's too generic imo.

Done.

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/usb_devices.html (right):

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/usb_devices.html:9: <link
rel="import" href="/site_settings/site_settings_prefs_browser_proxy.html"-->
Good catch.

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/usb_devices.html:27: 
On 2016/08/11 23:28:07, dschuyler wrote:
> This blank line seems unnecessary.

Done.

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/usb_devices.js (right):

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/usb_devices.js:8: * USB Devices
category under Site Settings.
On 2016/08/11 23:28:07, dschuyler wrote:
> nit: more of this line could go on the line above.

Done.

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/usb_devices.js:45:
console.log(item);
Oops, done.

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/site_settings_helper.h (right):

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/site_settings_helper.h:70: // This struct is declared
early so that it can used by functions below.
Took a stab at it.

https://codereview.chromium.org/2237823003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/usb_devices.html (right):

https://codereview.chromium.org/2237823003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/usb_devices.html:33:
<paper-menu-button>
I forgot to ask... do you know why the dropdown menu sticks around after I
select Remove? (I have to click outside to close it). I don't see anything
obvious that would cause that and my searches have come up empty.

[commit-bot: I haz the power, 2016-08-12 13:37:59]

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

[commit-bot: I haz the power, 2016-08-12 13:38:00]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-08-12 20:58:49]

LGTM with some nits/suggestions.

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:351: }
On 2016/08/12 13:11:25, Finnur wrote:
> Sorry, I interpreted your words at our encounter in LA as an indication that
if
> a css rule was used in more than one place, it should belong centrally (which
I
> thought was the settings_shared css). Moved this back.

Oh, that's correct. If it's used in more places then it should be moved to a 
more central place (such as this file). I didn't mean to contradict that. 
I could have been more clear by saying:

"If this is only used in **protocol_handlers.html**, please move it to 
where it is used. If it's useful to more than **protocol_handlers.html**,
please give it a name that is more general".

The comment about the name is because it would be weird if it were used in
Appearance or Search Engines settings and had the name "site-settings-*".

I see now that this is being added in usb_devices.html in addition to 
protocol_handlers.html. I think I'm fine with any of these: have two copies of
this style class (not ideal, though not terrible); or have a 
site_settings_css.html with the style; or add it to 
settings_shared_css.html with a different name on the class.

https://codereview.chromium.org/2237823003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/usb_devices.html (right):

https://codereview.chromium.org/2237823003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/usb_devices.html:33:
<paper-menu-button>
On 2016/08/12 13:11:26, Finnur wrote:
> I forgot to ask... do you know why the dropdown menu sticks around after I
> select Remove? (I have to click outside to close it). I don't see anything
> obvious that would cause that and my searches have come up empty.

Not right off. I just tried it and that doesn't happen in the build that
I have.

https://codereview.chromium.org/2237823003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.h (right):

https://codereview.chromium.org/2237823003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.h:36: // Usage info:
nit: it's an oddity that some comments end with ':'.
I think this comment should end in a period, because
the pattern for the colon comments is that they refer
to overridden methods of a base class.

https://codereview.chromium.org/2237823003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/site_settings_helper.cc (right):

https://codereview.chromium.org/2237823003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/site_settings_helper.cc:43: AllOriginObjects;
Optional: I'd find it easier to read if there were 
black lines between the three things above.

https://codereview.chromium.org/2237823003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/site_settings_helper.h (right):

https://codereview.chromium.org/2237823003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/site_settings_helper.h:119: // Fills in |exceptions|
with Values for the given |type| from |map|.
nit: should this say |chooser_type|?
and the should |map| be map instead?

[Finnur, 2016-08-15 11:51:56]

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

[commit-bot: I haz the power, 2016-08-15 11:52:02]

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-08-15 11:53:31]

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

[commit-bot: I haz the power, 2016-08-15 11:53:32]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Finnur, 2016-08-15 11:54:32]

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

[Finnur, 2016-08-15 11:54:34]

Steven, can you to OWNERS check on the code move from 
webui/options/content_settings_handler.*
... to ...
webui/site_settings_helper.*

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2237823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:351: }
Fair enough. I'll leave it as is for now and if I add more instances of this, or
more shared values for Site Settings then I'll consolidate those all into a
separate css file for site_settings.

https://codereview.chromium.org/2237823003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.h (right):

https://codereview.chromium.org/2237823003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.h:36: // Usage info:
On 2016/08/12 20:58:48, dschuyler wrote:
> nit: it's an oddity that some comments end with ':'.
> I think this comment should end in a period, because
> the pattern for the colon comments is that they refer
> to overridden methods of a base class.  

Done.

https://codereview.chromium.org/2237823003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/site_settings_helper.cc (right):

https://codereview.chromium.org/2237823003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/site_settings_helper.cc:43: AllOriginObjects;
Agreed.

https://codereview.chromium.org/2237823003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/site_settings_helper.h (right):

https://codereview.chromium.org/2237823003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/site_settings_helper.h:119: // Fills in |exceptions|
with Values for the given |type| from |map|.
On 2016/08/12 20:58:49, dschuyler wrote:
> nit: should this say |chooser_type|?
> and the should |map| be map instead?

Done.

[Finnur, 2016-08-15 12:07:47]

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

[commit-bot: I haz the power, 2016-08-15 12:07:55]

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

[Finnur, 2016-08-15 12:11:12]

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

[Finnur, 2016-08-15 12:11:12]

Hmm... looks like I need an OWNERS check for the webui dir as well (my new file
site_settings_helper.*)

Bernhard?

[commit-bot: I haz the power, 2016-08-15 12:49:30]

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

[commit-bot: I haz the power, 2016-08-15 12:49:32]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-08-15 14:14:55]

lgtm

https://codereview.chromium.org/2237823003/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/2237823003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/content_settings_handler.cc:103:
AllOriginObjects;
Do we still need these?

https://codereview.chromium.org/2237823003/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/site_settings_helper.cc (right):

https://codereview.chromium.org/2237823003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/site_settings_helper.cc:271: if
(profile->HasOffTheRecordProfile())
Invert the condition and return early?

[stevenjb, 2016-08-15 19:01:54]

owner lgtm

https://codereview.chromium.org/2237823003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/2237823003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_resources.grd:883: <structure
name="IDR_SETTINGS_USB_DEVICES_HTML"
Fix indent

[Finnur, 2016-08-16 10:13:55]

Thanks for reviewing. All addressed. Checking in.

https://codereview.chromium.org/2237823003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/2237823003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_resources.grd:883: <structure
name="IDR_SETTINGS_USB_DEVICES_HTML"
On 2016/08/15 19:01:54, stevenjb wrote:
> Fix indent

Done.

https://codereview.chromium.org/2237823003/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/2237823003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/content_settings_handler.cc:103:
AllOriginObjects;
Good catch. Nope.

https://codereview.chromium.org/2237823003/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/site_settings_helper.cc (right):

https://codereview.chromium.org/2237823003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/site_settings_helper.cc:271: if
(profile->HasOffTheRecordProfile())
On 2016/08/15 14:14:54, Bernhard Bauer wrote:
> Invert the condition and return early?

Done.

[Finnur, 2016-08-16 10:14:01]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2016-08-16 10:14:02]

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

[commit-bot: I haz the power, 2016-08-16 10:14:16]

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-08-16 11:09:41]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-08-16 11:11:47]

Description was changed from

==========
Site Settings Desktop: Implement USB devices section.

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

to

==========
Site Settings Desktop: Implement USB devices section.

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

Committed: https://crrev.com/0d9862f8d67ee97a5b6ec1cbdd19395f5720d0c0
Cr-Commit-Position: refs/heads/master@{#412212}
==========

[commit-bot: I haz the power, 2016-08-16 11:11:49]

Patchset 8 (id:??) landed as
https://crrev.com/0d9862f8d67ee97a5b6ec1cbdd19395f5720d0c0
Cr-Commit-Position: refs/heads/master@{#412212}