[MD settings] separate all-sites from site-list

[MD settings] separate all-sites from site-list

This CL moves the all-sites portion of site settings out of the site
list code. At one point the lists were more similar, I believe. It
became convoluted with special cases for all-sites and this CL simplifies
that.

BUG=712446
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2846723002
Cr-Commit-Position: refs/heads/master@{#473283}
Committed: https://chromium.googlesource.com/chromium/src/+/fa112ee5b220b30022bc27d68f3eeed618e9f154

[dschuyler, 2017-04-27 00:46:47]

Description was changed from

==========
[MD settings] separate all-sites from site-list

This CL moves the all-sites portion of site settings out of the site
list code. At one point the lists were more similar, I believe. It
became convoluted with special cases for all-sites and this CL simplifies
that.

BUG=712446
==========

to

==========
[MD settings] separate all-sites from site-list

This CL moves the all-sites portion of site settings out of the site
list code. At one point the lists were more similar, I believe. It
became convoluted with special cases for all-sites and this CL simplifies
that.

BUG=712446
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2017-04-27 00:46:49]

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

[commit-bot: I haz the power, 2017-04-27 00:47:51]

Dry run: CQ is trying da patch.

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

[dschuyler, 2017-04-27 00:49:09]

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

[commit-bot: I haz the power, 2017-04-27 01:59:53]

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

