Create chrome/browser/api directory. Move infobar delegates used by Autofill to the directory.

Create chrome/browser/api directory. Move infobar delegates used by Autofill to the directory.

Also, consolidate infobar delegate interfaces under infobars/ rather than some under infobars/ and some under tab_contents/, as the ones under tab_contents/ were not coupled with tab_contents/ in any way.

Remove feedback/proto/extension.pb.h from Autofill's DEPS file simply by dropping the include, it was not being used.

BUG=140037, 138280
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151279

[Jói, 2012-08-07 15:54:12]

Erik, I plan to add ben@ as a reviewer once you agree this looks good.

Other owners will be TBRed since the relevant updates are just changes to the
include paths in question.

Cheers,
Jói

[tfarina, 2012-08-07 15:57:46]

On 2012/08/07 15:54:12, Jói wrote:
> Erik, I plan to add ben@ as a reviewer once you agree this looks good.
> 
> Other owners will be TBRed since the relevant updates are just changes to the
> include paths in question.
> 
> Cheers,
> Jói

Joi, can we follow the content/ convention here?

i.e.:
content/public/browser/

for chrome:
chrome/public/browser

?

I think api is confusing, because I can see everything as an API, a simple
function declaration might be viewed as an API, etc.

Following the content design might be less confusing I think and more familiar
to everyone.

[erikwright (departed), 2012-08-07 21:11:15]

LGTM. I suggest adding a bug for the continued componentization of infobars.
Seems isolated enough that a new team member or even someone looking for a good
first bug could take it on. The more well-defined stuff we can farm out, the
better.

[Jói, 2012-08-08 14:52:19]

Hi Thiago,

I'm not sure it's good to follow the content/ convention for chrome/
for a number of reasons:

a) chrome/ is not in itself a reusable component, like content/ is.

b) chrome/browser/api is not really analogous to
content/public/browser; the latter is an API for external use whereas
the former is primarily for use _within_ chrome/ or by components
dependent on running inside of Chrome.

c) Individual Browser Components may also (perhaps more down the line)
have an api/ subdirectory (e.g. we might have
chrome/browser/bookmarks/api) for APIs they expose for use by other
components.  Thus I don't think following the same top-level "public"
directory convention as content (e.g. chrome/[NAME_OF_API_DIR]/browser
rather than chrome/browser/[NAME_OF_API_DIR]) is necessarily good.

In any case, the DEPS rules and documentation within the api/
subdirectory make it clear what it is.  We could call it 'public'
rather than 'api' and leave everything else the same, but I'm not sure
we would gain much.  'api' is shorter, and pretty clear IMO.

Cheers,
Jói


On Tue, Aug 7, 2012 at 3:57 PM,  <tfarina@chromium.org> wrote:
> On 2012/08/07 15:54:12, Jói wrote:
>>
>> Erik, I plan to add ben@ as a reviewer once you agree this looks good.
>
>
>> Other owners will be TBRed since the relevant updates are just changes to
>> the
>> include paths in question.
>
>
>> Cheers,
>> Jói
>
>
> Joi, can we follow the content/ convention here?
>
> i.e.:
> content/public/browser/
>
> for chrome:
> chrome/public/browser
>
> ?
>
> I think api is confusing, because I can see everything as an API, a simple
> function declaration might be viewed as an API, etc.
>
> Following the content design might be less confusing I think and more
> familiar
> to everyone.
>
>
> http://codereview.chromium.org/10843071/

[Jói, 2012-08-08 14:53:11]

Thanks Erik, sounds good to file a bug for infobars componentization.
Would you do the honors?

Cheers,
Jói


On Tue, Aug 7, 2012 at 9:11 PM,  <erikwright@chromium.org> wrote:
> LGTM. I suggest adding a bug for the continued componentization of infobars.
> Seems isolated enough that a new team member or even someone looking for a
> good
> first bug could take it on. The more well-defined stuff we can farm out, the
> better.
>
> http://codereview.chromium.org/10843071/

[Jói, 2012-08-08 14:54:37]

+ben@ as reviewer.

Will TBR for other required OWNERS approvals since they are just changes in
include paths.

Cheers,
Jói

[jam, 2012-08-08 16:29:58]

drive-by (didn't look in detail, this caught my attention)

http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
File chrome/browser/api/infobars/infobar_tab_helper.h (right):

http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
chrome/browser/api/infobars/infobar_tab_helper.h:16: class InfoBarTabHelper :
public content::WebContentsObserver,
i'm confused, this doesn't seem like an api of infobars that's provided by the
browser. i realize some stuff in here is needed by code that puts up an infobar,
so maybe a new interface should be added here. but this header seems like it
should stay in chrome with the infobar_tab_helper.cc file, which is an internal
implementation detail of TabContents.

[Jói, 2012-08-08 16:49:11]

It's poorly named.  InfoBarTabHelper is actually where you go to add
an infobar, remove an infobar, and replace an infobar.

Note also that in the Browser Components project, we decided we would
take as many easy victories as we can first, and then refine the API
as we go along.  For this reason, we are allowing non-abstract classes
in the api directory as long as they do not introduce header
dependencies on chrome/browser outside the api directory.  This is a
for-now kind of thing; later we will introduce tools to let us write
rules about link-time dependencies and will minimize those just as we
are doing for header dependencies, and this may force introduction of
abstract bases in some cases.  For this class in particular, there are
probably a few methods that could be moved out of the class that lives
in the api directory and into a subclass, e.g. the count of infobars
and the way to enumerate delegates.  I should probably add a TODO to
minimize the surface here.

See some more discussion in the chrome/browser/api/DEPS file in this change.

Cheers,
Jói



On Wed, Aug 8, 2012 at 4:29 PM,  <jam@chromium.org> wrote:
> drive-by (didn't look in detail, this caught my attention)
>
>
>
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
> File chrome/browser/api/infobars/infobar_tab_helper.h (right):
>
>
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
> chrome/browser/api/infobars/infobar_tab_helper.h:16: class
> InfoBarTabHelper : public content::WebContentsObserver,
> i'm confused, this doesn't seem like an api of infobars that's provided
> by the browser. i realize some stuff in here is needed by code that puts
> up an infobar, so maybe a new interface should be added here. but this
> header seems like it should stay in chrome with the
> infobar_tab_helper.cc file, which is an internal implementation detail
> of TabContents.
>
>
> http://codereview.chromium.org/10843071/
>
> --
> You received this message because you are subscribed to the Google Groups
> "browser-components-watch" group.
> To post to this group, send email to browser-components-watch@chromium.org.
> To unsubscribe from this group, send email to
> browser-components-watch+unsubscribe@chromium.org.
> For more options, visit
> https://groups.google.com/a/chromium.org/groups/opt_out.
>
>

[jam, 2012-08-08 16:55:21]

On Wed, Aug 8, 2012 at 9:48 AM, Jói Sigurðsson <joi@chromium.org> wrote:

> It's poorly named.  InfoBarTabHelper is actually where you go to add
> an infobar, remove an infobar, and replace an infobar.
>

Right, that's why I suggested maybe adding an interface here that allows
one to do this.


> Note also that in the Browser Components project, we decided we would
> take as many easy victories as we can first, and then refine the API
> as we go along.  For this reason, we are allowing non-abstract classes
> in the api directory as long as they do not introduce header
> dependencies on chrome/browser outside the api directory.  This is a
> for-now kind of thing; later we will introduce tools to let us write
> rules about link-time dependencies and will minimize those just as we
> are doing for header dependencies, and this may force introduction of
> abstract bases in some cases.  For this class in particular, there are
> probably a few methods that could be moved out of the class that lives
> in the api directory and into a subclass, e.g. the count of infobars
> and the way to enumerate delegates.  I should probably add a TODO to
> minimize the surface here.
>

I don't think this header should be in a separate directory from its cc
file. It's not just an api, it's also an implementation detail of
tabcontents. in the api directory, there could be a new interface that has
the add/remove infobar interface. until that interface is made, a temporary
DEPS exceptions for code to include infobar_tab_helper.h from component
directories could be allowed.


> See some more discussion in the chrome/browser/api/DEPS file in this
> change.
>
> Cheers,
> Jói
>
>
>
> On Wed, Aug 8, 2012 at 4:29 PM,  <jam@chromium.org> wrote:
> > drive-by (didn't look in detail, this caught my attention)
> >
> >
> >
>
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
> > File chrome/browser/api/infobars/infobar_tab_helper.h (right):
> >
> >
>
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
> > chrome/browser/api/infobars/infobar_tab_helper.h:16: class
> > InfoBarTabHelper : public content::WebContentsObserver,
> > i'm confused, this doesn't seem like an api of infobars that's provided
> > by the browser. i realize some stuff in here is needed by code that puts
> > up an infobar, so maybe a new interface should be added here. but this
> > header seems like it should stay in chrome with the
> > infobar_tab_helper.cc file, which is an internal implementation detail
> > of TabContents.
> >
> >
> > http://codereview.chromium.org/10843071/
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "browser-components-watch" group.
> > To post to this group, send email to
> browser-components-watch@chromium.org.
> > To unsubscribe from this group, send email to
> > browser-components-watch+unsubscribe@chromium.org.
> > For more options, visit
> > https://groups.google.com/a/chromium.org/groups/opt_out.
> >
> >
>

[Ben Goodger (Google), 2012-08-08 17:58:59]

+1. The api dir should contain an abstract interface that the components
use to talk to the browser.


On Wed, Aug 8, 2012 at 9:55 AM, John Abd-El-Malek <jam@chromium.org> wrote:

>
>
> On Wed, Aug 8, 2012 at 9:48 AM, Jói Sigurðsson <joi@chromium.org> wrote:
>
>> It's poorly named.  InfoBarTabHelper is actually where you go to add
>> an infobar, remove an infobar, and replace an infobar.
>>
>
> Right, that's why I suggested maybe adding an interface here that allows
> one to do this.
>
>
>> Note also that in the Browser Components project, we decided we would
>> take as many easy victories as we can first, and then refine the API
>> as we go along.  For this reason, we are allowing non-abstract classes
>> in the api directory as long as they do not introduce header
>> dependencies on chrome/browser outside the api directory.  This is a
>> for-now kind of thing; later we will introduce tools to let us write
>> rules about link-time dependencies and will minimize those just as we
>> are doing for header dependencies, and this may force introduction of
>> abstract bases in some cases.  For this class in particular, there are
>> probably a few methods that could be moved out of the class that lives
>> in the api directory and into a subclass, e.g. the count of infobars
>> and the way to enumerate delegates.  I should probably add a TODO to
>> minimize the surface here.
>>
>
> I don't think this header should be in a separate directory from its cc
> file. It's not just an api, it's also an implementation detail of
> tabcontents. in the api directory, there could be a new interface that has
> the add/remove infobar interface. until that interface is made, a temporary
> DEPS exceptions for code to include infobar_tab_helper.h from component
> directories could be allowed.
>
>
>> See some more discussion in the chrome/browser/api/DEPS file in this
>> change.
>>
>> Cheers,
>> Jói
>>
>>
>>
>> On Wed, Aug 8, 2012 at 4:29 PM,  <jam@chromium.org> wrote:
>> > drive-by (didn't look in detail, this caught my attention)
>> >
>> >
>> >
>>
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>> > File chrome/browser/api/infobars/infobar_tab_helper.h (right):
>> >
>> >
>>
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>> > chrome/browser/api/infobars/infobar_tab_helper.h:16: class
>> > InfoBarTabHelper : public content::WebContentsObserver,
>> > i'm confused, this doesn't seem like an api of infobars that's provided
>> > by the browser. i realize some stuff in here is needed by code that puts
>> > up an infobar, so maybe a new interface should be added here. but this
>> > header seems like it should stay in chrome with the
>> > infobar_tab_helper.cc file, which is an internal implementation detail
>> > of TabContents.
>> >
>> >
>> > http://codereview.chromium.org/10843071/
>> >
>> > --
>> > You received this message because you are subscribed to the Google
>> Groups
>> > "browser-components-watch" group.
>> > To post to this group, send email to
>> browser-components-watch@chromium.org.
>> > To unsubscribe from this group, send email to
>> > browser-components-watch+unsubscribe@chromium.org.
>> > For more options, visit
>> > https://groups.google.com/a/chromium.org/groups/opt_out.
>> >
>> >
>>
>
>

[joi, 2012-08-08 18:37:53]

Ben and John, thanks for the feedback. I have some thoughts on this subject
but am about to meet some friends for dinner. Will try to respond later
tonight.

Cheers,
Jói
On Aug 8, 2012 5:58 PM, "Ben Goodger (Google)" <ben@chromium.org> wrote:

> +1. The api dir should contain an abstract interface that the components
> use to talk to the browser.
>
>
> On Wed, Aug 8, 2012 at 9:55 AM, John Abd-El-Malek <jam@chromium.org>wrote:
>
>>
>>
>> On Wed, Aug 8, 2012 at 9:48 AM, Jói Sigurðsson <joi@chromium.org> wrote:
>>
>>> It's poorly named.  InfoBarTabHelper is actually where you go to add
>>> an infobar, remove an infobar, and replace an infobar.
>>>
>>
>> Right, that's why I suggested maybe adding an interface here that allows
>> one to do this.
>>
>>
>>> Note also that in the Browser Components project, we decided we would
>>> take as many easy victories as we can first, and then refine the API
>>> as we go along.  For this reason, we are allowing non-abstract classes
>>> in the api directory as long as they do not introduce header
>>> dependencies on chrome/browser outside the api directory.  This is a
>>> for-now kind of thing; later we will introduce tools to let us write
>>> rules about link-time dependencies and will minimize those just as we
>>> are doing for header dependencies, and this may force introduction of
>>> abstract bases in some cases.  For this class in particular, there are
>>> probably a few methods that could be moved out of the class that lives
>>> in the api directory and into a subclass, e.g. the count of infobars
>>> and the way to enumerate delegates.  I should probably add a TODO to
>>> minimize the surface here.
>>>
>>
>> I don't think this header should be in a separate directory from its cc
>> file. It's not just an api, it's also an implementation detail of
>> tabcontents. in the api directory, there could be a new interface that has
>> the add/remove infobar interface. until that interface is made, a temporary
>> DEPS exceptions for code to include infobar_tab_helper.h from component
>> directories could be allowed.
>>
>>
>>> See some more discussion in the chrome/browser/api/DEPS file in this
>>> change.
>>>
>>> Cheers,
>>> Jói
>>>
>>>
>>>
>>> On Wed, Aug 8, 2012 at 4:29 PM,  <jam@chromium.org> wrote:
>>> > drive-by (didn't look in detail, this caught my attention)
>>> >
>>> >
>>> >
>>>
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>>> > File chrome/browser/api/infobars/infobar_tab_helper.h (right):
>>> >
>>> >
>>>
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>>> > chrome/browser/api/infobars/infobar_tab_helper.h:16: class
>>> > InfoBarTabHelper : public content::WebContentsObserver,
>>> > i'm confused, this doesn't seem like an api of infobars that's provided
>>> > by the browser. i realize some stuff in here is needed by code that
>>> puts
>>> > up an infobar, so maybe a new interface should be added here. but this
>>> > header seems like it should stay in chrome with the
>>> > infobar_tab_helper.cc file, which is an internal implementation detail
>>> > of TabContents.
>>> >
>>> >
>>> > http://codereview.chromium.org/10843071/
>>> >
>>> > --
>>> > You received this message because you are subscribed to the Google
>>> Groups
>>> > "browser-components-watch" group.
>>> > To post to this group, send email to
>>> browser-components-watch@chromium.org.
>>> > To unsubscribe from this group, send email to
>>> > browser-components-watch+unsubscribe@chromium.org.
>>> > For more options, visit
>>> > https://groups.google.com/a/chromium.org/groups/opt_out.
>>> >
>>> >
>>>
>>
>>
>  --
> You received this message because you are subscribed to the Google Groups
> "browser-components-watch" group.
> To post to this group, send email to browser-components-watch@chromium.org
> .
> To unsubscribe from this group, send email to
> browser-components-watch+unsubscribe@chromium.org.
> For more options, visit
> https://groups.google.com/a/chromium.org/groups/opt_out.
>
>
>

[Jói, 2012-08-08 23:20:14]

[+browser-components-dev]

I'm back, so now let me try to explain my thoughts on this.  I'm not
going to try to summarize, because I think it's important that the
stakeholders in this read through my reasoning, but I will try to keep
this structured.

I'd like to leave the specifics of the current change alone for now,
and focus on the general principles we're operating on.  We can
discuss the specifics of the current change once we settle on the
principles.  I tried to make a similar case to the one I'm making
below while we were doing the content/ work, but I don't think I
explained it well enough or fought for it hard enough, plus the
situation there was somewhat different.

I think the issues below are important to discuss, because the choice
we make can have a large impact on how quickly we can move, in
particular how much progress we can make in a reasonably short period
of time, and quick progress in this area is important since it can
prevent changes from creeping into the codebase that otherwise will
take time to fix up later.

Ben and John, I think both of you are stating a preference that all or
at least most classes in the api directory should be abstract or very
close to abstract, and likely extracted from existing classes similar
to what we did for content/ (e.g. a class previously known as Foo
might be split into an abstract content::Foo interface and a concrete
content::FooImpl implementation).  Pardon me if that's not the exact
position you're representing; hopefully the following is still a worth
while discussion.

To state my position, I don't think we should extract abstract bases
for everything we place in the api directory.  I do think that we
will end up extracting a lot of abstract bases before we are done, but
I think we should do it only to the extent that it becomes practically
necessary to reach our goals.  I don't think it should be something we
do by default, or something we do right off the bat, before seeing
that there is a concrete need.

I'm working from the following theories about our desires for Browser
Components:

a) Our explicit goals could be described as follows: Rationalizing and
simplifying the dependency graph, both in terms of header dependencies
and in terms of link-time dependencies, with the goal of making it
possible to turn off certain components, and to allow injection (or
replacement by other means) of their currently-provided-by-Chrome
dependencies so that components can be reused in new contexts and more
easily unit tested separately.

b) It is not a goal in and of itself to make all API surfaces into
abstract base classes.  (If it is, then I need to understand the rationale
for that; the use of abstract base classes seems like something that
should be a means to some end rather than a goal in itself).

c) We want to minimize the time it takes to reach the entirety of our
goals, and at the same time we want to maximize the percentage of our
goals that we can achieve in a reasonably short timeframe (say one or
two quarters), i.e. an asymptotic curve towards our goal is more desirable
than a linear or an exponential one.

d) We want to help make the upstreaming of projects such as Chrome for
Android and the Android WebView successful, and such upstreaming is
happening in the very near term or has already started.

Corollaries of the above seem to be that we should defer work that we
are unsure will be necessary, and we should take any iterative quick wins
that we can across the breadth of code, rather than polishing things
fully on deep but narrow segments of code.  Finally, if we are
deferring work that we don't know whether or not will be necessary, we
should have ways to discover as early as possible that it is going to
be necessary.

Based on my work with John on the content/ refactoring, I am also
working from the theory that extracting an abstract base class is much
costlier in development time than not doing so.  This is especially
true when you get into class hierarchies.

Finally, I believe that adding the level of indirection that an
abstract base brings can make the code slightly harder for humans to
grok, and that even though each virtual method call is insignificant
in cost, we should not be adding unnecessary virtual method calls
(i.e. where no runtime polymorphism is necessary).  So I think there
is a maintenance and performance burden in addition to the cost of
added up-front development time, small or even tiny per instance but
significant when added up.

Based on all of those theories, this is roughly the plan I propose
that we follow:

1) Start by allowing headers containing concrete class definitions in
the api directory, as long as they don't bring in non-API headers.
This lets us get more quick wins earlier, since we get a more rational
header dependency structure sooner, and fewer temporarily-allowed
rules in DEPS files.

2) Defer the work of extracting an abstract base until practical
reasons show that it is actually necessary.  Oftentimes, we may end up
not actually needing runtime polymorphism, we may just need
compile-time specialization in which case e.g. a concrete API class
definition, possibly with a pImpl pattern, is fine.

3) Introduce a link-time or target-level dependency analysis tool
similar in spirit to the checkdeps tool we have for headers.  This
tool would let us write what are essentially aspirational link-time
dependency rules (with strict allow/disallow rules, and
temporarily-allowed exceptions).  Such a tool would help us find cases
where our current structure will be insufficient where e.g. abstract bases
plus factories exposed across module boundaries may actually needed
to break link-time dependencies.  (Credit to Erik for the idea of such a
tool, which I think is very good).

4) Do iterative work as Browser Components actually start being used
as components, as we start introducing stand-alone test harnesses for
components, etc., and as part of this iterative work, introduce abstract
bases when they show themselves to be necessary in practice, and do
compile-time specialization when appropriate, until we have achieved
our goals fully.

One last note on the suitability of concrete class definitions as
APIs: A header file containing a class with a publicf API and a
private section containing implementation details does not actually
leak implementation details to code, only to humans.  A private section
is a means of information hiding in the OO programming sense; it is
just an unfortunate coincidence of C++'s design that those hidden
implementation details show up as human-readable in the header file.
For code consuming the header file, those details are still invisible.

Thanks for reading this far.  I look forward to hearing what you think.

Cheers,
Jói


