Implement app banner info bars on desktop.

Implement app banner info bars on desktop.

This CL extends the app banner implementation to work on desktop,
enabled with the flag --enable-add-to-shelf. With this flag active,
users will be prompted with an infobar to add a site to their shelf
once they have engaged with it to some substantive amount (subject to
heuristics currently implemented for the mobile add to homescreen
banner).

Note: the icon used is a placeholder to be updated later.

BUG=491001
Committed: https://crrev.com/4d15cab83cf46e1e5b90b30541911c108b20c8d6
Cr-Commit-Position: refs/heads/master@{#332988}

[dominickn (DO NOT USE), 2015-06-01 06:40:18]

dominickn@google.com changed reviewers:
+ benwells@chromium.org, dfalcantara@chromium.org

[gone, 2015-06-01 17:22:40]

On 2015/06/01 06:43:14, dominickn wrote:

You seem to have broken the Android tests.

[gone, 2015-06-01 17:27:47]

On 2015/06/01 17:22:40, dfalcantara wrote:
> On 2015/06/01 06:43:14, dominickn wrote:
> 
> You seem to have broken the Android tests.

I think this is the stack:
Stack Trace:
  RELADDR   FUNCTION                                                            
                                      FILE:LINE
  000392fc  <unknown>                                                           
                                      /system/lib/libdvm.so
  0003fbf7  <unknown>                                                           
                                      /system/lib/libdvm.so
  0024e767  base::MessagePumpForUI::MessagePumpForUI()+14                       
                                      libgcc2.c:?
  00459a81  ConfirmInfoBar::~ConfirmInfoBar()+28                                
                                      libgcc2.c:?
  002b9c49 
banners::AppBannerInfoBarDelegateAndroid::UpdateInstallState(_JNIEnv*,
_jobject*)+28                       libgcc2.c:?
  002b9dc1 
banners::AppBannerInfoBarDelegateAndroid::OnInstallIntentReturned(_JNIEnv*,
_jobject*, unsigned char)+136  libgcc2.c:?
  0001dbcc  dvmPlatformInvoke+112                                               
                                      /system/lib/libdvm.so
  0004e123  dvmCallJNIMethod(unsigned int const*, JValue*, Method const*,
Thread*)+398                                 /system/lib/libdvm.so
  00026fe0  <unknown>                                                           
                                      /system/lib/libdvm.so
  0002dfa0  dvmMterpStd(Thread*)+76                                             
                                      /system/lib/libdvm.so
  0002b638  dvmInterpret(Thread*, Method const*, JValue*)+184                   
                                      /system/lib/libdvm.so
  00060861  dvmInvokeMethod(Object*, Method const*, ArrayObject*, ArrayObject*,
ClassObject*, bool)+392                /system/lib/libdvm.so
  000687c3  <unknown>                                                           
                                      /system/lib/libdvm.so
  00026fe0  <unknown>                                                           
                                      /system/lib/libdvm.so
  0002dfa0  dvmMterpStd(Thread*)+76                                             
                                      /system/lib/libdvm.so
  0002b638  dvmInterpret(Thread*, Method const*, JValue*)+184                   
                                      /system/lib/libdvm.so
  0006057d  dvmCallMethodV(Thread*, Method const*, Object*, bool, JValue*,
std::__va_list)+336                         /system/lib/libdvm.so
  00049d0b  <unknown>                                                           
                                      /system/lib/libdvm.so
  0004d2b3  <unknown>                                                           
                                      /system/lib/libandroid_runtime.so
  0004dfd7  android::AndroidRuntime::start(char const*, char const*)+354        
                                      /system/lib/libandroid_runtime.so
  0000105b  <unknown>                                                           
                                      /system/bin/app_process
  0000e34b  __libc_init+50                                                      
                                      /system/lib/libc.so
  00000d7c  <unknown>                                                           
                                      /system/bin/app_process

[dominickn (DO NOT USE), 2015-06-02 03:53:18]

I refactored out the actual infobar creation from the base data fetcher, but
managed to misplace it instead of factoring it into the Android data fetcher.
The test issue is now resolved.

I'm intending to submit tests for the banner on Chrome OS in a different CL.

[benwells, 2015-06-02 05:53:36]

Woot! Can't wait to play with this....

https://codereview.chromium.org/1161233005/diff/20001/chrome/app/theme/theme_...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/app/theme/theme_...
chrome/app/theme/theme_resources.grd:402: <structure type="chrome_scaled_image"
name="IDR_INFOBAR_APP_BANNER" file="cros/kiosk_app_user_pod_icon.png" />
I didn't realise we didn't have an icon in common/ that we could use. We
shouldn't be using an icon from cros/ on all platforms - is there an icon under
common/ that is vaguely close that we can use instead?

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_data_fetcher_android.cc (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:73: return
infobar;
Is there any reason to return infobar now? It feels weird to pass ownership of
it on to InfoBarService and then return a raw pointer.

Also as this function is showing the banner, maybe it should be called
ShowBanner instead of just CreateBanner?

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_data_fetcher.h (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.h:27: }  // namespace extensions
The generic fetcher class is now polluted with desktop only stuff. Can we have a
app_banner_data_fetcher_desktop instead?

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_infobar_delegate.cc (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_infobar_delegate.cc:50: return
infobar_service->AddInfoBar(infobar_service->CreateConfirmInfoBar(
The way the banner is added is inconsistent between android and desktop. Here it
is done by infobar delegate, but for android it is done by the fetcher. I don't
have a strong opinion about which way it should be, but it should be consistent.

If you do it here, the function should probably be Show, not Create.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_infobar_delegate.cc:72: :
IDS_ADD_TO_SHELF_INFOBAR_NEVER_BUTTON);
Having it sohw 'Never' is problematic. The heuristics we have will show it again
(in six months or something like that).

We should raise this with UX (rolfe).

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_infobar_delegate.cc:85: if (web_contents) {
Does the data fetcher need to be canceled at this point so it isn't active?

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_infobar_delegate.h (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_infobar_delegate.h:10: #include
"base/memory/weak_ptr.h"
Is weak_ptr used?

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_infobar_delegate.h:14: #include "url/gurl.h"
Is gurl used?

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_infobar_delegate.h:16: class InfoBarService;
This doesn't appear to be used either.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_infobar_delegate.h:35: class
AppBannerInfoBarDelegate : public ConfirmInfoBarDelegate {
Can we make this AppBannerInfoBarDelegateDesktop (with matching .h / .cc)?

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_infobar_delegate.h:43: const
content::Manifest& web_app_data,
We should call this web_manifest. web_app_data makes me think it is a
WebApplicationData.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_infobar_delegate.h:58:
AppBannerInfoBarDelegate(scoped_refptr<AppBannerDataFetcher> fetcher,
Is there a reason this is protected, not private?

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:23: int kMinimumIconSize = 144;
Can we use the extension icon constant? At some point (i.e. you can log a TODO
for now) we should insist on whatever the set of icons is that will guarantee
the best user experience.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:110:
web_contents->GetMainFrame()->Send(
The role of this helper is getting a bit haphazard. I am OK with it also doing
metrics, as it is a natural central point that everything goes through, but it
shouldn't be sending the outcome to the renderer. That feels too far out of its
role of helping out with settings.

About the metrics, if this class is going to encapsulate the metrics, it would
be great to have it define the rappor metrics we use. That is, instead of
passing in rappor_metric, pass in an enum WEB or NATIVE (or PLAY or ANDROID) and
use the correct rappor metric based on that.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper.cc (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:498:
OnDidGetManifest(manifest);
We should have some dchecks to make sure the manifest has all the stuff we need,
as the web_app_data will be empty for this code path.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:577: void
BookmarkAppHelper::ShowBookmarkAppBubble() {
Is there a reason to split this out, or was it just for clarity?

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/ui/tab_h...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/ui/tab_h...
chrome/browser/ui/tab_helpers.cc:205: #if defined(OS_CHROMEOS)
This should be just !defined(OS_ANDROID) I think.

[dominickn (DO NOT USE), 2015-06-02 06:51:33]

https://codereview.chromium.org/1161233005/diff/20001/chrome/app/theme/theme_...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/app/theme/theme_...
chrome/app/theme/theme_resources.grd:402: <structure type="chrome_scaled_image"
name="IDR_INFOBAR_APP_BANNER" file="cros/kiosk_app_user_pod_icon.png" />
On 2015/06/02 05:53:35, benwells wrote:
> I didn't realise we didn't have an icon in common/ that we could use. We
> shouldn't be using an icon from cros/ on all platforms - is there an icon
under
> common/ that is vaguely close that we can use instead?

apps_bookmark_bar_icon.png is the closest, but it's colourful, rather than grey.
I've been in contact with rolfe regarding the icons; she has sent me some, but
they don't seem to be lining up correctly in the info bar so I'm following up on
that.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_data_fetcher_android.cc (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:73: return
infobar;
On 2015/06/02 05:53:35, benwells wrote:
> Is there any reason to return infobar now? It feels weird to pass ownership of
> it on to InfoBarService and then return a raw pointer.

Done. 

> Also as this function is showing the banner, maybe it should be called
> ShowBanner instead of just CreateBanner?

There is already a ShowBanner function which sends a message to the renderer
asking if it's okay to show the banner. I can rename that function to
RequestShowBanner.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_data_fetcher.h (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.h:27: }  // namespace extensions
On 2015/06/02 05:53:35, benwells wrote:
> The generic fetcher class is now polluted with desktop only stuff. Can we have
a
> app_banner_data_fetcher_desktop instead?

This also means either adding an AppBannerManagerDesktop, or making the existing
AppBannerManager create an AppBannerDataFetcherDesktop instead of the currently
AppBannerDataFetcher.

I didn't add an explicit desktop version because I conceptualised the Android
one as overriding the desktop implementation, rather than both inheriting from a
single source. I felt like the number of desktop-specific pollutions in the
fetcher class was limited enough to not justify an additional couple of classes.
Thoughts?

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_infobar_delegate.cc (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_infobar_delegate.cc:50: return
infobar_service->AddInfoBar(infobar_service->CreateConfirmInfoBar(
On 2015/06/02 05:53:35, benwells wrote:
> The way the banner is added is inconsistent between android and desktop. Here
it
> is done by infobar delegate, but for android it is done by the fetcher. I
don't
> have a strong opinion about which way it should be, but it should be
consistent.
> 
> If you do it here, the function should probably be Show, not Create.

I tried doing it consistently, and created an AppBannerInfoBar class for desktop
and called InfoBarService->AddInfoBar from the data fetcher. However, it crashed
with a segfault on ChromeOS in the views code, somewhere in determining the
widget required to display the banner.

The current inconsistency is a bit annoying, but there is a fair amount of
functionality in the AppBannerInfoBarAndroid class which communicates with Java,
whilst I found few instances of desktop infobars which were created in the same
way as the Android version. This is why I ended up implementing it like I did,
avoiding too much hacking on the Android side. Do you think it's worth further
work to make it fully consistent? This would probably require figuring out where
the Java-specific stuff in the AppBannerInfoBarAndroid should hook into (if it
can hook into somewhere else).

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_infobar_delegate.cc:85: if (web_contents) {
On 2015/06/02 05:53:35, benwells wrote:
> Does the data fetcher need to be canceled at this point so it isn't active?

Done.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_infobar_delegate.h (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_infobar_delegate.h:10: #include
"base/memory/weak_ptr.h"
On 2015/06/02 05:53:35, benwells wrote:
> Is weak_ptr used?

Done.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_infobar_delegate.h:14: #include "url/gurl.h"
On 2015/06/02 05:53:35, benwells wrote:
> Is gurl used?

Done.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_infobar_delegate.h:16: class InfoBarService;
On 2015/06/02 05:53:35, benwells wrote:
> This doesn't appear to be used either.

Done.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_infobar_delegate.h:35: class
AppBannerInfoBarDelegate : public ConfirmInfoBarDelegate {
On 2015/06/02 05:53:35, benwells wrote:
> Can we make this AppBannerInfoBarDelegateDesktop (with matching .h / .cc)?

I feel like this should be connected to the decision regarding a desktop app
banner data fetcher - if that adds a desktop version, then this should too.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_infobar_delegate.h:43: const
content::Manifest& web_app_data,
On 2015/06/02 05:53:35, benwells wrote:
> We should call this web_manifest. web_app_data makes me think it is a
> WebApplicationData.

Done.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_infobar_delegate.h:58:
AppBannerInfoBarDelegate(scoped_refptr<AppBannerDataFetcher> fetcher,
On 2015/06/02 05:53:35, benwells wrote:
> Is there a reason this is protected, not private?

It was in case the delegate needed to be further subclassed, e.g. for a mac
implementation.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:23: int kMinimumIconSize = 144;
On 2015/06/02 05:53:35, benwells wrote:
> Can we use the extension icon constant? At some point (i.e. you can log a TODO
> for now) we should insist on whatever the set of icons is that will guarantee
> the best user experience.

The extensions code isn't compiled on Android, so this will either need compile
guards or an AppBannerManagerDesktop.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:110:
web_contents->GetMainFrame()->Send(
On 2015/06/02 05:53:36, benwells wrote:
> The role of this helper is getting a bit haphazard. I am OK with it also doing
> metrics, as it is a natural central point that everything goes through, but it
> shouldn't be sending the outcome to the renderer. That feels too far out of
its
> role of helping out with settings.
> 
> About the metrics, if this class is going to encapsulate the metrics, it would
> be great to have it define the rappor metrics we use. That is, instead of
> passing in rappor_metric, pass in an enum WEB or NATIVE (or PLAY or ANDROID)
and
> use the correct rappor metric based on that.

In progress.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper.cc (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:498:
OnDidGetManifest(manifest);
On 2015/06/02 05:53:36, benwells wrote:
> We should have some dchecks to make sure the manifest has all the stuff we
need,
> as the web_app_data will be empty for this code path.

In progress.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:577: void
BookmarkAppHelper::ShowBookmarkAppBubble() {
On 2015/06/02 05:53:36, benwells wrote:
> Is there a reason to split this out, or was it just for clarity?

It was initially necessary, but is now just for clarity and can be factored back
in.

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/ui/tab_h...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/1161233005/diff/20001/chrome/browser/ui/tab_h...
chrome/browser/ui/tab_helpers.cc:205: #if defined(OS_CHROMEOS)
On 2015/06/02 05:53:36, benwells wrote:
> This should be just !defined(OS_ANDROID) I think.

Done.

[dominickn (DO NOT USE), 2015-06-02 13:50:39]

Update: desktop app banner implementation has now been factored out.

[gone, 2015-06-02 21:36:05]

> I didn't add an explicit desktop version because I conceptualised the Android
> one as overriding the desktop implementation, rather than both inheriting from
a
> single source.

Yeah, that's generally how the abstractions work for other features (hence why
we've
got files with _android suffixes everywhere).  This particular feature is a
little strange
because it originated on Android, but I imagine that it'd follow the same
conventions as
the other features.  Would just suggest doing whatever's cleanest.

Generally, though, the Android bits lgtm.  Don't know enough about the desktop
bits to do a proper review, though.

https://codereview.chromium.org/1161233005/diff/80001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_data_fetcher.cc (right):

https://codereview.chromium.org/1161233005/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.cc:204: void
AppBannerDataFetcher::ShowBanner(const SkBitmap* icon,
This function is a little strange, now.  All the work to actually show the
banner is done in the request; this just records that it was without any
indication of why.  Given that this only works on Android and (soon) ChromeOS,
shouldn't some comment be left here about why it doesn't do anything?

[dominickn (DO NOT USE), 2015-06-02 23:37:04]

dominickn@google.com changed reviewers:
+ avi@chromium.org, oshima@chromium.org

[dominickn (DO NOT USE), 2015-06-03 00:54:34]

> This function is a little strange, now.  All the work to actually show the
> banner is done in the request; this just records that it was without any
> indication of why.  Given that this only works on Android and (soon) ChromeOS,
> shouldn't some comment be left here about why it doesn't do anything?

I've added a better explanatory comment here. Thanks!

+Avi: please look at tab_helpers.cc, +Oshima: please look at
theme_resources.grd. Thanks!

[benwells, 2015-06-03 03:33:07]

On 2015/06/03 00:54:34, dominickn wrote:
> > This function is a little strange, now.  All the work to actually show the
> > banner is done in the request; this just records that it was without any
> > indication of why.  Given that this only works on Android and (soon)
ChromeOS,
> > shouldn't some comment be left here about why it doesn't do anything?
> 
> I've added a better explanatory comment here. Thanks!
> 
> +Avi: please look at tab_helpers.cc, +Oshima: please look at
> theme_resources.grd. Thanks!

I'm getting 'error chunk mismatch' on some of the files. I think you need to
reupload the last patch set.

[benwells, 2015-06-03 06:13:55]

lgtm with comments addressed

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
File chrome/browser/banners/app_banner_data_fetcher.cc (right):

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_data_fetcher.cc:207: DCHECK(web_contents &&
!web_app_data_.IsEmpty());
I think you can remove this method's implementation and leave
AppBannerDataFetcher abstract.

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
File chrome/browser/banners/app_banner_data_fetcher_desktop.cc (right):

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_data_fetcher_desktop.cc:31: infobars::InfoBar*
infobar = nullptr;
Nit: This decl can just go down to line 44.

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_data_fetcher_desktop.cc:35: WebApplicationInfo
web_app_info;
Nit: You don't need this variable, you can just put WebApplicationInfo() in line
38.

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
File chrome/browser/banners/app_banner_data_fetcher_desktop.h (right):

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_data_fetcher_desktop.h:15: namespace infobars
{
Nit: I think this namespace / forward decl isn't used.

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager_desktop.cc (right):

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager_desktop.cc:20:
AppBannerManagerDesktop::AppBannerManagerDesktop(
Nit: keep functions here in same order as in .h

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager_desktop.h (right):

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager_desktop.h:26:
DISALLOW_COPY_AND_ASSIGN(AppBannerManagerDesktop);
Nit: blank line before DISALLOW....

[benwells, 2015-06-03 06:31:50]

Oops, one more thing - your description is now out of date as it is for desktop,
not just chromeos.

[dominickn (DO NOT USE), 2015-06-03 07:10:36]

On 2015/06/03 06:31:50, benwells wrote:
> Oops, one more thing - your description is now out of date as it is for
desktop,
> not just chromeos.

Fixed. Thanks!

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
File chrome/browser/banners/app_banner_data_fetcher.cc (right):

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_data_fetcher.cc:207: DCHECK(web_contents &&
!web_app_data_.IsEmpty());
On 2015/06/03 06:13:55, benwells wrote:
> I think you can remove this method's implementation and leave
> AppBannerDataFetcher abstract.

Done. This also required making the manager abstract.

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
File chrome/browser/banners/app_banner_data_fetcher_desktop.cc (right):

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_data_fetcher_desktop.cc:31: infobars::InfoBar*
infobar = nullptr;
On 2015/06/03 06:13:55, benwells wrote:
> Nit: This decl can just go down to line 44.

Done.

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_data_fetcher_desktop.cc:35: WebApplicationInfo
web_app_info;
On 2015/06/03 06:13:55, benwells wrote:
> Nit: You don't need this variable, you can just put WebApplicationInfo() in
line
> 38.

Done.

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
File chrome/browser/banners/app_banner_data_fetcher_desktop.h (right):

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_data_fetcher_desktop.h:15: namespace infobars
{
On 2015/06/03 06:13:55, benwells wrote:
> Nit: I think this namespace / forward decl isn't used.

Done. It's used in the cc file, so I've moved it there

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager_desktop.cc (right):

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager_desktop.cc:20:
AppBannerManagerDesktop::AppBannerManagerDesktop(
On 2015/06/03 06:13:55, benwells wrote:
> Nit: keep functions here in same order as in .h

Done.

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager_desktop.h (right):

https://codereview.chromium.org/1161233005/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager_desktop.h:26:
DISALLOW_COPY_AND_ASSIGN(AppBannerManagerDesktop);
On 2015/06/03 06:13:55, benwells wrote:
> Nit: blank line before DISALLOW....

Done.

[Avi (use Gerrit), 2015-06-03 15:12:43]

https://codereview.chromium.org/1161233005/diff/160001/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/1161233005/diff/160001/chrome/browser/ui/tab_...
chrome/browser/ui/tab_helpers.cc:210: #endif
This is really for not-android? Put this is the existing #else block for
OS_ANDROID and put the #include in the existing block, too.

[dominickn (DO NOT USE), 2015-06-03 22:43:11]

On 2015/06/03 15:12:43, Avi wrote:
>
https://codereview.chromium.org/1161233005/diff/160001/chrome/browser/ui/tab_...
> File chrome/browser/ui/tab_helpers.cc (right):
> 
>
https://codereview.chromium.org/1161233005/diff/160001/chrome/browser/ui/tab_...
> chrome/browser/ui/tab_helpers.cc:210: #endif
> This is really for not-android? Put this is the existing #else block for
> OS_ANDROID and put the #include in the existing block, too.

Done. Thanks!

[Avi (use Gerrit), 2015-06-04 21:20:08]

tab helpers lgtm

[oshima, 2015-06-04 21:57:54]

c/a/theme lgtm

[dominickn (DO NOT USE), 2015-06-05 00:06:10]

The CQ bit was checked by dominickn@google.com

[dominickn (DO NOT USE), 2015-06-05 00:06:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/1161233005/#ps180001
(title: "Moving feature enabling to existing #ifs")

[commit-bot: I haz the power, 2015-06-05 00:07:18]

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

[commit-bot: I haz the power, 2015-06-05 01:42:21]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-06-05 01:43:16]

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