Remove dependency from infobars component to the embedder

Remove dependency from infobars component to the embedder

The infobars::InfoBar::kSeparatorLineHeight, ... constants were declared
in the infobars component but only defined in the embedder. Due to that,
the component was not self contained and every client had to contains
those symbols and ensure they were only defined once (or encounter linker
errors).

This change the constants to globals defined in infobars::InfoBar that
should be initialized by the embedder using infobars::InfoBar::Initialize.
Default values are provided in the component so that they can be used for
the unit tests or as default values by the embedder.

Added a call to infobars::Infobar::Initialize in chrome_browser_main.cc
and define constants in the Chrome embedder that have varying value in
function of the UI system used (views, cocoa, android, ios).

BUG=382924

[sdefresne, 2014-12-03 13:30:44]

sdefresne@chromium.org changed reviewers:
+ pkasting@chromium.org, sky@chromium.org

[sdefresne, 2014-12-03 13:30:44]

pkasting@chromium.org: please review change in
- chrome/browser/ui/infobar_constants.h
- chrome/browser/ui/*/infobars
- components/infobars

sky@chromium.org: please review changes in
- chrome/browser/chrome_browser_main.cc
- chrome/browser/extensions/extension_infobar_delegate.cc

Note that the goal of this CL is to make infobars component self contained while
still configurable by the embedder.

[sdefresne, 2014-12-03 13:31:40]

pritam.nikam@, blundell@, droger@: FYI

[Peter Kasting, 2014-12-03 13:49:37]

This solution doesn't feel quite right to me.

Looking at how the constants are used, they're basically only needed for
InfoBar::RecalculateHeights().  A couple are used in the constructor, but I bet
those members could just be initialized to zero, or, in the limit, could call an
embedder function to get the right starting heights.  (See below for more about
this kind of "embedder function".)

The real issue is that RecalculateHeights() is basically using embedder-specific
knowledge, i.e. the actual dimensions of the various graphical elements.  It
seems like perhaps a better fix is to make RecalculateHeights() ask the embedder
for the information it needs to know.

There are a range of variations on how this could be implemented.  The obvious
way would be to have RecalculateHeights() call some embedder method to fetch the
values of all these constants.  Another way which may be better would be to
hoist the calculations at the core of this function entirely to the embedder, so
RecalculateHeights() basically says, "hey, here's my animation value, what are
my height, arrow height, etc.".  This latter design would allow embedders to
vary the behavior of infobars more -- if, for example, we wanted a different
kind of animation on Android, it could override the default embedder
implementation with a completely different one.  Maybe we don't need that
flexibility, though :)

It's not 100% clear to me where, precisely, this function would live on the
embedder side.  We wouldn't want a copy of it in each platform, so it would need
to be in some platform-independent file.  Perhaps a static method in
InfoBarService?  That's kind of ugly, but I don't instantly have a better idea.

WDYT of all this?  Does it sound more appealing than the
globals-plus-Initialize() route here?  It seems more philosophically correct, to
me at least...

[sdefresne, 2014-12-03 15:53:01]

On 2014/12/03 13:49:37, Peter Kasting wrote:
> This solution doesn't feel quite right to me.
> 
> Looking at how the constants are used, they're basically only needed for
> InfoBar::RecalculateHeights().  A couple are used in the constructor, but I
bet
> those members could just be initialized to zero, or, in the limit, could call
an
> embedder function to get the right starting heights.  (See below for more
about
> this kind of "embedder function".)
> 
> The real issue is that RecalculateHeights() is basically using
embedder-specific
> knowledge, i.e. the actual dimensions of the various graphical elements.  It
> seems like perhaps a better fix is to make RecalculateHeights() ask the
embedder
> for the information it needs to know.
> 
> There are a range of variations on how this could be implemented.  The obvious
> way would be to have RecalculateHeights() call some embedder method to fetch
the
> values of all these constants.  Another way which may be better would be to
> hoist the calculations at the core of this function entirely to the embedder,
so
> RecalculateHeights() basically says, "hey, here's my animation value, what are
> my height, arrow height, etc.".  This latter design would allow embedders to
> vary the behavior of infobars more -- if, for example, we wanted a different
> kind of animation on Android, it could override the default embedder
> implementation with a completely different one.  Maybe we don't need that
> flexibility, though :)
> 
> It's not 100% clear to me where, precisely, this function would live on the
> embedder side.  We wouldn't want a copy of it in each platform, so it would
need
> to be in some platform-independent file.  Perhaps a static method in
> InfoBarService?  That's kind of ugly, but I don't instantly have a better
idea.
> 
> WDYT of all this?  Does it sound more appealing than the
> globals-plus-Initialize() route here?  It seems more philosophically correct,
to
> me at least...

That is one option I considered too, however passing the constants directly to
RecalculateHeight is not feasible since 1. this method is called from multiple
location and we end up patching almost all the method of InfoBar, 2. the
constants are also used in InfoBarContainer, 3. the constants are also used in
multiple other locations, including the embedder.

I have another CL https://codereview.chromium.org/765643003/ that let the
embedder export the constants through the InfoBarManager interface. I think it
is not really cleaner that the current CL using InfoBar::Initialize() especially
since it increase the life-time dependencies of the different piece (i.e. many
method of InfoBar and InfoBarContainer will now DCHECK if they don't have an
owner_/infobar_manager_ set).

I'll let you choose which CL you prefer.

[Peter Kasting, 2014-12-03 19:59:46]

On 2014/12/03 15:53:01, sdefresne wrote:
> On 2014/12/03 13:49:37, Peter Kasting wrote:
> > This solution doesn't feel quite right to me.
> > 
> > Looking at how the constants are used, they're basically only needed for
> > InfoBar::RecalculateHeights().  A couple are used in the constructor, but I
> bet
> > those members could just be initialized to zero, or, in the limit, could
call
> an
> > embedder function to get the right starting heights.  (See below for more
> about
> > this kind of "embedder function".)
> > 
> > The real issue is that RecalculateHeights() is basically using
> embedder-specific
> > knowledge, i.e. the actual dimensions of the various graphical elements.  It
> > seems like perhaps a better fix is to make RecalculateHeights() ask the
> embedder
> > for the information it needs to know.
> > 
> > There are a range of variations on how this could be implemented.  The
obvious
> > way would be to have RecalculateHeights() call some embedder method to fetch
> the
> > values of all these constants.  Another way which may be better would be to
> > hoist the calculations at the core of this function entirely to the
embedder,
> so
> > RecalculateHeights() basically says, "hey, here's my animation value, what
are
> > my height, arrow height, etc.".  This latter design would allow embedders to
> > vary the behavior of infobars more -- if, for example, we wanted a different
> > kind of animation on Android, it could override the default embedder
> > implementation with a completely different one.  Maybe we don't need that
> > flexibility, though :)
> > 
> > It's not 100% clear to me where, precisely, this function would live on the
> > embedder side.  We wouldn't want a copy of it in each platform, so it would
> need
> > to be in some platform-independent file.  Perhaps a static method in
> > InfoBarService?  That's kind of ugly, but I don't instantly have a better
> idea.
> > 
> > WDYT of all this?  Does it sound more appealing than the
> > globals-plus-Initialize() route here?  It seems more philosophically
correct,
> to
> > me at least...
> 
> That is one option I considered too, however passing the constants directly to
> RecalculateHeight is not feasible since 1. this method is called from multiple
> location and we end up patching almost all the method of InfoBar, 2. the
> constants are also used in InfoBarContainer, 3. the constants are also used in
> multiple other locations, including the embedder.

I hadn't intended to suggest passing the constants as arguments to
RecalculateHeights().  I assumed the constants would stay defined as they are in
the embedder (which takes care of places in the embedder that use them), and
that we would hoist functionality that uses the constants from the component to
the embedder.  I admit I didn't see the usages in InfoBarContainer.  These
complicate the picture some.  We'd basically need to also hoist much of
InfoBarContainer::ArrowTargetHeightForInfoBar(), perhaps to the
InfoBarContainer::Delegate since it's already making some logic decisions here. 
That would then probably become the logical place to put _anything_ we hoist,
which means we'd have InfoBar accessing its container's delegate.  And we'd
probably define a cross-platform implementation of part of the delegate on the
embedder side, then override for specific platforms as necessary.

This is somewhat hand-wavey.  I think perhaps the right way for me to determine
if this works is to attempt to write the patch, which I can try to get to in the
next day or two (I hope).  It may turn out this solution is horrible.

The underlying theme of my response should be that I think the core problem is
not that we have data on the embedder side that we need to figure out how to
convey to the component, but that we have functionality on the component side
that really belongs in the embedder.  But it could be I'm wrong; it depends on
philosophically why we're componentizing things and how we want to think of the
roles of embedder and component.

[sdefresne, 2014-12-08 17:14:46]

On 2014/12/03 at 19:59:46, pkasting wrote:
> On 2014/12/03 15:53:01, sdefresne wrote:
> > On 2014/12/03 13:49:37, Peter Kasting wrote:
> > > This solution doesn't feel quite right to me.
> > > 
> > > Looking at how the constants are used, they're basically only needed for
> > > InfoBar::RecalculateHeights().  A couple are used in the constructor, but
I
> > bet
> > > those members could just be initialized to zero, or, in the limit, could
call
> > an
> > > embedder function to get the right starting heights.  (See below for more
> > about
> > > this kind of "embedder function".)
> > > 
> > > The real issue is that RecalculateHeights() is basically using
> > embedder-specific
> > > knowledge, i.e. the actual dimensions of the various graphical elements. 
It
> > > seems like perhaps a better fix is to make RecalculateHeights() ask the
> > embedder
> > > for the information it needs to know.
> > > 
> > > There are a range of variations on how this could be implemented.  The
obvious
> > > way would be to have RecalculateHeights() call some embedder method to
fetch
> > the
> > > values of all these constants.  Another way which may be better would be
to
> > > hoist the calculations at the core of this function entirely to the
embedder,
> > so
> > > RecalculateHeights() basically says, "hey, here's my animation value, what
are
> > > my height, arrow height, etc.".  This latter design would allow embedders
to
> > > vary the behavior of infobars more -- if, for example, we wanted a
different
> > > kind of animation on Android, it could override the default embedder
> > > implementation with a completely different one.  Maybe we don't need that
> > > flexibility, though :)
> > > 
> > > It's not 100% clear to me where, precisely, this function would live on
the
> > > embedder side.  We wouldn't want a copy of it in each platform, so it
would
> > need
> > > to be in some platform-independent file.  Perhaps a static method in
> > > InfoBarService?  That's kind of ugly, but I don't instantly have a better
> > idea.
> > > 
> > > WDYT of all this?  Does it sound more appealing than the
> > > globals-plus-Initialize() route here?  It seems more philosophically
correct,
> > to
> > > me at least...
> > 
> > That is one option I considered too, however passing the constants directly
to
> > RecalculateHeight is not feasible since 1. this method is called from
multiple
> > location and we end up patching almost all the method of InfoBar, 2. the
> > constants are also used in InfoBarContainer, 3. the constants are also used
in
> > multiple other locations, including the embedder.
> 
> I hadn't intended to suggest passing the constants as arguments to
RecalculateHeights().  I assumed the constants would stay defined as they are in
the embedder (which takes care of places in the embedder that use them), and
that we would hoist functionality that uses the constants from the component to
the embedder.  I admit I didn't see the usages in InfoBarContainer.  These
complicate the picture some.  We'd basically need to also hoist much of
InfoBarContainer::ArrowTargetHeightForInfoBar(), perhaps to the
InfoBarContainer::Delegate since it's already making some logic decisions here. 
That would then probably become the logical place to put _anything_ we hoist,
which means we'd have InfoBar accessing its container's delegate.  And we'd
probably define a cross-platform implementation of part of the delegate on the
embedder side, then override for specific platforms as necessary.
> 
> This is somewhat hand-wavey.  I think perhaps the right way for me to
determine if this works is to attempt to write the patch, which I can try to get
to in the next day or two (I hope).  It may turn out this solution is horrible.
> 
> The underlying theme of my response should be that I think the core problem is
not that we have data on the embedder side that we need to figure out how to
convey to the component, but that we have functionality on the component side
that really belongs in the embedder.  But it could be I'm wrong; it depends on
philosophically why we're componentizing things and how we want to think of the
roles of embedder and component.

pkasting: did you find time to attempt a patch following your idea? I found that
this is blocking another of my CL (https://codereview.chromium.org/777863002).
Or would you like us to discuss this offline and then have me work on a new
patch?

[Peter Kasting, 2014-12-08 20:46:26]

On 2014/12/08 17:14:46, sdefresne wrote:
> pkasting: did you find time to attempt a patch following your idea? I found
that
> this is blocking another of my CL (https://codereview.chromium.org/777863002).
> Or would you like us to discuss this offline and then have me work on a new
> patch?

No, I haven't managed to get to this.  I still think my suggestion is the right
thing to do, I'm just flooded.

If you think what I described above makes sense and can be implemented, you're
welcome to try it, otherwise I really will try to get to this, I just can't make
promises about time :(

[Peter Kasting, 2014-12-10 22:11:10]

OK, I have an implementation of my proposal up at
https://codereview.chromium.org/793783003/ .

[sdefresne, 2014-12-11 10:34:52]

On 2014/12/10 at 22:11:10, pkasting wrote:
> OK, I have an implementation of my proposal up at
https://codereview.chromium.org/793783003/ .

Thank you, your implementation is much cleaner. Closing.