Move infobar constants and the code that uses them from components/ to chrome/.

Move infobar constants and the code that uses them from components/ to chrome/.

Both the infobar and the infobar container were making use of various infobar
constants that were declared in components/ but (usually) defined in chrome/.
This was a symptom of having put code in the component that really probably
belonged in the embedder.

This CL hoists the relevant logic to a new cross-platform
InfoBarContainerDelegate class defined on the chrome side, which implements some
of the methods of InfoBarContainer::Delegate.  Platforms then subclass this to
implement the remaining, platform-specific bits.

This means that the constants in question now live solely in chrome/, avoiding a
bunch of link-time nastiness when trying to build the components code
separately.

BUG=382924
TEST=none
TBR=finnur

Committed: https://crrev.com/07622896891ccc5e8279be55bae52c6323f025ff
Cr-Commit-Position: refs/heads/master@{#308046}

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

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

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

sdefresne: Review
dfalcantara: What should I do for Android?  For the moment, this change doesn't
include c/b/ui/infobar_container_delegate.* in the Android source list, so if
Android compiles and links the code in infobar.cc and infobar_container.cc, it
will need its own implementations of these methods.  I wasn't sure if it did
compile and link these files, though, since infobar_android.cc claimed the
constants there weren't actually used.

[gone, 2014-12-10 23:18:45]

Compiled and ran it locally, navigated to http://google.fr to get an Infobar to
pop up.  Crashed as the infobar appeared though :/

1199 signal 11 (SIGSEGV), code 1, fault addr 0x0 in tid 12268
(oid.apps.chrome)^M
1200 pid: 12268, tid: 12268, name: oid.apps.chrome  >>>
com.google.android.apps.chrome <<<^M
1201 signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0^M
1202      r0 00000000  r1 00000000  r2 a10a3e2d  r3 3fc9dadc
1203      r4 97296818  r5 972967a0  r6 be857df8  r7 9768af28
1204      r8 00000000  r9 be857ee0  sl 00000000  fp 000150e8
1205      ip 00000000  sp be857db0  lr 9f869e93  pc a10a3c7a
1206 
1207 Stack Trace:
1208   RELADDR   FUNCTION                                                       
                         FILE:LINE
1209   02177c7a  infobars::InfoBar::RecalculateHeights(bool)                    
                        
/usr/local/google/ssd2/clankium/src/out/Debug/../../components/infobars/core/infobar.cc:138
1210   0093de91  gfx::LinearAnimation::Step(base::TimeTicks)                    
                        
/usr/local/google/ssd2/clankium/src/out/Debug/../../ui/gfx/animation/linear_animation.cc:84
1211   0093dc53  gfx::AnimationContainer::Run()                                 
                        
/usr/local/google/ssd2/clankium/src/out/Debug/../../ui/gfx/animation/animation_container.cc:77
1212   v------>  base::Callback<void ()>::Run() const                           
                        
/usr/local/google/ssd2/clankium/src/out/Debug/../../base/callback.h:396
1213   0032f833  base::Timer::RunScheduledTask()                                
                        
/usr/local/google/ssd2/clankium/src/out/Debug/../../base/timer/timer.cc:211
1214   v------>  base::Callback<void ()>::Run() const                           
                        
/usr/local/google/ssd2/clankium/src/out/Debug/../../base/callback.h:396
1215   00652661  base::debug::TaskAnnotator::RunTask(char const*, char const*,
base::PendingTask const&) 
/usr/local/google/ssd2/clankium/src/out/Debug/../../base/debug/task_annotator.cc:63
1216   0038b0a3  base::MessageLoop::RunTask(base::PendingTask const&)           
                        
/usr/local/google/ssd2/clankium/src/out/Debug/../../base/message_loop/message_loop.cc:448
1217   0038afb1  base::MessageLoop::DeferOrRunPendingTask(base::PendingTask
const&)                      
/usr/local/google/ssd2/clankium/src/out/Debug/../../base/message_loop/message_loop.cc:458
1218   0038ae0f  base::MessageLoop::DoDelayedWork(base::TimeTicks*)             
                        
/usr/local/google/ssd2/clankium/src/out/Debug/../../base/message_loop/message_loop.cc:605
1219   0025f0d9  DoRunLoopOnce(_JNIEnv*, _jobject*, long long, long long)       
                        
/usr/local/google/ssd2/clankium/src/out/Debug/../../base/message_loop/message_pump_android.cc:60
1220   002c13a7  <unknown>                                                      
                        
/data/dalvik-cache/arm/data@app@com.google.android.apps.chrome-1@base.apk@cla...

[Peter Kasting, 2014-12-10 23:49:44]

On 2014/12/10 23:18:45, dfalcantara wrote:
> Compiled and ran it locally, navigated to http://google.fr to get an Infobar
to
> pop up.  Crashed as the infobar appeared though :/
> 
> 1199 signal 11 (SIGSEGV), code 1, fault addr 0x0 in tid 12268
> (oid.apps.chrome)^M
> 1200 pid: 12268, tid: 12268, name: oid.apps.chrome  >>>
> com.google.android.apps.chrome <<<^M
> 1201 signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0^M
> 1202      r0 00000000  r1 00000000  r2 a10a3e2d  r3 3fc9dadc
> 1203      r4 97296818  r5 972967a0  r6 be857df8  r7 9768af28
> 1204      r8 00000000  r9 be857ee0  sl 00000000  fp 000150e8
> 1205      ip 00000000  sp be857db0  lr 9f869e93  pc a10a3c7a
> 1206 
> 1207 Stack Trace:
> 1208   RELADDR   FUNCTION                                                     
 
>                          FILE:LINE
> 1209   02177c7a  infobars::InfoBar::RecalculateHeights(bool)                  
 
>                         
>
/usr/local/google/ssd2/clankium/src/out/Debug/../../components/infobars/core/infobar.cc:138

Huh.  Not sure how the infobar is appearing and then the container is losing its
delegate.  Whatever, I needed to explicitly check for a NULL delegate anyway. 
Should be fixed in the next patch set.

However, that stack means you are, in fact, calling this code, which means
Android needs an implementation of the new delegate functions.  I thought that
including the new .cc file in the "non Android sources" list meant that it
wouldn't be linked in for Android (which should mean you should get link
errors)?

