Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1607483005:
Show data usage on Site Details (MDSettings) (Closed)
|
Created:
4 years, 11 months ago by Finnur Modified:
4 years, 10 months ago CC:
chromium-reviews, msramek+watch_chromium.org, michaelpg+watch-md-settings_chromium.org, jlklein+watch-closure_chromium.org, raymes+watch_chromium.org, arv+watch_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, markusheintz_, dbeam+watch-closure_chromium.org, msramek Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow data usage (storage) on Site Details (MDSettings).
Also create a basic abstraction layer for
SiteSettings to use when communicating with
the C++ layer.
BUG=543635
Committed: https://crrev.com/a7fe8e8f31f284d05056d54cb7e88b050c9513f6
Cr-Commit-Position: refs/heads/master@{#372102}
Patch Set 1 : #
Total comments: 34
Patch Set 2 : Address feedback #Patch Set 3 : Test (WIP) #
Total comments: 2
Patch Set 4 : Augment test #
Total comments: 19
Patch Set 5 : Address feedback #
Total comments: 6
Patch Set 6 : Address feedback and add owner #
Total comments: 16
Patch Set 7 : Address feedback #Patch Set 8 : Fix update problem #Messages
Total messages: 43 (21 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Show data usage on Site Details (MDSettings) BUG=543635 ========== to ========== Show data usage on Site Details (MDSettings). Also create a basic abstraction layer for SiteSettings to use when communicating with the C++ layer. BUG=543635 ==========
finnur@chromium.org changed reviewers: + michaelpg@chromium.org
PTAL. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... File chrome/browser/content_settings/storage_info_fetcher.h (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/storage_info_fetcher.h:11: class StorageInfoFetcher : This class is really a distillation of the one that serves the same purpose in website_preference_bridge.cc (not in this CL), and I intend to convert to use this as base as well, much like how site_settings_handler.cc in this CL uses it. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_handler.js (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_handler.js:40: }; Is all of the above a reasonable way to do this abstraction layer or is there a better way?
michaelpg@chromium.org changed reviewers: + stevenjb@chromium.org
michaelpg@chromium.org changed required reviewers: + michaelpg@chromium.org
I've added stevenjb for his thoughts on the JS "API" for site settings, he's done similar call-and-respond stuff for networks. stevenjb: just ctrl-f for "stevenjb" https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... File chrome/browser/content_settings/storage_info_fetcher.cc (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/storage_info_fetcher.cc:17: AddRef(); // Balanced in OnGetUsageInfoInternal. nit: 2 spaces before comment https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/storage_info_fetcher.cc:27: base::Bind(&StorageInfoFetcher::OnGetUsageInfoInternal, this)); does Bind have to be called on the IO thread? if not, why not post base::Bind(&QuotaManager::GetUsageInfo, quota_manager_, base::Bind(&StorageInfoFetcher::OnGetUsageInfoInternal)) in Run? https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... File chrome/browser/content_settings/storage_info_fetcher.h (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/storage_info_fetcher.h:8: #include "content/public/browser/browser_thread.h" is this used here? https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/storage_info_fetcher.h:11: class StorageInfoFetcher : add comment for class https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/storage_info_fetcher.h:12: public base::RefCountedThreadSafe<StorageInfoFetcher> { include ref_counted.h https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_resources.grd:478: <structure name="IDR_SETTINGS_SITE_SETTINGS_SITE_SETTINGS_HANDLER_JS" opt nit: use IDR_SETTINGS_SITE_SETTINGS_HANDLER_JS for consistency with site_settings/site_settings_* files above https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_details.html (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.html:14: <h2 i18n-content="siteSettingsUsage" id="usage" hidden></h2> alternatively: hidden$="[[!storedData_]]" and give storedData_ a default value of '' https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_details.js (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.js:64: this.OnUsageDataAvailable); // The callback. just bind this to |this| instead of passing the |this| object separately or maybe use a listener? +stevenjb https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.js:71: OnUsageDataAvailable: function(usage) { camelCase also, would you mind renaming this? i find it confusing that OnUsageDataAvailable is also defined on SiteSettingsHandler. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_handler.js (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_handler.js:15: * @param {Function=} callback The callback to run when the data is ready. uncapitalize function, and have it take a string: {function(string)} the "=" means "optional": if this is really optional, rename to opt_callback. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_handler.js:40: }; On 2016/01/19 17:08:57, Finnur wrote: > Is all of the above a reasonable way to do this abstraction layer or is there a > better way? I would make SiteSettingsHandler "static" and simplify the callback structure. +stevenjb for his thoughts. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:7: #include "base/bind.h" nit: include values.h https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:16: class MDSettingsStorageInfoFetcher : public StorageInfoFetcher { discussion question: Would it be better to compose and observe a StorageInfoFetcher instead of inheriting from it? https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.h (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.h:30: // Asynchronuysly fetches the usage for a given origin. Replies back with Asynchronously
raymes@chromium.org changed reviewers: + raymes@chromium.org
Small drive-by comment :) You may also want to update the CL description to specifically refer to "storage"? https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... File chrome/browser/content_settings/storage_info_fetcher.h (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/storage_info_fetcher.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Hmm not sure if this is the right place to put this file. I would either put it with the web UI side of things or the storage side of things.
Description was changed from ========== Show data usage on Site Details (MDSettings). Also create a basic abstraction layer for SiteSettings to use when communicating with the C++ layer. BUG=543635 ========== to ========== Show data usage (storage) on Site Details (MDSettings). Also create a basic abstraction layer for SiteSettings to use when communicating with the C++ layer. BUG=543635 ==========
Patchset #2 (id:60001) has been deleted
All addressed, I believe. PTAL. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... File chrome/browser/content_settings/storage_info_fetcher.cc (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/storage_info_fetcher.cc:17: AddRef(); // Balanced in OnGetUsageInfoInternal. On 2016/01/19 20:47:54, michaelpg wrote: > nit: 2 spaces before comment Done. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/storage_info_fetcher.cc:27: base::Bind(&StorageInfoFetcher::OnGetUsageInfoInternal, this)); On 2016/01/19 20:47:54, michaelpg wrote: > does Bind have to be called on the IO thread? if not, why not post > > base::Bind(&QuotaManager::GetUsageInfo, quota_manager_, > base::Bind(&StorageInfoFetcher::OnGetUsageInfoInternal)) > > in Run? Done. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... File chrome/browser/content_settings/storage_info_fetcher.h (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/storage_info_fetcher.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. It doesn't belong under Web UI because I plan on making Android depend on this as well, so I moved it with storage. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/storage_info_fetcher.h:8: #include "content/public/browser/browser_thread.h" Nope, moved. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/storage_info_fetcher.h:11: class StorageInfoFetcher : On 2016/01/19 20:47:54, michaelpg wrote: > add comment for class Done. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/storage_info_fetcher.h:12: public base::RefCountedThreadSafe<StorageInfoFetcher> { On 2016/01/19 20:47:54, michaelpg wrote: > include ref_counted.h Done. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_resources.grd:478: <structure name="IDR_SETTINGS_SITE_SETTINGS_SITE_SETTINGS_HANDLER_JS" On 2016/01/19 20:47:54, michaelpg wrote: > opt nit: use IDR_SETTINGS_SITE_SETTINGS_HANDLER_JS for consistency with > site_settings/site_settings_* files above Done. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_details.html (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.html:14: <h2 i18n-content="siteSettingsUsage" id="usage" hidden></h2> On 2016/01/19 20:47:54, michaelpg wrote: > alternatively: hidden$="[[!storedData_]]" > > and give storedData_ a default value of '' Done. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_details.js (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.js:64: this.OnUsageDataAvailable); // The callback. Obsolete code now. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.js:71: OnUsageDataAvailable: function(usage) { Also obsolete. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_handler.js (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_handler.js:15: * @param {Function=} callback The callback to run when the data is ready. On 2016/01/19 20:47:54, michaelpg wrote: > uncapitalize function, and have it take a string: {function(string)} > > the "=" means "optional": if this is really optional, rename to opt_callback. Done. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_handler.js:40: }; Obsolete code now. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:7: #include "base/bind.h" On 2016/01/19 20:47:54, michaelpg wrote: > nit: include values.h Done. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:16: class MDSettingsStorageInfoFetcher : public StorageInfoFetcher { Yeah, I like that. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.h (right): https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/01/19 20:47:54, michaelpg wrote: > no (c) Done. https://codereview.chromium.org/1607483005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.h:30: // Asynchronuysly fetches the usage for a given origin. Replies back with On 2016/01/19 20:47:54, michaelpg wrote: > Asynchronously Done.
CL 3 is for a test I was experimenting with. https://codereview.chromium.org/1607483005/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/site_details_tests.js (right): https://codereview.chromium.org/1607483005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/site_details_tests.js:11: }; This is purely here to facilitate discussion. I ran out of time before completing the test, but would appreciate a comment on whether this is in line with what you would expect, or if I need to take a different approach. Currently, I see that the storedData_ is set on an HTMLElement, but the Polymer object that I'm testing does not appear to show the UI through hidden$="[[!storedData_]]". It therefore asserts on line 158.
> CL 3 is for a test I was experimenting with. Patchset 3, I mean.
finnur@chromium.org changed reviewers: + dgrogan@chromium.org
+David for OWNERS approval for two files added in: chrome/browser/storage/ I added them there because I intend to share it with Chrome for Android code to fix issue crbug.com/579109 (there is a similar, but faulty version of this class there, which I intend to delete). https://codereview.chromium.org/1607483005/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/site_details_tests.js (right): https://codereview.chromium.org/1607483005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/site_details_tests.js:11: }; Ignore the comment above, this code is obsolete. Instead, look at the test below and let me know if this approach is valid.
Also (forgot to mention): Michael, PTAL. :)
rs lgtm for chrome/browser/storage owner Want to add yourself as an OWNER? Then that directory would have GMT coverage.
just looked at web-side stuff for now https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_details.html (right): https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_details.html:53: <website-usage-private-api id="usageApi" website-usage-private-api element seems to expect to only be created once, is that correct? how do you enforce this (prevent people from making another site-details instance)? https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/website_usage_private_api.js (right): https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:58: instance.websiteDataUsage = usage; shouldn't we check that the origin is still the same? e.g.: user checks usage data for A.com, user checks usage data for B.com, C++ responds with A.com usage, site details for B.com shows A.com usage https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.h (right): https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.h:35: // Asynchronusly fetches the usage for a given origin. Replies back with Asynchronously :-) https://codereview.chromium.org/1607483005/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/site_details_tests.js (right): https://codereview.chromium.org/1607483005/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/site_details_tests.js:146: var parent = testElement.$.usageApi.parentNode; see Element.prototype.remove https://codereview.chromium.org/1607483005/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/site_details_tests.js:151: is:"mock-website-usage-private-api", space https://codereview.chromium.org/1607483005/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/site_details_tests.js:151: is:"mock-website-usage-private-api", use single quotes in JS, here & below https://codereview.chromium.org/1607483005/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/site_details_tests.js:157: testElement.$['usageApi'] = api; $.usageApi
PTAL https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_details.html (right): https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_details.html:53: <website-usage-private-api id="usageApi" This seems to me like a general problem with the solution proposed by Hector, no? I recognized a solution would be needed for the settings-site-list element as that is created twice in the site-settings-category element, but I wasn't planning on enforcing it for this element -- because it is designed to be used only once. But, as you point out, I can't prevent someone in the future taking the element and trying to create another instance. I don't have a good answer -- not sure what construct is appropriate in js/polymer to enforce this. Do you have any suggestions? Should we make all these objects handle being created multiple times? Or enforce single-use by default and just handle "duplicity" as needed? https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/website_usage_private_api.js (right): https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:58: instance.websiteDataUsage = usage; Like so? https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.h (right): https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.h:35: // Asynchronusly fetches the usage for a given origin. Replies back with Thanks. (Jeez, fingers -- for once... can you work like you are supposed to!?) ;) https://codereview.chromium.org/1607483005/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/site_details_tests.js (right): https://codereview.chromium.org/1607483005/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/site_details_tests.js:146: var parent = testElement.$.usageApi.parentNode; On 2016/01/25 18:33:54, michaelpg wrote: > see Element.prototype.remove Done. https://codereview.chromium.org/1607483005/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/site_details_tests.js:151: is:"mock-website-usage-private-api", Done. Here & below & above. :) https://codereview.chromium.org/1607483005/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/site_details_tests.js:157: testElement.$['usageApi'] = api; On 2016/01/25 18:33:54, michaelpg wrote: > $.usageApi Done.
https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_details.html (right): https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_details.html:53: <website-usage-private-api id="usageApi" On 2016/01/26 15:54:53, Finnur wrote: > This seems to me like a general problem with the solution proposed by Hector, > no? > > I recognized a solution would be needed for the settings-site-list element as > that is created twice in the site-settings-category element, but I wasn't > planning on enforcing it for this element -- because it is designed to be used > only once. But, as you point out, I can't prevent someone in the future taking > the element and trying to create another instance. > > I don't have a good answer -- not sure what construct is appropriate in > js/polymer to enforce this. Do you have any suggestions? Should we make all > these objects handle being created multiple times? Or enforce single-use by > default and just handle "duplicity" as needed? I'm not sure either. I think we should raise the question on the thread. I wouldn't block this CL on that though. https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/website_usage_private_api.js (right): https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:58: instance.websiteDataUsage = usage; On 2016/01/26 15:54:53, Finnur wrote: > Like so? Yeah, but no need to assert -- it's a valid sequence of events, and I think just ignoring the outdated response is the right thing to do. https://codereview.chromium.org/1607483005/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/site_details_tests.js (right): https://codereview.chromium.org/1607483005/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/site_details_tests.js:146: var parent = testElement.$.usageApi.parentNode; On 2016/01/26 15:54:54, Finnur wrote: > On 2016/01/25 18:33:54, michaelpg wrote: > > see Element.prototype.remove > > Done. Sorry, that was silly of me: i meant that the node, as an instance of Element.prototype, has that function! testElement.$.usageApi.remove(); of course what you have works exactly the same, but is more verbose than necessary. https://codereview.chromium.org/1607483005/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_details.js (right): https://codereview.chromium.org/1607483005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_details.js:63: var urlParser = document.createElement('a'); 1. Perhaps use `new URL(this.origin)` if it's equivalent but cheaper? 2. The behavior seems really wonky: var a = document.createElement('a'); a.href = 'http://google.com:442'; a.host // 'google.com:442' a.href = 'http://google.com:443'; a.host // 'google.com:443' a.href = 'https://google.com:443'; a.host // 'google.com' a.href = 'http:google.com'; a.host // 'google.com' on https://example.com: a.href = 'http:google.com'; a.host // 'google.com' a.href = 'https:google.com'; a.host // 'example.com' on https://example.com: a.href = 'http:google.com'; a.host // 'example.com' a.href = 'https:google.com'; a.host // 'google.com' https://codereview.chromium.org/1607483005/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/website_usage_private_api.js (right): https://codereview.chromium.org/1607483005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:50: if (instance != null) { nit: no {} around if https://codereview.chromium.org/1607483005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:70: if (this.host == host) { same nit
Patchset #6 (id:160001) has been deleted
All addressed. Michael, PTAL. https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_details.html (right): https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_details.html:53: <website-usage-private-api id="usageApi" OK, I'll bring it up. https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/website_usage_private_api.js (right): https://codereview.chromium.org/1607483005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:58: instance.websiteDataUsage = usage; On 2016/01/27 06:37:46, michaelpg wrote: > On 2016/01/26 15:54:53, Finnur wrote: > > Like so? > > Yeah, but no need to assert -- it's a valid sequence of events, and I think just > ignoring the outdated response is the right thing to do. Done. https://codereview.chromium.org/1607483005/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/site_details_tests.js (right): https://codereview.chromium.org/1607483005/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/site_details_tests.js:146: var parent = testElement.$.usageApi.parentNode; On 2016/01/27 06:37:46, michaelpg wrote: > On 2016/01/26 15:54:54, Finnur wrote: > > On 2016/01/25 18:33:54, michaelpg wrote: > > > see Element.prototype.remove > > > > Done. > > Sorry, that was silly of me: i meant that the node, as an instance of > Element.prototype, has that function! > > testElement.$.usageApi.remove(); > > of course what you have works exactly the same, but is more verbose than > necessary. Done. https://codereview.chromium.org/1607483005/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_details.js (right): https://codereview.chromium.org/1607483005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_details.js:63: var urlParser = document.createElement('a'); Thanks for investigating! Switched to the URL object and now strip away the port. https://codereview.chromium.org/1607483005/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/website_usage_private_api.js (right): https://codereview.chromium.org/1607483005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:50: if (instance != null) { On 2016/01/27 06:37:47, michaelpg wrote: > nit: no {} around if Done. https://codereview.chromium.org/1607483005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:70: if (this.host == host) { On 2016/01/27 06:37:47, michaelpg wrote: > same nit Done.
mostly lgtm https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_details.js (right): https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_details.js:64: this.$.usageApi.fetchUsageTotal(url.host.split(':')[0]); Looks like this is equivalent to url.hostname: https://developer.mozilla.org/en-US/docs/Web/API/URL https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/website_usage_private_api.js (right): https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:25: fetchUsageTotal: function(host) { /** @param {string} host */ https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:36: websiteUsagePolymerInstance = null; var https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:48: fetchUsageTotal = function(host) { var. You're creating global variables, they should be local (and then returned in the return function at the bottom) https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:53: this.host = host; do you mean to use the "var host" defined at 41? you could rename it "host_" or something, and set it instead of using "this": host_ = host; https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:39: for (i = entries.begin(); i != entries.end(); ++i) { opt nit: consider "for (const auto& entry : entries)" https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.h (right): https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.h:16: class Value; ListValue
All done. Checking in. https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_details.js (right): https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_details.js:64: this.$.usageApi.fetchUsageTotal(url.host.split(':')[0]); On 2016/01/27 18:42:47, michaelpg wrote: > Looks like this is equivalent to url.hostname: > https://developer.mozilla.org/en-US/docs/Web/API/URL Done. https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/website_usage_private_api.js (right): https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:25: fetchUsageTotal: function(host) { On 2016/01/27 18:42:48, michaelpg wrote: > /** @param {string} host */ Done. https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:36: websiteUsagePolymerInstance = null; On 2016/01/27 18:42:48, michaelpg wrote: > var Done. https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:48: fetchUsageTotal = function(host) { Done, I believe. Happy to do a follow-up if I got it wrong (but it seems to work fine). :) https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:53: this.host = host; On 2016/01/27 18:42:48, michaelpg wrote: > do you mean to use the "var host" defined at 41? you could rename it "host_" or > something, and set it instead of using "this": > > host_ = host; Done. https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/01/27 18:42:48, michaelpg wrote: > no (c) Done. https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:39: for (i = entries.begin(); i != entries.end(); ++i) { On 2016/01/27 18:42:48, michaelpg wrote: > opt nit: consider "for (const auto& entry : entries)" Done. https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.h (right): https://codereview.chromium.org/1607483005/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.h:16: class Value; On 2016/01/27 18:42:48, michaelpg wrote: > ListValue Done.
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgrogan@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1607483005/#ps200001 (title: "Address feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1607483005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1607483005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
Patchset #8 (id:220001) has been deleted
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgrogan@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1607483005/#ps240001 (title: "Fix update problem")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1607483005/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1607483005/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by finnur@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1607483005/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1607483005/240001
Message was sent while issue was closed.
Description was changed from ========== Show data usage (storage) on Site Details (MDSettings). Also create a basic abstraction layer for SiteSettings to use when communicating with the C++ layer. BUG=543635 ========== to ========== Show data usage (storage) on Site Details (MDSettings). Also create a basic abstraction layer for SiteSettings to use when communicating with the C++ layer. BUG=543635 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Show data usage (storage) on Site Details (MDSettings). Also create a basic abstraction layer for SiteSettings to use when communicating with the C++ layer. BUG=543635 ========== to ========== Show data usage (storage) on Site Details (MDSettings). Also create a basic abstraction layer for SiteSettings to use when communicating with the C++ layer. BUG=543635 Committed: https://crrev.com/a7fe8e8f31f284d05056d54cb7e88b050c9513f6 Cr-Commit-Position: refs/heads/master@{#372102} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/a7fe8e8f31f284d05056d54cb7e88b050c9513f6 Cr-Commit-Position: refs/heads/master@{#372102} |
