[MD settings] cookie and site data from dialog to subpage

[MD settings] cookie and site data from dialog to subpage

This CL moves the cookie data details from a dialog into a settings
subpage.

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

Committed: https://crrev.com/0f58402502d291872de78265747751f4728b9e36
Cr-Commit-Position: refs/heads/master@{#429392}

[dschuyler, 2016-10-27 01:30:06]

Description was changed from

==========
[MD settings] cookie and site data from dialog to subpage

This CL moves the cookie data details from a dialog into a settings
subpage.

BUG=654943
==========

to

==========
[MD settings] cookie and site data from dialog to subpage

This CL moves the cookie data details from a dialog into a settings
subpage.

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

[dschuyler, 2016-10-27 01:30:08]

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

[commit-bot: I haz the power, 2016-10-27 01:30:36]

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-27 01:31:45]

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

[commit-bot: I haz the power, 2016-10-27 01:32:17]

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-10-27 01:35:17]

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

[commit-bot: I haz the power, 2016-10-27 01:35:17]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dschuyler, 2016-10-27 21:00:22]

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

[commit-bot: I haz the power, 2016-10-27 21:01:06]

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-10-27 21:09:43]

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

[commit-bot: I haz the power, 2016-10-27 21:09:44]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  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-10-27 21:16:59]

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

[dschuyler, 2016-10-29 00:26:32]

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

[commit-bot: I haz the power, 2016-10-29 00:26:51]

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-10-29 01:19:03]

sorry i didn't get to this today, will look over the weekend or on monday

[commit-bot: I haz the power, 2016-10-29 01:26:24]

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

[commit-bot: I haz the power, 2016-10-29 01:26:25]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-11-01 06:33:28]

lookin' pretty good!

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:155:
r.SITE_SETTINGS_COOKIES.createChild('/siteSettings/siteDataDetails');
this is kind of an odd URL to me... a saner URL would be like /cookies/<url> but
i guess that can come later.  can be at least /cookies/details or
/cookies/<something> in the mean time?

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/cookie_tree_node.js (left):

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:187: 
why did you make all these changes? ^

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

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:217: *
@return {settings.CookieTreeNode} The node found, if any.
?settings.CookieTreeNode

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:221: if
(this.children_[i] == null)
when does this happen?

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings/site_data_details_subpage.html
(right):

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.html:15:
<div class="settings-box" first$="[[!index]]">
cool hack

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

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js:43:
'#siteDataDetailsRemoveAll').addEventListener(
what is this doing?  adding an event listener to a sibling?  why?

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js:53:
if (settings.getCurrentRoute() != settings.Route.SITE_SETTINGS_DATA_DETAILS)
can we use contains() instead of equality?

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js:56:
if (this.siteTitle_ == undefined)
if (!this.siteTitle_)

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js:63:
* @return {!Array<CookieDataForDisplay>}
nit: !CookieDataForDisplay unless any of the items can be null

[dschuyler, 2016-11-01 18:52:42]

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

[commit-bot: I haz the power, 2016-11-01 18:53:04]

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-01 18:55:35]

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:155:
r.SITE_SETTINGS_COOKIES.createChild('/siteSettings/siteDataDetails');
On 2016/11/01 06:33:27, Dan Beam wrote:
> this is kind of an odd URL to me... a saner URL would be like /cookies/<url>
but
> i guess that can come later.  can be at least /cookies/details or
> /cookies/<something> in the mean time?

Done.

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/cookie_tree_node.js (left):

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:187: 
On 2016/11/01 06:33:27, Dan Beam wrote:
> why did you make all these changes? ^

Personal readability and over application of the c++ style guide. Doesn't need
to be checked in though.
I'd still like to keep this last change as it was the most egregious (with a
blank line between one closing brace and another).

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

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:217: *
@return {settings.CookieTreeNode} The node found, if any.
On 2016/11/01 06:33:27, Dan Beam wrote:
> ?settings.CookieTreeNode

Done.

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:221: if
(this.children_[i] == null)
On 2016/11/01 06:33:27, Dan Beam wrote:
> when does this happen?

This was based on a similar existing function. This also didn't need to be
recursive, so I've reworked it.

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings/site_data_details_subpage.html
(right):

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.html:15:
<div class="settings-box" first$="[[!index]]">
On 2016/11/01 06:33:27, Dan Beam wrote:
> cool hack

