Add an option page for searching and managing resources and permissions.

Add an option page for searching and managing resources and permissions by origin.

Add a command line flag for activating this option page.

Design doc and mocks: https://docs.google.com/a/chromium.org/document/d/1oQwmj3AU4QYhTyGrYEGr6zaZhHUfx-wqUgEcQGbUU-U/edit

BUG=372607
TEST=Enable chrome://flags/#enable-website-settings and go to chrome://settings and click the the "Manage sites..." button under the "Web content" category (click Show advanced settings...). Verify that the list of sites are those granted for a given permission and that the list is searchable.

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

[Daniel Nishi, 2014-07-09 22:52:06]

dbeam: Please review
c/b/resources/options/*
c/b/ui/webui/options/*

and the corresponding file and flag additions in:

chrome/chrome_browser_ui.gypi
chrome/chrome_tests.gypi
chrome/common/chrome_switches.cc and h
c/b/about_flags.cc

bauerb: Please review chrome/browser/content_settings/*

Thanks a bunch!

[Dan Beam, 2014-07-10 02:57:25]

overall, looking pretty good

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/options_bundle.js:80: <include
src="origin_resources_list.js"></include>
why here?  this list seems to be alphabetized...

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:8: /** @const */
ArrayDataModel = cr.ui.ArrayDataModel;
ArrayDataModel, InlineEditeableItem{,List} are unused

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:13: var el =
cr.doc.createElement('div');
why can't you just do

  var listItem = new OriginListItem();
  listItem.decorate();
  return listItem;

?

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:17: };
}; -> }

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:22: }
indent off

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:22: }
};

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:26: 
does this need /** @override */?

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:35: var container =
this.ownerDocument.createElement('div');
inline |container| or at least move lower

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:38: titleEl.className
= 'title';
titleEl.className = 'title favicon-cell weakrtl';

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.css:7: margin-left: -15px;
any {margin,padding}-left -> -webkit-{margin,padding}-start
any {margin,padding}-right -> -webkit-{margin,padding}-end

so it'll work in RTL (right-to-left languages like hebrew or arabic).

