Bring up of the enhanced bookmarks cluster service.

Bring up of the enhanced bookmarks cluster service.

This service retrieves the cluster of bookmarks from the bookmark
server.

BUG=None

Committed: https://crrev.com/6bb374fc58bfeeb7966b6a548b1c7b41af9c8e47
Cr-Commit-Position: refs/heads/master@{#298582}

Committed: https://crrev.com/daaac3af72f231dec2df4dc566b84356d3dc10df
Cr-Commit-Position: refs/heads/master@{#298682}

[noyau (Ping after 24h), 2014-09-04 21:56:05]

noyau@chromium.org changed reviewers:
+ blundell@chromium.org, yfriedman@chromium.org

[Yaron, 2014-09-05 05:17:59]

yfriedman@chromium.org changed reviewers:
+ mcolbert@chromium.org

[Yaron, 2014-09-05 05:18:01]

https://codereview.chromium.org/539173004/diff/20001/chrome/browser/enhanced_...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc
(right):

https://codereview.chromium.org/539173004/diff/20001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:40:
void ChromeBookmarkServerClusterService::OnSyncCycleCompleted() {
This seems a bit aggressive. I think sync cycles are too common (not to mention
that you can have multiple sync cycles in a row) to issue requests each time.
Can you not use BookmarkModelObserver instead?

Super nit: if you are keeping it, just  move the impl to |OnStateChanged| and
remove the override of this function.

https://codereview.chromium.org/539173004/diff/20001/chrome/browser/enhanced_...
File
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service_factory.cc
(right):

https://codereview.chromium.org/539173004/diff/20001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service_factory.cc:21:
ChromeBookmarkServerClusterServiceFactory::GetInstance() {
Any reason you included the factory here but not for the search service in
https://codereview.chromium.org/538903003 ?

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
File components/enhanced_bookmarks.gypi (right):

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
components/enhanced_bookmarks.gypi:81: 'target_name':
'enhanced_bookmarks_proto',
update gn build as well

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_cluster_service.cc (right):

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:25:
"ios_pref_service_key_for_stored_bookmark_clusters";
nit: not ios specific. changing means clients will dump clusters on the first
load but they'll get rebuilt.

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:190: void
BookmarkServerClusterService::BookmarkNodeAdded(BookmarkModel* model,
these overrides seem unnecessary unless you take my other suggestion of not
triggering based on sync cycles. In either case, we should probably add
throttling to avoid issuing too many requests in a short time period.

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:239:
cluster_data_.swap(loaded_data);
if we failed to deserialize, issue a request to get new clusters?

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_cluster_service.h (right):

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:17: class
BookmarkServerClusterClient;
unused

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:21: // Manages
requests to the bookmark server to do just-in-time requests.
seems out of date. Should just talk about clustering.

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
File components/enhanced_bookmarks/proto/cluster.proto (right):

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
components/enhanced_bookmarks/proto/cluster.proto:38: // TODO(kej): Reevaluate
parameters after any major algorithm or data changes.
remove this and revise other comments to not reference servers

[blundell, 2014-09-05 09:10:44]

I defer to the others for the review here.

[noyau (Ping after 24h), 2014-09-10 08:53:57]

https://codereview.chromium.org/539173004/diff/20001/chrome/browser/enhanced_...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc
(right):

https://codereview.chromium.org/539173004/diff/20001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:40:
void ChromeBookmarkServerClusterService::OnSyncCycleCompleted() {
On 2014/09/05 05:18:00, Yaron wrote:
> This seems a bit aggressive. I think sync cycles are too common (not to
mention
> that you can have multiple sync cycles in a row) to issue requests each time.
> Can you not use BookmarkModelObserver instead?
> 
> Super nit: if you are keeping it, just  move the impl to |OnStateChanged| and
> remove the override of this function.
This class originally used the model notification, and those happened way to
early. Doing it after a sync cycle at least guarantees that the data made it
onto the server. for that reason OnSyncCycleCompleted seems the right choice.

Also sync doesn't bracket the changes it does with ScopedGroupBookmarkAction,
this means that there is no way to know when a sync is done without using this
call (crbug/372432)

https://codereview.chromium.org/539173004/diff/20001/chrome/browser/enhanced_...
File
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service_factory.cc
(right):

https://codereview.chromium.org/539173004/diff/20001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service_factory.cc:21:
ChromeBookmarkServerClusterServiceFactory::GetInstance() {
On 2014/09/05 05:18:00, Yaron wrote:
> Any reason you included the factory here but not for the search service in
> https://codereview.chromium.org/538903003 ?

Because the other one doesn't keep state, so doesn't need to be unique or
persistent, and as such is not a KeyedService, just a class you instantiate to
perform a search and release when you are done.

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
File components/enhanced_bookmarks.gypi (right):

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
components/enhanced_bookmarks.gypi:81: 'target_name':
'enhanced_bookmarks_proto',
On 2014/09/05 05:18:00, Yaron wrote:
> update gn build as well

Done.

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_cluster_service.cc (right):

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:25:
"ios_pref_service_key_for_stored_bookmark_clusters";
On 2014/09/05 05:18:01, Yaron wrote:
> nit: not ios specific. changing means clients will dump clusters on the first
> load but they'll get rebuilt.

Done.

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:239:
cluster_data_.swap(loaded_data);
On 2014/09/05 05:18:01, Yaron wrote:
> if we failed to deserialize, issue a request to get new clusters?

I didn't on purpose, to avoid looping over and over on bad results. A request
will be done next time a bookmark is created or modifed, so it will eventually
retrigger.

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_cluster_service.h (right):

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:17: class
BookmarkServerClusterClient;
On 2014/09/05 05:18:01, Yaron wrote:
> unused

Done.

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:21: // Manages
requests to the bookmark server to do just-in-time requests.
On 2014/09/05 05:18:01, Yaron wrote:
> seems out of date. Should just talk about clustering.

Done.

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
File components/enhanced_bookmarks/proto/cluster.proto (right):

https://codereview.chromium.org/539173004/diff/20001/components/enhanced_book...
components/enhanced_bookmarks/proto/cluster.proto:38: // TODO(kej): Reevaluate
parameters after any major algorithm or data changes.
On 2014/09/05 05:18:01, Yaron wrote:
> remove this and revise other comments to not reference servers

Done.

[chromium-reviews, 2014-09-12 01:44:28]

Sorry I didn't get back to this today and am heading out for a few days.
Kibeom: can you review?

On Wednesday, September 10, 2014, <noyau@chromium.org> wrote:

>
> https://codereview.chromium.org/539173004/diff/20001/
> chrome/browser/enhanced_bookmarks/chrome_bookmark_
> server_cluster_service.cc
> File
> chrome/browser/enhanced_bookmarks/chrome_bookmark_
> server_cluster_service.cc
> (right):
>
> https://codereview.chromium.org/539173004/diff/20001/
> chrome/browser/enhanced_bookmarks/chrome_bookmark_
> server_cluster_service.cc#newcode40
> chrome/browser/enhanced_bookmarks/chrome_bookmark_
> server_cluster_service.cc:40:
> void ChromeBookmarkServerClusterService::OnSyncCycleCompleted() {
> On 2014/09/05 05:18:00, Yaron wrote:
>
>> This seems a bit aggressive. I think sync cycles are too common (not
>>
> to mention
>
>> that you can have multiple sync cycles in a row) to issue requests
>>
> each time.
>
>> Can you not use BookmarkModelObserver instead?
>>
>
>  Super nit: if you are keeping it, just  move the impl to
>>
> |OnStateChanged| and
>
>> remove the override of this function.
>>
> This class originally used the model notification, and those happened
> way to early. Doing it after a sync cycle at least guarantees that the
> data made it onto the server. for that reason OnSyncCycleCompleted seems
> the right choice.
>
> Also sync doesn't bracket the changes it does with
> ScopedGroupBookmarkAction, this means that there is no way to know when
> a sync is done without using this call (crbug/372432)
>
> https://codereview.chromium.org/539173004/diff/20001/
> chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service_
> factory.cc
> File
> chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service_
> factory.cc
> (right):
>
> https://codereview.chromium.org/539173004/diff/20001/
> chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service_
> factory.cc#newcode21
> chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service_
> factory.cc:21:
> ChromeBookmarkServerClusterServiceFactory::GetInstance() {
> On 2014/09/05 05:18:00, Yaron wrote:
>
>> Any reason you included the factory here but not for the search
>>
> service in
>
>> https://codereview.chromium.org/538903003 ?
>>
>
> Because the other one doesn't keep state, so doesn't need to be unique
> or persistent, and as such is not a KeyedService, just a class you
> instantiate to perform a search and release when you are done.
>
> https://codereview.chromium.org/539173004/diff/20001/
> components/enhanced_bookmarks.gypi
> File components/enhanced_bookmarks.gypi (right):
>
> https://codereview.chromium.org/539173004/diff/20001/
> components/enhanced_bookmarks.gypi#newcode81
> components/enhanced_bookmarks.gypi:81: 'target_name':
> 'enhanced_bookmarks_proto',
> On 2014/09/05 05:18:00, Yaron wrote:
>
>> update gn build as well
>>
>
> Done.
>
> https://codereview.chromium.org/539173004/diff/20001/
> components/enhanced_bookmarks/bookmark_server_cluster_service.cc
> File components/enhanced_bookmarks/bookmark_server_cluster_service.cc
> (right):
>
> https://codereview.chromium.org/539173004/diff/20001/
> components/enhanced_bookmarks/bookmark_server_cluster_service.cc#newcode25
> components/enhanced_bookmarks/bookmark_server_cluster_service.cc:25:
> "ios_pref_service_key_for_stored_bookmark_clusters";
> On 2014/09/05 05:18:01, Yaron wrote:
>
>> nit: not ios specific. changing means clients will dump clusters on
>>
> the first
>
>> load but they'll get rebuilt.
>>
>
> Done.
>
> https://codereview.chromium.org/539173004/diff/20001/
> components/enhanced_bookmarks/bookmark_server_cluster_
> service.cc#newcode239
> components/enhanced_bookmarks/bookmark_server_cluster_service.cc:239:
> cluster_data_.swap(loaded_data);
> On 2014/09/05 05:18:01, Yaron wrote:
>
>> if we failed to deserialize, issue a request to get new clusters?
>>
>
> I didn't on purpose, to avoid looping over and over on bad results. A
> request will be done next time a bookmark is created or modifed, so it
> will eventually retrigger.
>
> https://codereview.chromium.org/539173004/diff/20001/
> components/enhanced_bookmarks/bookmark_server_cluster_service.h
> File components/enhanced_bookmarks/bookmark_server_cluster_service.h
> (right):
>
> https://codereview.chromium.org/539173004/diff/20001/
> components/enhanced_bookmarks/bookmark_server_cluster_service.h#newcode17
> components/enhanced_bookmarks/bookmark_server_cluster_service.h:17:
> class BookmarkServerClusterClient;
> On 2014/09/05 05:18:01, Yaron wrote:
>
>> unused
>>
>
> Done.
>
> https://codereview.chromium.org/539173004/diff/20001/
> components/enhanced_bookmarks/bookmark_server_cluster_service.h#newcode21
> components/enhanced_bookmarks/bookmark_server_cluster_service.h:21: //
> Manages requests to the bookmark server to do just-in-time requests.
> On 2014/09/05 05:18:01, Yaron wrote:
>
>> seems out of date. Should just talk about clustering.
>>
>
> Done.
>
> https://codereview.chromium.org/539173004/diff/20001/
> components/enhanced_bookmarks/proto/cluster.proto
> File components/enhanced_bookmarks/proto/cluster.proto (right):
>
> https://codereview.chromium.org/539173004/diff/20001/
> components/enhanced_bookmarks/proto/cluster.proto#newcode38
> components/enhanced_bookmarks/proto/cluster.proto:38: // TODO(kej):
> Reevaluate parameters after any major algorithm or data changes.
> On 2014/09/05 05:18:01, Yaron wrote:
>
>> remove this and revise other comments to not reference servers
>>
>
> Done.
>
> https://codereview.chromium.org/539173004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[noyau (Ping after 24h), 2014-09-12 11:01:18]

noyau@chromium.org changed reviewers:
+ battre@chromium.org, kkimlabs@chromium.org

[noyau (Ping after 24h), 2014-09-12 11:01:19]

+kkimlabs@ for review in Yaron absence
+battre@ for the changes in browser_prefs.cc

[battre, 2014-09-12 16:49:02]

Hi.

I had literally just 5 minutes to look into this. (in an all day even today).

My general feeling is that you could simplify this significantly by splitting
the one preference into multiple preferences.

I did not have sufficient time to look into this but wanted to give quick
feedback.

Best regards,
Dominic

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_cluster_service.cc (right):

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:25:
"pref_service_key_for_stored_bookmark_clusters";
Please use a different key. E.g. "stored_bookmark_clusters"

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:111:
kPrefServiceKey,
Please follow the style of e.g. components/component_updater/pref_names.h and
put the preference into a file pair pref_names.*, use a nameservice, add
documentation, etc.

[Kibeom Kim (inactive), 2014-09-12 21:31:25]

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_cluster_service.cc (right):

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:101: for (auto
pair : cluster_data_)
I'm not sure if this is changed, but people on chromium-dev once told not to use
c++11 things yet few weeks ago.
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/A5lr2jKAIaI/I2ZDw...

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:153: if
(!title.length())
.empty()? (also #157 and #160)

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:167:
cluster_data_.begin())) {
can we use std:map's operator == here? (I think the result should be same,
though unsure if there is some performance difference)

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:189: void
BookmarkServerClusterService::BookmarkNodeAdded(BookmarkModel* model,
Q: do we need BookmarkNodeAdded and BookmarkMetaInfoChanged functions here? It
seems we can omit them and let them fall back to BookmarkServerService functions
directly.

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:227:
pref_service_->SetString(kPrefServiceKey, serialized);
Q: looks like this line won't be called on ::GoogleSignedOut, is it ok to leave
kPrefServiceKey perf key ?

[noyau (Ping after 24h), 2014-09-18 12:35:32]

Patchset #5 (id:80001) has been deleted

[noyau (Ping after 24h), 2014-09-18 12:41:48]

PTAL. Sorry it took so long to get back to this.

On 2014/09/12 16:49:02, battre wrote:
> My general feeling is that you could simplify this significantly by splitting
> the one preference into multiple preferences.
> 
I'd rather keep it as one preference, so we are sure that they can't get out of
sync. It's rather important that both the version number and the auth_id are
kept with the data. Also changing the data would require doing an upgrade of the
existing flags as we've been using this code on iOS and Android for a while now,
so if you insist that we should do it I'd rather do this upgrade in a separate
CL if possible.

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_cluster_service.cc (right):

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:25:
"pref_service_key_for_stored_bookmark_clusters";
On 2014/09/12 16:49:02, battre wrote:
> Please use a different key. E.g. "stored_bookmark_clusters"

Done. And moved to pref_names.

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:101: for (auto
pair : cluster_data_)
On 2014/09/12 21:31:25, Kibeom Kim wrote:
> I'm not sure if this is changed, but people on chromium-dev once told not to
use
> c++11 things yet few weeks ago.
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/A5lr2jKAIaI/I2ZDw...

Yes you're right. Porting code from iOS I'm always caught by that one (not to
mention that the bots all compiled this code with no problem!). Fixed, I removed
all the auto and C++11 for loops from the file.

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:111:
kPrefServiceKey,
On 2014/09/12 16:49:01, battre wrote:
> Please follow the style of e.g. components/component_updater/pref_names.h and
> put the preference into a file pair pref_names.*, use a nameservice, add
> documentation, etc.

Done.

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:153: if
(!title.length())
On 2014/09/12 21:31:24, Kibeom Kim wrote:
> .empty()? (also #157 and #160)

Done.

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:167:
cluster_data_.begin())) {
On 2014/09/12 21:31:24, Kibeom Kim wrote:
> can we use std:map's operator == here? (I think the result should be same,
> though unsure if there is some performance difference)
Excellent question. According to the spec std::map does not implement operator
==, but it compiles on iOS, so obviously something implements it. The semantics
of std::equals are clear, and I can't find documentation on the semantics of ==
for std::map, so in doubt, I'd rather keep that code as is: I know it works,
we've been using it for months on iOS.

Performance there is quite irrelevant anyway, as this only happens following a
user action.

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:189: void
BookmarkServerClusterService::BookmarkNodeAdded(BookmarkModel* model,
On 2014/09/12 21:31:25, Kibeom Kim wrote:
> Q: do we need BookmarkNodeAdded and BookmarkMetaInfoChanged functions here? It
> seems we can omit them and let them fall back to BookmarkServerService
functions
> directly.

Done.

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:227:
pref_service_->SetString(kPrefServiceKey, serialized);
On 2014/09/12 21:31:25, Kibeom Kim wrote:
> Q: looks like this line won't be called on ::GoogleSignedOut, is it ok to
leave
> kPrefServiceKey perf key ?
The key will be there, with the proper version number, but with empty content.
So it is fine.  The only case where this would fail is if Serialize returns
false, and this actually can't happen! Fixing the function signature.

[Mark, 2014-09-18 18:08:52]

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_cluster_service.cc (right):

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:123: url =
net::AppendQueryParameter(url, "hl", application_language_code_);
You are also going to need to specify the version parameter (v=[version tag])

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
File components/enhanced_bookmarks/proto/cluster.proto (right):

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/proto/cluster.proto:20: optional HypnoOptions
options = 2;
Consider using "extensions 2;" here to denote this field is reserved without
needing to bring in any of the HypnoOptions.

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/proto/cluster.proto:37: message HypnoOptions {
You cannot set the HypnoOptions when querying GWS, so I'm not sure if we need to
include this right now.

[Mark, 2014-09-18 18:10:43]

Sorry for all these slow updates.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
File components/enhanced_bookmarks/bookmark_server_cluster_service.cc (right):

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:268:
base::JSONReader reader;
Any reason why you would serialize this down to JSON instead of just serializing
the protobufs directly?

[Yaron, 2014-09-23 17:08:28]

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

[Yaron, 2014-09-23 17:08:30]

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
File components/enhanced_bookmarks/proto/cluster.proto (right):

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/proto/cluster.proto:37: message HypnoOptions {
On 2014/09/18 18:08:52, Mark wrote:
> You cannot set the HypnoOptions when querying GWS, so I'm not sure if we need
to
> include this right now.

Sounds good to me. Let's keep this minimal and remove

https://codereview.chromium.org/539173004/diff/100001/chrome/browser/enhanced...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc
(right):

https://codereview.chromium.org/539173004/diff/100001/chrome/browser/enhanced...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:40:
void ChromeBookmarkServerClusterService::OnSyncCycleCompleted() {
So per previous comment I think this still happens too often to issue a request
every time. You could add a delay/throttling or maybe there's a better option
for deferring the cluster request.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
File components/enhanced_bookmarks/bookmark_server_cluster_service.cc (right):

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:69: std::string
stars_id = *it;
star_id

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:80: std::string
starid = RemoteIDForBookmark(bookmark);
star_id

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:268:
base::JSONReader reader;
On 2014/09/18 18:10:43, Mark wrote:
> Any reason why you would serialize this down to JSON instead of just
serializing
> the protobufs directly?

+1

[Yaron, 2014-09-23 17:11:21]

https://codereview.chromium.org/539173004/diff/100001/chrome/browser/enhanced...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc
(right):

https://codereview.chromium.org/539173004/diff/100001/chrome/browser/enhanced...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:40:
void ChromeBookmarkServerClusterService::OnSyncCycleCompleted() {
On 2014/09/23 17:08:30, Yaron wrote:
> So per previous comment I think this still happens too often to issue a
request
> every time. You could add a delay/throttling or maybe there's a better option
> for deferring the cluster request.

Err +Nicolas to comment

[battre, 2014-09-26 14:33:06]

https://codereview.chromium.org/539173004/diff/100001/chrome/browser/enhanced...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h
(right):

https://codereview.chromium.org/539173004/diff/100001/chrome/browser/enhanced...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h:20:
const std::string& application_language_code,
nit: add a comment on this? What is an application_language_code? E.g. for
Germany would it be "de", "de_DE", "GER"?

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
File components/enhanced_bookmarks/bookmark_server_cluster_service.cc (right):

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:69: std::string
stars_id = *it;
const std::string& star_id = *it; ?

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:214:
BookmarkServerClusterService::Serialize(cluster_data_, &serialized, auth_id);
Nit: Do you need the BookmarkServerClusterService:: prefix?

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:215:
pref_service_->SetString(prefs::kBookmarkClusters, serialized);
Why do you store this as a string instead of a dictionary?

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:237:
base::ListValue* all_clusters = new base::ListValue;
scoped_ptr?

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:243:
base::ListValue* cluster = new base::ListValue;
scoped_ptr?

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:254:
data.SetString(kAuthIdKey, auth_id);
Why don't you just return data?

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:268:
base::JSONReader reader;
On 2014/09/23 17:08:30, Yaron wrote:
> On 2014/09/18 18:10:43, Mark wrote:
> > Any reason why you would serialize this down to JSON instead of just
> serializing
> > the protobufs directly?
> 
> +1

I think this is used only for the pref service. There is no need to serialize it
at all. Just store it as a dictionary.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:277: if
(!result)
nit: I would inline all of these
if (!value->GetAsDictionary(&dictionary_value))
  return false;

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:303:
base::ListValue* cluster;
base::ListValue* cluster = NULL;

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
File components/enhanced_bookmarks/bookmark_server_cluster_service.h (right):

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:22: // state.
Add a description of what a cluster and cluster name is?

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:27: typedef
std::map<std::string, std::vector<std::string> > ClusterMap;
describe key and value?

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:29: const
std::string& application_language_code,
describe parameter? (see above)

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:37: // Retrieves
all the bookmarks associated with a cluster.
Comment on ownership of BookmarkNodes?

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:51: protected:
// BookmarkServerService methods.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:59: //
BookmarkModelObserver methods.
I can't find this inheritance.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:85: // profile
prefs is always up to date.
What's the cluster_map parameter?

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:96: const
std::string& auth_id);
make the output parameter the last one? (same for Deserialize)

what's auth_id?

[noyau (Ping after 24h), 2014-09-26 15:21:51]

Thanks for all the comments. This code has rotten a bit with its superclass
changing quite a bit. I need to fix this, and then change the way the values are
archived in the user prefs (storing the proto directly is quite obvious).

Stay tuned for an update early next week.

[battre, 2014-09-26 15:28:16]

On 2014/09/26 15:21:51, noyau wrote:
> Thanks for all the comments. This code has rotten a bit with its superclass
> changing quite a bit. I need to fix this, and then change the way the values
are
> archived in the user prefs (storing the proto directly is quite obvious).
> 
> Stay tuned for an update early next week.

I am not sure that storing a proto in the user preferences is a good idea.

[Mark, 2014-09-26 19:17:19]

https://codereview.chromium.org/539173004/diff/100001/chrome/browser/enhanced...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h
(right):

https://codereview.chromium.org/539173004/diff/100001/chrome/browser/enhanced...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h:20:
const std::string& application_language_code,
On 2014/09/26 14:33:04, battre wrote:
> nit: add a comment on this? What is an application_language_code? E.g. for
> Germany would it be "de", "de_DE", "GER"?

Eventually we should be ISO 639-6 compliant, but I think for now we can just use
BCP47 since both will be readable by the LanguageCode processor server-side.

[Kibeom Kim (inactive), 2014-09-29 21:15:56]

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_cluster_service.cc (right):

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:167:
cluster_data_.begin())) {
On 2014/09/18 12:41:47, noyau wrote:
> On 2014/09/12 21:31:24, Kibeom Kim wrote:
> > can we use std:map's operator == here? (I think the result should be same,
> > though unsure if there is some performance difference)
> Excellent question. According to the spec std::map does not implement operator
> ==, but it compiles on iOS, so obviously something implements it. The
semantics
> of std::equals are clear, and I can't find documentation on the semantics of
==
> for std::map, so in doubt, I'd rather keep that code as is: I know it works,
> we've been using it for months on iOS.
> 
> Performance there is quite irrelevant anyway, as this only happens following a
> user action.

I agree that we don't need to change already working code. :)

Just for reference: http://en.cppreference.com/w/cpp/container/map/operator_cmp

[noyau (Ping after 24h), 2014-09-30 14:43:02]

On 2014/09/26 15:28:16, battre wrote:
> On 2014/09/26 15:21:51, noyau wrote:
> > Thanks for all the comments. This code has rotten a bit with its superclass
> > changing quite a bit. I need to fix this, and then change the way the values
> are
> > archived in the user prefs (storing the proto directly is quite obvious).
> > 
> > Stay tuned for an update early next week.
> 
> I am not sure that storing a proto in the user preferences is a good idea.

Can you expand on that? Why do you think it is a bad idea?

I'm on the archiving code right now, and storing the proto directly simplifies
the class quite a bit (no need to encode anymore, just save the proto as
received from the server; only one decoder, from proto to the needed map). This
also include the versioning, as proto are designed for that purpose. Coupled
with removing the preference on sign out event, this get rid of the dictionary
all together.

-- Eric

[battre, 2014-09-30 14:53:47]

On 2014/09/30 14:43:02, noyau wrote:
> On 2014/09/26 15:28:16, battre wrote:
> > On 2014/09/26 15:21:51, noyau wrote:
> > > Thanks for all the comments. This code has rotten a bit with its
superclass
> > > changing quite a bit. I need to fix this, and then change the way the
values
> > are
> > > archived in the user prefs (storing the proto directly is quite obvious).
> > > 
> > > Stay tuned for an update early next week.
> > 
> > I am not sure that storing a proto in the user preferences is a good idea.
> 
> Can you expand on that? Why do you think it is a bad idea?
> 
> I'm on the archiving code right now, and storing the proto directly simplifies
> the class quite a bit (no need to encode anymore, just save the proto as
> received from the server; only one decoder, from proto to the needed map).
This
> also include the versioning, as proto are designed for that purpose. Coupled
> with removing the preference on sign out event, this get rid of the dictionary
> all together.
> 
> -- Eric

I think that this would be the first data entry that dumps binary data into our
JSON file. I think this is setting a negative precedent in that the file should
be human readable. Otherwise, we could have used sqlite for the preferences.

Best regards,
Dominic

[Mark, 2014-09-30 16:02:24]

On 2014/09/30 14:53:47, battre wrote:
> On 2014/09/30 14:43:02, noyau wrote:
> > On 2014/09/26 15:28:16, battre wrote:
> > > On 2014/09/26 15:21:51, noyau wrote:
> > > > Thanks for all the comments. This code has rotten a bit with its
> superclass
> > > > changing quite a bit. I need to fix this, and then change the way the
> values
> > > are
> > > > archived in the user prefs (storing the proto directly is quite
obvious).
> > > > 
> > > > Stay tuned for an update early next week.
> > > 
> > > I am not sure that storing a proto in the user preferences is a good idea.
> > 
> > Can you expand on that? Why do you think it is a bad idea?
> > 
> > I'm on the archiving code right now, and storing the proto directly
simplifies
> > the class quite a bit (no need to encode anymore, just save the proto as
> > received from the server; only one decoder, from proto to the needed map).
> This
> > also include the versioning, as proto are designed for that purpose. Coupled
> > with removing the preference on sign out event, this get rid of the
dictionary
> > all together.
> > 
> > -- Eric
> 
> I think that this would be the first data entry that dumps binary data into
our
> JSON file. I think this is setting a negative precedent in that the file
should
> be human readable. Otherwise, we could have used sqlite for the preferences.
> 
> Best regards,
> Dominic

Maybe SQLite is the route we should use then.  It seems cumbersome to have to
keep another translation layer solely for the purpose of precedence.  Overall,
changing the protobuf between server and client code is fairly cumbersome. 
Adding even more layers of translation to add a new feature seems like a lot of
future work.  In any case, I am not an OWNER in Chrome, so your call.

Cheers,
Mark

[Nicolas Zea, 2014-09-30 22:01:25]

On 2014/09/30 16:02:24, Mark wrote:
> On 2014/09/30 14:53:47, battre wrote:
> > On 2014/09/30 14:43:02, noyau wrote:
> > > On 2014/09/26 15:28:16, battre wrote:
> > > > On 2014/09/26 15:21:51, noyau wrote:
> > > > > Thanks for all the comments. This code has rotten a bit with its
> > superclass
> > > > > changing quite a bit. I need to fix this, and then change the way the
> > values
> > > > are
> > > > > archived in the user prefs (storing the proto directly is quite
> obvious).
> > > > > 
> > > > > Stay tuned for an update early next week.
> > > > 
> > > > I am not sure that storing a proto in the user preferences is a good
idea.
> > > 
> > > Can you expand on that? Why do you think it is a bad idea?
> > > 
> > > I'm on the archiving code right now, and storing the proto directly
> simplifies
> > > the class quite a bit (no need to encode anymore, just save the proto as
> > > received from the server; only one decoder, from proto to the needed map).
> > This
> > > also include the versioning, as proto are designed for that purpose.
Coupled
> > > with removing the preference on sign out event, this get rid of the
> dictionary
> > > all together.
> > > 
> > > -- Eric
> > 
> > I think that this would be the first data entry that dumps binary data into
> our
> > JSON file. I think this is setting a negative precedent in that the file
> should
> > be human readable. Otherwise, we could have used sqlite for the preferences.
> > 
> > Best regards,
> > Dominic
> 
> Maybe SQLite is the route we should use then.  It seems cumbersome to have to
> keep another translation layer solely for the purpose of precedence.  Overall,
> changing the protobuf between server and client code is fairly cumbersome. 
> Adding even more layers of translation to add a new feature seems like a lot
of
> future work.  In any case, I am not an OWNER in Chrome, so your call.
> 
> Cheers,
> Mark

FYI, Sync is building a feature where we provide basic leveldb based persistence
on the client that is tightly coupled to the sync metadata and sync api (such
that all the sync metadata will reside in the database next to the actual data,
and its all updated automatically). Designing for level db from the beginning
might make sense so that the enhanced bookmarks sync type can eventually use
this scheme (once its finished) without needing a separate system for
persistence of other data.

[noyau (Ping after 24h), 2014-10-01 15:38:37]

Patchset #6 (id:120001) has been deleted

[noyau (Ping after 24h), 2014-10-01 16:04:44]

All rebased, all using the new API introduced after the creation of this CL. All
the tests for those class pass (The tests are not yet present here, they are
currently in the iOS repository and I'll move them in chromium as soon as their
dependencies are resolved)

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_cluster_service.cc (right):

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:123: url =
net::AppendQueryParameter(url, "hl", application_language_code_);
On 2014/09/18 18:08:51, Mark wrote:
> You are also going to need to specify the version parameter (v=[version tag])

Done. Added the version for the search service as well.

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:167:
cluster_data_.begin())) {
On 2014/09/29 21:15:55, Kibeom Kim wrote:
> On 2014/09/18 12:41:47, noyau wrote:
> > On 2014/09/12 21:31:24, Kibeom Kim wrote:
> > > can we use std:map's operator == here? (I think the result should be same,
> > > though unsure if there is some performance difference)
> > Excellent question. According to the spec std::map does not implement
operator
> > ==, but it compiles on iOS, so obviously something implements it. The
> semantics
> > of std::equals are clear, and I can't find documentation on the semantics of
> ==
> > for std::map, so in doubt, I'd rather keep that code as is: I know it works,
> > we've been using it for months on iOS.
> > 
> > Performance there is quite irrelevant anyway, as this only happens following
a
> > user action.
> 
> I agree that we don't need to change already working code. :)
> 
> Just for reference:
http://en.cppreference.com/w/cpp/container/map/operator_cmp

Acknowledged.

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
File components/enhanced_bookmarks/proto/cluster.proto (right):

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/proto/cluster.proto:20: optional HypnoOptions
options = 2;
On 2014/09/18 18:08:51, Mark wrote:
> Consider using "extensions 2;" here to denote this field is reserved without
> needing to bring in any of the HypnoOptions.

Done.

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/proto/cluster.proto:37: message HypnoOptions {
On 2014/09/18 18:08:52, Mark wrote:
> You cannot set the HypnoOptions when querying GWS, so I'm not sure if we need
to
> include this right now.

Done.

https://codereview.chromium.org/539173004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/proto/cluster.proto:37: message HypnoOptions {
On 2014/09/23 17:08:29, Yaron wrote:
> On 2014/09/18 18:08:52, Mark wrote:
> > You cannot set the HypnoOptions when querying GWS, so I'm not sure if we
need
> to
> > include this right now.
> 
> Sounds good to me. Let's keep this minimal and remove

Done.

https://codereview.chromium.org/539173004/diff/100001/chrome/browser/enhanced...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc
(right):

https://codereview.chromium.org/539173004/diff/100001/chrome/browser/enhanced...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:40:
void ChromeBookmarkServerClusterService::OnSyncCycleCompleted() {
On 2014/09/23 17:11:21, Yaron wrote:
> On 2014/09/23 17:08:30, Yaron wrote:
> > So per previous comment I think this still happens too often to issue a
> request
> > every time. You could add a delay/throttling or maybe there's a better
option
> > for deferring the cluster request.
> 
> Err +Nicolas to comment

Acknowledged. Can I defer this to a subsequent CL?

https://codereview.chromium.org/539173004/diff/100001/chrome/browser/enhanced...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h
(right):

https://codereview.chromium.org/539173004/diff/100001/chrome/browser/enhanced...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h:20:
const std::string& application_language_code,
On 2014/09/26 19:17:19, Mark wrote:
> On 2014/09/26 14:33:04, battre wrote:
> > nit: add a comment on this? What is an application_language_code? E.g. for
> > Germany would it be "de", "de_DE", "GER"?
> 
> Eventually we should be ISO 639-6 compliant, but I think for now we can just
use
> BCP47 since both will be readable by the LanguageCode processor server-side.

BCP47 is what we use currently on iOS. Made it clear in the comment in the
superclass.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
File components/enhanced_bookmarks/bookmark_server_cluster_service.cc (right):

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:69: std::string
stars_id = *it;
On 2014/09/26 14:33:04, battre wrote:
> const std::string& star_id = *it; ?

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:69: std::string
stars_id = *it;
On 2014/09/23 17:08:30, Yaron wrote:
> star_id

Done.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:80: std::string
starid = RemoteIDForBookmark(bookmark);
On 2014/09/23 17:08:30, Yaron wrote:
> star_id

Done.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:214:
BookmarkServerClusterService::Serialize(cluster_data_, &serialized, auth_id);
On 2014/09/26 14:33:05, battre wrote:
> Nit: Do you need the BookmarkServerClusterService:: prefix?

Done.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:215:
pref_service_->SetString(prefs::kBookmarkClusters, serialized);
On 2014/09/26 14:33:04, battre wrote:
> Why do you store this as a string instead of a dictionary?

I didn't do this code originally, so I must admit I didn't think too much about
it when I moved it here. I changed it to store the dictionary directly.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:237:
base::ListValue* all_clusters = new base::ListValue;
On 2014/09/26 14:33:05, battre wrote:
> scoped_ptr?
Used scoped_ptr and release() to make the ownership transfer explicit.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:243:
base::ListValue* cluster = new base::ListValue;
On 2014/09/26 14:33:05, battre wrote:
> scoped_ptr?

Same, removed the comment and used scoped_ptr and release() to make the transfer
of ownership explicit.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:254:
data.SetString(kAuthIdKey, auth_id);
On 2014/09/26 14:33:05, battre wrote:
> Why don't you just return data?

base::DictionaryValue is not copiable (DISALLOW_COPY_AND_ASSIGN). I made it
return a scoped pointer instead.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:268:
base::JSONReader reader;
On 2014/09/26 14:33:04, battre wrote:
> On 2014/09/23 17:08:30, Yaron wrote:
> > On 2014/09/18 18:10:43, Mark wrote:
> > > Any reason why you would serialize this down to JSON instead of just
> > serializing
> > > the protobufs directly?
> > 
> > +1
> 
> I think this is used only for the pref service. There is no need to serialize
it
> at all. Just store it as a dictionary.

The is no precedent in storing binary data in prefs, so it is now stored as a
dictionary.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:277: if
(!result)
On 2014/09/26 14:33:05, battre wrote:
> nit: I would inline all of these
> if (!value->GetAsDictionary(&dictionary_value))
>   return false;

Done.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:303:
base::ListValue* cluster;
On 2014/09/26 14:33:05, battre wrote:
> base::ListValue* cluster = NULL;

Done.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
File components/enhanced_bookmarks/bookmark_server_cluster_service.h (right):

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:22: // state.
On 2014/09/26 14:33:05, battre wrote:
> Add a description of what a cluster and cluster name is?

Done.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:27: typedef
std::map<std::string, std::vector<std::string> > ClusterMap;
On 2014/09/26 14:33:05, battre wrote:
> describe key and value?

Done.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:29: const
std::string& application_language_code,
On 2014/09/26 14:33:05, battre wrote:
> describe parameter? (see above)

Done.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:37: // Retrieves
all the bookmarks associated with a cluster.
On 2014/09/26 14:33:05, battre wrote:
> Comment on ownership of BookmarkNodes?

Done.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:51: protected:
On 2014/09/26 14:33:05, battre wrote:
> // BookmarkServerService methods.

Done.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:59: //
BookmarkModelObserver methods.
On 2014/09/26 14:33:05, battre wrote:
> I can't find this inheritance.

Yes, the code changed, and this inheritance disappeared in the superclass.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:85: // profile
prefs is always up to date.
On 2014/09/26 14:33:05, battre wrote:
> What's the cluster_map parameter?

Done.

https://codereview.chromium.org/539173004/diff/100001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:96: const
std::string& auth_id);
On 2014/09/26 14:33:05, battre wrote:
> make the output parameter the last one? (same for Deserialize)
> 
> what's auth_id?

Done.

[noyau (Ping after 24h), 2014-10-01 18:49:50]

Patchset #8 (id:180001) has been deleted

[Yaron, 2014-10-01 23:45:21]

lgtm

https://codereview.chromium.org/539173004/diff/100001/chrome/browser/enhanced...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc
(right):

https://codereview.chromium.org/539173004/diff/100001/chrome/browser/enhanced...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:40:
void ChromeBookmarkServerClusterService::OnSyncCycleCompleted() {
On 2014/10/01 16:04:42, noyau wrote:
> On 2014/09/23 17:11:21, Yaron wrote:
> > On 2014/09/23 17:08:30, Yaron wrote:
> > > So per previous comment I think this still happens too often to issue a
> > request
> > > every time. You could add a delay/throttling or maybe there's a better
> option
> > > for deferring the cluster request.
> > 
> > Err +Nicolas to comment
> 
> Acknowledged. Can I defer this to a subsequent CL? 

I'm pretty sure this will be too often. Since it's not invoked unless the
experiment is active, that's fine but I think we'll have to fix it regardless

https://codereview.chromium.org/539173004/diff/220001/components/enhanced_boo...
File components/enhanced_bookmarks/bookmark_server_cluster_service.h (right):

https://codereview.chromium.org/539173004/diff/220001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:100: //
Serialize the |cluster_map| into the returned dictionaty value.. The
s/dictionaty value../dictionary value./

[noyau (Ping after 24h), 2014-10-06 15:50:54]

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

[noyau (Ping after 24h), 2014-10-06 15:50:55]

+sky for the addition of the chrome/browser/enhanced_bookmarks folder.

[noyau (Ping after 24h), 2014-10-06 16:21:15]

https://codereview.chromium.org/539173004/diff/220001/components/enhanced_boo...
File components/enhanced_bookmarks/bookmark_server_cluster_service.h (right):

https://codereview.chromium.org/539173004/diff/220001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:100: //
Serialize the |cluster_map| into the returned dictionaty value.. The
On 2014/10/01 23:45:21, Yaron wrote:
> s/dictionaty value../dictionary value./

Done.

[sky, 2014-10-06 18:24:12]

https://codereview.chromium.org/539173004/diff/240001/chrome/browser/enhanced...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h
(right):

https://codereview.chromium.org/539173004/diff/240001/chrome/browser/enhanced...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h:15:
// Manages requests to the bookmark server to do just-in-time requests.
It isn't clear to me at all what any of this code is doing. There needs to be a
description some where.

https://codereview.chromium.org/539173004/diff/240001/chrome/browser/enhanced...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h:30:
virtual void OnStateChanged() OVERRIDE;
OVERRIDE->override

https://codereview.chromium.org/539173004/diff/240001/chrome/browser/enhanced...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h:31:
// The stars cluster API relies on the information in chrome-sync. Sending a
Style guide says no comments like this for overriden sections. Move to .cc.

[noyau (Ping after 24h), 2014-10-07 11:11:57]

PTAL

https://codereview.chromium.org/539173004/diff/240001/chrome/browser/enhanced...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h
(right):

https://codereview.chromium.org/539173004/diff/240001/chrome/browser/enhanced...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h:15:
// Manages requests to the bookmark server to do just-in-time requests.
On 2014/10/06 18:24:11, sky wrote:
> It isn't clear to me at all what any of this code is doing. There needs to be
a
> description some where.

Updated the comment.

https://codereview.chromium.org/539173004/diff/240001/chrome/browser/enhanced...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h:30:
virtual void OnStateChanged() OVERRIDE;
On 2014/10/06 18:24:11, sky wrote:
> OVERRIDE->override

This CL actually predates that :). I fixed it in all the files that this CL
touches.

https://codereview.chromium.org/539173004/diff/240001/chrome/browser/enhanced...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h:31:
// The stars cluster API relies on the information in chrome-sync. Sending a
On 2014/10/06 18:24:11, sky wrote:
> Style guide says no comments like this for overriden sections. Move to .cc.

Moved.

[noyau (Ping after 24h), 2014-10-07 12:22:52]

Patchset #11 (id:260001) has been deleted

[battre, 2014-10-07 13:39:30]

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
File components/enhanced_bookmarks/bookmark_server_cluster_service.cc (right):

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:70: const
std::string& star_id = *it;
nit/opt:
you can use C++11 syntax now http://chromium-cpp.appspot.com/

for (const std::string& star_id : cluster_it->second) {

That might simplify some loops below as well.

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:89: const
std::vector<std::string>& v = it->second;
nit/opt: rename v to stars_ids?

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:116:
net::URLFetcher* BookmarkServerClusterService::CreateFetcher() {
scoped_ptr<net::URLFetcher>?

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:153: const
image_collections::ClusterResponse_Cluster cluster =
nit/opt: You could store the result as a const reference to a temporayr object
like you do in BookmarkServerClusterService::ClustersForBookmark

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:158:
std::vector<std::string> starids;
stars_ids? (plural and underscore)

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:174:
UpdateModel(&new_cluster_data);
Do you want to call this SwapModel?

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:227:
UpdateModel(&empty);
You don't have unit tests. Did you check whether the
GetSigninManager()->GetAuthenticatedAccountId() call in UpdateModel still
returns a valid account_id when you get a GoogleSignedOut() call?

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:256: 
nit: remove empty line

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:262: for (auto
it : cluster_map) {
auto&

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:271:
scoped_ptr<base::DictionaryValue> data(new base::DictionaryValue());
I would be consistent with () in calls of the constructor in the function.

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:281: 
-newline

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:300:
base::ListValue const* all_clusters = NULL;
I think that the common syntax is "const base::ListValue*". Same below.

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
File components/enhanced_bookmarks/bookmark_server_cluster_service.h (right):

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:53: //
Dynamically generates a vector of all clusters.
all cluster names?

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:62: 
nit: remove line 62 and 65?

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:79:
FRIEND_TEST_ALL_PREFIXES(BookmarkServerServiceTest, Cluster);
Did you forget to add a file to this CL?

[noyau (Ping after 24h), 2014-10-07 15:19:32]

PTAL

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
File components/enhanced_bookmarks/bookmark_server_cluster_service.cc (right):

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:70: const
std::string& star_id = *it;
On 2014/10/07 13:39:29, battre wrote:
> nit/opt:
> you can use C++11 syntax now http://chromium-cpp.appspot.com/
> 
> for (const std::string& star_id : cluster_it->second) {
> 
> That might simplify some loops below as well.

Arrrgh! DOne another pass reverting the changes to go back to C++11 loops.

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:89: const
std::vector<std::string>& v = it->second;
On 2014/10/07 13:39:28, battre wrote:
> nit/opt: rename v to stars_ids?

Done.

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:116:
net::URLFetcher* BookmarkServerClusterService::CreateFetcher() {
On 2014/10/07 13:39:28, battre wrote:
> scoped_ptr<net::URLFetcher>?

Done.

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:153: const
image_collections::ClusterResponse_Cluster cluster =
On 2014/10/07 13:39:28, battre wrote:
> nit/opt: You could store the result as a const reference to a temporayr object
> like you do in BookmarkServerClusterService::ClustersForBookmark

Used auto&

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:158:
std::vector<std::string> starids;
On 2014/10/07 13:39:28, battre wrote:
> stars_ids? (plural and underscore)

Done.

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:174:
UpdateModel(&new_cluster_data);
On 2014/10/07 13:39:29, battre wrote:
> Do you want to call this SwapModel?

Done.

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:227:
UpdateModel(&empty);
On 2014/10/07 13:39:29, battre wrote:
> You don't have unit tests. Did you check whether the
> GetSigninManager()->GetAuthenticatedAccountId() call in UpdateModel still
> returns a valid account_id when you get a GoogleSignedOut() call?

This returns the empty string when signed out. But you're right, I didn't have
unit tests covering this particular case, I was only checking swapping from a
signed user to another signed user. I've added the test in the iOS tree.

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:256: 
On 2014/10/07 13:39:29, battre wrote:
> nit: remove empty line

Done.

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:262: for (auto
it : cluster_map) {
On 2014/10/07 13:39:29, battre wrote:
> auto&

Done.

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:271:
scoped_ptr<base::DictionaryValue> data(new base::DictionaryValue());
On 2014/10/07 13:39:28, battre wrote:
> I would be consistent with () in calls of the constructor in the function.

Done.

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:281: 
On 2014/10/07 13:39:28, battre wrote:
> -newline

Done.

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:300:
base::ListValue const* all_clusters = NULL;
On 2014/10/07 13:39:28, battre wrote:
> I think that the common syntax is "const base::ListValue*". Same below.

Done.

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
File components/enhanced_bookmarks/bookmark_server_cluster_service.h (right):

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:53: //
Dynamically generates a vector of all clusters.
On 2014/10/07 13:39:29, battre wrote:
> all cluster names?

Done.

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:62: 
On 2014/10/07 13:39:29, battre wrote:
> nit: remove line 62 and 65?

Done.

https://codereview.chromium.org/539173004/diff/280001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.h:79:
FRIEND_TEST_ALL_PREFIXES(BookmarkServerServiceTest, Cluster);
On 2014/10/07 13:39:29, battre wrote:
> Did you forget to add a file to this CL?

No I did not. Those tests do exist in the iOS repository, but are tied to some
other code (The FakeSignInManager and the FakeOAuthTokenService are not
componentized yet, and the test itself has a dependency on web/content
abstractions that are not present in chromium yet). Both are in progress…

[battre, 2014-10-07 16:09:43]

LGTM

I suppose that somebody else reviews the tests.

https://codereview.chromium.org/539173004/diff/300001/components/enhanced_boo...
File components/enhanced_bookmarks/bookmark_server_cluster_service.cc (right):

https://codereview.chromium.org/539173004/diff/300001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:98: }
Nit: remove {}?

https://codereview.chromium.org/539173004/diff/300001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:242: //
// static

https://codereview.chromium.org/539173004/diff/300001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:266: 
// static

[noyau (Ping after 24h), 2014-10-07 16:28:39]

https://codereview.chromium.org/539173004/diff/300001/components/enhanced_boo...
File components/enhanced_bookmarks/bookmark_server_cluster_service.cc (right):

https://codereview.chromium.org/539173004/diff/300001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:98: }
On 2014/10/07 16:09:43, battre wrote:
> Nit: remove {}?

Done.

https://codereview.chromium.org/539173004/diff/300001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:242: //
On 2014/10/07 16:09:43, battre wrote:
> // static

Done (x3)

https://codereview.chromium.org/539173004/diff/300001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_cluster_service.cc:266: 
On 2014/10/07 16:09:43, battre wrote:
> // static

Done.

[sky, 2014-10-07 17:45:24]

LGTM

[noyau (Ping after 24h), 2014-10-07 20:22:01]

The CQ bit was checked by noyau@chromium.org

[commit-bot: I haz the power, 2014-10-07 20:23:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/539173004/320001

[commit-bot: I haz the power, 2014-10-07 22:00:57]

Committed patchset #13 (id:320001) as 22ae3f7319bfada27fc53d57c7e59beebcc16651

[commit-bot: I haz the power, 2014-10-07 22:01:36]

Patchset 13 (id:??) landed as
https://crrev.com/6bb374fc58bfeeb7966b6a548b1c7b41af9c8e47
Cr-Commit-Position: refs/heads/master@{#298582}

[Sorin Jianu, 2014-10-08 01:03:06]

A revert of this CL (patchset #13 id:320001) has been created in
https://codereview.chromium.org/641473002/ by sorin@chromium.org.

The reason for reverting is: This CL added static initializers which broke the
sizes bots on Mac, Linux, and Linux x64.

Before the change:
# Static initializers in src/out/Release/chrome:
# HINT: To get this list, run tools/linux/dump_static_initializers.py
# HINT: diff against the log from the last run to see what changed
# atomicops_internals_x86_gcc.cc .L.str
# atomicops_internals_x86_gcc.cc AtomicOps_Internalx86CPUFeatures
# atomicops_internals_x86_gcc.cc
_GLOBAL__sub_I_atomicops_internals_x86_gcc.cc+0x65
# atomicops_internals_x86_gcc.cc
_GLOBAL__sub_I_atomicops_internals_x86_gcc.cc+0xaa
# atomicops_internals_x86_gcc.cc __stack_chk_fail@plt
# atomicops_internals_x86_gcc.cc strcmp@plt
# atomicops_internals_x86_gcc.cc .L.str
# atomicops_internals_x86_gcc.cc
_GLOBAL__sub_I_atomicops_internals_x86_gcc.cc+0x6b
# atomicops_internals_x86_gcc.cc
_GLOBAL__sub_I_atomicops_internals_x86_gcc.cc+0xc2
# atomicops_internals_x86_gcc.cc
google::protobuf::internal::AtomicOps_Internalx86CPUFeatures
# atomicops_internals_x86_gcc.cc
google::protobuf::internal::AtomicOps_Internalx86CPUFeatures+0x1
# atomicops_internals_x86_gcc.cc __stack_chk_fail@plt
# atomicops_internals_x86_gcc.cc strcmp@plt
# atomicops_internals_x86_gcc.cc .L.str
# atomicops_internals_x86_gcc.cc
_GLOBAL__sub_I_atomicops_internals_x86_gcc.cc+0x65
# atomicops_internals_x86_gcc.cc
_GLOBAL__sub_I_atomicops_internals_x86_gcc.cc+0xaa
# atomicops_internals_x86_gcc.cc v8::base::AtomicOps_Internalx86CPUFeatures
# atomicops_internals_x86_gcc.cc __stack_chk_fail@plt
# atomicops_internals_x86_gcc.cc strcmp@plt
# debugallocation_shim.cc module_enter_exit_hook
# debugallocation_shim.cc (anonymous namespace)::large_alloc_threshold
# debugallocation_shim.cc TCMallocGuard::TCMallocGuard()
# debugallocation_shim.cc __cxa_atexit@plt [registers a dtor to run at exit]
# debugallocation_shim.cc __init_array_end+0x3538
# debugallocation_shim.cc __init_array_end+0x3540
# debugallocation_shim.cc __init_array_end+0x3548
# memory_region_map.cc libpthread_initialized
# spinlock.cc _GLOBAL__sub_I_spinlock.cc+0x12
# spinlock.cc NumCPUs()
# spinlock.cc adaptive_spin_count
# spinlock_internal.cc _GLOBAL__sub_I_spinlock_internal.cc+0x79
# spinlock_internal.cc have_futex
# spinlock_internal.cc futex_private_flag
# spinlock_internal.cc syscall@plt
# Found 34 static initializers in 7 files.

After the change:
# Static initializers in src/out/Release/chrome:
# HINT: To get this list, run tools/linux/dump_static_initializers.py
# HINT: diff against the log from the last run to see what changed
# atomicops_internals_x86_gcc.cc .L.str
# atomicops_internals_x86_gcc.cc AtomicOps_Internalx86CPUFeatures
# atomicops_internals_x86_gcc.cc
_GLOBAL__sub_I_atomicops_internals_x86_gcc.cc+0x65
# atomicops_internals_x86_gcc.cc
_GLOBAL__sub_I_atomicops_internals_x86_gcc.cc+0xaa
# atomicops_internals_x86_gcc.cc __stack_chk_fail@plt
# atomicops_internals_x86_gcc.cc strcmp@plt
# atomicops_internals_x86_gcc.cc .L.str
# atomicops_internals_x86_gcc.cc
_GLOBAL__sub_I_atomicops_internals_x86_gcc.cc+0x6b
# atomicops_internals_x86_gcc.cc
_GLOBAL__sub_I_atomicops_internals_x86_gcc.cc+0xc2
# atomicops_internals_x86_gcc.cc
google::protobuf::internal::AtomicOps_Internalx86CPUFeatures
# atomicops_internals_x86_gcc.cc
google::protobuf::internal::AtomicOps_Internalx86CPUFeatures+0x1
# atomicops_internals_x86_gcc.cc __stack_chk_fail@plt
# atomicops_internals_x86_gcc.cc strcmp@plt
# atomicops_internals_x86_gcc.cc .L.str
# atomicops_internals_x86_gcc.cc
_GLOBAL__sub_I_atomicops_internals_x86_gcc.cc+0x65
# atomicops_internals_x86_gcc.cc
_GLOBAL__sub_I_atomicops_internals_x86_gcc.cc+0xaa
# atomicops_internals_x86_gcc.cc v8::base::AtomicOps_Internalx86CPUFeatures
# atomicops_internals_x86_gcc.cc __stack_chk_fail@plt
# atomicops_internals_x86_gcc.cc strcmp@plt
# bookmark_server_cluster_service.cc .L.str
# bookmark_server_cluster_service.cc (anonymous namespace)::kClusterUrl
# bookmark_server_cluster_service.cc std::basic_string<char,
std::char_traits<char>, std::allocator<char> >::basic_string(char const*,
std::allocator<char> const&)@plt
# bookmark_server_cluster_service.cc __cxa_atexit@plt [registers a dtor to run
at exit]
# bookmark_server_cluster_service.cc __init_array_end+0x3540
# bookmark_server_cluster_service.cc __init_array_end+0xe220
# debugallocation_shim.cc module_enter_exit_hook
# debugallocation_shim.cc (anonymous namespace)::large_alloc_threshold
# debugallocation_shim.cc TCMallocGuard::TCMallocGuard()
# debugallocation_shim.cc __cxa_atexit@plt [registers a dtor to run at exit]
# debugallocation_shim.cc __init_array_end+0x3538
# debugallocation_shim.cc __init_array_end+0x3540
# debugallocation_shim.cc __init_array_end+0x3548
# memory_region_map.cc libpthread_initialized
# spinlock.cc _GLOBAL__sub_I_spinlock.cc+0x12
# spinlock.cc NumCPUs()
# spinlock.cc adaptive_spin_count
# spinlock_internal.cc _GLOBAL__sub_I_spinlock_internal.cc+0x79
# spinlock_internal.cc have_futex
# spinlock_internal.cc futex_private_flag
# spinlock_internal.cc syscall@plt
# Found 40 static initializers in 8 files.

See this bot, as an example:
https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/71728/steps...

In addition to this, the CL declares a static instance of std::string which is
not allowed. The S string constants also should be declared as const char
kPrefServiceVersionKey[] = "version" for example.
.

[noyau (Ping after 24h), 2014-10-08 09:04:09]

noyau@chromium.org changed reviewers:
+ sdefresne@chromium.org

[noyau (Ping after 24h), 2014-10-08 09:04:10]

Fixing the static initializers.

[sdefresne, 2014-10-08 09:08:21]

LGTM with nits

https://codereview.chromium.org/539173004/diff/340001/components/enhanced_boo...
File components/enhanced_bookmarks/bookmark_server_search_service.cc (right):

https://codereview.chromium.org/539173004/diff/340001/components/enhanced_boo...
components/enhanced_bookmarks/bookmark_server_search_service.cc:14: const char
kSearchUrl[] =
nit: this fits on one line

[noyau (Ping after 24h), 2014-10-08 09:10:36]

The CQ bit was checked by noyau@chromium.org

[commit-bot: I haz the power, 2014-10-08 09:11:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/539173004/360001

[commit-bot: I haz the power, 2014-10-08 11:11:22]

Committed patchset #15 (id:360001) as 3538c71001343ffef6c042eed650c4f0016a79ad

[commit-bot: I haz the power, 2014-10-08 11:12:10]

Patchset 15 (id:??) landed as
https://crrev.com/daaac3af72f231dec2df4dc566b84356d3dc10df
Cr-Commit-Position: refs/heads/master@{#298682}