Create bookmarks component with names of bookmark preferences

Create bookmarks component with names of bookmark preferences

Introduce a new bookmarks component and update DEPS for chrome/browser
and chrome/chrome_browser.gypi.

Move names of bookmark preferences from chrome/common/pref_names.{cc,h}
to components/bookmarks/core/common/bookmark_pref_names.{cc,h} and add
an include in chrome/common/pref_names.h to limit modification to the
client files.

BUG=360613
TBR=sky

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263016

[sdefresne, 2014-04-08 15:51:00]

Please have a look.

[tfarina, 2014-04-08 16:11:49]

https://codereview.chromium.org/228393004/diff/1/components/bookmarks.gypi
File components/bookmarks.gypi (right):

https://codereview.chromium.org/228393004/diff/1/components/bookmarks.gypi#ne...
components/bookmarks.gypi:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
I will add a BUILD.gn if you don't bet me.

https://codereview.chromium.org/228393004/diff/1/components/bookmarks.gypi#ne...
components/bookmarks.gypi:8: 'target_name': 'bookmarks',
you could have tweaked the similarity passed to git cl upload so it does not
find it as a copy of metrics.gypi, I think there is another flag
(no-find-copies) that you can also pass to git cl upload, so this file would
show up as an "added" file.

[sdefresne, 2014-04-08 16:19:42]

https://codereview.chromium.org/228393004/diff/1/components/bookmarks.gypi
File components/bookmarks.gypi (right):

https://codereview.chromium.org/228393004/diff/1/components/bookmarks.gypi#ne...
components/bookmarks.gypi:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
On 2014/04/08 16:11:50, tfarina wrote:
> I will add a BUILD.gn if you don't bet me.
> 

That would be helpful as I still don't know the syntax of the BUILD.gn files.

https://codereview.chromium.org/228393004/diff/1/components/bookmarks.gypi#ne...
components/bookmarks.gypi:8: 'target_name': 'bookmarks',
On 2014/04/08 16:11:50, tfarina wrote:
> you could have tweaked the similarity passed to git cl upload so it does not
> find it as a copy of metrics.gypi, I think there is another flag
> (no-find-copies) that you can also pass to git cl upload, so this file would
> show up as an "added" file.

Done.

[tfarina, 2014-04-08 16:30:34]

I only looked at the files under components/ and I have only tiny nits.

lgtm otherwise (unofficial, since I'm not an owner).

thanks,

https://codereview.chromium.org/228393004/diff/20001/components/bookmarks/boo...
File components/bookmarks/bookmark_pref_names.h (right):

https://codereview.chromium.org/228393004/diff/20001/components/bookmarks/boo...
components/bookmarks/bookmark_pref_names.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
optional-super-tiny-nit: some files use (c), others don't, but as far as I know
the right is without.

https://codereview.chromium.org/228393004/diff/20001/components/bookmarks/boo...
components/bookmarks/bookmark_pref_names.h:11: extern const char
kBookmarkEditorExpandedNodes[];
nit-nit: please, keep one blank line after namespace prefs. same below.

https://codereview.chromium.org/228393004/diff/20001/components/components_te...
File components/components_tests.gyp (right):

https://codereview.chromium.org/228393004/diff/20001/components/components_te...
components/components_tests.gyp:170: # Dependencies of auto_login_parser
one day I want to clean this list and remove this "Dependencies of", it simply
does not scale IMO!

[sdefresne, 2014-04-08 22:04:42]

https://codereview.chromium.org/228393004/diff/20001/components/bookmarks/boo...
File components/bookmarks/bookmark_pref_names.h (right):

https://codereview.chromium.org/228393004/diff/20001/components/bookmarks/boo...
components/bookmarks/bookmark_pref_names.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/04/08 16:30:34, tfarina wrote:
> optional-super-tiny-nit: some files use (c), others don't, but as far as I
know
> the right is without.

Done.

https://codereview.chromium.org/228393004/diff/20001/components/bookmarks/boo...
components/bookmarks/bookmark_pref_names.h:11: extern const char
kBookmarkEditorExpandedNodes[];
On 2014/04/08 16:30:34, tfarina wrote:
> nit-nit: please, keep one blank line after namespace prefs. same below.

Done.

https://codereview.chromium.org/228393004/diff/20001/components/components_te...
File components/components_tests.gyp (right):

https://codereview.chromium.org/228393004/diff/20001/components/components_te...
components/components_tests.gyp:170: # Dependencies of auto_login_parser
On 2014/04/08 16:30:34, tfarina wrote:
> one day I want to clean this list and remove this "Dependencies of", it simply
> does not scale IMO!

Ack.

[blundell, 2014-04-09 09:13:00]

https://codereview.chromium.org/228393004/diff/60001/chrome/browser/android/b...
File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right):