[commit-bot: I haz the power, 2017-04-27 01:59:55]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dpapad, 2017-04-28 23:54:06]

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/all_sites.js (right):

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:28: ready:
function() {
@override missing.

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:38: * @return
{!Promise}
What is the type of |promiseList|? Can we make the annotation here more
descriptive (like Promise<SomeTypeGoesHere>).

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:43: for (var type
in settings.ContentSettingsTypes) {
Do we need to iterate on the keys of ContentSettingsTypes when we only care
about the values? How about the following shorter version:


var types = Object.getValues(settings.ContentSettingsTypes);
for (var i = 0; i < values.length; i++) {
  var type = types[i];
  if (type == settings.ContentSettingsTypes.PROTOCOL_HANDLERS ||
      type == settings.ContentSettingsTypes.USB_DEVICES ||
      type == settings.ContentSettingsTypes.ZOOM_LEVELS) {
    // Some categories store their data in a custom way.
    continue;
  }

  promiseList.push(this.browserProxy_.getExceptionList(type));
}

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:74:
this.getAllSitesList_().then(function(lists) {
this.getAllSitesList_().then(this.processExceptions_.bind(this));

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:101: * @param
{!Array<RawSiteException>} sites A list of sites to convert.
Should this be !Array<!RawSiteException>? Similar question elsewhere in this
file.

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:116: if (comparison
== 0) {
Do those three if (comparison == 0) checks need to be nested? How about the
following?

var comparison = url1.host.localeCompare(url2.host) ||
    url1.protocol.localeCompare(url2.protocol) ||
    url1.port.localeCompare(url2.port);

if (comparison == 0) {
  ...
  return host1.localeCompare(host2);
}

return comparison;

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:136: if
(siteException.origin == lastOrigin &&
|origin| and |embeddingOrigin| already exist on |sites[i]|. Can we delay calling
|expandSiteException| until after line we throw out the duplicates? This would
save unnecessary work.

[dschuyler, 2017-05-18 23:14:59]

Patchset #2 (id:20001) has been deleted

[dschuyler, 2017-05-18 23:16:11]

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/all_sites.js (right):

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:28: ready:
function() {
On 2017/04/28 23:54:05, dpapad wrote:
> @override missing.

Done.

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:38: * @return
{!Promise}
On 2017/04/28 23:54:05, dpapad wrote:
> What is the type of |promiseList|? Can we make the annotation here more
> descriptive (like Promise<SomeTypeGoesHere>).

Done.

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:43: for (var type
in settings.ContentSettingsTypes) {
On 2017/04/28 23:54:05, dpapad wrote:
> Do we need to iterate on the keys of ContentSettingsTypes when we only care
> about the values? How about the following shorter version:
> 
> 
> var types = Object.getValues(settings.ContentSettingsTypes);
> for (var i = 0; i < values.length; i++) {
>   var type = types[i];
>   if (type == settings.ContentSettingsTypes.PROTOCOL_HANDLERS ||
>       type == settings.ContentSettingsTypes.USB_DEVICES ||
>       type == settings.ContentSettingsTypes.ZOOM_LEVELS) {
>     // Some categories store their data in a custom way.
>     continue;
>   }
> 
>   promiseList.push(this.browserProxy_.getExceptionList(type));
> }

done.

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:74:
this.getAllSitesList_().then(function(lists) {
On 2017/04/28 23:54:05, dpapad wrote:
> this.getAllSitesList_().then(this.processExceptions_.bind(this));

Done.

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:101: * @param
{!Array<RawSiteException>} sites A list of sites to convert.
On 2017/04/28 23:54:05, dpapad wrote:
> Should this be !Array<!RawSiteException>? Similar question elsewhere in this
> file.

Done.

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:116: if (comparison
== 0) {
On 2017/04/28 23:54:05, dpapad wrote:
> Do those three if (comparison == 0) checks need to be nested? How about the
> following?
> 
> var comparison = url1.host.localeCompare(url2.host) ||
>     url1.protocol.localeCompare(url2.protocol) ||
>     url1.port.localeCompare(url2.port);
> 
> if (comparison == 0) {
>   ...
>   return host1.localeCompare(host2);
> }
> 
> return comparison;

I agree that would work. Is it more readable, or faster, or just different?
Using the logical-or short-circuit is fewer lines, which is often good; will it
be clear to other's what is going on; i.e. is the trick obvious to others (I'm
not sure, I'm really asking)?

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:136: if
(siteException.origin == lastOrigin &&
On 2017/04/28 23:54:05, dpapad wrote:
> |origin| and |embeddingOrigin| already exist on |sites[i]|. Can we delay
calling
> |expandSiteException| until after line we throw out the duplicates? This would
> save unnecessary work.

Sure, but I don't want to change this code too much, the intent was to just move
it.

[dpapad, 2017-05-18 23:37:08]

LGTM

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/all_sites.js (right):

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:116: if (comparison
== 0) {
Treat this suggestion as optional. FWIW, there is already a TODO at line 97 to
move this logic elsewhere.

> I agree that would work. Is it more readable, or faster, or just different?

I suggested this because IMO eliminating unnecessary nesting is more readable
and more understandable. Can't speak for everyone else (readability is
subjective). Feel free to add a 2nd reviewer to get a bigger sample of opinions.

> Using the logical-or short-circuit is fewer lines, which is often good; will
it be clear to other's what is going on; i.e. is the trick obvious to others
(I'm not sure, I'm really asking)?

It is a pretty standard technique in JS, for example it is used to give default
values to optional params.

var boolean = opt_boolean || false;
var number = opt_number || 0;
var string = opt_string || '';

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:136: if
(siteException.origin == lastOrigin &&
On 2017/05/18 at 23:16:11, dschuyler wrote:
> On 2017/04/28 23:54:05, dpapad wrote:
> > |origin| and |embeddingOrigin| already exist on |sites[i]|. Can we delay
calling
> > |expandSiteException| until after line we throw out the duplicates? This
would
> > save unnecessary work.
> 
> Sure, but I don't want to change this code too much, the intent was to just
move it.

Can you add a TODO so that we don't completely forget about this?

[dschuyler, 2017-05-18 23:44:22]

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/all_sites.js (right):

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:116: if (comparison
== 0) {
On 2017/05/18 23:37:08, dpapad wrote:
> Treat this suggestion as optional. FWIW, there is already a TODO at line 97 to
> move this logic elsewhere.
> 
> > I agree that would work. Is it more readable, or faster, or just different?
> 
> I suggested this because IMO eliminating unnecessary nesting is more readable
> and more understandable. Can't speak for everyone else (readability is
> subjective). Feel free to add a 2nd reviewer to get a bigger sample of
opinions.
> 
> > Using the logical-or short-circuit is fewer lines, which is often good; will
> it be clear to other's what is going on; i.e. is the trick obvious to others
> (I'm not sure, I'm really asking)?
> 
> It is a pretty standard technique in JS, for example it is used to give
default
> values to optional params.
> 
> var boolean = opt_boolean || false;
> var number = opt_number || 0;
> var string = opt_string || '';

I'll think on it more and/or ask for another opinion and follow-up in a separate
CL if desired. I'm not sure that I have a strong feelings either way, but I do
favor leaving it alone if there is equal benefit with either choice.

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:136: if
(siteException.origin == lastOrigin &&
On 2017/05/18 23:37:08, dpapad wrote:
> On 2017/05/18 at 23:16:11, dschuyler wrote:
> > On 2017/04/28 23:54:05, dpapad wrote:
> > > |origin| and |embeddingOrigin| already exist on |sites[i]|. Can we delay
> calling
> > > |expandSiteException| until after line we throw out the duplicates? This
> would
> > > save unnecessary work.
> > 
> > Sure, but I don't want to change this code too much, the intent was to just
> move it.
> 
> Can you add a TODO so that we don't completely forget about this?

Sorry, my response above wasn't clear - I see that now - I did make the change.
I should have included "done."

[dschuyler, 2017-05-18 23:49:29]

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

[commit-bot: I haz the power, 2017-05-18 23:50:55]

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, 2017-05-19 00:05:48]

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

[commit-bot: I haz the power, 2017-05-19 00:05:49]

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

[dpapad, 2017-05-19 19:25:31]

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/all_sites.js (right):

https://codereview.chromium.org/2846723002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/all_sites.js:136: if
(siteException.origin == lastOrigin &&
> Sorry, my response above wasn't clear - I see that now - I did make the
change. I should have included "done."

Ah ok. Yes it was not clear that it was addressed. Still LGTM.

[dschuyler, 2017-05-19 19:31:34]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2017-05-19 19:33:09]

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, 2017-05-19 19:40:19]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495222294364350,
"parent_rev": "a497cfc6c3e91af6ddc3014a29c6b18c0d426ae6", "commit_rev":
"fa112ee5b220b30022bc27d68f3eeed618e9f154"}

[commit-bot: I haz the power, 2017-05-19 19:40:37]

Description was changed from

==========
[MD settings] separate all-sites from site-list

This CL moves the all-sites portion of site settings out of the site
list code. At one point the lists were more similar, I believe. It
became convoluted with special cases for all-sites and this CL simplifies
that.

BUG=712446
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] separate all-sites from site-list

This CL moves the all-sites portion of site settings out of the site
list code. At one point the lists were more similar, I believe. It
became convoluted with special cases for all-sites and this CL simplifies
that.

BUG=712446
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2846723002
Cr-Commit-Position: refs/heads/master@{#473283}
Committed:
https://chromium.googlesource.com/chromium/src/+/fa112ee5b220b30022bc27d68f3e...
==========

[commit-bot: I haz the power, 2017-05-19 19:40:40]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/fa112ee5b220b30022bc27d68f3e...