that said, why do you need a negative margin here?

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.css:12: margin-right: 17em;
this is not the right way to put the search box to the right side of the dialog,
i'd recommend using flexbox or a table or lots of other ways

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.css:15: /* Styles for the
origin list elements in the website settings page.. */
nit: .. -> .

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.css:17: border: 1px solid
#D9D9D9;
nit: d9d9d9 (lowercase)

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.css:19: margin-top: 5px;
nit: margin: 5px 0 0;

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.html:7: <select
id="resourceType" class="resource-type-select">
why does this need a huge margin?

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.html:10: <option
value="notifications" i18n-content="websitesLabelNotifications"></option>
80 col wrap

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.html:21: i18n-content="done">
fits in 1 line

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:42:
this.pageDiv.querySelector('.resource-type-select').onchange =
      this.pageDiv.querySelector('.resource-type-select').onchange =
          function(event) {
        ...
      };

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:43: function(event) {
nit: assert(event.target.tagName == 'SELECT');

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:55: e.stopPropagation();
do you want to stop the propagation or prevent the default?

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:80: entries = [];
^ always start with var, this is creating a global |entries|

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:84:
this.originList_.dataModel = new ArrayDataModel(entries);
|entries| => Object.keys(originDict)

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:86: 
/** Doc comment */

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:87: searchOrigins:
function() {
make @private

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:90:
chrome.send('updateOriginsSearchResults', [filter]);
indent off (needs 1 more \s)

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:92: 
/**
 * Doc comment.
 * @private
 */

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:98: 160);
ident off (needs 1 more \s)

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:13: const char*
kPreferencesSource = "preference";
const char kPreferenceSource[] = "preference";

for 1 less static initializer

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:56: const
content::NotificationDetails& details) {
what's the purpose of this?

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:79: void
WebsiteSettingsHandler::UpdateOrigins_(ContentSettingsType content_type,
remove |content_type|

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:82:
HostContentSettingsMap* settings = profile->GetHostContentSettingsMap();
nit: \n

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:100: }
nit: no { curlies }

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.h:7: 
#include <string>

#include "base/macros.h"
#include "base/basictypes.h"

(macros.h for DISALLOW_COPY_AND_ASSIGN() and basictypes.h for OVERRIDE)

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.h:32: // Update the
page with all origins for a given Content Setting.
list what |args| contains in each of these handlers

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.h:33: void
UpdateOrigins(const base::ListValue* args);
nit: if it's chrome.send('messageName') name the C++ handler method
HandleMessageName()

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.h:37: 
doc comment

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.h:38: void
UpdateOrigins_(ContentSettingsType content_type,
no _ at the end for private methods

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.h:41:
ContentSettingsType last_setting_;
DISALLOW_COPY_AND_ASSIGN(WebsiteSettingsHandler);

https://codereview.chromium.org/380893005/diff/20001/chrome/common/chrome_swi...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/380893005/diff/20001/chrome/common/chrome_swi...
chrome/common/chrome_switches.cc:663: const char kEnableWebsiteSettingsManager[]
= "enable-website-settings";
nit: one more \s before =

https://codereview.chromium.org/380893005/diff/20001/chrome/common/chrome_swi...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/380893005/diff/20001/chrome/common/chrome_swi...
chrome/common/chrome_switches.h:193: extern const char
kEnableWebsiteSettingsManager[];
nit: alphabetize harder

[Bernhard Bauer, 2014-07-10 08:47:43]

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_utils.cc (right):

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_utils.cc:69: if (name ==
std::string(GetTypeName(ContentSettingsType(type)))) {
If you want to micro-optimize this a bit, use `name.compare(kTypeNames[type]) ==
0`, which will avoid constructing a new string for each type.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_utils.cc:73: return
CONTENT_SETTINGS_TYPE_DEFAULT;
It might be a bit better to return a success value from this function and put
the type into an outparam, so we can distinguish between unknown types and the
default type.

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:35: var container =
this.ownerDocument.createElement('div');
On 2014/07/10 02:57:23, Dan Beam wrote:
> inline |container| or at least move lower

Yeah, it doesn't really seem to be used at all.

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:39:
this.pageDiv.querySelector('.website-settings-overlay-confirm').onclick =
Would it make sense to make this an ID instead of a class?

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:84:
this.originList_.dataModel = new ArrayDataModel(entries);
On 2014/07/10 02:57:24, Dan Beam wrote:
> |entries| => Object.keys(originDict)

Alternatively, just pass an array to this method. If you want to make it a dict
mapping to the last time used, you can always do that later.

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:13: const char*
kPreferencesSource = "preference";
On 2014/07/10 02:57:24, Dan Beam wrote:
> const char kPreferenceSource[] = "preference";
> 
> for 1 less static initializer

Does that actually create a static initializer? I though it's still Plain Old
Data, so it wouldn't create one (and the difference between those two is just
that the constant is put into a different section of the binary)?

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:85:
ContentSettingsForOneType::const_iterator it = all_settings.begin();
If this is not inside of the for-loop header, can you at least move it as close
to there as possible?

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:96: const
std::string origin = it->primary_pattern.ToString();
We don't usually use const for non-reference, non-pointer objects.

[Daniel Nishi, 2014-07-10 19:00:10]

PTAL.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_utils.cc (right):

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_utils.cc:69: if (name ==
std::string(GetTypeName(ContentSettingsType(type)))) {
On 2014/07/10 08:47:42, Bernhard Bauer wrote:
> If you want to micro-optimize this a bit, use `name.compare(kTypeNames[type])
==
> 0`, which will avoid constructing a new string for each type.

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_utils.cc:73: return
CONTENT_SETTINGS_TYPE_DEFAULT;
On 2014/07/10 08:47:42, Bernhard Bauer wrote:
> It might be a bit better to return a success value from this function and put
> the type into an outparam, so we can distinguish between unknown types and the
> default type.

Good point. I was thinking about maybe placing a NOTREACHED() there on the way
to work this morning, but this solution sounds better.

Done.

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/options_bundle.js:80: <include
src="origin_resources_list.js"></include>
On 2014/07/10 02:57:23, Dan Beam wrote:
> why here?  this list seems to be alphabetized...

Done.

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:8: /** @const */
ArrayDataModel = cr.ui.ArrayDataModel;
On 2014/07/10 02:57:23, Dan Beam wrote:
> ArrayDataModel, InlineEditeableItem{,List} are unused

Whoops. Removed the holdovers from a previous implementation.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:13: var el =
cr.doc.createElement('div');
On 2014/07/10 02:57:23, Dan Beam wrote:
> why can't you just do
> 
>   var listItem = new OriginListItem();
>   listItem.decorate();
>   return listItem;
> 
> ?

That would cause an infinite loop calling the constructor.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:17: };
On 2014/07/10 02:57:23, Dan Beam wrote:
> }; -> }

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:22: }
On 2014/07/10 02:57:23, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:22: }
On 2014/07/10 02:57:23, Dan Beam wrote:
> };

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:26: 
On 2014/07/10 02:57:23, Dan Beam wrote:
> does this need /** @override */?

