Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2582713003:
[USS] Impelementation for GetData and GetAllData (Closed)
|
Created:
4 years ago by Gang Wu Modified:
3 years, 11 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[USS] Impelementation for GetData and GetAllData
BUG=672617
Committed: https://crrev.com/b5e5d4663754f905b2d1465c258ca1c21979c70a
Cr-Commit-Position: refs/heads/master@{#440589}
Patch Set 1 : rebase #Patch Set 2 : self review #
Total comments: 37
Patch Set 3 : skym review #
Total comments: 44
Patch Set 4 : skym review 2 #
Total comments: 18
Patch Set 5 : vabr review #Patch Set 6 : mark CreateAutofillEntry to public #
Messages
Total messages: 58 (43 generated)
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...
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 ========== impelementation for GetData and GetAllData BUG= ========== to ========== [USS] Impelementation for GetData and GetAllData BUG=672617 ==========
gangwu@chromium.org changed reviewers: + mathp@chromium.org, skym@chromium.org
Patchset #3 (id:40001) has been deleted
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...)
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 #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #1 (id:1) 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/2582713003/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:166: std::unique_ptr<syncer::EntityData> AutocompleteSyncBridge::CreateEntityData( Looks like you could get away with this being an anonymous namespace scoped method. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h (right): https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:73: const std::string GetStorageKeyFromAutofillEntry( Why are all these functions returning const strings? I don't see what value the const is adding. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:83: const sync_pb::AutofillSpecifics* autofill_specifics); Why not take a const sync_pb::AutofillSpecifics& https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:37: void VerifyEqual(const AutofillSpecifics& s1, const AutofillSpecifics& s2) { Can you just call SerializeAsString() on both and make sure the strings are equal? Sorry I set a bad example in device info sync bridge unittest! https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:79: void VerifyDataBatch(std::map<std::string, AutofillSpecifics> expected, Hmm, I wonder if we could make a templated version of this. Out of scope of this CL though. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:95: AutofillEntry* MakeAutofillEntry(const char* name, Seems like a bunch of code is getting copied out of profile_sync_service_autofill_unittest.cc. Would be ideal if we didn't duplicate it. Also, returning an owning raw ptr is probably not a great idea. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:97: int time_shift0, time_shift<n> is a weird naming scheme. Also, this defaulting logic is really confusing. What do you think of doing something like the following instead: ret foo1(arg1, arg2, arg3) { return foo2(arg1, arg2, arg3, arg3); } ret foo2(arg1, arg2, arg3, arg4) { /* actual logic */ } https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:122: void NotifyThatSyncHasStarted(syncer::ModelType /* model_type */) override {} I didn't know you didn't have to name the variable. Cool. However, I'd remove the commented out variable name. It's not adding anything. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:129: class FakeWebDataService : public AutofillWebDataService { I'm probably missing something, but I don't understand why we need this. Can you add some comments explaining how this works? https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:146: AutocompleteSyncBridge* Bridge() { return bridge_.get(); } I think I typically see this kind of function named bridge(), with a lowercase b. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:150: CreateModelTypeChangeProcessor(syncer::ModelType type, This method doesn't need to be an instanced method right now. Though this may be short sighted. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:160: table_->UpdateAutofillEntries(new_entries); You can use C++11 list-initialization to inline the vector. table_->UpdateAutofillEntries({AutocompleteSyncBridge::CreateAutofillEntry(&specifics)}); https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:176: void SetUp() override { Seems to me like you could move most of the contents of this function into the constructor. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:181: table_.reset(new AutofillTable); table_ = base::MakeUnique<AutofillTable>(); https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:182: db_.reset(new WebDatabase); MakeUnique https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:205: std::unique_ptr<AutofillEntry> entry_; I don't like this entry instance field you're holding onto here. Lets make these things on the stack as we need them. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:215: SaveSpecificsToTable(specifics1); Would it be nicer if you could pass a vector of specifics to save them? const AutofillSpecifics specifics1 = CreateSpecifics(1); const AutofillSpecifics specifics2 = CreateSpecifics(2); const AutofillSpecifics specifics3 = CreateSpecifics(3); SaveSpecificsToTable({specifics1, specifics2, specifics3}); Or maybe you could create them already saved const AutofillSpecifics specifics1 = CreateSavedSpecifics(1); const AutofillSpecifics specifics2 = CreateSavedSpecifics(2); const AutofillSpecifics specifics3 = CreateSavedSpecifics(3); https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:222: Bridge()->GetData({GetStorageKey(specifics1), GetStorageKey(specifics3)}, You should also test calling GetData for a key that doesn't exist.
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...
updated. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:166: std::unique_ptr<syncer::EntityData> AutocompleteSyncBridge::CreateEntityData( On 2016/12/19 17:40:08, skym wrote: > Looks like you could get away with this being an anonymous namespace scoped > method. Done. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h (right): https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:73: const std::string GetStorageKeyFromAutofillEntry( On 2016/12/19 17:40:08, skym wrote: > Why are all these functions returning const strings? I don't see what value the > const is adding. Done. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:83: const sync_pb::AutofillSpecifics* autofill_specifics); On 2016/12/19 17:40:08, skym wrote: > Why not take a const sync_pb::AutofillSpecifics& Done. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:37: void VerifyEqual(const AutofillSpecifics& s1, const AutofillSpecifics& s2) { On 2016/12/19 17:40:09, skym wrote: > Can you just call SerializeAsString() on both and make sure the strings are > equal? Sorry I set a bad example in device info sync bridge unittest! Done. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:79: void VerifyDataBatch(std::map<std::string, AutofillSpecifics> expected, On 2016/12/19 17:40:09, skym wrote: > Hmm, I wonder if we could make a templated version of this. Out of scope of this > CL though. Acknowledged. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:95: AutofillEntry* MakeAutofillEntry(const char* name, On 2016/12/19 17:40:08, skym wrote: > Seems like a bunch of code is getting copied out of > profile_sync_service_autofill_unittest.cc. Would be ideal if we didn't duplicate > it. > > Also, returning an owning raw ptr is probably not a great idea. Done. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:97: int time_shift0, On 2016/12/19 17:40:08, skym wrote: > time_shift<n> is a weird naming scheme. > > Also, this defaulting logic is really confusing. What do you think of doing > something like the following instead: > > ret foo1(arg1, arg2, arg3) { return foo2(arg1, arg2, arg3, arg3); } > ret foo2(arg1, arg2, arg3, arg4) { /* actual logic */ } Done. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:122: void NotifyThatSyncHasStarted(syncer::ModelType /* model_type */) override {} On 2016/12/19 17:40:09, skym wrote: > I didn't know you didn't have to name the variable. Cool. However, I'd remove > the commented out variable name. It's not adding anything. Done. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:129: class FakeWebDataService : public AutofillWebDataService { On 2016/12/19 17:40:08, skym wrote: > I'm probably missing something, but I don't understand why we need this. Can you > add some comments explaining how this works? Done. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:146: AutocompleteSyncBridge* Bridge() { return bridge_.get(); } On 2016/12/19 17:40:09, skym wrote: > I think I typically see this kind of function named bridge(), with a lowercase > b. Done. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:150: CreateModelTypeChangeProcessor(syncer::ModelType type, On 2016/12/19 17:40:09, skym wrote: > This method doesn't need to be an instanced method right now. Though this may be > short sighted. Done. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:160: table_->UpdateAutofillEntries(new_entries); On 2016/12/19 17:40:08, skym wrote: > You can use C++11 list-initialization to inline the vector. > > table_->UpdateAutofillEntries({AutocompleteSyncBridge::CreateAutofillEntry(&specifics)}); Acknowledged. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:176: void SetUp() override { On 2016/12/19 17:40:08, skym wrote: > Seems to me like you could move most of the contents of this function into the > constructor. Done. since assert cannot be in constructor, so I still need to leave something here. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:181: table_.reset(new AutofillTable); On 2016/12/19 17:40:08, skym wrote: > table_ = base::MakeUnique<AutofillTable>(); Done. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:182: db_.reset(new WebDatabase); On 2016/12/19 17:40:08, skym wrote: > MakeUnique Done. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:205: std::unique_ptr<AutofillEntry> entry_; On 2016/12/19 17:40:09, skym wrote: > I don't like this entry instance field you're holding onto here. Lets make these > things on the stack as we need them. Done. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:215: SaveSpecificsToTable(specifics1); On 2016/12/19 17:40:09, skym wrote: > Would it be nicer if you could pass a vector of specifics to save them? > > const AutofillSpecifics specifics1 = CreateSpecifics(1); > const AutofillSpecifics specifics2 = CreateSpecifics(2); > const AutofillSpecifics specifics3 = CreateSpecifics(3); > SaveSpecificsToTable({specifics1, specifics2, specifics3}); > > Or maybe you could create them already saved > > const AutofillSpecifics specifics1 = CreateSavedSpecifics(1); > const AutofillSpecifics specifics2 = CreateSavedSpecifics(2); > const AutofillSpecifics specifics3 = CreateSavedSpecifics(3); Done. https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:222: Bridge()->GetData({GetStorageKey(specifics1), GetStorageKey(specifics3)}, On 2016/12/19 17:40:08, skym wrote: > You should also test calling GetData for a key that doesn't exist. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:119: for (const AutofillEntry& it : entries) { I'm not a big fan of using the variable "it", because this isn't an iterator, right? You just have a const reference to a single entry at a time. Maybe call it "entry"? https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:133: for (const AutofillEntry& it : entries) { Again, "it" isn't accurate. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:139: std::string AutocompleteSyncBridge::GetClientTag( Not really in scope for this CL, but we need to add unit tests to guarantee backwards comparability. Created crbug.com/675991 https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:173: std::string AutocompleteSyncBridge::FormatStorageKey(const std::string& name, Again, out of scope of this CL, but this looks wrong to me. I don't think we want to use a StorageKey that's non-trivial to reverse. I don't know if net::EscapePath is escaping the '|' character, if so, this is going to get ugly, if not, then this approach does not work. We don't have specifics when handling deletes in USS. Created crbug.com/675992 https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:57: AutofillEntry MakeAutofillEntry(const char* name, What's the reason for const char* instead of const std::string&? https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:59: int create_time_shift, What do you think of taking base::TimeDelta instead of ints? https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:60: int delete_time_shift) { Why is this called delete_time_shift? Isn't this the last used time? https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:63: static Time base_time = Time::Now().LocalMidnight(); I didn't realize this was static. That actually is kinda nice. Can you add to the comment explaining that this variable is static so that all autofill entries are created with shifts relative to the same reference point in time. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:65: Time date_created = base_time + TimeDelta::FromSeconds(create_time_shift); FromSeconds is actually the wrong conversion, right? base::Time::FromInternalValue is not converting to/from seconds. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:67: if (delete_time_shift >= 0) Lets remove this >= 0 behavior completely and just trust our caller to give us a reasonable second time. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:76: return MakeAutofillEntry(name, value, time_shift, -1); I think passing -1 is bad. I was trying to suggest passing |time_shift| as both the 3rd and 4th param to make it explicit that this overload is forcing both creation time and last used time to be the same value. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:82: auto processor = base::MakeUnique<syncer::FakeModelTypeChangeProcessor>(); No reason for this function body to be two lines. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:111: AutocompleteSyncBridge* bridge() { return bridge_.get(); } This can be protected too, right? https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:116: for (const auto& specific : specifics) { Sorry to get really nit picky here, but I'm not happy with these variable names. AutofillSpecifics has an 's' on the end of it. I'd prefer seeing something like for (const auto& specifics : specifics_list) { https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:138: file_ = temp_dir_.GetPath().AppendASCII("SyncTestWebDatabase"); This can be in the ctor. It doesn't actually do file IO so we can can assert the parent directory actually exists at a later time. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:140: db_.AddTable(&table_); Why not move this into the ctor? https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:141: ASSERT_EQ(sql::INIT_OK, db_.Init(file_)); Probably out of scope of this CL, but we'll likely want to test the bridge's code handling non-immediate initialization, which means this will probably have to be in a separate method that test cases can delay calling. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:148: void TearDown() override {} I'd just remove this until we need it. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:150: base::FilePath file_; These should all be private. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:157: AutofillEntry entry_; Still, why do you hang onto this AutofillEntry object?
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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Hi, I won't be able to do the components/autofill review in time for you (going on holiday). Sorry!
gangwu@chromium.org changed reviewers: + estade@chromium.org - mathp@chromium.org
updated https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:119: for (const AutofillEntry& it : entries) { On 2016/12/20 17:23:43, skym wrote: > I'm not a big fan of using the variable "it", because this isn't an iterator, > right? You just have a const reference to a single entry at a time. Maybe call > it "entry"? Done. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:133: for (const AutofillEntry& it : entries) { On 2016/12/20 17:23:43, skym wrote: > Again, "it" isn't accurate. Done. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:139: std::string AutocompleteSyncBridge::GetClientTag( On 2016/12/20 17:23:43, skym wrote: > Not really in scope for this CL, but we need to add unit tests to guarantee > backwards comparability. Created crbug.com/675991 Acknowledged. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:173: std::string AutocompleteSyncBridge::FormatStorageKey(const std::string& name, On 2016/12/20 17:23:43, skym wrote: > Again, out of scope of this CL, but this looks wrong to me. I don't think we > want to use a StorageKey that's non-trivial to reverse. I don't know if > net::EscapePath is escaping the '|' character, if so, this is going to get ugly, > if not, then this approach does not work. We don't have specifics when handling > deletes in USS. Created crbug.com/675992 Acknowledged. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:57: AutofillEntry MakeAutofillEntry(const char* name, On 2016/12/20 17:23:43, skym wrote: > What's the reason for const char* instead of const std::string&? function removed https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:59: int create_time_shift, On 2016/12/20 17:23:43, skym wrote: > What do you think of taking base::TimeDelta instead of ints? Done. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:60: int delete_time_shift) { On 2016/12/20 17:23:44, skym wrote: > Why is this called delete_time_shift? Isn't this the last used time? Done. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:63: static Time base_time = Time::Now().LocalMidnight(); On 2016/12/20 17:23:44, skym wrote: > I didn't realize this was static. That actually is kinda nice. Can you add to > the comment explaining that this variable is static so that all autofill entries > are created with shifts relative to the same reference point in time. Done. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:65: Time date_created = base_time + TimeDelta::FromSeconds(create_time_shift); On 2016/12/20 17:23:44, skym wrote: > FromSeconds is actually the wrong conversion, right? > base::Time::FromInternalValue is not converting to/from seconds. Done. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:67: if (delete_time_shift >= 0) On 2016/12/20 17:23:44, skym wrote: > Lets remove this >= 0 behavior completely and just trust our caller to give us a > reasonable second time. Done. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:76: return MakeAutofillEntry(name, value, time_shift, -1); On 2016/12/20 17:23:43, skym wrote: > I think passing -1 is bad. I was trying to suggest passing |time_shift| as both > the 3rd and 4th param to make it explicit that this overload is forcing both > creation time and last used time to be the same value. function removed. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:82: auto processor = base::MakeUnique<syncer::FakeModelTypeChangeProcessor>(); On 2016/12/20 17:23:43, skym wrote: > No reason for this function body to be two lines. Done. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:111: AutocompleteSyncBridge* bridge() { return bridge_.get(); } On 2016/12/20 17:23:43, skym wrote: > This can be protected too, right? Done. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:116: for (const auto& specific : specifics) { On 2016/12/20 17:23:44, skym wrote: > Sorry to get really nit picky here, but I'm not happy with these variable names. > AutofillSpecifics has an 's' on the end of it. I'd prefer seeing something like > > for (const auto& specifics : specifics_list) { Done. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:138: file_ = temp_dir_.GetPath().AppendASCII("SyncTestWebDatabase"); On 2016/12/20 17:23:44, skym wrote: > This can be in the ctor. It doesn't actually do file IO so we can can assert the > parent directory actually exists at a later time. https://cs.chromium.org/search/?q=CreateUniqueTempDir+file:%5Esrc/components/... most of our unittest put this piece of code in the SetUp function, so for consistence, I think we should do it in same way. If you think we do need to put it in ctor, maybe we can do it in another CL which move all the code into ctor. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:140: db_.AddTable(&table_); On 2016/12/20 17:23:44, skym wrote: > Why not move this into the ctor? same reason, autofill_table_unittest.cc:130, I just do the same way as autofill table unittest did. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:141: ASSERT_EQ(sql::INIT_OK, db_.Init(file_)); On 2016/12/20 17:23:44, skym wrote: > Probably out of scope of this CL, but we'll likely want to test the bridge's > code handling non-immediate initialization, which means this will probably have > to be in a separate method that test cases can delay calling. maybe cover in integration tests? https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:148: void TearDown() override {} On 2016/12/20 17:23:43, skym wrote: > I'd just remove this until we need it. Done. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:150: base::FilePath file_; On 2016/12/20 17:23:44, skym wrote: > These should all be private. Done. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:157: AutofillEntry entry_; On 2016/12/20 17:23:44, skym wrote: > Still, why do you hang onto this AutofillEntry object? Done.
lgtm https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:138: file_ = temp_dir_.GetPath().AppendASCII("SyncTestWebDatabase"); On 2016/12/20 21:55:51, Gang Wu wrote: > On 2016/12/20 17:23:44, skym wrote: > > This can be in the ctor. It doesn't actually do file IO so we can can assert > the > > parent directory actually exists at a later time. > > https://cs.chromium.org/search/?q=CreateUniqueTempDir+file:%5Esrc/components/... > most of our unittest put this piece of code in the SetUp function, so for > consistence, I think we should do it in same way. > If you think we do need to put it in ctor, maybe we can do it in another CL > which move all the code into ctor. Looking at your search, there are a couple places that don't actually put GetPath() in SetUp(). I'm not arguing to move CreateUnqiueTempDir(), I'm sold on it being right here. However, my previous suggestion was completely wrong. Looking at the implementation, you cannot re-order CreateUniqueTempDir() and GetPath(), because CreateUniqueTempDir() creates internal state in memory that GetPath() returns. But, okay, new suggestion! You don't need to hang onto |file_|, right? You can create it and pass it all inline when calling Init(..) on the DB. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:140: db_.AddTable(&table_); On 2016/12/20 21:55:51, Gang Wu wrote: > On 2016/12/20 17:23:44, skym wrote: > > Why not move this into the ctor? > > same reason, autofill_table_unittest.cc:130, I just do the same way as autofill > table unittest did. See https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... , it seems to be generally better style to put it in the ctor if possible. Though, splitting logic between the two is probably messier than putting everything in SetUp(). Okay, I'm fine with your approach.
vabr@chromium.org changed reviewers: + vabr@chromium.org
Hi, an alternative autofill owner here. This change looks mostly good to me, but please have a look at the comments below. Cheers, Vaclav https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:181: AutofillKey key(autofill_specifics.name().c_str(), optional: Avoiding going through the C-style would have the advantage of not having the measure the string length again. Therefore, please consider converting to string16 (going through the implicit StringPiece constructor from a std::string: AutofillKey key(base::UTF8ToUTF16(autofill_specifics.name()), base::UTF8ToUTF16(autofill_specifics.value())); https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:186: if (timestamps.size() > 0) { Why not !timestamps.empty() instead? (Non-emptiness might be in general easier to determine than the exact size.) https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h (right): https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:68: friend class AutocompleteSyncBridgeTest; Please do not friend the test. Tests should only depend on the public API, making them depend on the internals makes them fragile. I added some concrete suggestions below for how to keep your current test without the need to friend it with the bridge. https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:79: static AutofillEntry CreateAutofillEntry( I can only see this used in the test. Could you define it as an anonymous function there? https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:105: return AutocompleteSyncBridge::FormatStorageKey(specifics.name(), Could you use some of the public-facing methods based on FormatStorageKey instead? https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:110: ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); Could the work of SetUp be done in the constructor instead? The constructor is generally preferred, see https://github.com/google/googletest/blob/master/googletest/docs/V1_7_FAQ.md#... and https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/J2K-2Dmupb8... https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:128: std::unique_ptr<AutocompleteSyncBridge> bridge_; Please #include <memory> for this. https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:130: DISALLOW_COPY_AND_ASSIGN(AutocompleteSyncBridgeTest); Please #include "base/macros.h" for this.
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/2582713003/diff/140001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h (right): https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:79: static AutofillEntry CreateAutofillEntry( On 2016/12/21 08:48:07, vabr (Chromium) wrote: > I can only see this used in the test. Could you define it as an anonymous > function there? I think moving this function to the test file would be short sighted. We're going to need this function for the bridge implementation. Maybe this could simply be public instead of private? I don't think it would hurt to unittest this function directly. I think converting between specifics and AutofillEntry and/or AutofillKey objects is essentially core functionality the bridge performs.
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...
thanks vabr jump in to review! CL updated! https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:138: file_ = temp_dir_.GetPath().AppendASCII("SyncTestWebDatabase"); On 2016/12/20 22:38:05, skym wrote: > On 2016/12/20 21:55:51, Gang Wu wrote: > > On 2016/12/20 17:23:44, skym wrote: > > > This can be in the ctor. It doesn't actually do file IO so we can can assert > > the > > > parent directory actually exists at a later time. > > > > > https://cs.chromium.org/search/?q=CreateUniqueTempDir+file:%5Esrc/components/... > > most of our unittest put this piece of code in the SetUp function, so for > > consistence, I think we should do it in same way. > > If you think we do need to put it in ctor, maybe we can do it in another CL > > which move all the code into ctor. > > Looking at your search, there are a couple places that don't actually put > GetPath() in SetUp(). I'm not arguing to move CreateUnqiueTempDir(), I'm sold on > it being right here. > > However, my previous suggestion was completely wrong. Looking at the > implementation, you cannot re-order CreateUniqueTempDir() and GetPath(), because > CreateUniqueTempDir() creates internal state in memory that GetPath() returns. > > But, okay, new suggestion! You don't need to hang onto |file_|, right? You can > create it and pass it all inline when calling Init(..) on the DB. Done. https://codereview.chromium.org/2582713003/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:140: db_.AddTable(&table_); On 2016/12/20 22:38:05, skym wrote: > On 2016/12/20 21:55:51, Gang Wu wrote: > > On 2016/12/20 17:23:44, skym wrote: > > > Why not move this into the ctor? > > > > same reason, autofill_table_unittest.cc:130, I just do the same way as > autofill > > table unittest did. > > See > https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... > , it seems to be generally better style to put it in the ctor if possible. > > Though, splitting logic between the two is probably messier than putting > everything in SetUp(). Okay, I'm fine with your approach. Acknowledged. https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:181: AutofillKey key(autofill_specifics.name().c_str(), On 2016/12/21 08:48:07, vabr (Chromium) wrote: > optional: > Avoiding going through the C-style would have the advantage of not having the > measure the string length again. > > Therefore, please consider converting to string16 (going through the implicit > StringPiece constructor from a std::string: > AutofillKey key(base::UTF8ToUTF16(autofill_specifics.name()), > base::UTF8ToUTF16(autofill_specifics.value())); Done. https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:186: if (timestamps.size() > 0) { On 2016/12/21 08:48:07, vabr (Chromium) wrote: > Why not > !timestamps.empty() > instead? (Non-emptiness might be in general easier to determine than the exact > size.) Done. https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h (right): https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:68: friend class AutocompleteSyncBridgeTest; On 2016/12/21 08:48:07, vabr (Chromium) wrote: > Please do not friend the test. Tests should only depend on the public API, > making them depend on the internals makes them fragile. I added some concrete > suggestions below for how to keep your current test without the need to friend > it with the bridge. Done. https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:79: static AutofillEntry CreateAutofillEntry( On 2016/12/21 08:48:07, vabr (Chromium) wrote: > I can only see this used in the test. Could you define it as an anonymous > function there? Done. https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:79: static AutofillEntry CreateAutofillEntry( On 2016/12/21 21:50:41, skym wrote: > On 2016/12/21 08:48:07, vabr (Chromium) wrote: > > I can only see this used in the test. Could you define it as an anonymous > > function there? > > I think moving this function to the test file would be short sighted. We're > going to need this function for the bridge implementation. Maybe this could > simply be public instead of private? I don't think it would hurt to unittest > this function directly. I think converting between specifics and AutofillEntry > and/or AutofillKey objects is essentially core functionality the bridge > performs. you are right, we need this function later on, even I move this one to unittest, we will move it back when sync data from sync server to autofill. So change it to public. https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:105: return AutocompleteSyncBridge::FormatStorageKey(specifics.name(), On 2016/12/21 08:48:07, vabr (Chromium) wrote: > Could you use some of the public-facing methods based on FormatStorageKey > instead? Done. https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:110: ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); On 2016/12/21 08:48:07, vabr (Chromium) wrote: > Could the work of SetUp be done in the constructor instead? > > The constructor is generally preferred, see > https://github.com/google/googletest/blob/master/googletest/docs/V1_7_FAQ.md#... > and > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/J2K-2Dmupb8... > Done. https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:128: std::unique_ptr<AutocompleteSyncBridge> bridge_; On 2016/12/21 08:48:07, vabr (Chromium) wrote: > Please #include <memory> for this. Done. https://codereview.chromium.org/2582713003/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:130: DISALLOW_COPY_AND_ASSIGN(AutocompleteSyncBridgeTest); On 2016/12/21 08:48:07, vabr (Chromium) wrote: > Please #include "base/macros.h" for this. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you, this CL LGTM. Cheers, Vaclav
The CQ bit was checked by gangwu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org Link to the patchset: https://codereview.chromium.org/2582713003/#ps180001 (title: "mark CreateAutofillEntry to public")
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": 180001, "attempt_start_ts": 1482457665408880,
"parent_rev": "fa412bbc07cc0cd96ce0a0353547e765313f1ff1", "commit_rev":
"484c072208db7222fed33a2dc6949bbfa5668c77"}
Message was sent while issue was closed.
Description was changed from ========== [USS] Impelementation for GetData and GetAllData BUG=672617 ========== to ========== [USS] Impelementation for GetData and GetAllData BUG=672617 Review-Url: https://codereview.chromium.org/2582713003 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [USS] Impelementation for GetData and GetAllData BUG=672617 Review-Url: https://codereview.chromium.org/2582713003 ========== to ========== [USS] Impelementation for GetData and GetAllData BUG=672617 Committed: https://crrev.com/b5e5d4663754f905b2d1465c258ca1c21979c70a Cr-Commit-Position: refs/heads/master@{#440589} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b5e5d4663754f905b2d1465c258ca1c21979c70a Cr-Commit-Position: refs/heads/master@{#440589}
Message was sent while issue was closed.
https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2582713003/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:37: void VerifyEqual(const AutofillSpecifics& s1, const AutofillSpecifics& s2) { On 2016/12/19 17:40:09, skym wrote: > Can you just call SerializeAsString() on both and make sure the strings are > equal? Sorry I set a bad example in device info sync bridge unittest! I think I was wrong here to suggest this. Seeing two SerializeAsString() byte arrays is not useful when you have a failing test case. Sorry! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
