[Sync] Add support for associating a new Synced Bookmarks node.

[Sync] Add support for associating a new Synced Bookmarks node.

Previously we would result in an unrecoverable error when the server sends
us a newly created Synced Bookmarks node. We now properly detect and associate
it, allowing us to also associate new bookmarks created under the Synced
Bookmarks folder.

In addition, we soften our conditions for unrecoverable errors. A bookmark
that cannot be added is ignored, instead of triggering an error. There are
very few of these, and are likely triggered by a crash during initial
association. 

BUG=108978
TEST=sync_integration_tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127101

[Nicolas Zea, 2012-03-02 01:09:41]

Raghu for tests, Tim for bookmark logic.

[Raghu Simha, 2012-03-02 04:24:29]

Test code LGTM.

http://codereview.chromium.org/9570055/diff/2001/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/bookmarks_helper.h (right):

http://codereview.chromium.org/9570055/diff/2001/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/bookmarks_helper.h:31: 
nit: add a brief comment here.

[Nicolas Zea, 2012-03-02 22:35:14]

https://chromiumcodereview.appspot.com/9570055/diff/2001/chrome/browser/sync/...
File chrome/browser/sync/test/integration/bookmarks_helper.h (right):

https://chromiumcodereview.appspot.com/9570055/diff/2001/chrome/browser/sync/...
chrome/browser/sync/test/integration/bookmarks_helper.h:31: 
On 2012/03/02 04:24:29, rsimha wrote:
> nit: add a brief comment here.

Done.

[Nicolas Zea, 2012-03-02 22:35:39]

+Nick for python changes

[tim (not reviewing), 2012-03-05 23:28:18]

LGTM

http://codereview.chromium.org/9570055/diff/12002/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/9570055/diff/12002/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:927: def
TriggerCreateSyncedBookmarks(self):
Could add a chromiumsync_test.py test for this?

[Nicolas Zea, 2012-03-06 20:42:32]

ping Nick?

https://chromiumcodereview.appspot.com/9570055/diff/12002/net/tools/testserve...
File net/tools/testserver/chromiumsync.py (right):

https://chromiumcodereview.appspot.com/9570055/diff/12002/net/tools/testserve...
net/tools/testserver/chromiumsync.py:927: def
TriggerCreateSyncedBookmarks(self):
On 2012/03/05 23:28:19, timsteele wrote:
> Could add a chromiumsync_test.py test for this?

Done.

[ncarter (slow), 2012-03-07 22:05:03]

http://codereview.chromium.org/9570055/diff/22023/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/9570055/diff/22023/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:930: Clients will then received the Synced
Bookmarks folder on future
"will then recieved"

http://codereview.chromium.org/9570055/diff/22023/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:938:
self._PERMANENT_ITEM_SPECS.append(synced_bookmarks_spec)
You are appending to a constant here. Anything in all caps should be regarded as
a constant. A second problem is that this function is non-idempotent, because
calling it twice would mean we wind up with two copies of the permanent item in
the list of specs.

Could we have the spec in the list permanently, and do some other filtering
(e.g, add a property to PermanentItem encapsulating the additional conditions
for creation)?

http://codereview.chromium.org/9570055/diff/22023/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:1092:
self.account.TriggerCreateSyncedBookmarks()
I'm assuming the answer is "yes but it was hard to initiate from the test", but
did you consider implementing this using the sync protocol, rather than a new
kind of sideband request?

By "using the sync protocol," I mean implementing the semantics for the
"include_syncable_bookmarks" in sync.proto.

http://codereview.chromium.org/9570055/diff/22023/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync_test.py (right):

http://codereview.chromium.org/9570055/diff/22023/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync_test.py:75: if self.model._ItemExists(
It seems circular to query the model from the test expectation. This will mask
bugs where the folder is created when it shouldn't be, or bugs where the folder
isn't created when it should be.

Instead, you might add a boolean property to SyncDataModelTest called
syncedBookmarksFolderShouldExist, and set it to True whenever the folder is
explicitly created.

[Nicolas Zea, 2012-03-15 21:22:49]

Comments addressed. PTAL