I think so.

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:35: var container =
this.ownerDocument.createElement('div');
On 2014/07/10 02:57:23, Dan Beam wrote:
> inline |container| or at least move lower

Removed the container.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:35: var container =
this.ownerDocument.createElement('div');
On 2014/07/10 08:47:42, Bernhard Bauer wrote:
> On 2014/07/10 02:57:23, Dan Beam wrote:
> > inline |container| or at least move lower
> 
> Yeah, it doesn't really seem to be used at all.

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:38: titleEl.className
= 'title';
On 2014/07/10 02:57:23, Dan Beam wrote:
> titleEl.className = 'title favicon-cell weakrtl';

Done.

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.css:7: margin-left: -15px;
On 2014/07/10 02:57:23, Dan Beam wrote:
> any {margin,padding}-left -> -webkit-{margin,padding}-start
> any {margin,padding}-right -> -webkit-{margin,padding}-end
> 
> so it'll work in RTL (right-to-left languages like hebrew or arabic).
> 
> that said, why do you need a negative margin here?

Haven't done much work with CSS before, so I cargo culted that over.

Removed now.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.css:12: margin-right: 17em;
On 2014/07/10 02:57:23, Dan Beam wrote:
> this is not the right way to put the search box to the right side of the
dialog,
> i'd recommend using flexbox or a table or lots of other ways

Replaced with flex and a margin-right:auto.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.css:15: /* Styles for the
origin list elements in the website settings page.. */
On 2014/07/10 02:57:23, Dan Beam wrote:
> nit: .. -> .

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.css:17: border: 1px solid
#D9D9D9;
On 2014/07/10 02:57:23, Dan Beam wrote:
> nit: d9d9d9 (lowercase)

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.css:19: margin-top: 5px;
On 2014/07/10 02:57:23, Dan Beam wrote:
> nit: margin: 5px 0 0;

Done.

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.html:7: <select
id="resourceType" class="resource-type-select">
On 2014/07/10 02:57:24, Dan Beam wrote:
> why does this need a huge margin?

Bad method of alignment. Changed as per your comment.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.html:10: <option
value="notifications" i18n-content="websitesLabelNotifications"></option>
On 2014/07/10 02:57:23, Dan Beam wrote:
> 80 col wrap

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.html:21: i18n-content="done">
On 2014/07/10 02:57:24, Dan Beam wrote:
> fits in 1 line

