Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7084024:
Add chrome://quota-internals/ (Closed)
|
Created:
9 years, 6 months ago by tzik Modified:
9 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org Visibility:
Public. |
DescriptionAdd chrome://quota-internals/
BUG=84397
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=92356
Patch Set 1 #
Total comments: 33
Patch Set 2 : Reflect comments, move QuotaInternals{MessageHandler,Proxy} to .cc. #
Total comments: 41
Patch Set 3 : move QuotaInternals{MessageHandler,Proxy} to other file #Patch Set 4 : '' #
Total comments: 10
Patch Set 5 : '' #
Total comments: 2
Patch Set 6 : '' #
Total comments: 13
Patch Set 7 : '' #
Total comments: 16
Patch Set 8 : '' #
Total comments: 1
Patch Set 9 : '' #Patch Set 10 : Rebase, put Value on base namespace #Patch Set 11 : fix clang compile error #Messages
Total messages: 25 (0 generated)
This cl is {.h,.cc} part of chrome://quota-internals/,
diff to http://codereview.chromium.org/7084024/.
http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... File chrome/browser/ui/webui/quota_internals_ui.cc (right): http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:20: std::string toString(const T& v) { style-nit: please start function names with capital letter http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:26: std::string storageTypeToString(quota::StorageType type) { ditto http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:36: } style-nit: can you add a comment? } // anonymous namespace http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:105: if (data.usage >= 0) data.unlimited_usage ? http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:108: dict.SetString("quota", toString(data.quota)); Can we show these values even if they are < 0? Hmm I see we set -1 as n/a values... ok. http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:122: dict->SetString("type", storageTypeToString(itr->type)); could we simply show "unknown" for unknown types? http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:143: dict->SetString("type", storageTypeToString(itr->type)); ditto http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:186: void QuotaInternalsProxy::RunRequestData( For better readability can we rename this to RequestDataOnIOThread? http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:224: BrowserThread::UI)), You could use BrowserThread::PostTask(BrowserThread::{UI,IO}...) and get rid of these member vars I think http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:264: void QuotaInternalsProxy::RunReportHostData( For better readability can we rename this to ReportHostDataOnUIThread? http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:279: void QuotaInternalsProxy::RunReportOriginData( For better readability can we rename this to ReportOriginDataOnUIThread? http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:296: ReportGlobalData((GlobalData) {type, -1, -1, quota}); I might make this a constructor. qq: do they need to be a POD type? http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... File chrome/browser/ui/webui/quota_internals_ui.h (right): http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.h:39: virtual std::string GetMimeType(const std::string&) const; nit: OVERRIDE http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.h:46: class QuotaInternalsMessageHandler : public WebUIMessageHandler { nit: could this be declared in .cc? http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.h:99: QuotaInternalsProxyDeleter> { As we're in chrome/ I think you can simply use BrowserThread::DeleteOnIOThread http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.h:107: // Called on IO Thread style-nit: please end the comment with '.' http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.h:154: // base::ScopedCallbackFactory<QuotaInternalsProxy> callback_factory_; you forgot to delete this line?
http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... File chrome/browser/ui/webui/quota_internals_ui.cc (right): http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:461: base::WeakPtr<quota::QuotaManager> quota_manager_; can this be a scoped_refptr<> http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:521: AddDataSource(new quota_internals::QuotaInternalsHTMLSource); When is this data source removed? It looks like a new one is added for each tab in which quota-internals is viewed (is that right)? Do previously added ones get removed thru some means that's not visible in this CL? http://codereview.chromium.org/7084024/diff/5002/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7084024/diff/5002/webkit/quota/quota_manager.h... webkit/quota/quota_manager.h:81: public base::SupportsWeakPtr<QuotaManager> { Is this really needed? From the WebUI ctor, it looks like an instance is scoped to the life of an individual tab (which will hold the profile open). And the QuotaInternalsMessageHandler is scoped to the WebUI. And it looks like QuotaInternalsHTMLSource instance is scoped to the Profile (although when do they get removed?). Seems like you could safely addref the quota_manager from within your UI handler stuff. I guess what i'm asking is, what are the lifecycles of these objects.. QuotaInternalsUI QuotaInternalsMessageHandler QuotaInternalsHTMLSource
http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... File chrome/browser/ui/webui/quota_internals_ui.cc (right): http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:20: std::string toString(const T& v) { On 2011/05/30 10:18:00, kinuko wrote: > style-nit: please start function names with capital letter Done. http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:26: std::string storageTypeToString(quota::StorageType type) { On 2011/05/30 10:18:00, kinuko wrote: > ditto Done. http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:36: } On 2011/05/30 10:18:00, kinuko wrote: > style-nit: can you add a comment? > > } // anonymous namespace Done. http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:105: if (data.usage >= 0) On 2011/05/30 10:18:00, kinuko wrote: > data.unlimited_usage ? Done. http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:122: dict->SetString("type", storageTypeToString(itr->type)); On 2011/05/30 10:18:00, kinuko wrote: > could we simply show "unknown" for unknown types? Done. http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:143: dict->SetString("type", storageTypeToString(itr->type)); On 2011/05/30 10:18:00, kinuko wrote: > ditto Done. http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:186: void QuotaInternalsProxy::RunRequestData( On 2011/05/30 10:18:00, kinuko wrote: > For better readability can we rename this to RequestDataOnIOThread? Done. http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:224: BrowserThread::UI)), On 2011/05/30 10:18:00, kinuko wrote: > You could use BrowserThread::PostTask(BrowserThread::{UI,IO}...) and get rid of > these member vars I think Done. http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:264: void QuotaInternalsProxy::RunReportHostData( On 2011/05/30 10:18:00, kinuko wrote: > For better readability can we rename this to ReportHostDataOnUIThread? Done. http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:279: void QuotaInternalsProxy::RunReportOriginData( On 2011/05/30 10:18:00, kinuko wrote: > For better readability can we rename this to ReportOriginDataOnUIThread? Done. http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.cc:296: ReportGlobalData((GlobalData) {type, -1, -1, quota}); On 2011/05/30 10:18:00, kinuko wrote: > I might make this a constructor. > qq: do they need to be a POD type? Done. http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... File chrome/browser/ui/webui/quota_internals_ui.h (right): http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.h:39: virtual std::string GetMimeType(const std::string&) const; On 2011/05/30 10:18:00, kinuko wrote: > nit: OVERRIDE Done. http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.h:46: class QuotaInternalsMessageHandler : public WebUIMessageHandler { On 2011/05/30 10:18:00, kinuko wrote: > nit: could this be declared in .cc? Done. http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.h:99: QuotaInternalsProxyDeleter> { On 2011/05/30 10:18:00, kinuko wrote: > As we're in chrome/ I think you can simply use BrowserThread::DeleteOnIOThread Done. http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.h:107: // Called on IO Thread On 2011/05/30 10:18:00, kinuko wrote: > style-nit: please end the comment with '.' Done. http://codereview.chromium.org/7084024/diff/1/chrome/browser/ui/webui/quota_i... chrome/browser/ui/webui/quota_internals_ui.h:154: // base::ScopedCallbackFactory<QuotaInternalsProxy> callback_factory_; On 2011/05/30 10:18:00, kinuko wrote: > you forgot to delete this line? Done. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... File chrome/browser/ui/webui/quota_internals_ui.cc (right): http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:521: AddDataSource(new quota_internals::QuotaInternalsHTMLSource); On 2011/05/30 19:36:57, michaeln wrote: > When is this data source removed? It looks like a new one is added for each tab > in which quota-internals is viewed (is that right)? Do previously added ones get > removed thru some means that's not visible in this CL? Yes, we add DataSource in each quota-internals tab. Ownership of DataSource is passed to ChromeURLDataManager, and it hold DataSource as scoped_refptr in ChromeURLDataManager. I believe it controls the lifetime of DataSource in right way. http://codereview.chromium.org/7084024/diff/5002/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7084024/diff/5002/webkit/quota/quota_manager.h... webkit/quota/quota_manager.h:81: public base::SupportsWeakPtr<QuotaManager> { On 2011/05/30 19:36:57, michaeln wrote: > Is this really needed? > > From the WebUI ctor, it looks like an instance is scoped to the life of an > individual tab (which will hold the profile open). And the > QuotaInternalsMessageHandler is scoped to the WebUI. And it looks like > QuotaInternalsHTMLSource instance is scoped to the Profile (although when do > they get removed?). > > Seems like you could safely addref the quota_manager from within your UI handler > stuff. > > I guess what i'm asking is, what are the lifecycles of these objects.. > QuotaInternalsUI > QuotaInternalsMessageHandler > QuotaInternalsHTMLSource I'm sure that we can use scoped_ptr<QuotaManager> safely in QuotaInternals{UI,MessageHandler,HTMLSource} as you say. But I think, QuotaInternalsProxy is also owned by tasks made from Run***On{IO,UI}Thread. So, QuotaInternalsProxy may have longer lifetime than QuotaInternalsMessageHandler. I'm afraid scoped_refptr in QuotaInternalsProxy expands the lifetime of QuotaManager. I'm not sure that causes no problem.
Are there any WebUI based things whose data source is on the IO thread, any examples of such a WebUI arrangement? http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... File chrome/browser/ui/webui/quota_internals_ui.cc (right): http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:244: typedef QuotaInternalsProxy QuotaInternalsProxyDeleter; doesn't look used anywhere http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:248: BrowserThread::DeleteOnIOThread> { Extending the life of the QM shouldn't be an issue, but not being able to delete it (or this class) on the IO thread would be an issue. So long as this class is deleted on the IO thread, holding a scoped_refptr<QM> in here should not be a problem. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:424: (HostData) {host, type, usage, -1}); use the ctor here instead http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:473: void QuotaInternalsMessageHandler::OnRequestData(const ListValue*) { I'd move this method definition next to the other QuotaInternalsMessageHandler method w/o having the QuotaInternalsProxy method in between them. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:521: AddDataSource(new quota_internals::QuotaInternalsHTMLSource); On 2011/05/31 04:53:01, tzik wrote: > On 2011/05/30 19:36:57, michaeln wrote: > > When is this data source removed? It looks like a new one is added for each > tab > > in which quota-internals is viewed (is that right)? Do previously added ones > get > > removed thru some means that's not visible in this CL? > > Yes, we add DataSource in each quota-internals tab. > > Ownership of DataSource is passed to ChromeURLDataManager, and it hold > DataSource as scoped_refptr in ChromeURLDataManager. > I believe it controls the lifetime of DataSource in right way. I see, any old DataSource under this ones name is release when the new one is added... // Adds a DataSource to the collection of data sources. This *must* be invoked // on the UI thread. // // If |AddDataSource| is called more than once for a particular name it will // release the old |DataSource|, most likely resulting in it getting deleted // as there are no other references to it. |DataSource| uses the // |DeleteOnUIThread| trait to insure that the destructor is called on the UI // thread. This is necessary as some |DataSource|s notably |FileIconSource| // and |FaviconSource|, have members that will DCHECK if they are not // destructed in the same thread as they are constructed (the UI thread). void AddDataSource(DataSource* source); http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... File chrome/browser/ui/webui/quota_internals_ui.h (right): http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.h:18: #include "webkit/quota/quota_types.h" many of the includes in the .h file don't look needed. please move them to the .cc file as needed. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.h:20: class TabContents; this isn't really needed since it's forward delcared in web_ui.h which must be included here http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.h:25: virtual ~QuotaInternalsUI() {} is a dtor really needed at all? i think you could delete this line http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.h:30: i think this can melt away to... #include "content/browser/webui/web_ui.h" class QuotaInternalsUI : public WebUI { public: explicit QuotaInternalsUI(TabContents* contents); private: DISALLOW_COPY_AND_ASSIGN(QuotaInternalsUI); };
http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... File chrome/browser/ui/webui/quota_internals_ui.cc (right): http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:18: namespace { insert blank line http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:35: } insert blank line http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:177: class QuotaInternalsMessageHandler : public WebUIMessageHandler { this class needs to go in its own file http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:205: itr != end; ++itr) need curlies http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:206: values.Append(itr->NewValue()); newline http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:214: itr != end; ++itr) need curlies http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:224: itr != end; ++itr) need curlies http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:225: dict.SetString(itr->first, itr->second); newline http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:492: for (std::set<GURL>::iterator itr(origins.begin()), end(origins.end()); it's confusing to define a variable here that's not actually part of the iteration. I'd just do itr != origins.end()
On 2011/06/01 01:09:32, michaeln wrote: > Are there any WebUI based things whose data source is on the IO thread, any > examples of such a WebUI arrangement? I found gpu-internals, net-internals and print have similar situation. In gpu-internals, they make task for each JS messages. In net-internals, a proxy class on IO thread (IOThreadImpl) recieves and replies all JS message. Print preview UI is to be read.
http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... File chrome/browser/ui/webui/quota_internals_ui.cc (right): http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:18: namespace { On 2011/06/01 01:15:04, Evan Stade wrote: > insert blank line Done. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:35: } On 2011/06/01 01:15:04, Evan Stade wrote: > insert blank line Done. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:177: class QuotaInternalsMessageHandler : public WebUIMessageHandler { On 2011/06/01 01:15:04, Evan Stade wrote: > this class needs to go in its own file Done. Should I move QuotaInternalsProxy to another file? http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:205: itr != end; ++itr) On 2011/06/01 01:15:04, Evan Stade wrote: > need curlies Done. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:206: values.Append(itr->NewValue()); On 2011/06/01 01:15:04, Evan Stade wrote: > newline Done. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:214: itr != end; ++itr) On 2011/06/01 01:15:04, Evan Stade wrote: > need curlies Done. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:224: itr != end; ++itr) On 2011/06/01 01:15:04, Evan Stade wrote: > need curlies Done. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:225: dict.SetString(itr->first, itr->second); On 2011/06/01 01:15:04, Evan Stade wrote: > newline Done. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:244: typedef QuotaInternalsProxy QuotaInternalsProxyDeleter; On 2011/06/01 01:09:32, michaeln wrote: > doesn't look used anywhere Done. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:248: BrowserThread::DeleteOnIOThread> { On 2011/06/01 01:09:32, michaeln wrote: > Extending the life of the QM shouldn't be an issue, but not being able to delete > it (or this class) on the IO thread would be an issue. So long as this class is > deleted on the IO thread, holding a scoped_refptr<QM> in here should not be a > problem. I see. Done. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:424: (HostData) {host, type, usage, -1}); On 2011/06/01 01:09:32, michaeln wrote: > use the ctor here instead Done. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:461: base::WeakPtr<quota::QuotaManager> quota_manager_; On 2011/05/30 19:36:57, michaeln wrote: > can this be a scoped_refptr<> Done. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:473: void QuotaInternalsMessageHandler::OnRequestData(const ListValue*) { On 2011/06/01 01:09:32, michaeln wrote: > I'd move this method definition next to the other QuotaInternalsMessageHandler > method w/o having the QuotaInternalsProxy method in between them. Done. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:492: for (std::set<GURL>::iterator itr(origins.begin()), end(origins.end()); On 2011/06/01 01:15:04, Evan Stade wrote: > it's confusing to define a variable here that's not actually part of the > iteration. I'd just do itr != origins.end() Done. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... File chrome/browser/ui/webui/quota_internals_ui.h (right): http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.h:18: #include "webkit/quota/quota_types.h" On 2011/06/01 01:09:32, michaeln wrote: > many of the includes in the .h file don't look needed. please move them to the > .cc file as needed. Done. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.h:20: class TabContents; On 2011/06/01 01:09:32, michaeln wrote: > this isn't really needed since it's forward delcared in web_ui.h which must be > included here Done. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.h:25: virtual ~QuotaInternalsUI() {} On 2011/06/01 01:09:32, michaeln wrote: > is a dtor really needed at all? i think you could delete this line Done. http://codereview.chromium.org/7084024/diff/5002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.h:30: On 2011/06/01 01:09:32, michaeln wrote: > i think this can melt away to... > > #include "content/browser/webui/web_ui.h" > > class QuotaInternalsUI : public WebUI { > public: > explicit QuotaInternalsUI(TabContents* contents); > > private: > DISALLOW_COPY_AND_ASSIGN(QuotaInternalsUI); > }; Done.
Thanks for you all, http://codereview.chromium.org/7053009/ has landed, then how about this one? Could you look this again?
http://codereview.chromium.org/7084024/diff/23001/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_handler.h (right): http://codereview.chromium.org/7084024/diff/23001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_handler.h:28: class GlobalData { class comments on all of these. Why are the first 2 classes and the last one a struct? http://codereview.chromium.org/7084024/diff/23001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_handler.h:101: class QuotaInternalsProxy I think this warrants its own file http://codereview.chromium.org/7084024/diff/23001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_handler.h:118: typedef quota::QuotaManager::QuotaTableEntry QuotaTableEntry; I don't see where this is used http://codereview.chromium.org/7084024/diff/23001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_handler.h:119: typedef quota::QuotaManager::OriginInfoTableEntry OriginInfoTableEntry; or this one http://codereview.chromium.org/7084024/diff/23001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_handler.h:121: typedef quota::QuotaManager::OriginInfoTableEntries OriginInfoTableEntries; I think you should remove this typedef and if you want to reduce verbosity in the c++ file, you can use `using`.
http://codereview.chromium.org/7084024/diff/23001/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_handler.h (right): http://codereview.chromium.org/7084024/diff/23001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_handler.h:28: class GlobalData { On 2011/06/28 17:36:49, Evan Stade wrote: > class comments on all of these. > > Why are the first 2 classes and the last one a struct? Done. That is forgotten to modify. Thanks. http://codereview.chromium.org/7084024/diff/23001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_handler.h:101: class QuotaInternalsProxy On 2011/06/28 17:36:49, Evan Stade wrote: > I think this warrants its own file Done. http://codereview.chromium.org/7084024/diff/23001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_handler.h:118: typedef quota::QuotaManager::QuotaTableEntry QuotaTableEntry; On 2011/06/28 17:36:49, Evan Stade wrote: > I don't see where this is used Done. http://codereview.chromium.org/7084024/diff/23001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_handler.h:119: typedef quota::QuotaManager::OriginInfoTableEntry OriginInfoTableEntry; On 2011/06/28 17:36:49, Evan Stade wrote: > or this one Done. http://codereview.chromium.org/7084024/diff/23001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_handler.h:121: typedef quota::QuotaManager::OriginInfoTableEntries OriginInfoTableEntries; On 2011/06/28 17:36:49, Evan Stade wrote: > I think you should remove this typedef and if you want to reduce verbosity in > the c++ file, you can use `using`. Did you mean "using quota::QuotaManager::QuotaTableEntries;"? That is allowed for only subclass of QuotaManager. Or did you mean "using quota::QuotaManager;"?
http://codereview.chromium.org/7084024/diff/32001/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_proxy.cc (right): http://codereview.chromium.org/7084024/diff/32001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_proxy.cc:26: available_space)); nit: there seem to be so many mostly-empty relaying methods in this class. For the sake of simplicity, is it possible/reasonable to merge some of those methods? like: QuotaInternalsProxy::ReportFooToUIThread(...) { if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) { BrowserThread::PostTask(... /* call this method on UI thread*/); return; } if (handler_) handler_->ReportFoo(...); }
http://codereview.chromium.org/7084024/diff/32001/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_proxy.cc (right): http://codereview.chromium.org/7084024/diff/32001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_proxy.cc:26: available_space)); On 2011/07/01 06:51:37, kinuko wrote: > nit: there seem to be so many mostly-empty relaying methods in this class. For > the sake of simplicity, is it possible/reasonable to merge some of those > methods? > > like: > QuotaInternalsProxy::ReportFooToUIThread(...) { > if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) { > BrowserThread::PostTask(... /* call this method on UI thread*/); > return; > } > if (handler_) > handler_->ReportFoo(...); > } Done.
A few more nits. The change looks good to me if it looks good to others. http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_handler.h (right): http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_handler.h:29: // All methods in this class can be called on UI thread. nit: 'can be' or 'should be'? http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_proxy.h (right): http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_proxy.h:30: // This class is the bridge between QuotaInternalsHandler and QuotaManager. nit: Would be nice to also note that who creates and owns an instance. http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_types.h (right): http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_types.h:98: typedef std::map<std::string, std::string> Statistics; Not necessary? (Or it might be worth having this in quota_types.h, removing dup'ed lines in many .h files)
Some more comments about HostData/GlobalData constructor parameters for unavailable/undefined values. http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_proxy.cc (right): http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_proxy.cc:227: quota_manager_->IsOriginInUse(*itr) ? 1 : 0, -1, The fact that the caller needs to explicitly give these magic constant values (like: -1, 0, 1 for uninitialized, false and true) may look a bit awkward... Instead of having the generic constructor that requires initial values for all the member variables could we have a minimal constructor which only takes the parameters that are always available? origin_data.push_back(OriginData(*itr, type)); origin_data.back().set_in_use(qm_->IsOriginUse(*itr)); The constructor could initialize remaining member variables with hidden magic 'uninitialized' values (like: -1 for used_count_). The code would become a bit longer but caller would no longer need to know what values they should give for 'undefined/uninitialized' values. Wdyt? http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_types.h (right): http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_types.h:33: // deleting the result. nit: 'deleting the returned pointer' might be better http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_types.h:88: // be available only for temporary storage for now. Do we need this comment here (in this file)? I think the entities dealing with this class should only know what values they should set/see when some values are not available. (same for the comment in HostData) http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_types.h:91: // * base::Time() for |last_access_time_| and |last_modified_time_|. Should this be noted at the constructor if the caller needs to know these conventions? (Please also see the other comment in quota_internals_proxy.cc)
Thanks.
I reflected kinuko-san's comments.
Also, I renamed {Global,Host,Origin}Data to
{Global,PerHost,PerOrigin}StorageInfo in this change set.
http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo...
File chrome/browser/ui/webui/quota_internals_handler.h (right):
http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo...
chrome/browser/ui/webui/quota_internals_handler.h:29: // All methods in this
class can be called on UI thread.
On 2011/07/04 08:24:27, kinuko wrote:
> nit: 'can be' or 'should be'?
Done.
http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo...
File chrome/browser/ui/webui/quota_internals_proxy.cc (right):
http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo...
chrome/browser/ui/webui/quota_internals_proxy.cc:227:
quota_manager_->IsOriginInUse(*itr) ? 1 : 0, -1,
On 2011/07/04 08:58:29, kinuko wrote:
> The fact that the caller needs to explicitly give these magic constant values
> (like: -1, 0, 1 for uninitialized, false and true) may look a bit awkward...
>
> Instead of having the generic constructor that requires initial values for all
> the member variables could we have a minimal constructor which only takes the
> parameters that are always available?
>
> origin_data.push_back(OriginData(*itr, type));
> origin_data.back().set_in_use(qm_->IsOriginUse(*itr));
>
> The constructor could initialize remaining member variables with hidden magic
> 'uninitialized' values (like: -1 for used_count_). The code would become a
bit
> longer but caller would no longer need to know what values they should give
for
> 'undefined/uninitialized' values. Wdyt?
Done. Sounds good.
http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo...
File chrome/browser/ui/webui/quota_internals_proxy.h (right):
http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo...
chrome/browser/ui/webui/quota_internals_proxy.h:30: // This class is the bridge
between QuotaInternalsHandler and QuotaManager.
On 2011/07/04 08:24:27, kinuko wrote:
> nit: Would be nice to also note that who creates and owns an instance.
Done.
http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo...
File chrome/browser/ui/webui/quota_internals_types.h (right):
http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo...
chrome/browser/ui/webui/quota_internals_types.h:33: // deleting the result.
On 2011/07/04 08:58:29, kinuko wrote:
> nit: 'deleting the returned pointer' might be better
Done.
http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo...
chrome/browser/ui/webui/quota_internals_types.h:88: // be available only for
temporary storage for now.
On 2011/07/04 08:58:29, kinuko wrote:
> Do we need this comment here (in this file)? I think the entities dealing
with
> this class should only know what values they should set/see when some values
are
> not available.
> (same for the comment in HostData)
Done.
http://codereview.chromium.org/7084024/diff/35001/chrome/browser/ui/webui/quo...
chrome/browser/ui/webui/quota_internals_types.h:98: typedef
std::map<std::string, std::string> Statistics;
On 2011/07/04 08:24:27, kinuko wrote:
> Not necessary?
> (Or it might be worth having this in quota_types.h, removing dup'ed lines in
> many .h files)
Done.
quota related c++ code LGTM
http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_proxy.cc (right): http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_proxy.cc:54: if (handler_) seems like you could move this check above the thread check, it would save a small amount of work. http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_proxy.cc:71: void QuotaInternalsProxy::ReportStatistics(const Statistics& stats) { you use this pattern a lot, it seems ripe for a macro. http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_types.cc (right): http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_types.cc:34: scoped_ptr<DictionaryValue> dict(new DictionaryValue); not much point in using a scoped_ptr here http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_types.cc:37: dict->SetDouble("usage", static_cast<double>(usage_)); maybe we should have a LongInteger Value type (or make Integer take int64 if it's easy), could you add a TODO/file a bug? http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_types.cc:51: scoped_ptr<DictionaryValue> dict(new DictionaryValue); ditto http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_types.cc:72: scoped_ptr<DictionaryValue> dict(new DictionaryValue); ditto http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_types.cc:84: if (!last_modified_time_.is_null()) curlies for if statements that have multi-line bodies (even if they are continuation lines)
http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_proxy.cc (right): http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_proxy.cc:54: if (handler_) On 2011/07/07 20:58:17, Evan Stade wrote: > seems like you could move this check above the thread check, it would save a > small amount of work. Done. http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_proxy.cc:71: void QuotaInternalsProxy::ReportStatistics(const Statistics& stats) { On 2011/07/07 20:58:17, Evan Stade wrote: > you use this pattern a lot, it seems ripe for a macro. Done. http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_types.cc (right): http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_types.cc:34: scoped_ptr<DictionaryValue> dict(new DictionaryValue); On 2011/07/07 20:58:17, Evan Stade wrote: > not much point in using a scoped_ptr here Done. http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_types.cc:37: dict->SetDouble("usage", static_cast<double>(usage_)); On 2011/07/07 20:58:17, Evan Stade wrote: > maybe we should have a LongInteger Value type (or make Integer take int64 if > it's easy), could you add a TODO/file a bug? I want it too. ECMAScript should have richer datatypes around integers. But where can I post it? It looks very deep roots. http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_types.cc:51: scoped_ptr<DictionaryValue> dict(new DictionaryValue); On 2011/07/07 20:58:17, Evan Stade wrote: > ditto Done. http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_types.cc:72: scoped_ptr<DictionaryValue> dict(new DictionaryValue); On 2011/07/07 20:58:17, Evan Stade wrote: > ditto Done. http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_types.cc:84: if (!last_modified_time_.is_null()) On 2011/07/07 20:58:17, Evan Stade wrote: > curlies for if statements that have multi-line bodies (even if they are > continuation lines) Done.
lgtm http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_types.cc (right): http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_types.cc:37: dict->SetDouble("usage", static_cast<double>(usage_)); On 2011/07/07 23:00:35, tzik wrote: > On 2011/07/07 20:58:17, Evan Stade wrote: > > maybe we should have a LongInteger Value type (or make Integer take int64 if > > it's easy), could you add a TODO/file a bug? > > I want it too. ECMAScript should have richer datatypes around integers. I thought you were using double because int (base type of integer FundamentalValue) can't store large enough numbers? > But where can I post it? It looks very deep roots. base/values.{h,cc} (eventually, not in this CL) http://codereview.chromium.org/7084024/diff/51001/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_proxy.cc (right): http://codereview.chromium.org/7084024/diff/51001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_proxy.cc:42: #undef RELAY_TO_HANDLER I don't think you need to worry about undefing this since it's in a .cc file, but I guess it doesn't hurt either.
http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_types.cc (right): http://codereview.chromium.org/7084024/diff/39002/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_types.cc:37: dict->SetDouble("usage", static_cast<double>(usage_)); On 2011/07/11 20:53:23, Evan Stade wrote: > On 2011/07/07 23:00:35, tzik wrote: > > On 2011/07/07 20:58:17, Evan Stade wrote: > > > maybe we should have a LongInteger Value type (or make Integer take int64 if > > > it's easy), could you add a TODO/file a bug? > > > > I want it too. ECMAScript should have richer datatypes around integers. > > I thought you were using double because int (base type of integer > FundamentalValue) can't store large enough numbers? Yes, It may be greater than 2^31. > > But where can I post it? It looks very deep roots. > > base/values.{h,cc} (eventually, not in this CL) OK. I'll post it. I wonder how should it behave when we pack it to JSON with near 2^64 integers. Value is tightly related to JavaScript which doesn't have Int64. I thought we need to modify JavaScript language to add Int64 safely.
yea, it seems you're right. ignore me
Try job failure for 7084024-55001 (retry) on linux for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...
Commit queue patch verification failed without an error message. Something went wrong, probably a crash, a hickup or simply the monkeys went out for diner. Ping the relevant dude on a on-needed basis.
Change committed as 92356 |