[gone, 2014-12-11 00:00:07]

dfalcantara@chromium.org changed reviewers:
+ cjhopman@chromium.org

[gone, 2014-12-11 00:00:07]

+cjhopman as potential gyp guru

[gone, 2014-12-11 00:08:02]

On 2014/12/11 00:00:07, dfalcantara wrote:
> +cjhopman as potential gyp guru

AFAICT, those files aren't compiled in.  Forced a compile error after moving the
source files out of that section in the GYP file.

[cjhopman, 2014-12-11 00:08:54]

On 2014/12/10 23:49:44, Peter Kasting wrote:
> On 2014/12/10 23:18:45, dfalcantara wrote:
> > Compiled and ran it locally, navigated to http://google.fr to get an Infobar
> to
> > pop up.  Crashed as the infobar appeared though :/
> > 
> > 1199 signal 11 (SIGSEGV), code 1, fault addr 0x0 in tid 12268
> > (oid.apps.chrome)^M
> > 1200 pid: 12268, tid: 12268, name: oid.apps.chrome  >>>
> > com.google.android.apps.chrome <<<^M
> > 1201 signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0^M
> > 1202      r0 00000000  r1 00000000  r2 a10a3e2d  r3 3fc9dadc
> > 1203      r4 97296818  r5 972967a0  r6 be857df8  r7 9768af28
> > 1204      r8 00000000  r9 be857ee0  sl 00000000  fp 000150e8
> > 1205      ip 00000000  sp be857db0  lr 9f869e93  pc a10a3c7a
> > 1206 
> > 1207 Stack Trace:
> > 1208   RELADDR   FUNCTION                                                   
 
>  
> >                          FILE:LINE
> > 1209   02177c7a  infobars::InfoBar::RecalculateHeights(bool)                
 
>  
> >                         
> >
>
/usr/local/google/ssd2/clankium/src/out/Debug/../../components/infobars/core/infobar.cc:138
> 
> Huh.  Not sure how the infobar is appearing and then the container is losing
its
> delegate.  Whatever, I needed to explicitly check for a NULL delegate anyway. 
> Should be fixed in the next patch set.
> 
> However, that stack means you are, in fact, calling this code, which means
> Android needs an implementation of the new delegate functions.  I thought that
> including the new .cc file in the "non Android sources" list meant that it
> wouldn't be linked in for Android (which should mean you should get link
> errors)?