Done.

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:39:
this.pageDiv.querySelector('.website-settings-overlay-confirm').onclick =
On 2014/07/10 08:47:42, Bernhard Bauer wrote:
> Would it make sense to make this an ID instead of a class?

I think so.

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:42:
this.pageDiv.querySelector('.resource-type-select').onchange =
On 2014/07/10 02:57:24, Dan Beam wrote:
>       this.pageDiv.querySelector('.resource-type-select').onchange =
>           function(event) {
>         ...
>       };

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:43: function(event) {
On 2014/07/10 02:57:24, Dan Beam wrote:
> nit: assert(event.target.tagName == 'SELECT');

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:55: e.stopPropagation();
On 2014/07/10 02:57:24, Dan Beam wrote:
> do you want to stop the propagation or prevent the default?

Prevent the default is better.

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:80: entries = [];
On 2014/07/10 02:57:24, Dan Beam wrote:
> ^ always start with var, this is creating a global |entries|

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:84:
this.originList_.dataModel = new ArrayDataModel(entries);
On 2014/07/10 02:57:24, Dan Beam wrote:
> |entries| => Object.keys(originDict)

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:86: 
On 2014/07/10 02:57:24, Dan Beam wrote:
> /** Doc comment */

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:87: searchOrigins:
function() {
On 2014/07/10 02:57:24, Dan Beam wrote:
> make @private

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:90:
chrome.send('updateOriginsSearchResults', [filter]);
On 2014/07/10 02:57:24, Dan Beam wrote:
> indent off (needs 1 more \s)

Oops. I think I had the outside indented wrong.

Should line up now.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:92: 
On 2014/07/10 02:57:24, Dan Beam wrote:
> /**
>  * Doc comment.
>  * @private
>  */

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:98: 160);
On 2014/07/10 02:57:24, Dan Beam wrote:
> ident off (needs 1 more \s)

Fixed function indentation.

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:13: const char*
kPreferencesSource = "preference";
On 2014/07/10 08:47:42, Bernhard Bauer wrote:
> On 2014/07/10 02:57:24, Dan Beam wrote:
> > const char kPreferenceSource[] = "preference";
> > 
> > for 1 less static initializer
> 
> Does that actually create a static initializer? I though it's still Plain Old
> Data, so it wouldn't create one (and the difference between those two is just
> that the constant is put into a different section of the binary)?

I don't think it makes a static initializer, but there are advantages to making
it an array. I think.

