Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2901093009:
[USS] Implement GetAllData and GetStorageKey. (Closed)
DescriptionImplement GetAllData and GetStorageKey. Also add TypedURLSyncBridge to
be an observer of HistoryBackend.
Change storageKey using big endian, since user event is using big endian, and it is better to unify our number style.
BUG=726158
The following 4 functions are ported from TypedUrlSyncableService.
GetErrorPercentage()
WriteToTypedUrlSpecifics()
ClearErrorStats()
FixupURLAndGetVisits()
Review-Url: https://codereview.chromium.org/2901093009
Cr-Commit-Position: refs/heads/master@{#477079}
Committed: https://chromium.googlesource.com/chromium/src/+/c2ae2ba24bb3d8a1a1324bde1252dd543e2bf708
Patch Set 1 : add include #
Total comments: 14
Patch Set 2 : pavely code review update #
Total comments: 18
Patch Set 3 : pavely review 2 #
Total comments: 4
Patch Set 4 : add DCHECK #
Total comments: 12
Patch Set 5 : brettw review and rebase #Messages
Total messages: 65 (53 generated)
Description was changed from ========== first checkin BUG= ========== to ========== Implement GetAllData and GetStorageKey. BUG=726158 ==========
Description was changed from ========== Implement GetAllData and GetStorageKey. BUG=726158 ========== to ========== Implement GetAllData and GetStorageKey. BUG=726158 The following 4 functions are ported from TypedUrlSyncableService. GetErrorPercentage() WriteToTypedUrlSpecifics() ClearErrorStats() FixupURLAndGetVisits() ==========
gangwu@chromium.org changed reviewers: + pavely@chromium.org
Description was changed from ========== Implement GetAllData and GetStorageKey. BUG=726158 The following 4 functions are ported from TypedUrlSyncableService. GetErrorPercentage() WriteToTypedUrlSpecifics() ClearErrorStats() FixupURLAndGetVisits() ========== to ========== Implement GetAllData and GetStorageKey. Also add TypedURLSyncBridge to be an observer of HistoryBackend. BUG=726158 The following 4 functions are ported from TypedUrlSyncableService. GetErrorPercentage() WriteToTypedUrlSpecifics() ClearErrorStats() FixupURLAndGetVisits() ==========
The CQ bit was checked by gangwu@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 gangwu@chromium.org to run a CQ dry run
The CQ bit was checked by gangwu@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...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2901093009/diff/60001/components/history/core... File components/history/core/browser/history_backend.h (right): https://codereview.chromium.org/2901093009/diff/60001/components/history/core... components/history/core/browser/history_backend.h:581: friend class TypedURLSyncBridgeTest; I assume you make TypedURLSyncBridgeTest a friend just to give it ability to inject typed_url_sync_bridge_. Written this way it allows TypedURLSyncBridgeTest to call any private method and modify any private field of HistoryBackend. This makes it hard to form an expectation of the kinds of modifications test actually does. I think it is better to create public helper function for test that allows to inject typed_url_sync_bridge_. This way it is clear that test doesn't have any special privileges with HistoryBackend. https://codereview.chromium.org/2901093009/diff/60001/components/history/core... File components/history/core/browser/typed_url_sync_bridge.cc (right): https://codereview.chromium.org/2901093009/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:48: return base::Int64ToString(row.id()); Consider using base::WriteBigEndian(storage_key.data(), row.id()) It has a benefit of costing slightly less CPU for serializing/parsing. https://codereview.chromium.org/2901093009/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:55: TypedURLSyncMetadataDatabase* sync_metadata_store, Since you are changing the type doe it make sense to change variable name as well to sync_metadata_database? https://codereview.chromium.org/2901093009/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:86: history_backend_observer_.Add(history_backend_); Once you performed Merge and restarted, the Merge will not be called again. I think you still need to subscribe for events in this case. https://codereview.chromium.org/2901093009/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:113: "Could not get the typed_url entries.."); nit: .. => . https://codereview.chromium.org/2901093009/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:118: for (auto url : typed_urls) { What does "auto" resolve to? Does declaring url this way cause making a copy of URLRow to be passed to FixupURLAndGetVisits? If it does then you can potentially take a reference of element in typed_urls instead. https://codereview.chromium.org/2901093009/diff/60001/components/history/core... File components/history/core/browser/typed_url_sync_bridge.h (right): https://codereview.chromium.org/2901093009/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.h:84: // 2. No visits fpr the passed url, or all the visits are expired. fpr => for
Description was changed from ========== Implement GetAllData and GetStorageKey. Also add TypedURLSyncBridge to be an observer of HistoryBackend. BUG=726158 The following 4 functions are ported from TypedUrlSyncableService. GetErrorPercentage() WriteToTypedUrlSpecifics() ClearErrorStats() FixupURLAndGetVisits() ========== to ========== Implement GetAllData and GetStorageKey. Also add TypedURLSyncBridge to be an observer of HistoryBackend. Change storageKey using big endian, BUG=726158 The following 4 functions are ported from TypedUrlSyncableService. GetErrorPercentage() WriteToTypedUrlSpecifics() ClearErrorStats() FixupURLAndGetVisits() ==========
The CQ bit was checked by gangwu@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 checked by gangwu@chromium.org to run a CQ dry run
Patchset #2 (id:80001) has been deleted
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 gangwu@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 checked by gangwu@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...
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
updated, use big endian as storage key. https://codereview.chromium.org/2901093009/diff/60001/components/history/core... File components/history/core/browser/history_backend.h (right): https://codereview.chromium.org/2901093009/diff/60001/components/history/core... components/history/core/browser/history_backend.h:581: friend class TypedURLSyncBridgeTest; On 2017/05/30 14:25:40, pavely wrote: > I assume you make TypedURLSyncBridgeTest a friend just to give it ability to > inject typed_url_sync_bridge_. > Written this way it allows TypedURLSyncBridgeTest to call any private method and > modify any private field of HistoryBackend. This makes it hard to form an > expectation of the kinds of modifications test actually does. > I think it is better to create public helper function for test that allows to > inject typed_url_sync_bridge_. This way it is clear that test doesn't have any > special privileges with HistoryBackend. Done. https://codereview.chromium.org/2901093009/diff/60001/components/history/core... File components/history/core/browser/typed_url_sync_bridge.cc (right): https://codereview.chromium.org/2901093009/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:48: return base::Int64ToString(row.id()); On 2017/05/30 14:25:41, pavely wrote: > Consider using base::WriteBigEndian(storage_key.data(), row.id()) > It has a benefit of costing slightly less CPU for serializing/parsing. Done. https://codereview.chromium.org/2901093009/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:55: TypedURLSyncMetadataDatabase* sync_metadata_store, On 2017/05/30 14:25:41, pavely wrote: > Since you are changing the type doe it make sense to change variable name as > well to sync_metadata_database? Done. https://codereview.chromium.org/2901093009/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:86: history_backend_observer_.Add(history_backend_); On 2017/05/30 14:25:40, pavely wrote: > Once you performed Merge and restarted, the Merge will not be called again. I > think you still need to subscribe for events in this case. Done. https://codereview.chromium.org/2901093009/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:113: "Could not get the typed_url entries.."); On 2017/05/30 14:25:40, pavely wrote: > nit: .. => . Done. https://codereview.chromium.org/2901093009/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:118: for (auto url : typed_urls) { On 2017/05/30 14:25:41, pavely wrote: > What does "auto" resolve to? Does declaring url this way cause making a copy of > URLRow to be passed to FixupURLAndGetVisits? If it does then you can potentially > take a reference of element in typed_urls instead. Done. https://codereview.chromium.org/2901093009/diff/60001/components/history/core... File components/history/core/browser/typed_url_sync_bridge.h (right): https://codereview.chromium.org/2901093009/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.h:84: // 2. No visits fpr the passed url, or all the visits are expired. On 2017/05/30 14:25:41, pavely wrote: > fpr => for Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... File components/history/core/browser/history_backend.h (right): https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/history_backend.h:480: void SetTypedURLSyncBridge(std::unique_ptr<TypedURLSyncBridge> bridge) { Could you name it SetTypedURLSyncBridgeForTest. This is common pattern in Chromium. Makes usecases of this function obvious. https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/history_backend.h:481: typed_url_sync_bridge_.swap(bridge); I'd prefer "typed_url_sync_bridge_ = std::move(bridge);" It conveys the intention. Swap would make sense if you were planning to return old value. https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... File components/history/core/browser/typed_url_sync_bridge.cc (right): https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.cc:48: std::string storage_key(8, 0); If row.id() changes type or simply has different size on different platforms then allocating hardcoded 8 bytes could cause issues. Could you replace 8 with sizeof(). https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.cc:49: base::WriteBigEndian(&storage_key[0], row.id()); nit: You can use storage_key.data() instead of &storage_key[0]. https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... File components/history/core/browser/typed_url_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/typed_url_sync_bridge_unittest.cc:240: bool TypedURLSyncBridgeTest::WriteToTypedUrlSpecifics( You could have inlined this method along with the rest of TypedURLSyncBridgeTest methods. https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/typed_url_sync_bridge_unittest.cc:261: // Check that the local cache was is still correct. remove "was". https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:58: base::ReadBigEndian(storage_key.data(), &storage_key_int); When deserializing could you DCHECK that size of storage_key matches size of int that receives the value. https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:123: std::string storage_key(8, 0); Same comment as in typed_url_sync_bridge.cc https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database_unittest.cc (right): https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database_unittest.cc:26: base::WriteBigEndian(&storage_key[0], static_cast<URLID>(i)); nit: I think you can explicitly specify which variant of WriteBigEndian to use by providing template parameter: base::WriteBigEndian<URLID>(storage_key.data(), i);
The CQ bit was checked by gangwu@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 checked by gangwu@chromium.org to run a CQ dry run
Patchset #3 (id:160001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
updated https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... File components/history/core/browser/history_backend.h (right): https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/history_backend.h:480: void SetTypedURLSyncBridge(std::unique_ptr<TypedURLSyncBridge> bridge) { On 2017/06/01 20:49:18, pavely wrote: > Could you name it SetTypedURLSyncBridgeForTest. This is common pattern in > Chromium. Makes usecases of this function obvious. Done. https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/history_backend.h:481: typed_url_sync_bridge_.swap(bridge); On 2017/06/01 20:49:18, pavely wrote: > I'd prefer "typed_url_sync_bridge_ = std::move(bridge);" > It conveys the intention. Swap would make sense if you were planning to return > old value. Done. https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... File components/history/core/browser/typed_url_sync_bridge.cc (right): https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.cc:48: std::string storage_key(8, 0); On 2017/06/01 20:49:18, pavely wrote: > If row.id() changes type or simply has different size on different platforms > then allocating hardcoded 8 bytes could cause issues. Could you replace 8 with > sizeof(). Done. https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.cc:49: base::WriteBigEndian(&storage_key[0], row.id()); On 2017/06/01 20:49:18, pavely wrote: > nit: You can use storage_key.data() instead of &storage_key[0]. it won't work, I received the error message as following. "candidate function not viable: 1st argument ('const char *') would lose const qualifier" https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... File components/history/core/browser/typed_url_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/typed_url_sync_bridge_unittest.cc:240: bool TypedURLSyncBridgeTest::WriteToTypedUrlSpecifics( On 2017/06/01 20:49:18, pavely wrote: > You could have inlined this method along with the rest of TypedURLSyncBridgeTest > methods. Done. https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/typed_url_sync_bridge_unittest.cc:261: // Check that the local cache was is still correct. On 2017/06/01 20:49:18, pavely wrote: > remove "was". Done. https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:58: base::ReadBigEndian(storage_key.data(), &storage_key_int); On 2017/06/01 20:49:18, pavely wrote: > When deserializing could you DCHECK that size of storage_key matches size of int > that receives the value. Done. https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:123: std::string storage_key(8, 0); On 2017/06/01 20:49:18, pavely wrote: > Same comment as in typed_url_sync_bridge.cc Done. https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database_unittest.cc (right): https://codereview.chromium.org/2901093009/diff/140001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database_unittest.cc:26: base::WriteBigEndian(&storage_key[0], static_cast<URLID>(i)); On 2017/06/01 20:49:18, pavely wrote: > nit: I think you can explicitly specify which variant of WriteBigEndian to use > by providing template parameter: > base::WriteBigEndian<URLID>(storage_key.data(), i); Done.
lgtm % comment. https://codereview.chromium.org/2901093009/diff/180001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2901093009/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:59: base::ReadBigEndian(storage_key.data(), &storage_key_int); What I meant was to DCHECK that storage_key.size() matches size of storage_key_int. Imagine what would happen if storage_key contained 16 bytes instead of 8. https://codereview.chromium.org/2901093009/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:79: base::ReadBigEndian(storage_key.data(), &storage_key_int); Do the same check here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gangwu@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...
gangwu@chromium.org changed reviewers: + brettw@chromium.org
Hi Brettw@, PTAL https://codereview.chromium.org/2901093009/diff/180001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2901093009/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:59: base::ReadBigEndian(storage_key.data(), &storage_key_int); On 2017/06/02 00:33:06, pavely wrote: > What I meant was to DCHECK that storage_key.size() matches size of > storage_key_int. Imagine what would happen if storage_key contained 16 bytes > instead of 8. Done. https://codereview.chromium.org/2901093009/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:79: base::ReadBigEndian(storage_key.data(), &storage_key_int); On 2017/06/02 00:33:06, pavely wrote: > Do the same check here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... File components/history/core/browser/typed_url_sync_bridge.cc (right): https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.cc:71: LoadMetadata(); I like to avoid database operations or other similar "big work" in the constructor. Depending on how it is created, the database may not be set up at the time the constructor is called. And the database operations can fail which the constructor can't easily report. I would usually do something like this in a separate Init() step so the caller can control when things are ready to start reading, and also get back error codes if necessary. https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.cc:225: for (VisitVector::const_iterator visit = visits.begin(); This would be clearer using a range-based one: for (VisitVector::const_iterator visit : visits) https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.cc:309: CHECK(history_backend_); Is this necessary? The constructor already dchecks that this is not null. I would just delete this line. https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.cc:342: std::reverse(visits->begin(), visits->end()); I asked in the header to document this ordering. But it's not clear to me why you need any specific ordering. This is only called by GetAllData which may not care about the ordering. The comment contrasts it to GetMostRecentVisitsForURL but it's not called from there. Can you clarify? https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... File components/history/core/browser/typed_url_sync_bridge.h (right): https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.h:81: // modifying the passed |url| object and |visits| vector. Can this document the ordering of the resulting visits vector? https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:60: base::ReadBigEndian(storage_key.data(), &storage_key_int); The commit message says you changed this to big endian but doesn't motivate this change. Can the commit message explain why this change is necessary/desirable?
Description was changed from ========== Implement GetAllData and GetStorageKey. Also add TypedURLSyncBridge to be an observer of HistoryBackend. Change storageKey using big endian, BUG=726158 The following 4 functions are ported from TypedUrlSyncableService. GetErrorPercentage() WriteToTypedUrlSpecifics() ClearErrorStats() FixupURLAndGetVisits() ========== to ========== Implement GetAllData and GetStorageKey. Also add TypedURLSyncBridge to be an observer of HistoryBackend. Change storageKey using big endian, since user event is using big endian, and it is better to unify our number style. BUG=726158 The following 4 functions are ported from TypedUrlSyncableService. GetErrorPercentage() WriteToTypedUrlSpecifics() ClearErrorStats() FixupURLAndGetVisits() ==========
The CQ bit was checked by gangwu@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.
updated https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... File components/history/core/browser/typed_url_sync_bridge.cc (right): https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.cc:71: LoadMetadata(); On 2017/06/05 17:31:15, brettw wrote: > I like to avoid database operations or other similar "big work" in the > constructor. Depending on how it is created, the database may not be set up at > the time the constructor is called. And the database operations can fail which > the constructor can't easily report. I would usually do something like this in a > separate Init() step so the caller can control when things are ready to start > reading, and also get back error codes if necessary. Done. https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.cc:225: for (VisitVector::const_iterator visit = visits.begin(); On 2017/06/05 17:31:15, brettw wrote: > This would be clearer using a range-based one: > for (VisitVector::const_iterator visit : visits) Done. https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.cc:309: CHECK(history_backend_); On 2017/06/05 17:31:15, brettw wrote: > Is this necessary? The constructor already dchecks that this is not null. I > would just delete this line. Done. https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.cc:342: std::reverse(visits->begin(), visits->end()); On 2017/06/05 17:31:15, brettw wrote: > I asked in the header to document this ordering. But it's not clear to me why > you need any specific ordering. This is only called by GetAllData which may not > care about the ordering. The comment contrasts it to GetMostRecentVisitsForURL > but it's not called from there. Can you clarify? Visits may not need to be order in this CL, but in the next CL about merging remote visits and local visits, ordering will be needed. two ordered visits list can make compare easier and fast. https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... File components/history/core/browser/typed_url_sync_bridge.h (right): https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.h:81: // modifying the passed |url| object and |visits| vector. On 2017/06/05 17:31:15, brettw wrote: > Can this document the ordering of the resulting visits vector? Done. https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2901093009/diff/200001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:60: base::ReadBigEndian(storage_key.data(), &storage_key_int); On 2017/06/05 17:31:15, brettw wrote: > The commit message says you changed this to big endian but doesn't motivate this > change. Can the commit message explain why this change is necessary/desirable? Changing to big endian is from a comment from first iteration of this CL "It has a benefit of costing slightly less CPU for serializing/parsing." And I think it is better to unify our storage key, if a storage key is number, then using big endian, since another datatype is using big endian(https://cs.chromium.org/chromium/src/components/sync/user_events/user_event_sync_bridge.cc?l=38), so typed urls just following that style.
lgtm
The CQ bit was checked by gangwu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org Link to the patchset: https://codereview.chromium.org/2901093009/#ps220001 (title: "brettw review and rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1496695556202280,
"parent_rev": "d134f7b26ca09ea180cb70cfc527388765b3606f", "commit_rev":
"c2ae2ba24bb3d8a1a1324bde1252dd543e2bf708"}
Message was sent while issue was closed.
Description was changed from ========== Implement GetAllData and GetStorageKey. Also add TypedURLSyncBridge to be an observer of HistoryBackend. Change storageKey using big endian, since user event is using big endian, and it is better to unify our number style. BUG=726158 The following 4 functions are ported from TypedUrlSyncableService. GetErrorPercentage() WriteToTypedUrlSpecifics() ClearErrorStats() FixupURLAndGetVisits() ========== to ========== Implement GetAllData and GetStorageKey. Also add TypedURLSyncBridge to be an observer of HistoryBackend. Change storageKey using big endian, since user event is using big endian, and it is better to unify our number style. BUG=726158 The following 4 functions are ported from TypedUrlSyncableService. GetErrorPercentage() WriteToTypedUrlSpecifics() ClearErrorStats() FixupURLAndGetVisits() Review-Url: https://codereview.chromium.org/2901093009 Cr-Commit-Position: refs/heads/master@{#477079} Committed: https://chromium.googlesource.com/chromium/src/+/c2ae2ba24bb3d8a1a1324bde1252... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:220001) as https://chromium.googlesource.com/chromium/src/+/c2ae2ba24bb3d8a1a1324bde1252... |
