Better support for patterns.

Better support for patterns.

BUG=598864, 543635
Committed: https://crrev.com/2f2f3d62eb79b7d389b8124c41ff368a7652c93b
Cr-Commit-Position: refs/heads/master@{#386420}

[Finnur, 2016-04-08 16:59:41]

Description was changed from

==========
Better support for patterns.

BUG=TBD
==========

to

==========
Better support for patterns.

BUG=598864, 543635
==========

[Finnur, 2016-04-08 16:59:41]

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

[Finnur, 2016-04-08 17:10:27]

Mind taking a look?

[michaelpg, 2016-04-11 03:55:35]

looks good, but tidy up the compare function please

https://codereview.chromium.org/1867363003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1867363003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:283: var
embeddingOriginA = self.ensureUrlHasScheme(a.embeddingOrigin);
you don't use these embedding* vars until 299, so create them down there and
maybe condense them a bit

https://codereview.chromium.org/1867363003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:290: new
URL(embeddingOriginA.replace('[*.]'));
missing 2nd arg (apparently results in String(undefined) being used)

https://codereview.chromium.org/1867363003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_behavior.js
(right):

https://codereview.chromium.org/1867363003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:291: *
@return {string} The URL with a scheme.
... or the empty string

https://codereview.chromium.org/1867363003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:295:
return url.indexOf('://') != -1 ? url : 'http://' + url;
url.includes

https://codereview.chromium.org/1867363003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/1867363003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:545: // If this fails with 2
instead of the expected 1, then the
nit: move comment to assert message :-)

[Finnur, 2016-04-11 11:45:33]

https://codereview.chromium.org/1867363003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1867363003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:283: var
embeddingOriginA = self.ensureUrlHasScheme(a.embeddingOrigin);
On 2016/04/11 03:55:35, michaelpg wrote:
> you don't use these embedding* vars until 299, so create them down there and
> maybe condense them a bit

Done.

https://codereview.chromium.org/1867363003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:290: new
URL(embeddingOriginA.replace('[*.]'));
Good catch. Explains the weirdness I was facing below (fixed now).

https://codereview.chromium.org/1867363003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_behavior.js
(right):

https://codereview.chromium.org/1867363003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:291: *
@return {string} The URL with a scheme.
On 2016/04/11 03:55:35, michaelpg wrote:
> ... or the empty string

Done.

https://codereview.chromium.org/1867363003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:295:
return url.indexOf('://') != -1 ? url : 'http://' + url;
On 2016/04/11 03:55:35, michaelpg wrote:
> url.includes

Done.

https://codereview.chromium.org/1867363003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/1867363003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:545: // If this fails with 2
instead of the expected 1, then the
It doesn't really do anything because all asserts failing in this test result in
an error like this:

Error: Script error. (:0)
", source: mocha_adapter.js (44)

(which has been hugely frustrating in debugging efforts).

[michaelpg, 2016-04-11 14:09:55]

https://codereview.chromium.org/1867363003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/1867363003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:545: // If this fails with 2
instead of the expected 1, then the
On 2016/04/11 11:45:33, Finnur wrote:
> It doesn't really do anything because all asserts failing in this test result
in
> an error like this:
> 
> Error: Script error. (:0)
> ", source: mocha_adapter.js (44)
> 
> (which has been hugely frustrating in debugging efforts).

This is because the error is being thrown asynchronously so mocha doesn't know
about it, it's a JS limitation unfortunately.

Mocha gives us two options: call done(error) or return a promise.

1. use try/catch to call "done" with the error:

  browserProxy.whenCalled('foo').then(function(value) {
    testElement.async(function() {
      var error;
      try {
        assertFalse(true);
      } catch (e) {
        error = e;
      }
      done(error);
    });
  });

2. remove "done" and return a promise instead:
  return browserProxy.whenCalled('foo').then(function(value) {
    var resolver = new PromiseResolver();
    testElement.async(resolver.resolve);
    return resolver.promise.then(function() {
      assertFalse(true);
    });
  });

(PromiseResolver is just a helper function to create a promise that can be
resolved by calling resolve(). The point here is to throw *inside* the promise
and return the promise to mocha so it knows what's up.)

[michaelpg, 2016-04-11 14:14:33]

lgtm though, lmk if you still have issues getting the assert to show up

https://codereview.chromium.org/1867363003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1867363003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:273: * @return
{URL} The URL to return (or null if origin is not a valid URL).
be explicit about nullable types: {!URL}

[michaelpg, 2016-04-11 14:32:18]

https://codereview.chromium.org/1867363003/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/1867363003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:563: });
forgot to mention: indent these 2 lines

[Finnur, 2016-04-11 17:10:54]

> lmk if you still have issues getting the assert to show up

Nope, the resolver thing fixes the issue. Thanks!

I've addressed all comments, including the one before (to convert the comment to
an assert message) and will check in.

https://codereview.chromium.org/1867363003/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/1867363003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:563: });
On 2016/04/11 14:32:18, michaelpg wrote:
> forgot to mention: indent these 2 lines

Done.

[Finnur, 2016-04-11 17:11:00]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2016-04-11 17:11:00]

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

[commit-bot: I haz the power, 2016-04-11 17:11:11]

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

[commit-bot: I haz the power, 2016-04-11 18:28:47]

Description was changed from

==========
Better support for patterns.

BUG=598864, 543635
==========

to

==========
Better support for patterns.

BUG=598864, 543635
==========

[commit-bot: I haz the power, 2016-04-11 18:28:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-11 18:31:21]

Description was changed from

==========
Better support for patterns.

BUG=598864, 543635
==========

to

==========
Better support for patterns.

BUG=598864, 543635

Committed: https://crrev.com/2f2f3d62eb79b7d389b8124c41ff368a7652c93b
Cr-Commit-Position: refs/heads/master@{#386420}
==========

[commit-bot: I haz the power, 2016-04-11 18:31:23]

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

[Dan Beam, 2016-04-11 19:15:31]

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

[Dan Beam, 2016-04-11 19:15:31]

https://codereview.chromium.org/1867363003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1867363003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:278: return null;
should this throw?

https://codereview.chromium.org/1867363003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:291: toSiteArray_:
function(sites) {
this should have a @return {!Array<SomeTypeGoesHere>}