[USS] Write integration tests for USS autocomplete |
||
Issue descriptionThis includes adding the bot config! Don't forget.
,
Feb 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/08754c122f848c02be04f9858a623ec706fbca34 commit 08754c122f848c02be04f9858a623ec706fbca34 Author: skym <skym@chromium.org> Date: Thu Feb 02 16:38:55 2017 [Sync] Incorporate value into USS AUTOFILL non_unique_name. The directory based AUTOFILL implemented used to set autofill_entry|name|value as the non_unique_name. During the USS rewrite this was changed to just be the name of the form field. This is less helpful when looking at sync-internals, you may have many entries for the same form with different values. It would be nice if this name distinguished them. Thinking about the cost of increasing non_unique_name values, it directly impacts bandwidth usage, being part of commits and GUs, though it does get compressed. We don't seem to keep this value in memory at runtime when steady state, which is really the most important scenario. BUG= 686172 Review-Url: https://codereview.chromium.org/2663423002 Cr-Commit-Position: refs/heads/master@{#447776} [modify] https://crrev.com/08754c122f848c02be04f9858a623ec706fbca34/components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc
,
Feb 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c3389b63ea8217144db344e29451b9ee3bbd05e3 commit c3389b63ea8217144db344e29451b9ee3bbd05e3 Author: skym <skym@chromium.org> Date: Thu Feb 02 16:56:58 2017 [Sync] Only get bridge on model thread for ModelTypeController. AutocompleteSyncBridge cannot be fetched on the UI thread, because of the way AutofillWebDataService provides it. This can only be done on the model thread. Previously the ModelTypeController was updated to mostly respect this in https://codereview.chromium.org/2508263003/ . However, we seem to have missed the DisableSync() case. This CL copies the existing pattern for this scenario. BUG= 686172 Review-Url: https://codereview.chromium.org/2668323002 Cr-Commit-Position: refs/heads/master@{#447779} [modify] https://crrev.com/c3389b63ea8217144db344e29451b9ee3bbd05e3/components/sync/driver/model_type_controller.cc
,
Feb 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d09c38982b3b999ecfa67acfb8018d0b198bc4bd commit d09c38982b3b999ecfa67acfb8018d0b198bc4bd Author: skym <skym@chromium.org> Date: Fri Feb 03 22:09:02 2017 [Sync] Make EnableDisableSingleClientTest more robust. EnableDisableSingleClientTest's tests enable or disable a type, and then go verify that the root node was or was not present. This doesn't work for USS, as USS doesn't create root nodes. So ever time we migrate a USS type we have to update this file. However, this was a difficult task because of the way it was set up. Full or hard coded rules about relationships, and dependent on specific orderings of ModelType. The most egregious violation being it would call enable/disable on non-user selectable types, this would no-op, and then it'd verify it had the correct affect, because typically these grouped types are preceded by their corresponding user selectable type, and this test went through the types in order. To fix this, we start by making the test harness return false (failure) if you try to enable/disable a non-selectable type. This in turns means that EnableDisableSingleClientTest's cases need to understand some of this type group mapping. So SyncPrefs was updated to expose ResolvePrefGroups to allow intelligent tests. Not only does this allow us to remove most hard coded type rules, but we are also able to test on a more precise level, knowing typically exactly which types should change as a result of an enable/disable. The main caveat being types that are mapped to by multiple selectable types, which we're slightly less strictly testing when they change. Also updated test to guess that any new type of syncer::PRINTERS or later will be USS. This strives to reduce the pain of adding new types, although it is a risk in that the logic is fragile. BUG= 686172 Review-Url: https://codereview.chromium.org/2670903002 Cr-Commit-Position: refs/heads/master@{#448085} [modify] https://crrev.com/d09c38982b3b999ecfa67acfb8018d0b198bc4bd/chrome/browser/sync/test/integration/enable_disable_test.cc [modify] https://crrev.com/d09c38982b3b999ecfa67acfb8018d0b198bc4bd/chrome/browser/sync/test/integration/profile_sync_service_harness.cc [modify] https://crrev.com/d09c38982b3b999ecfa67acfb8018d0b198bc4bd/components/sync/base/sync_prefs.h
,
Feb 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6261bc1e8181556af9c67ad860cca8ebfc3469e0 commit 6261bc1e8181556af9c67ad860cca8ebfc3469e0 Author: skym <skym@chromium.org> Date: Wed Feb 15 17:26:58 2017 [Sync] Switch bots to run integ tests for autofill as USS. ModelTypeControllers are responsible for posting to the model thread. They also typically grab the bridge or the processor through the bridge when doing this. UI thread based controllers can simply go through their SyncClient to do this, and all is well. However, for autofill, and any WebDataService backed type, they cannot, they can only grab the weak ref to their bridge on their model thread. Initially, we were simply posting a task to the model thread that contained a SyncClient pointer. This turns out to not be safe. During shutdown, autofill would consistently crash here by posting a task to the model thread, and before that gets resolved, the SyncClient is destroyed. Presumably all the other posts to model thread were unsafe as well, though hitting the race condition was more unlikely. To get around this we now post a task with a ref counted WebDataService. This required an ability to override the default handling of the ModelTypeController, and any WebDataService backed controller should use the new class created here. While I was in there, some slight refactoring to ModelTypeController and ModelTypeDebugInfo to make the other code changes as clean as possible. BUG= 686172 Review-Url: https://codereview.chromium.org/2672493002 Cr-Commit-Position: refs/heads/master@{#450736} [modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/chrome/browser/sync/chrome_sync_client.cc [modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/chrome/browser/sync/test/integration/enable_disable_test.cc [modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/autofill/core/browser/BUILD.gn [modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc [modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/autofill/core/browser/webdata/autocomplete_sync_bridge.h [add] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/autofill/core/browser/webdata/web_data_model_type_controller.cc [add] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/autofill/core/browser/webdata/web_data_model_type_controller.h [modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/browser_sync/profile_sync_components_factory_impl.cc [modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/sync/driver/model_type_controller.cc [modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/sync/driver/model_type_controller.h [modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/sync/model/model_type_debug_info.cc [modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/sync/model/model_type_debug_info.h [modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/testing/variations/fieldtrial_testing_config.json
,
Feb 22 2017
This is done, marking a such.
,
Apr 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/903ebf3235f4057351f7734c0e148e7938009dbd commit 903ebf3235f4057351f7734c0e148e7938009dbd Author: skym <skym@chromium.org> Date: Mon Apr 10 17:59:38 2017 [Sync] Post back to UI thread for USS GetAllNodes. When running GetAllNodes, USS types post to the model thread to access their processor/bridge. This means after they've gathered the nodes, they need to return to the UI thread to give the results back to the GetAllNodesRequestHelper. Unfortunately I removed a wrapping BindToCurrentThread call in https://codereview.chromium.org/2672493002/diff/80001/components/sync/driver/model_type_controller.cc that did the jump back to the UI thread. This meant that non-UI USS types (namely AUTOFILL) were updating GetAllNodesRequestHelper from a different thread, through very non-thread safe code. To fix simply added the BindToCurrentThread back. Also added a added a thread checker to GetAllNodesRequestHelper in hopes of stopping similar bugs from occurring again. BUG= 686172 Review-Url: https://codereview.chromium.org/2808113003 Cr-Commit-Position: refs/heads/master@{#463320} [modify] https://crrev.com/903ebf3235f4057351f7734c0e148e7938009dbd/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/903ebf3235f4057351f7734c0e148e7938009dbd/components/sync/driver/model_type_controller.cc |
||
►
Sign in to add a comment |
||
Comment 1 by s...@chromium.org
, Feb 1 2017