It would only get compile errors if Android used an InfoBarContainer delegate.
If the delegate is always null then it would compile+link fine.

[Peter Kasting, 2014-12-11 00:12:54]

On 2014/12/11 00:08:54, cjhopman wrote:
> On 2014/12/10 23:49:44, Peter Kasting wrote:
> > On 2014/12/10 23:18:45, dfalcantara wrote:
> > > Compiled and ran it locally, navigated to http://google.fr to get an
Infobar
> > to
> > > pop up.  Crashed as the infobar appeared though :/
> > > 
> > > 1199 signal 11 (SIGSEGV), code 1, fault addr 0x0 in tid 12268
> > > (oid.apps.chrome)^M
> > > 1200 pid: 12268, tid: 12268, name: oid.apps.chrome  >>>
> > > com.google.android.apps.chrome <<<^M
> > > 1201 signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0^M
> > > 1202      r0 00000000  r1 00000000  r2 a10a3e2d  r3 3fc9dadc
> > > 1203      r4 97296818  r5 972967a0  r6 be857df8  r7 9768af28
> > > 1204      r8 00000000  r9 be857ee0  sl 00000000  fp 000150e8
> > > 1205      ip 00000000  sp be857db0  lr 9f869e93  pc a10a3c7a
> > > 1206 
> > > 1207 Stack Trace:
> > > 1208   RELADDR   FUNCTION                                                 
 
>  
> >  
> > >                          FILE:LINE
> > > 1209   02177c7a  infobars::InfoBar::RecalculateHeights(bool)              
 
>  
> >  
> > >                         
> > >
> >
>
/usr/local/google/ssd2/clankium/src/out/Debug/../../components/infobars/core/infobar.cc:138
> > 
> > Huh.  Not sure how the infobar is appearing and then the container is losing
> its
> > delegate.  Whatever, I needed to explicitly check for a NULL delegate
anyway. 
> > Should be fixed in the next patch set.
> > 
> > However, that stack means you are, in fact, calling this code, which means
> > Android needs an implementation of the new delegate functions.  I thought
that
> > including the new .cc file in the "non Android sources" list meant that it
> > wouldn't be linked in for Android (which should mean you should get link
> > errors)?
> 
> It would only get compile errors if Android used an InfoBarContainer delegate.
> If the delegate is always null then it would compile+link fine.

Does Android use InfoBar and InfoBarContainer but not
InfoBarContainer::Delegate?

Really, the core of my question has always been intended to be: "I don't know
how the Android infobar system works.  Based on what I'm trying to do in this
patch, what do I need to do to avoid breaking Android, either by breaking the
build or by simply breaking the infobar layout functionality?"

[gone, 2014-12-11 00:23:47]

On 2014/12/11 00:12:54, Peter Kasting wrote:
> On 2014/12/11 00:08:54, cjhopman wrote:
> > On 2014/12/10 23:49:44, Peter Kasting wrote:
> > > On 2014/12/10 23:18:45, dfalcantara wrote:
> > > > Compiled and ran it locally, navigated to http://google.fr to get an
> Infobar
> > > to
> > > > pop up.  Crashed as the infobar appeared though :/
> > > > 
> > > > 1199 signal 11 (SIGSEGV), code 1, fault addr 0x0 in tid 12268
> > > > (oid.apps.chrome)^M
> > > > 1200 pid: 12268, tid: 12268, name: oid.apps.chrome  >>>
> > > > com.google.android.apps.chrome <<<^M
> > > > 1201 signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0^M
> > > > 1202      r0 00000000  r1 00000000  r2 a10a3e2d  r3 3fc9dadc
> > > > 1203      r4 97296818  r5 972967a0  r6 be857df8  r7 9768af28
> > > > 1204      r8 00000000  r9 be857ee0  sl 00000000  fp 000150e8
> > > > 1205      ip 00000000  sp be857db0  lr 9f869e93  pc a10a3c7a
> > > > 1206 
> > > > 1207 Stack Trace:
> > > > 1208   RELADDR   FUNCTION                                               
 
>  
> >  
> > >  
> > > >                          FILE:LINE
> > > > 1209   02177c7a  infobars::InfoBar::RecalculateHeights(bool)            
 