Acknowledged.

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

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js:43:
'#siteDataDetailsRemoveAll').addEventListener(
On 2016/11/01 06:33:27, Dan Beam wrote:
> what is this doing?  adding an event listener to a sibling?  why?

The goal was to know when that button is pushed, without adding unrelated code
to the parent, and while getting the button positioned correctly. To position
the button within the title area, a content select is used. In essence, we need
to put the button 'over-there' and get the message that it was clicked
'over-here'. I'm happy to learn a better way to get that done if a better way is
available.

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js:53:
if (settings.getCurrentRoute() != settings.Route.SITE_SETTINGS_DATA_DETAILS)
On 2016/11/01 06:33:27, Dan Beam wrote:
> can we use contains() instead of equality?

I think that's a possibility, but it would also open the possibility of false
positives. The direct comparison is what is used in other cases I've seen with
getCurrentRoute(). The query string is culled from the route iiuc, so that
wouldn't be a problem for the comparison.

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js:56:
if (this.siteTitle_ == undefined)
On 2016/11/01 06:33:27, Dan Beam wrote:
> if (!this.siteTitle_)

Done.

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js:63:
* @return {!Array<CookieDataForDisplay>}
On 2016/11/01 06:33:27, Dan Beam wrote:
> nit: !CookieDataForDisplay unless any of the items can be null

Done.

[Dan Beam, 2016-11-01 19:10:15]

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/cookie_tree_node.js (left):

https://codereview.chromium.org/2450423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:187: 
On 2016/11/01 18:55:34, dschuyler wrote:
> On 2016/11/01 06:33:27, Dan Beam wrote:
> > why did you make all these changes? ^
> 
> Personal readability and over application of the c++ style guide. Doesn't need
> to be checked in though.
> I'd still like to keep this last change as it was the most egregious (with a
> blank line between one closing brace and another).

yeah, last removal is fine

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

https://codereview.chromium.org/2450423002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js:44:
'tap', this.onRemoveAll_.bind(this));
this certainly does a few funky things.  can you even browsePreload to this URL
only and run a test?  i think this code will throw when it doesn't find
#siteDataDetailsRemoveAll

i think it'd make the most sense for the sibling to fire('remove-all') and the
parent to catch it and call .removeAll() (as public method you'd make in this
element)

also, i'm not sure we want to use addEventListener('tap') directly anywhere, but
maybe this.listen('tap', ...) instead (if using on-tap doesn't work)

[dschuyler, 2016-11-01 19:42:17]

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

[commit-bot: I haz the power, 2016-11-01 19:42:52]

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-01 19:46:55]

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

https://codereview.chromium.org/2450423002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js:44:
'tap', this.onRemoveAll_.bind(this));
On 2016/11/01 19:10:15, Dan Beam wrote:
> this certainly does a few funky things.  can you even browsePreload to this
URL
> only and run a test?  i think this code will throw when it doesn't find
> #siteDataDetailsRemoveAll
> 
> i think it'd make the most sense for the sibling to fire('remove-all') and the
> parent to catch it and call .removeAll() (as public method you'd make in this
> element)
Handling the event in the privacy page means that I can do a regular on-tap.
I changed this to what I think you're looking for in patch#6

> 
> also, i'm not sure we want to use addEventListener('tap') directly anywhere,
but
> maybe this.listen('tap', ...) instead (if using on-tap doesn't work)

[commit-bot: I haz the power, 2016-11-01 19:58:46]

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

[commit-bot: I haz the power, 2016-11-01 19:58:47]

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-01 21:04:08]

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

[commit-bot: I haz the power, 2016-11-01 21:04:44]

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-11-01 22:45:40]

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

[commit-bot: I haz the power, 2016-11-01 22:45:41]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-11-02 00:16:10]

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

[commit-bot: I haz the power, 2016-11-02 00:16:48]

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-11-02 01:30:48]

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

[commit-bot: I haz the power, 2016-11-02 01:30:49]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-11-02 06:13:11]

lgtm

https://codereview.chromium.org/2450423002/diff/130001/chrome/browser/resourc...
File
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js
(right):

https://codereview.chromium.org/2450423002/diff/130001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js:40:
this.onCookiesLoaded_.bind(this));
nit: listeners: {'cookie-tree-changed': 'onCookiesLoaded_'},

[dschuyler, 2016-11-02 18:23:59]

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

[commit-bot: I haz the power, 2016-11-02 18:24:38]

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-02 18:24:49]

https://codereview.chromium.org/2450423002/diff/130001/chrome/browser/resourc...
File
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js
(right):

https://codereview.chromium.org/2450423002/diff/130001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data_details_subpage.js:40:
this.onCookiesLoaded_.bind(this));
On 2016/11/02 06:13:11, Dan Beam wrote:
> nit: listeners: {'cookie-tree-changed': 'onCookiesLoaded_'},

Done.

[commit-bot: I haz the power, 2016-11-02 20:02:57]

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

[commit-bot: I haz the power, 2016-11-02 20:02:58]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-11-02 20:35:30]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2016-11-02 20:35:31]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2450423002/#ps150001
(title: "review nit")

[commit-bot: I haz the power, 2016-11-02 20:36:11]

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-02 20:45:12]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2016-11-02 20:48:30]

Description was changed from

==========
[MD settings] cookie and site data from dialog to subpage

This CL moves the cookie data details from a dialog into a settings
subpage.

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

to

==========
[MD settings] cookie and site data from dialog to subpage

This CL moves the cookie data details from a dialog into a settings
subpage.

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

Committed: https://crrev.com/0f58402502d291872de78265747751f4728b9e36
Cr-Commit-Position: refs/heads/master@{#429392}
==========

[commit-bot: I haz the power, 2016-11-02 20:48:31]

Patchset 9 (id:??) landed as
https://crrev.com/0f58402502d291872de78265747751f4728b9e36
Cr-Commit-Position: refs/heads/master@{#429392}