http://codereview.chromium.org/9570055/diff/22023/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/9570055/diff/22023/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:930: Clients will then received the Synced
Bookmarks folder on future
On 2012/03/07 22:05:03, ncarter wrote:
> "will then recieved"

Done.

http://codereview.chromium.org/9570055/diff/22023/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:938:
self._PERMANENT_ITEM_SPECS.append(synced_bookmarks_spec)
On 2012/03/07 22:05:03, ncarter wrote:
> You are appending to a constant here. Anything in all caps should be regarded
as
> a constant. A second problem is that this function is non-idempotent, because
> calling it twice would mean we wind up with two copies of the permanent item
in
> the list of specs.
> 
> Could we have the spec in the list permanently, and do some other filtering
> (e.g, add a property to PermanentItem encapsulating the additional conditions
> for creation)?

Done.

http://codereview.chromium.org/9570055/diff/22023/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:1092:
self.account.TriggerCreateSyncedBookmarks()
On 2012/03/07 22:05:03, ncarter wrote:
> I'm assuming the answer is "yes but it was hard to initiate from the test",
but
> did you consider implementing this using the sync protocol, rather than a new
> kind of sideband request?
> 
> By "using the sync protocol," I mean implementing the semantics for the
> "include_syncable_bookmarks" in sync.proto.

Discussed offline, went ahead and kept current approach.

http://codereview.chromium.org/9570055/diff/22023/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync_test.py (right):

http://codereview.chromium.org/9570055/diff/22023/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync_test.py:75: if self.model._ItemExists(
On 2012/03/07 22:05:03, ncarter wrote:
> It seems circular to query the model from the test expectation. This will mask
> bugs where the folder is created when it shouldn't be, or bugs where the
folder
> isn't created when it should be.
> 
> Instead, you might add a boolean property to SyncDataModelTest called
> syncedBookmarksFolderShouldExist, and set it to True whenever the folder is
> explicitly created.

Done.

[ncarter (slow), 2012-03-15 22:44:52]

lgtm

LGTM with nits.

http://codereview.chromium.org/9570055/diff/31003/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/9570055/diff/31003/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:945: spec.tag == "synced_bookmarks").next()
This is a pretty unidiomatic use of a generator expression.

The first improvement I'd suggest would be to use a list comprehension and not a
generator expression, unless there's a clear benefit to suspending the
evaluation. When you're doing a filtering operation like this, there's no harm
in having the loop evaluate completely.  But the style guide isn't explicit on
this, it's more of a personal preference. Here, it's your call.

Secondly, calling .next() on the result of a generator expression is a code
smell: comprehensions are syntactic sugar that are supposed to let your code
read naturally, and .next() breaks that sweet illusion. In essence you're
forcing me to remember that "the semantics of a generator expression are
equivalent to creating an anonymous generator function and calling it".  Don't
make me remember this.

Fortunately there's a good python idiom for unpacking lists.  You are probably
familiar with this:

 a, b = [1, 2]  # afterwards, a = 1 and b = 2

And this:
 a = [1]  # afterwards, a = [1]

But did you know about this?
 a, = [1]  # afterwards, a = 1

Thirdly, the convention I've seen for formatting multi-line generator
expressions is to prefer breaking the line before the keywords ("for" / "in" /
"if").  Reference:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Generators

http://codereview.chromium.org/9570055/diff/31003/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync_test.py (right):

http://codereview.chromium.org/9570055/diff/31003/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync_test.py:117:
self._expect_synced_bookmarks_folder else 0,
FWIW, I don't find this very readable -- it's almost an operator-precedence
puzzle.  I would probably just use an if/else pair and two asserts.

[commit-bot: I haz the power, 2012-03-15 23:09:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/9570055/28002

[commit-bot: I haz the power, 2012-03-15 23:58:38]

Commit queue patch verification failed without an error message.
Something went wrong, probably a crash, a hickup or simply
the monkeys went out for diner.
Ping the relevant dude on a on-needed basis.

[commit-bot: I haz the power, 2012-03-16 00:32:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/9570055/28002

[commit-bot: I haz the power, 2012-03-16 04:24:19]

Change committed as 127101