>  
> >  
> > >  
> > > >                         
> > > >
> > >
> >
>
/usr/local/google/ssd2/clankium/src/out/Debug/../../components/infobars/core/infobar.cc:138
> > > 
> > > Huh.  Not sure how the infobar is appearing and then the container is
losing
> > its
> > > delegate.  Whatever, I needed to explicitly check for a NULL delegate
> anyway. 
> > > Should be fixed in the next patch set.
> > > 
> > > However, that stack means you are, in fact, calling this code, which means
> > > Android needs an implementation of the new delegate functions.  I thought
> that
> > > including the new .cc file in the "non Android sources" list meant that it
> > > wouldn't be linked in for Android (which should mean you should get link
> > > errors)?
> > 
> > It would only get compile errors if Android used an InfoBarContainer
delegate.
> > If the delegate is always null then it would compile+link fine.
> 
> Does Android use InfoBar and InfoBarContainer but not
> InfoBarContainer::Delegate?
> 
> Really, the core of my question has always been intended to be: "I don't know
> how the Android infobar system works.  Based on what I'm trying to do in this
> patch, what do I need to do to avoid breaking Android, either by breaking the
> build or by simply breaking the infobar layout functionality?"

I'm fairly certain we don't use the InfoBarContainer::Delegate because of the
differences in how Android's UI is set up (we don't have an overlapping UI
arrow, for example).  InfoBarContainerAndroid's constructor seems to support
this, because we pass in NULL for the delegate to use in the InfoBarContainer.

With your null pointer check, things seem to work fine on http://google.fr's
infobars.

[cjhopman, 2014-12-11 00:28:27]

On 2014/12/11 00:12:54, Peter Kasting wrote:
> On 2014/12/11 00:08:54, cjhopman wrote:
> > On 2014/12/10 23:49:44, Peter Kasting wrote:
> > > On 2014/12/10 23:18:45, dfalcantara wrote:
> > > > Compiled and ran it locally, navigated to http://google.fr to get an
> Infobar
> > > to
> > > > pop up.  Crashed as the infobar appeared though :/
> > > > 
> > > > 1199 signal 11 (SIGSEGV), code 1, fault addr 0x0 in tid 12268
> > > > (oid.apps.chrome)^M
> > > > 1200 pid: 12268, tid: 12268, name: oid.apps.chrome  >>>
> > > > com.google.android.apps.chrome <<<^M
> > > > 1201 signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0^M
> > > > 1202      r0 00000000  r1 00000000  r2 a10a3e2d  r3 3fc9dadc
> > > > 1203      r4 97296818  r5 972967a0  r6 be857df8  r7 9768af28
> > > > 1204      r8 00000000  r9 be857ee0  sl 00000000  fp 000150e8
> > > > 1205      ip 00000000  sp be857db0  lr 9f869e93  pc a10a3c7a
> > > > 1206 
> > > > 1207 Stack Trace:
> > > > 1208   RELADDR   FUNCTION                                               
 
>  
> >  
> > >  
> > > >                          FILE:LINE
> > > > 1209   02177c7a  infobars::InfoBar::RecalculateHeights(bool)            
 
>  
> >  
> > >  
> > > >                         
> > > >
> > >
> >
>
/usr/local/google/ssd2/clankium/src/out/Debug/../../components/infobars/core/infobar.cc:138
> > > 
> > > Huh.  Not sure how the infobar is appearing and then the container is
losing
> > its
> > > delegate.  Whatever, I needed to explicitly check for a NULL delegate
> anyway. 
> > > Should be fixed in the next patch set.
> > > 
> > > However, that stack means you are, in fact, calling this code, which means
> > > Android needs an implementation of the new delegate functions.  I thought
> that
> > > including the new .cc file in the "non Android sources" list meant that it
> > > wouldn't be linked in for Android (which should mean you should get link
> > > errors)?
> > 
> > It would only get compile errors if Android used an InfoBarContainer
delegate.
> > If the delegate is always null then it would compile+link fine.
> 
> Does Android use InfoBar and InfoBarContainer but not
> InfoBarContainer::Delegate?

I expect that is the case (and the source of the original confusion). Looking a
little bit into it, I don't see Android creating a delegate anywhere
(particularly see
chrome/browser/ui/android/infobars/infobar_container_android.cc).

