Add USB devices to the content settings page.

Add USB devices to the content settings page.

This patch adds infrastructure for displaying chooser-style permissions
such as USB devices selected by the user on the content settings page.
These permissions are used by the WebUSB API. This infrastructure will
be used for per-device WebBluetooth permissions as well.

BUG=424667, 529950
Committed: https://crrev.com/92e28e54832687afef1c6e9968fcd4608be05db9
Cr-Commit-Position: refs/heads/master@{#362290}

[Reilly Grant (use Gerrit), 2015-11-14 00:01:09]

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

[Reilly Grant (use Gerrit), 2015-11-14 00:01:10]

Please take a look.

go/chrome-chooser-design for more details on this series patches.

[Reilly Grant (use Gerrit), 2015-11-14 01:02:43]

Description was changed from

==========
Add USB devices to the content settings page.

This patch adds infrastructure for displaying chooser-style permissions
such as USB devices selected by the user on the content settings page.
These permissions are used by the WebUSB API. This infrastructure will
be used for per-device WebBluetooth permissions as well.

BUG=529950
==========

to

==========
Add USB devices to the content settings page.

This patch adds infrastructure for displaying chooser-style permissions
such as USB devices selected by the user on the content settings page.
These permissions are used by the WebUSB API. This infrastructure will
be used for per-device WebBluetooth permissions as well.

BUG=424667,529950
==========

[Bernhard Bauer, 2015-11-16 12:53:31]

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:40: else if
(contentType == 'zoomlevels')
Else is not necessary if the previous branch returns.

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:160:
this.dataItem.hasOwnProperty('object')) {
I don't think you need to work around any Javascript weirdnesses here -- you can
just check for `dataItem.object`.

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:662:
.forEach(function(div) { div.hidden = (div.id != valueColumnId); });
Move the code inside the loop to a new line?

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

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:75: struct
ChooserTypeNameEntry {
Move this into the anonymous namespace? I also wouldn't have a problem with
defining it right where it's used.

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:1136:
ScopedVector<base::ListValue> all_provider_exceptions;
I think there is some code here that is shared with
GetExceptionsFromHostContentSettingsMap() and could be extracted into common
methods? For example, the initialization here, or the appending loop below.

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:1200:
ScopedVector<base::Value> reverse_helper;
Also, this should use std::vector<scoped_ptr<>>. Or maybe you could use
std::list<scoped_ptr<>> (or std::deque?) so you can remove elements from the
front and get rid of the double loop?

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:1602: const
base::DictionaryValue* object;
Initialize this to null? Otherwise you might end up with an uninitialized value
in a release build.

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:1607: if (mode !=
"normal") {
Braces are unnecessary.

[Reilly Grant (use Gerrit), 2015-11-16 22:18:27]

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:40: else if
(contentType == 'zoomlevels')
On 2015/11/16 12:53:30, Bernhard Bauer wrote:
> Else is not necessary if the previous branch returns.

Done.

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:160:
this.dataItem.hasOwnProperty('object')) {
On 2015/11/16 12:53:30, Bernhard Bauer wrote:
> I don't think you need to work around any Javascript weirdnesses here -- you
can
> just check for `dataItem.object`.

Done.

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:662:
.forEach(function(div) { div.hidden = (div.id != valueColumnId); });
On 2015/11/16 12:53:30, Bernhard Bauer wrote:
> Move the code inside the loop to a new line?

Done.

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

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:75: struct
ChooserTypeNameEntry {
On 2015/11/16 12:53:30, Bernhard Bauer wrote:
> Move this into the anonymous namespace? I also wouldn't have a problem with
> defining it right where it's used.

It is necessary to define this type outside of the anonymous namespace as it is
used as an argument to some ContentSettingsHandler methods.

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:1136:
ScopedVector<base::ListValue> all_provider_exceptions;
On 2015/11/16 12:53:31, Bernhard Bauer wrote:
> I think there is some code here that is shared with
> GetExceptionsFromHostContentSettingsMap() and could be extracted into common
> methods? For example, the initialization here, or the appending loop below.

I've rewritten the code in both places to avoid the complex initialization and
loops so the sharable code is minimal.

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:1200:
ScopedVector<base::Value> reverse_helper;
On 2015/11/16 12:53:30, Bernhard Bauer wrote:
> Also, this should use std::vector<scoped_ptr<>>. Or maybe you could use
> std::list<scoped_ptr<>> (or std::deque?) so you can remove elements from the
> front and get rid of the double loop?

Done.

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:1602: const
base::DictionaryValue* object;
On 2015/11/16 12:53:30, Bernhard Bauer wrote:
> Initialize this to null? Otherwise you might end up with an uninitialized
value
> in a release build.

Done.

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:1607: if (mode !=
"normal") {
On 2015/11/16 12:53:30, Bernhard Bauer wrote:
> Braces are unnecessary.

Done.

[Bernhard Bauer, 2015-11-17 13:36:54]

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

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:75: struct
ChooserTypeNameEntry {
On 2015/11/16 22:18:27, Reilly Grant wrote:
> On 2015/11/16 12:53:30, Bernhard Bauer wrote:
> > Move this into the anonymous namespace? I also wouldn't have a problem with
> > defining it right where it's used.
> 
> It is necessary to define this type outside of the anonymous namespace as it
is
> used as an argument to some ContentSettingsHandler methods.

Oh, right! In that case, can you move it into the ContentSettingsHandler class
(keep forward-declaring it in the header and defining in this file), so it
doesn't show up in the global namespace?

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

https://codereview.chromium.org/1447813002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:1170: //
|requesting_origin| has been embedded.
Nit: Move this to the previous line.

https://codereview.chromium.org/1447813002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:1172:
requesting_origin, requesting_origin, source, "", nullptr));
Use the empty std::string() constructor to save some instructions.

[Reilly Grant (use Gerrit), 2015-11-18 00:30:09]

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

https://codereview.chromium.org/1447813002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:75: struct
ChooserTypeNameEntry {
On 2015/11/17 13:36:53, Bernhard Bauer wrote:
> On 2015/11/16 22:18:27, Reilly Grant wrote:
> > On 2015/11/16 12:53:30, Bernhard Bauer wrote:
> > > Move this into the anonymous namespace? I also wouldn't have a problem
with
> > > defining it right where it's used.
> > 
> > It is necessary to define this type outside of the anonymous namespace as it
> is
> > used as an argument to some ContentSettingsHandler methods.
> 
> Oh, right! In that case, can you move it into the ContentSettingsHandler class
> (keep forward-declaring it in the header and defining in this file), so it
> doesn't show up in the global namespace?

Done.

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

https://codereview.chromium.org/1447813002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:1170: //
|requesting_origin| has been embedded.
On 2015/11/17 13:36:53, Bernhard Bauer wrote:
> Nit: Move this to the previous line.

Done.

https://codereview.chromium.org/1447813002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:1172:
requesting_origin, requesting_origin, source, "", nullptr));
On 2015/11/17 13:36:53, Bernhard Bauer wrote:
> Use the empty std::string() constructor to save some instructions.

Done.

[Reilly Grant (use Gerrit), 2015-11-18 02:07:24]

Patchset #4 (id:60001) has been deleted

[Bernhard Bauer, 2015-11-18 14:11:06]

lgtm

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

https://codereview.chromium.org/1447813002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.h:34: struct
ChooserTypeNameEntry;
This can't be in the private section because it's used outside of the class,
right? (*sigh*)

[Reilly Grant (use Gerrit), 2015-11-30 22:44:31]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2015-11-30 22:44:31]

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

[commit-bot: I haz the power, 2015-11-30 22:48:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1447813002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1447813002/100001

[commit-bot: I haz the power, 2015-12-01 00:42:23]

Description was changed from

==========
Add USB devices to the content settings page.

This patch adds infrastructure for displaying chooser-style permissions
such as USB devices selected by the user on the content settings page.
These permissions are used by the WebUSB API. This infrastructure will
be used for per-device WebBluetooth permissions as well.

BUG=424667,529950
==========

to

==========
Add USB devices to the content settings page.

This patch adds infrastructure for displaying chooser-style permissions
such as USB devices selected by the user on the content settings page.
These permissions are used by the WebUSB API. This infrastructure will
be used for per-device WebBluetooth permissions as well.

BUG=424667,529950
==========

[commit-bot: I haz the power, 2015-12-01 00:42:24]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-12-01 00:46:07]

Description was changed from

==========
Add USB devices to the content settings page.

This patch adds infrastructure for displaying chooser-style permissions
such as USB devices selected by the user on the content settings page.
These permissions are used by the WebUSB API. This infrastructure will
be used for per-device WebBluetooth permissions as well.

BUG=424667,529950
==========

to

==========
Add USB devices to the content settings page.

This patch adds infrastructure for displaying chooser-style permissions
such as USB devices selected by the user on the content settings page.
These permissions are used by the WebUSB API. This infrastructure will
be used for per-device WebBluetooth permissions as well.

BUG=424667,529950

Committed: https://crrev.com/92e28e54832687afef1c6e9968fcd4608be05db9
Cr-Commit-Position: refs/heads/master@{#362290}
==========

[commit-bot: I haz the power, 2015-12-01 00:46:07]

Patchset 5 (id:??) landed as
https://crrev.com/92e28e54832687afef1c6e9968fcd4608be05db9
Cr-Commit-Position: refs/heads/master@{#362290}

[Dan Beam, 2015-12-02 04:09:49]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2015-12-02 04:09:50]

https://codereview.chromium.org/1447813002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

https://codereview.chromium.org/1447813002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/content_settings_exceptions_area.js:17:
function IsEditableType(contentType) {
ah, this shouldn't be capitalized either. bummer

https://codereview.chromium.org/1447813002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/content_settings_exceptions_area.js:33:
function IsChosenObjectType(contentType) {
should be isChosenObjectType

https://codereview.chromium.org/1447813002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/content_settings_exceptions_area.js:37:
function ValueColumnForContentType(contentType) {
valueColumnForContentType

https://codereview.chromium.org/1447813002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/content_settings_exceptions_area.js:163:
objectLabel.textContent = this.dataItem.objectName;
^ this broke the closure compile for some reason...

https://codereview.chromium.org/1447813002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/content_settings_exceptions_area.js:582: var
dataItem = listItem.dataItem;
var params = [
  listItem.contentType,
  listItem.mode,
  dataItem.origin,
  dataItem.embeddingOrigin,
];

if (isChosenObjectType(this.contentType))
  params.push(dataItem.object);

chrome.send('removeException', params);