[MD settings] use promises rather than a counter var to fetch cookie data

[MD settings] use promises rather than a counter var to fetch cookie data

This CL replaces a var counter for fetching cookie data with javascript
promises. The counter had issues with reentrancy, while promise arrays
are more robust.

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

Committed: https://crrev.com/3c14e795c62b093cd9b76b1ac5d1812278902175
Cr-Commit-Position: refs/heads/master@{#427130}

[dschuyler, 2016-10-21 21:34:23]

Description was changed from

==========
[MD settings] use promises rather than a counter var to fetch cookie data

This CL replaces a var counter for fetching cookie data with javascript
promises. The counter had issues with reentrancy, while promise arrays
are more robust.

BUG=None
==========

to

==========
[MD settings] use promises rather than a counter var to fetch cookie data

This CL replaces a var counter for fetching cookie data with javascript
promises. The counter had issues with reentrancy, while promise arrays
are more robust.

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

[dschuyler, 2016-10-21 21:34:25]

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

[commit-bot: I haz the power, 2016-10-21 21:35:24]

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

[dschuyler, 2016-10-21 21:35:41]

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

[dpapad, 2016-10-21 22:03:08]

N-1 nits, and 1 actual question.

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

https://codereview.chromium.org/2439233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data.js:26: * @private
Nit: @private {!settings.CookieTreeNode}
for consistency within this file.

https://codereview.chromium.org/2439233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data.js:94: if
(filterString.length == 0)
Nit(optional): I know you are not touching this part. This can be compressed as
follows

return loadTimeData.getString(filterString.length == 0 ?
    'siteSettingsCookieRemoveAll' :
    'siteSettingsCookieRemoveAllShown';

https://codereview.chromium.org/2439233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data.js:110: var
promiseList = [];
Nit: s/promiseList/promises/. It's shorter, and conveys the same info.

https://codereview.chromium.org/2439233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data.js:111: for (var i =
0; i < data.length; ++i) {
Nit(optional): A more functional way to do this (and more compact) is

var promises = list.children.
    filter(function(c) { return c.hasChildren; }).
    map(function(c) {
      return this.browserProxy.loadCookieChildren(data[i].id).then(
          this.loadChildrenRecurse_.bind(this));
    }.bind(this));

Just mentioning it as a potential style. No need to change it unless you like
the more functional aspect of it.

https://codereview.chromium.org/2439233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data.js:114:
data[i].id).then(function(list) {
Nit (optional): I think is more readable if you skip the anonymous function.

 promiseList.push(this.browserProxy.loadCookieChildren(data[i].id).then(
    this.loadChildrenRecurse_.bind(this));

https://codereview.chromium.org/2439233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data.js:129:
loadChildrenRecurse_: function(list) {
loadChildrenRecurse_ and loadChildren_ seem almost identical. Is there a way we
can reduce the repeated logic? Something like (pseudocode, I've ommitted calls
to bind() for simplicity).

loadChildren_: function(rootList) {
 ...// initial stuff here.
 function loadChildrenRecurse_(list) {...};

 // Start recursion from rootList.
 return loadChildrenRecurse_(rootList).then(function() {
   this.sites = this.treeNodes_.getSummaryList();
 });
};

Basically currently, it seems as if we are starting the recursion from the
children of the root list, instead of the root list itself, unnecessarily.

https://codereview.chromium.org/2439233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data.js:130: assert(list.id
!= null);
Do we need to assert, on top of relying on the compiler type checking, which
indicates that list is never null? (same for assertion at 105).

[dschuyler, 2016-10-22 00:48:05]

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

[commit-bot: I haz the power, 2016-10-22 00:48:25]

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

[dschuyler, 2016-10-22 00:50:45]

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

https://codereview.chromium.org/2439233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data.js:26: * @private
On 2016/10/21 22:03:08, dpapad wrote:
> Nit: @private {!settings.CookieTreeNode}
> for consistency within this file.

Discussed this offline, leaving this out.

https://codereview.chromium.org/2439233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data.js:94: if
(filterString.length == 0)
On 2016/10/21 22:03:07, dpapad wrote:
> Nit(optional): I know you are not touching this part. This can be compressed
as
> follows
> 
> return loadTimeData.getString(filterString.length == 0 ?
>     'siteSettingsCookieRemoveAll' :
>     'siteSettingsCookieRemoveAllShown';

I'd rather put these in the html with $i18n
and not call loadTimeData; let's leave that
for another CL.

https://codereview.chromium.org/2439233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data.js:110: var
promiseList = [];
On 2016/10/21 22:03:08, dpapad wrote:
> Nit: s/promiseList/promises/. It's shorter, and conveys the same info.

Done.

https://codereview.chromium.org/2439233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data.js:111: for (var i =
0; i < data.length; ++i) {
On 2016/10/21 22:03:07, dpapad wrote:
> Nit(optional): A more functional way to do this (and more compact) is
> 
> var promises = list.children.
>     filter(function(c) { return c.hasChildren; }).
>     map(function(c) {
>       return this.browserProxy.loadCookieChildren(data[i].id).then(
>           this.loadChildrenRecurse_.bind(this));
>     }.bind(this));
> 
> Just mentioning it as a potential style. No need to change it unless you like
> the more functional aspect of it.

javascript map is often suggested as alternative to a
for loop, but each time I've tested it in the past
the for loop is faster at run-time (or the same if the
jit converts the map to a for loop). I find the for 
loop easier to read as well, but that's just preference.
If map were faster I'd switch to using it more often.

https://codereview.chromium.org/2439233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data.js:114:
data[i].id).then(function(list) {
On 2016/10/21 22:03:07, dpapad wrote:
> Nit (optional): I think is more readable if you skip the anonymous function.
> 
>  promiseList.push(this.browserProxy.loadCookieChildren(data[i].id).then(
>     this.loadChildrenRecurse_.bind(this));
Thanks

Done.

https://codereview.chromium.org/2439233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data.js:129:
loadChildrenRecurse_: function(list) {
On 2016/10/21 22:03:07, dpapad wrote:
> loadChildrenRecurse_ and loadChildren_ seem almost identical. Is there a way
we
> can reduce the repeated logic? Something like (pseudocode, I've ommitted calls
> to bind() for simplicity).
> 
> loadChildren_: function(rootList) {
>  ...// initial stuff here.
>  function loadChildrenRecurse_(list) {...};
> 
>  // Start recursion from rootList.
>  return loadChildrenRecurse_(rootList).then(function() {
>    this.sites = this.treeNodes_.getSummaryList();
>  });
> };
> 
> Basically currently, it seems as if we are starting the recursion from the
> children of the root list, instead of the root list itself, unnecessarily.

Done.

https://codereview.chromium.org/2439233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data.js:130: assert(list.id
!= null);
On 2016/10/21 22:03:07, dpapad wrote:
> Do we need to assert, on top of relying on the compiler type checking, which
> indicates that list is never null? (same for assertion at 105).

The assserts are testing whether id is null (rather
than list), I'd put them in
to be sure I wasn't messing up when converting the 
code, now that I think it's correct they can be removed.

[dpapad, 2016-10-22 01:05:57]

LGTM

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

https://codereview.chromium.org/2439233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data.js:111: for (var i =
0; i < data.length; ++i) {
> javascript map is often suggested as alternative to a
> for loop, but each time I've tested it in the past
> the for loop is faster at run-time (or the same if the
> jit converts the map to a for loop). I find the for 
> loop easier to read as well, but that's just preference.
> If map were faster I'd switch to using it more often.

forEach() and for() are usually compared in terms of performance, and you are
right, in all benchmarks I've seen there is a difference.

Trivia: map() is a bit different though, because strictly speaking you gain some
performance by pre-allocating the array instead of growing it little by little
with push() (which ends up copying the array and doubling its size as many times
as needed).

Basically for the for vs map comparison to be entirely equivalent one has to do
the following
 
var array = new Array(count); // Pre-allocate space.
for (var i = 0; i < count; i++) {
  array[i] = ...;
}

Perhaps the new for..of is the best of both worlds between for and forEach,
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/....
Easier syntax, no new function calls, no need to bind, so I am guessing same
performance with for().

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

https://codereview.chromium.org/2439233002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.js:122: }.bind(this);
Perhaps add a \n here?

[dschuyler, 2016-10-22 01:15:14]

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

[commit-bot: I haz the power, 2016-10-22 01:15:45]

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

[dschuyler, 2016-10-22 01:17:11]

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

https://codereview.chromium.org/2439233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data.js:111: for (var i =
0; i < data.length; ++i) {
On 2016/10/22 01:05:57, dpapad wrote:
> > javascript map is often suggested as alternative to a
> > for loop, but each time I've tested it in the past
> > the for loop is faster at run-time (or the same if the
> > jit converts the map to a for loop). I find the for 
> > loop easier to read as well, but that's just preference.
> > If map were faster I'd switch to using it more often.
> 
> forEach() and for() are usually compared in terms of performance, and you are
> right, in all benchmarks I've seen there is a difference.
> 
> Trivia: map() is a bit different though, because strictly speaking you gain
some
> performance by pre-allocating the array instead of growing it little by little
> with push() (which ends up copying the array and doubling its size as many
times
> as needed).
> 
> Basically for the for vs map comparison to be entirely equivalent one has to
do
> the following
>  
> var array = new Array(count); // Pre-allocate space.
> for (var i = 0; i < count; i++) {
>   array[i] = ...;
> }
> 
> Perhaps the new for..of is the best of both worlds between for and forEach,
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/....
> Easier syntax, no new function calls, no need to bind, so I am guessing same
> performance with for().

The poor performance of map, afaik, comes from the function call overhead.

I switched to for...of too.

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

https://codereview.chromium.org/2439233002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.js:122: }.bind(this);
On 2016/10/22 01:05:57, dpapad wrote:
> Perhaps add a \n here?

Done.

[dschuyler, 2016-10-22 01:26:51]

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

[commit-bot: I haz the power, 2016-10-22 01:27:08]

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

[dschuyler, 2016-10-22 01:27:35]

with for(let child...

[commit-bot: I haz the power, 2016-10-22 05:08:56]

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

[commit-bot: I haz the power, 2016-10-22 05:08:57]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-10-24 17:10:56]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2016-10-24 17:10:56]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2439233002/#ps60001
(title: "fix a whoops")

[commit-bot: I haz the power, 2016-10-24 17:11:30]

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-10-24 20:10:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-24 20:14:50]

Description was changed from

==========
[MD settings] use promises rather than a counter var to fetch cookie data

This CL replaces a var counter for fetching cookie data with javascript
promises. The counter had issues with reentrancy, while promise arrays
are more robust.

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

to

==========
[MD settings] use promises rather than a counter var to fetch cookie data

This CL replaces a var counter for fetching cookie data with javascript
promises. The counter had issues with reentrancy, while promise arrays
are more robust.

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

Committed: https://crrev.com/3c14e795c62b093cd9b76b1ac5d1812278902175
Cr-Commit-Position: refs/heads/master@{#427130}
==========

[commit-bot: I haz the power, 2016-10-24 20:14:51]

Patchset 4 (id:??) landed as
https://crrev.com/3c14e795c62b093cd9b76b1ac5d1812278902175
Cr-Commit-Position: refs/heads/master@{#427130}