> 
> Really, the core of my question has always been intended to be: "I don't know
> how the Android infobar system works.  Based on what I'm trying to do in this
> patch, what do I need to do to avoid breaking Android, either by breaking the
> build or by simply breaking the infobar layout functionality?"

I'm sure Dan can answer this better, but handling a null delegate will probably
just make it work on Android. It looks like the things being added to the
delegate are things that Android doesn't really care about (based on the removed
comment in infobar_android.cc).

[Peter Kasting, 2014-12-11 00:46:07]

On 2014/12/11 00:23:47, dfalcantara wrote:
> With your null pointer check, things seem to work fine on http://google.fr
> infobars.

I imagine this means whatever code you use to draw infobars doesn't look at
accessors like InfoBar::arrow_height() and total_height().  Regardless of
whether Android animates infobars, it does its height calculations in
Android-specific drawing code that ignores these model elements.

If so, then this patch will work as-is, but it also gives you the hooks you
would need if you wanted to make Android's control flow more like other
platforms, by implementing an InfoBarContainer::Delegate and having the
newly-added functions set/return appropriate values (which may just be
constants), then having your drawing code read the relevant values off the
InfoBar.  Whether this is a benefit is up to you.

[gone, 2014-12-11 01:04:23]

I think one of the reasons we went with handling it all through Java was that
the JNI calls could make animations choppier than the UX folks would accept. 
Given that they've also completely changed how infobars are supposed on Android
for Material Design (they all come from the bottom now and don't overlap the UI
at all), I'm not sure if there'll be convergence any time soon :/

[sdefresne, 2014-12-11 10:33:48]

lgtm

https://codereview.chromium.org/793783003/diff/40001/chrome/browser/ui/infoba...
File chrome/browser/ui/infobar_container_delegate.cc (right):

https://codereview.chromium.org/793783003/diff/40001/chrome/browser/ui/infoba...
chrome/browser/ui/infobar_container_delegate.cc:34: if
(!DrawInfoBarArrows(NULL))
nit: maybe uses nullptr instead of NULL in those new files?

[Peter Kasting, 2014-12-12 01:52:24]

Landing.

https://codereview.chromium.org/793783003/diff/40001/chrome/browser/ui/infoba...
File chrome/browser/ui/infobar_container_delegate.cc (right):

https://codereview.chromium.org/793783003/diff/40001/chrome/browser/ui/infoba...
chrome/browser/ui/infobar_container_delegate.cc:34: if
(!DrawInfoBarArrows(NULL))
On 2014/12/11 10:33:48, sdefresne wrote:
> nit: maybe uses nullptr instead of NULL in those new files?

Ah, yeah, I should have fixed that instead of copy-and-pasting.  Done.

[Peter Kasting, 2014-12-12 01:52:29]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-12-12 01:53:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/793783003/60001

[commit-bot: I haz the power, 2014-12-12 02:02:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-12 02:02:40]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Peter Kasting, 2014-12-12 02:15:56]

pkasting@chromium.org changed reviewers:
+ finnur@chromium.org

[Peter Kasting, 2014-12-12 02:15:57]

TBR=finnur for mechanical changes in
chrome/browser/extensions/extension_infobar_delegate.cc.

[Peter Kasting, 2014-12-12 02:16:14]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-12-12 02:17:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/793783003/60001

[commit-bot: I haz the power, 2014-12-12 03:34:31]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-12 03:35:16]

Patchset 4 (id:??) landed as
https://crrev.com/07622896891ccc5e8279be55bae52c6323f025ff
Cr-Commit-Position: refs/heads/master@{#308046}

[Lei Zhang, 2014-12-12 10:11:33]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2014-12-12 10:11:34]

https://codereview.chromium.org/793783003/diff/60001/chrome/browser/ui/infoba...
File chrome/browser/ui/infobar_container_delegate.cc (right):

https://codereview.chromium.org/793783003/diff/60001/chrome/browser/ui/infoba...
chrome/browser/ui/infobar_container_delegate.cc:11: kDefaultArrowTargetHeight;
This creates a static initializer and the sizes step is failing on Linux / Mac.
See noms' post: http://meowni.ca/posts/static-initializers/

Sample failure:
http://build.chromium.org/p/chromium/builders/Linux/builds/56162/steps/sizes/...
(compare vs build 56161)

Fix: https://codereview.chromium.org/801753002

[Finnur, 2014-12-12 15:08:02]

extension_infobar_delegate.cc LGTM