[MD settings] get cookie details from site without transfering cookie tree

[MD settings] get cookie details from site without transfering cookie tree

This CL accelerates the site cookie details page. To fully address the
bug, the main list of cookies (non-details) needs improvement as well.
This CL is a separable step in toward that goal.

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

Committed: https://crrev.com/68cd6a7fb8998e99a7832617363f3a7f43200fd0
Cr-Commit-Position: refs/heads/master@{#430021}

[dschuyler, 2016-11-03 21:35:48]

Description was changed from

==========
[MD settings] get cookie details from site without transfering cookie tree

This CL accelerates the site cookie details page. To fully address the
bug, the main list of cookies (non-details) needs improvement as well.
This CL is a separable step in toward that goal.

BUG=659865
==========

to

==========
[MD settings] get cookie details from site without transfering cookie tree

This CL accelerates the site cookie details page. To fully address the
bug, the main list of cookies (non-details) needs improvement as well.
This CL is a separable step in toward that goal.

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

[dschuyler, 2016-11-03 21:35:51]

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

[commit-bot: I haz the power, 2016-11-03 21:37:01]

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

[dschuyler, 2016-11-03 21:43:56]

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

[commit-bot: I haz the power, 2016-11-03 21:44:22]

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

[dschuyler, 2016-11-03 21:50:10]

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

[commit-bot: I haz the power, 2016-11-03 21:50:16]

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

[dschuyler, 2016-11-03 21:51:13]

Patchset #1 (id:1) has been deleted

[dschuyler, 2016-11-03 21:51:20]

Patchset #1 (id:20001) has been deleted

[dschuyler, 2016-11-03 21:51:31]

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

[commit-bot: I haz the power, 2016-11-03 23:26:59]

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

[commit-bot: I haz the power, 2016-11-03 23:26:59]

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

[dschuyler, 2016-11-03 23:48:28]

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

[commit-bot: I haz the power, 2016-11-03 23:49:40]

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

[Dan Beam, 2016-11-04 00:35:32]

https://codereview.chromium.org/2476753002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/cookies_tree_model_util.h (right):

https://codereview.chromium.org/2476753002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/cookies_tree_model_util.h:54: // is not valid.
^

https://codereview.chromium.org/2476753002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_cookies_view_handler.cc (right):

https://codereview.chromium.org/2476753002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_cookies_view_handler.cc:196: if
(!args->GetString(1, &site))
should you still be settings callback_id_ if getting site fails?

https://codereview.chromium.org/2476753002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_cookies_view_handler.cc:199:
AllowJavascript();
shouldn't we call this even if !GetString(1)

https://codereview.chromium.org/2476753002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_cookies_view_handler.cc:206:
callback_id_ = "";
callback_id_.clear();

https://codereview.chromium.org/2476753002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_cookies_view_handler.cc:290:
callback_id_ = "";
callback_id_.clear();

[dschuyler, 2016-11-04 00:49:36]

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

[commit-bot: I haz the power, 2016-11-04 00:50:16]

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

[dschuyler, 2016-11-04 00:50:45]

I changed the if() tests to CHECK for the params.

https://codereview.chromium.org/2476753002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/cookies_tree_model_util.h (right):

https://codereview.chromium.org/2476753002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/cookies_tree_model_util.h:54: // is not valid.
On 2016/11/04 00:35:31, Dan Beam wrote:
> ^

Done.

https://codereview.chromium.org/2476753002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_cookies_view_handler.cc (right):

https://codereview.chromium.org/2476753002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_cookies_view_handler.cc:196: if
(!args->GetString(1, &site))
On 2016/11/04 00:35:32, Dan Beam wrote:
> should you still be settings callback_id_ if getting site fails?

Done.

https://codereview.chromium.org/2476753002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_cookies_view_handler.cc:199:
AllowJavascript();
On 2016/11/04 00:35:31, Dan Beam wrote:
> shouldn't we call this even if !GetString(1)

Done.

https://codereview.chromium.org/2476753002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_cookies_view_handler.cc:206:
callback_id_ = "";
On 2016/11/04 00:35:32, Dan Beam wrote:
> callback_id_.clear();

Done.

https://codereview.chromium.org/2476753002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_cookies_view_handler.cc:290:
callback_id_ = "";
On 2016/11/04 00:35:32, Dan Beam wrote:
> callback_id_.clear();

Done.

[Dan Beam, 2016-11-04 02:03:35]

i'm fairly unfamiliar with cookies code, so take my review there with a grain of
salt, but lgtm

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

https://codereview.chromium.org/2476753002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js:61:
if (site == undefined || site == this.site_)
nit: can we use === here for things like comparing '' to undefined and such?

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

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

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

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

[dschuyler, 2016-11-04 18:16:52]

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

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

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

[dschuyler, 2016-11-04 18:17:42]

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

https://codereview.chromium.org/2476753002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js:61:
if (site == undefined || site == this.site_)
On 2016/11/04 02:03:35, Dan Beam wrote:
> nit: can we use === here for things like comparing '' to undefined and such?

Done.

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

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

[commit-bot: I haz the power, 2016-11-04 18:32:49]

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

[dschuyler, 2016-11-04 18:45:57]

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

[commit-bot: I haz the power, 2016-11-04 18:46:29]

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

[dschuyler, 2016-11-04 18:48:21]

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

https://codereview.chromium.org/2476753002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js:61:
if (site == undefined || site == this.site_)
On 2016/11/04 18:17:42, dschuyler wrote:
> On 2016/11/04 02:03:35, Dan Beam wrote:
> > nit: can we use === here for things like comparing '' to undefined and such?
> 
> Done.

Ah, closure noticed that the change allowed site to be null. So I changed
it to !site. I believe we don't want null, undefined, or empty site strings.
So !site covers that well.

[Dan Beam, 2016-11-04 18:48:54]

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

https://codereview.chromium.org/2476753002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js:61:
if (site == undefined || site == this.site_)
On 2016/11/04 18:48:21, dschuyler wrote:
> On 2016/11/04 18:17:42, dschuyler wrote:
> > On 2016/11/04 02:03:35, Dan Beam wrote:
> > > nit: can we use === here for things like comparing '' to undefined and
such?
> > 
> > Done.
> 
> Ah, closure noticed that the change allowed site to be null. So I changed
> it to !site. I believe we don't want null, undefined, or empty site strings.
> So !site covers that well.

great, still lgtm

[commit-bot: I haz the power, 2016-11-04 20:22:38]

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

[commit-bot: I haz the power, 2016-11-04 20:22:38]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-11-04 21:30:22]

The CQ bit was checked by dschuyler@chromium.org

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

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-11-04 21:37:54]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-11-04 21:40:14]

Description was changed from

==========
[MD settings] get cookie details from site without transfering cookie tree

This CL accelerates the site cookie details page. To fully address the
bug, the main list of cookies (non-details) needs improvement as well.
This CL is a separable step in toward that goal.

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

to

==========
[MD settings] get cookie details from site without transfering cookie tree

This CL accelerates the site cookie details page. To fully address the
bug, the main list of cookies (non-details) needs improvement as well.
This CL is a separable step in toward that goal.

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

Committed: https://crrev.com/68cd6a7fb8998e99a7832617363f3a7f43200fd0
Cr-Commit-Position: refs/heads/master@{#430021}
==========

[commit-bot: I haz the power, 2016-11-04 21:40:15]

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