Extract DependencyGraph from ProfileDependencyManager.

Extract DependencyGraph from ProfileDependencyManager.

This will be helpful for extracting non-Chrome parts
to its own component.

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

[Paweł Hajdan Jr., 2013-04-05 23:10:07]

Elliot, I'm going to polish it more (comments etc), but wanted to submit this so
more high-level issues can be resolved. Does the idea look good to you?

[Elliot Glaysher, 2013-04-05 23:44:15]

https://codereview.chromium.org/13454032/diff/1/chrome/browser/profiles/depen...
File chrome/browser/profiles/dependency_graph.h (right):

https://codereview.chromium.org/13454032/diff/1/chrome/browser/profiles/depen...
chrome/browser/profiles/dependency_graph.h:28: void AddEdge(void* depended,
void* dependee);
I'd prefer it if you got rid of these void pointers and replaced this with an
empty class which every Node had to derive from.

Theoretically, the Google style guide bans multiple inheritance. In practice,
especially in the UI code, we use it all over the place and there's historically
been lots of errors where the use of a void* or a static_cast<> has kept the C++
compiler from doing the correct pointer offset behaviour and I'd like to be
defensive here. Please define a DependencyNode class (or whatnot).

[Paweł Hajdan Jr., 2013-04-08 20:26:43]

https://codereview.chromium.org/13454032/diff/1/chrome/browser/profiles/depen...
File chrome/browser/profiles/dependency_graph.h (right):

https://codereview.chromium.org/13454032/diff/1/chrome/browser/profiles/depen...
chrome/browser/profiles/dependency_graph.h:28: void AddEdge(void* depended,
void* dependee);
On 2013/04/05 23:44:15, Elliot Glaysher wrote:
> I'd prefer it if you got rid of these void pointers and replaced this with an
> empty class which every Node had to derive from.
> 
> Theoretically, the Google style guide bans multiple inheritance. In practice,
> especially in the UI code, we use it all over the place and there's
historically
> been lots of errors where the use of a void* or a static_cast<> has kept the
C++
> compiler from doing the correct pointer offset behaviour and I'd like to be
> defensive here. Please define a DependencyNode class (or whatnot).

Glad to see a concern for safety and correctness.

I was thinking about the best way to address that too, and one thing I'm trying
to do is to remove the need for the class being a node to know in any way about
DependencyGraph.

I was considering a template-based solution to provide type safety. What do you
think about that? Convert everything to template, or just a templated wrapper
around void* ?

[Elliot Glaysher, 2013-04-11 20:09:28]

https://codereview.chromium.org/13454032/diff/1/chrome/browser/profiles/depen...
File chrome/browser/profiles/dependency_graph.h (right):

https://codereview.chromium.org/13454032/diff/1/chrome/browser/profiles/depen...
chrome/browser/profiles/dependency_graph.h:28: void AddEdge(void* depended,
void* dependee);
On 2013/04/08 20:26:43, Paweł Hajdan Jr. wrote:
> On 2013/04/05 23:44:15, Elliot Glaysher wrote:
> > I'd prefer it if you got rid of these void pointers and replaced this with
an
> > empty class which every Node had to derive from.
> > 
> > Theoretically, the Google style guide bans multiple inheritance. In
practice,
> > especially in the UI code, we use it all over the place and there's
> historically
> > been lots of errors where the use of a void* or a static_cast<> has kept the
> C++
> > compiler from doing the correct pointer offset behaviour and I'd like to be
> > defensive here. Please define a DependencyNode class (or whatnot).
> 
> Glad to see a concern for safety and correctness.
> 
> I was thinking about the best way to address that too, and one thing I'm
trying
> to do is to remove the need for the class being a node to know in any way
about
> DependencyGraph.

So what's the motivation for doing that? Creating a base node is straightforward
enough and doesn't make us depend on templates.

[Paweł Hajdan Jr., 2013-04-11 23:04:27]

PTAL

Now it uses a DependencyNode* instead of void*

The motivation is to extract non-chrome-specific parts out of
ProfileDependencyManager, so that eventually we can have a BrowserContext-based
dependency manager, or even one for some different objects (WebContentHelpers
are being considered).

I decided to extract the DependencyGraph first, because it's quite general and
could be used for many different things.

