Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23240002:
Backend for DevTools quota managements. (Closed)
|
Created:
7 years, 4 months ago by SeRya Modified:
7 years, 2 months ago CC:
chromium-reviews, michaeln, vsevik, tzik+watch_chromium.org, jam, yurys, joi+watch-content_chromium.org, paulirish+reviews_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, kinuko+watch, aandrey+blink_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionBackend for DevTools quota managements.
Introduced handler for 'Quota.queryUsageAndQuota(securityOrigin)' command of DevTools protocol
(the command to be added in the protocol in Blink) providing detailed information about storage
usage and quota.
TBR=jochen@chromium.org for content/content_tests.gypi
BUG=281252
TEST=content_browsertests --gtest_filter=RendererOverridesHandlerTest.*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225448
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Unit tests fixed. #Patch Set 4 : WinDbg fix. #Patch Set 5 : Merged #
Total comments: 20
Patch Set 6 : #Patch Set 7 : Merged. #
Total comments: 4
Patch Set 8 : #
Total comments: 2
Patch Set 9 : #
Total comments: 22
Patch Set 10 : #Patch Set 11 : #
Total comments: 6
Patch Set 12 : #Patch Set 13 : #
Total comments: 24
Patch Set 14 : #
Total comments: 4
Patch Set 15 : #Patch Set 16 : #
Total comments: 4
Patch Set 17 : #
Total comments: 2
Patch Set 18 : #
Total comments: 4
Patch Set 19 : #Patch Set 20 : #Messages
Total messages: 41 (0 generated)
https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... File content/browser/devtools/devtools_protocol.h (right): https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/devtools_protocol.h:123: class CONTENT_EXPORT Handler { Why is this necessary? For tests? https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... File content/browser/devtools/renderer_overrides_handler.cc (right): https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/renderer_overrides_handler.cc:398: namespace { This namespace is 200 lines of very task-specific and non-trivial code. I suggest extracting it into a separate file. https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/renderer_overrides_handler.cc:423: response_loop_proxy_(base::MessageLoopProxy::current()) { Looks a bit over-engineered. I assume that the caller is running on some well-known thread (UI?), why not use it? https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/renderer_overrides_handler.cc:449: void ResponseOnCallerThread(scoped_ptr<base::DictionaryValue> response) { RespondOnCallerThread is better English https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/renderer_overrides_handler.cc:453: static void ResponseOnCallerThreadAndDelete(QuotaTask* task, RespondOnCallerThreadAndDelete is better English https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/renderer_overrides_handler.cc:474: readyToNextStep_ = true; readyForNextStep https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/renderer_overrides_handler.cc:493: switch (step_) { This switch makes me sad. Please consider replacing it with an array of callbacks. https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/renderer_overrides_handler.cc:613: &RendererOverridesHandler::AsyncTaskDone, base::Unretained(this)); base::Unretained allows this to be deleted before QuotaTask uses the callback. I suggest using weak pointer (see weak_factory_ usages in this file). https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... File content/browser/devtools/renderer_overrides_handler_browsertest.cc (right): https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/renderer_overrides_handler_browsertest.cc:58: EXPECT_TRUE(MessageContains("\"temporaryGlobalQuota\"")); Duplicate line? https://chromiumcodereview.appspot.com/23240002/diff/42001/webkit/browser/fil... File webkit/browser/fileapi/file_system_context.cc (right): https://chromiumcodereview.appspot.com/23240002/diff/42001/webkit/browser/fil... webkit/browser/fileapi/file_system_context.cc:133: if (quota_manager_proxy) { You made this change because the order apparently matters. Please comment why it matters so that someone does not break this later. https://chromiumcodereview.appspot.com/23240002/diff/42001/webkit/browser/quo... File webkit/browser/quota/usage_tracker.cc (right): https://chromiumcodereview.appspot.com/23240002/diff/42001/webkit/browser/quo... webkit/browser/quota/usage_tracker.cc:81: if ((*iter)->DoesSupport(type)) { Why do we need this method? Deserves a comment here or at the base class declaration.
I extracted a separate patch for things related to ClientUsageTrackers: https://codereview.chromium.org/23545016/. This code temporarily left in this patch as well to keep it compilable and runnable. So please don't review it here. https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... File content/browser/devtools/devtools_protocol.h (right): https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/devtools_protocol.h:123: class CONTENT_EXPORT Handler { On 2013/08/28 10:46:14, Vladislav Kaznacheev wrote: > Why is this necessary? For tests? Yes, otherwise content_browsertests fails to build in Windows. https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... File content/browser/devtools/renderer_overrides_handler.cc (right): https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/renderer_overrides_handler.cc:398: namespace { On 2013/08/28 10:46:14, Vladislav Kaznacheev wrote: > This namespace is 200 lines of very task-specific and non-trivial code. I > suggest extracting it into a separate file. Only 60 likes are quite generic (QuotaTask), 120 lines is about single DevToolsProtocol message. And even that 60 lines not such generic to be useful outside of this file. I think this file is a good place for this code. https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/renderer_overrides_handler.cc:423: response_loop_proxy_(base::MessageLoopProxy::current()) { On 2013/08/28 10:46:14, Vladislav Kaznacheev wrote: > Looks a bit over-engineered. I assume that the caller is running on some > well-known thread (UI?), why not use it? I'm a bit hesitate to introduce first thread dependency in the file. We don't have unit tests for this code but if we would it makes testing herder. And by the way quota::QuotaTask uses the same approach (likely because QuotaManager does have unit tests. It worth only 2 additional lines of code (declaration and initialization response_loop_proxy_). https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/renderer_overrides_handler.cc:449: void ResponseOnCallerThread(scoped_ptr<base::DictionaryValue> response) { On 2013/08/28 10:46:14, Vladislav Kaznacheev wrote: > RespondOnCallerThread is better English Done. https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/renderer_overrides_handler.cc:453: static void ResponseOnCallerThreadAndDelete(QuotaTask* task, On 2013/08/28 10:46:14, Vladislav Kaznacheev wrote: > RespondOnCallerThreadAndDelete is better English Done. https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/renderer_overrides_handler.cc:474: readyToNextStep_ = true; On 2013/08/28 10:46:14, Vladislav Kaznacheev wrote: > readyForNextStep Done. https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/renderer_overrides_handler.cc:493: switch (step_) { On 2013/08/28 10:46:14, Vladislav Kaznacheev wrote: > This switch makes me sad. Please consider replacing it with an array of > callbacks. Done. https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/renderer_overrides_handler.cc:613: &RendererOverridesHandler::AsyncTaskDone, base::Unretained(this)); On 2013/08/28 10:46:14, Vladislav Kaznacheev wrote: > base::Unretained allows this to be deleted before QuotaTask uses the callback. I > suggest using weak pointer (see weak_factory_ usages in this file). Good catch. Thanks. https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... File content/browser/devtools/renderer_overrides_handler_browsertest.cc (right): https://chromiumcodereview.appspot.com/23240002/diff/42001/content/browser/de... content/browser/devtools/renderer_overrides_handler_browsertest.cc:58: EXPECT_TRUE(MessageContains("\"temporaryGlobalQuota\"")); On 2013/08/28 10:46:14, Vladislav Kaznacheev wrote: > Duplicate line? Done.
Merged with https://codereview.chromium.org/23545016/. Vlad, please review.
https://chromiumcodereview.appspot.com/23240002/diff/72001/content/browser/de... File content/browser/devtools/renderer_overrides_handler.cc (right): https://chromiumcodereview.appspot.com/23240002/diff/72001/content/browser/de... content/browser/devtools/renderer_overrides_handler.cc:490: void Done(scoped_ptr<base::DictionaryValue> responseData) { Why are these methods public? They are only used by the derived class. https://chromiumcodereview.appspot.com/23240002/diff/72001/content/browser/de... File content/browser/devtools/renderer_overrides_handler_browsertest.cc (right): https://chromiumcodereview.appspot.com/23240002/diff/72001/content/browser/de... content/browser/devtools/renderer_overrides_handler_browsertest.cc:58: EXPECT_TRUE(MessageContains("\"temporaryGlobalQuota\"")); Duplicate line
https://chromiumcodereview.appspot.com/23240002/diff/72001/content/browser/de... File content/browser/devtools/renderer_overrides_handler.cc (right): https://chromiumcodereview.appspot.com/23240002/diff/72001/content/browser/de... content/browser/devtools/renderer_overrides_handler.cc:490: void Done(scoped_ptr<base::DictionaryValue> responseData) { On 2013/09/05 12:46:10, Vladislav Kaznacheev wrote: > Why are these methods public? They are only used by the derived class. It is protected, not public (see line before the constrictor). https://chromiumcodereview.appspot.com/23240002/diff/72001/content/browser/de... File content/browser/devtools/renderer_overrides_handler_browsertest.cc (right): https://chromiumcodereview.appspot.com/23240002/diff/72001/content/browser/de... content/browser/devtools/renderer_overrides_handler_browsertest.cc:58: EXPECT_TRUE(MessageContains("\"temporaryGlobalQuota\"")); On 2013/09/05 12:46:10, Vladislav Kaznacheev wrote: > Duplicate line Sory. Lost this change.
LGTM. Please include pfeldman@
Drive-by comment. https://codereview.chromium.org/23240002/diff/81001/content/browser/devtools/... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/23240002/diff/81001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:538: MakeQuotaCallback("temporaryGlobalQuota"))); The real per-site temporary quota we give to each site is different from global quota. I think it'd be probably useful to also show the actual quota value given to the app? It can be obtained by QuotaManager::GetUsageAndQuotaForWebApps().
https://codereview.chromium.org/23240002/diff/81001/content/browser/devtools/... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/23240002/diff/81001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:538: MakeQuotaCallback("temporaryGlobalQuota"))); On 2013/09/09 12:36:35, kinuko wrote: > The real per-site temporary quota we give to each site is different from global > quota. I think it'd be probably useful to also show the actual quota value given > to the app? It can be obtained by QuotaManager::GetUsageAndQuotaForWebApps(). Done.
https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... File content/browser/devtools/devtools_protocol_constants.cc (right): https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/devtools_protocol_constants.cc:121: const char kName[] = "Quota.queryUsageAndQuota"; Lets define this on Page domain. https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:533: namespace { We already have anonymous namespace defined above. https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:550: const RespondCallback& callback, poor indentation. https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:566: base::MessageLoopProxy* loop = response_loop_proxy_.get(); You should not define methods inline. https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:599: class GetUsageAndQuotaTask : public QuotaTask { There is only one task. Why extracting superclass? https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:602: const RespondCallback& callback, indent https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:606: quota_(new base::DictionaryValue), indent https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:608: steps_.push_back(base::Bind("a::QuotaManager::GetAvailableSpace, You should use barrier. https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:629: "temporaryFileSystemUsage")); You should have a dictionary per storage type and loop through values. https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:698: DCHECK(readyForNextStep_); Please add DCHECK that it is currently on IO
https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... File content/browser/devtools/devtools_protocol_constants.cc (right): https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/devtools_protocol_constants.cc:121: const char kName[] = "Quota.queryUsageAndQuota"; On 2013/09/16 14:15:36, pfeldman wrote: > Lets define this on Page domain. Done. https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:533: namespace { On 2013/09/16 14:15:36, pfeldman wrote: > We already have anonymous namespace defined above. Moved there. https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:550: const RespondCallback& callback, On 2013/09/16 14:15:36, pfeldman wrote: > poor indentation. Done. https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:566: base::MessageLoopProxy* loop = response_loop_proxy_.get(); On 2013/09/16 14:15:36, pfeldman wrote: > You should not define methods inline. Done. https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:599: class GetUsageAndQuotaTask : public QuotaTask { On 2013/09/16 14:15:36, pfeldman wrote: > There is only one task. Why extracting superclass? Initially I planned to implement more methods based on QuotaTask. Then decided to leave it as is because it provides separation on two levels of abstraction: infrastructure for execution on IO thread (and now I moved step-by-step execution there) and second once for higher level code what extracts data and packs it to send back. I don't insist on this splitting though. https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:602: const RespondCallback& callback, On 2013/09/16 14:15:36, pfeldman wrote: > indent Done. https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:606: quota_(new base::DictionaryValue), On 2013/09/16 14:15:36, pfeldman wrote: > indent Done. https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:608: steps_.push_back(base::Bind("a::QuotaManager::GetAvailableSpace, On 2013/09/16 14:15:36, pfeldman wrote: > You should use barrier. I had a look in base/barrier_closure.h and don't see how it can make this code any better. It better suit for starting several async tasks ans waiting for results rather than executing several tasks sequentially. https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:629: "temporaryFileSystemUsage")); On 2013/09/16 14:15:36, pfeldman wrote: > You should have a dictionary per storage type and loop through values. I employed different approach: introduced a method and moved repeating lines there. https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:698: DCHECK(readyForNextStep_); On 2013/09/16 14:15:36, pfeldman wrote: > Please add DCHECK that it is currently on IO Done.
https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:608: steps_.push_back(base::Bind("a::QuotaManager::GetAvailableSpace, > I had a look in base/barrier_closure.h and don't see how it can make this code > any better. It better suit for starting several async tasks ans waiting for > results rather than executing several tasks sequentially. I am not sure I follow.
Made with BarrierClosure
https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/23240002/diff/91001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:599: class GetUsageAndQuotaTask : public QuotaTask { I think it is now clear you should not split these. https://codereview.chromium.org/23240002/diff/109001/content/browser/devtools... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/23240002/diff/109001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:94: scoped_refptr<quota::QuotaManager> GetQuotaManager() { These will not be needed. https://codereview.chromium.org/23240002/diff/109001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:611: } // QuotaTask ------------------------------------------ // static ... https://codereview.chromium.org/23240002/diff/109001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:736: AddStep(base::Bind(&GetUsageAndQuotaTask::GetHostUsage, Why do you need to collect steps and not simply run them?
https://codereview.chromium.org/23240002/diff/109001/content/browser/devtools... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/23240002/diff/109001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:94: scoped_refptr<quota::QuotaManager> GetQuotaManager() { On 2013/09/17 15:19:45, pfeldman wrote: > These will not be needed. Done. https://codereview.chromium.org/23240002/diff/109001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:611: On 2013/09/17 15:19:45, pfeldman wrote: > } > > > // QuotaTask ------------------------------------------ > > // static > ... Done. https://codereview.chromium.org/23240002/diff/109001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:736: AddStep(base::Bind(&GetUsageAndQuotaTask::GetHostUsage, On 2013/09/17 15:19:45, pfeldman wrote: > Why do you need to collect steps and not simply run them? BarrierClosure needs to be created before running any of them (since step may finish synchronously) and it needs number of steps.
Rewritten in functional style as we discussed with Pavel.
https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:542: static void QueryAndUsageQuotaCompletedOnIOThread( QueryUsageAndQuotaCompletedOnIOThread https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:542: static void QueryAndUsageQuotaCompletedOnIOThread( Please move into anonymous namespace. https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:548: response_data->Set("usage", usage->DeepCopy()); Copying usage and quota leaks them, you should set originals. https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:553: base::Bind( Does this fit 80 chars? https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:572: base::DictionaryValue* usage; why not scoped_ptr? https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:578: static void GetHostUsage( Please move these into anonymous namespace. https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:625: base::Closure barrier = BarrierClosure(kExpectedResults, on_complete); Just inline on_complete. https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:634: base::Bind( No need to put them into columns according to the new code style. https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:642: base::Bind( ditto https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:650: GetHostUsage( Following code does the same and does not require params, defining GetHostUsageCommonParams and GetHostUsage. quota_manager->GetHostUsage(host, quota::kStorageTypeTemporary, quota::QuotaClient::kFileSystem, base::Bind(&DidGetHostUsage, "temporaryFileSystemUsage", usage, barrier)); https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:708: // Will be deleted when done. What will be deleted when done? https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:723: SendRawMessage( SendAsyncResponse(command->SuccessResponse(response_data.release()))
https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:542: static void QueryAndUsageQuotaCompletedOnIOThread( On 2013/09/19 12:35:49, pfeldman wrote: > QueryUsageAndQuotaCompletedOnIOThread Done. https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:542: static void QueryAndUsageQuotaCompletedOnIOThread( On 2013/09/19 12:35:49, pfeldman wrote: > Please move into anonymous namespace. Done. https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:548: response_data->Set("usage", usage->DeepCopy()); On 2013/09/19 12:35:49, pfeldman wrote: > Copying usage and quota leaks them, you should set originals. They don't leak because of using base::Owned (base/callback.h states: "The object will be deleted when the callback is destroyed"). https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:553: base::Bind( On 2013/09/19 12:35:49, pfeldman wrote: > Does this fit 80 chars? Done. https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:572: base::DictionaryValue* usage; On 2013/09/19 12:35:49, pfeldman wrote: > why not scoped_ptr? Because this object is owned by on_complete callback (now anonymous barrier's closure). https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:578: static void GetHostUsage( On 2013/09/19 12:35:49, pfeldman wrote: > Please move these into anonymous namespace. Done. https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:625: base::Closure barrier = BarrierClosure(kExpectedResults, on_complete); On 2013/09/19 12:35:49, pfeldman wrote: > Just inline on_complete. Done. https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:634: base::Bind( On 2013/09/19 12:35:49, pfeldman wrote: > No need to put them into columns according to the new code style. Done. https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:642: base::Bind( On 2013/09/19 12:35:49, pfeldman wrote: > ditto Done. https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:650: GetHostUsage( On 2013/09/19 12:35:49, pfeldman wrote: > Following code does the same and does not require params, defining > GetHostUsageCommonParams and GetHostUsage. > > quota_manager->GetHostUsage(host, quota::kStorageTypeTemporary, > quota::QuotaClient::kFileSystem, > base::Bind(&DidGetHostUsage, > "temporaryFileSystemUsage", > usage, barrier)); Done. https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:708: // Will be deleted when done. On 2013/09/19 12:35:49, pfeldman wrote: > What will be deleted when done? Removed. https://codereview.chromium.org/23240002/diff/126001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:723: SendRawMessage( On 2013/09/19 12:35:49, pfeldman wrote: > SendAsyncResponse(command->SuccessResponse(response_data.release())) Done.
Looks good. Could you share the protocol.json for this change? https://codereview.chromium.org/23240002/diff/138001/content/browser/devtools... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/23240002/diff/138001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:602: base::Owned(usage), Then you could remove base::Owned and get rid of the DeepCopy. https://codereview.chromium.org/23240002/diff/138001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:623: "temporaryFileSystemUsage", Could you share a protocol.json? If it mentions these, you would need constants for them in devtools_protocol_constants.
https://codereview.chromium.org/23240002/diff/138001/content/browser/devtools... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/23240002/diff/138001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:602: base::Owned(usage), On 2013/09/19 15:15:34, pfeldman wrote: > Then you could remove base::Owned and get rid of the DeepCopy. Done. https://codereview.chromium.org/23240002/diff/138001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:623: "temporaryFileSystemUsage", On 2013/09/19 15:15:34, pfeldman wrote: > Could you share a protocol.json? If it mentions these, you would need constants > for them in devtools_protocol_constants. https://codereview.chromium.org/23264011/diff/4001/Source/devtools/protocol.json
Adjusted to new protocol.json.
https://codereview.chromium.org/23240002/diff/77001/content/browser/devtools/... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/23240002/diff/77001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:576: usage_item->SetString(devtools::Page::UsageItem::kItemID, client_id); You could attach usage lists lazily and get rid of manual creation / lots of code below: base::ListValue* usage_list; if (!usage->GetList(usage_type, &usage_list)) { usage_list = new base::ListValue(); usage->Set(usage_type, usage_list); } https://codereview.chromium.org/23240002/diff/77001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:609: base::ListValue* temporaryStorageUsage = new base::ListValue; You should create them as scoped_ptr<>s
Also, why do you hard-code possible pairs of client ids and types? You should go through all of them in a loop and dump all possible combinations (while respecting DoesSupport). That way this code will be implemented once and for all.
https://codereview.chromium.org/23240002/diff/77001/content/browser/devtools/... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/23240002/diff/77001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:576: usage_item->SetString(devtools::Page::UsageItem::kItemID, client_id); On 2013/09/20 13:26:25, pfeldman wrote: > You could attach usage lists lazily and get rid of manual creation / lots of > code below: > > base::ListValue* usage_list; > if (!usage->GetList(usage_type, &usage_list)) { > usage_list = new base::ListValue(); > usage->Set(usage_type, usage_list); > } Now I create in a loop. https://codereview.chromium.org/23240002/diff/77001/content/browser/devtools/... content/browser/devtools/renderer_overrides_handler.cc:609: base::ListValue* temporaryStorageUsage = new base::ListValue; On 2013/09/20 13:26:25, pfeldman wrote: > You should create them as scoped_ptr<>s Done.
lgtm https://codereview.chromium.org/23240002/diff/165001/content/browser/devtools... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/23240002/diff/165001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:616: static const quota::StorageType kStorageTypes[] = { You should instead iterate over StorageType enum (via adding a last item into that enum). Then you introduce static std::string GetStorageTypeName(StorageType) { } above.
https://codereview.chromium.org/23240002/diff/165001/content/browser/devtools... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/23240002/diff/165001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:616: static const quota::StorageType kStorageTypes[] = { On 2013/09/23 10:26:03, pfeldman wrote: > You should instead iterate over StorageType enum (via adding a last item into > that enum). Then you introduce > > static std::string GetStorageTypeName(StorageType) { > } > > above. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/serya@chromium.org/23240002/58001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
On 2013/09/23 12:10:55, I haz the power (commit-bot) wrote: > Retried try job too often on chromium_presubmit for step(s) presubmit > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... You'll need more lgtm's...
webkit/browser/quota lgtm + some nits (I think you can TBR for one-line addition in content/content_tests.gypi) https://codereview.chromium.org/23240002/diff/58001/webkit/browser/quota/quot... File webkit/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/23240002/diff/58001/webkit/browser/quota/quot... webkit/browser/quota/quota_manager.cc:1095: return GetUsageTracker(type)->GetClientTracker(client_id) != 0; We're in chromium but not in blink, can we use NULL for pointers instead of 0? (Or just !!GetUsageTracker(type)->GetClientTracker(..) should work) https://codereview.chromium.org/23240002/diff/58001/webkit/browser/quota/quot... File webkit/browser/quota/quota_manager.h (right): https://codereview.chromium.org/23240002/diff/58001/webkit/browser/quota/quot... webkit/browser/quota/quota_manager.h:208: QuotaClient::ID client_id, const UsageCallback& callback); style-nit: please break lines after 'client_id,' (host and type can be viewed as 'pair', but others are not)
https://codereview.chromium.org/23240002/diff/58001/webkit/browser/quota/quot... File webkit/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/23240002/diff/58001/webkit/browser/quota/quot... webkit/browser/quota/quota_manager.cc:1095: return GetUsageTracker(type)->GetClientTracker(client_id) != 0; On 2013/09/23 21:41:05, kinuko wrote: > We're in chromium but not in blink, can we use NULL for pointers instead of 0? > (Or just !!GetUsageTracker(type)->GetClientTracker(..) should work) Done. https://codereview.chromium.org/23240002/diff/58001/webkit/browser/quota/quot... File webkit/browser/quota/quota_manager.h (right): https://codereview.chromium.org/23240002/diff/58001/webkit/browser/quota/quot... webkit/browser/quota/quota_manager.h:208: QuotaClient::ID client_id, const UsageCallback& callback); On 2013/09/23 21:41:05, kinuko wrote: > style-nit: please break lines after 'client_id,' (host and type can be viewed as > 'pair', but others are not) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/serya@chromium.org/23240002/181001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/serya@chromium.org/23240002/204001
Retried try job too often on linux_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/serya@chromium.org/23240002/204001
Retried try job too often on linux_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/serya@chromium.org/23240002/204001
Retried try job too often on linux_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/serya@chromium.org/23240002/204001
Message was sent while issue was closed.
Change committed as 225448 |
