Site Settings Desktop: Implement individual cookie removal and RemoveAll.

Site Settings Desktop: Implement individual cookie removal and RemoveAll.

BUG=635852, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/f40c843852d50fc41aa353588d7799f0f6203626
Cr-Commit-Position: refs/heads/master@{#413151}

[Finnur, 2016-08-17 16:17:04]

Description was changed from

==========
Site Settings Desktop: Implement individual cookie removal and RemoveAll.

BUG=635852, 614277
==========

to

==========
Site Settings Desktop: Implement individual cookie removal and RemoveAll.

BUG=635852, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Finnur, 2016-08-17 16:17:07]

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

[commit-bot: I haz the power, 2016-08-17 16:17:54]

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

[Finnur, 2016-08-17 16:45:36]

Patchset #1 (id:1) has been deleted

[Finnur, 2016-08-17 16:47:40]

Description was changed from

==========
Site Settings Desktop: Implement individual cookie removal and RemoveAll.

BUG=635852, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Site Settings Desktop: Implement individual cookie removal and RemoveAll.

BUG=635852, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Finnur, 2016-08-17 16:47:40]

finnur@chromium.org changed reviewers:
+ dschuyler@chromium.org, mkwst@chromium.org

[Finnur, 2016-08-17 16:55:54]

Mike, can you take cookie_tree_model.cc. 

Dave, can you take the rest?

https://codereview.chromium.org/2248683006/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/cookies_tree_model.cc (right):

https://codereview.chromium.org/2248683006/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/cookies_tree_model.cc:1528: SetBatchExpectation(0,
true);
This is an augmentation of the fix I submitted here:
https://codereview.chromium.org/863503002/

The problem I encountered was when showing the cookie data list. It starts off a
batch of number of batches. Once it ends, batches_started_ and batches_ended_
match and are non-zero. All good up till then.

But once I select RemoveAll cookies, it starts a new batch but because
batches_started > 0, it doesn't fire off TreeModelBeginBatch because of line
1508 above. It does, however, fire TreeModelEndBatch (line 1527), which confuses
the clients (they DCHECK on batches ending that haven't been started).

If I null out the counters when the final batch has ended, this problem doesn't
happen.

[Finnur, 2016-08-17 16:57:18]

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

[commit-bot: I haz the power, 2016-08-17 16:57:34]

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

[Finnur, 2016-08-17 16:58:54]

https://codereview.chromium.org/2248683006/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/cookies_tree_model.cc (right):

https://codereview.chromium.org/2248683006/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/cookies_tree_model.cc:1528: SetBatchExpectation(0,
true);
Second paragraph should have read:

> The problem I encountered was when showing the cookie data list. That starts
off a
> number of batches and once it ends, batches_started_ and batches_ended_
> match and are non-zero. All good up till then.

[commit-bot: I haz the power, 2016-08-17 17:41:26]

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

[commit-bot: I haz the power, 2016-08-17 17:41:27]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-08-18 00:44:03]

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

https://codereview.chromium.org/2248683006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.html:19:
-webkit-margin-end: -29px;
Just curious: what is this fixing (or helping with)?

https://codereview.chromium.org/2248683006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.html:44: <div
class="favicon-image" style$="[[computeSiteIcon(item.site)]]"
The whole row area (other than the delete
button at the end) should be clickable to 
do the onSiteTap_. This may need an extra
div wrapping the icon and the item.* text.
That new div would have the on-tap="onSiteTap_"
and the actionable.

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

https://codereview.chromium.org/2248683006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.js:50:
this.browserProxy.reloadCookies();
Can we change reloadCookies() and removeAllCookies()
to return a promise and remove the this.requests_ 
tracking entirely?

https://codereview.chromium.org/2248683006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.js:120:
this.requests_++;
nit: please use pre-increment unless post-increment
is needed for the specific case. i.e.
++this.requests_;

https://codereview.chromium.org/2248683006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.js:121:
this.browserProxy.removeAllCookies();
Same comment:

Can we change reloadCookies() and removeAllCookies()
to return a promise and remove the this.requests_ 
tracking entirely?

[Mike West, 2016-08-18 08:39:17]

cookie_tree_model LGTM.

[Finnur, 2016-08-18 16:20:21]

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

[Finnur, 2016-08-18 16:20:30]

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

https://codereview.chromium.org/2248683006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.html:19:
-webkit-margin-end: -29px;
Aligning the new trashcan so that it is under the other icons above the section.

https://codereview.chromium.org/2248683006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.html:44: <div
class="favicon-image" style$="[[computeSiteIcon(item.site)]]"
On 2016/08/18 00:44:03, dschuyler wrote:
> The whole row area (other than the delete
> button at the end) should be clickable to 
> do the onSiteTap_. This may need an extra
> div wrapping the icon and the item.* text.
> That new div would have the on-tap="onSiteTap_"
> and the actionable.

Done.

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