[Elliot Glaysher, 2013-04-12 20:15:06]

lgtm with maybe nit.

On 2013/04/11 23:04:27, Paweł Hajdan Jr. wrote:
> The motivation is to extract non-chrome-specific parts out of
> ProfileDependencyManager, so that eventually we can have a
BrowserContext-based
> dependency manager, or even one for some different objects (WebContentHelpers
> are being considered).

I think we're speaking past each other. I'm asking why you think that a class
needing to be a specific kind of node is bad. I take the opposite view: having a
base class allows a reader, at a glance, to know what is and isn't a member of a
dependency graph, which is useful information in its own right.

(Besides, you either expose this data explicitly to the compiler, make the
programmer keep track of types with casts, or you use templates which brings in
all the problems of templated solutions.)

https://codereview.chromium.org/13454032/diff/8001/chrome/browser/profiles/de...
File chrome/browser/profiles/dependency_node.h (right):

https://codereview.chromium.org/13454032/diff/8001/chrome/browser/profiles/de...
chrome/browser/profiles/dependency_node.h:11: ~DependencyNode() {}
does this need to be virtual?

[Paweł Hajdan Jr., 2013-04-12 22:59:31]

On 2013/04/12 20:15:06, Elliot Glaysher wrote:
> I think we're speaking past each other. I'm asking why you think that a class
> needing to be a specific kind of node is bad.

It's fine for me, I was just trying to apply the ideas from the ~design doc that
try to avoid that.

I think the idea is that when making a class use PKS dependency resolution
mechanism it shouldn't be necessary to modify the class itself, only add things
around it. I don't have a strong general opinion about that, except for keeping
things simple and safe.

> I take the opposite view: having a
> base class allows a reader, at a glance, to know what is and isn't a member of
a
> dependency graph, which is useful information in its own right.

I agree it's useful when searching across the codebase.

> (Besides, you either expose this data explicitly to the compiler, make the
> programmer keep track of types with casts, or you use templates which brings
in
> all the problems of templated solutions.)

Agreed.

I added comments in the latest patchset.

[Paweł Hajdan Jr., 2013-04-12 23:00:00]

Jói, could you also take a look?

https://codereview.chromium.org/13454032/diff/8001/chrome/browser/profiles/de...
File chrome/browser/profiles/dependency_node.h (right):

https://codereview.chromium.org/13454032/diff/8001/chrome/browser/profiles/de...
chrome/browser/profiles/dependency_node.h:11: ~DependencyNode() {}
On 2013/04/12 20:15:06, Elliot Glaysher wrote:
> does this need to be virtual?

I don't think it's needed - it seems there is a general preference not to make a
class require vptr unless necessary, so I'm trying to follow that here.

That would be different if it was a real interface with virtual methods, or if
we'd be destroying objects using DependencyNode* .

[Jói, 2013-04-14 15:32:59]

Sorry for the late response, I've been heads down preparing for a promo
committee meeting tomorrow, and right now am at the airport to get to the
meeting, on a very crappy connection. Let me see if I manage to review before my
flight, if not I'll try to take a look tonight since all of tomorrow will be
consumed by the meeting.

Cheers,
Jói

[Jói, 2013-04-14 15:36:02]

Looks like I have to get to my flight soon and this is too big for me to give a
proper review right now.

Let me say that I like the idea of extracting the dependency graph logic out to
be an independent class.  I also see no problem with having a DependencyNode
type that things participating in such a graph should inherit from; it seems
like the most straight-forward approach to make things safe.

Will try to take a better look tonight.

Cheers,
Jói

https://codereview.chromium.org/13454032/diff/15001/chrome/browser/profiles/d...
File chrome/browser/profiles/dependency_node.h (right):

https://codereview.chromium.org/13454032/diff/15001/chrome/browser/profiles/d...
chrome/browser/profiles/dependency_node.h:11: // This is intended to be used by
the subclass, not directly.
nit: the subclass -> subclasses

[Jói, 2013-04-14 23:13:20]

LGTM with a couple of nits.

https://codereview.chromium.org/13454032/diff/15001/chrome/browser/profiles/d...
File chrome/browser/profiles/dependency_graph.h (right):

