Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2721713002:
[sync] Add typed url sync metadata to the history db (Closed)
Description[sync] Add typed url sync metadata to the history db
Two tables of sync metadata, are now being added to the history database.
These two types are the per-entry EntityMetadata and the ModelTypeState,
which is global state for the typed url model type. This change adds a
migration to add the tables, and a new class TypedURLSyncMetadataDatabase
wich would read/write/delete metadatas, with associated tests.
Also a key word "AUTOINCREMENT" is added into urls table's column 'id'
which is primary key. AUTOINCREMENT can avoid to reuse id number when an
url got deleted and then recycled.
BUG=696701
Review-Url: https://codereview.chromium.org/2721713002
Cr-Commit-Position: refs/heads/master@{#458232}
Committed: https://chromium.googlesource.com/chromium/src/+/e8cbd06453b8b4dfc01fa61d004fa0469f42bd98
Patch Set 1 #Patch Set 2 : DEPS issue #
Total comments: 2
Patch Set 3 : Pavely review #
Total comments: 22
Patch Set 4 : sky review #Patch Set 5 : fix test issue caused by removing favicon #Patch Set 6 : git rebase #
Total comments: 16
Patch Set 7 : brettw review #
Total comments: 4
Patch Set 8 : move meta table into typed_url_metadata_database #
Total comments: 8
Patch Set 9 : update for comments #Messages
Total messages: 78 (62 generated)
Description was changed from ========== update comments add unittests BUG= ========== to ========== update comments add unittests BUG=696701 ==========
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...
Description was changed from ========== update comments add unittests BUG=696701 ========== to ========== [sync] Add typed url sync metadata to the history db Two tables of sync metadata, are now being added to the history database. These two types are the per-entry EntityMetadata and the ModelTypeState, which is global state for the typed url model type. This change adds a migration to add the tables, and a new class TypedURLSyncMetadataDatabase wich would read/write/delete metadatas, with associated tests. Also a key word "AUTOINCREMENT" is added into urls table's column 'id' which is primary key. AUTOINCREMENT can avoid to reuse id number when an url got deleted and then recycled. BUG=696701 ==========
gangwu@chromium.org changed reviewers: + pavely@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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:20001) has been deleted
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2721713002/diff/40001/components/history/core... File components/history/core/browser/DEPS (right): https://codereview.chromium.org/2721713002/diff/40001/components/history/core... components/history/core/browser/DEPS:3: "+components/sync", Did you add this because of warning you encountered? components/history/DEPS already allows include from components/sync.
gangwu@chromium.org changed reviewers: + sky@chromium.org
@sky, PTAL https://codereview.chromium.org/2721713002/diff/40001/components/history/core... File components/history/core/browser/DEPS (right): https://codereview.chromium.org/2721713002/diff/40001/components/history/core... components/history/core/browser/DEPS:3: "+components/sync", On 2017/02/28 22:46:03, pavely wrote: > Did you add this because of warning you encountered? > components/history/DEPS already allows include from components/sync. you are right, I think I should delete it here.
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.
https://codereview.chromium.org/2721713002/diff/60001/components/history/core... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.cc:22: for (const auto& pair : metadata_records) { no {} https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.cc:23: metadata_batch->AddMetadata(pair.first, pair.second); Using std::pair for this class makes for hard to understand code. I have no idea what pair.first or pair.second is. Please use a locally defined structure to make this code readable. https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.cc:50: s.BindString(1, metadata.SerializeAsString()); Who handles versioning of this string? https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.cc:102: "storage_key VARCHAR PRIMARY KEY NOT NULL," Document what storage_key is. In fact please document the scheme at the top of the file such as other classes in this directory do. https://codereview.chromium.org/2721713002/diff/60001/components/history/core... File components/history/core/browser/typed_url_sync_metadata_database.h (right): https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.h:20: // states) for each urls. Datatype state table contains the state of typed url urls->url https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.h:31: bool GetAllSyncMetadata(syncer::ModelType model_type, Why does this need to take a model_type? Same comment for all these functions. https://codereview.chromium.org/2721713002/diff/60001/components/history/core... File components/history/core/browser/typed_url_sync_metadata_database_unittest.cc (right): https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database_unittest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no (c) (see style guide) https://codereview.chromium.org/2721713002/diff/60001/components/history/core... File components/history/core/browser/url_database.cc (right): https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/url_database.cc:586: "id INTEGER PRIMARY KEY AUTOINCREMENT," You need to add test coverage of the migration. Specifically I would like a test that has some gaps in the ids, with visits referencing those ids and then verify they are there after the migration. See HistoryBackendTest.MigrationVisitSource for a test of migration. https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/url_database.cc:593: "favicon_id INTEGER DEFAULT 0 NOT NULL)"); // favicon_id is not used now. Now that you're migrating please remove this column. I don't believe it's needed anymore.
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 #5 (id:140001) 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, PTAL https://codereview.chromium.org/2721713002/diff/60001/components/history/core... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/03/01 04:15:24, sky wrote: > no (c) Done. https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.cc:22: for (const auto& pair : metadata_records) { On 2017/03/01 04:15:24, sky wrote: > no {} Done. https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.cc:23: metadata_batch->AddMetadata(pair.first, pair.second); On 2017/03/01 04:15:24, sky wrote: > Using std::pair for this class makes for hard to understand code. I have no idea > what pair.first or pair.second is. Please use a locally defined structure to > make this code readable. This is C++ STL auto iterator for std::map, I cannot change .first and .second for it, but I changed variable name for easy understand, a little better? or do you have other suggestions? https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.cc:50: s.BindString(1, metadata.SerializeAsString()); On 2017/03/01 04:15:24, sky wrote: > Who handles versioning of this string? protobuf will control backward compatibility for metadata serialize and deserialize. for metadata itself, if we update the definition of EntityMetadata, we will update version of database version to handle it. https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.cc:102: "storage_key VARCHAR PRIMARY KEY NOT NULL," On 2017/03/01 04:15:23, sky wrote: > Document what storage_key is. In fact please document the scheme at the top of > the file such as other classes in this directory do. Done. https://codereview.chromium.org/2721713002/diff/60001/components/history/core... File components/history/core/browser/typed_url_sync_metadata_database.h (right): https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/03/01 04:15:24, sky wrote: > no (c) Done. https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.h:20: // states) for each urls. Datatype state table contains the state of typed url On 2017/03/01 04:15:24, sky wrote: > urls->url Done. https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.h:31: bool GetAllSyncMetadata(syncer::ModelType model_type, On 2017/03/01 04:15:24, sky wrote: > Why does this need to take a model_type? Same comment for all these functions. I was thinking about merge with AutofillTable's GetAllSyncMetadata and other functions, making an interface class and let HistoryDatabase class and AutofillTable class to be derived class. but after discuss with team, it seems not necessary, so I should remove model_type. https://codereview.chromium.org/2721713002/diff/60001/components/history/core... File components/history/core/browser/typed_url_sync_metadata_database_unittest.cc (right): https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database_unittest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/03/01 04:15:24, sky wrote: > no (c) (see style guide) Done. https://codereview.chromium.org/2721713002/diff/60001/components/history/core... File components/history/core/browser/url_database.cc (right): https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/url_database.cc:586: "id INTEGER PRIMARY KEY AUTOINCREMENT," On 2017/03/01 04:15:24, sky wrote: > You need to add test coverage of the migration. Specifically I would like a test > that has some gaps in the ids, with visits referencing those ids and then verify > they are there after the migration. See HistoryBackendTest.MigrationVisitSource > for a test of migration. Done. https://codereview.chromium.org/2721713002/diff/60001/components/history/core... components/history/core/browser/url_database.cc:593: "favicon_id INTEGER DEFAULT 0 NOT NULL)"); // favicon_id is not used now. On 2017/03/01 04:15:24, sky wrote: > Now that you're migrating please remove this column. I don't believe it's needed > anymore. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sky@chromium.org changed reviewers: + brettw@chromium.org - sky@chromium.org
sky@chromium.org changed reviewers: + sky@chromium.org
I'm going to kick this over to Brett as he is going to be handling history reviews. I realized I started the review, but no doubt there are going to be future reviews and it would be good to have understand all the changes going forward (sorry Brett).
sky@chromium.org changed reviewers: - sky@chromium.org
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.
https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:14: // storage_key id in urls talbe, used by service to look up native data talbe -> table https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:16: // value Serialize sync EntityMetatada, which is for tracking sync EntityMetatada -> EntityMetadata https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:20: // id should inly one record in the table and record's id should This kind of thing is what the "meta" table is for. See //sql/meta_table.h. It stores the version number of the database, but the interface also has the ability to get/set key/value pairs. You can just re-use that interface for this value and avoid creating a new table (which has a lot more overhead). https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:22: // value Serialize sync ModelTypeState, which is for tracking sync "Serialize" -> "Serialization of" https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:97: "storage_key VARCHAR PRIMARY KEY NOT NULL," At the top of the file you describe the storage_key as "id in urls table". I would expect from the comment that the value is is the rowid of an entry in the urls table. But here it looks like a string. Can you clarify the comment at the top? https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... File components/history/core/browser/url_database.cc (right): https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... components/history/core/browser/url_database.cc:586: "id INTEGER PRIMARY KEY AUTOINCREMENT," I don't understand the bigger picture about why autoincrement is required here. Can you explain? According to the sqlite page on this: https://www.sqlite.org/autoinc.html we shouldn't use autoincrement unless we want the specific behavior.
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/2721713002/diff/180001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:14: // storage_key id in urls talbe, used by service to look up native data On 2017/03/13 19:58:59, brettw (plz ping after 24h) wrote: > talbe -> table Done. https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:16: // value Serialize sync EntityMetatada, which is for tracking sync On 2017/03/13 19:58:59, brettw (plz ping after 24h) wrote: > EntityMetatada -> EntityMetadata Done. https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:20: // id should inly one record in the table and record's id should On 2017/03/13 19:58:59, brettw (plz ping after 24h) wrote: > This kind of thing is what the "meta" table is for. See //sql/meta_table.h. It > stores the version number of the database, but the interface also has the > ability to get/set key/value pairs. You can just re-use that interface for this > value and avoid creating a new table (which has a lot more overhead). Yes, we did not realize there is a //sql/meta_table.h. but it seems HistoryDatabase already use it as meta_table_, I think we cannot use it here, otherwise two "meta" tables will easily mess up, right? And also since we just need an one entry table, I think it is ok to add a new table here. What do you think? https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:22: // value Serialize sync ModelTypeState, which is for tracking sync On 2017/03/13 19:58:59, brettw (plz ping after 24h) wrote: > "Serialize" -> "Serialization of" Done. https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:97: "storage_key VARCHAR PRIMARY KEY NOT NULL," On 2017/03/13 19:58:59, brettw (plz ping after 24h) wrote: > At the top of the file you describe the storage_key as "id in urls table". I > would expect from the comment that the value is is the rowid of an entry in the > urls table. But here it looks like a string. Can you clarify the comment at the > top? comment updated, here using string is because Sync read all those metadata into EntityMetadataMap, and EntityMetadataMap is std::map<std::string, sync_pb::EntityMetadata>, so mark storage_key as string here for easier translate. otherwise we will convert int to string later. https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... File components/history/core/browser/url_database.cc (right): https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... components/history/core/browser/url_database.cc:586: "id INTEGER PRIMARY KEY AUTOINCREMENT," On 2017/03/13 19:58:59, brettw (plz ping after 24h) wrote: > I don't understand the bigger picture about why autoincrement is required here. > Can you explain? According to the sqlite page on this: > https://www.sqlite.org/autoinc.html > we shouldn't use autoincrement unless we want the specific behavior. For sync propose, we need to have an unique key to identify the URLs, using URL as a key is too expensive since URL could be very long. ROWID is short, so we prefer to use it. But since ROWID might be reused after deletion(without AUTOINCREMENT), and if Sync not working somehow during deleting and reusing, once Sync come back, Sync would use ROWID and timestamp to see if there is an update need to be synced. And sync will only send new URL to server, but missed deleted URL. that is why we need to add AUTOINCREMENT here.
https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:20: // id should inly one record in the table and record's id should Can you pass the meta table to this class? (Be careful about destruction order). Using a separate table for this is way overkill (sqlite tables are always at least one page on disk, the statements to read and modify the table can't be cached, etc). https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:97: "storage_key VARCHAR PRIMARY KEY NOT NULL," On 2017/03/14 01:18:49, Gang Wu wrote: > On 2017/03/13 19:58:59, brettw (plz ping after 24h) wrote: > > At the top of the file you describe the storage_key as "id in urls table". I > > would expect from the comment that the value is is the rowid of an entry in > the > > urls table. But here it looks like a string. Can you clarify the comment at > the > > top? > > comment updated, here using string is because Sync read all those metadata into > EntityMetadataMap, and EntityMetadataMap is std::map<std::string, > sync_pb::EntityMetadata>, so mark storage_key as string here for easier > translate. otherwise we will convert int to string later. I would prefer to have the database store the int and to convert at the edges. The correlation to the URl table will be more clear, lookup will be faster, storage will be smaller, and it will be possible to do joins across tables. https://codereview.chromium.org/2721713002/diff/200001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2721713002/diff/200001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:33: for (const auto& storayge_key_to_metadata : metadata_records_map) It's a little bit hard for me to give good suggestions here since this code isn't hooked up to sync yet (I think) so I can't see final usage. But we need to change something here. GetAllSyncMetadata calls GetAllSyncEntityMetadata which makes a map from a database query. Then this iterates through the map (not using the map-ness of it) and calls MetadataBatch::AddMetadata which is the same kind of map created above. This is a LOT of wasted effort! We should make it so there's not an intermediate structure in between the database and the MetadataBatch. I think it would be nice also to make sure there's no extra copies of EntityMetadata (currently there are 3 for each item). This seems like a common pattern. How about a new function bool MetadataBatch:::AddSerializedMetadata(const std::string& key, const std::string& serialized_metadata) that deserializes it in place into the map with no extra copies? If we end up keeping this: - Needs {} since the inside is > 1 (Chrome style). - Also spelling "storayge"
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...
moved meta table. https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:20: // id should inly one record in the table and record's id should On 2017/03/14 19:24:43, brettw (plz ping after 24h) wrote: > Can you pass the meta table to this class? (Be careful about destruction order). > Using a separate table for this is way overkill (sqlite tables are always at > least one page on disk, the statements to read and modify the table can't be > cached, etc). Done. https://codereview.chromium.org/2721713002/diff/180001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:97: "storage_key VARCHAR PRIMARY KEY NOT NULL," On 2017/03/14 19:24:43, brettw (plz ping after 24h) wrote: > On 2017/03/14 01:18:49, Gang Wu wrote: > > On 2017/03/13 19:58:59, brettw (plz ping after 24h) wrote: > > > At the top of the file you describe the storage_key as "id in urls table". I > > > would expect from the comment that the value is is the rowid of an entry in > > the > > > urls table. But here it looks like a string. Can you clarify the comment at > > the > > > top? > > > > comment updated, here using string is because Sync read all those metadata > into > > EntityMetadataMap, and EntityMetadataMap is std::map<std::string, > > sync_pb::EntityMetadata>, so mark storage_key as string here for easier > > translate. otherwise we will convert int to string later. > > I would prefer to have the database store the int and to convert at the edges. > The correlation to the URl table will be more clear, lookup will be faster, > storage will be smaller, and it will be possible to do joins across tables. Done. https://codereview.chromium.org/2721713002/diff/200001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2721713002/diff/200001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:33: for (const auto& storayge_key_to_metadata : metadata_records_map) On 2017/03/14 19:24:43, brettw (plz ping after 24h) wrote: > It's a little bit hard for me to give good suggestions here since this code > isn't hooked up to sync yet (I think) so I can't see final usage. > > But we need to change something here. GetAllSyncMetadata calls > GetAllSyncEntityMetadata which makes a map from a database query. Then this > iterates through the map (not using the map-ness of it) and calls > MetadataBatch::AddMetadata which is the same kind of map created above. This is > a LOT of wasted effort! > > We should make it so there's not an intermediate structure in between the > database and the MetadataBatch. > > I think it would be nice also to make sure there's no extra copies of > EntityMetadata (currently there are 3 for each item). This seems like a common > pattern. How about a new function > bool MetadataBatch:::AddSerializedMetadata(const std::string& key, const > std::string& serialized_metadata) > that deserializes it in place into the map with no extra copies? > > If we end up keeping this: > - Needs {} since the inside is > 1 (Chrome style). > - Also spelling "storayge" This is really a good suggestion! But since add this new function will touch several files, I will do it in another CL, add a todo here for now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
ping
I think I'd like to take a final look before checkin. But since this has dragged on, please ping me on IM when you upload the new one so I can take a look right away. https://codereview.chromium.org/2721713002/diff/200001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2721713002/diff/200001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:33: for (const auto& storayge_key_to_metadata : metadata_records_map) Following up with another patch for this is OK to me, partially because I feel guilty about sitting on this CL for so long :) But I don't think we should keep this code here for very long. Will you be able to follow up in the next week or so? https://codereview.chromium.org/2721713002/diff/220001/components/history/cor... File components/history/core/browser/history_database.h (left): https://codereview.chromium.org/2721713002/diff/220001/components/history/cor... components/history/core/browser/history_database.h:194: sql::MetaTable meta_table_; Conceptually, the meta table should live at this layer since it's really a global history database thing. The not-so-great inheritance use by the history system is my fault from long ago that makes this more difficult than it might otherwise be. The best thing I can think of is if your typed table class had a set_meta_table(MetaTable* table) member. HistoryDatabase would set this on init and clear this in it's destructor (to prevent dangling pointers on destruction). What do you think? https://codereview.chromium.org/2721713002/diff/220001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2721713002/diff/220001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:14: const char kTypedURLModelTypeStateKey[] = "typed_url_model_type_state"; Can you provide documentation somewhere (maybe here is the best?) for what the value of this key means in the database? https://codereview.chromium.org/2721713002/diff/220001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.h (right): https://codereview.chromium.org/2721713002/diff/220001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.h:36: bool UpdateSyncMetadata(const std::string& storage_key, I was expecting the types here would match the database types (so this would be an int64). Generally the row updating functions in these database classes are pretty low-level and just provide a C++ interface to the storage. It seems this is not hooked up so it's hard for me to see which approach is more appropriate. What code would be responsible for calling this function? If this is some kind of typed URL sync adapter class, I'd rather have that adapter class do the StringtoInt conversions. https://codereview.chromium.org/2721713002/diff/220001/components/history/cor... File components/history/core/browser/url_database.cc (right): https://codereview.chromium.org/2721713002/diff/220001/components/history/cor... components/history/core/browser/url_database.cc:586: "id INTEGER PRIMARY KEY AUTOINCREMENT," This should have a comment about why AUTOINCREMENT is required. Be sure to mention about sync not working between deleting and re-using that you wrote in response to my question in a previous pass, that's the critical part.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Patchset #9 (id:240001) 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, PTAL https://codereview.chromium.org/2721713002/diff/200001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2721713002/diff/200001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:33: for (const auto& storayge_key_to_metadata : metadata_records_map) On 2017/03/20 17:27:06, brettw (plz ping after 24h) wrote: > Following up with another patch for this is OK to me, partially because I feel > guilty about sitting on this CL for so long :) But I don't think we should keep > this code here for very long. Will you be able to follow up in the next week or > so? yes, I talked with team, I think I will fix this in 2 weeks after this CL checked in. https://codereview.chromium.org/2721713002/diff/220001/components/history/cor... File components/history/core/browser/history_database.h (left): https://codereview.chromium.org/2721713002/diff/220001/components/history/cor... components/history/core/browser/history_database.h:194: sql::MetaTable meta_table_; On 2017/03/20 17:27:06, brettw (plz ping after 24h) wrote: > Conceptually, the meta table should live at this layer since it's really a > global history database thing. > > The not-so-great inheritance use by the history system is my fault from long ago > that makes this more difficult than it might otherwise be. > > The best thing I can think of is if your typed table class had a > set_meta_table(MetaTable* table) member. HistoryDatabase would set this on init > and clear this in it's destructor (to prevent dangling pointers on destruction). > What do you think? I copied idea from db_ and GetDB(), in base class has a virtual class to get MetaTable, do you think it is ok? https://codereview.chromium.org/2721713002/diff/220001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.cc (right): https://codereview.chromium.org/2721713002/diff/220001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.cc:14: const char kTypedURLModelTypeStateKey[] = "typed_url_model_type_state"; On 2017/03/20 17:27:06, brettw (plz ping after 24h) wrote: > Can you provide documentation somewhere (maybe here is the best?) for what the > value of this key means in the database? Done. https://codereview.chromium.org/2721713002/diff/220001/components/history/cor... File components/history/core/browser/typed_url_sync_metadata_database.h (right): https://codereview.chromium.org/2721713002/diff/220001/components/history/cor... components/history/core/browser/typed_url_sync_metadata_database.h:36: bool UpdateSyncMetadata(const std::string& storage_key, On 2017/03/20 17:27:06, brettw (plz ping after 24h) wrote: > I was expecting the types here would match the database types (so this would be > an int64). Generally the row updating functions in these database classes are > pretty low-level and just provide a C++ interface to the storage. > > It seems this is not hooked up so it's hard for me to see which approach is more > appropriate. > > What code would be responsible for calling this function? If this is some kind > of typed URL sync adapter class, I'd rather have that adapter class do the > StringtoInt conversions. Actually, I am think about make those functions as an interface, UpdateModelTypeState, ClearModelTypeState, UpdateMetadata and ClearMetadata, and maybe GetAllSyncMetadata. It seems only MetadataChangeList will call this 4 functions, here is aotofill/autocomplete implementation, https://cs.chromium.org/chromium/src/components/autofill/core/browser/webdata..., so I think maybe we can unify them and make it as interface. https://codereview.chromium.org/2721713002/diff/220001/components/history/cor... File components/history/core/browser/url_database.cc (right): https://codereview.chromium.org/2721713002/diff/220001/components/history/cor... components/history/core/browser/url_database.cc:586: "id INTEGER PRIMARY KEY AUTOINCREMENT," On 2017/03/20 17:27:06, brettw (plz ping after 24h) wrote: > This should have a comment about why AUTOINCREMENT is required. Be sure to > mention about sync not working between deleting and re-using that you wrote in > response to my question in a previous pass, that's the critical part. Done.
lgtm
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
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/2721713002/#ps260001 (title: "update for comments")
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": 260001, "attempt_start_ts": 1490051615139930,
"parent_rev": "96c63e3daa19994e75cf804fd234d09fd5021ab0", "commit_rev":
"e8cbd06453b8b4dfc01fa61d004fa0469f42bd98"}
Message was sent while issue was closed.
Description was changed from ========== [sync] Add typed url sync metadata to the history db Two tables of sync metadata, are now being added to the history database. These two types are the per-entry EntityMetadata and the ModelTypeState, which is global state for the typed url model type. This change adds a migration to add the tables, and a new class TypedURLSyncMetadataDatabase wich would read/write/delete metadatas, with associated tests. Also a key word "AUTOINCREMENT" is added into urls table's column 'id' which is primary key. AUTOINCREMENT can avoid to reuse id number when an url got deleted and then recycled. BUG=696701 ========== to ========== [sync] Add typed url sync metadata to the history db Two tables of sync metadata, are now being added to the history database. These two types are the per-entry EntityMetadata and the ModelTypeState, which is global state for the typed url model type. This change adds a migration to add the tables, and a new class TypedURLSyncMetadataDatabase wich would read/write/delete metadatas, with associated tests. Also a key word "AUTOINCREMENT" is added into urls table's column 'id' which is primary key. AUTOINCREMENT can avoid to reuse id number when an url got deleted and then recycled. BUG=696701 Review-Url: https://codereview.chromium.org/2721713002 Cr-Commit-Position: refs/heads/master@{#458232} Committed: https://chromium.googlesource.com/chromium/src/+/e8cbd06453b8b4dfc01fa61d004f... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:260001) as https://chromium.googlesource.com/chromium/src/+/e8cbd06453b8b4dfc01fa61d004f... |