https://codereview.chromium.org/228393004/diff/60001/chrome/browser/android/b...
chrome/browser/android/bookmarks/bookmarks_bridge.cc:17: #include
"components/bookmarks/bookmark_pref_names.h"
Rather than adding this include everywhere, the pattern has to been to add the
include of the new file to chrome/common/pref_names.h.

https://codereview.chromium.org/228393004/diff/60001/components/bookmarks.gypi
File components/bookmarks.gypi (right):

https://codereview.chromium.org/228393004/diff/60001/components/bookmarks.gyp...
components/bookmarks.gypi:8: 'target_name': 'bookmarks',
bookmarks will be a layered component, right? If so, its internal structure
should mirror that of e.g. autofill, so this target should be
bookmarks_core_common.

[sdefresne, 2014-04-09 13:16:06]

PTAL

https://codereview.chromium.org/228393004/diff/60001/chrome/browser/android/b...
File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right):

https://codereview.chromium.org/228393004/diff/60001/chrome/browser/android/b...
chrome/browser/android/bookmarks/bookmarks_bridge.cc:17: #include
"components/bookmarks/bookmark_pref_names.h"
On 2014/04/09 09:13:00, blundell wrote:
> Rather than adding this include everywhere, the pattern has to been to add the
> include of the new file to chrome/common/pref_names.h.

Done.

https://codereview.chromium.org/228393004/diff/60001/components/bookmarks.gypi
File components/bookmarks.gypi (right):

https://codereview.chromium.org/228393004/diff/60001/components/bookmarks.gyp...
components/bookmarks.gypi:8: 'target_name': 'bookmarks',
On 2014/04/09 09:13:00, blundell wrote:
> bookmarks will be a layered component, right? If so, its internal structure
> should mirror that of e.g. autofill, so this target should be
> bookmarks_core_common.

Done.

[tfarina, 2014-04-09 15:27:24]

https://codereview.chromium.org/228393004/diff/60001/chrome/browser/android/b...
File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right):

https://codereview.chromium.org/228393004/diff/60001/chrome/browser/android/b...
chrome/browser/android/bookmarks/bookmarks_bridge.cc:17: #include
"components/bookmarks/bookmark_pref_names.h"
On 2014/04/09 09:13:00, blundell wrote:
> Rather than adding this include everywhere, the pattern has to been to add the
> include of the new file to chrome/common/pref_names.h.
No. Colin, what is the benefit of doing that? Things like that never get clean
up after the fact.

Please, do the right thing instead of doing paliative solutions for speedness of
the work.

[tfarina, 2014-04-09 15:28:25]

https://codereview.chromium.org/228393004/diff/80001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/228393004/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.h:15: #include
"components/bookmarks/core/common/bookmark_pref_names.h"
not lgtm, because of this change.

+Lei, for chrome/OWNERS.

[blundell, 2014-04-09 15:32:24]