Changed to array and tucked into a namespace because I forgot to do that.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:56: const
content::NotificationDetails& details) {
On 2014/07/10 02:57:24, Dan Beam wrote:
> what's the purpose of this?

Whoops. I guess I forgot to remove that.

Removed the function.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:79: void
WebsiteSettingsHandler::UpdateOrigins_(ContentSettingsType content_type,
Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:82:
HostContentSettingsMap* settings = profile->GetHostContentSettingsMap();
On 2014/07/10 02:57:24, Dan Beam wrote:
> nit: \n

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:85:
ContentSettingsForOneType::const_iterator it = all_settings.begin();
On 2014/07/10 08:47:42, Bernhard Bauer wrote:
> If this is not inside of the for-loop header, can you at least move it as
close
> to there as possible?

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:96: const
std::string origin = it->primary_pattern.ToString();
On 2014/07/10 08:47:42, Bernhard Bauer wrote:
> We don't usually use const for non-reference, non-pointer objects.

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:100: }
On 2014/07/10 02:57:24, Dan Beam wrote:
> nit: no { curlies }

Done.

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.h:7: 
On 2014/07/10 02:57:25, Dan Beam wrote:
> #include <string>
> 
> #include "base/macros.h"
> #include "base/basictypes.h"
> 
> (macros.h for DISALLOW_COPY_AND_ASSIGN() and basictypes.h for OVERRIDE)

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.h:32: // Update the
page with all origins for a given Content Setting.
On 2014/07/10 02:57:24, Dan Beam wrote:
> list what |args| contains in each of these handlers

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.h:33: void
UpdateOrigins(const base::ListValue* args);
On 2014/07/10 02:57:24, Dan Beam wrote:
> nit: if it's chrome.send('messageName') name the C++ handler method
> HandleMessageName()

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.h:37: 
On 2014/07/10 02:57:25, Dan Beam wrote:
> doc comment

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.h:38: void
UpdateOrigins_(ContentSettingsType content_type,
On 2014/07/10 02:57:25, Dan Beam wrote:
> no _ at the end for private methods

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.h:41:
ContentSettingsType last_setting_;
On 2014/07/10 02:57:25, Dan Beam wrote:
> DISALLOW_COPY_AND_ASSIGN(WebsiteSettingsHandler);

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/common/chrome_swi...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/380893005/diff/20001/chrome/common/chrome_swi...
chrome/common/chrome_switches.cc:663: const char kEnableWebsiteSettingsManager[]
= "enable-website-settings";
On 2014/07/10 02:57:25, Dan Beam wrote:
> nit: one more \s before =

Done.

https://codereview.chromium.org/380893005/diff/20001/chrome/common/chrome_swi...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/380893005/diff/20001/chrome/common/chrome_swi...
chrome/common/chrome_switches.h:193: extern const char
kEnableWebsiteSettingsManager[];
On 2014/07/10 02:57:25, Dan Beam wrote:
> nit: alphabetize harder

Alphabetization now harder, better, faster, stronger.

[Daniel Nishi, 2014-07-14 22:26:51]

Ping.

[Dan Beam, 2014-07-15 03:39:16]

super close

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:13: const char*
kPreferencesSource = "preference";
On 2014/07/10 08:47:42, Bernhard Bauer wrote:
> On 2014/07/10 02:57:24, Dan Beam wrote:
> > const char kPreferenceSource[] = "preference";
> > 
> > for 1 less static initializer
> 
> Does that actually create a static initializer? I though it's still Plain Old
> Data, so it wouldn't create one (and the difference between those two is just
> that the constant is put into a different section of the binary)?

iirc, i've had to revert and change to this to make the bots happy.  it may
depend on other things (external linkage or something voodoo-y).

https://codereview.chromium.org/380893005/diff/20001/chrome/common/chrome_swi...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/380893005/diff/20001/chrome/common/chrome_swi...
chrome/common/chrome_switches.h:193: extern const char
kEnableWebsiteSettingsManager[];
On 2014/07/10 19:00:10, Daniel Nishi wrote:
> On 2014/07/10 02:57:25, Dan Beam wrote:
> > nit: alphabetize harder
> 
> Alphabetization now harder, better, faster, stronger.

:)

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/origin_resources_list.js (right):

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:18: el.decorate();
indent off (2 \s less)

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings.css (right):

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.css:5: /* Styles for the
website settings page. */
^ don't think this comment is all that necessary

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.css:15: margin-right: auto;
if this depends on text-direction (i.e. would change if the text goes right to
left):
  -webkit-margin-end: auto;

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings.html (right):

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.html:11: ></option>
nit: either

  <option value="media-stream"
      i18n-content="websitesLabelMediaStream"></option>

or

  <option value="media-stream" i18n-content="websitesLabelMediaStream">
  </option>

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings.js (right):

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:6: 
nit: remove \n

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:46:
[event.target.options[event.target.selectedIndex].value]);
nit: remove 4 \s indent inside this function

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:91:
this.pageDiv.querySelector('.website-settings-search-box').value;
indent off (4 \s for continuations)

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:117: 
remove \n

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

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:101: //
TODO(dhnishi): Make this map be a map of origin to last usage of API.
i'm not sure what this comment means...

https://codereview.chromium.org/380893005/diff/40001/chrome/common/chrome_swi...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/380893005/diff/40001/chrome/common/chrome_swi...
chrome/common/chrome_switches.cc:666: const char kEnableWebsiteSettingsManager[]
 = "enable-website-settings";
