Skip managed bookmarks at the BookmarkChangeProcessor.

Skip managed bookmarks at the BookmarkChangeProcessor.

BUG=405075, 386145
Committed: https://crrev.com/0c637552fb1e1126ba5d8d9c08a50df194c21755
Cr-Commit-Position: refs/heads/master@{#294048}

[Joao da Silva, 2014-09-08 17:30:49]

joaodasilva@chromium.org changed reviewers:
+ rlarocque@chromium.org

[Joao da Silva, 2014-09-08 17:30:49]

Hi Richard, PTAL. This should avoid some DCHECKs and some spurious sync errors.

[Nicolas Zea, 2014-09-08 17:36:03]

zea@chromium.org changed reviewers:
+ zea@chromium.org

[Nicolas Zea, 2014-09-08 17:36:04]

FYI, Richard is no longer on sync (we probably need to update the owners file),
I'll review.

This patch seems to catch all the changes that happen after association. What is
the expected behavior at association time? Or are we already filtering managed
nodes at association time?

[Joao da Silva, 2014-09-08 21:17:25]

On 2014/09/08 17:36:04, Nicolas Zea wrote:
> FYI, Richard is no longer on sync (we probably need to update the owners
file),
> I'll review.
> 
> This patch seems to catch all the changes that happen after association. What
is
> the expected behavior at association time? Or are we already filtering managed
> nodes at association time?

Thanks Nicolas.

Managed bookmarks are never synced (their root node is never added to the
associator). But they do trigger events, and this is sometimes confusing the
bookmark_change_processor because it can't find a sync node for e.g. a bookmark
node that triggered BookmarkNodeChanged.

This hasn't been an issue because it mostly hits NOTREACHED() statements
followed by early returns, or generates spurious SyncErrors. This change should
fix that.

[Nicolas Zea, 2014-09-08 21:32:37]

Makes sense. Do you know how tough would it be to add an integration test for
this? Do we have any existing single client integration tests with managed
users?

[Joao da Silva, 2014-09-08 21:56:10]

On 2014/09/08 21:32:37, Nicolas Zea wrote:
> Makes sense. Do you know how tough would it be to add an integration test for
> this? Do we have any existing single client integration tests with managed
> users?

We have tons of unittests for enterprise, and a lot of integration tests too
(see cloud_policy_browsertest.cc & policy_browsertest.cc). However none of them
are testing managed bookmarks & sync.

I'm happy to extend a sync integration test to verify that managed bookmarks can
exist in the BookmarkModel but aren't getting synced. Can you give some
pointers?

[Joao da Silva, 2014-09-09 15:21:13]

Added a new test to two_client_bookmarks_sync_test.cc. This test should catch
regressions if a managed bookmark ever ends up in the sync model.

This required some adaptations:

- we can override a global policy provider for all profiles; I added new policy
code to override it for a single profile (otherwise the 2nd profile in the test
will see the same managed bookmarks)

- we want to skip the managed nodes when comparing bookmark models, and that
requires having a handle to the BookmarkModel in the prune function; adapter the
iterator to use a Callback so that we can bind this argument

PTAL, thanks!

[Nicolas Zea, 2014-09-09 18:07:15]

sync LGTM, thanks for adding the test!

[Joao da Silva, 2014-09-09 18:46:40]

joaodasilva@chromium.org changed reviewers:
+ sky@chromium.org

[Joao da Silva, 2014-09-09 18:46:41]

+Scott: please have a look at ui/ and components/bookmarks. Thanks!

[sky, 2014-09-09 19:23:55]

LGTM

[Joao da Silva, 2014-09-09 21:44:34]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-09-09 22:06:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/549193002/40001

[commit-bot: I haz the power, 2014-09-10 00:17:35]

Committed patchset #3 (id:40001) as a678c00dcf143d90f52f687ee74131cc18b48ced

[commit-bot: I haz the power, 2014-09-10 03:56:53]

Patchset 3 (id:??) landed as
https://crrev.com/0c637552fb1e1126ba5d8d9c08a50df194c21755
Cr-Commit-Position: refs/heads/master@{#294048}