https://codereview.chromium.org/13454032/diff/15001/chrome/browser/profiles/d...
chrome/browser/profiles/dependency_graph.h:22: // Constructor. The name callback
is used for debugging.
Clarify the documentation: Is a non-is_null() callback optional?

https://codereview.chromium.org/13454032/diff/15001/chrome/browser/profiles/d...
File chrome/browser/profiles/dependency_node.h (right):

https://codereview.chromium.org/13454032/diff/15001/chrome/browser/profiles/d...
chrome/browser/profiles/dependency_node.h:8: // Base class for use with
DependencyGraph.
suggest "Base class representing a node in a DependencyGraph."

[Paweł Hajdan Jr., 2013-04-15 19:07:40]

PTAL

https://codereview.chromium.org/13454032/diff/15001/chrome/browser/profiles/d...
File chrome/browser/profiles/dependency_graph.h (right):

https://codereview.chromium.org/13454032/diff/15001/chrome/browser/profiles/d...
chrome/browser/profiles/dependency_graph.h:22: // Constructor. The name callback
is used for debugging.
On 2013/04/14 23:13:20, Jói wrote:
> Clarify the documentation: Is a non-is_null() callback optional?

Done.

https://codereview.chromium.org/13454032/diff/15001/chrome/browser/profiles/d...
File chrome/browser/profiles/dependency_node.h (right):

https://codereview.chromium.org/13454032/diff/15001/chrome/browser/profiles/d...
chrome/browser/profiles/dependency_node.h:8: // Base class for use with
DependencyGraph.
On 2013/04/14 23:13:20, Jói wrote:
> suggest "Base class representing a node in a DependencyGraph."

Done.

https://codereview.chromium.org/13454032/diff/15001/chrome/browser/profiles/d...
chrome/browser/profiles/dependency_node.h:11: // This is intended to be used by
the subclass, not directly.
On 2013/04/14 15:36:02, Jói wrote:
> nit: the subclass -> subclasses

Done.

[Jói, 2013-04-15 20:56:00]

LGTM, thanks.

[Paweł Hajdan Jr., 2013-04-16 00:04:35]

Sorry for repeated submission - I added tests, based on
ProfileDependencyManager. I consider that a simple, almost mechanical change.

PTAL

[Jói, 2013-04-16 08:36:01]

https://codereview.chromium.org/13454032/diff/33001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_dependency_manager_unittest.cc (right):

https://codereview.chromium.org/13454032/diff/33001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_dependency_manager_unittest.cc:93:
TEST_F(ProfileDependencyManagerUnittests, MConfiguration) {
Just to check: Is it intentional to leave the same set of tests in this file as
well?

[Paweł Hajdan Jr., 2013-04-16 16:44:57]

https://codereview.chromium.org/13454032/diff/33001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_dependency_manager_unittest.cc (right):

https://codereview.chromium.org/13454032/diff/33001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_dependency_manager_unittest.cc:93:
TEST_F(ProfileDependencyManagerUnittests, MConfiguration) {
On 2013/04/16 08:36:01, Jói wrote:
> Just to check: Is it intentional to leave the same set of tests in this file
as
> well?

Yes, I think if the tests are there I'd rather keep them - note that
dependency_graph would move to a different place, while
profile_dependency_manager would stay here.

[Jói, 2013-04-16 19:42:53]

LGTM

On Tue, Apr 16, 2013 at 5:44 PM,  <phajdan.jr@chromium.org> wrote:
>
>
https://codereview.chromium.org/13454032/diff/33001/chrome/browser/profiles/p...
> File chrome/browser/profiles/profile_dependency_manager_unittest.cc
> (right):
>
>
https://codereview.chromium.org/13454032/diff/33001/chrome/browser/profiles/p...
> chrome/browser/profiles/profile_dependency_manager_unittest.cc:93:
> TEST_F(ProfileDependencyManagerUnittests, MConfiguration) {
> On 2013/04/16 08:36:01, Jói wrote:
>>
>> Just to check: Is it intentional to leave the same set of tests in
>
> this file as
>>
>> well?
>
>
> Yes, I think if the tests are there I'd rather keep them - note that
> dependency_graph would move to a different place, while
> profile_dependency_manager would stay here.
>
> https://codereview.chromium.org/13454032/

[Paweł Hajdan Jr., 2013-04-16 21:05:21]

Committed patchset #5 manually as r194449.