Add a page for showing the details for a given origin

Add a page for showing the details for a given origin.

BUG=543635
Committed: https://crrev.com/00000252d04d4344c8921a7d3006a814926b3cfc
Cr-Commit-Position: refs/heads/master@{#359088}

[Finnur, 2015-11-06 11:22:00]

Patchset #1 (id:1) has been deleted

[Finnur, 2015-11-06 11:24:08]

Patchset #1 (id:20001) has been deleted

[Finnur, 2015-11-06 11:38:40]

Patchset #1 (id:40001) has been deleted

[Finnur, 2015-11-06 11:43:57]

Description was changed from

==========
Add some details for the origin.

BUG=543635
==========

to

==========
Add a page for showing the details for a given origin.

BUG=543635
==========

[Finnur, 2015-11-06 11:43:57]

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

[Finnur, 2015-11-06 11:45:36]

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

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.js:69:
if (originPref.setting == settings.DefaultValues.ALLOW) {
One question: Do you know how to take care of this Closure error?

##
/media/largedrive/code/chromium/src/chrome/browser/resources/settings/site_settings/site_details_permission.js:69:
ERROR - Property setting never defined on originPref
##     if (originPref.setting == settings.DefaultValues.ALLOW) {
##                    ^

[Finnur, 2015-11-09 16:13:37]

Friendly ping...

[michaelpg, 2015-11-09 18:26:30]

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details.css (right):

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.css:14:
margin-left: 15px;
-webkit-margin-start

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

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.html:11: <div
class="origin">{{origin}}</div>
nit: we tend to use [[]] instead of {{}} when the binding can only be one-way.
There's no effective difference in cases like this. I just think it adds clarity
and shows your intention better.

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.html:14: <div
class="flex">{{storedData}}</div>
import iron-flex-layout (or use a custom class and write your own CSS)

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.html:16:
on-tap="onClearStorage_" alt="delete"></paper-icon-button>
i18n-values="alt:delete" (and add the string)

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

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:58: if
(this.storedData !== undefined) {
i'd prefer doing this in an observer, unless there's a good reason a host can't
change storedData after creating this element

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:59:
this.$.usage.hidden = false;
if they're logically grouped together, wrap them both in a single <div>?

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:65:
this.$.cookies.origin = this.origin;
what if origin is set before ready? why not set these origin properties in the
template?

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:80:
this.$.cookies.resetPermission();
consider:

Array.prototype.forEach.call(
    this.querySelectorAll('site-details-permission'),
    function(el) { el.resetPermission(); }):

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details_permission.css
(right):

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.css:26:
margin: -20px 3px 12px -13px;
rtl?

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

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.html:17:
<div id="details" class="horizontal layout top" hidden>
import iron-flex-layout

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.html:24:
>[[computeTitleForContentCategory(category)]]</div>
This hack should no longer be necessary:

https://github.com/Polymer/polymer/pull/2489/commits

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

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.js:36:
category: {
category: Number

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.js:69:
if (originPref.setting == settings.DefaultValues.ALLOW) {
On 2015/11/06 11:45:36, Finnur wrote:
> One question: Do you know how to take care of this Closure error?
> 
> ##
>
/media/largedrive/code/chromium/src/chrome/browser/resources/settings/site_settings/site_details_permission.js:69:
> ERROR - Property setting never defined on originPref
> ##     if (originPref.setting == settings.DefaultValues.ALLOW) {
> ##                    ^

Ultimately you'll need to annotate the type of originPref for Closure so it
knows it has a "setting" property of type {number}. Or just use
originPref['setting'] if you'd rather not typedef it.

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.js:69:
if (originPref.setting == settings.DefaultValues.ALLOW) {
hmm, kinda wish we had named this something more specific,
"settings.DefaultValues" is a weird name for something specific to content
settings

[Finnur, 2015-11-10 00:36:46]

Haven't updated the code, just wanted to get two quick questions in so I have a
chance of completing a full round of review comments after I get in in the
morning.

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details_permission.css
(right):

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.css:26:
margin: -20px 3px 12px -13px;
Do you mean 'convert to -webkit-margin-start etc'?

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

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.html:24:
>[[computeTitleForContentCategory(category)]]</div>
I'm sorry, I don't follow... which hack?

The |title| isn't compound and this is used elsewhere, so a utility function
makes sense.

[michaelpg, 2015-11-10 00:40:45]

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details_permission.css
(right):

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.css:26:
margin: -20px 3px 12px -13px;
On 2015/11/10 00:36:45, Finnur wrote:
> Do you mean 'convert to -webkit-margin-start etc'?

Yep.

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

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.html:24:
>[[computeTitleForContentCategory(category)]]</div>
On 2015/11/10 00:36:46, Finnur wrote:
> I'm sorry, I don't follow... which hack?
> 
> The |title| isn't compound and this is used elsewhere, so a utility function
> makes sense.

No, I meant that putting ">" after the break so that the [[annotation]] spans
the entire tag is no longer necessary, because string interpolation is now
supported in Polymer.

<div>
  [[yayWhitespace(hooray)]]
</div>

[Finnur, 2015-11-10 16:00:52]

Thanks for the feedback, all addressed/answered.

PTAL.

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details.css (right):

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.css:14:
margin-left: 15px;
On 2015/11/09 18:26:29, michaelpg wrote:
> -webkit-margin-start

Done.

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

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.html:11: <div
class="origin">{{origin}}</div>
On 2015/11/09 18:26:29, michaelpg wrote:
> nit: we tend to use [[]] instead of {{}} when the binding can only be one-way.
> There's no effective difference in cases like this. I just think it adds
clarity
> and shows your intention better.

Done.

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.html:14: <div
class="flex">{{storedData}}</div>
On 2015/11/09 18:26:30, michaelpg wrote:
> import iron-flex-layout (or use a custom class and write your own CSS)

Done.

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.html:16:
on-tap="onClearStorage_" alt="delete"></paper-icon-button>
On 2015/11/09 18:26:30, michaelpg wrote:
> i18n-values="alt:delete" (and add the string)

Done.

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

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:58: if
(this.storedData !== undefined) {
Sure! Done.

I suspect a chrome.send call is required to fetch the actual storage data and so
the assignment will happen slightly differently in the future.

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:59:
this.$.usage.hidden = false;
Usage is a header that, at the moment, only contains Storage, but should contain
things like Battery Usage. The idea here is that the header is hidden until one
or more subsection, like Storage, decides that it has data and shows itself and
requests the header to be shown as well.

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:65:
this.$.cookies.origin = this.origin;
There was something that was not working before when I did this, but I just
checked and it works fine now, so I converted.

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:80:
this.$.cookies.resetPermission();
Neat trick, but I can't get it to work. No matter what I pass in, be it class
names or element ids, it always returns an empty array. :/

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details_permission.css
(right):

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.css:26:
margin: -20px 3px 12px -13px;
On 2015/11/10 00:40:45, michaelpg wrote:
> On 2015/11/10 00:36:45, Finnur wrote:
> > Do you mean 'convert to -webkit-margin-start etc'?
> 
> Yep.

Done.

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

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.html:17:
<div id="details" class="horizontal layout top" hidden>
On 2015/11/09 18:26:30, michaelpg wrote:
> import iron-flex-layout

Done.

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.html:24:
>[[computeTitleForContentCategory(category)]]</div>
Woot! Done.

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

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.js:36:
category: {
On 2015/11/09 18:26:30, michaelpg wrote:
> category: Number

Done.

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.js:69:
if (originPref.setting == settings.DefaultValues.ALLOW) {
> Ultimately you'll need to annotate the type of originPref for Closure so it
> knows it has a "setting" property of type {number}. Or just use
> originPref['setting'] if you'd rather not typedef it.

For some reason I was stuck in the mindset of having to define a struct with
types somewhere, when instead I can just add the Closure directive right there.

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.js:69:
if (originPref.setting == settings.DefaultValues.ALLOW) {
On 2015/11/09 18:26:30, michaelpg wrote:
> hmm, kinda wish we had named this something more specific,
> "settings.DefaultValues" is a weird name for something specific to content
> settings

I agree. Renamed.

[michaelpg, 2015-11-10 18:24:13]

Great, LGTM modulo a few bytes.

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

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:80:
this.$.cookies.resetPermission();
On 2015/11/10 16:00:51, Finnur wrote:
> Neat trick, but I can't get it to work. No matter what I pass in, be it class
> names or element ids, it always returns an empty array. :/

Sorry, should be done on "this.root" (the local DOM), not "this":

  this.root.querySelectorAll

But it's up to you -- what you have is more straightforward (just potentially
prone to omissions).

https://codereview.chromium.org/1428313002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details.html (right):

https://codereview.chromium.org/1428313002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.html:18:
i18n-valued="alt:siteSettingsDelete"></paper-icon-button>
typo: i18n-values

https://codereview.chromium.org/1428313002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings/site_details_permission.html
(right):

https://codereview.chromium.org/1428313002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.html:6:
<link rel="import"
href="chrome://resources/polymer/v1_0/iron-flex-layout/iron-flex-layout.html">
nit: alphabetize

[Finnur, 2015-11-11 11:37:37]

All addressed. Thanks!

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

https://codereview.chromium.org/1428313002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:80:
this.$.cookies.resetPermission();
Sold!

https://codereview.chromium.org/1428313002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details.html (right):

https://codereview.chromium.org/1428313002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.html:18:
i18n-valued="alt:siteSettingsDelete"></paper-icon-button>
But, how will I then feel valued, you know... internationalizationally speaking?

Oh well, fixed. Thanks for spotting.

https://codereview.chromium.org/1428313002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings/site_details_permission.html
(right):

https://codereview.chromium.org/1428313002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.html:6:
<link rel="import"
href="chrome://resources/polymer/v1_0/iron-flex-layout/iron-flex-layout.html">
On 2015/11/10 18:24:13, michaelpg wrote:
> nit: alphabetize

Done.

[Finnur, 2015-11-11 11:37:44]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2015-11-11 11:37:45]

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/1428313002/#ps100001
(title: "Address feedback")

[commit-bot: I haz the power, 2015-11-11 11:37:56]

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

[commit-bot: I haz the power, 2015-11-11 11:52:04]

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

[commit-bot: I haz the power, 2015-11-11 11:52:05]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Finnur, 2015-11-11 12:34:15]

Hmm... that doesn't look related to my change. Retrying.

[Finnur, 2015-11-11 12:35:40]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2015-11-11 12:35:58]

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

[commit-bot: I haz the power, 2015-11-11 13:05:55]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2015-11-11 13:06:41]

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