On 2014/04/09 15:27:24, tfarina wrote:
>
https://codereview.chromium.org/228393004/diff/60001/chrome/browser/android/b...
> File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right):
> 
>
https://codereview.chromium.org/228393004/diff/60001/chrome/browser/android/b...
> chrome/browser/android/bookmarks/bookmarks_bridge.cc:17: #include
> "components/bookmarks/bookmark_pref_names.h"
> On 2014/04/09 09:13:00, blundell wrote:
> > Rather than adding this include everywhere, the pattern has to been to add
the
> > include of the new file to chrome/common/pref_names.h.
> No. Colin, what is the benefit of doing that? Things like that never get clean
> up after the fact.
> 
> Please, do the right thing instead of doing paliative solutions for speedness
of
> the work.

I think it's fine for the inclusion of //chrome/common/pref_names.h to capture
all the pref names of interest to //chrome. It seems like pure developer
overhead to have developers have to know what prefs file a given pref is coming
from in order to use it. The same thing is done for
//chrome/common/chrome_switches.h. In both of those cases, the pattern was
started before I got involved with componentization.

I don't see this as being bad for the health of the codebase.

[Lei Zhang, 2014-04-09 17:58:44]

https://codereview.chromium.org/228393004/diff/80001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/228393004/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.h:15: #include
"components/bookmarks/core/common/bookmark_pref_names.h"
On 2014/04/09 15:28:26, tfarina wrote:
> not lgtm, because of this change.
> 
> +Lei, for chrome/OWNERS.

Well, this is consistent with the #includes surrounding it, so I wouldn't nack
this CL for doing what's already done.

Looking at chrome_switches.h, it has included base and content switches for a
long time. There might be other files in chrome/common that does this as well.

For base, and components, since they are meant to be reused, it's good to only
#include what's necessary. The chrome directory, OTOH, is intended to be an edge
node on the other end of the dependency graph, so a little extra work for the
C++ preprocessor here is ok, since it's saving developer time and not a
frivolous #include that adds no value.

[tfarina, 2014-04-09 18:26:52]

OK, since Lei has more authority than me here, you have my ack.

Since it has a precedent, as Lei noted, it seems ok. The other reason is that
the include does not bring any additional headers with it, so it might be OK.

Hopefully we don't make this assumption for everything else. Including
unnecessary headers slow down the compilation and causes clients to be
recompiled with a change in one of these headers change.

informal lgtm.

[blundell, 2014-04-10 13:20:18]

LGTM with the below fixes

Consider using tools/git/move_source_file.py and tools/git/mass-rename.py

https://codereview.chromium.org/228393004/diff/80001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/DEPS (right):

https://codereview.chromium.org/228393004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/DEPS:12: "+components/bookmarks",
You don't need to add this.

https://codereview.chromium.org/228393004/diff/80001/components/bookmarks/DEPS
File components/bookmarks/DEPS (right):

https://codereview.chromium.org/228393004/diff/80001/components/bookmarks/DEP...
components/bookmarks/DEPS:3: # can be used by embedder that don't use content/
(Chrome on iOS).
I would say "so that it can be shared on iOS."

https://codereview.chromium.org/228393004/diff/80001/components/bookmarks/cor...
File components/bookmarks/core/common/bookmark_pref_names.cc (right):

https://codereview.chromium.org/228393004/diff/80001/components/bookmarks/cor...
components/bookmarks/core/common/bookmark_pref_names.cc:5: #include
"components/bookmarks/bookmark_pref_names.h"
thhe include path is wrong

https://codereview.chromium.org/228393004/diff/80001/components/bookmarks/cor...
File components/bookmarks/core/common/bookmark_pref_names.h (right):

https://codereview.chromium.org/228393004/diff/80001/components/bookmarks/cor...
components/bookmarks/core/common/bookmark_pref_names.h:7: #ifndef
COMPONENTS_BOOKMARKS_BOOKMARK_PREF_NAMES_H_
the include guard is wrong

[sdefresne, 2014-04-10 15:07:44]

Thank you for the reviews.

https://codereview.chromium.org/228393004/diff/80001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/DEPS (right):