On Wed, Aug 8, 2012 at 6:37 PM, Jói Sigurðsson <joi@google.com> wrote:
> Ben and John, thanks for the feedback. I have some thoughts on this subject
> but am about to meet some friends for dinner. Will try to respond later
> tonight.
>
> Cheers,
> Jói
>
> On Aug 8, 2012 5:58 PM, "Ben Goodger (Google)" <ben@chromium.org> wrote:
>>
>> +1. The api dir should contain an abstract interface that the components
>> use to talk to the browser.
>>
>>
>> On Wed, Aug 8, 2012 at 9:55 AM, John Abd-El-Malek <jam@chromium.org>
>> wrote:
>>>
>>>
>>>
>>> On Wed, Aug 8, 2012 at 9:48 AM, Jói Sigurðsson <joi@chromium.org> wrote:
>>>>
>>>> It's poorly named.  InfoBarTabHelper is actually where you go to add
>>>> an infobar, remove an infobar, and replace an infobar.
>>>
>>>
>>> Right, that's why I suggested maybe adding an interface here that allows
>>> one to do this.
>>>
>>>>
>>>> Note also that in the Browser Components project, we decided we would
>>>> take as many easy victories as we can first, and then refine the API
>>>> as we go along.  For this reason, we are allowing non-abstract classes
>>>> in the api directory as long as they do not introduce header
>>>> dependencies on chrome/browser outside the api directory.  This is a
>>>> for-now kind of thing; later we will introduce tools to let us write
>>>> rules about link-time dependencies and will minimize those just as we
>>>> are doing for header dependencies, and this may force introduction of
>>>> abstract bases in some cases.  For this class in particular, there are
>>>> probably a few methods that could be moved out of the class that lives
>>>> in the api directory and into a subclass, e.g. the count of infobars
>>>> and the way to enumerate delegates.  I should probably add a TODO to
>>>> minimize the surface here.
>>>
>>>
>>> I don't think this header should be in a separate directory from its cc
>>> file. It's not just an api, it's also an implementation detail of
>>> tabcontents. in the api directory, there could be a new interface that has
>>> the add/remove infobar interface. until that interface is made, a temporary
>>> DEPS exceptions for code to include infobar_tab_helper.h from component
>>> directories could be allowed.
>>>
>>>>
>>>> See some more discussion in the chrome/browser/api/DEPS file in this
>>>> change.
>>>>
>>>> Cheers,
>>>> Jói
>>>>
>>>>
>>>>
>>>> On Wed, Aug 8, 2012 at 4:29 PM,  <jam@chromium.org> wrote:
>>>> > drive-by (didn't look in detail, this caught my attention)
>>>> >
>>>> >
>>>> >
>>>> >
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>>>> > File chrome/browser/api/infobars/infobar_tab_helper.h (right):
>>>> >
>>>> >
>>>> >
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>>>> > chrome/browser/api/infobars/infobar_tab_helper.h:16: class
>>>> > InfoBarTabHelper : public content::WebContentsObserver,
>>>> > i'm confused, this doesn't seem like an api of infobars that's
>>>> > provided
>>>> > by the browser. i realize some stuff in here is needed by code that
>>>> > puts
>>>> > up an infobar, so maybe a new interface should be added here. but this
>>>> > header seems like it should stay in chrome with the
>>>> > infobar_tab_helper.cc file, which is an internal implementation detail
>>>> > of TabContents.
>>>> >
>>>> >
>>>> > http://codereview.chromium.org/10843071/
>>>> >
>>>> > --
>>>> > You received this message because you are subscribed to the Google
>>>> > Groups
>>>> > "browser-components-watch" group.
>>>> > To post to this group, send email to
>>>> > browser-components-watch@chromium.org.
>>>> > To unsubscribe from this group, send email to
>>>> > browser-components-watch+unsubscribe@chromium.org.
>>>> > For more options, visit
>>>> > https://groups.google.com/a/chromium.org/groups/opt_out.
>>>> >
>>>> >
>>>
>>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "browser-components-watch" group.
>> To post to this group, send email to
>> browser-components-watch@chromium.org.
>> To unsubscribe from this group, send email to
>> browser-components-watch+unsubscribe@chromium.org.
>> For more options, visit
>> https://groups.google.com/a/chromium.org/groups/opt_out.
>>
>>

[jam, 2012-08-09 00:02:03]

hey Joi, I'll attempt to answer your main points. Probably might be worth
having a vc to chat about this some more?

For preventing of more usage of chrome headers that need to be wrapped with
APIs, this can be done through deps. i.e. a component like autofill might
have a restrictive DEPS that doesn't allow chrome, but only
chrome/browser/api and a temporary allowance for
chrome/browser/infobars/infobar_tab_helper.h. With your improvements to
checkdeps, we can ensure that no more includes are added.

Classes like InfobarTabHelper are not public APIs of chrome. That's an
internal implementation detail. The way I view it, if browser components
are separate from chrome, then they should talk to it through a thin layer.
Aside from this, classes like InfobarTabHelper need to have the header and
implementation file in the same directory; that's just convention from all
the chrome code. Keeping them in different directories is confusing. Also,
having a component like autofill talk to InfobarTabHelper, instead of an
interface, makes testing harder. It seems that an attractive part of
splitting up components is that they can be tested more independently and
with less setup of other unnecessary classes. Having interfaces like this
would make that easier.

While I agree that adding more interfaces is sometimes harder to read, I
think in a codebase the size of chrome, they do make things easier when
used to separate large features. In comparing this to the Content API work,
I agree that adding interfaces around each component (i.e. autofill) will
be a lot of work and may not be necessary. But adding interfaces for how
these components talk out is much less effort, since it usually is a small
delegate-style interface. This is also what provides the most
bang-for-the-buck benefit in terms of encapsulation and testability.

On Wed, Aug 8, 2012 at 4:19 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> [+browser-components-dev]
>
> I'm back, so now let me try to explain my thoughts on this.  I'm not
> going to try to summarize, because I think it's important that the
> stakeholders in this read through my reasoning, but I will try to keep
> this structured.
>
> I'd like to leave the specifics of the current change alone for now,
> and focus on the general principles we're operating on.  We can
> discuss the specifics of the current change once we settle on the
> principles.  I tried to make a similar case to the one I'm making
> below while we were doing the content/ work, but I don't think I
> explained it well enough or fought for it hard enough, plus the
> situation there was somewhat different.
>
> I think the issues below are important to discuss, because the choice
> we make can have a large impact on how quickly we can move, in
> particular how much progress we can make in a reasonably short period
> of time, and quick progress in this area is important since it can
> prevent changes from creeping into the codebase that otherwise will
> take time to fix up later.


> Ben and John, I think both of you are stating a preference that all or
> at least most classes in the api directory should be abstract or very
> close to abstract, and likely extracted from existing classes similar
> to what we did for content/ (e.g. a class previously known as Foo
> might be split into an abstract content::Foo interface and a concrete
> content::FooImpl implementation).  Pardon me if that's not the exact
> position you're representing; hopefully the following is still a worth
> while discussion.
>
> To state my position, I don't think we should extract abstract bases
> for everything we place in the api directory.  I do think that we
> will end up extracting a lot of abstract bases before we are done, but
> I think we should do it only to the extent that it becomes practically
> necessary to reach our goals.  I don't think it should be something we
> do by default, or something we do right off the bat, before seeing
> that there is a concrete need.
>
> I'm working from the following theories about our desires for Browser
> Components:
>
> a) Our explicit goals could be described as follows: Rationalizing and
> simplifying the dependency graph, both in terms of header dependencies
> and in terms of link-time dependencies, with the goal of making it
> possible to turn off certain components, and to allow injection (or
> replacement by other means) of their currently-provided-by-Chrome
> dependencies so that components can be reused in new contexts and more
> easily unit tested separately.
>
> b) It is not a goal in and of itself to make all API surfaces into
> abstract base classes.  (If it is, then I need to understand the rationale
> for that; the use of abstract base classes seems like something that
> should be a means to some end rather than a goal in itself).
>
> c) We want to minimize the time it takes to reach the entirety of our
> goals, and at the same time we want to maximize the percentage of our
> goals that we can achieve in a reasonably short timeframe (say one or
> two quarters), i.e. an asymptotic curve towards our goal is more desirable
> than a linear or an exponential one.
>
> d) We want to help make the upstreaming of projects such as Chrome for
> Android and the Android WebView successful, and such upstreaming is
> happening in the very near term or has already started.
>
> Corollaries of the above seem to be that we should defer work that we
> are unsure will be necessary, and we should take any iterative quick wins
> that we can across the breadth of code, rather than polishing things
> fully on deep but narrow segments of code.  Finally, if we are
> deferring work that we don't know whether or not will be necessary, we
> should have ways to discover as early as possible that it is going to
> be necessary.
>
> Based on my work with John on the content/ refactoring, I am also
> working from the theory that extracting an abstract base class is much
> costlier in development time than not doing so.  This is especially
> true when you get into class hierarchies.
>
> Finally, I believe that adding the level of indirection that an
> abstract base brings can make the code slightly harder for humans to
> grok, and that even though each virtual method call is insignificant
> in cost, we should not be adding unnecessary virtual method calls
> (i.e. where no runtime polymorphism is necessary).  So I think there
> is a maintenance and performance burden in addition to the cost of
> added up-front development time, small or even tiny per instance but
> significant when added up.
>
> Based on all of those theories, this is roughly the plan I propose
> that we follow:
>
> 1) Start by allowing headers containing concrete class definitions in
> the api directory, as long as they don't bring in non-API headers.
> This lets us get more quick wins earlier, since we get a more rational
> header dependency structure sooner, and fewer temporarily-allowed
> rules in DEPS files.
>
> 2) Defer the work of extracting an abstract base until practical
> reasons show that it is actually necessary.  Oftentimes, we may end up
> not actually needing runtime polymorphism, we may just need
> compile-time specialization in which case e.g. a concrete API class
> definition, possibly with a pImpl pattern, is fine.
>
> 3) Introduce a link-time or target-level dependency analysis tool
> similar in spirit to the checkdeps tool we have for headers.  This
> tool would let us write what are essentially aspirational link-time
> dependency rules (with strict allow/disallow rules, and
> temporarily-allowed exceptions).  Such a tool would help us find cases
> where our current structure will be insufficient where e.g. abstract bases
> plus factories exposed across module boundaries may actually needed
> to break link-time dependencies.  (Credit to Erik for the idea of such a
> tool, which I think is very good).
>
> 4) Do iterative work as Browser Components actually start being used
> as components, as we start introducing stand-alone test harnesses for
> components, etc., and as part of this iterative work, introduce abstract
> bases when they show themselves to be necessary in practice, and do
> compile-time specialization when appropriate, until we have achieved
> our goals fully.
>
> One last note on the suitability of concrete class definitions as
> APIs: A header file containing a class with a publicf API and a
> private section containing implementation details does not actually
> leak implementation details to code, only to humans.  A private section
> is a means of information hiding in the OO programming sense; it is
> just an unfortunate coincidence of C++'s design that those hidden
> implementation details show up as human-readable in the header file.
> For code consuming the header file, those details are still invisible.
>
> Thanks for reading this far.  I look forward to hearing what you think.
>
> Cheers,
> Jói
>
>
> On Wed, Aug 8, 2012 at 6:37 PM, Jói Sigurðsson <joi@google.com> wrote:
> > Ben and John, thanks for the feedback. I have some thoughts on this
> subject
> > but am about to meet some friends for dinner. Will try to respond later
> > tonight.
> >
> > Cheers,
> > Jói
> >
> > On Aug 8, 2012 5:58 PM, "Ben Goodger (Google)" <ben@chromium.org> wrote:
> >>
> >> +1. The api dir should contain an abstract interface that the components
> >> use to talk to the browser.
> >>
> >>
> >> On Wed, Aug 8, 2012 at 9:55 AM, John Abd-El-Malek <jam@chromium.org>
> >> wrote:
> >>>
> >>>
> >>>
> >>> On Wed, Aug 8, 2012 at 9:48 AM, Jói Sigurðsson <joi@chromium.org>
> wrote:
> >>>>
> >>>> It's poorly named.  InfoBarTabHelper is actually where you go to add
> >>>> an infobar, remove an infobar, and replace an infobar.
> >>>
> >>>
> >>> Right, that's why I suggested maybe adding an interface here that
> allows
> >>> one to do this.
> >>>
> >>>>
> >>>> Note also that in the Browser Components project, we decided we would
> >>>> take as many easy victories as we can first, and then refine the API
> >>>> as we go along.  For this reason, we are allowing non-abstract classes
> >>>> in the api directory as long as they do not introduce header
> >>>> dependencies on chrome/browser outside the api directory.  This is a
> >>>> for-now kind of thing; later we will introduce tools to let us write
> >>>> rules about link-time dependencies and will minimize those just as we
> >>>> are doing for header dependencies, and this may force introduction of
> >>>> abstract bases in some cases.  For this class in particular, there are
> >>>> probably a few methods that could be moved out of the class that lives
> >>>> in the api directory and into a subclass, e.g. the count of infobars
> >>>> and the way to enumerate delegates.  I should probably add a TODO to
> >>>> minimize the surface here.
> >>>
> >>>
> >>> I don't think this header should be in a separate directory from its cc
> >>> file. It's not just an api, it's also an implementation detail of
> >>> tabcontents. in the api directory, there could be a new interface that
> has
> >>> the add/remove infobar interface. until that interface is made, a
> temporary
> >>> DEPS exceptions for code to include infobar_tab_helper.h from component
> >>> directories could be allowed.
> >>>
> >>>>
> >>>> See some more discussion in the chrome/browser/api/DEPS file in this
> >>>> change.
> >>>>
> >>>> Cheers,
> >>>> Jói
> >>>>
> >>>>
> >>>>
> >>>> On Wed, Aug 8, 2012 at 4:29 PM,  <jam@chromium.org> wrote:
> >>>> > drive-by (didn't look in detail, this caught my attention)
> >>>> >
> >>>> >
> >>>> >
> >>>> >
>
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
> >>>> > File chrome/browser/api/infobars/infobar_tab_helper.h (right):
> >>>> >
> >>>> >
> >>>> >
>
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
> >>>> > chrome/browser/api/infobars/infobar_tab_helper.h:16: class
> >>>> > InfoBarTabHelper : public content::WebContentsObserver,
> >>>> > i'm confused, this doesn't seem like an api of infobars that's
> >>>> > provided
> >>>> > by the browser. i realize some stuff in here is needed by code that
> >>>> > puts
> >>>> > up an infobar, so maybe a new interface should be added here. but
> this
> >>>> > header seems like it should stay in chrome with the
> >>>> > infobar_tab_helper.cc file, which is an internal implementation
> detail
> >>>> > of TabContents.
> >>>> >
> >>>> >
> >>>> > http://codereview.chromium.org/10843071/
> >>>> >
> >>>> > --
> >>>> > You received this message because you are subscribed to the Google
> >>>> > Groups
> >>>> > "browser-components-watch" group.
> >>>> > To post to this group, send email to
> >>>> > browser-components-watch@chromium.org.
> >>>> > To unsubscribe from this group, send email to
> >>>> > browser-components-watch+unsubscribe@chromium.org.
> >>>> > For more options, visit
> >>>> > https://groups.google.com/a/chromium.org/groups/opt_out.
> >>>> >
> >>>> >
> >>>
> >>>
> >>
> >> --
> >> You received this message because you are subscribed to the Google
> Groups
> >> "browser-components-watch" group.
> >> To post to this group, send email to
> >> browser-components-watch@chromium.org.
> >> To unsubscribe from this group, send email to
> >> browser-components-watch+unsubscribe@chromium.org.
> >> For more options, visit
> >> https://groups.google.com/a/chromium.org/groups/opt_out.
> >>
> >>
>

[jam, 2012-08-09 01:54:31]

On Wed, Aug 8, 2012 at 5:02 PM, John Abd-El-Malek <jam@chromium.org> wrote:

> hey Joi, I'll attempt to answer your main points. Probably might be worth
> having a vc to chat about this some more?
>
> For preventing of more usage of chrome headers that need to be wrapped
> with APIs, this can be done through deps. i.e. a component like autofill
> might have a restrictive DEPS that doesn't allow chrome, but only
> chrome/browser/api and a temporary allowance for
> chrome/browser/infobars/infobar_tab_helper.h. With your improvements to
> checkdeps, we can ensure that no more includes are added.
>
> Classes like InfobarTabHelper are not public APIs of chrome. That's an
> internal implementation detail. The way I view it, if browser components
> are separate from chrome, then they should talk to it through a thin layer.
> Aside from this, classes like InfobarTabHelper need to have the header and
> implementation file in the same directory; that's just convention from all
> the chrome code. Keeping them in different directories is confusing.
>

I just saw that the .h and .cc files for the infobar delegates have also
been split up. We need to keep these together as well. It seems that except
for infobar_tab_helper.h, they only include each other and
content/public/browser headers, so it's fine to move those cc files to be
beside their headers.


> Also, having a component like autofill talk to InfobarTabHelper, instead
> of an interface, makes testing harder. It seems that an attractive part of
> splitting up components is that they can be tested more independently and
> with less setup of other unnecessary classes. Having interfaces like this
> would make that easier.
>
> While I agree that adding more interfaces is sometimes harder to read, I
> think in a codebase the size of chrome, they do make things easier when
> used to separate large features. In comparing this to the Content API work,
> I agree that adding interfaces around each component (i.e. autofill) will
> be a lot of work and may not be necessary. But adding interfaces for how
> these components talk out is much less effort, since it usually is a small
> delegate-style interface. This is also what provides the most
> bang-for-the-buck benefit in terms of encapsulation and testability.
>
>
> On Wed, Aug 8, 2012 at 4:19 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>
>> [+browser-components-dev]
>>
>> I'm back, so now let me try to explain my thoughts on this.  I'm not
>> going to try to summarize, because I think it's important that the
>> stakeholders in this read through my reasoning, but I will try to keep
>> this structured.
>>
>> I'd like to leave the specifics of the current change alone for now,
>> and focus on the general principles we're operating on.  We can
>> discuss the specifics of the current change once we settle on the
>> principles.  I tried to make a similar case to the one I'm making
>> below while we were doing the content/ work, but I don't think I
>> explained it well enough or fought for it hard enough, plus the
>> situation there was somewhat different.
>>
>> I think the issues below are important to discuss, because the choice
>> we make can have a large impact on how quickly we can move, in
>> particular how much progress we can make in a reasonably short period
>> of time, and quick progress in this area is important since it can
>> prevent changes from creeping into the codebase that otherwise will
>> take time to fix up later.
>
>
>> Ben and John, I think both of you are stating a preference that all or
>> at least most classes in the api directory should be abstract or very
>> close to abstract, and likely extracted from existing classes similar
>> to what we did for content/ (e.g. a class previously known as Foo
>> might be split into an abstract content::Foo interface and a concrete
>> content::FooImpl implementation).  Pardon me if that's not the exact
>> position you're representing; hopefully the following is still a worth
>> while discussion.
>>
>> To state my position, I don't think we should extract abstract bases
>> for everything we place in the api directory.  I do think that we
>> will end up extracting a lot of abstract bases before we are done, but
>> I think we should do it only to the extent that it becomes practically
>> necessary to reach our goals.  I don't think it should be something we
>> do by default, or something we do right off the bat, before seeing
>> that there is a concrete need.
>>
>> I'm working from the following theories about our desires for Browser
>> Components:
>>
>> a) Our explicit goals could be described as follows: Rationalizing and
>> simplifying the dependency graph, both in terms of header dependencies
>> and in terms of link-time dependencies, with the goal of making it
>> possible to turn off certain components, and to allow injection (or
>> replacement by other means) of their currently-provided-by-Chrome
>> dependencies so that components can be reused in new contexts and more
>> easily unit tested separately.
>>
>> b) It is not a goal in and of itself to make all API surfaces into
>> abstract base classes.  (If it is, then I need to understand the rationale
>> for that; the use of abstract base classes seems like something that
>> should be a means to some end rather than a goal in itself).
>>
>> c) We want to minimize the time it takes to reach the entirety of our
>> goals, and at the same time we want to maximize the percentage of our
>> goals that we can achieve in a reasonably short timeframe (say one or
>> two quarters), i.e. an asymptotic curve towards our goal is more desirable
>> than a linear or an exponential one.
>>
>> d) We want to help make the upstreaming of projects such as Chrome for
>> Android and the Android WebView successful, and such upstreaming is
>> happening in the very near term or has already started.
>>
>> Corollaries of the above seem to be that we should defer work that we
>> are unsure will be necessary, and we should take any iterative quick wins
>> that we can across the breadth of code, rather than polishing things
>> fully on deep but narrow segments of code.  Finally, if we are
>> deferring work that we don't know whether or not will be necessary, we
>> should have ways to discover as early as possible that it is going to
>> be necessary.
>>
>> Based on my work with John on the content/ refactoring, I am also
>> working from the theory that extracting an abstract base class is much
>> costlier in development time than not doing so.  This is especially
>> true when you get into class hierarchies.
>>
>> Finally, I believe that adding the level of indirection that an
>> abstract base brings can make the code slightly harder for humans to
>> grok, and that even though each virtual method call is insignificant
>> in cost, we should not be adding unnecessary virtual method calls
>> (i.e. where no runtime polymorphism is necessary).  So I think there
>> is a maintenance and performance burden in addition to the cost of
>> added up-front development time, small or even tiny per instance but
>> significant when added up.
>>
>> Based on all of those theories, this is roughly the plan I propose
>> that we follow:
>>
>> 1) Start by allowing headers containing concrete class definitions in
>> the api directory, as long as they don't bring in non-API headers.
>> This lets us get more quick wins earlier, since we get a more rational
>> header dependency structure sooner, and fewer temporarily-allowed
>> rules in DEPS files.
>>
>> 2) Defer the work of extracting an abstract base until practical
>> reasons show that it is actually necessary.  Oftentimes, we may end up
>> not actually needing runtime polymorphism, we may just need
>> compile-time specialization in which case e.g. a concrete API class
>> definition, possibly with a pImpl pattern, is fine.
>>
>> 3) Introduce a link-time or target-level dependency analysis tool
>> similar in spirit to the checkdeps tool we have for headers.  This
>> tool would let us write what are essentially aspirational link-time
>> dependency rules (with strict allow/disallow rules, and
>> temporarily-allowed exceptions).  Such a tool would help us find cases
>> where our current structure will be insufficient where e.g. abstract bases
>> plus factories exposed across module boundaries may actually needed
>> to break link-time dependencies.  (Credit to Erik for the idea of such a
>> tool, which I think is very good).
>>
>> 4) Do iterative work as Browser Components actually start being used
>> as components, as we start introducing stand-alone test harnesses for
>> components, etc., and as part of this iterative work, introduce abstract
>> bases when they show themselves to be necessary in practice, and do
>> compile-time specialization when appropriate, until we have achieved
>> our goals fully.
>>
>> One last note on the suitability of concrete class definitions as
>> APIs: A header file containing a class with a publicf API and a
>> private section containing implementation details does not actually
>> leak implementation details to code, only to humans.  A private section
>> is a means of information hiding in the OO programming sense; it is
>> just an unfortunate coincidence of C++'s design that those hidden
>> implementation details show up as human-readable in the header file.
>> For code consuming the header file, those details are still invisible.
>>
>> Thanks for reading this far.  I look forward to hearing what you think.
>>
>> Cheers,
>> Jói
>>
>>
>> On Wed, Aug 8, 2012 at 6:37 PM, Jói Sigurðsson <joi@google.com> wrote:
>> > Ben and John, thanks for the feedback. I have some thoughts on this
>> subject
>> > but am about to meet some friends for dinner. Will try to respond later
>> > tonight.
>> >
>> > Cheers,
>> > Jói
>> >
>> > On Aug 8, 2012 5:58 PM, "Ben Goodger (Google)" <ben@chromium.org>
>> wrote:
>> >>
>> >> +1. The api dir should contain an abstract interface that the
>> components
>> >> use to talk to the browser.
>> >>
>> >>
>> >> On Wed, Aug 8, 2012 at 9:55 AM, John Abd-El-Malek <jam@chromium.org>
>> >> wrote:
>> >>>
>> >>>
>> >>>
>> >>> On Wed, Aug 8, 2012 at 9:48 AM, Jói Sigurðsson <joi@chromium.org>
>> wrote:
>> >>>>
>> >>>> It's poorly named.  InfoBarTabHelper is actually where you go to add
>> >>>> an infobar, remove an infobar, and replace an infobar.
>> >>>
>> >>>
>> >>> Right, that's why I suggested maybe adding an interface here that
>> allows
>> >>> one to do this.
>> >>>
>> >>>>
>> >>>> Note also that in the Browser Components project, we decided we would
>> >>>> take as many easy victories as we can first, and then refine the API
>> >>>> as we go along.  For this reason, we are allowing non-abstract
>> classes
>> >>>> in the api directory as long as they do not introduce header
>> >>>> dependencies on chrome/browser outside the api directory.  This is a
>> >>>> for-now kind of thing; later we will introduce tools to let us write
>> >>>> rules about link-time dependencies and will minimize those just as we
>> >>>> are doing for header dependencies, and this may force introduction of
>> >>>> abstract bases in some cases.  For this class in particular, there
>> are
>> >>>> probably a few methods that could be moved out of the class that
>> lives
>> >>>> in the api directory and into a subclass, e.g. the count of infobars
>> >>>> and the way to enumerate delegates.  I should probably add a TODO to
>> >>>> minimize the surface here.
>> >>>
>> >>>
>> >>> I don't think this header should be in a separate directory from its
>> cc
>> >>> file. It's not just an api, it's also an implementation detail of
>> >>> tabcontents. in the api directory, there could be a new interface
>> that has
>> >>> the add/remove infobar interface. until that interface is made, a
>> temporary
>> >>> DEPS exceptions for code to include infobar_tab_helper.h from
>> component
>> >>> directories could be allowed.
>> >>>
>> >>>>
>> >>>> See some more discussion in the chrome/browser/api/DEPS file in this
>> >>>> change.
>> >>>>
>> >>>> Cheers,
>> >>>> Jói
>> >>>>
>> >>>>
>> >>>>
>> >>>> On Wed, Aug 8, 2012 at 4:29 PM,  <jam@chromium.org> wrote:
>> >>>> > drive-by (didn't look in detail, this caught my attention)
>> >>>> >
>> >>>> >
>> >>>> >
>> >>>> >
>>
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>> >>>> > File chrome/browser/api/infobars/infobar_tab_helper.h (right):
>> >>>> >
>> >>>> >
>> >>>> >
>>
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>> >>>> > chrome/browser/api/infobars/infobar_tab_helper.h:16: class
>> >>>> > InfoBarTabHelper : public content::WebContentsObserver,
>> >>>> > i'm confused, this doesn't seem like an api of infobars that's
>> >>>> > provided
>> >>>> > by the browser. i realize some stuff in here is needed by code that
>> >>>> > puts
>> >>>> > up an infobar, so maybe a new interface should be added here. but
>> this
>> >>>> > header seems like it should stay in chrome with the
>> >>>> > infobar_tab_helper.cc file, which is an internal implementation
>> detail
>> >>>> > of TabContents.
>> >>>> >
>> >>>> >
>> >>>> > http://codereview.chromium.org/10843071/
>> >>>> >
>> >>>> > --
>> >>>> > You received this message because you are subscribed to the Google
>> >>>> > Groups
>> >>>> > "browser-components-watch" group.
>> >>>> > To post to this group, send email to
>> >>>> > browser-components-watch@chromium.org.
>> >>>> > To unsubscribe from this group, send email to
>> >>>> > browser-components-watch+unsubscribe@chromium.org.
>> >>>> > For more options, visit
>> >>>> > https://groups.google.com/a/chromium.org/groups/opt_out.
>> >>>> >
>> >>>> >
>> >>>
>> >>>
>> >>
>> >> --
>> >> You received this message because you are subscribed to the Google
>> Groups
>> >> "browser-components-watch" group.
>> >> To post to this group, send email to
>> >> browser-components-watch@chromium.org.
>> >> To unsubscribe from this group, send email to
>> >> browser-components-watch+unsubscribe@chromium.org.
>> >> For more options, visit
>> >> https://groups.google.com/a/chromium.org/groups/opt_out.
>> >>
>> >>
>>
>
>

[tfarina, 2012-08-09 01:58:15]

Peter (pkasting) should be probably added to chrome/browser/api/infobars/OWNERS
as he rewrote it ownership model and made it cross-platform.

Joi, so we will then have:
chrome/browser/api/bookmarks/* # interfaces go here
chrome/browser/bookmarks/ # internal implementation goes here.

[joi, 2012-08-09 13:16:31]

Hi Thiago,

> Peter (pkasting) should be probably added to
> chrome/browser/api/infobars/OWNERS
> as he rewrote it ownership model and made it cross-platform.

If he wants to add himself that makes perfect sense to me.

> Joi, so we will then have:
> chrome/browser/api/bookmarks/* # interfaces go here
> chrome/browser/bookmarks/ # internal implementation goes here.

Yes, for those components that have an API.  The plan is that only
components that provide services to other components would have an
API, at least initially; we don't necessarily intend to go all the way
to encapsulating all components in an API, we are focusing on breaking
dependencies in the other direction, i.e. from components to the rest
of Chrome.

Cheers,
Jói



On Thu, Aug 9, 2012 at 1:58 AM,  <tfarina@chromium.org> wrote:
> Peter (pkasting) should be probably added to
> chrome/browser/api/infobars/OWNERS
> as he rewrote it ownership model and made it cross-platform.
>
> Joi, so we will then have:
> chrome/browser/api/bookmarks/* # interfaces go here
> chrome/browser/bookmarks/ # internal implementation goes here.
>
> http://codereview.chromium.org/10843071/

[Jói, 2012-08-09 13:33:21]

Hi John,

Yep, a VC chat sounds like a good idea.

Let me try to answer some of your points quickly so that we know each
others' position a bit better before we talk live.

> For preventing of more usage of chrome headers that need to be wrapped with
> APIs, this can be done through deps [...]

Yes, it can be done via a temporarily-allowed include rule in DEPS,
but this is less valuable than actually moving the header to the api
directory.  For one thing, a temporarily-allowed rule in one component
does not document that it is actually OK for other components to
#include the header, because it is in the process of being made into
an API.

> Classes like InfobarTabHelper are not public APIs of chrome. That's an
> internal implementation detail.

I maintain that we can move faster and achieve a large percentage of
our goals in a short period of time if we allow concrete class
definitions as API.  Over time, I think we will reach a very similar
end state to what you are proposing; almost everything in API will
either be an abstract base, except for a few simple utility or POD
classes.  My point is that the faster we can rationalize the header
dependencies, even though we know we will later have to introduce some
abstractness etc. to do the same for link-time dependencies, the less
painful the upstreaming process will be for the projects that are
upstreaming, and the less chance new changes get introduced in the
code base that do things that will later need to be unwound.

So my point of view is, in the interest of making the biggest quick
wins we can, we could allow a class like InfoBarTabHelper, that does
not #include anything not suitable for the api directory, as an API
for now, although possibly we should reduce its public interface to
just the Add/Remove/ReplaceInfobar methods.  I don't think the stuff
in the private section of the class leaks any implementation details
(it does for human readers, but not for code using the class, which is
the important thing).  We would then make it abstract as the concrete
need to do so surfaces.

> The way I view it, if browser components are
> separate from chrome, then they should talk to it through a thin layer.

Agreed.  Such a thin layer does not need to be completely pure virtual
though, at least not to achieve many of our initial goals quickly.
The public section of a class is its interface, and in some cases we
can simply move the class definitions to the api directory as they
have a relatively clean public interface surface.  Later, we are
fairly likely to make the class abstract e.g. to allow easier testing
or to break link-time dependencies, but we can take the quick wins
across the breadth of the code base sooner if we allow ourselves to
defer this work.

> Aside from this, classes like InfobarTabHelper need to have the header and
> implementation file in the same directory; that's just convention from all
> the chrome code. Keeping them in different directories is confusing.

This is a convention I think we should allow breaking in the interest
of expediency.  I don't think it's very confusing; in particular I
don't think it adds any confusion above the confusion you would have
if you have an API header in the api directory, and then a concrete
header in the "regular" directory (alongside the .cc file) that does
more or less nothing except define a class FooImpl : public Foo {}
with an empty class body.  In the end you have to find the real
definition of the class's interface anyway.

> Also, having a component like autofill talk to InfobarTabHelper, instead of
an
> interface, makes testing harder. It seems that an attractive part of
> splitting up components is that they can be tested more independently and
> with less setup of other unnecessary classes. Having interfaces like this
> would make that easier.

I agree in principle.  However:
a) Improving the tests for components is part of our plan, but it is
not one of the first things we should focus on; there are bigger wins
to achieve earlier than that; and
b) We don't know for sure at this moment that every class in the API
will need to be pure virtual to enable testing; if we defer the
decision we may save some work.

Therefore, I think it makes sense to defer making classes abstract in
some cases.

Note that I'm not suggesting that every class being used in a
sort-of-API-like way today should just be moved wholesale to the api
directory.  In many cases we definitely do need to extract the subset
of the class's public interface that truly is an API rather than an
implementation detail into a base interface.  I just want us to have
the freedom to defer that work when the class's public interface is
close enough to being suitable as an API.

I'll try to find a time today for us to talk.

Cheers,
Jói



On Thu, Aug 9, 2012 at 12:02 AM, John Abd-El-Malek <jam@chromium.org> wrote:
> hey Joi, I'll attempt to answer your main points. Probably might be worth
> having a vc to chat about this some more?
>
> For preventing of more usage of chrome headers that need to be wrapped with
> APIs, this can be done through deps. i.e. a component like autofill might
> have a restrictive DEPS that doesn't allow chrome, but only
> chrome/browser/api and a temporary allowance for
> chrome/browser/infobars/infobar_tab_helper.h. With your improvements to
> checkdeps, we can ensure that no more includes are added.
>
> Classes like InfobarTabHelper are not public APIs of chrome. That's an
> internal implementation detail. The way I view it, if browser components are
> separate from chrome, then they should talk to it through a thin layer.
> Aside from this, classes like InfobarTabHelper need to have the header and
> implementation file in the same directory; that's just convention from all
> the chrome code. Keeping them in different directories is confusing. Also,
> having a component like autofill talk to InfobarTabHelper, instead of an
> interface, makes testing harder. It seems that an attractive part of
> splitting up components is that they can be tested more independently and
> with less setup of other unnecessary classes. Having interfaces like this
> would make that easier.
>
> While I agree that adding more interfaces is sometimes harder to read, I
> think in a codebase the size of chrome, they do make things easier when used
> to separate large features. In comparing this to the Content API work, I
> agree that adding interfaces around each component (i.e. autofill) will be a
> lot of work and may not be necessary. But adding interfaces for how these
> components talk out is much less effort, since it usually is a small
> delegate-style interface. This is also what provides the most
> bang-for-the-buck benefit in terms of encapsulation and testability.
>
>
> On Wed, Aug 8, 2012 at 4:19 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>
>> [+browser-components-dev]
>>
>> I'm back, so now let me try to explain my thoughts on this.  I'm not
>> going to try to summarize, because I think it's important that the
>> stakeholders in this read through my reasoning, but I will try to keep
>> this structured.
>>
>> I'd like to leave the specifics of the current change alone for now,
>> and focus on the general principles we're operating on.  We can
>> discuss the specifics of the current change once we settle on the
>> principles.  I tried to make a similar case to the one I'm making
>> below while we were doing the content/ work, but I don't think I
>> explained it well enough or fought for it hard enough, plus the
>> situation there was somewhat different.
>>
>> I think the issues below are important to discuss, because the choice
>> we make can have a large impact on how quickly we can move, in
>> particular how much progress we can make in a reasonably short period
>> of time, and quick progress in this area is important since it can
>> prevent changes from creeping into the codebase that otherwise will
>> take time to fix up later.
>>
>>
>> Ben and John, I think both of you are stating a preference that all or
>> at least most classes in the api directory should be abstract or very
>> close to abstract, and likely extracted from existing classes similar
>> to what we did for content/ (e.g. a class previously known as Foo
>> might be split into an abstract content::Foo interface and a concrete
>> content::FooImpl implementation).  Pardon me if that's not the exact
>> position you're representing; hopefully the following is still a worth
>> while discussion.
>>
>> To state my position, I don't think we should extract abstract bases
>> for everything we place in the api directory.  I do think that we
>> will end up extracting a lot of abstract bases before we are done, but
>> I think we should do it only to the extent that it becomes practically
>> necessary to reach our goals.  I don't think it should be something we
>> do by default, or something we do right off the bat, before seeing
>> that there is a concrete need.
>>
>> I'm working from the following theories about our desires for Browser
>> Components:
>>
>> a) Our explicit goals could be described as follows: Rationalizing and
>> simplifying the dependency graph, both in terms of header dependencies
>> and in terms of link-time dependencies, with the goal of making it
>> possible to turn off certain components, and to allow injection (or
>> replacement by other means) of their currently-provided-by-Chrome
>> dependencies so that components can be reused in new contexts and more
>> easily unit tested separately.
>>
>> b) It is not a goal in and of itself to make all API surfaces into
>> abstract base classes.  (If it is, then I need to understand the rationale
>> for that; the use of abstract base classes seems like something that
>> should be a means to some end rather than a goal in itself).
>>
>> c) We want to minimize the time it takes to reach the entirety of our
>> goals, and at the same time we want to maximize the percentage of our
>> goals that we can achieve in a reasonably short timeframe (say one or
>> two quarters), i.e. an asymptotic curve towards our goal is more desirable
>> than a linear or an exponential one.
>>
>> d) We want to help make the upstreaming of projects such as Chrome for
>> Android and the Android WebView successful, and such upstreaming is
>> happening in the very near term or has already started.
>>
>> Corollaries of the above seem to be that we should defer work that we
>> are unsure will be necessary, and we should take any iterative quick wins
>> that we can across the breadth of code, rather than polishing things
>> fully on deep but narrow segments of code.  Finally, if we are
>> deferring work that we don't know whether or not will be necessary, we
>> should have ways to discover as early as possible that it is going to
>> be necessary.
>>
>> Based on my work with John on the content/ refactoring, I am also
>> working from the theory that extracting an abstract base class is much
>> costlier in development time than not doing so.  This is especially
>> true when you get into class hierarchies.
>>
>> Finally, I believe that adding the level of indirection that an
>> abstract base brings can make the code slightly harder for humans to
>> grok, and that even though each virtual method call is insignificant
>> in cost, we should not be adding unnecessary virtual method calls
>> (i.e. where no runtime polymorphism is necessary).  So I think there
>> is a maintenance and performance burden in addition to the cost of
>> added up-front development time, small or even tiny per instance but
>> significant when added up.
>>
>> Based on all of those theories, this is roughly the plan I propose
>> that we follow:
>>
>> 1) Start by allowing headers containing concrete class definitions in
>> the api directory, as long as they don't bring in non-API headers.
>> This lets us get more quick wins earlier, since we get a more rational
>> header dependency structure sooner, and fewer temporarily-allowed
>> rules in DEPS files.
>>
>> 2) Defer the work of extracting an abstract base until practical
>> reasons show that it is actually necessary.  Oftentimes, we may end up
>> not actually needing runtime polymorphism, we may just need
>> compile-time specialization in which case e.g. a concrete API class
>> definition, possibly with a pImpl pattern, is fine.
>>
>> 3) Introduce a link-time or target-level dependency analysis tool
>> similar in spirit to the checkdeps tool we have for headers.  This
>> tool would let us write what are essentially aspirational link-time
>> dependency rules (with strict allow/disallow rules, and
>> temporarily-allowed exceptions).  Such a tool would help us find cases
>> where our current structure will be insufficient where e.g. abstract bases
>> plus factories exposed across module boundaries may actually needed
>> to break link-time dependencies.  (Credit to Erik for the idea of such a
>> tool, which I think is very good).
>>
>> 4) Do iterative work as Browser Components actually start being used
>> as components, as we start introducing stand-alone test harnesses for
>> components, etc., and as part of this iterative work, introduce abstract
>> bases when they show themselves to be necessary in practice, and do
>> compile-time specialization when appropriate, until we have achieved
>> our goals fully.
>>
>> One last note on the suitability of concrete class definitions as
>> APIs: A header file containing a class with a publicf API and a
>> private section containing implementation details does not actually
>> leak implementation details to code, only to humans.  A private section
>> is a means of information hiding in the OO programming sense; it is
>> just an unfortunate coincidence of C++'s design that those hidden
>> implementation details show up as human-readable in the header file.
>> For code consuming the header file, those details are still invisible.
>>
>> Thanks for reading this far.  I look forward to hearing what you think.
>>
>> Cheers,
>> Jói
>>
>>
>> On Wed, Aug 8, 2012 at 6:37 PM, Jói Sigurðsson <joi@google.com> wrote:
>> > Ben and John, thanks for the feedback. I have some thoughts on this
>> > subject
>> > but am about to meet some friends for dinner. Will try to respond later
>> > tonight.
>> >
>> > Cheers,
>> > Jói
>> >
>> > On Aug 8, 2012 5:58 PM, "Ben Goodger (Google)" <ben@chromium.org> wrote:
>> >>
>> >> +1. The api dir should contain an abstract interface that the
>> >> components
>> >> use to talk to the browser.
>> >>
>> >>
>> >> On Wed, Aug 8, 2012 at 9:55 AM, John Abd-El-Malek <jam@chromium.org>
>> >> wrote:
>> >>>
>> >>>
>> >>>
>> >>> On Wed, Aug 8, 2012 at 9:48 AM, Jói Sigurðsson <joi@chromium.org>
>> >>> wrote:
>> >>>>
>> >>>> It's poorly named.  InfoBarTabHelper is actually where you go to add
>> >>>> an infobar, remove an infobar, and replace an infobar.
>> >>>
>> >>>
>> >>> Right, that's why I suggested maybe adding an interface here that
>> >>> allows
>> >>> one to do this.
>> >>>
>> >>>>
>> >>>> Note also that in the Browser Components project, we decided we would
>> >>>> take as many easy victories as we can first, and then refine the API
>> >>>> as we go along.  For this reason, we are allowing non-abstract
>> >>>> classes
>> >>>> in the api directory as long as they do not introduce header
>> >>>> dependencies on chrome/browser outside the api directory.  This is a
>> >>>> for-now kind of thing; later we will introduce tools to let us write
>> >>>> rules about link-time dependencies and will minimize those just as we
>> >>>> are doing for header dependencies, and this may force introduction of
>> >>>> abstract bases in some cases.  For this class in particular, there
>> >>>> are
>> >>>> probably a few methods that could be moved out of the class that
>> >>>> lives
>> >>>> in the api directory and into a subclass, e.g. the count of infobars
>> >>>> and the way to enumerate delegates.  I should probably add a TODO to
>> >>>> minimize the surface here.
>> >>>
>> >>>
>> >>> I don't think this header should be in a separate directory from its
>> >>> cc
>> >>> file. It's not just an api, it's also an implementation detail of
>> >>> tabcontents. in the api directory, there could be a new interface that
>> >>> has
>> >>> the add/remove infobar interface. until that interface is made, a
>> >>> temporary
>> >>> DEPS exceptions for code to include infobar_tab_helper.h from
>> >>> component
>> >>> directories could be allowed.
>> >>>
>> >>>>
>> >>>> See some more discussion in the chrome/browser/api/DEPS file in this
>> >>>> change.
>> >>>>
>> >>>> Cheers,
>> >>>> Jói
>> >>>>
>> >>>>
>> >>>>
>> >>>> On Wed, Aug 8, 2012 at 4:29 PM,  <jam@chromium.org> wrote:
>> >>>> > drive-by (didn't look in detail, this caught my attention)
>> >>>> >
>> >>>> >
>> >>>> >
>> >>>> >
>> >>>> >
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>> >>>> > File chrome/browser/api/infobars/infobar_tab_helper.h (right):
>> >>>> >
>> >>>> >
>> >>>> >
>> >>>> >
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>> >>>> > chrome/browser/api/infobars/infobar_tab_helper.h:16: class
>> >>>> > InfoBarTabHelper : public content::WebContentsObserver,
>> >>>> > i'm confused, this doesn't seem like an api of infobars that's
>> >>>> > provided
>> >>>> > by the browser. i realize some stuff in here is needed by code that
>> >>>> > puts
>> >>>> > up an infobar, so maybe a new interface should be added here. but
>> >>>> > this
>> >>>> > header seems like it should stay in chrome with the
>> >>>> > infobar_tab_helper.cc file, which is an internal implementation
>> >>>> > detail
>> >>>> > of TabContents.
>> >>>> >
>> >>>> >
>> >>>> > http://codereview.chromium.org/10843071/
>> >>>> >
>> >>>> > --
>> >>>> > You received this message because you are subscribed to the Google
>> >>>> > Groups
>> >>>> > "browser-components-watch" group.
>> >>>> > To post to this group, send email to
>> >>>> > browser-components-watch@chromium.org.
>> >>>> > To unsubscribe from this group, send email to
>> >>>> > browser-components-watch+unsubscribe@chromium.org.
>> >>>> > For more options, visit
>> >>>> > https://groups.google.com/a/chromium.org/groups/opt_out.
>> >>>> >
>> >>>> >
>> >>>
>> >>>
>> >>
>> >> --
>> >> You received this message because you are subscribed to the Google
>> >> Groups
>> >> "browser-components-watch" group.
>> >> To post to this group, send email to
>> >> browser-components-watch@chromium.org.
>> >> To unsubscribe from this group, send email to
>> >> browser-components-watch+unsubscribe@chromium.org.
>> >> For more options, visit
>> >> https://groups.google.com/a/chromium.org/groups/opt_out.
>> >>
>> >>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "browser-components-watch" group.
> To post to this group, send email to browser-components-watch@chromium.org.
> To unsubscribe from this group, send email to
> browser-components-watch+unsubscribe@chromium.org.
> For more options, visit
> https://groups.google.com/a/chromium.org/groups/opt_out.
>
>

[Jói, 2012-08-09 13:40:10]

I just booked something for 10am in MTV, 1pm in MON, 5pm here.  Let me
know if it doesn't work, my time today/tonight is flexible.

Cheers,
Jói


On Thu, Aug 9, 2012 at 1:32 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> Hi John,
>
> Yep, a VC chat sounds like a good idea.
>
> Let me try to answer some of your points quickly so that we know each
> others' position a bit better before we talk live.
>
>> For preventing of more usage of chrome headers that need to be wrapped with
>> APIs, this can be done through deps [...]
>
> Yes, it can be done via a temporarily-allowed include rule in DEPS,
> but this is less valuable than actually moving the header to the api
> directory.  For one thing, a temporarily-allowed rule in one component
> does not document that it is actually OK for other components to
> #include the header, because it is in the process of being made into
> an API.
>
>> Classes like InfobarTabHelper are not public APIs of chrome. That's an
>> internal implementation detail.
>
> I maintain that we can move faster and achieve a large percentage of
> our goals in a short period of time if we allow concrete class
> definitions as API.  Over time, I think we will reach a very similar
> end state to what you are proposing; almost everything in API will
> either be an abstract base, except for a few simple utility or POD
> classes.  My point is that the faster we can rationalize the header
> dependencies, even though we know we will later have to introduce some
> abstractness etc. to do the same for link-time dependencies, the less
> painful the upstreaming process will be for the projects that are
> upstreaming, and the less chance new changes get introduced in the
> code base that do things that will later need to be unwound.
>
> So my point of view is, in the interest of making the biggest quick
> wins we can, we could allow a class like InfoBarTabHelper, that does
> not #include anything not suitable for the api directory, as an API
> for now, although possibly we should reduce its public interface to
> just the Add/Remove/ReplaceInfobar methods.  I don't think the stuff
> in the private section of the class leaks any implementation details
> (it does for human readers, but not for code using the class, which is
> the important thing).  We would then make it abstract as the concrete
> need to do so surfaces.
>
>> The way I view it, if browser components are
>> separate from chrome, then they should talk to it through a thin layer.
>
> Agreed.  Such a thin layer does not need to be completely pure virtual
> though, at least not to achieve many of our initial goals quickly.
> The public section of a class is its interface, and in some cases we
> can simply move the class definitions to the api directory as they
> have a relatively clean public interface surface.  Later, we are
> fairly likely to make the class abstract e.g. to allow easier testing
> or to break link-time dependencies, but we can take the quick wins
> across the breadth of the code base sooner if we allow ourselves to
> defer this work.
>
>> Aside from this, classes like InfobarTabHelper need to have the header and
>> implementation file in the same directory; that's just convention from all
>> the chrome code. Keeping them in different directories is confusing.
>
> This is a convention I think we should allow breaking in the interest
> of expediency.  I don't think it's very confusing; in particular I
> don't think it adds any confusion above the confusion you would have
> if you have an API header in the api directory, and then a concrete
> header in the "regular" directory (alongside the .cc file) that does
> more or less nothing except define a class FooImpl : public Foo {}
> with an empty class body.  In the end you have to find the real
> definition of the class's interface anyway.
>
>> Also, having a component like autofill talk to InfobarTabHelper, instead of
an
>> interface, makes testing harder. It seems that an attractive part of
>> splitting up components is that they can be tested more independently and
>> with less setup of other unnecessary classes. Having interfaces like this
>> would make that easier.
>
> I agree in principle.  However:
> a) Improving the tests for components is part of our plan, but it is
> not one of the first things we should focus on; there are bigger wins
> to achieve earlier than that; and
> b) We don't know for sure at this moment that every class in the API
> will need to be pure virtual to enable testing; if we defer the
> decision we may save some work.
>
> Therefore, I think it makes sense to defer making classes abstract in
> some cases.
>
> Note that I'm not suggesting that every class being used in a
> sort-of-API-like way today should just be moved wholesale to the api
> directory.  In many cases we definitely do need to extract the subset
> of the class's public interface that truly is an API rather than an
> implementation detail into a base interface.  I just want us to have
> the freedom to defer that work when the class's public interface is
> close enough to being suitable as an API.
>
> I'll try to find a time today for us to talk.
>
> Cheers,
> Jói
>
>
>
> On Thu, Aug 9, 2012 at 12:02 AM, John Abd-El-Malek <jam@chromium.org> wrote:
>> hey Joi, I'll attempt to answer your main points. Probably might be worth
>> having a vc to chat about this some more?
>>
>> For preventing of more usage of chrome headers that need to be wrapped with
>> APIs, this can be done through deps. i.e. a component like autofill might
>> have a restrictive DEPS that doesn't allow chrome, but only
>> chrome/browser/api and a temporary allowance for
>> chrome/browser/infobars/infobar_tab_helper.h. With your improvements to
>> checkdeps, we can ensure that no more includes are added.
>>
>> Classes like InfobarTabHelper are not public APIs of chrome. That's an
>> internal implementation detail. The way I view it, if browser components are
>> separate from chrome, then they should talk to it through a thin layer.
>> Aside from this, classes like InfobarTabHelper need to have the header and
>> implementation file in the same directory; that's just convention from all
>> the chrome code. Keeping them in different directories is confusing. Also,
>> having a component like autofill talk to InfobarTabHelper, instead of an
>> interface, makes testing harder. It seems that an attractive part of
>> splitting up components is that they can be tested more independently and
>> with less setup of other unnecessary classes. Having interfaces like this
>> would make that easier.
>>
>> While I agree that adding more interfaces is sometimes harder to read, I
>> think in a codebase the size of chrome, they do make things easier when used
>> to separate large features. In comparing this to the Content API work, I
>> agree that adding interfaces around each component (i.e. autofill) will be a
>> lot of work and may not be necessary. But adding interfaces for how these
>> components talk out is much less effort, since it usually is a small
>> delegate-style interface. This is also what provides the most
>> bang-for-the-buck benefit in terms of encapsulation and testability.
>>
>>
>> On Wed, Aug 8, 2012 at 4:19 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>>
>>> [+browser-components-dev]
>>>
>>> I'm back, so now let me try to explain my thoughts on this.  I'm not
>>> going to try to summarize, because I think it's important that the
>>> stakeholders in this read through my reasoning, but I will try to keep
>>> this structured.
>>>
>>> I'd like to leave the specifics of the current change alone for now,
>>> and focus on the general principles we're operating on.  We can
>>> discuss the specifics of the current change once we settle on the
>>> principles.  I tried to make a similar case to the one I'm making
>>> below while we were doing the content/ work, but I don't think I
>>> explained it well enough or fought for it hard enough, plus the
>>> situation there was somewhat different.
>>>
>>> I think the issues below are important to discuss, because the choice
>>> we make can have a large impact on how quickly we can move, in
>>> particular how much progress we can make in a reasonably short period
>>> of time, and quick progress in this area is important since it can
>>> prevent changes from creeping into the codebase that otherwise will
>>> take time to fix up later.
>>>
>>>
>>> Ben and John, I think both of you are stating a preference that all or
>>> at least most classes in the api directory should be abstract or very
>>> close to abstract, and likely extracted from existing classes similar
>>> to what we did for content/ (e.g. a class previously known as Foo
>>> might be split into an abstract content::Foo interface and a concrete
>>> content::FooImpl implementation).  Pardon me if that's not the exact
>>> position you're representing; hopefully the following is still a worth
>>> while discussion.
>>>
>>> To state my position, I don't think we should extract abstract bases
>>> for everything we place in the api directory.  I do think that we
>>> will end up extracting a lot of abstract bases before we are done, but
>>> I think we should do it only to the extent that it becomes practically
>>> necessary to reach our goals.  I don't think it should be something we
>>> do by default, or something we do right off the bat, before seeing
>>> that there is a concrete need.
>>>
>>> I'm working from the following theories about our desires for Browser
>>> Components:
>>>
>>> a) Our explicit goals could be described as follows: Rationalizing and
>>> simplifying the dependency graph, both in terms of header dependencies
>>> and in terms of link-time dependencies, with the goal of making it
>>> possible to turn off certain components, and to allow injection (or
>>> replacement by other means) of their currently-provided-by-Chrome
>>> dependencies so that components can be reused in new contexts and more
>>> easily unit tested separately.
>>>
>>> b) It is not a goal in and of itself to make all API surfaces into
>>> abstract base classes.  (If it is, then I need to understand the rationale
>>> for that; the use of abstract base classes seems like something that
>>> should be a means to some end rather than a goal in itself).
>>>
>>> c) We want to minimize the time it takes to reach the entirety of our
>>> goals, and at the same time we want to maximize the percentage of our
>>> goals that we can achieve in a reasonably short timeframe (say one or
>>> two quarters), i.e. an asymptotic curve towards our goal is more desirable
>>> than a linear or an exponential one.
>>>
>>> d) We want to help make the upstreaming of projects such as Chrome for
>>> Android and the Android WebView successful, and such upstreaming is
>>> happening in the very near term or has already started.
>>>
>>> Corollaries of the above seem to be that we should defer work that we
>>> are unsure will be necessary, and we should take any iterative quick wins
>>> that we can across the breadth of code, rather than polishing things
>>> fully on deep but narrow segments of code.  Finally, if we are
>>> deferring work that we don't know whether or not will be necessary, we
>>> should have ways to discover as early as possible that it is going to
>>> be necessary.
>>>
>>> Based on my work with John on the content/ refactoring, I am also
>>> working from the theory that extracting an abstract base class is much
>>> costlier in development time than not doing so.  This is especially
>>> true when you get into class hierarchies.
>>>
>>> Finally, I believe that adding the level of indirection that an
>>> abstract base brings can make the code slightly harder for humans to
>>> grok, and that even though each virtual method call is insignificant
>>> in cost, we should not be adding unnecessary virtual method calls
>>> (i.e. where no runtime polymorphism is necessary).  So I think there
>>> is a maintenance and performance burden in addition to the cost of
>>> added up-front development time, small or even tiny per instance but
>>> significant when added up.
>>>
>>> Based on all of those theories, this is roughly the plan I propose
>>> that we follow:
>>>
>>> 1) Start by allowing headers containing concrete class definitions in
>>> the api directory, as long as they don't bring in non-API headers.
>>> This lets us get more quick wins earlier, since we get a more rational
>>> header dependency structure sooner, and fewer temporarily-allowed
>>> rules in DEPS files.
>>>
>>> 2) Defer the work of extracting an abstract base until practical
>>> reasons show that it is actually necessary.  Oftentimes, we may end up
>>> not actually needing runtime polymorphism, we may just need
>>> compile-time specialization in which case e.g. a concrete API class
>>> definition, possibly with a pImpl pattern, is fine.
>>>
>>> 3) Introduce a link-time or target-level dependency analysis tool
>>> similar in spirit to the checkdeps tool we have for headers.  This
>>> tool would let us write what are essentially aspirational link-time
>>> dependency rules (with strict allow/disallow rules, and
>>> temporarily-allowed exceptions).  Such a tool would help us find cases
>>> where our current structure will be insufficient where e.g. abstract bases
>>> plus factories exposed across module boundaries may actually needed
>>> to break link-time dependencies.  (Credit to Erik for the idea of such a
>>> tool, which I think is very good).
>>>
>>> 4) Do iterative work as Browser Components actually start being used
>>> as components, as we start introducing stand-alone test harnesses for
>>> components, etc., and as part of this iterative work, introduce abstract
>>> bases when they show themselves to be necessary in practice, and do
>>> compile-time specialization when appropriate, until we have achieved
>>> our goals fully.
>>>
>>> One last note on the suitability of concrete class definitions as
>>> APIs: A header file containing a class with a publicf API and a
>>> private section containing implementation details does not actually
>>> leak implementation details to code, only to humans.  A private section
>>> is a means of information hiding in the OO programming sense; it is
>>> just an unfortunate coincidence of C++'s design that those hidden
>>> implementation details show up as human-readable in the header file.
>>> For code consuming the header file, those details are still invisible.
>>>
>>> Thanks for reading this far.  I look forward to hearing what you think.
>>>
>>> Cheers,
>>> Jói
>>>
>>>
>>> On Wed, Aug 8, 2012 at 6:37 PM, Jói Sigurðsson <joi@google.com> wrote:
>>> > Ben and John, thanks for the feedback. I have some thoughts on this
>>> > subject
>>> > but am about to meet some friends for dinner. Will try to respond later
>>> > tonight.
>>> >
>>> > Cheers,
>>> > Jói
>>> >
>>> > On Aug 8, 2012 5:58 PM, "Ben Goodger (Google)" <ben@chromium.org> wrote:
>>> >>
>>> >> +1. The api dir should contain an abstract interface that the
>>> >> components
>>> >> use to talk to the browser.
>>> >>
>>> >>
>>> >> On Wed, Aug 8, 2012 at 9:55 AM, John Abd-El-Malek <jam@chromium.org>
>>> >> wrote:
>>> >>>
>>> >>>
>>> >>>
>>> >>> On Wed, Aug 8, 2012 at 9:48 AM, Jói Sigurðsson <joi@chromium.org>
>>> >>> wrote:
>>> >>>>
>>> >>>> It's poorly named.  InfoBarTabHelper is actually where you go to add
>>> >>>> an infobar, remove an infobar, and replace an infobar.
>>> >>>
>>> >>>
>>> >>> Right, that's why I suggested maybe adding an interface here that
>>> >>> allows
>>> >>> one to do this.
>>> >>>
>>> >>>>
>>> >>>> Note also that in the Browser Components project, we decided we would
>>> >>>> take as many easy victories as we can first, and then refine the API
>>> >>>> as we go along.  For this reason, we are allowing non-abstract
>>> >>>> classes
>>> >>>> in the api directory as long as they do not introduce header
>>> >>>> dependencies on chrome/browser outside the api directory.  This is a
>>> >>>> for-now kind of thing; later we will introduce tools to let us write
>>> >>>> rules about link-time dependencies and will minimize those just as we
>>> >>>> are doing for header dependencies, and this may force introduction of
>>> >>>> abstract bases in some cases.  For this class in particular, there
>>> >>>> are
>>> >>>> probably a few methods that could be moved out of the class that
>>> >>>> lives
>>> >>>> in the api directory and into a subclass, e.g. the count of infobars
>>> >>>> and the way to enumerate delegates.  I should probably add a TODO to
>>> >>>> minimize the surface here.
>>> >>>
>>> >>>
>>> >>> I don't think this header should be in a separate directory from its
>>> >>> cc
>>> >>> file. It's not just an api, it's also an implementation detail of
>>> >>> tabcontents. in the api directory, there could be a new interface that
>>> >>> has
>>> >>> the add/remove infobar interface. until that interface is made, a
>>> >>> temporary
>>> >>> DEPS exceptions for code to include infobar_tab_helper.h from
>>> >>> component
>>> >>> directories could be allowed.
>>> >>>
>>> >>>>
>>> >>>> See some more discussion in the chrome/browser/api/DEPS file in this
>>> >>>> change.
>>> >>>>
>>> >>>> Cheers,
>>> >>>> Jói
>>> >>>>
>>> >>>>
>>> >>>>
>>> >>>> On Wed, Aug 8, 2012 at 4:29 PM,  <jam@chromium.org> wrote:
>>> >>>> > drive-by (didn't look in detail, this caught my attention)
>>> >>>> >
>>> >>>> >
>>> >>>> >
>>> >>>> >
>>> >>>> >
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>>> >>>> > File chrome/browser/api/infobars/infobar_tab_helper.h (right):
>>> >>>> >
>>> >>>> >
>>> >>>> >
>>> >>>> >
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>>> >>>> > chrome/browser/api/infobars/infobar_tab_helper.h:16: class
>>> >>>> > InfoBarTabHelper : public content::WebContentsObserver,
>>> >>>> > i'm confused, this doesn't seem like an api of infobars that's
>>> >>>> > provided
>>> >>>> > by the browser. i realize some stuff in here is needed by code that
>>> >>>> > puts
>>> >>>> > up an infobar, so maybe a new interface should be added here. but
>>> >>>> > this
>>> >>>> > header seems like it should stay in chrome with the
>>> >>>> > infobar_tab_helper.cc file, which is an internal implementation
>>> >>>> > detail
>>> >>>> > of TabContents.
>>> >>>> >
>>> >>>> >
>>> >>>> > http://codereview.chromium.org/10843071/
>>> >>>> >
>>> >>>> > --
>>> >>>> > You received this message because you are subscribed to the Google
>>> >>>> > Groups
>>> >>>> > "browser-components-watch" group.
>>> >>>> > To post to this group, send email to
>>> >>>> > browser-components-watch@chromium.org.
>>> >>>> > To unsubscribe from this group, send email to
>>> >>>> > browser-components-watch+unsubscribe@chromium.org.
>>> >>>> > For more options, visit
>>> >>>> > https://groups.google.com/a/chromium.org/groups/opt_out.
>>> >>>> >
>>> >>>> >
>>> >>>
>>> >>>
>>> >>
>>> >> --
>>> >> You received this message because you are subscribed to the Google
>>> >> Groups
>>> >> "browser-components-watch" group.
>>> >> To post to this group, send email to
>>> >> browser-components-watch@chromium.org.
>>> >> To unsubscribe from this group, send email to
>>> >> browser-components-watch+unsubscribe@chromium.org.
>>> >> For more options, visit
>>> >> https://groups.google.com/a/chromium.org/groups/opt_out.
>>> >>
>>> >>
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "browser-components-watch" group.
>> To post to this group, send email to browser-components-watch@chromium.org.
>> To unsubscribe from this group, send email to
>> browser-components-watch+unsubscribe@chromium.org.
>> For more options, visit
>> https://groups.google.com/a/chromium.org/groups/opt_out.
>>
>>

[Jói, 2012-08-09 17:36:43]

Thanks to Ben, John and Erik for their time discussing this with me
just now.  We had a good discussion and I became convinced that it is
a good policy to extract abstract base classes as we move things to
the api directory rather than deferring this extraction in some cases.
 The only exception to this will be similar to content, i.e. very
simple classes whose .cc files can live with them in the api directory
(I think the InfobarDelegate classes in the current change are
examples of this) and classes that are just POD may be able to move to
the api directory without modification.

The rationale goes something like this:

a) Most classes will need to have an abstract API extracted at some
point anyway to break link-time dependencies and to enable unit
testing through mocks;

b) The only classes where we might have been able to save time by
initially moving their definition wholesale to the api directory
without first extracting an abstract base are going to be very simple,
and thus extraction is likely trivial.  Larger more complex classes
almost always have way too large of a public interface surface to be
moved wholesale, and thus you can't realize the savings for these
anyway;

c) The approach we are following and the current status of the work is
likely to be more easily understood by the layprogrammer if we follow
a consistent rule of extracting an abstract base up front rather than
doing it later.  For example, given the content-like approach, if you
look at a class in the api directory and it is non-abstract, you know
it will stay non-abstract.  In the previous proposal, you could have
looked at such a class and been unsure whether it would stay like that
or eventually have an abstract base extracted.

Hope that makes sense.  I will work on updating the change and reuploading
soon.

Cheers,
Jói


On Thu, Aug 9, 2012 at 1:39 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> I just booked something for 10am in MTV, 1pm in MON, 5pm here.  Let me
> know if it doesn't work, my time today/tonight is flexible.
>
> Cheers,
> Jói
>
>
> On Thu, Aug 9, 2012 at 1:32 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>> Hi John,
>>
>> Yep, a VC chat sounds like a good idea.
>>
>> Let me try to answer some of your points quickly so that we know each
>> others' position a bit better before we talk live.
>>
>>> For preventing of more usage of chrome headers that need to be wrapped with
>>> APIs, this can be done through deps [...]
>>
>> Yes, it can be done via a temporarily-allowed include rule in DEPS,
>> but this is less valuable than actually moving the header to the api
>> directory.  For one thing, a temporarily-allowed rule in one component
>> does not document that it is actually OK for other components to
>> #include the header, because it is in the process of being made into
>> an API.
>>
>>> Classes like InfobarTabHelper are not public APIs of chrome. That's an
>>> internal implementation detail.
>>
>> I maintain that we can move faster and achieve a large percentage of
>> our goals in a short period of time if we allow concrete class
>> definitions as API.  Over time, I think we will reach a very similar
>> end state to what you are proposing; almost everything in API will
>> either be an abstract base, except for a few simple utility or POD
>> classes.  My point is that the faster we can rationalize the header
>> dependencies, even though we know we will later have to introduce some
>> abstractness etc. to do the same for link-time dependencies, the less
>> painful the upstreaming process will be for the projects that are
>> upstreaming, and the less chance new changes get introduced in the
>> code base that do things that will later need to be unwound.
>>
>> So my point of view is, in the interest of making the biggest quick
>> wins we can, we could allow a class like InfoBarTabHelper, that does
>> not #include anything not suitable for the api directory, as an API
>> for now, although possibly we should reduce its public interface to
>> just the Add/Remove/ReplaceInfobar methods.  I don't think the stuff
>> in the private section of the class leaks any implementation details
>> (it does for human readers, but not for code using the class, which is
>> the important thing).  We would then make it abstract as the concrete
>> need to do so surfaces.
>>
>>> The way I view it, if browser components are
>>> separate from chrome, then they should talk to it through a thin layer.
>>
>> Agreed.  Such a thin layer does not need to be completely pure virtual
>> though, at least not to achieve many of our initial goals quickly.
>> The public section of a class is its interface, and in some cases we
>> can simply move the class definitions to the api directory as they
>> have a relatively clean public interface surface.  Later, we are
>> fairly likely to make the class abstract e.g. to allow easier testing
>> or to break link-time dependencies, but we can take the quick wins
>> across the breadth of the code base sooner if we allow ourselves to
>> defer this work.
>>
>>> Aside from this, classes like InfobarTabHelper need to have the header and
>>> implementation file in the same directory; that's just convention from all
>>> the chrome code. Keeping them in different directories is confusing.
>>
>> This is a convention I think we should allow breaking in the interest
>> of expediency.  I don't think it's very confusing; in particular I
>> don't think it adds any confusion above the confusion you would have
>> if you have an API header in the api directory, and then a concrete
>> header in the "regular" directory (alongside the .cc file) that does
>> more or less nothing except define a class FooImpl : public Foo {}
>> with an empty class body.  In the end you have to find the real
>> definition of the class's interface anyway.
>>
>>> Also, having a component like autofill talk to InfobarTabHelper, instead of
an
>>> interface, makes testing harder. It seems that an attractive part of
>>> splitting up components is that they can be tested more independently and
>>> with less setup of other unnecessary classes. Having interfaces like this
>>> would make that easier.
>>
>> I agree in principle.  However:
>> a) Improving the tests for components is part of our plan, but it is
>> not one of the first things we should focus on; there are bigger wins
>> to achieve earlier than that; and
>> b) We don't know for sure at this moment that every class in the API
>> will need to be pure virtual to enable testing; if we defer the
>> decision we may save some work.
>>
>> Therefore, I think it makes sense to defer making classes abstract in
>> some cases.
>>
>> Note that I'm not suggesting that every class being used in a
>> sort-of-API-like way today should just be moved wholesale to the api
>> directory.  In many cases we definitely do need to extract the subset
>> of the class's public interface that truly is an API rather than an
>> implementation detail into a base interface.  I just want us to have
>> the freedom to defer that work when the class's public interface is
>> close enough to being suitable as an API.
>>
>> I'll try to find a time today for us to talk.
>>
>> Cheers,
>> Jói
>>
>>
>>
>> On Thu, Aug 9, 2012 at 12:02 AM, John Abd-El-Malek <jam@chromium.org> wrote:
>>> hey Joi, I'll attempt to answer your main points. Probably might be worth
>>> having a vc to chat about this some more?
>>>
>>> For preventing of more usage of chrome headers that need to be wrapped with
>>> APIs, this can be done through deps. i.e. a component like autofill might
>>> have a restrictive DEPS that doesn't allow chrome, but only
>>> chrome/browser/api and a temporary allowance for
>>> chrome/browser/infobars/infobar_tab_helper.h. With your improvements to
>>> checkdeps, we can ensure that no more includes are added.
>>>
>>> Classes like InfobarTabHelper are not public APIs of chrome. That's an
>>> internal implementation detail. The way I view it, if browser components
are
>>> separate from chrome, then they should talk to it through a thin layer.
>>> Aside from this, classes like InfobarTabHelper need to have the header and
>>> implementation file in the same directory; that's just convention from all
>>> the chrome code. Keeping them in different directories is confusing. Also,
>>> having a component like autofill talk to InfobarTabHelper, instead of an
>>> interface, makes testing harder. It seems that an attractive part of
>>> splitting up components is that they can be tested more independently and
>>> with less setup of other unnecessary classes. Having interfaces like this
>>> would make that easier.
>>>
>>> While I agree that adding more interfaces is sometimes harder to read, I
>>> think in a codebase the size of chrome, they do make things easier when
used
>>> to separate large features. In comparing this to the Content API work, I
>>> agree that adding interfaces around each component (i.e. autofill) will be
a
>>> lot of work and may not be necessary. But adding interfaces for how these
>>> components talk out is much less effort, since it usually is a small
>>> delegate-style interface. This is also what provides the most
>>> bang-for-the-buck benefit in terms of encapsulation and testability.
>>>
>>>
>>> On Wed, Aug 8, 2012 at 4:19 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>>>
>>>> [+browser-components-dev]
>>>>
>>>> I'm back, so now let me try to explain my thoughts on this.  I'm not
>>>> going to try to summarize, because I think it's important that the
>>>> stakeholders in this read through my reasoning, but I will try to keep
>>>> this structured.
>>>>
>>>> I'd like to leave the specifics of the current change alone for now,
>>>> and focus on the general principles we're operating on.  We can
>>>> discuss the specifics of the current change once we settle on the
>>>> principles.  I tried to make a similar case to the one I'm making
>>>> below while we were doing the content/ work, but I don't think I
>>>> explained it well enough or fought for it hard enough, plus the
>>>> situation there was somewhat different.
>>>>
>>>> I think the issues below are important to discuss, because the choice
>>>> we make can have a large impact on how quickly we can move, in
>>>> particular how much progress we can make in a reasonably short period
>>>> of time, and quick progress in this area is important since it can
>>>> prevent changes from creeping into the codebase that otherwise will
>>>> take time to fix up later.
>>>>
>>>>
>>>> Ben and John, I think both of you are stating a preference that all or
>>>> at least most classes in the api directory should be abstract or very
>>>> close to abstract, and likely extracted from existing classes similar
>>>> to what we did for content/ (e.g. a class previously known as Foo
>>>> might be split into an abstract content::Foo interface and a concrete
>>>> content::FooImpl implementation).  Pardon me if that's not the exact
>>>> position you're representing; hopefully the following is still a worth
>>>> while discussion.
>>>>
>>>> To state my position, I don't think we should extract abstract bases
>>>> for everything we place in the api directory.  I do think that we
>>>> will end up extracting a lot of abstract bases before we are done, but
>>>> I think we should do it only to the extent that it becomes practically
>>>> necessary to reach our goals.  I don't think it should be something we
>>>> do by default, or something we do right off the bat, before seeing
>>>> that there is a concrete need.
>>>>
>>>> I'm working from the following theories about our desires for Browser
>>>> Components:
>>>>
>>>> a) Our explicit goals could be described as follows: Rationalizing and
>>>> simplifying the dependency graph, both in terms of header dependencies
>>>> and in terms of link-time dependencies, with the goal of making it
>>>> possible to turn off certain components, and to allow injection (or
>>>> replacement by other means) of their currently-provided-by-Chrome
>>>> dependencies so that components can be reused in new contexts and more
>>>> easily unit tested separately.
>>>>
>>>> b) It is not a goal in and of itself to make all API surfaces into
>>>> abstract base classes.  (If it is, then I need to understand the rationale
>>>> for that; the use of abstract base classes seems like something that
>>>> should be a means to some end rather than a goal in itself).
>>>>
>>>> c) We want to minimize the time it takes to reach the entirety of our
>>>> goals, and at the same time we want to maximize the percentage of our
>>>> goals that we can achieve in a reasonably short timeframe (say one or
>>>> two quarters), i.e. an asymptotic curve towards our goal is more desirable
>>>> than a linear or an exponential one.
>>>>
>>>> d) We want to help make the upstreaming of projects such as Chrome for
>>>> Android and the Android WebView successful, and such upstreaming is
>>>> happening in the very near term or has already started.
>>>>
>>>> Corollaries of the above seem to be that we should defer work that we
>>>> are unsure will be necessary, and we should take any iterative quick wins
>>>> that we can across the breadth of code, rather than polishing things
>>>> fully on deep but narrow segments of code.  Finally, if we are
>>>> deferring work that we don't know whether or not will be necessary, we
>>>> should have ways to discover as early as possible that it is going to
>>>> be necessary.
>>>>
>>>> Based on my work with John on the content/ refactoring, I am also
>>>> working from the theory that extracting an abstract base class is much
>>>> costlier in development time than not doing so.  This is especially
>>>> true when you get into class hierarchies.
>>>>
>>>> Finally, I believe that adding the level of indirection that an
>>>> abstract base brings can make the code slightly harder for humans to
>>>> grok, and that even though each virtual method call is insignificant
>>>> in cost, we should not be adding unnecessary virtual method calls
>>>> (i.e. where no runtime polymorphism is necessary).  So I think there
>>>> is a maintenance and performance burden in addition to the cost of
>>>> added up-front development time, small or even tiny per instance but
>>>> significant when added up.
>>>>
>>>> Based on all of those theories, this is roughly the plan I propose
>>>> that we follow:
>>>>
>>>> 1) Start by allowing headers containing concrete class definitions in
>>>> the api directory, as long as they don't bring in non-API headers.
>>>> This lets us get more quick wins earlier, since we get a more rational
>>>> header dependency structure sooner, and fewer temporarily-allowed
>>>> rules in DEPS files.
>>>>
>>>> 2) Defer the work of extracting an abstract base until practical
>>>> reasons show that it is actually necessary.  Oftentimes, we may end up
>>>> not actually needing runtime polymorphism, we may just need
>>>> compile-time specialization in which case e.g. a concrete API class
>>>> definition, possibly with a pImpl pattern, is fine.
>>>>
>>>> 3) Introduce a link-time or target-level dependency analysis tool
>>>> similar in spirit to the checkdeps tool we have for headers.  This
>>>> tool would let us write what are essentially aspirational link-time
>>>> dependency rules (with strict allow/disallow rules, and
>>>> temporarily-allowed exceptions).  Such a tool would help us find cases
>>>> where our current structure will be insufficient where e.g. abstract bases
>>>> plus factories exposed across module boundaries may actually needed
>>>> to break link-time dependencies.  (Credit to Erik for the idea of such a
>>>> tool, which I think is very good).
>>>>
>>>> 4) Do iterative work as Browser Components actually start being used
>>>> as components, as we start introducing stand-alone test harnesses for
>>>> components, etc., and as part of this iterative work, introduce abstract
>>>> bases when they show themselves to be necessary in practice, and do
>>>> compile-time specialization when appropriate, until we have achieved
>>>> our goals fully.
>>>>
>>>> One last note on the suitability of concrete class definitions as
>>>> APIs: A header file containing a class with a publicf API and a
>>>> private section containing implementation details does not actually
>>>> leak implementation details to code, only to humans.  A private section
>>>> is a means of information hiding in the OO programming sense; it is
>>>> just an unfortunate coincidence of C++'s design that those hidden
>>>> implementation details show up as human-readable in the header file.
>>>> For code consuming the header file, those details are still invisible.
>>>>
>>>> Thanks for reading this far.  I look forward to hearing what you think.
>>>>
>>>> Cheers,
>>>> Jói
>>>>
>>>>
>>>> On Wed, Aug 8, 2012 at 6:37 PM, Jói Sigurðsson <joi@google.com> wrote:
>>>> > Ben and John, thanks for the feedback. I have some thoughts on this
>>>> > subject
>>>> > but am about to meet some friends for dinner. Will try to respond later
>>>> > tonight.
>>>> >
>>>> > Cheers,
>>>> > Jói
>>>> >
>>>> > On Aug 8, 2012 5:58 PM, "Ben Goodger (Google)" <ben@chromium.org> wrote:
>>>> >>
>>>> >> +1. The api dir should contain an abstract interface that the
>>>> >> components
>>>> >> use to talk to the browser.
>>>> >>
>>>> >>
>>>> >> On Wed, Aug 8, 2012 at 9:55 AM, John Abd-El-Malek <jam@chromium.org>
>>>> >> wrote:
>>>> >>>
>>>> >>>
>>>> >>>
>>>> >>> On Wed, Aug 8, 2012 at 9:48 AM, Jói Sigurðsson <joi@chromium.org>
>>>> >>> wrote:
>>>> >>>>
>>>> >>>> It's poorly named.  InfoBarTabHelper is actually where you go to add
>>>> >>>> an infobar, remove an infobar, and replace an infobar.
>>>> >>>
>>>> >>>
>>>> >>> Right, that's why I suggested maybe adding an interface here that
>>>> >>> allows
>>>> >>> one to do this.
>>>> >>>
>>>> >>>>
>>>> >>>> Note also that in the Browser Components project, we decided we would
>>>> >>>> take as many easy victories as we can first, and then refine the API
>>>> >>>> as we go along.  For this reason, we are allowing non-abstract
>>>> >>>> classes
>>>> >>>> in the api directory as long as they do not introduce header
>>>> >>>> dependencies on chrome/browser outside the api directory.  This is a
>>>> >>>> for-now kind of thing; later we will introduce tools to let us write
>>>> >>>> rules about link-time dependencies and will minimize those just as we
>>>> >>>> are doing for header dependencies, and this may force introduction of
>>>> >>>> abstract bases in some cases.  For this class in particular, there
>>>> >>>> are
>>>> >>>> probably a few methods that could be moved out of the class that
>>>> >>>> lives
>>>> >>>> in the api directory and into a subclass, e.g. the count of infobars
>>>> >>>> and the way to enumerate delegates.  I should probably add a TODO to
>>>> >>>> minimize the surface here.
>>>> >>>
>>>> >>>
>>>> >>> I don't think this header should be in a separate directory from its
>>>> >>> cc
>>>> >>> file. It's not just an api, it's also an implementation detail of
>>>> >>> tabcontents. in the api directory, there could be a new interface that
>>>> >>> has
>>>> >>> the add/remove infobar interface. until that interface is made, a
>>>> >>> temporary
>>>> >>> DEPS exceptions for code to include infobar_tab_helper.h from
>>>> >>> component
>>>> >>> directories could be allowed.
>>>> >>>
>>>> >>>>
>>>> >>>> See some more discussion in the chrome/browser/api/DEPS file in this
>>>> >>>> change.
>>>> >>>>
>>>> >>>> Cheers,
>>>> >>>> Jói
>>>> >>>>
>>>> >>>>
>>>> >>>>
>>>> >>>> On Wed, Aug 8, 2012 at 4:29 PM,  <jam@chromium.org> wrote:
>>>> >>>> > drive-by (didn't look in detail, this caught my attention)
>>>> >>>> >
>>>> >>>> >
>>>> >>>> >
>>>> >>>> >
>>>> >>>> >
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>>>> >>>> > File chrome/browser/api/infobars/infobar_tab_helper.h (right):
>>>> >>>> >
>>>> >>>> >
>>>> >>>> >
>>>> >>>> >
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>>>> >>>> > chrome/browser/api/infobars/infobar_tab_helper.h:16: class
>>>> >>>> > InfoBarTabHelper : public content::WebContentsObserver,
>>>> >>>> > i'm confused, this doesn't seem like an api of infobars that's
>>>> >>>> > provided
>>>> >>>> > by the browser. i realize some stuff in here is needed by code that
>>>> >>>> > puts
>>>> >>>> > up an infobar, so maybe a new interface should be added here. but
>>>> >>>> > this
>>>> >>>> > header seems like it should stay in chrome with the
>>>> >>>> > infobar_tab_helper.cc file, which is an internal implementation
>>>> >>>> > detail
>>>> >>>> > of TabContents.
>>>> >>>> >
>>>> >>>> >
>>>> >>>> > http://codereview.chromium.org/10843071/
>>>> >>>> >
>>>> >>>> > --
>>>> >>>> > You received this message because you are subscribed to the Google
>>>> >>>> > Groups
>>>> >>>> > "browser-components-watch" group.
>>>> >>>> > To post to this group, send email to
>>>> >>>> > browser-components-watch@chromium.org.
>>>> >>>> > To unsubscribe from this group, send email to
>>>> >>>> > browser-components-watch+unsubscribe@chromium.org.
>>>> >>>> > For more options, visit
>>>> >>>> > https://groups.google.com/a/chromium.org/groups/opt_out.
>>>> >>>> >
>>>> >>>> >
>>>> >>>
>>>> >>>
>>>> >>
>>>> >> --
>>>> >> You received this message because you are subscribed to the Google
>>>> >> Groups
>>>> >> "browser-components-watch" group.
>>>> >> To post to this group, send email to
>>>> >> browser-components-watch@chromium.org.
>>>> >> To unsubscribe from this group, send email to
>>>> >> browser-components-watch+unsubscribe@chromium.org.
>>>> >> For more options, visit
>>>> >> https://groups.google.com/a/chromium.org/groups/opt_out.
>>>> >>
>>>> >>
>>>
>>>
>>> --
>>> You received this message because you are subscribed to the Google Groups
>>> "browser-components-watch" group.
>>> To post to this group, send email to browser-components-watch@chromium.org.
>>> To unsubscribe from this group, send email to
>>> browser-components-watch+unsubscribe@chromium.org.
>>> For more options, visit
>>> https://groups.google.com/a/chromium.org/groups/opt_out.
>>>
>>>

[Jói, 2012-08-09 17:40:03]

John:  What do you think in general of the move of a couple of
InfobarDelegate subclasses and the InfoBarTabHelper to
chrome/browser/infobars?  To me it is a more natural location since
none of those things have any dependency on TabContents and are fairly
coupled with the other infobar code.

For the API extracted out of InfoBarTabHelper, what do you think of
InfoBarService (with Add/Remove/ReplaceInfobar methods)?  I guess if
we call it that, we should also rename InfoBarTabHelper to
InfoBarServiceImpl.  WDYT?

Cheers,
Jói


On Thu, Aug 9, 2012 at 5:35 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> Thanks to Ben, John and Erik for their time discussing this with me
> just now.  We had a good discussion and I became convinced that it is
> a good policy to extract abstract base classes as we move things to
> the api directory rather than deferring this extraction in some cases.
>  The only exception to this will be similar to content, i.e. very
> simple classes whose .cc files can live with them in the api directory
> (I think the InfobarDelegate classes in the current change are
> examples of this) and classes that are just POD may be able to move to
> the api directory without modification.
>
> The rationale goes something like this:
>
> a) Most classes will need to have an abstract API extracted at some
> point anyway to break link-time dependencies and to enable unit
> testing through mocks;
>
> b) The only classes where we might have been able to save time by
> initially moving their definition wholesale to the api directory
> without first extracting an abstract base are going to be very simple,
> and thus extraction is likely trivial.  Larger more complex classes
> almost always have way too large of a public interface surface to be
> moved wholesale, and thus you can't realize the savings for these
> anyway;
>
> c) The approach we are following and the current status of the work is
> likely to be more easily understood by the layprogrammer if we follow
> a consistent rule of extracting an abstract base up front rather than
> doing it later.  For example, given the content-like approach, if you
> look at a class in the api directory and it is non-abstract, you know
> it will stay non-abstract.  In the previous proposal, you could have
> looked at such a class and been unsure whether it would stay like that
> or eventually have an abstract base extracted.
>
> Hope that makes sense.  I will work on updating the change and reuploading
soon.
>
> Cheers,
> Jói
>
>
> On Thu, Aug 9, 2012 at 1:39 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>> I just booked something for 10am in MTV, 1pm in MON, 5pm here.  Let me
>> know if it doesn't work, my time today/tonight is flexible.
>>
>> Cheers,
>> Jói
>>
>>
>> On Thu, Aug 9, 2012 at 1:32 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>> Hi John,
>>>
>>> Yep, a VC chat sounds like a good idea.
>>>
>>> Let me try to answer some of your points quickly so that we know each
>>> others' position a bit better before we talk live.
>>>
>>>> For preventing of more usage of chrome headers that need to be wrapped
with
>>>> APIs, this can be done through deps [...]
>>>
>>> Yes, it can be done via a temporarily-allowed include rule in DEPS,
>>> but this is less valuable than actually moving the header to the api
>>> directory.  For one thing, a temporarily-allowed rule in one component
>>> does not document that it is actually OK for other components to
>>> #include the header, because it is in the process of being made into
>>> an API.
>>>
>>>> Classes like InfobarTabHelper are not public APIs of chrome. That's an
>>>> internal implementation detail.
>>>
>>> I maintain that we can move faster and achieve a large percentage of
>>> our goals in a short period of time if we allow concrete class
>>> definitions as API.  Over time, I think we will reach a very similar
>>> end state to what you are proposing; almost everything in API will
>>> either be an abstract base, except for a few simple utility or POD
>>> classes.  My point is that the faster we can rationalize the header
>>> dependencies, even though we know we will later have to introduce some
>>> abstractness etc. to do the same for link-time dependencies, the less
>>> painful the upstreaming process will be for the projects that are
>>> upstreaming, and the less chance new changes get introduced in the
>>> code base that do things that will later need to be unwound.
>>>
>>> So my point of view is, in the interest of making the biggest quick
>>> wins we can, we could allow a class like InfoBarTabHelper, that does
>>> not #include anything not suitable for the api directory, as an API
>>> for now, although possibly we should reduce its public interface to
>>> just the Add/Remove/ReplaceInfobar methods.  I don't think the stuff
>>> in the private section of the class leaks any implementation details
>>> (it does for human readers, but not for code using the class, which is
>>> the important thing).  We would then make it abstract as the concrete
>>> need to do so surfaces.
>>>
>>>> The way I view it, if browser components are
>>>> separate from chrome, then they should talk to it through a thin layer.
>>>
>>> Agreed.  Such a thin layer does not need to be completely pure virtual
>>> though, at least not to achieve many of our initial goals quickly.
>>> The public section of a class is its interface, and in some cases we
>>> can simply move the class definitions to the api directory as they
>>> have a relatively clean public interface surface.  Later, we are
>>> fairly likely to make the class abstract e.g. to allow easier testing
>>> or to break link-time dependencies, but we can take the quick wins
>>> across the breadth of the code base sooner if we allow ourselves to
>>> defer this work.
>>>
>>>> Aside from this, classes like InfobarTabHelper need to have the header and
>>>> implementation file in the same directory; that's just convention from all
>>>> the chrome code. Keeping them in different directories is confusing.
>>>
>>> This is a convention I think we should allow breaking in the interest
>>> of expediency.  I don't think it's very confusing; in particular I
>>> don't think it adds any confusion above the confusion you would have
>>> if you have an API header in the api directory, and then a concrete
>>> header in the "regular" directory (alongside the .cc file) that does
>>> more or less nothing except define a class FooImpl : public Foo {}
>>> with an empty class body.  In the end you have to find the real
>>> definition of the class's interface anyway.
>>>
>>>> Also, having a component like autofill talk to InfobarTabHelper, instead of
an
>>>> interface, makes testing harder. It seems that an attractive part of
>>>> splitting up components is that they can be tested more independently and
>>>> with less setup of other unnecessary classes. Having interfaces like this
>>>> would make that easier.
>>>
>>> I agree in principle.  However:
>>> a) Improving the tests for components is part of our plan, but it is
>>> not one of the first things we should focus on; there are bigger wins
>>> to achieve earlier than that; and
>>> b) We don't know for sure at this moment that every class in the API
>>> will need to be pure virtual to enable testing; if we defer the
>>> decision we may save some work.
>>>
>>> Therefore, I think it makes sense to defer making classes abstract in
>>> some cases.
>>>
>>> Note that I'm not suggesting that every class being used in a
>>> sort-of-API-like way today should just be moved wholesale to the api
>>> directory.  In many cases we definitely do need to extract the subset
>>> of the class's public interface that truly is an API rather than an
>>> implementation detail into a base interface.  I just want us to have
>>> the freedom to defer that work when the class's public interface is
>>> close enough to being suitable as an API.
>>>
>>> I'll try to find a time today for us to talk.
>>>
>>> Cheers,
>>> Jói
>>>
>>>
>>>
>>> On Thu, Aug 9, 2012 at 12:02 AM, John Abd-El-Malek <jam@chromium.org>
wrote:
>>>> hey Joi, I'll attempt to answer your main points. Probably might be worth
>>>> having a vc to chat about this some more?
>>>>
>>>> For preventing of more usage of chrome headers that need to be wrapped
with
>>>> APIs, this can be done through deps. i.e. a component like autofill might
>>>> have a restrictive DEPS that doesn't allow chrome, but only
>>>> chrome/browser/api and a temporary allowance for
>>>> chrome/browser/infobars/infobar_tab_helper.h. With your improvements to
>>>> checkdeps, we can ensure that no more includes are added.
>>>>
>>>> Classes like InfobarTabHelper are not public APIs of chrome. That's an
>>>> internal implementation detail. The way I view it, if browser components
are
>>>> separate from chrome, then they should talk to it through a thin layer.
>>>> Aside from this, classes like InfobarTabHelper need to have the header and
>>>> implementation file in the same directory; that's just convention from all
>>>> the chrome code. Keeping them in different directories is confusing. Also,
>>>> having a component like autofill talk to InfobarTabHelper, instead of an
>>>> interface, makes testing harder. It seems that an attractive part of
>>>> splitting up components is that they can be tested more independently and
>>>> with less setup of other unnecessary classes. Having interfaces like this
>>>> would make that easier.
>>>>
>>>> While I agree that adding more interfaces is sometimes harder to read, I
>>>> think in a codebase the size of chrome, they do make things easier when
used
>>>> to separate large features. In comparing this to the Content API work, I
>>>> agree that adding interfaces around each component (i.e. autofill) will be
a
>>>> lot of work and may not be necessary. But adding interfaces for how these
>>>> components talk out is much less effort, since it usually is a small
>>>> delegate-style interface. This is also what provides the most
>>>> bang-for-the-buck benefit in terms of encapsulation and testability.
>>>>
>>>>
>>>> On Wed, Aug 8, 2012 at 4:19 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>>>>
>>>>> [+browser-components-dev]
>>>>>
>>>>> I'm back, so now let me try to explain my thoughts on this.  I'm not
>>>>> going to try to summarize, because I think it's important that the
>>>>> stakeholders in this read through my reasoning, but I will try to keep
>>>>> this structured.
>>>>>
>>>>> I'd like to leave the specifics of the current change alone for now,
>>>>> and focus on the general principles we're operating on.  We can
>>>>> discuss the specifics of the current change once we settle on the
>>>>> principles.  I tried to make a similar case to the one I'm making
>>>>> below while we were doing the content/ work, but I don't think I
>>>>> explained it well enough or fought for it hard enough, plus the
>>>>> situation there was somewhat different.
>>>>>
>>>>> I think the issues below are important to discuss, because the choice
>>>>> we make can have a large impact on how quickly we can move, in
>>>>> particular how much progress we can make in a reasonably short period
>>>>> of time, and quick progress in this area is important since it can
>>>>> prevent changes from creeping into the codebase that otherwise will
>>>>> take time to fix up later.
>>>>>
>>>>>
>>>>> Ben and John, I think both of you are stating a preference that all or
>>>>> at least most classes in the api directory should be abstract or very
>>>>> close to abstract, and likely extracted from existing classes similar
>>>>> to what we did for content/ (e.g. a class previously known as Foo
>>>>> might be split into an abstract content::Foo interface and a concrete
>>>>> content::FooImpl implementation).  Pardon me if that's not the exact
>>>>> position you're representing; hopefully the following is still a worth
>>>>> while discussion.
>>>>>
>>>>> To state my position, I don't think we should extract abstract bases
>>>>> for everything we place in the api directory.  I do think that we
>>>>> will end up extracting a lot of abstract bases before we are done, but
>>>>> I think we should do it only to the extent that it becomes practically
>>>>> necessary to reach our goals.  I don't think it should be something we
>>>>> do by default, or something we do right off the bat, before seeing
>>>>> that there is a concrete need.
>>>>>
>>>>> I'm working from the following theories about our desires for Browser
>>>>> Components:
>>>>>
>>>>> a) Our explicit goals could be described as follows: Rationalizing and
>>>>> simplifying the dependency graph, both in terms of header dependencies
>>>>> and in terms of link-time dependencies, with the goal of making it
>>>>> possible to turn off certain components, and to allow injection (or
>>>>> replacement by other means) of their currently-provided-by-Chrome
>>>>> dependencies so that components can be reused in new contexts and more
>>>>> easily unit tested separately.
>>>>>
>>>>> b) It is not a goal in and of itself to make all API surfaces into
>>>>> abstract base classes.  (If it is, then I need to understand the
rationale
>>>>> for that; the use of abstract base classes seems like something that
>>>>> should be a means to some end rather than a goal in itself).
>>>>>
>>>>> c) We want to minimize the time it takes to reach the entirety of our
>>>>> goals, and at the same time we want to maximize the percentage of our
>>>>> goals that we can achieve in a reasonably short timeframe (say one or
>>>>> two quarters), i.e. an asymptotic curve towards our goal is more
desirable
>>>>> than a linear or an exponential one.
>>>>>
>>>>> d) We want to help make the upstreaming of projects such as Chrome for
>>>>> Android and the Android WebView successful, and such upstreaming is
>>>>> happening in the very near term or has already started.
>>>>>
>>>>> Corollaries of the above seem to be that we should defer work that we
>>>>> are unsure will be necessary, and we should take any iterative quick wins
>>>>> that we can across the breadth of code, rather than polishing things
>>>>> fully on deep but narrow segments of code.  Finally, if we are
>>>>> deferring work that we don't know whether or not will be necessary, we
>>>>> should have ways to discover as early as possible that it is going to
>>>>> be necessary.
>>>>>
>>>>> Based on my work with John on the content/ refactoring, I am also
>>>>> working from the theory that extracting an abstract base class is much
>>>>> costlier in development time than not doing so.  This is especially
>>>>> true when you get into class hierarchies.
>>>>>
>>>>> Finally, I believe that adding the level of indirection that an
>>>>> abstract base brings can make the code slightly harder for humans to
>>>>> grok, and that even though each virtual method call is insignificant
>>>>> in cost, we should not be adding unnecessary virtual method calls
>>>>> (i.e. where no runtime polymorphism is necessary).  So I think there
>>>>> is a maintenance and performance burden in addition to the cost of
>>>>> added up-front development time, small or even tiny per instance but
>>>>> significant when added up.
>>>>>
>>>>> Based on all of those theories, this is roughly the plan I propose
>>>>> that we follow:
>>>>>
>>>>> 1) Start by allowing headers containing concrete class definitions in
>>>>> the api directory, as long as they don't bring in non-API headers.
>>>>> This lets us get more quick wins earlier, since we get a more rational
>>>>> header dependency structure sooner, and fewer temporarily-allowed
>>>>> rules in DEPS files.
>>>>>
>>>>> 2) Defer the work of extracting an abstract base until practical
>>>>> reasons show that it is actually necessary.  Oftentimes, we may end up
>>>>> not actually needing runtime polymorphism, we may just need
>>>>> compile-time specialization in which case e.g. a concrete API class
>>>>> definition, possibly with a pImpl pattern, is fine.
>>>>>
>>>>> 3) Introduce a link-time or target-level dependency analysis tool
>>>>> similar in spirit to the checkdeps tool we have for headers.  This
>>>>> tool would let us write what are essentially aspirational link-time
>>>>> dependency rules (with strict allow/disallow rules, and
>>>>> temporarily-allowed exceptions).  Such a tool would help us find cases
>>>>> where our current structure will be insufficient where e.g. abstract
bases
>>>>> plus factories exposed across module boundaries may actually needed
>>>>> to break link-time dependencies.  (Credit to Erik for the idea of such a
>>>>> tool, which I think is very good).
>>>>>
>>>>> 4) Do iterative work as Browser Components actually start being used
>>>>> as components, as we start introducing stand-alone test harnesses for
>>>>> components, etc., and as part of this iterative work, introduce abstract
>>>>> bases when they show themselves to be necessary in practice, and do
>>>>> compile-time specialization when appropriate, until we have achieved
>>>>> our goals fully.
>>>>>
>>>>> One last note on the suitability of concrete class definitions as
>>>>> APIs: A header file containing a class with a publicf API and a
>>>>> private section containing implementation details does not actually
>>>>> leak implementation details to code, only to humans.  A private section
>>>>> is a means of information hiding in the OO programming sense; it is
>>>>> just an unfortunate coincidence of C++'s design that those hidden
>>>>> implementation details show up as human-readable in the header file.
>>>>> For code consuming the header file, those details are still invisible.
>>>>>
>>>>> Thanks for reading this far.  I look forward to hearing what you think.
>>>>>
>>>>> Cheers,
>>>>> Jói
>>>>>
>>>>>
>>>>> On Wed, Aug 8, 2012 at 6:37 PM, Jói Sigurðsson <joi@google.com> wrote:
>>>>> > Ben and John, thanks for the feedback. I have some thoughts on this
>>>>> > subject
>>>>> > but am about to meet some friends for dinner. Will try to respond later
>>>>> > tonight.
>>>>> >
>>>>> > Cheers,
>>>>> > Jói
>>>>> >
>>>>> > On Aug 8, 2012 5:58 PM, "Ben Goodger (Google)" <ben@chromium.org>
wrote:
>>>>> >>
>>>>> >> +1. The api dir should contain an abstract interface that the
>>>>> >> components
>>>>> >> use to talk to the browser.
>>>>> >>
>>>>> >>
>>>>> >> On Wed, Aug 8, 2012 at 9:55 AM, John Abd-El-Malek <jam@chromium.org>
>>>>> >> wrote:
>>>>> >>>
>>>>> >>>
>>>>> >>>
>>>>> >>> On Wed, Aug 8, 2012 at 9:48 AM, Jói Sigurðsson <joi@chromium.org>
>>>>> >>> wrote:
>>>>> >>>>
>>>>> >>>> It's poorly named.  InfoBarTabHelper is actually where you go to add
>>>>> >>>> an infobar, remove an infobar, and replace an infobar.
>>>>> >>>
>>>>> >>>
>>>>> >>> Right, that's why I suggested maybe adding an interface here that
>>>>> >>> allows
>>>>> >>> one to do this.
>>>>> >>>
>>>>> >>>>
>>>>> >>>> Note also that in the Browser Components project, we decided we
would
>>>>> >>>> take as many easy victories as we can first, and then refine the API
>>>>> >>>> as we go along.  For this reason, we are allowing non-abstract
>>>>> >>>> classes
>>>>> >>>> in the api directory as long as they do not introduce header
>>>>> >>>> dependencies on chrome/browser outside the api directory.  This is a
>>>>> >>>> for-now kind of thing; later we will introduce tools to let us write
>>>>> >>>> rules about link-time dependencies and will minimize those just as
we
>>>>> >>>> are doing for header dependencies, and this may force introduction
of
>>>>> >>>> abstract bases in some cases.  For this class in particular, there
>>>>> >>>> are
>>>>> >>>> probably a few methods that could be moved out of the class that
>>>>> >>>> lives
>>>>> >>>> in the api directory and into a subclass, e.g. the count of infobars
>>>>> >>>> and the way to enumerate delegates.  I should probably add a TODO to
>>>>> >>>> minimize the surface here.
>>>>> >>>
>>>>> >>>
>>>>> >>> I don't think this header should be in a separate directory from its
>>>>> >>> cc
>>>>> >>> file. It's not just an api, it's also an implementation detail of
>>>>> >>> tabcontents. in the api directory, there could be a new interface
that
>>>>> >>> has
>>>>> >>> the add/remove infobar interface. until that interface is made, a
>>>>> >>> temporary
>>>>> >>> DEPS exceptions for code to include infobar_tab_helper.h from
>>>>> >>> component
>>>>> >>> directories could be allowed.
>>>>> >>>
>>>>> >>>>
>>>>> >>>> See some more discussion in the chrome/browser/api/DEPS file in this
>>>>> >>>> change.
>>>>> >>>>
>>>>> >>>> Cheers,
>>>>> >>>> Jói
>>>>> >>>>
>>>>> >>>>
>>>>> >>>>
>>>>> >>>> On Wed, Aug 8, 2012 at 4:29 PM,  <jam@chromium.org> wrote:
>>>>> >>>> > drive-by (didn't look in detail, this caught my attention)
>>>>> >>>> >
>>>>> >>>> >
>>>>> >>>> >
>>>>> >>>> >
>>>>> >>>> >
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>>>>> >>>> > File chrome/browser/api/infobars/infobar_tab_helper.h (right):
>>>>> >>>> >
>>>>> >>>> >
>>>>> >>>> >
>>>>> >>>> >
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>>>>> >>>> > chrome/browser/api/infobars/infobar_tab_helper.h:16: class
>>>>> >>>> > InfoBarTabHelper : public content::WebContentsObserver,
>>>>> >>>> > i'm confused, this doesn't seem like an api of infobars that's
>>>>> >>>> > provided
>>>>> >>>> > by the browser. i realize some stuff in here is needed by code
that
>>>>> >>>> > puts
>>>>> >>>> > up an infobar, so maybe a new interface should be added here. but
>>>>> >>>> > this
>>>>> >>>> > header seems like it should stay in chrome with the
>>>>> >>>> > infobar_tab_helper.cc file, which is an internal implementation
>>>>> >>>> > detail
>>>>> >>>> > of TabContents.
>>>>> >>>> >
>>>>> >>>> >
>>>>> >>>> > http://codereview.chromium.org/10843071/
>>>>> >>>> >
>>>>> >>>> > --
>>>>> >>>> > You received this message because you are subscribed to the Google
>>>>> >>>> > Groups
>>>>> >>>> > "browser-components-watch" group.
>>>>> >>>> > To post to this group, send email to
>>>>> >>>> > browser-components-watch@chromium.org.
>>>>> >>>> > To unsubscribe from this group, send email to
>>>>> >>>> > browser-components-watch+unsubscribe@chromium.org.
>>>>> >>>> > For more options, visit
>>>>> >>>> > https://groups.google.com/a/chromium.org/groups/opt_out.
>>>>> >>>> >
>>>>> >>>> >
>>>>> >>>
>>>>> >>>
>>>>> >>
>>>>> >> --
>>>>> >> You received this message because you are subscribed to the Google
>>>>> >> Groups
>>>>> >> "browser-components-watch" group.
>>>>> >> To post to this group, send email to
>>>>> >> browser-components-watch@chromium.org.
>>>>> >> To unsubscribe from this group, send email to
>>>>> >> browser-components-watch+unsubscribe@chromium.org.
>>>>> >> For more options, visit
>>>>> >> https://groups.google.com/a/chromium.org/groups/opt_out.
>>>>> >>
>>>>> >>
>>>>
>>>>
>>>> --
>>>> You received this message because you are subscribed to the Google Groups
>>>> "browser-components-watch" group.
>>>> To post to this group, send email to
browser-components-watch@chromium.org.
>>>> To unsubscribe from this group, send email to
>>>> browser-components-watch+unsubscribe@chromium.org.
>>>> For more options, visit
>>>> https://groups.google.com/a/chromium.org/groups/opt_out.
>>>>
>>>>

[Ilya Sherman, 2012-08-09 17:55:06]

/cc pkasting

[jam, 2012-08-09 18:37:19]

On Thu, Aug 9, 2012 at 10:39 AM, Jói Sigurðsson <joi@chromium.org> wrote:

> John:  What do you think in general of the move of a couple of
> InfobarDelegate subclasses


I thought the InfobarDelegates would all move to the api directory?


> and the InfoBarTabHelper to
> chrome/browser/infobars?


that one is already there?

 To me it is a more natural location since
> none of those things have any dependency on TabContents and are fairly
> coupled with the other infobar code.
>
> For the API extracted out of InfoBarTabHelper, what do you think of
> InfoBarService (with Add/Remove/ReplaceInfobar methods)?


hmm. I'll defer to other folks who spend more time in src/chrome than me :)


>  I guess if
> we call it that, we should also rename InfoBarTabHelper to
> InfoBarServiceImpl.  WDYT?
>

I think since this is still something that hangs off TabContents, it should
stay being a TabHelper to be consistent with all the other objects that
hang off it (Avi has felt strongly about that convention, see the
documentation in tab_contents.h).

>
> Cheers,
> Jói
>
>
> On Thu, Aug 9, 2012 at 5:35 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> > Thanks to Ben, John and Erik for their time discussing this with me
> > just now.  We had a good discussion and I became convinced that it is
> > a good policy to extract abstract base classes as we move things to
> > the api directory rather than deferring this extraction in some cases.
> >  The only exception to this will be similar to content, i.e. very
> > simple classes whose .cc files can live with them in the api directory
> > (I think the InfobarDelegate classes in the current change are
> > examples of this) and classes that are just POD may be able to move to
> > the api directory without modification.
> >
> > The rationale goes something like this:
> >
> > a) Most classes will need to have an abstract API extracted at some
> > point anyway to break link-time dependencies and to enable unit
> > testing through mocks;
> >
> > b) The only classes where we might have been able to save time by
> > initially moving their definition wholesale to the api directory
> > without first extracting an abstract base are going to be very simple,
> > and thus extraction is likely trivial.  Larger more complex classes
> > almost always have way too large of a public interface surface to be
> > moved wholesale, and thus you can't realize the savings for these
> > anyway;
> >
> > c) The approach we are following and the current status of the work is
> > likely to be more easily understood by the layprogrammer if we follow
> > a consistent rule of extracting an abstract base up front rather than
> > doing it later.  For example, given the content-like approach, if you
> > look at a class in the api directory and it is non-abstract, you know
> > it will stay non-abstract.  In the previous proposal, you could have
> > looked at such a class and been unsure whether it would stay like that
> > or eventually have an abstract base extracted.
> >
> > Hope that makes sense.  I will work on updating the change and
> reuploading soon.
> >
> > Cheers,
> > Jói
> >
> >
> > On Thu, Aug 9, 2012 at 1:39 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> >> I just booked something for 10am in MTV, 1pm in MON, 5pm here.  Let me
> >> know if it doesn't work, my time today/tonight is flexible.
> >>
> >> Cheers,
> >> Jói
> >>
> >>
> >> On Thu, Aug 9, 2012 at 1:32 PM, Jói Sigurðsson <joi@chromium.org>
> wrote:
> >>> Hi John,
> >>>
> >>> Yep, a VC chat sounds like a good idea.
> >>>
> >>> Let me try to answer some of your points quickly so that we know each
> >>> others' position a bit better before we talk live.
> >>>
> >>>> For preventing of more usage of chrome headers that need to be
> wrapped with
> >>>> APIs, this can be done through deps [...]
> >>>
> >>> Yes, it can be done via a temporarily-allowed include rule in DEPS,
> >>> but this is less valuable than actually moving the header to the api
> >>> directory.  For one thing, a temporarily-allowed rule in one component
> >>> does not document that it is actually OK for other components to
> >>> #include the header, because it is in the process of being made into
> >>> an API.
> >>>
> >>>> Classes like InfobarTabHelper are not public APIs of chrome. That's an
> >>>> internal implementation detail.
> >>>
> >>> I maintain that we can move faster and achieve a large percentage of
> >>> our goals in a short period of time if we allow concrete class
> >>> definitions as API.  Over time, I think we will reach a very similar
> >>> end state to what you are proposing; almost everything in API will
> >>> either be an abstract base, except for a few simple utility or POD
> >>> classes.  My point is that the faster we can rationalize the header
> >>> dependencies, even though we know we will later have to introduce some
> >>> abstractness etc. to do the same for link-time dependencies, the less
> >>> painful the upstreaming process will be for the projects that are
> >>> upstreaming, and the less chance new changes get introduced in the
> >>> code base that do things that will later need to be unwound.
> >>>
> >>> So my point of view is, in the interest of making the biggest quick
> >>> wins we can, we could allow a class like InfoBarTabHelper, that does
> >>> not #include anything not suitable for the api directory, as an API
> >>> for now, although possibly we should reduce its public interface to
> >>> just the Add/Remove/ReplaceInfobar methods.  I don't think the stuff
> >>> in the private section of the class leaks any implementation details
> >>> (it does for human readers, but not for code using the class, which is
> >>> the important thing).  We would then make it abstract as the concrete
> >>> need to do so surfaces.
> >>>
> >>>> The way I view it, if browser components are
> >>>> separate from chrome, then they should talk to it through a thin
> layer.
> >>>
> >>> Agreed.  Such a thin layer does not need to be completely pure virtual
> >>> though, at least not to achieve many of our initial goals quickly.
> >>> The public section of a class is its interface, and in some cases we
> >>> can simply move the class definitions to the api directory as they
> >>> have a relatively clean public interface surface.  Later, we are
> >>> fairly likely to make the class abstract e.g. to allow easier testing
> >>> or to break link-time dependencies, but we can take the quick wins
> >>> across the breadth of the code base sooner if we allow ourselves to
> >>> defer this work.
> >>>
> >>>> Aside from this, classes like InfobarTabHelper need to have the
> header and
> >>>> implementation file in the same directory; that's just convention
> from all
> >>>> the chrome code. Keeping them in different directories is confusing.
> >>>
> >>> This is a convention I think we should allow breaking in the interest
> >>> of expediency.  I don't think it's very confusing; in particular I
> >>> don't think it adds any confusion above the confusion you would have
> >>> if you have an API header in the api directory, and then a concrete
> >>> header in the "regular" directory (alongside the .cc file) that does
> >>> more or less nothing except define a class FooImpl : public Foo {}
> >>> with an empty class body.  In the end you have to find the real
> >>> definition of the class's interface anyway.
> >>>
> >>>> Also, having a component like autofill talk to InfobarTabHelper,
> instead of an
> >>>> interface, makes testing harder. It seems that an attractive part of
> >>>> splitting up components is that they can be tested more independently
> and
> >>>> with less setup of other unnecessary classes. Having interfaces like
> this
> >>>> would make that easier.
> >>>
> >>> I agree in principle.  However:
> >>> a) Improving the tests for components is part of our plan, but it is
> >>> not one of the first things we should focus on; there are bigger wins
> >>> to achieve earlier than that; and
> >>> b) We don't know for sure at this moment that every class in the API
> >>> will need to be pure virtual to enable testing; if we defer the
> >>> decision we may save some work.
> >>>
> >>> Therefore, I think it makes sense to defer making classes abstract in
> >>> some cases.
> >>>
> >>> Note that I'm not suggesting that every class being used in a
> >>> sort-of-API-like way today should just be moved wholesale to the api
> >>> directory.  In many cases we definitely do need to extract the subset
> >>> of the class's public interface that truly is an API rather than an
> >>> implementation detail into a base interface.  I just want us to have
> >>> the freedom to defer that work when the class's public interface is
> >>> close enough to being suitable as an API.
> >>>
> >>> I'll try to find a time today for us to talk.
> >>>
> >>> Cheers,
> >>> Jói
> >>>
> >>>
> >>>
> >>> On Thu, Aug 9, 2012 at 12:02 AM, John Abd-El-Malek <jam@chromium.org>
> wrote:
> >>>> hey Joi, I'll attempt to answer your main points. Probably might be
> worth
> >>>> having a vc to chat about this some more?
> >>>>
> >>>> For preventing of more usage of chrome headers that need to be
> wrapped with
> >>>> APIs, this can be done through deps. i.e. a component like autofill
> might
> >>>> have a restrictive DEPS that doesn't allow chrome, but only
> >>>> chrome/browser/api and a temporary allowance for
> >>>> chrome/browser/infobars/infobar_tab_helper.h. With your improvements
> to
> >>>> checkdeps, we can ensure that no more includes are added.
> >>>>
> >>>> Classes like InfobarTabHelper are not public APIs of chrome. That's an
> >>>> internal implementation detail. The way I view it, if browser
> components are
> >>>> separate from chrome, then they should talk to it through a thin
> layer.
> >>>> Aside from this, classes like InfobarTabHelper need to have the
> header and
> >>>> implementation file in the same directory; that's just convention
> from all
> >>>> the chrome code. Keeping them in different directories is confusing.
> Also,
> >>>> having a component like autofill talk to InfobarTabHelper, instead of
> an
> >>>> interface, makes testing harder. It seems that an attractive part of
> >>>> splitting up components is that they can be tested more independently
> and
> >>>> with less setup of other unnecessary classes. Having interfaces like
> this
> >>>> would make that easier.
> >>>>
> >>>> While I agree that adding more interfaces is sometimes harder to
> read, I
> >>>> think in a codebase the size of chrome, they do make things easier
> when used
> >>>> to separate large features. In comparing this to the Content API
> work, I
> >>>> agree that adding interfaces around each component (i.e. autofill)
> will be a
> >>>> lot of work and may not be necessary. But adding interfaces for how
> these
> >>>> components talk out is much less effort, since it usually is a small
> >>>> delegate-style interface. This is also what provides the most
> >>>> bang-for-the-buck benefit in terms of encapsulation and testability.
> >>>>
> >>>>
> >>>> On Wed, Aug 8, 2012 at 4:19 PM, Jói Sigurðsson <joi@chromium.org>
> wrote:
> >>>>>
> >>>>> [+browser-components-dev]
> >>>>>
> >>>>> I'm back, so now let me try to explain my thoughts on this.  I'm not
> >>>>> going to try to summarize, because I think it's important that the
> >>>>> stakeholders in this read through my reasoning, but I will try to
> keep
> >>>>> this structured.
> >>>>>
> >>>>> I'd like to leave the specifics of the current change alone for now,
> >>>>> and focus on the general principles we're operating on.  We can
> >>>>> discuss the specifics of the current change once we settle on the
> >>>>> principles.  I tried to make a similar case to the one I'm making
> >>>>> below while we were doing the content/ work, but I don't think I
> >>>>> explained it well enough or fought for it hard enough, plus the
> >>>>> situation there was somewhat different.
> >>>>>
> >>>>> I think the issues below are important to discuss, because the choice
> >>>>> we make can have a large impact on how quickly we can move, in
> >>>>> particular how much progress we can make in a reasonably short period
> >>>>> of time, and quick progress in this area is important since it can
> >>>>> prevent changes from creeping into the codebase that otherwise will
> >>>>> take time to fix up later.
> >>>>>
> >>>>>
> >>>>> Ben and John, I think both of you are stating a preference that all
> or
> >>>>> at least most classes in the api directory should be abstract or very
> >>>>> close to abstract, and likely extracted from existing classes similar
> >>>>> to what we did for content/ (e.g. a class previously known as Foo
> >>>>> might be split into an abstract content::Foo interface and a concrete
> >>>>> content::FooImpl implementation).  Pardon me if that's not the exact
> >>>>> position you're representing; hopefully the following is still a
> worth
> >>>>> while discussion.
> >>>>>
> >>>>> To state my position, I don't think we should extract abstract bases
> >>>>> for everything we place in the api directory.  I do think that we
> >>>>> will end up extracting a lot of abstract bases before we are done,
> but
> >>>>> I think we should do it only to the extent that it becomes
> practically
> >>>>> necessary to reach our goals.  I don't think it should be something
> we
> >>>>> do by default, or something we do right off the bat, before seeing
> >>>>> that there is a concrete need.
> >>>>>
> >>>>> I'm working from the following theories about our desires for Browser
> >>>>> Components:
> >>>>>
> >>>>> a) Our explicit goals could be described as follows: Rationalizing
> and
> >>>>> simplifying the dependency graph, both in terms of header
> dependencies
> >>>>> and in terms of link-time dependencies, with the goal of making it
> >>>>> possible to turn off certain components, and to allow injection (or
> >>>>> replacement by other means) of their currently-provided-by-Chrome
> >>>>> dependencies so that components can be reused in new contexts and
> more
> >>>>> easily unit tested separately.
> >>>>>
> >>>>> b) It is not a goal in and of itself to make all API surfaces into
> >>>>> abstract base classes.  (If it is, then I need to understand the
> rationale
> >>>>> for that; the use of abstract base classes seems like something that
> >>>>> should be a means to some end rather than a goal in itself).
> >>>>>
> >>>>> c) We want to minimize the time it takes to reach the entirety of our
> >>>>> goals, and at the same time we want to maximize the percentage of our
> >>>>> goals that we can achieve in a reasonably short timeframe (say one or
> >>>>> two quarters), i.e. an asymptotic curve towards our goal is more
> desirable
> >>>>> than a linear or an exponential one.
> >>>>>
> >>>>> d) We want to help make the upstreaming of projects such as Chrome
> for
> >>>>> Android and the Android WebView successful, and such upstreaming is
> >>>>> happening in the very near term or has already started.
> >>>>>
> >>>>> Corollaries of the above seem to be that we should defer work that we
> >>>>> are unsure will be necessary, and we should take any iterative quick
> wins
> >>>>> that we can across the breadth of code, rather than polishing things
> >>>>> fully on deep but narrow segments of code.  Finally, if we are
> >>>>> deferring work that we don't know whether or not will be necessary,
> we
> >>>>> should have ways to discover as early as possible that it is going to
> >>>>> be necessary.
> >>>>>
> >>>>> Based on my work with John on the content/ refactoring, I am also
> >>>>> working from the theory that extracting an abstract base class is
> much
> >>>>> costlier in development time than not doing so.  This is especially
> >>>>> true when you get into class hierarchies.
> >>>>>
> >>>>> Finally, I believe that adding the level of indirection that an
> >>>>> abstract base brings can make the code slightly harder for humans to
> >>>>> grok, and that even though each virtual method call is insignificant
> >>>>> in cost, we should not be adding unnecessary virtual method calls
> >>>>> (i.e. where no runtime polymorphism is necessary).  So I think there
> >>>>> is a maintenance and performance burden in addition to the cost of
> >>>>> added up-front development time, small or even tiny per instance but
> >>>>> significant when added up.
> >>>>>
> >>>>> Based on all of those theories, this is roughly the plan I propose
> >>>>> that we follow:
> >>>>>
> >>>>> 1) Start by allowing headers containing concrete class definitions in
> >>>>> the api directory, as long as they don't bring in non-API headers.
> >>>>> This lets us get more quick wins earlier, since we get a more
> rational
> >>>>> header dependency structure sooner, and fewer temporarily-allowed
> >>>>> rules in DEPS files.
> >>>>>
> >>>>> 2) Defer the work of extracting an abstract base until practical
> >>>>> reasons show that it is actually necessary.  Oftentimes, we may end
> up
> >>>>> not actually needing runtime polymorphism, we may just need
> >>>>> compile-time specialization in which case e.g. a concrete API class
> >>>>> definition, possibly with a pImpl pattern, is fine.
> >>>>>
> >>>>> 3) Introduce a link-time or target-level dependency analysis tool
> >>>>> similar in spirit to the checkdeps tool we have for headers.  This
> >>>>> tool would let us write what are essentially aspirational link-time
> >>>>> dependency rules (with strict allow/disallow rules, and
> >>>>> temporarily-allowed exceptions).  Such a tool would help us find
> cases
> >>>>> where our current structure will be insufficient where e.g. abstract
> bases
> >>>>> plus factories exposed across module boundaries may actually needed
> >>>>> to break link-time dependencies.  (Credit to Erik for the idea of
> such a
> >>>>> tool, which I think is very good).
> >>>>>
> >>>>> 4) Do iterative work as Browser Components actually start being used
> >>>>> as components, as we start introducing stand-alone test harnesses for
> >>>>> components, etc., and as part of this iterative work, introduce
> abstract
> >>>>> bases when they show themselves to be necessary in practice, and do
> >>>>> compile-time specialization when appropriate, until we have achieved
> >>>>> our goals fully.
> >>>>>
> >>>>> One last note on the suitability of concrete class definitions as
> >>>>> APIs: A header file containing a class with a publicf API and a
> >>>>> private section containing implementation details does not actually
> >>>>> leak implementation details to code, only to humans.  A private
> section
> >>>>> is a means of information hiding in the OO programming sense; it is
> >>>>> just an unfortunate coincidence of C++'s design that those hidden
> >>>>> implementation details show up as human-readable in the header file.
> >>>>> For code consuming the header file, those details are still
> invisible.
> >>>>>
> >>>>> Thanks for reading this far.  I look forward to hearing what you
> think.
> >>>>>
> >>>>> Cheers,
> >>>>> Jói
> >>>>>
> >>>>>
> >>>>> On Wed, Aug 8, 2012 at 6:37 PM, Jói Sigurðsson <joi@google.com>
> wrote:
> >>>>> > Ben and John, thanks for the feedback. I have some thoughts on this
> >>>>> > subject
> >>>>> > but am about to meet some friends for dinner. Will try to respond
> later
> >>>>> > tonight.
> >>>>> >
> >>>>> > Cheers,
> >>>>> > Jói
> >>>>> >
> >>>>> > On Aug 8, 2012 5:58 PM, "Ben Goodger (Google)" <ben@chromium.org>
> wrote:
> >>>>> >>
> >>>>> >> +1. The api dir should contain an abstract interface that the
> >>>>> >> components
> >>>>> >> use to talk to the browser.
> >>>>> >>
> >>>>> >>
> >>>>> >> On Wed, Aug 8, 2012 at 9:55 AM, John Abd-El-Malek <
> jam@chromium.org>
> >>>>> >> wrote:
> >>>>> >>>
> >>>>> >>>
> >>>>> >>>
> >>>>> >>> On Wed, Aug 8, 2012 at 9:48 AM, Jói Sigurðsson <joi@chromium.org
> >
> >>>>> >>> wrote:
> >>>>> >>>>
> >>>>> >>>> It's poorly named.  InfoBarTabHelper is actually where you go
> to add
> >>>>> >>>> an infobar, remove an infobar, and replace an infobar.
> >>>>> >>>
> >>>>> >>>
> >>>>> >>> Right, that's why I suggested maybe adding an interface here that
> >>>>> >>> allows
> >>>>> >>> one to do this.
> >>>>> >>>
> >>>>> >>>>
> >>>>> >>>> Note also that in the Browser Components project, we decided we
> would
> >>>>> >>>> take as many easy victories as we can first, and then refine
> the API
> >>>>> >>>> as we go along.  For this reason, we are allowing non-abstract
> >>>>> >>>> classes
> >>>>> >>>> in the api directory as long as they do not introduce header
> >>>>> >>>> dependencies on chrome/browser outside the api directory.  This
> is a
> >>>>> >>>> for-now kind of thing; later we will introduce tools to let us
> write
> >>>>> >>>> rules about link-time dependencies and will minimize those just
> as we
> >>>>> >>>> are doing for header dependencies, and this may force
> introduction of
> >>>>> >>>> abstract bases in some cases.  For this class in particular,
> there
> >>>>> >>>> are
> >>>>> >>>> probably a few methods that could be moved out of the class that
> >>>>> >>>> lives
> >>>>> >>>> in the api directory and into a subclass, e.g. the count of
> infobars
> >>>>> >>>> and the way to enumerate delegates.  I should probably add a
> TODO to
> >>>>> >>>> minimize the surface here.
> >>>>> >>>
> >>>>> >>>
> >>>>> >>> I don't think this header should be in a separate directory from
> its
> >>>>> >>> cc
> >>>>> >>> file. It's not just an api, it's also an implementation detail of
> >>>>> >>> tabcontents. in the api directory, there could be a new
> interface that
> >>>>> >>> has
> >>>>> >>> the add/remove infobar interface. until that interface is made, a
> >>>>> >>> temporary
> >>>>> >>> DEPS exceptions for code to include infobar_tab_helper.h from
> >>>>> >>> component
> >>>>> >>> directories could be allowed.
> >>>>> >>>
> >>>>> >>>>
> >>>>> >>>> See some more discussion in the chrome/browser/api/DEPS file in
> this
> >>>>> >>>> change.
> >>>>> >>>>
> >>>>> >>>> Cheers,
> >>>>> >>>> Jói
> >>>>> >>>>
> >>>>> >>>>
> >>>>> >>>>
> >>>>> >>>> On Wed, Aug 8, 2012 at 4:29 PM,  <jam@chromium.org> wrote:
> >>>>> >>>> > drive-by (didn't look in detail, this caught my attention)
> >>>>> >>>> >
> >>>>> >>>> >
> >>>>> >>>> >
> >>>>> >>>> >
> >>>>> >>>> >
>
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
> >>>>> >>>> > File chrome/browser/api/infobars/infobar_tab_helper.h (right):
> >>>>> >>>> >
> >>>>> >>>> >
> >>>>> >>>> >
> >>>>> >>>> >
>
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
> >>>>> >>>> > chrome/browser/api/infobars/infobar_tab_helper.h:16: class
> >>>>> >>>> > InfoBarTabHelper : public content::WebContentsObserver,
> >>>>> >>>> > i'm confused, this doesn't seem like an api of infobars that's
> >>>>> >>>> > provided
> >>>>> >>>> > by the browser. i realize some stuff in here is needed by
> code that
> >>>>> >>>> > puts
> >>>>> >>>> > up an infobar, so maybe a new interface should be added here.
> but
> >>>>> >>>> > this
> >>>>> >>>> > header seems like it should stay in chrome with the
> >>>>> >>>> > infobar_tab_helper.cc file, which is an internal
> implementation
> >>>>> >>>> > detail
> >>>>> >>>> > of TabContents.
> >>>>> >>>> >
> >>>>> >>>> >
> >>>>> >>>> > http://codereview.chromium.org/10843071/
> >>>>> >>>> >
> >>>>> >>>> > --
> >>>>> >>>> > You received this message because you are subscribed to the
> Google
> >>>>> >>>> > Groups
> >>>>> >>>> > "browser-components-watch" group.
> >>>>> >>>> > To post to this group, send email to
> >>>>> >>>> > browser-components-watch@chromium.org.
> >>>>> >>>> > To unsubscribe from this group, send email to
> >>>>> >>>> > browser-components-watch+unsubscribe@chromium.org.
> >>>>> >>>> > For more options, visit
> >>>>> >>>> > https://groups.google.com/a/chromium.org/groups/opt_out.
> >>>>> >>>> >
> >>>>> >>>> >
> >>>>> >>>
> >>>>> >>>
> >>>>> >>
> >>>>> >> --
> >>>>> >> You received this message because you are subscribed to the Google
> >>>>> >> Groups
> >>>>> >> "browser-components-watch" group.
> >>>>> >> To post to this group, send email to
> >>>>> >> browser-components-watch@chromium.org.
> >>>>> >> To unsubscribe from this group, send email to
> >>>>> >> browser-components-watch+unsubscribe@chromium.org.
> >>>>> >> For more options, visit
> >>>>> >> https://groups.google.com/a/chromium.org/groups/opt_out.
> >>>>> >>
> >>>>> >>
> >>>>
> >>>>
> >>>> --
> >>>> You received this message because you are subscribed to the Google
> Groups
> >>>> "browser-components-watch" group.
> >>>> To post to this group, send email to
> browser-components-watch@chromium.org.
> >>>> To unsubscribe from this group, send email to
> >>>> browser-components-watch+unsubscribe@chromium.org.
> >>>> For more options, visit
> >>>> https://groups.google.com/a/chromium.org/groups/opt_out.
> >>>>
> >>>>
>

[Jói, 2012-08-10 11:29:26]

>> John:  What do you think in general of the move of a couple of
>> InfobarDelegate subclasses
>
>
> I thought the InfobarDelegates would all move to the api directory?

Confirmed, they will all be in the chrome/browser/api/infobars
directory.  I meant the move of a couple from tab_contents to infobars
(InsecureContentInfoBarDelegate, LinkInfoBarDelegate,
SimpleAlertInfoBarDelegate) whereas at least the base InfoBarDelegate
was already under chrome/browser/infobars.

>> and the InfoBarTabHelper to
>> chrome/browser/infobars?
>
>
> that one is already there?

Sorry, my bad, I thought it had started under tab_contents.

> I think since this is still something that hangs off TabContents, it should
> stay being a TabHelper to be consistent with all the other objects that hang
> off it (Avi has felt strongly about that convention, see the documentation
> in tab_contents.h).

Will loop Avi into the review once I've updated the patch.

Cheers,
Jói


On Thu, Aug 9, 2012 at 6:37 PM, John Abd-El-Malek <jam@chromium.org> wrote:
>
>
> On Thu, Aug 9, 2012 at 10:39 AM, Jói Sigurðsson <joi@chromium.org> wrote:
>>
>> John:  What do you think in general of the move of a couple of
>> InfobarDelegate subclasses
>
>
> I thought the InfobarDelegates would all move to the api directory?
>
>>
>> and the InfoBarTabHelper to
>> chrome/browser/infobars?
>
>
> that one is already there?
>
>>  To me it is a more natural location since
>> none of those things have any dependency on TabContents and are fairly
>> coupled with the other infobar code.
>>
>> For the API extracted out of InfoBarTabHelper, what do you think of
>> InfoBarService (with Add/Remove/ReplaceInfobar methods)?
>
>
> hmm. I'll defer to other folks who spend more time in src/chrome than me :)
>
>>
>>  I guess if
>> we call it that, we should also rename InfoBarTabHelper to
>> InfoBarServiceImpl.  WDYT?
>
>
> I think since this is still something that hangs off TabContents, it should
> stay being a TabHelper to be consistent with all the other objects that hang
> off it (Avi has felt strongly about that convention, see the documentation
> in tab_contents.h).
>>
>>
>> Cheers,
>> Jói
>>
>>
>> On Thu, Aug 9, 2012 at 5:35 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>> > Thanks to Ben, John and Erik for their time discussing this with me
>> > just now.  We had a good discussion and I became convinced that it is
>> > a good policy to extract abstract base classes as we move things to
>> > the api directory rather than deferring this extraction in some cases.
>> >  The only exception to this will be similar to content, i.e. very
>> > simple classes whose .cc files can live with them in the api directory
>> > (I think the InfobarDelegate classes in the current change are
>> > examples of this) and classes that are just POD may be able to move to
>> > the api directory without modification.
>> >
>> > The rationale goes something like this:
>> >
>> > a) Most classes will need to have an abstract API extracted at some
>> > point anyway to break link-time dependencies and to enable unit
>> > testing through mocks;
>> >
>> > b) The only classes where we might have been able to save time by
>> > initially moving their definition wholesale to the api directory
>> > without first extracting an abstract base are going to be very simple,
>> > and thus extraction is likely trivial.  Larger more complex classes
>> > almost always have way too large of a public interface surface to be
>> > moved wholesale, and thus you can't realize the savings for these
>> > anyway;
>> >
>> > c) The approach we are following and the current status of the work is
>> > likely to be more easily understood by the layprogrammer if we follow
>> > a consistent rule of extracting an abstract base up front rather than
>> > doing it later.  For example, given the content-like approach, if you
>> > look at a class in the api directory and it is non-abstract, you know
>> > it will stay non-abstract.  In the previous proposal, you could have
>> > looked at such a class and been unsure whether it would stay like that
>> > or eventually have an abstract base extracted.
>> >
>> > Hope that makes sense.  I will work on updating the change and
>> > reuploading soon.
>> >
>> > Cheers,
>> > Jói
>> >
>> >
>> > On Thu, Aug 9, 2012 at 1:39 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>> >> I just booked something for 10am in MTV, 1pm in MON, 5pm here.  Let me
>> >> know if it doesn't work, my time today/tonight is flexible.
>> >>
>> >> Cheers,
>> >> Jói
>> >>
>> >>
>> >> On Thu, Aug 9, 2012 at 1:32 PM, Jói Sigurðsson <joi@chromium.org>
>> >> wrote:
>> >>> Hi John,
>> >>>
>> >>> Yep, a VC chat sounds like a good idea.
>> >>>
>> >>> Let me try to answer some of your points quickly so that we know each
>> >>> others' position a bit better before we talk live.
>> >>>
>> >>>> For preventing of more usage of chrome headers that need to be
>> >>>> wrapped with
>> >>>> APIs, this can be done through deps [...]
>> >>>
>> >>> Yes, it can be done via a temporarily-allowed include rule in DEPS,
>> >>> but this is less valuable than actually moving the header to the api
>> >>> directory.  For one thing, a temporarily-allowed rule in one component
>> >>> does not document that it is actually OK for other components to
>> >>> #include the header, because it is in the process of being made into
>> >>> an API.
>> >>>
>> >>>> Classes like InfobarTabHelper are not public APIs of chrome. That's
>> >>>> an
>> >>>> internal implementation detail.
>> >>>
>> >>> I maintain that we can move faster and achieve a large percentage of
>> >>> our goals in a short period of time if we allow concrete class
>> >>> definitions as API.  Over time, I think we will reach a very similar
>> >>> end state to what you are proposing; almost everything in API will
>> >>> either be an abstract base, except for a few simple utility or POD
>> >>> classes.  My point is that the faster we can rationalize the header
>> >>> dependencies, even though we know we will later have to introduce some
>> >>> abstractness etc. to do the same for link-time dependencies, the less
>> >>> painful the upstreaming process will be for the projects that are
>> >>> upstreaming, and the less chance new changes get introduced in the
>> >>> code base that do things that will later need to be unwound.
>> >>>
>> >>> So my point of view is, in the interest of making the biggest quick
>> >>> wins we can, we could allow a class like InfoBarTabHelper, that does
>> >>> not #include anything not suitable for the api directory, as an API
>> >>> for now, although possibly we should reduce its public interface to
>> >>> just the Add/Remove/ReplaceInfobar methods.  I don't think the stuff
>> >>> in the private section of the class leaks any implementation details
>> >>> (it does for human readers, but not for code using the class, which is
>> >>> the important thing).  We would then make it abstract as the concrete
>> >>> need to do so surfaces.
>> >>>
>> >>>> The way I view it, if browser components are
>> >>>> separate from chrome, then they should talk to it through a thin
>> >>>> layer.
>> >>>
>> >>> Agreed.  Such a thin layer does not need to be completely pure virtual
>> >>> though, at least not to achieve many of our initial goals quickly.
>> >>> The public section of a class is its interface, and in some cases we
>> >>> can simply move the class definitions to the api directory as they
>> >>> have a relatively clean public interface surface.  Later, we are
>> >>> fairly likely to make the class abstract e.g. to allow easier testing
>> >>> or to break link-time dependencies, but we can take the quick wins
>> >>> across the breadth of the code base sooner if we allow ourselves to
>> >>> defer this work.
>> >>>
>> >>>> Aside from this, classes like InfobarTabHelper need to have the
>> >>>> header and
>> >>>> implementation file in the same directory; that's just convention
>> >>>> from all
>> >>>> the chrome code. Keeping them in different directories is confusing.
>> >>>
>> >>> This is a convention I think we should allow breaking in the interest
>> >>> of expediency.  I don't think it's very confusing; in particular I
>> >>> don't think it adds any confusion above the confusion you would have
>> >>> if you have an API header in the api directory, and then a concrete
>> >>> header in the "regular" directory (alongside the .cc file) that does
>> >>> more or less nothing except define a class FooImpl : public Foo {}
>> >>> with an empty class body.  In the end you have to find the real
>> >>> definition of the class's interface anyway.
>> >>>
>> >>>> Also, having a component like autofill talk to InfobarTabHelper,
>> >>>> instead of an
>> >>>> interface, makes testing harder. It seems that an attractive part of
>> >>>> splitting up components is that they can be tested more independently
>> >>>> and
>> >>>> with less setup of other unnecessary classes. Having interfaces like
>> >>>> this
>> >>>> would make that easier.
>> >>>
>> >>> I agree in principle.  However:
>> >>> a) Improving the tests for components is part of our plan, but it is
>> >>> not one of the first things we should focus on; there are bigger wins
>> >>> to achieve earlier than that; and
>> >>> b) We don't know for sure at this moment that every class in the API
>> >>> will need to be pure virtual to enable testing; if we defer the
>> >>> decision we may save some work.
>> >>>
>> >>> Therefore, I think it makes sense to defer making classes abstract in
>> >>> some cases.
>> >>>
>> >>> Note that I'm not suggesting that every class being used in a
>> >>> sort-of-API-like way today should just be moved wholesale to the api
>> >>> directory.  In many cases we definitely do need to extract the subset
>> >>> of the class's public interface that truly is an API rather than an
>> >>> implementation detail into a base interface.  I just want us to have
>> >>> the freedom to defer that work when the class's public interface is
>> >>> close enough to being suitable as an API.
>> >>>
>> >>> I'll try to find a time today for us to talk.
>> >>>
>> >>> Cheers,
>> >>> Jói
>> >>>
>> >>>
>> >>>
>> >>> On Thu, Aug 9, 2012 at 12:02 AM, John Abd-El-Malek <jam@chromium.org>
>> >>> wrote:
>> >>>> hey Joi, I'll attempt to answer your main points. Probably might be
>> >>>> worth
>> >>>> having a vc to chat about this some more?
>> >>>>
>> >>>> For preventing of more usage of chrome headers that need to be
>> >>>> wrapped with
>> >>>> APIs, this can be done through deps. i.e. a component like autofill
>> >>>> might
>> >>>> have a restrictive DEPS that doesn't allow chrome, but only
>> >>>> chrome/browser/api and a temporary allowance for
>> >>>> chrome/browser/infobars/infobar_tab_helper.h. With your improvements
>> >>>> to
>> >>>> checkdeps, we can ensure that no more includes are added.
>> >>>>
>> >>>> Classes like InfobarTabHelper are not public APIs of chrome. That's
>> >>>> an
>> >>>> internal implementation detail. The way I view it, if browser
>> >>>> components are
>> >>>> separate from chrome, then they should talk to it through a thin
>> >>>> layer.
>> >>>> Aside from this, classes like InfobarTabHelper need to have the
>> >>>> header and
>> >>>> implementation file in the same directory; that's just convention
>> >>>> from all
>> >>>> the chrome code. Keeping them in different directories is confusing.
>> >>>> Also,
>> >>>> having a component like autofill talk to InfobarTabHelper, instead of
>> >>>> an
>> >>>> interface, makes testing harder. It seems that an attractive part of
>> >>>> splitting up components is that they can be tested more independently
>> >>>> and
>> >>>> with less setup of other unnecessary classes. Having interfaces like
>> >>>> this
>> >>>> would make that easier.
>> >>>>
>> >>>> While I agree that adding more interfaces is sometimes harder to
>> >>>> read, I
>> >>>> think in a codebase the size of chrome, they do make things easier
>> >>>> when used
>> >>>> to separate large features. In comparing this to the Content API
>> >>>> work, I
>> >>>> agree that adding interfaces around each component (i.e. autofill)
>> >>>> will be a
>> >>>> lot of work and may not be necessary. But adding interfaces for how
>> >>>> these
>> >>>> components talk out is much less effort, since it usually is a small
>> >>>> delegate-style interface. This is also what provides the most
>> >>>> bang-for-the-buck benefit in terms of encapsulation and testability.
>> >>>>
>> >>>>
>> >>>> On Wed, Aug 8, 2012 at 4:19 PM, Jói Sigurðsson <joi@chromium.org>
>> >>>> wrote:
>> >>>>>
>> >>>>> [+browser-components-dev]
>> >>>>>
>> >>>>> I'm back, so now let me try to explain my thoughts on this.  I'm not
>> >>>>> going to try to summarize, because I think it's important that the
>> >>>>> stakeholders in this read through my reasoning, but I will try to
>> >>>>> keep
>> >>>>> this structured.
>> >>>>>
>> >>>>> I'd like to leave the specifics of the current change alone for now,
>> >>>>> and focus on the general principles we're operating on.  We can
>> >>>>> discuss the specifics of the current change once we settle on the
>> >>>>> principles.  I tried to make a similar case to the one I'm making
>> >>>>> below while we were doing the content/ work, but I don't think I
>> >>>>> explained it well enough or fought for it hard enough, plus the
>> >>>>> situation there was somewhat different.
>> >>>>>
>> >>>>> I think the issues below are important to discuss, because the
>> >>>>> choice
>> >>>>> we make can have a large impact on how quickly we can move, in
>> >>>>> particular how much progress we can make in a reasonably short
>> >>>>> period
>> >>>>> of time, and quick progress in this area is important since it can
>> >>>>> prevent changes from creeping into the codebase that otherwise will
>> >>>>> take time to fix up later.
>> >>>>>
>> >>>>>
>> >>>>> Ben and John, I think both of you are stating a preference that all
>> >>>>> or
>> >>>>> at least most classes in the api directory should be abstract or
>> >>>>> very
>> >>>>> close to abstract, and likely extracted from existing classes
>> >>>>> similar
>> >>>>> to what we did for content/ (e.g. a class previously known as Foo
>> >>>>> might be split into an abstract content::Foo interface and a
>> >>>>> concrete
>> >>>>> content::FooImpl implementation).  Pardon me if that's not the exact
>> >>>>> position you're representing; hopefully the following is still a
>> >>>>> worth
>> >>>>> while discussion.
>> >>>>>
>> >>>>> To state my position, I don't think we should extract abstract bases
>> >>>>> for everything we place in the api directory.  I do think that we
>> >>>>> will end up extracting a lot of abstract bases before we are done,
>> >>>>> but
>> >>>>> I think we should do it only to the extent that it becomes
>> >>>>> practically
>> >>>>> necessary to reach our goals.  I don't think it should be something
>> >>>>> we
>> >>>>> do by default, or something we do right off the bat, before seeing
>> >>>>> that there is a concrete need.
>> >>>>>
>> >>>>> I'm working from the following theories about our desires for
>> >>>>> Browser
>> >>>>> Components:
>> >>>>>
>> >>>>> a) Our explicit goals could be described as follows: Rationalizing
>> >>>>> and
>> >>>>> simplifying the dependency graph, both in terms of header
>> >>>>> dependencies
>> >>>>> and in terms of link-time dependencies, with the goal of making it
>> >>>>> possible to turn off certain components, and to allow injection (or
>> >>>>> replacement by other means) of their currently-provided-by-Chrome
>> >>>>> dependencies so that components can be reused in new contexts and
>> >>>>> more
>> >>>>> easily unit tested separately.
>> >>>>>
>> >>>>> b) It is not a goal in and of itself to make all API surfaces into
>> >>>>> abstract base classes.  (If it is, then I need to understand the
>> >>>>> rationale
>> >>>>> for that; the use of abstract base classes seems like something that
>> >>>>> should be a means to some end rather than a goal in itself).
>> >>>>>
>> >>>>> c) We want to minimize the time it takes to reach the entirety of
>> >>>>> our
>> >>>>> goals, and at the same time we want to maximize the percentage of
>> >>>>> our
>> >>>>> goals that we can achieve in a reasonably short timeframe (say one
>> >>>>> or
>> >>>>> two quarters), i.e. an asymptotic curve towards our goal is more
>> >>>>> desirable
>> >>>>> than a linear or an exponential one.
>> >>>>>
>> >>>>> d) We want to help make the upstreaming of projects such as Chrome
>> >>>>> for
>> >>>>> Android and the Android WebView successful, and such upstreaming is
>> >>>>> happening in the very near term or has already started.
>> >>>>>
>> >>>>> Corollaries of the above seem to be that we should defer work that
>> >>>>> we
>> >>>>> are unsure will be necessary, and we should take any iterative quick
>> >>>>> wins
>> >>>>> that we can across the breadth of code, rather than polishing things
>> >>>>> fully on deep but narrow segments of code.  Finally, if we are
>> >>>>> deferring work that we don't know whether or not will be necessary,
>> >>>>> we
>> >>>>> should have ways to discover as early as possible that it is going
>> >>>>> to
>> >>>>> be necessary.
>> >>>>>
>> >>>>> Based on my work with John on the content/ refactoring, I am also
>> >>>>> working from the theory that extracting an abstract base class is
>> >>>>> much
>> >>>>> costlier in development time than not doing so.  This is especially
>> >>>>> true when you get into class hierarchies.
>> >>>>>
>> >>>>> Finally, I believe that adding the level of indirection that an
>> >>>>> abstract base brings can make the code slightly harder for humans to
>> >>>>> grok, and that even though each virtual method call is insignificant
>> >>>>> in cost, we should not be adding unnecessary virtual method calls
>> >>>>> (i.e. where no runtime polymorphism is necessary).  So I think there
>> >>>>> is a maintenance and performance burden in addition to the cost of
>> >>>>> added up-front development time, small or even tiny per instance but
>> >>>>> significant when added up.
>> >>>>>
>> >>>>> Based on all of those theories, this is roughly the plan I propose
>> >>>>> that we follow:
>> >>>>>
>> >>>>> 1) Start by allowing headers containing concrete class definitions
>> >>>>> in
>> >>>>> the api directory, as long as they don't bring in non-API headers.
>> >>>>> This lets us get more quick wins earlier, since we get a more
>> >>>>> rational
>> >>>>> header dependency structure sooner, and fewer temporarily-allowed
>> >>>>> rules in DEPS files.
>> >>>>>
>> >>>>> 2) Defer the work of extracting an abstract base until practical
>> >>>>> reasons show that it is actually necessary.  Oftentimes, we may end
>> >>>>> up
>> >>>>> not actually needing runtime polymorphism, we may just need
>> >>>>> compile-time specialization in which case e.g. a concrete API class
>> >>>>> definition, possibly with a pImpl pattern, is fine.
>> >>>>>
>> >>>>> 3) Introduce a link-time or target-level dependency analysis tool
>> >>>>> similar in spirit to the checkdeps tool we have for headers.  This
>> >>>>> tool would let us write what are essentially aspirational link-time
>> >>>>> dependency rules (with strict allow/disallow rules, and
>> >>>>> temporarily-allowed exceptions).  Such a tool would help us find
>> >>>>> cases
>> >>>>> where our current structure will be insufficient where e.g. abstract
>> >>>>> bases
>> >>>>> plus factories exposed across module boundaries may actually needed
>> >>>>> to break link-time dependencies.  (Credit to Erik for the idea of
>> >>>>> such a
>> >>>>> tool, which I think is very good).
>> >>>>>
>> >>>>> 4) Do iterative work as Browser Components actually start being used
>> >>>>> as components, as we start introducing stand-alone test harnesses
>> >>>>> for
>> >>>>> components, etc., and as part of this iterative work, introduce
>> >>>>> abstract
>> >>>>> bases when they show themselves to be necessary in practice, and do
>> >>>>> compile-time specialization when appropriate, until we have achieved
>> >>>>> our goals fully.
>> >>>>>
>> >>>>> One last note on the suitability of concrete class definitions as
>> >>>>> APIs: A header file containing a class with a publicf API and a
>> >>>>> private section containing implementation details does not actually
>> >>>>> leak implementation details to code, only to humans.  A private
>> >>>>> section
>> >>>>> is a means of information hiding in the OO programming sense; it is
>> >>>>> just an unfortunate coincidence of C++'s design that those hidden
>> >>>>> implementation details show up as human-readable in the header file.
>> >>>>> For code consuming the header file, those details are still
>> >>>>> invisible.
>> >>>>>
>> >>>>> Thanks for reading this far.  I look forward to hearing what you
>> >>>>> think.
>> >>>>>
>> >>>>> Cheers,
>> >>>>> Jói
>> >>>>>
>> >>>>>
>> >>>>> On Wed, Aug 8, 2012 at 6:37 PM, Jói Sigurðsson <joi@google.com>
>> >>>>> wrote:
>> >>>>> > Ben and John, thanks for the feedback. I have some thoughts on
>> >>>>> > this
>> >>>>> > subject
>> >>>>> > but am about to meet some friends for dinner. Will try to respond
>> >>>>> > later
>> >>>>> > tonight.
>> >>>>> >
>> >>>>> > Cheers,
>> >>>>> > Jói
>> >>>>> >
>> >>>>> > On Aug 8, 2012 5:58 PM, "Ben Goodger (Google)" <ben@chromium.org>
>> >>>>> > wrote:
>> >>>>> >>
>> >>>>> >> +1. The api dir should contain an abstract interface that the
>> >>>>> >> components
>> >>>>> >> use to talk to the browser.
>> >>>>> >>
>> >>>>> >>
>> >>>>> >> On Wed, Aug 8, 2012 at 9:55 AM, John Abd-El-Malek
>> >>>>> >> <jam@chromium.org>
>> >>>>> >> wrote:
>> >>>>> >>>
>> >>>>> >>>
>> >>>>> >>>
>> >>>>> >>> On Wed, Aug 8, 2012 at 9:48 AM, Jói Sigurðsson
>> >>>>> >>> <joi@chromium.org>
>> >>>>> >>> wrote:
>> >>>>> >>>>
>> >>>>> >>>> It's poorly named.  InfoBarTabHelper is actually where you go
>> >>>>> >>>> to add
>> >>>>> >>>> an infobar, remove an infobar, and replace an infobar.
>> >>>>> >>>
>> >>>>> >>>
>> >>>>> >>> Right, that's why I suggested maybe adding an interface here
>> >>>>> >>> that
>> >>>>> >>> allows
>> >>>>> >>> one to do this.
>> >>>>> >>>
>> >>>>> >>>>
>> >>>>> >>>> Note also that in the Browser Components project, we decided we
>> >>>>> >>>> would
>> >>>>> >>>> take as many easy victories as we can first, and then refine
>> >>>>> >>>> the API
>> >>>>> >>>> as we go along.  For this reason, we are allowing non-abstract
>> >>>>> >>>> classes
>> >>>>> >>>> in the api directory as long as they do not introduce header
>> >>>>> >>>> dependencies on chrome/browser outside the api directory.  This
>> >>>>> >>>> is a
>> >>>>> >>>> for-now kind of thing; later we will introduce tools to let us
>> >>>>> >>>> write
>> >>>>> >>>> rules about link-time dependencies and will minimize those just
>> >>>>> >>>> as we
>> >>>>> >>>> are doing for header dependencies, and this may force
>> >>>>> >>>> introduction of
>> >>>>> >>>> abstract bases in some cases.  For this class in particular,
>> >>>>> >>>> there
>> >>>>> >>>> are
>> >>>>> >>>> probably a few methods that could be moved out of the class
>> >>>>> >>>> that
>> >>>>> >>>> lives
>> >>>>> >>>> in the api directory and into a subclass, e.g. the count of
>> >>>>> >>>> infobars
>> >>>>> >>>> and the way to enumerate delegates.  I should probably add a
>> >>>>> >>>> TODO to
>> >>>>> >>>> minimize the surface here.
>> >>>>> >>>
>> >>>>> >>>
>> >>>>> >>> I don't think this header should be in a separate directory from
>> >>>>> >>> its
>> >>>>> >>> cc
>> >>>>> >>> file. It's not just an api, it's also an implementation detail
>> >>>>> >>> of
>> >>>>> >>> tabcontents. in the api directory, there could be a new
>> >>>>> >>> interface that
>> >>>>> >>> has
>> >>>>> >>> the add/remove infobar interface. until that interface is made,
>> >>>>> >>> a
>> >>>>> >>> temporary
>> >>>>> >>> DEPS exceptions for code to include infobar_tab_helper.h from
>> >>>>> >>> component
>> >>>>> >>> directories could be allowed.
>> >>>>> >>>
>> >>>>> >>>>
>> >>>>> >>>> See some more discussion in the chrome/browser/api/DEPS file in
>> >>>>> >>>> this
>> >>>>> >>>> change.
>> >>>>> >>>>
>> >>>>> >>>> Cheers,
>> >>>>> >>>> Jói
>> >>>>> >>>>
>> >>>>> >>>>
>> >>>>> >>>>
>> >>>>> >>>> On Wed, Aug 8, 2012 at 4:29 PM,  <jam@chromium.org> wrote:
>> >>>>> >>>> > drive-by (didn't look in detail, this caught my attention)
>> >>>>> >>>> >
>> >>>>> >>>> >
>> >>>>> >>>> >
>> >>>>> >>>> >
>> >>>>> >>>> >
>> >>>>> >>>> >
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>> >>>>> >>>> > File chrome/browser/api/infobars/infobar_tab_helper.h
>> >>>>> >>>> > (right):
>> >>>>> >>>> >
>> >>>>> >>>> >
>> >>>>> >>>> >
>> >>>>> >>>> >
>> >>>>> >>>> >
http://codereview.chromium.org/10843071/diff/2003/chrome/browser/api/infobars...
>> >>>>> >>>> > chrome/browser/api/infobars/infobar_tab_helper.h:16: class
>> >>>>> >>>> > InfoBarTabHelper : public content::WebContentsObserver,
>> >>>>> >>>> > i'm confused, this doesn't seem like an api of infobars
>> >>>>> >>>> > that's
>> >>>>> >>>> > provided
>> >>>>> >>>> > by the browser. i realize some stuff in here is needed by
>> >>>>> >>>> > code that
>> >>>>> >>>> > puts
>> >>>>> >>>> > up an infobar, so maybe a new interface should be added here.
>> >>>>> >>>> > but
>> >>>>> >>>> > this
>> >>>>> >>>> > header seems like it should stay in chrome with the
>> >>>>> >>>> > infobar_tab_helper.cc file, which is an internal
>> >>>>> >>>> > implementation
>> >>>>> >>>> > detail
>> >>>>> >>>> > of TabContents.
>> >>>>> >>>> >
>> >>>>> >>>> >
>> >>>>> >>>> > http://codereview.chromium.org/10843071/
>> >>>>> >>>> >
>> >>>>> >>>> > --
>> >>>>> >>>> > You received this message because you are subscribed to the
>> >>>>> >>>> > Google
>> >>>>> >>>> > Groups
>> >>>>> >>>> > "browser-components-watch" group.
>> >>>>> >>>> > To post to this group, send email to
>> >>>>> >>>> > browser-components-watch@chromium.org.
>> >>>>> >>>> > To unsubscribe from this group, send email to
>> >>>>> >>>> > browser-components-watch+unsubscribe@chromium.org.
>> >>>>> >>>> > For more options, visit
>> >>>>> >>>> > https://groups.google.com/a/chromium.org/groups/opt_out.
>> >>>>> >>>> >
>> >>>>> >>>> >
>> >>>>> >>>
>> >>>>> >>>
>> >>>>> >>
>> >>>>> >> --
>> >>>>> >> You received this message because you are subscribed to the
>> >>>>> >> Google
>> >>>>> >> Groups
>> >>>>> >> "browser-components-watch" group.
>> >>>>> >> To post to this group, send email to
>> >>>>> >> browser-components-watch@chromium.org.
>> >>>>> >> To unsubscribe from this group, send email to
>> >>>>> >> browser-components-watch+unsubscribe@chromium.org.
>> >>>>> >> For more options, visit
>> >>>>> >> https://groups.google.com/a/chromium.org/groups/opt_out.
>> >>>>> >>
>> >>>>> >>
>> >>>>
>> >>>>
>> >>>> --
>> >>>> You received this message because you are subscribed to the Google
>> >>>> Groups
>> >>>> "browser-components-watch" group.
>> >>>> To post to this group, send email to
>> >>>> browser-components-watch@chromium.org.
>> >>>> To unsubscribe from this group, send email to
>> >>>> browser-components-watch+unsubscribe@chromium.org.
>> >>>> For more options, visit
>> >>>> https://groups.google.com/a/chromium.org/groups/opt_out.
>> >>>>
>> >>>>
>
>

[Jói, 2012-08-10 12:05:26]

PTAL, updated per discussion.

erikwright & jam: Main reviewers since you've previously reviewed this change.

pkasting: Optional reviewer. Also, per tfarina@'s suggestion, do you want me to
add an OWNERS file to the chrome/browser/infobars and
chrome/browser/infobars/OWNERS directories, with you in it?

In this patch:
- .h and .cc files for the infobar delegates go together in the api directory.
- I'm leaving infobar_tab_helper.h and .cc alone for now, will look at that
dependency in a separate change. There are a couple of TODOs from pkasting@ that
I will discuss with him related to the next change, but they are outside of
scope for this change.

Cheers,
Jói

[jam, 2012-08-10 16:13:30]

http://codereview.chromium.org/10843071/diff/14001/chrome/browser/infobars/in...
File chrome/browser/infobars/insecure_content_infobar_delegate.h (right):

http://codereview.chromium.org/10843071/diff/14001/chrome/browser/infobars/in...
chrome/browser/infobars/insecure_content_infobar_delegate.h:5: #ifndef
CHROME_BROWSER_INFOBARS_INSECURE_CONTENT_INFOBAR_DELEGATE_H_
it seems that all the infobar delegates should be in the same directory? I
understand they may not be used by the one component (autofill) now, but it's a
little confusing if they're split up in the meantime?

[Peter Kasting, 2012-08-10 17:14:42]

Is there actually anything substantive for me to review?  If it's all mechanical
I don't need to look.

Adding me in relevant OWNERS files seems like a good idea.

[erikwright (departed), 2012-08-10 19:11:02]

I agree with John that it seems sensible to move all of the infobar delegates at
this time.

http://codereview.chromium.org/10843071/diff/14001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/10843071/diff/14001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:40: #include
"chrome/browser/gpu_util.h"
??

[joi, 2012-08-13 10:29:05]

> Is there actually anything substantive for me to review?  If it's all
> mechanical
> I don't need to look.

The only non-mechanical bit is whether you agree to consolidate the
InfoBarDelegate subclasses under chrome/browser/api/infobars, whereas
before, a few of them were under chrome/browser/tab_contents (but had
no dependency on stuff under tab_contents).

> Adding me in relevant OWNERS files seems like a good idea.

Will do in the next patch I upload.

Cheers,
Jói


On Fri, Aug 10, 2012 at 5:14 PM,  <pkasting@chromium.org> wrote:
> Is there actually anything substantive for me to review?  If it's all
> mechanical
> I don't need to look.
>
> Adding me in relevant OWNERS files seems like a good idea.
>
> http://codereview.chromium.org/10843071/

[Jói, 2012-08-13 11:47:54]

PTAL.  Added OWNERS files for infobars/, moved SimpleAlertInfoBarDelegate but
not InsecureContentInfoBarDelegate, see details below.

Cheers,
Jói

http://codereview.chromium.org/10843071/diff/14001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/10843071/diff/14001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:40: #include
"chrome/browser/gpu_util.h"
On 2012/08/10 19:11:02, erikwright wrote:
> ??

Bad merge resolution, thanks for noticing.  Fixed now.

http://codereview.chromium.org/10843071/diff/14001/chrome/browser/infobars/in...
File chrome/browser/infobars/insecure_content_infobar_delegate.h (right):

http://codereview.chromium.org/10843071/diff/14001/chrome/browser/infobars/in...
chrome/browser/infobars/insecure_content_infobar_delegate.h:5: #ifndef
CHROME_BROWSER_INFOBARS_INSECURE_CONTENT_INFOBAR_DELEGATE_H_
On 2012/08/10 16:13:30, John Abd-El-Malek wrote:
> it seems that all the infobar delegates should be in the same directory? I
> understand they may not be used by the one component (autofill) now, but it's
a
> little confusing if they're split up in the meantime?

Agreed for SimpleAlertInfoBarDelegate.  InsecureContentInfoBarDelegate on the
other hand is an implementation detail of InfoBarTabHelper and not used anywhere
outside of it, so I don't think it should move to the api directory.  Also, it
has additional chrome/browser dependencies that would then have to be broken.

[jam, 2012-08-13 15:55:43]

lgtm, thanks

[Peter Kasting, 2012-08-13 17:36:50]

I don't have a problem moving all infobars to live somewhere else.

I'm a bit unsure whether "browser/api/infobars" makes sense, though.  I guess
I'm not clear on what the "api" part of this means.  Infobars don't seem like
they're part of any kind of "API" I could describe.

http://codereview.chromium.org/10843071/diff/14001/chrome/browser/infobars/in...
File chrome/browser/infobars/insecure_content_infobar_delegate.h (right):

http://codereview.chromium.org/10843071/diff/14001/chrome/browser/infobars/in...
chrome/browser/infobars/insecure_content_infobar_delegate.h:5: #ifndef
CHROME_BROWSER_INFOBARS_INSECURE_CONTENT_INFOBAR_DELEGATE_H_
On 2012/08/13 11:47:54, Jói wrote:
> On 2012/08/10 16:13:30, John Abd-El-Malek wrote:
> > it seems that all the infobar delegates should be in the same directory? I
> > understand they may not be used by the one component (autofill) now, but
it's
> a
> > little confusing if they're split up in the meantime?
> 
> Agreed for SimpleAlertInfoBarDelegate.  InsecureContentInfoBarDelegate on the
> other hand is an implementation detail of InfoBarTabHelper and not used
anywhere
> outside of it, so I don't think it should move to the api directory.  Also, it
> has additional chrome/browser dependencies that would then have to be broken.

InsecureContentInfoBarDelegate should not actually be handled specially by
InfoBarTabHelper.  All the message stuff relating to this in InfoBarTabHelper
should really live elsewhere as its placement in InfoBarTabHelper is really
incidental.  Then this infobar delegate can move to be with the others.

[joi, 2012-08-13 19:45:15]

Thanks Peter.

The 'api' directory is the set of stuff that Browser Components will
be allowed to depend on.  Delegate interfaces like the InfoBarDelegate
and its subclasses are likely to be pretty typical since existing
services will often require the client to implement a delegate.
Details at
http://www.chromium.org/developers/design-documents/browser-components.

Noted about the InsecureContentInfoBarDelegate.  It's outside the
scope of this change to clean up the location of where the message
gets handled but I'll see if we can get to it anyway, e.g. as a
starter task for somebody.

Cheers,
Jói



On Mon, Aug 13, 2012 at 5:36 PM,  <pkasting@chromium.org> wrote:
> I don't have a problem moving all infobars to live somewhere else.
>
> I'm a bit unsure whether "browser/api/infobars" makes sense, though.  I
> guess
> I'm not clear on what the "api" part of this means.  Infobars don't seem
> like
> they're part of any kind of "API" I could describe.
>
>
>
>
http://codereview.chromium.org/10843071/diff/14001/chrome/browser/infobars/in...
> File chrome/browser/infobars/insecure_content_infobar_delegate.h
> (right):
>
>
http://codereview.chromium.org/10843071/diff/14001/chrome/browser/infobars/in...
> chrome/browser/infobars/insecure_content_infobar_delegate.h:5: #ifndef
> CHROME_BROWSER_INFOBARS_INSECURE_CONTENT_INFOBAR_DELEGATE_H_
> On 2012/08/13 11:47:54, Jói wrote:
>>
>> On 2012/08/10 16:13:30, John Abd-El-Malek wrote:
>> > it seems that all the infobar delegates should be in the same
>
> directory? I
>>
>> > understand they may not be used by the one component (autofill) now,
>
> but it's
>>
>> a
>> > little confusing if they're split up in the meantime?
>
>
>> Agreed for SimpleAlertInfoBarDelegate.  InsecureContentInfoBarDelegate
>
> on the
>>
>> other hand is an implementation detail of InfoBarTabHelper and not
>
> used anywhere
>>
>> outside of it, so I don't think it should move to the api directory.
>
> Also, it
>>
>> has additional chrome/browser dependencies that would then have to be
>
> broken.
>
> InsecureContentInfoBarDelegate should not actually be handled specially
> by InfoBarTabHelper.  All the message stuff relating to this in
> InfoBarTabHelper should really live elsewhere as its placement in
> InfoBarTabHelper is really incidental.  Then this infobar delegate can
> move to be with the others.
>
> http://codereview.chromium.org/10843071/

[Peter Kasting, 2012-08-14 01:58:20]

On Mon, Aug 13, 2012 at 12:44 PM, Jói Sigurðsson <joi@google.com> wrote:

> The 'api' directory is the set of stuff that Browser Components will
> be allowed to depend on.  Delegate interfaces like the InfoBarDelegate
> and its subclasses are likely to be pretty typical since existing
> services will often require the client to implement a delegate.
> Details at
> http://www.chromium.org/developers/design-documents/browser-components.


That sounds sort of like the base infobar delegates -- InfoBarDelegate,
ConfirmInfoBarDelegate, maybe SimpleAlertInfoBarDelegate -- should live in
this directory, while the other infobar delegates should move to live
alongside the specific features that use them.  (For example, anything
geolocation-related should be in chrome/browser/geolocation).  I don't
think we should expose all of the feature-specific infobars as things that
other people should be depending on.

PK

[joi, 2012-08-14 11:36:28]

Right, the ones being moved are InfoBarDelegate,
ConfirmInfoBarDelegate, LinkInfoBarDelegate, and
SimpleAlertInfoBarDelegate.  The feature-specific ones will stay with
their respective features.

Cheers,
Jói


On Tue, Aug 14, 2012 at 1:58 AM, Peter Kasting <pkasting@chromium.org> wrote:
> On Mon, Aug 13, 2012 at 12:44 PM, Jói Sigurðsson <joi@google.com> wrote:
>>
>> The 'api' directory is the set of stuff that Browser Components will
>> be allowed to depend on.  Delegate interfaces like the InfoBarDelegate
>> and its subclasses are likely to be pretty typical since existing
>> services will often require the client to implement a delegate.
>> Details at
>> http://www.chromium.org/developers/design-documents/browser-components.
>
>
> That sounds sort of like the base infobar delegates -- InfoBarDelegate,
> ConfirmInfoBarDelegate, maybe SimpleAlertInfoBarDelegate -- should live in
> this directory, while the other infobar delegates should move to live
> alongside the specific features that use them.  (For example, anything
> geolocation-related should be in chrome/browser/geolocation).  I don't think
> we should expose all of the feature-specific infobars as things that other
> people should be depending on.
>
> PK

[Peter Kasting, 2012-08-14 17:42:48]

On Tue, Aug 14, 2012 at 4:35 AM, Jói Sigurðsson <joi@google.com> wrote:

> Right, the ones being moved are InfoBarDelegate,
> ConfirmInfoBarDelegate, LinkInfoBarDelegate, and
> SimpleAlertInfoBarDelegate.  The feature-specific ones will stay with
> their respective features.


Moving those four is fine.

PK