enable-website-settings-manager

[Bernhard Bauer, 2014-07-15 08:32:49]

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:39:
this.pageDiv.querySelector('.website-settings-overlay-confirm').onclick =
On 2014/07/10 19:00:09, Daniel Nishi wrote:
> On 2014/07/10 08:47:42, Bernhard Bauer wrote:
> > Would it make sense to make this an ID instead of a class?
> 
> I think so.
> 
> Done.

We can probably do that for the other elements here as well.

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:13: const char*
kPreferencesSource = "preference";
On 2014/07/10 19:00:09, Daniel Nishi wrote:
> On 2014/07/10 08:47:42, Bernhard Bauer wrote:
> > On 2014/07/10 02:57:24, Dan Beam wrote:
> > > const char kPreferenceSource[] = "preference";
> > > 
> > > for 1 less static initializer
> > 
> > Does that actually create a static initializer? I though it's still Plain
Old
> > Data, so it wouldn't create one (and the difference between those two is
just
> > that the constant is put into a different section of the binary)?
> 
> I don't think it makes a static initializer, but there are advantages to
making
> it an array. I think.
> 
> Changed to array and tucked into a namespace because I forgot to do that.

Note that the anonymous namespace is not necessary for constants, as those
automatically get internal linkage.

Really, what we should do is expose the constants from
host_content_settings_map.cc. Can you add a TODO(bauerb) for that?

[hosseinbehzady62, 2014-07-15 17:05:21]

lgtm

[hosseinbehzady62, 2014-07-15 17:06:50]

On 2014/07/15 17:05:21, hosseinbehzady62 wrote:
> lgtm

[Daniel Nishi, 2014-07-15 17:12:55]

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

https://codereview.chromium.org/380893005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:13: const char*
kPreferencesSource = "preference";
On 2014/07/15 08:32:49, Bernhard Bauer wrote:
> On 2014/07/10 19:00:09, Daniel Nishi wrote:
> > On 2014/07/10 08:47:42, Bernhard Bauer wrote:
> > > On 2014/07/10 02:57:24, Dan Beam wrote:
> > > > const char kPreferenceSource[] = "preference";
> > > > 
> > > > for 1 less static initializer
> > > 
> > > Does that actually create a static initializer? I though it's still Plain
> Old
> > > Data, so it wouldn't create one (and the difference between those two is
> just
> > > that the constant is put into a different section of the binary)?
> > 
> > I don't think it makes a static initializer, but there are advantages to
> making
> > it an array. I think.
> > 
> > Changed to array and tucked into a namespace because I forgot to do that.
> 
> Note that the anonymous namespace is not necessary for constants, as those
> automatically get internal linkage.
> 
> Really, what we should do is expose the constants from
> host_content_settings_map.cc. Can you add a TODO(bauerb) for that?

I added a "TODO(bauerb): Expose constants." to host_content_settings_map.cc.

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/origin_resources_list.js (right):

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:18: el.decorate();
On 2014/07/15 03:39:15, Dan Beam wrote:
> indent off (2 \s less)

Done.

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings.css (right):

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.css:5: /* Styles for the
website settings page. */
On 2014/07/15 03:39:15, Dan Beam wrote:
> ^ don't think this comment is all that necessary

Done.

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.css:15: margin-right: auto;
On 2014/07/15 03:39:15, Dan Beam wrote:
> if this depends on text-direction (i.e. would change if the text goes right to
> left):
>   -webkit-margin-end: auto;

Done.

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings.html (right):

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.html:11: ></option>
On 2014/07/15 03:39:15, Dan Beam wrote:
> nit: either
> 
>   <option value="media-stream"
>       i18n-content="websitesLabelMediaStream"></option>
> 
> or
> 
>   <option value="media-stream" i18n-content="websitesLabelMediaStream">
>   </option>