https://codereview.chromium.org/228393004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/DEPS:12: "+components/bookmarks",
On 2014/04/10 13:20:19, blundell wrote:
> You don't need to add this.

Done.

https://codereview.chromium.org/228393004/diff/80001/components/bookmarks/DEPS
File components/bookmarks/DEPS (right):

https://codereview.chromium.org/228393004/diff/80001/components/bookmarks/DEP...
components/bookmarks/DEPS:3: # can be used by embedder that don't use content/
(Chrome on iOS).
On 2014/04/10 13:20:19, blundell wrote:
> I would say "so that it can be shared on iOS."

Done.

https://codereview.chromium.org/228393004/diff/80001/components/bookmarks/cor...
File components/bookmarks/core/common/bookmark_pref_names.cc (right):

https://codereview.chromium.org/228393004/diff/80001/components/bookmarks/cor...
components/bookmarks/core/common/bookmark_pref_names.cc:5: #include
"components/bookmarks/bookmark_pref_names.h"
On 2014/04/10 13:20:19, blundell wrote:
> thhe include path is wrong

Done.

https://codereview.chromium.org/228393004/diff/80001/components/bookmarks/cor...
File components/bookmarks/core/common/bookmark_pref_names.h (right):

https://codereview.chromium.org/228393004/diff/80001/components/bookmarks/cor...
components/bookmarks/core/common/bookmark_pref_names.h:7: #ifndef
COMPONENTS_BOOKMARKS_BOOKMARK_PREF_NAMES_H_
On 2014/04/10 13:20:19, blundell wrote:
> the include guard is wrong

Done.

[sdefresne, 2014-04-10 15:08:19]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-04-10 15:08:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sdefresne@chromium.org/228393004/100001

[commit-bot: I haz the power, 2014-04-10 17:03:24]

Change committed as 263016

[tfarina, 2014-04-10 17:48:26]

https://codereview.chromium.org/228393004/diff/100001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/228393004/diff/100001/chrome/browser/DEPS#new...
chrome/browser/DEPS:15: "+components/bookmarks",
you didn't need to include this just yet. only chrome/common/ was including from
components/bookmarks/core/common.

[tfarina, 2014-04-10 17:53:18]

https://codereview.chromium.org/228393004/diff/100001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/228393004/diff/100001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:31:
'../components/components.gyp:bookmarks_core_common',
also, no. I think this should have been added to dependencies list of
common_constants target.

https://codereview.chromium.org/228393004/diff/100001/components/components_t...
File components/components_tests.gyp (right):

https://codereview.chromium.org/228393004/diff/100001/components/components_t...
components/components_tests.gyp:183: 'components.gyp:bookmarks_core_common',
why did you add this dep here? As far I as can see components_unittests does not
have any bookmark tests yet and nobody needs to link with bookmark_pref_names.o
yet.

[sdefresne, 2014-04-10 21:23:51]

On 2014/04/10 17:53:18, tfarina wrote:
>
https://codereview.chromium.org/228393004/diff/100001/chrome/chrome_browser.gypi
> File chrome/chrome_browser.gypi (right):
> 
>
https://codereview.chromium.org/228393004/diff/100001/chrome/chrome_browser.g...
> chrome/chrome_browser.gypi:31:
> '../components/components.gyp:bookmarks_core_common',
> also, no. I think this should have been added to dependencies list of
> common_constants target.
> 
>
https://codereview.chromium.org/228393004/diff/100001/components/components_t...
> File components/components_tests.gyp (right):
> 
>
https://codereview.chromium.org/228393004/diff/100001/components/components_t...
> components/components_tests.gyp:183: 'components.gyp:bookmarks_core_common',
> why did you add this dep here? As far I as can see components_unittests does
not
> have any bookmark tests yet and nobody needs to link with
bookmark_pref_names.o
> yet.

Thank you for pointing those errors.
I've fixed them with https://codereview.chromium.org/233953002