Componentize visitedlinks to src/components/visitedlink

Componentize visitedlinks to src/components/visitedlink

All visitedlink files moved to src/components/visitedlink/[browser|common|renderer|test]
Created static lib targets for browser, common, and renderer.
Test files are moved but still uses old test targets in chrome/

Dcommitted due to AllowScopedIO being moved but still
triggers presubmit error. Bots are mostly happy.

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

[boliu, 2013-01-08 23:42:08]

I haven't tried moving the tests, not sure how hard it is to create a test
harness. Also if there are new tests targets, then all the bots will need to be
updated to run those tests, so feels like I should keep the tests in this CL no
matter what?

For message_generator, it would be really really nice if we can share a common
file between all the different components since this is all boilerplate code,
but I guess that is not possible (without hacks) since each component is a
separate statically linked library?

There is a presubmit error:
** Presubmit ERRORS **
Banned functions were used.
    components/visitedlink/browser/visitedlink_master.cc:238:
      New code should not use ScopedAllowIO. Post a task to the blocking
      pool or the FILE thread instead.

I plan to just ignore this for this CL.

https://codereview.chromium.org/11825011/diff/1/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/11825011/diff/1/components/components.gyp#new...
components/components.gyp:5: {
I think should have a common components.gyp that just includes a whole bunch of
gypi's.

https://codereview.chromium.org/11825011/diff/1/components/visitedlink/OWNERS
File components/visitedlink/OWNERS (right):

https://codereview.chromium.org/11825011/diff/1/components/visitedlink/OWNERS...
components/visitedlink/OWNERS:1: brettw@chromium.org
these used to be chrome/browser/visitedlink/OWNERS

[Jói, 2013-01-09 00:51:44]

Seems reasonable to me to ignore the ScopedAllowIO error since you are just
moving code.

See comments below about how to handle tests in the short term.  In the near
term we need to decide how to handle tests for components, primarily whether we
establish a components_unittests target that collects all tests within
src/component/ or whether we want to go more fine-grained than that.  The latter
option has obvious benefits but the drawack at the moment is that there is
significant work involved in adding new tests to the configuration for our bots.

For message generation I think we should probably leave it per-component so that
they are independent of each other.

Cheers,
Jói

https://codereview.chromium.org/11825011/diff/1/chrome/browser/visitedlink/vi...
File chrome/browser/visitedlink/visitedlink_unittest.cc (right):

https://codereview.chromium.org/11825011/diff/1/chrome/browser/visitedlink/vi...
chrome/browser/visitedlink/visitedlink_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
You may want to move this file with the code being componentized.

What we've done previously with unit tests of stuff that's moving out, is we've
moved the unit test implementations but left them being compiled in .gypi files
under src/chrome/ with a TODO to break them out.

https://codereview.chromium.org/11825011/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/11825011/diff/1/chrome/chrome_tests.gypi#newc...
chrome/chrome_tests.gypi:2338: #'browser/visitedlink/visitedlink_perftest.cc',
Did you intend to leave this commented out?

https://codereview.chromium.org/11825011/diff/1/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/11825011/diff/1/chrome/chrome_tests_unit.gypi...
chrome/chrome_tests_unit.gypi:1427:
#'browser/visitedlink/visitedlink_unittest.cc',
As I mentioned, we've done this before by:
- Moving to the bottom of the source list
- Including via relative paths, in your case I guess
'../components/visitedlink/browser/visitedlink_unittest.cc'
- Adding a TODO to break the files out into separate test executables (or at
least move to a test executable other than 'unit_tests').

https://codereview.chromium.org/11825011/diff/1/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/11825011/diff/1/components/components.gyp#new...
components/components.gyp:5: {
On 2013/01/08 23:42:08, boliu wrote:
> I think should have a common components.gyp that just includes a whole bunch
of
> gypi's.

I tend to agree.  You can add the existing .gypi file for the tracing component
to this, or we can do it in a follow-up change.

https://codereview.chromium.org/11825011/diff/1/components/visitedlink/visite...
File components/visitedlink/visitedlink.gypi (right):

https://codereview.chromium.org/11825011/diff/1/components/visitedlink/visite...
components/visitedlink/visitedlink.gypi:6: 'targets': [
It would be nice if there was a 'visitedlink' target that in the components
build would be a DLL.  Can do in a follow-up change, since it means establishing
the right exports etc.

[boliu, 2013-01-09 01:16:40]

Are you aware of other important build configurations that are not tested on the
cq, or additional fyi trybots that I should run through before committing?

https://codereview.chromium.org/11825011/diff/1/chrome/browser/visitedlink/vi...
File chrome/browser/visitedlink/visitedlink_unittest.cc (right):

https://codereview.chromium.org/11825011/diff/1/chrome/browser/visitedlink/vi...
chrome/browser/visitedlink/visitedlink_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/01/09 00:51:44, Jói wrote:
> You may want to move this file with the code being componentized.
> 
> What we've done previously with unit tests of stuff that's moving out, is
we've
> moved the unit test implementations but left them being compiled in .gypi
files
> under src/chrome/ with a TODO to break them out.

Great idea!!

There is also perftest.cc. Do you think that should move in this CL and be
componentized eventually as well?

https://codereview.chromium.org/11825011/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/11825011/diff/1/chrome/chrome_tests.gypi#newc...
chrome/chrome_tests.gypi:2338: #'browser/visitedlink/visitedlink_perftest.cc',
On 2013/01/09 00:51:44, Jói wrote:
> Did you intend to leave this commented out?

Sort of...I moved the tests then moved them back when I realized I had to create
test harness.

https://codereview.chromium.org/11825011/diff/1/components/visitedlink/visite...
File components/visitedlink/visitedlink.gypi (right):

https://codereview.chromium.org/11825011/diff/1/components/visitedlink/visite...
components/visitedlink/visitedlink.gypi:6: 'targets': [
On 2013/01/09 00:51:44, Jói wrote:
> It would be nice if there was a 'visitedlink' target that in the components
> build would be a DLL.  Can do in a follow-up change, since it means
establishing
> the right exports etc.

I don't entirely know what this means. I've heard of components build and that
recent thread on windows pdb size, but I've never tried to use it, or ever
compiled on windows.

My guess is this is just a case of copy paste gyp configuration? Do you have an
example of this somewhere?

[Jói, 2013-01-09 18:42:42]

I think the default set of CQ trybots should be sufficient for this change,
since really it's just moving stuff around.

Cheers,
Jói

https://codereview.chromium.org/11825011/diff/1/chrome/browser/visitedlink/vi...
File chrome/browser/visitedlink/visitedlink_unittest.cc (right):

https://codereview.chromium.org/11825011/diff/1/chrome/browser/visitedlink/vi...
chrome/browser/visitedlink/visitedlink_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/01/09 01:16:40, boliu wrote:
> On 2013/01/09 00:51:44, Jói wrote:
> > You may want to move this file with the code being componentized.
> > 
> > What we've done previously with unit tests of stuff that's moving out, is
> we've
> > moved the unit test implementations but left them being compiled in .gypi
> files
> > under src/chrome/ with a TODO to break them out.
> 
> Great idea!!
> 
> There is also perftest.cc. Do you think that should move in this CL and be
> componentized eventually as well?

Yes, it should move.  It has no chrome/ dependencies so could easily be moved
later to a components_perftests target or some such.

https://codereview.chromium.org/11825011/diff/1/components/visitedlink/visite...
File components/visitedlink/visitedlink.gypi (right):

https://codereview.chromium.org/11825011/diff/1/components/visitedlink/visite...
components/visitedlink/visitedlink.gypi:6: 'targets': [
On 2013/01/09 01:16:40, boliu wrote:
> On 2013/01/09 00:51:44, Jói wrote:
> > It would be nice if there was a 'visitedlink' target that in the components
> > build would be a DLL.  Can do in a follow-up change, since it means
> establishing
> > the right exports etc.
> 
> I don't entirely know what this means. I've heard of components build and that
> recent thread on windows pdb size, but I've never tried to use it, or ever
> compiled on windows.
> 
> My guess is this is just a case of copy paste gyp configuration? Do you have
an
> example of this somewhere?

You use '<(component)' as the type of the target; this makes it a lib in the
normal build, and a DLL in the component build.  See e.g. base/base.gyp and
search for 'base_prefs'.

In that case I think 'visitedlink_common' would remain a lib, linked into both
'visitedlink_browser' and 'visitedlink_renderer' which could each be a
'<(component)'.

Once a target is sometimes built as a DLL, you need to add export macros to
export the relevant bits out of the DLL.  To see this in action in the Prefs
target, search for BASE_PREFS_EXPORT.

[boliu, 2013-01-10 01:33:31]

Oops, was not trying to send out mail..

Anyway, Joi: could you do a first pass for review, I think it's building on all
platforms now.

https://codereview.chromium.org/11825011/diff/13001/components/visitedlink/vi...
File components/visitedlink/visitedlink.gypi (right):

https://codereview.chromium.org/11825011/diff/13001/components/visitedlink/vi...
components/visitedlink/visitedlink.gypi:12: '../skia/config',
this is needed for mac build apparently

copied over from other places in the tree

[Jói, 2013-01-10 02:01:40]

LGTM

[boliu, 2013-01-10 04:22:36]

brettw,sky: chrome/, and notify I added you guys as owner of all of visitedlink
component

jln: security stamp for ipc messages (only moving messages, no new messages)

https://codereview.chromium.org/11825011/diff/8053/components/visitedlink/com...
File components/visitedlink/common/OWNERS (right):

https://codereview.chromium.org/11825011/diff/8053/components/visitedlink/com...
components/visitedlink/common/OWNERS:1: # Changes to IPC messages require a
security review to avoid introducing
Should this have per-file set noparent?

[jln (very slow on Chromium), 2013-01-10 05:03:05]

*_messages.h lgtm on the basis that this is a move.

https://codereview.chromium.org/11825011/diff/8053/components/visitedlink/com...
File components/visitedlink/common/OWNERS (right):

https://codereview.chromium.org/11825011/diff/8053/components/visitedlink/com...
components/visitedlink/common/OWNERS:1: # Changes to IPC messages require a
security review to avoid introducing
On 2013/01/10 04:22:37, boliu wrote:
> Should this have per-file set noparent?

I don't understand your question. Currently it does have per-file no parent, do
you think that's wrong?

[boliu, 2013-01-10 05:53:55]

https://codereview.chromium.org/11825011/diff/8053/components/visitedlink/com...
File components/visitedlink/common/OWNERS (right):

https://codereview.chromium.org/11825011/diff/8053/components/visitedlink/com...
components/visitedlink/common/OWNERS:1: # Changes to IPC messages require a
security review to avoid introducing
On 2013/01/10 05:03:05, Julien Tinnes wrote:
> On 2013/01/10 04:22:37, boliu wrote:
> > Should this have per-file set noparent?
> 
> I don't understand your question. Currently it does have per-file no parent,
do
> you think that's wrong?

Oops. I can't read my own (copied) code...

[boliu, 2013-01-11 01:36:39]

brettw,sky: ping!

[boliu, 2013-01-14 21:55:23]

On 2013/01/11 01:36:39, boliu wrote:
> brettw,sky: ping!

ping! x2

[sky, 2013-01-14 22:20:33]

Adding multiple reviewers for the same code is a recipe for everyone
to ignore. Brett knows the visitedlink code better than I, so he
should review this (sorry Brett).

  -Scott

On Mon, Jan 14, 2013 at 1:55 PM,  <boliu@chromium.org> wrote:
> On 2013/01/11 01:36:39, boliu wrote:
>>
>> brettw,sky: ping!
>
>
> ping! x2
>
> https://codereview.chromium.org/11825011/

[brettw, 2013-01-18 21:58:47]

LGTM, just some nits.

https://codereview.chromium.org/11825011/diff/18001/chrome/browser/history/hi...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/11825011/diff/18001/chrome/browser/history/hi...
chrome/browser/history/history_backend.cc:51: using
components::VisitedLinkDelegate;
I think this is only used like once. I'd remove the using and just update that
place.

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/DEPS
File components/visitedlink/DEPS (right):

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/DE...
components/visitedlink/DEPS:1: # Empty since include rules are covered by DEPS
in subdirectories and src/DEPS.
I'd just skip adding this file if it's empty.

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/br...
File components/visitedlink/browser/visitedlink_event_listener.h (right):

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/br...
components/visitedlink/browser/visitedlink_event_listener.h:26: class
VisitedLinkUpdater;
Just move this inside the components namespace below.

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/br...
File components/visitedlink/browser/visitedlink_master.h (right):

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/br...
components/visitedlink/browser/visitedlink_master.h:29: class
VisitedLinkDelegate;
Ditto

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/te...
File components/visitedlink/test/DEPS (right):

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/te...
components/visitedlink/test/DEPS:2: "+components/visitedlink",
I thought this was unnecessary since you can see the previous dir by default. Is
that not true?

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/te...
File components/visitedlink/test/visitedlink_perftest.cc (right):

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/te...
components/visitedlink/test/visitedlink_perftest.cc:20: using
components::VisitedLinkCommon;
I'd just wrap this file in "namespace components" instead.

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/te...
File components/visitedlink/test/visitedlink_unittest.cc (right):

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/te...
components/visitedlink/test/visitedlink_unittest.cc:32: using
components::VisitedLinkCommon;
Ditto.

[boliu, 2013-01-18 22:42:15]

Will wait for the tryjobs go green and dcommit due to the presubmit warning
about ThreadRestrictions::ScopedAllowIO in VisitedLinkMaster::Init (entire file
was moved).

https://codereview.chromium.org/11825011/diff/18001/chrome/browser/history/hi...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/11825011/diff/18001/chrome/browser/history/hi...
chrome/browser/history/history_backend.cc:51: using
components::VisitedLinkDelegate;
On 2013/01/18 21:58:47, brettw wrote:
> I think this is only used like once. I'd remove the using and just update that
> place.

Done.

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/DEPS
File components/visitedlink/DEPS (right):

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/DE...
components/visitedlink/DEPS:1: # Empty since include rules are covered by DEPS
in subdirectories and src/DEPS.
On 2013/01/18 21:58:47, brettw wrote:
> I'd just skip adding this file if it's empty.

Done.

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/br...
File components/visitedlink/browser/visitedlink_event_listener.h (right):

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/br...
components/visitedlink/browser/visitedlink_event_listener.h:26: class
VisitedLinkUpdater;
On 2013/01/18 21:58:47, brettw wrote:
> Just move this inside the components namespace below.

Done.

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/te...
File components/visitedlink/test/DEPS (right):

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/te...
components/visitedlink/test/DEPS:2: "+components/visitedlink",
On 2013/01/18 21:58:47, brettw wrote:
> I thought this was unnecessary since you can see the previous dir by default.
Is
> that not true?

You are right. Learning new things about our tools

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/te...
File components/visitedlink/test/visitedlink_perftest.cc (right):

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/te...
components/visitedlink/test/visitedlink_perftest.cc:20: using
components::VisitedLinkCommon;
On 2013/01/18 21:58:47, brettw wrote:
> I'd just wrap this file in "namespace components" instead.

Done.

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/te...
File components/visitedlink/test/visitedlink_unittest.cc (right):

https://codereview.chromium.org/11825011/diff/18001/components/visitedlink/te...
components/visitedlink/test/visitedlink_unittest.cc:32: using
components::VisitedLinkCommon;
On 2013/01/18 21:58:47, brettw wrote:
> Ditto.

Done.

[boliu, 2013-01-18 23:57:33]

Joi: Please review components/visitedlink.gypi again, I had to exclude
visitedlink_renderer on ios due to compile error. For reference,
chrome_renderer.gypi is excluded for ios under chrome/.

Now I'm wondering if putting everything in a single components.gyp is such a
good idea...

[Jói, 2013-01-21 10:32:07]

LGTM

I don't see why the fact that some .gypi files might need conditional
compilation would impact whether or not we should use a single
//components/components.gyp file?

Cheers,
Jói

[boliu, 2013-01-23 00:51:23]

dcommitted. Most bot runs are on PS16. Hoping it won't break anything...

On 2013/01/21 10:32:07, Jói wrote:
> LGTM
> 
> I don't see why the fact that some .gypi files might need conditional
> compilation would impact whether or not we should use a single
> //components/components.gyp file?
> 
> Cheers,
> Jói

As I understand it, gyp will process all targets and its dependencies in a gyp
file, even if it is not needed from the all target. So in this case, if there
were a separate visitedlink_renderer.gyp, then that gyp file would not be
included by ios in the first place so there would be no need for the guards.

It feels like components would be structured such that different OSs can pick
and choose which components to include without modifying the component gyp
files.

I don't feel too strongly either way, just that I started components.gyp, and
here is a point against that decision.

[Jói, 2013-01-23 10:05:06]

> [...] So in this case, if
> there
> were a separate visitedlink_renderer.gyp, then that gyp file would not be
> included by ios in the first place so there would be no need for the guards.

I think you could actually use a conditional block in components.gyp
to fully exclude components_visitedlink.gypi on iOS, instead of having
the conditional block in components_visitedlink.gypi; this seems like
it would achieve nearly the same thing.  Not sure it's cleaner than
what you have now, though.

In either case, I think a single components.gyp is fine, and it echoes
the structure of .gyp files from other top-level directories.

Cheers,
Jói



On Wed, Jan 23, 2013 at 12:51 AM,  <boliu@chromium.org> wrote:
> dcommitted. Most bot runs are on PS16. Hoping it won't break anything...
>
>
> On 2013/01/21 10:32:07, Jói wrote:
>>
>> LGTM
>
>
>> I don't see why the fact that some .gypi files might need conditional
>> compilation would impact whether or not we should use a single
>> //components/components.gyp file?
>
>
>> Cheers,
>> Jói
>
>
> As I understand it, gyp will process all targets and its dependencies in a
> gyp
> file, even if it is not needed from the all target. So in this case, if
> there
> were a separate visitedlink_renderer.gyp, then that gyp file would not be
> included by ios in the first place so there would be no need for the guards.
>
> It feels like components would be structured such that different OSs can
> pick
> and choose which components to include without modifying the component gyp
> files.
>
> I don't feel too strongly either way, just that I started components.gyp,
> and
> here is a point against that decision.
>
> https://codereview.chromium.org/11825011/