Done with the second.

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings.js (right):

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:6: 
On 2014/07/15 03:39:15, Dan Beam wrote:
> nit: remove \n

Done.

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:46:
[event.target.options[event.target.selectedIndex].value]);
On 2014/07/15 03:39:15, Dan Beam wrote:
> nit: remove 4 \s indent inside this function

I think this solves the indent.

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:91:
this.pageDiv.querySelector('.website-settings-search-box').value;
On 2014/07/15 03:39:15, Dan Beam wrote:
> indent off (4 \s for continuations)

Done.

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:117: 
On 2014/07/15 03:39:15, Dan Beam wrote:
> remove \n

Done.

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

https://codereview.chromium.org/380893005/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:101: //
TODO(dhnishi): Make this map be a map of origin to last usage of API.
On 2014/07/15 03:39:15, Dan Beam wrote:
> i'm not sure what this comment means...

Clarified comment.

https://codereview.chromium.org/380893005/diff/40001/chrome/common/chrome_swi...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/380893005/diff/40001/chrome/common/chrome_swi...
chrome/common/chrome_switches.cc:666: const char kEnableWebsiteSettingsManager[]
 = "enable-website-settings";
On 2014/07/15 03:39:16, Dan Beam wrote:
> enable-website-settings-manager

Done.

[Dan Beam, 2014-07-15 21:46:19]

lgtm

https://codereview.chromium.org/380893005/diff/80001/chrome/browser/resources...
File chrome/browser/resources/options/origin_resources_list.js (right):

https://codereview.chromium.org/380893005/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:12:
OriginListItem.decorate(el);
if OriginListItem.decorate() is only called once, just inline this method here,
e.g.

  var el = cr.doc.createElement('div');
  el.origin_ = origin;
  el.__proto__ = OriginListItem.prototype;
  el.decorate();

https://codereview.chromium.org/380893005/diff/80001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings.js (right):

https://codereview.chromium.org/380893005/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:41:
$('resourceType').onchange =
$('resourceType').onchange = function() {
  var target = event.target;
  ...
};

https://codereview.chromium.org/380893005/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:46:
[target.options[target.selectedIndex].value]);
isn't this the same as target.value?

https://codereview.chromium.org/380893005/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:91:
chrome.send('updateOriginsSearchResults', [filter]);
indent off

[Bernhard Bauer, 2014-07-15 21:52:17]

lgtm

[Daniel Nishi, 2014-07-15 22:18:24]

https://codereview.chromium.org/380893005/diff/80001/chrome/browser/resources...
File chrome/browser/resources/options/origin_resources_list.js (right):

https://codereview.chromium.org/380893005/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:12:
OriginListItem.decorate(el);
On 2014/07/15 21:46:19, Dan Beam wrote:
> if OriginListItem.decorate() is only called once, just inline this method
here,
> e.g.
> 
>   var el = cr.doc.createElement('div');
>   el.origin_ = origin;
>   el.__proto__ = OriginListItem.prototype;
>   el.decorate();

Done.

https://codereview.chromium.org/380893005/diff/80001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings.js (right):

https://codereview.chromium.org/380893005/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:41:
$('resourceType').onchange =
On 2014/07/15 21:46:19, Dan Beam wrote:
> $('resourceType').onchange = function() {
>   var target = event.target;
>   ...
> };

Done.

https://codereview.chromium.org/380893005/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:46:
[target.options[target.selectedIndex].value]);
On 2014/07/15 21:46:19, Dan Beam wrote:
> isn't this the same as target.value?

Appears to be so.

Switched to the simpler value.

https://codereview.chromium.org/380893005/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:91:
chrome.send('updateOriginsSearchResults', [filter]);
On 2014/07/15 21:46:19, Dan Beam wrote:
> indent off

Done.

[Daniel Nishi, 2014-07-15 22:46:59]

The CQ bit was checked by dhnishi@chromium.org

