Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2107173002:
Add the future storage framework for the BudgetDatabase. (Closed)
DescriptionAdd the future storage framework for the BudgetDatabase. This isn't
currently plugged into the BackgroundBudgetService, but will be in future
work.
It will be able to store objects that fit the Budget protobuf type, which
has pieces of budget that have been allocated, an expiration time for those
pieces, and how much (if any) in debt a service worker is.
BUG=617971
Committed: https://crrev.com/883658b07cc779484d846289669570b54af5e7f3
Cr-Commit-Position: refs/heads/master@{#405975}
Patch Set 1 #
Total comments: 30
Patch Set 2 : Code review feedback and added histograms.xml update. #
Total comments: 16
Patch Set 3 : Update namespace in test file #
Total comments: 2
Patch Set 4 : Code review comments #Patch Set 5 : Rebase #
Total comments: 18
Patch Set 6 : nits and fix path error #
Messages
Total messages: 48 (21 generated)
Description was changed from ========== Add the future storage framework for the BudgetDatabase. This isn't currently plugged into the BackgroundBudgetService, but will be in future work. It will be able to store objects that fit the Budget protobuf type, which has pieces of budget that have been allocated, an expiration time for those pieces, and how much (if any) in debt a service worker is. BUG=617971 ========== to ========== Add the future storage framework for the BudgetDatabase. This isn't currently plugged into the BackgroundBudgetService, but will be in future work. It will be able to store objects that fit the Budget protobuf type, which has pieces of budget that have been allocated, an expiration time for those pieces, and how much (if any) in debt a service worker is. BUG=617971 ==========
harkness@chromium.org changed reviewers: + peter@chromium.org
https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/budget.proto (right): https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget.proto:7: package chrome_browser_budget_service; package budget_service; https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget.proto:9: // Chrome requires this. What is the added value of this comment? https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget.proto:12: // Description of budget chuks. What is the added value of this comment? https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget.proto:12: // Description of budget chuks. Please annotate messages with the next available ID, e.g.: // Next available id: 3. https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget.proto:26: // The timestamp when the budget expires. It would be good to document what format the timestamp is in— why double and not int64? (e.g. Time::ToInternalValue()) https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.cc:8: #include "components/leveldb_proto/proto_database.h" nit: you already include this in the header file, no need to include it twice https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.cc:16: const char kDatabaseUMAName[] = "BackgroundBudgetService"; nit: blank lines after lines 12 and 16. https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.cc:32: // TODO(harkness): Consider caching the budget database now? Caching is an interesting question. A developer could hypothetically flood disk I/O now by just getting their budget value over and over again. Do you have thoughts about this? https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.cc:37: db_->GetEntry(origin.spec(), callback); DCHECK_EQ(origin.GetOrigin(), origin); (Also on line 43. Best to be pedantic when it comes down to persistent on-disk storage.) https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/budget_database.h (right): https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.h:21: using chrome_browser_budget_service::Budget; `using` declarations in headers are forbidden by our style guide. In either case, this shouldn't be a problem anymore when you rename it to budget_service::Budget :) https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.h:23: class BudgetDatabase { ++docs (why does it do what it does, why does it need the |task_runner|, ...) https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.h:36: void OnDatabaseInit(bool success); This should be private. https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database_unittest.cc:85: bool success_; |thread_bundle_| should be private. |success_| should be protected. https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database_unittest.cc:102: } What is your plan with this class? Will there be other unit tests? You have generalized a lot of logic in the BudgetDatabaseTest class now, while there's only one test using it. That's something we usually only do for tests when there are several. https://codereview.chromium.org/2107173002/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/2107173002/diff/1/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:4093: # protocol buffer. micro nit: I think this fits on one line?
https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/budget.proto (right): https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget.proto:7: package chrome_browser_budget_service; On 2016/06/29 13:37:27, Peter Beverloo wrote: > package budget_service; I much prefer that, but it's not the pattern I see used in other components of chrome/browser. Looking again, it's half and half, so I'll go with the shorter one! https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget.proto:9: // Chrome requires this. On 2016/06/29 13:37:27, Peter Beverloo wrote: > What is the added value of this comment? The first proto I looked at didn't have the comment, so I didn't include it, planning to investigate it later. Of course, when I built later, I'd forgotten that and it took me a while to see why my build was failing. Looking at other protos, many of them include the comment, which would have saved me time if it had been in the original file. https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget.proto:12: // Description of budget chuks. On 2016/06/29 13:37:27, Peter Beverloo wrote: > Please annotate messages with the next available ID, e.g.: > > // Next available id: 3. Done. https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget.proto:12: // Description of budget chuks. On 2016/06/29 13:37:27, Peter Beverloo wrote: > What is the added value of this comment? That was commenting v1, deprecated by v2, but I forgot to remove it. Done now. https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget.proto:26: // The timestamp when the budget expires. On 2016/06/29 13:37:27, Peter Beverloo wrote: > It would be good to document what format the timestamp is in— why double and not > int64? (e.g. Time::ToInternalValue()) Added a comment. I think I'm going to want to change it to int once we're not using the site engagement timestamps. https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.cc:8: #include "components/leveldb_proto/proto_database.h" On 2016/06/29 13:37:27, Peter Beverloo wrote: > nit: you already include this in the header file, no need to include it twice Acknowledged. https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.cc:16: const char kDatabaseUMAName[] = "BackgroundBudgetService"; On 2016/06/29 13:37:27, Peter Beverloo wrote: > nit: blank lines after lines 12 and 16. Done. https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.cc:32: // TODO(harkness): Consider caching the budget database now? On 2016/06/29 13:37:27, Peter Beverloo wrote: > Caching is an interesting question. A developer could hypothetically flood disk > I/O now by just getting their budget value over and over again. Do you have > thoughts about this? I need to dig into the implementation of ProtoDatabase a bit more. When we were talking earlier in the week, you mentioned that the implementation read everything into memory on the first read, but I haven't found that happening in the code. If the underlying implementation doesn't have a cache, I want to add one in BudgetDatabase, which is the reason I've got the TODO in there. https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.cc:37: db_->GetEntry(origin.spec(), callback); On 2016/06/29 13:37:27, Peter Beverloo wrote: > DCHECK_EQ(origin.GetOrigin(), origin); > > (Also on line 43. Best to be pedantic when it comes down to persistent on-disk > storage.) Done. https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/budget_database.h (right): https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.h:21: using chrome_browser_budget_service::Budget; On 2016/06/29 13:37:27, Peter Beverloo wrote: > `using` declarations in headers are forbidden by our style guide. In either > case, this shouldn't be a problem anymore when you rename it to > budget_service::Budget :) Yes, the package names was truly horrible. https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.h:23: class BudgetDatabase { On 2016/06/29 13:37:28, Peter Beverloo wrote: > ++docs (why does it do what it does, why does it need the |task_runner|, ...) Done. https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.h:36: void OnDatabaseInit(bool success); On 2016/06/29 13:37:27, Peter Beverloo wrote: > This should be private. Done. https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database_unittest.cc:85: bool success_; On 2016/06/29 13:37:28, Peter Beverloo wrote: > |thread_bundle_| should be private. > |success_| should be protected. Done. https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database_unittest.cc:102: } On 2016/06/29 13:37:28, Peter Beverloo wrote: > What is your plan with this class? Will there be other unit tests? > > You have generalized a lot of logic in the BudgetDatabaseTest class now, while > there's only one test using it. That's something we usually only do for tests > when there are several. I'm assuming there will be at least another 3-4 tests. For instance, I will need to add budget to origins. Right now, the caller would have to read the budget, append another BudgetChunk, then write it back. I'm planning to add a method on the BudgetDatabase which would wrap that, and then I'd need a test for that. I'm also going to need to do a periodic cleanup, which I'm planning to put on the BudgetDatabase, etc. https://codereview.chromium.org/2107173002/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/2107173002/diff/1/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:4093: # protocol buffer. On 2016/06/29 13:37:28, Peter Beverloo wrote: > micro nit: I think this fits on one line? Done.
The CQ bit was checked by harkness@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by harkness@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/06/30 10:41:58, harkness wrote: > https://codereview.chromium.org/2107173002/diff/1/chrome/browser/budget_servi... > chrome/browser/budget_service/budget.proto:26: // The timestamp when the budget > expires. > On 2016/06/29 13:37:27, Peter Beverloo wrote: > > It would be good to document what format the timestamp is in— why double and > not > > int64? (e.g. Time::ToInternalValue()) > > Added a comment. I think I'm going to want to change it to int once we're not > using the site engagement timestamps. Sure, but keep in mind that we lose this ability the second we hook up the database with production code! https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget.proto (right): https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget.proto:27: // The timestamp when the budget expires. Format is double to integrate with Yes, but what does the `double` contain? Milliseconds since the UNIX epoch? Something else? https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:54: db_->UpdateEntries(std::move(entries), std::move(keys_to_remove), callback); micro nit: blank lines to separate the logical sections in this method could reduce the "blob of code" feeling. https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.h (right): https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:22: // assigned to service workers. The class uses an underlying levelDB. service workers -> origins? Nothing in this class is tied to a particular Service Worker afaict. https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:38: using SetValueCallback = base::Callback<void(bool success)>; micro nit: sorry for missing this earlier— types before methods. https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:46: void OnDatabaseInit(bool success); optional bikeshedding to consider: DidInitializeDatabase? It's not an event we get from something we observe, but rather the response in reaction to something we trigger (requesting the database to initialize). https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:33: db_(profile_.GetPath().Append(FILE_PATH_LITERAL(kDatabaseFileDir)), A constant is not a literal :-). You use FILE_PATH_LITERAL *only* with actual C strings, because it'll expand the string at compile-time on Windows. Just inline "BudgetDatabase" here instead of using a constant. https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:43: budget_service::Budget b; nit re: b, bc1, bc2: please don't use such acronyms in Chromium, even in testing code. (Also on line 93.) Silly as may be, we're trying hard to stay consistent here, which already is tricky with a large code base (as you found out with the protobuf's namespace name!) https://codereview.chromium.org/2107173002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2107173002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:95734: + label="Databases for storing budget information for service workers."/> service workers -> origins https://codereview.chromium.org/2107173002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2107173002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:91: } // namespace Why did you move this? (The entire file can remain in the anonymous namespace.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget.proto (right): https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget.proto:27: // The timestamp when the budget expires. Format is double to integrate with On 2016/06/30 13:12:46, Peter Beverloo wrote: > Yes, but what does the `double` contain? Milliseconds since the UNIX epoch? > Something else? Done. https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:54: db_->UpdateEntries(std::move(entries), std::move(keys_to_remove), callback); On 2016/06/30 13:12:46, Peter Beverloo wrote: > micro nit: blank lines to separate the logical sections in this method could > reduce the "blob of code" feeling. Good point. https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.h (right): https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:22: // assigned to service workers. The class uses an underlying levelDB. On 2016/06/30 13:12:46, Peter Beverloo wrote: > service workers -> origins? Nothing in this class is tied to a particular > Service Worker afaict. Good point. I tend to think of this applying to service workers, because that's our particular use case, but it could be more generic. https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:38: using SetValueCallback = base::Callback<void(bool success)>; On 2016/06/30 13:12:46, Peter Beverloo wrote: > micro nit: sorry for missing this earlier— types before methods. > > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done. https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:46: void OnDatabaseInit(bool success); On 2016/06/30 13:12:46, Peter Beverloo wrote: > optional bikeshedding to consider: DidInitializeDatabase? > > It's not an event we get from something we observe, but rather the response in > reaction to something we trigger (requesting the database to initialize). I prefer the On* in this case, because I'm planning to do work there (likely initializing a cache) and that feels more like a response to an event. https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:33: db_(profile_.GetPath().Append(FILE_PATH_LITERAL(kDatabaseFileDir)), On 2016/06/30 13:12:46, Peter Beverloo wrote: > A constant is not a literal :-). You use FILE_PATH_LITERAL *only* with actual C > strings, because it'll expand the string at compile-time on Windows. > > Just inline "BudgetDatabase" here instead of using a constant. hah! That's what I get for trying to clean things up before sending them out for code review. Although it's odd that this only causes the try builds to fail, and not my local build. https://codereview.chromium.org/2107173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:43: budget_service::Budget b; On 2016/06/30 13:12:46, Peter Beverloo wrote: > nit re: b, bc1, bc2: please don't use such acronyms in Chromium, even in testing > code. (Also on line 93.) > > Silly as may be, we're trying hard to stay consistent here, which already is > tricky with a large code base (as you found out with the protobuf's namespace > name!) Done. https://codereview.chromium.org/2107173002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2107173002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:95734: + label="Databases for storing budget information for service workers."/> On 2016/06/30 13:12:46, Peter Beverloo wrote: > service workers -> origins Done. https://codereview.chromium.org/2107173002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2107173002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:91: } // namespace On 2016/06/30 13:12:46, Peter Beverloo wrote: > Why did you move this? (The entire file can remain in the anonymous namespace.) It was an experiment to fix the build failure, because I didn't see what was going wrong. I'll revert it.
The CQ bit was checked by harkness@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gyp_...)
The CQ bit was checked by harkness@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm % compile fix and nits Thank you! :) https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... File chrome/browser/budget_service/budget.proto (right): https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... chrome/browser/budget_service/budget.proto:28: // Site Engagement and Webkit and is the number of seconds since Jan 1, 1970. Webkit -> Blink https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.h (right): https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:11: #include "base/memory/weak_ptr.h" +base/macros.h for brownie points :) https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:12: #include "base/run_loop.h" unused https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:13: #include "base/sequenced_task_runner.h" forward declare? (it has a fairly heavy include-tree) https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:15: #include "url/gurl.h" forward declare? https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:22: // assigned to an origin. The class uses an underlying levelDB. levelDB -> LevelDB (product name) https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:30: db_(profile_.GetPath().Append("BudgetDabase"), db_(profile_.GetPath().Append(FILE_PATH_LITERAL("BudgetDabase")) Paths have platform-dependent storage: on Windows we store them using wchar_t[], on other platforms using char[]. base::FilePath's underlying storage reflects this. The FILE_PATH_LITERAL() macro manages this difference for you: it passes through the string on all platforms but Windows, where it prefixes the string with L (i.e. L"BudgetDatabase"). That is the wide string literal: https://msdn.microsoft.com/en-us/library/69ze775t.aspx ========================= char kMyString[] = "foo"; wchar_t kMyWideString[] = L"foo"; ========================= String literals must be used on.. actual strings, not variables— they are applied at compile-time, not run-time. The error you ran in to previously was caused by using a variable. The macro expanded it as follows: ========================= FILE_PATH_LITERAL("foo") -> L"foo" FILE_PATH_LITERAL(kMyString) -> LkMyString ========================= Which, of course, does not exist. https://codereview.chromium.org/2107173002/diff/80001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/2107173002/diff/80001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:122: 'browser/budget_service/budget_database.h', budget comes after background (alphabetize) https://codereview.chromium.org/2107173002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2107173002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:96084: + label="Databases for storing budget information for origins."/> Databases -> Database (singular, for consistency with other suffixes)
The CQ bit was checked by harkness@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by harkness@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... File chrome/browser/budget_service/budget.proto (right): https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... chrome/browser/budget_service/budget.proto:28: // Site Engagement and Webkit and is the number of seconds since Jan 1, 1970. On 2016/07/05 13:01:48, Peter Beverloo wrote: > Webkit -> Blink Done. https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.h (right): https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:11: #include "base/memory/weak_ptr.h" On 2016/07/05 13:01:48, Peter Beverloo wrote: > +base/macros.h for brownie points :) Done. https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:12: #include "base/run_loop.h" On 2016/07/05 13:01:48, Peter Beverloo wrote: > unused Done. https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:13: #include "base/sequenced_task_runner.h" On 2016/07/05 13:01:48, Peter Beverloo wrote: > forward declare? (it has a fairly heavy include-tree) Done. https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:15: #include "url/gurl.h" On 2016/07/05 13:01:48, Peter Beverloo wrote: > forward declare? Done. https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:22: // assigned to an origin. The class uses an underlying levelDB. On 2016/07/05 13:01:48, Peter Beverloo wrote: > levelDB -> LevelDB (product name) Done. https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2107173002/diff/80001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:30: db_(profile_.GetPath().Append("BudgetDabase"), On 2016/07/05 13:01:48, Peter Beverloo wrote: > db_(profile_.GetPath().Append(FILE_PATH_LITERAL("BudgetDabase")) > > > Paths have platform-dependent storage: on Windows we store them using wchar_t[], > on other platforms using char[]. base::FilePath's underlying storage reflects > this. > > The FILE_PATH_LITERAL() macro manages this difference for you: it passes through > the string on all platforms but Windows, where it prefixes the string with L > (i.e. L"BudgetDatabase"). That is the wide string literal: > > https://msdn.microsoft.com/en-us/library/69ze775t.aspx > > ========================= > char kMyString[] = "foo"; > wchar_t kMyWideString[] = L"foo"; > ========================= > > String literals must be used on.. actual strings, not variables— they are > applied at compile-time, not run-time. > > The error you ran in to previously was caused by using a variable. The macro > expanded it as follows: > > ========================= > FILE_PATH_LITERAL("foo") -> L"foo" > FILE_PATH_LITERAL(kMyString) -> LkMyString > ========================= > > Which, of course, does not exist. ahhh! Thanks for the explanation! https://codereview.chromium.org/2107173002/diff/80001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/2107173002/diff/80001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:122: 'browser/budget_service/budget_database.h', On 2016/07/05 13:01:49, Peter Beverloo wrote: > budget comes after background (alphabetize) Done. https://codereview.chromium.org/2107173002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2107173002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:96084: + label="Databases for storing budget information for origins."/> On 2016/07/05 13:01:49, Peter Beverloo wrote: > Databases -> Database (singular, for consistency with other suffixes) Done.
harkness@chromium.org changed reviewers: + holte@chromium.org, thestig@chromium.org
@holte, could you take a look at histograms.xml please? @thestig, could you take a look at the build file change please?
lgtm
lgtm
The CQ bit was checked by harkness@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/2107173002/#ps100001 (title: "nits and fix path error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by harkness@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add the future storage framework for the BudgetDatabase. This isn't currently plugged into the BackgroundBudgetService, but will be in future work. It will be able to store objects that fit the Budget protobuf type, which has pieces of budget that have been allocated, an expiration time for those pieces, and how much (if any) in debt a service worker is. BUG=617971 ========== to ========== Add the future storage framework for the BudgetDatabase. This isn't currently plugged into the BackgroundBudgetService, but will be in future work. It will be able to store objects that fit the Budget protobuf type, which has pieces of budget that have been allocated, an expiration time for those pieces, and how much (if any) in debt a service worker is. BUG=617971 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add the future storage framework for the BudgetDatabase. This isn't currently plugged into the BackgroundBudgetService, but will be in future work. It will be able to store objects that fit the Budget protobuf type, which has pieces of budget that have been allocated, an expiration time for those pieces, and how much (if any) in debt a service worker is. BUG=617971 ========== to ========== Add the future storage framework for the BudgetDatabase. This isn't currently plugged into the BackgroundBudgetService, but will be in future work. It will be able to store objects that fit the Budget protobuf type, which has pieces of budget that have been allocated, an expiration time for those pieces, and how much (if any) in debt a service worker is. BUG=617971 Committed: https://crrev.com/883658b07cc779484d846289669570b54af5e7f3 Cr-Commit-Position: refs/heads/master@{#405975} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/883658b07cc779484d846289669570b54af5e7f3 Cr-Commit-Position: refs/heads/master@{#405975} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