https://codereview.chromium.org/2248683006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.js:50:
this.browserProxy.reloadCookies();
I've converted to promises, but it still doesn't alleviate the need to track
requests. It does, however, make the tracking a bit saner.

The reason I need to track the number of outstanding requests is to figure out
when I'm done fetching data so that I'm not calling getSummaryList() repeatedly
while the data is being populated -- it slows down the page loading.

The CookieTreeModel, when reset (as in reload-all-cookies), will give you the
top-level nodes. It is up to the client to decide what to do with it. We want
all the data, so we keep issuing more requests for more details until we have
everything.

I only want to update the UI when that is complete. So, I have to keep track of
the requests to know when. It feels more natural to do this at the JS layer
because the C++ layer doesn't really care if more requests are coming in and it
doesn't feel right to have it try to decipher what the client will do (or tell
the client when it is done).

Does this make sense?

https://codereview.chromium.org/2248683006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.js:120:
this.requests_++;
On 2016/08/18 00:44:03, dschuyler wrote:
> nit: please use pre-increment unless post-increment
> is needed for the specific case. i.e.
> ++this.requests_;

Acknowledged.

[commit-bot: I haz the power, 2016-08-18 16:20:45]

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, 2016-08-18 17:21:43]

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

[commit-bot: I haz the power, 2016-08-18 17:21:44]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-08-18 22:51:10]

LGTM if the |getSummeryList| thing is not an issue.

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

https://codereview.chromium.org/2248683006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.js:56: * determine
whether the button should be visible.
nit: indent 'determine...'

https://codereview.chromium.org/2248683006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.js:67: loadChildren_:
function(list) {
@param for list.

https://codereview.chromium.org/2248683006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.js:93: this.sites =
this.treeNodes_.getSummaryList();
It looks like this may be called more often
than once. i.e. it may trigger once per level
in the tree (*I think*). If the goal is just
to reduce the calls to getSummaryList, this 
may be fine. If getSummeryList should not
be called more than once (in a strict sense)
then there /may/ be an issue here.

https://codereview.chromium.org/2248683006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.js:104:
onTreeItemRemoved_: function(args) {
Can we @param the args?

[Finnur, 2016-08-18 23:27:38]

No code change, just wanted to get a round of discussions in before going to
bed. :)

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

https://codereview.chromium.org/2248683006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.js:93: this.sites =
this.treeNodes_.getSummaryList();
I don't think this is an issue, but will check tomorrow when I get in to work
again.

https://codereview.chromium.org/2248683006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.js:104:
onTreeItemRemoved_: function(args) {
Umm, yeah -- |we| probably can. :) I, however, already made an attempt at it
yesterday and gave up. I'm just not sure how to represent the data types
involved to Closure. Your input here would be valuable.

Essentially, I wanted to represent that |args| always looks like this:

Array<String, Number, Number>

In other words, it is a list value with always the same three data types.

But Closure doesn't like that kind of definition. I also tried something like
Array<String|Number>, but then Closure complains about the call to
removeByParentId below (because it wants String but gets String|Number.

What's the right way to do this?

[Finnur, 2016-08-19 12:12:31]

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

[commit-bot: I haz the power, 2016-08-19 12:13:03]

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

[Finnur, 2016-08-19 12:18:32]

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

https://codereview.chromium.org/2248683006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.js:56: * determine
whether the button should be visible.
On 2016/08/18 22:51:10, dschuyler wrote:
> nit: indent 'determine...'

Done.

https://codereview.chromium.org/2248683006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.js:93: this.sites =
this.treeNodes_.getSummaryList();
I verified this again and getSummaryList is called only once (at the end, when
all levels have finished updating).

https://codereview.chromium.org/2248683006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data.js:104:
onTreeItemRemoved_: function(args) {
Actually, scratch that...

Coming in this morning with a fresh mind I realize that sending the data the way
the handler is doing it is a bit dumb. So I changed it to a dictionary and added
the @params.

[commit-bot: I haz the power, 2016-08-19 13:33:51]

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

[commit-bot: I haz the power, 2016-08-19 13:33:52]

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_...)

[Finnur, 2016-08-19 14:30:45]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2016-08-19 14:30:45]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2248683006/#ps60001
(title: "Address feedback")

[commit-bot: I haz the power, 2016-08-19 14:31:06]

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-08-19 15:28:11]

Description was changed from

==========
Site Settings Desktop: Implement individual cookie removal and RemoveAll.

BUG=635852, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Site Settings Desktop: Implement individual cookie removal and RemoveAll.

BUG=635852, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-08-19 15:28:12]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-08-19 15:29:40]

Description was changed from

==========
Site Settings Desktop: Implement individual cookie removal and RemoveAll.

BUG=635852, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Site Settings Desktop: Implement individual cookie removal and RemoveAll.

BUG=635852, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/f40c843852d50fc41aa353588d7799f0f6203626
Cr-Commit-Position: refs/heads/master@{#413151}
==========

[commit-bot: I haz the power, 2016-08-19 15:29:42]

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