[commit-bot: I haz the power, 2014-07-15 22:50:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dhnishi@chromium.org/380893005/120001

[commit-bot: I haz the power, 2014-07-16 07:47:24]

Change committed as 283388

[Vitaly Pavlenko, 2014-09-07 05:14:32]

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

[Vitaly Pavlenko, 2014-09-07 05:14:33]

https://codereview.chromium.org/380893005/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/website_settings.js (right):

https://codereview.chromium.org/380893005/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/website_settings.js:42: var target =
event.target;
I don't understand why |event| is defined at this point. Could you explain,
please?

[Daniel Nishi, 2014-09-08 15:27:49]

On 2014/09/07 05:14:33, Vitaly Pavlenko wrote:
>
https://codereview.chromium.org/380893005/diff/120001/chrome/browser/resource...
> File chrome/browser/resources/options/website_settings.js (right):
> 
>
https://codereview.chromium.org/380893005/diff/120001/chrome/browser/resource...
> chrome/browser/resources/options/website_settings.js:42: var target =
> event.target;
> I don't understand why |event| is defined at this point. Could you explain,
> please?

In JS, I think you can still access function parameters even if they were not
explicitly declared. |event| is still getting passed to onchange() and is
accessible within the function, but it isn't in the function header.

Forgetting the parameter was a mistake on my part, I'll have a patch up to add
the parameter in explicitly!

[Daniel Nishi, 2014-09-08 15:35:16]

On 2014/09/08 15:27:49, Daniel Nishi wrote:
> On 2014/09/07 05:14:33, Vitaly Pavlenko wrote:
> >
>
https://codereview.chromium.org/380893005/diff/120001/chrome/browser/resource...
> > File chrome/browser/resources/options/website_settings.js (right):
> > 
> >
>
https://codereview.chromium.org/380893005/diff/120001/chrome/browser/resource...
> > chrome/browser/resources/options/website_settings.js:42: var target =
> > event.target;
> > I don't understand why |event| is defined at this point. Could you explain,
> > please?
> 
> In JS, I think you can still access function parameters even if they were not
> explicitly declared. |event| is still getting passed to onchange() and is
> accessible within the function, but it isn't in the function header.
> 
> Forgetting the parameter was a mistake on my part, I'll have a patch up to add
> the parameter in explicitly!

Whoops. After doing a bit more research, I think my previous statement was
incorrect. I think it's actually accessing the scope where the call originated
and that the event is also called event there.

Regardless, I'll have a patch up to address it.

[Dan Beam, 2014-09-08 22:57:52]

On 2014/09/08 15:35:16, Daniel Nishi wrote:
> On 2014/09/08 15:27:49, Daniel Nishi wrote:
> > On 2014/09/07 05:14:33, Vitaly Pavlenko wrote:
> > >
> >
>
https://codereview.chromium.org/380893005/diff/120001/chrome/browser/resource...
> > > File chrome/browser/resources/options/website_settings.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/380893005/diff/120001/chrome/browser/resource...
> > > chrome/browser/resources/options/website_settings.js:42: var target =
> > > event.target;
> > > I don't understand why |event| is defined at this point. Could you
explain,
> > > please?
> > 
> > In JS, I think you can still access function parameters even if they were
not
> > explicitly declared. |event| is still getting passed to onchange() and is
> > accessible within the function, but it isn't in the function header.
> > 
> > Forgetting the parameter was a mistake on my part, I'll have a patch up to
add
> > the parameter in explicitly!
> 
> Whoops. After doing a bit more research, I think my previous statement was
> incorrect. I think it's actually accessing the scope where the call originated
> and that the event is also called event there.
> 
> Regardless, I'll have a patch up to address it.

if I had to guess: window.event is provided while inside event handlers for
backward-compatibility for IE-specific code.

http://msdn.microsoft.com/en-us/library/ie/ms535863(v=vs.85).aspx