Add "Request app banner" context menu in DevTools

Add "Request app banner" context menu in DevTools

BUG=540491
Demo: https://drive.google.com/file/d/0B6skYAFVnosEc1RQRm9KYmxCeXM/view?usp=sharing

When the user clicks "Request app banner" context menu, Chrome tries to open
"add to homescreen" banner. And if error happens, it will show the detailed
information about why the app banner is not opened even if
"bypass-app-banner-engagement-checks" flag is not set.

RequestAppBanner method from DevTools is handled like this:
 Android:
  PageHandler::RequestAppBanner()
   -> TabWebContentsDelegateAndroid::RequestAppBanner()
    -> Java TabWebContentsDelegateAndroid.requestAppBanner()
     -> Java Tab.requestAppBanner()
      -> Java AppBannerManager.requestAppBanner()
       -> nativeRequestAppBanner()
        -> AppBannerManagerAndroid::RequestAppBanner()
         -> AppBannerManager::TriggerAppBannerFetch()

 Desktop:
  PageHandler::RequestAppBanner()
   -> Browser::RequestAppBanner()
    -> AppBannerManager::TriggerAppBannerFetch()

Committed: https://crrev.com/9638ad271db786762315556b7c2bc9c0a55c8e76
Cr-Commit-Position: refs/heads/master@{#371239}

[horo, 2016-01-08 10:40:48]

Description was changed from

==========
[WIP]Manifest tools
==========

to

==========
[WIP] Add "Open app banner" context menu.

Demo:https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing
==========

[horo, 2016-01-08 11:04:39]

Description was changed from

==========
[WIP] Add "Open app banner" context menu.

Demo:https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing
==========

to

==========
[WIP] Add "Open app banner" context menu.

Demo:
https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing
==========

[pfeldman, 2016-01-14 04:43:05]

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

[pfeldman, 2016-01-14 04:43:05]

Looked at the devtools parts. Please run the app banner part against someone
familiar with the code.

https://codereview.chromium.org/1569893003/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/1569893003/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:41:
scoped_ptr<base::DictionaryValue> CanOpenAppBanner(
I am not sure we need this. Should we make it available at all times when
emulating?

https://codereview.chromium.org/1569893003/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:56:
banners::AppBannerManagerDesktop* manaager =
manager

https://codereview.chromium.org/1569893003/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:73: {
why new scope?

https://codereview.chromium.org/1569893003/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:74: int id = 0;
This already has happened in the network handler. Should we crack the command
here and pass the id/method/params into network handler? Or even better dispatch
commands here and make explicit calls into network_handler / app banner methods.

https://codereview.chromium.org/1569893003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js
(right):

https://codereview.chromium.org/1569893003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js:514:
contextMenu.appendItem(WebInspector.UIString.capitalize("Open app banner"),
this._openAppBanner.bind(this, target.pageAgent()));
I think this item should go into the device mode tree dots menu, as a top level
item for now.

1) DeviceModeView.js should get
contextMenu.appendItemsAtLocation("deviceModeMenu")

2) declare in emulation module:
        {
            "type": "@WebInspector.ActionDelegate",
            "actionId": "emulation.open-app-banner",
            "className": "WebInspector.OpenAppBanner",
            "title": "Open app banner"
        },
        {
            "type": "context-menu-item",
            "location": "deviceModeMenu/tools",
            "order": 10,
            "actionId": "emulation.open-app-banner"
        }

[horo, 2016-01-15 09:36:59]

Description was changed from

==========
[WIP] Add "Open app banner" context menu.

Demo:
https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing
==========

to

==========
[WIP] Add "Open app banner" context menu.

BUG=540491

Demo:
https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing
==========

[horo, 2016-01-15 09:54:16]

Description was changed from

==========
[WIP] Add "Open app banner" context menu.

BUG=540491

Demo:
https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing
==========

to

==========
[WIP] Add "Open app banner" context menu.

BUG=540491

Demo:
https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing

OpenAppBanner method from DevTools is handled like this:
 Desktop:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> OpenAppBanner()
    -> CoreTabHelper::OpenAppBanner()
     -> Browser::OpenAppBanner()
      -> AppBannerManager::TriggerAppBannerFetch()
 Android:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> OpenAppBanner()
    -> CoreTabHelper::OpenAppBanner()
     -> TabAndroid::OpenAppBanner()
      -> Java Tab.openAppBanner()
       -> Java AppBannerManager.openAppBanner()
        -> nativeOpenAppBanner()
         -> AppBannerManagerAndroid::OpenAppBanner()
          -> AppBannerManager::TriggerAppBannerFetch()
==========

[horo, 2016-01-15 09:57:29]

Description was changed from

==========
[WIP] Add "Open app banner" context menu.

BUG=540491

Demo:
https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing

OpenAppBanner method from DevTools is handled like this:
 Desktop:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> OpenAppBanner()
    -> CoreTabHelper::OpenAppBanner()
     -> Browser::OpenAppBanner()
      -> AppBannerManager::TriggerAppBannerFetch()
 Android:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> OpenAppBanner()
    -> CoreTabHelper::OpenAppBanner()
     -> TabAndroid::OpenAppBanner()
      -> Java Tab.openAppBanner()
       -> Java AppBannerManager.openAppBanner()
        -> nativeOpenAppBanner()
         -> AppBannerManagerAndroid::OpenAppBanner()
          -> AppBannerManager::TriggerAppBannerFetch()
==========

to

==========
Add "Open app banner" context menu.

BUG=540491

Demo:
https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing

When the user clicks "Open app banner" context menu, Chrome tries to open "add
to homescreen" banner.
And if error happens, it will show the detailed information about why the app
banner is not opened even if "bypass-app-banner-engagement-checks" flag is not
set.

OpenAppBanner method from DevTools is handled like this:
 Desktop:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> OpenAppBanner()
    -> CoreTabHelper::OpenAppBanner()
     -> Browser::OpenAppBanner()
      -> AppBannerManager::TriggerAppBannerFetch()
 Android:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> OpenAppBanner()
    -> CoreTabHelper::OpenAppBanner()
     -> TabAndroid::OpenAppBanner()
      -> Java Tab.openAppBanner()
       -> Java AppBannerManager.openAppBanner()
        -> nativeOpenAppBanner()
         -> AppBannerManagerAndroid::OpenAppBanner()
          -> AppBannerManager::TriggerAppBannerFetch()
==========

[horo, 2016-01-15 09:58:01]

Description was changed from

==========
Add "Open app banner" context menu.

BUG=540491

Demo:
https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing

When the user clicks "Open app banner" context menu, Chrome tries to open "add
to homescreen" banner.
And if error happens, it will show the detailed information about why the app
banner is not opened even if "bypass-app-banner-engagement-checks" flag is not
set.

OpenAppBanner method from DevTools is handled like this:
 Desktop:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> OpenAppBanner()
    -> CoreTabHelper::OpenAppBanner()
     -> Browser::OpenAppBanner()
      -> AppBannerManager::TriggerAppBannerFetch()
 Android:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> OpenAppBanner()
    -> CoreTabHelper::OpenAppBanner()
     -> TabAndroid::OpenAppBanner()
      -> Java Tab.openAppBanner()
       -> Java AppBannerManager.openAppBanner()
        -> nativeOpenAppBanner()
         -> AppBannerManagerAndroid::OpenAppBanner()
          -> AppBannerManager::TriggerAppBannerFetch()
==========

to

==========
Add "Open app banner" context menu.

BUG=540491
Demo:
https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing

When the user clicks "Open app banner" context menu, Chrome tries to open "add
to homescreen" banner. And if error happens, it will show the detailed
information about why the app banner is not opened even if
"bypass-app-banner-engagement-checks" flag is not set.

OpenAppBanner method from DevTools is handled like this:
 Desktop:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> OpenAppBanner()
    -> CoreTabHelper::OpenAppBanner()
     -> Browser::OpenAppBanner()
      -> AppBannerManager::TriggerAppBannerFetch()
 Android:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> OpenAppBanner()
    -> CoreTabHelper::OpenAppBanner()
     -> TabAndroid::OpenAppBanner()
      -> Java Tab.openAppBanner()
       -> Java AppBannerManager.openAppBanner()
        -> nativeOpenAppBanner()
         -> AppBannerManagerAndroid::OpenAppBanner()
          -> AppBannerManager::TriggerAppBannerFetch()
==========

[horo, 2016-01-15 09:58:18]

Description was changed from

==========
Add "Open app banner" context menu.

BUG=540491
Demo:
https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing

When the user clicks "Open app banner" context menu, Chrome tries to open "add
to homescreen" banner. And if error happens, it will show the detailed
information about why the app banner is not opened even if
"bypass-app-banner-engagement-checks" flag is not set.

OpenAppBanner method from DevTools is handled like this:
 Desktop:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> OpenAppBanner()
    -> CoreTabHelper::OpenAppBanner()
     -> Browser::OpenAppBanner()
      -> AppBannerManager::TriggerAppBannerFetch()
 Android:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> OpenAppBanner()
    -> CoreTabHelper::OpenAppBanner()
     -> TabAndroid::OpenAppBanner()
      -> Java Tab.openAppBanner()
       -> Java AppBannerManager.openAppBanner()
        -> nativeOpenAppBanner()
         -> AppBannerManagerAndroid::OpenAppBanner()
          -> AppBannerManager::TriggerAppBannerFetch()
==========

to

==========
Add "Open app banner" context menu in DevTools

BUG=540491
Demo:
https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing

When the user clicks "Open app banner" context menu, Chrome tries to open "add
to homescreen" banner. And if error happens, it will show the detailed
information about why the app banner is not opened even if
"bypass-app-banner-engagement-checks" flag is not set.

OpenAppBanner method from DevTools is handled like this:
 Desktop:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> OpenAppBanner()
    -> CoreTabHelper::OpenAppBanner()
     -> Browser::OpenAppBanner()
      -> AppBannerManager::TriggerAppBannerFetch()
 Android:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> OpenAppBanner()
    -> CoreTabHelper::OpenAppBanner()
     -> TabAndroid::OpenAppBanner()
      -> Java Tab.openAppBanner()
       -> Java AppBannerManager.openAppBanner()
        -> nativeOpenAppBanner()
         -> AppBannerManagerAndroid::OpenAppBanner()
          -> AppBannerManager::TriggerAppBannerFetch()
==========

[horo, 2016-01-15 10:00:21]

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

[horo, 2016-01-15 10:00:22]

https://codereview.chromium.org/1569893003/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/1569893003/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:41:
scoped_ptr<base::DictionaryValue> CanOpenAppBanner(
On 2016/01/14 04:43:05, pfeldman wrote:
> I am not sure we need this. Should we make it available at all times when
> emulating?

I think we should.
But AppBannerDataFetcherDesktop is created only when "enable-add-to-shelf" flag
is set.
So in this cl I want to show the error message if not enabled.
"Please check chrome://flags/#enable-add-to-shelf"

dfalcantara@, dominickn@
Do you have a plan to enable this flag by default for all platforms?

https://codereview.chromium.org/1569893003/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:56:
banners::AppBannerManagerDesktop* manaager =
On 2016/01/14 04:43:05, pfeldman wrote:
> manager

Done.

https://codereview.chromium.org/1569893003/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:73: {
On 2016/01/14 04:43:05, pfeldman wrote:
> why new scope?

Removed.
Sorry this was still a WIP patch.

https://codereview.chromium.org/1569893003/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:74: int id = 0;
On 2016/01/14 04:43:05, pfeldman wrote:
> This already has happened in the network handler. Should we crack the command
> here and pass the id/method/params into network handler? Or even better
dispatch
> commands here and make explicit calls into network_handler / app banner
methods.

Done.

https://codereview.chromium.org/1569893003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js
(right):

https://codereview.chromium.org/1569893003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js:514:
contextMenu.appendItem(WebInspector.UIString.capitalize("Open app banner"),
this._openAppBanner.bind(this, target.pageAgent()));
On 2016/01/14 04:43:05, pfeldman wrote:
> I think this item should go into the device mode tree dots menu, as a top
level
> item for now.
> 
> 1) DeviceModeView.js should get
> contextMenu.appendItemsAtLocation("deviceModeMenu")
> 
> 2) declare in emulation module:
>         {
>             "type": mailto:"@WebInspector.ActionDelegate",
>             "actionId": "emulation.open-app-banner",
>             "className": "WebInspector.OpenAppBanner",
>             "title": "Open app banner"
>         },
>         {
>             "type": "context-menu-item",
>             "location": "deviceModeMenu/tools",
>             "order": 10,
>             "actionId": "emulation.open-app-banner"
>         }

Implemented in DeviceModeView.
But I think DeviceModeView is not available while remote debugging.
Is my understanding wrong?
I think this "Open app banner" must be available while remote debugging.

[horo, 2016-01-15 10:00:48]

dfalcantara@, dominickn@
Could you please review this CL?

[horo, 2016-01-15 10:01:04]

horo@chromium.org changed reviewers:
+ dominickn@chromium.org

[dominickn, 2016-01-17 23:20:19]

Mostly nits from me. This feature will be super useful to include (and would
have been great to have a few months ago while debugging all of this!)

General comment: I'm not a fan of the "open app banner" terminology, since
"open" is more associated with windows, tabs, and URLs. I prefer "request app
banner" - but I'm not a devtools owner so if "open" is better there then okay.

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager_android.cc:190: true);
Nit: could you call this as:

 AppBannerManager::TriggerAppBannerFetch(web_contents()->GetMainFrame(),
web_contents()->GetLastCommitedURL(), true);

That way you don't need to import render_frame_host.h

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

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.h:101: bool is_debug_mode() const
{ return is_debug_mode_; }
Nit: after reordering, please place this following page_requested_prompt()

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.h:198: const bool is_debug_mode_;
Nit: please put this below page_requested_prompt so that the 4 bools are
together.

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_data_fetcher_unittest.cc (right):

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher_unittest.cc:38: // method if a
null web_contents pointer is provided, so this is safe.
Nit: add a comment about the third argument here.

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

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:74: content::RenderFrameHost*
render_frame_host,
I dislike how render_frame_host is passed in here, but I suppose it *is*
possible for a developer to try putting in an app banner in a subframe. Perhaps
you could add an additional call to OutputDeveloperNotShownMessage when an app
banner is created and render_frame_host->GetParent() is not null?

Nit: I prefer the name "RequestAppBanner" for this method; banners themselves
don't need to be fetched from anywhere, but data for them does.

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:104:
TriggerAppBannerFetch(render_frame_host, validated_url, false);
Nit: add a comment explaining argument 3.

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/devtools...
File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:34: "add-to-shelf is
not supported. Please check chrome://flags/#" +
Nit: this error message could be clearer - perhaps "App banners are currently
disabled. Please check chrome://flags/#

[horo, 2016-01-18 06:28:18]

Description was changed from

==========
Add "Open app banner" context menu in DevTools

BUG=540491
Demo:
https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing

When the user clicks "Open app banner" context menu, Chrome tries to open "add
to homescreen" banner. And if error happens, it will show the detailed
information about why the app banner is not opened even if
"bypass-app-banner-engagement-checks" flag is not set.

OpenAppBanner method from DevTools is handled like this:
 Desktop:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> OpenAppBanner()
    -> CoreTabHelper::OpenAppBanner()
     -> Browser::OpenAppBanner()
      -> AppBannerManager::TriggerAppBannerFetch()
 Android:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> OpenAppBanner()
    -> CoreTabHelper::OpenAppBanner()
     -> TabAndroid::OpenAppBanner()
      -> Java Tab.openAppBanner()
       -> Java AppBannerManager.openAppBanner()
        -> nativeOpenAppBanner()
         -> AppBannerManagerAndroid::OpenAppBanner()
          -> AppBannerManager::TriggerAppBannerFetch()
==========

to

==========
Add "Request app banner" context menu in DevTools

BUG=540491
Demo:
https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing

When the user clicks "Request app banner" context menu, Chrome tries to open
"add to homescreen" banner. And if error happens, it will show the detailed
information about why the app banner is not opened even if
"bypass-app-banner-engagement-checks" flag is not set.

RequestAppBanner method from DevTools is handled like this:
 Desktop:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> RequestAppBanner()
    -> CoreTabHelper::RequestAppBanner()
     -> Browser::RequestAppBanner()
      -> AppBannerManager::TriggerAppBannerFetch()
 Android:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> RequestAppBanner()
    -> CoreTabHelper::RequestAppBanner()
     -> TabAndroid::RequestAppBanner()
      -> Java Tab.requestAppBanner()
       -> Java AppBannerManager.requestAppBanner()
        -> nativeRequestAppBanner()
         -> AppBannerManagerAndroid::RequestAppBanner()
          -> AppBannerManager::TriggerAppBannerFetch()
==========

[horo, 2016-01-18 06:28:56]

Thank you!

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager_android.cc:190: true);
On 2016/01/17 23:20:18, dominickn wrote:
> Nit: could you call this as:
> 
>  AppBannerManager::TriggerAppBannerFetch(web_contents()->GetMainFrame(),
> web_contents()->GetLastCommitedURL(), true);
> 
> That way you don't need to import render_frame_host.h

Done.

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

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.h:101: bool is_debug_mode() const
{ return is_debug_mode_; }
On 2016/01/17 23:20:18, dominickn wrote:
> Nit: after reordering, please place this following page_requested_prompt()

Done.

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.h:198: const bool is_debug_mode_;
On 2016/01/17 23:20:18, dominickn wrote:
> Nit: please put this below page_requested_prompt so that the 4 bools are
> together.

Done.

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_data_fetcher_unittest.cc (right):

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher_unittest.cc:38: // method if a
null web_contents pointer is provided, so this is safe.
On 2016/01/17 23:20:18, dominickn wrote:
> Nit: add a comment about the third argument here.

Done.

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

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:74: content::RenderFrameHost*
render_frame_host,
On 2016/01/17 23:20:19, dominickn wrote:
> I dislike how render_frame_host is passed in here, but I suppose it *is*
> possible for a developer to try putting in an app banner in a subframe.
Perhaps
> you could add an additional call to OutputDeveloperNotShownMessage when an app
> banner is created and render_frame_host->GetParent() is not null?
> 
> Nit: I prefer the name "RequestAppBanner" for this method; banners themselves
> don't need to be fetched from anywhere, but data for them does.

Done.

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:104:
TriggerAppBannerFetch(render_frame_host, validated_url, false);
On 2016/01/17 23:20:18, dominickn wrote:
> Nit: add a comment explaining argument 3.

Done.

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/devtools...
File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/1569893003/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:34: "add-to-shelf is
not supported. Please check chrome://flags/#" +
On 2016/01/17 23:20:19, dominickn wrote:
> Nit: this error message could be clearer - perhaps "App banners are currently
> disabled. Please check chrome://flags/#

Done.

[dominickn, 2016-01-18 23:20:42]

Looking good! Just a couple of small bits to better integrate is_debug_mode with
the existing bypass-app-banner-engagement-checks flag. Sorry I missed this
before.

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

https://codereview.chromium.org/1569893003/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.cc:86:
is_debug_mode_(is_debug_mode),
Change this to:

is_debug_mode_(is_debug_mode ||
base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kBypassAppBannerEngagementChecks)

https://codereview.chromium.org/1569893003/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.cc:300:
!base::CommandLine::ForCurrentProcess()->HasSwitch(
Replace:

!base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kBypassAppBannerEngagementChecks)

with:

!is_debug_mode_

Otherwise, developers who have already installed the webapp on desktop will get
the already installed error when they request the app banner and haven't enabled
the bypass switch. We'll put the command line check in the constructor of
is_debug_mode_.

https://codereview.chromium.org/1569893003/diff/60001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_debug_log.cc (right):

https://codereview.chromium.org/1569893003/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_debug_log.cc:60:
switches::kBypassAppBannerEngagementChecks)) &&
With is_debug_mode incorporating the CommandLine check in the
AppBannerDataFetcher constructor you can remove the command line check in here,
i.e. just do if (is_debug_mode && web_contents).

https://codereview.chromium.org/1569893003/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1569893003/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1989: return true;
Nit: like previously, call this as
manager->RequestAppBanner(web_contents->GetMainFrame(),
web_contents->GetLastCommittedURL(), true). Then remove the extraneous
render_frame_host include.

[horo, 2016-01-19 01:45:43]

Thank you

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

https://codereview.chromium.org/1569893003/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.cc:86:
is_debug_mode_(is_debug_mode),
On 2016/01/18 23:20:41, dominickn wrote:
> Change this to:
> 
> is_debug_mode_(is_debug_mode ||
>
base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kBypassAppBannerEngagementChecks)

Done.

https://codereview.chromium.org/1569893003/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.cc:300:
!base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2016/01/18 23:20:41, dominickn wrote:
> Replace:
> 
>
!base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kBypassAppBannerEngagementChecks)
> 
> with:
> 
> !is_debug_mode_
> 
> Otherwise, developers who have already installed the webapp on desktop will
get
> the already installed error when they request the app banner and haven't
enabled
> the bypass switch. We'll put the command line check in the constructor of
> is_debug_mode_.

Done.

https://codereview.chromium.org/1569893003/diff/60001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_debug_log.cc (right):

https://codereview.chromium.org/1569893003/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_debug_log.cc:60:
switches::kBypassAppBannerEngagementChecks)) &&
On 2016/01/18 23:20:41, dominickn wrote:
> With is_debug_mode incorporating the CommandLine check in the
> AppBannerDataFetcher constructor you can remove the command line check in
here,
> i.e. just do if (is_debug_mode && web_contents).

Done.

https://codereview.chromium.org/1569893003/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1569893003/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1989: return true;
On 2016/01/18 23:20:41, dominickn wrote:
> Nit: like previously, call this as
> manager->RequestAppBanner(web_contents->GetMainFrame(),
> web_contents->GetLastCommittedURL(), true). Then remove the extraneous
> render_frame_host include.

Done.

[dominickn, 2016-01-19 02:25:10]

lgtm (with a follow up for me to look at). I'll defer to dfalcantara for the
final app banner OWNERs stamp.

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

https://codereview.chromium.org/1569893003/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.cc:303:
OutputDeveloperNotShownMessage(web_contents, kBannerAlreadyAdded,
Looking at this again, printing an error message here never made sense (you
would only get an error message if the bypass flag was off, but then you would
only see the error when the bypass flag was on, so it would never show up).

Your CL leaves the current (broken) functionality intact, so no change necessary
here. I've filed crbug.com/578926 to track, and I'll follow up with a fix.

[horo, 2016-01-19 03:40:53]

On 2016/01/19 02:25:10, dominickn wrote:
> lgtm (with a follow up for me to look at). I'll defer to dfalcantara for the
> final app banner OWNERs stamp.
> 
>
https://codereview.chromium.org/1569893003/diff/80001/chrome/browser/banners/...
> File chrome/browser/banners/app_banner_data_fetcher.cc (right):
> 
>
https://codereview.chromium.org/1569893003/diff/80001/chrome/browser/banners/...
> chrome/browser/banners/app_banner_data_fetcher.cc:303:
> OutputDeveloperNotShownMessage(web_contents, kBannerAlreadyAdded,
> Looking at this again, printing an error message here never made sense (you
> would only get an error message if the bypass flag was off, but then you would
> only see the error when the bypass flag was on, so it would never show up).
> 
> Your CL leaves the current (broken) functionality intact, so no change
necessary
> here. I've filed crbug.com/578926 to track, and I'll follow up with a fix.

OK. Thanks.
I added myself to the CC of the bug.

[gone, 2016-01-19 18:35:00]

Android bits lgtm % nits.

https://codereview.chromium.org/1569893003/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/1569893003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2185: /**
Requests the app banner. This method is called from the DevTools. */
Does this function need to be public?  If it's called only by native code, then
it can be marked as private and the compiler will accept it.

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

https://codereview.chromium.org/1569893003/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.h:94: // Returns whether it was
created by the user action in DevTools.
nit: This is inaccurate.  You || the devtools check with the command line flag.

https://codereview.chromium.org/1569893003/diff/80001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/1569893003/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.h:42: void
RequestAppBanner(content::RenderFrameHost* render_frame_host,
nit: Add a comment here?

https://codereview.chromium.org/1569893003/diff/80001/chrome/browser/devtools...
File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/1569893003/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:69: int id = 0;
nit: Can you break this up with newlines?  It's hard to parse.

[horo, 2016-01-20 02:26:34]

Thank you!

https://codereview.chromium.org/1569893003/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/1569893003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2185: /**
Requests the app banner. This method is called from the DevTools. */
On 2016/01/19 18:35:00, dfalcantara wrote:
> Does this function need to be public?  If it's called only by native code,
then
> it can be marked as private and the compiler will accept it.

Done.
Changed to private.

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

https://codereview.chromium.org/1569893003/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.h:94: // Returns whether it was
created by the user action in DevTools.
On 2016/01/19 18:35:00, dfalcantara wrote:
> nit: This is inaccurate.  You || the devtools check with the command line
flag.

Done.

https://codereview.chromium.org/1569893003/diff/80001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/1569893003/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.h:42: void
RequestAppBanner(content::RenderFrameHost* render_frame_host,
On 2016/01/19 18:35:00, dfalcantara wrote:
> nit: Add a comment here?

Done.

https://codereview.chromium.org/1569893003/diff/80001/chrome/browser/devtools...
File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/1569893003/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:69: int id = 0;
On 2016/01/19 18:35:00, dfalcantara wrote:
> nit: Can you break this up with newlines?  It's hard to parse.

Done.

[pfeldman, 2016-01-20 04:30:17]

https://codereview.chromium.org/1569893003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://codereview.chromium.org/1569893003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:142:
nativeRequestAppBanner(mNativePointer);
It seems like DevTools uses a different code path to request the app banner
which defeats the emulation purpose. Is there a way to trigger the same code
path instead?

For example, I can see the WebAppBannerClient.h on the Blink level. Can we reuse
it or extend it to do what we need here?

https://codereview.chromium.org/1569893003/diff/100001/chrome/browser/banners...
File chrome/browser/banners/app_banner_debug_log.cc (right):

https://codereview.chromium.org/1569893003/diff/100001/chrome/browser/banners...
chrome/browser/banners/app_banner_debug_log.cc:55: if (!is_debug_mode ||
!web_contents)
Why do we need to plumb is_debug_mode all the way to here? Can we send it to the
console unconditionally?

https://codereview.chromium.org/1569893003/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/1569893003/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser.h:676: bool RequestAppBanner(content::WebContents*
web_contents) override;
Since content is already appbanner-aware, you can put it into the
web_contents_delegate and save on the chrome-side protocol handling. You'd be
able to call this from content/'s regular handler.

https://codereview.chromium.org/1569893003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/emulation/module.json (right):

https://codereview.chromium.org/1569893003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/emulation/module.json:114: "type":
"context-menu-item",
You can also conditionally put it into mainMenu/tools via the "condition":
"remoteFrontend" in the descriptor.

https://codereview.chromium.org/1569893003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js
(right):

https://codereview.chromium.org/1569893003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js:555:
agent.requestAppBanner();
poor indentation.

https://codereview.chromium.org/1569893003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js:567: if
(node.parent.isRoot()) {
Ditto. This context menu is not discoverable. And it is equally important to
make things work in device mode as well as during remote debugging for
discoverability. We can place action in the main menu in case of remote
debugging and into device menu when emulating. But here nobody finds it.

[dominickn, 2016-01-20 05:37:25]

pfeldman: some more background on the banner implementation. Broadly, I think
the CL does the right thing wrt emulating the banner dispatch process and
displaying console output.

https://codereview.chromium.org/1569893003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://codereview.chromium.org/1569893003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:142:
nativeRequestAppBanner(mNativePointer);
On 2016/01/20 04:30:17, pfeldman wrote:
> It seems like DevTools uses a different code path to request the app banner
> which defeats the emulation purpose. Is there a way to trigger the same code
> path instead?
> 
> For example, I can see the WebAppBannerClient.h on the Blink level. Can we
reuse
> it or extend it to do what we need here?

WebAppBannerClient and the other banner Blink bits dispatch an event to the
developer that Chrome has determined that an app banner can be shown. Developers
can then preventDefault() on the event to stop the banner, or redispatch it
later. If the event isn't stopped, Blink calls back to Chromium, telling it to
actually display the banner. All of the real work (checking manifests, origins,
icons, etc. before deciding that a banner is permitted) happens in the browser
process, not in Blink.

In Chromium, banners are first triggered in AppBannerManager:DidFinishLoad,
which is called on navigations to pages. As far as I can see, this CL makes
DevTools call into the same code path by splitting it out of DidFinishLoad and
into RequestAppBanner, triggering it manually from DevTools and from
DidFinishLoad on navigations.

https://codereview.chromium.org/1569893003/diff/100001/chrome/browser/banners...
File chrome/browser/banners/app_banner_debug_log.cc (right):

https://codereview.chromium.org/1569893003/diff/100001/chrome/browser/banners...
chrome/browser/banners/app_banner_debug_log.cc:55: if (!is_debug_mode ||
!web_contents)
On 2016/01/20 04:30:17, pfeldman wrote:
> Why do we need to plumb is_debug_mode all the way to here? Can we send it to
the
> console unconditionally?

Currently, the logging functions are *always* called, but output is only sent to
the console if the bypass-app-banner-engagement-checks flag is present. When the
flag is off, then nothing should be sent to the console to avoid spamming it.

Sending output to the console unconditionally would be very noisy: every site
would trigger at least one message on Android and ChromeOS, where this feature
is 100% rolled out. Most of the messages would be "not a secure origin", or "no
service worker", or "no web manifest".

The alternative is to guard every call to this logging with if (is_debug_mode),
but I think it's cleaner having one if check in here (at the cost of plumbing
the boolean). Previously, it would check the command line switch in here; this
CL combines the command line switch into is_debug_mode.

[horo, 2016-01-20 10:56:41]

Description was changed from

==========
Add "Request app banner" context menu in DevTools

BUG=540491
Demo:
https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing

When the user clicks "Request app banner" context menu, Chrome tries to open
"add to homescreen" banner. And if error happens, it will show the detailed
information about why the app banner is not opened even if
"bypass-app-banner-engagement-checks" flag is not set.

RequestAppBanner method from DevTools is handled like this:
 Desktop:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> RequestAppBanner()
    -> CoreTabHelper::RequestAppBanner()
     -> Browser::RequestAppBanner()
      -> AppBannerManager::TriggerAppBannerFetch()
 Android:
  ChromeDevToolsManagerDelegate:HandleCommand()
   -> RequestAppBanner()
    -> CoreTabHelper::RequestAppBanner()
     -> TabAndroid::RequestAppBanner()
      -> Java Tab.requestAppBanner()
       -> Java AppBannerManager.requestAppBanner()
        -> nativeRequestAppBanner()
         -> AppBannerManagerAndroid::RequestAppBanner()
          -> AppBannerManager::TriggerAppBannerFetch()
==========

to

==========
Add "Request app banner" context menu in DevTools

BUG=540491
Demo:
https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing

When the user clicks "Request app banner" context menu, Chrome tries to open
"add to homescreen" banner. And if error happens, it will show the detailed
information about why the app banner is not opened even if
"bypass-app-banner-engagement-checks" flag is not set.

RequestAppBanner method from DevTools is handled like this:
 Android:
  PageHandler::RequestAppBanner()
   -> TabWebContentsDelegateAndroid::RequestAppBanner()
    -> Java TabWebContentsDelegateAndroid.requestAppBanner()
     -> Java Tab.requestAppBanner()
      -> Java AppBannerManager.requestAppBanner()
       -> nativeRequestAppBanner()
        -> AppBannerManagerAndroid::RequestAppBanner()
         -> AppBannerManager::TriggerAppBannerFetch()
         
 Desktop:
  PageHandler::RequestAppBanner()
   -> Browser::RequestAppBanner()
    -> AppBannerManager::TriggerAppBannerFetch()
==========

[horo, 2016-01-20 11:00:31]

Patchset #7 (id:120001) has been deleted

[horo, 2016-01-20 11:01:42]

https://codereview.chromium.org/1569893003/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/1569893003/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser.h:676: bool RequestAppBanner(content::WebContents*
web_contents) override;
On 2016/01/20 04:30:17, pfeldman wrote:
> Since content is already appbanner-aware, you can put it into the
> web_contents_delegate and save on the chrome-side protocol handling. You'd be
> able to call this from content/'s regular handler.

Done.

https://codereview.chromium.org/1569893003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/emulation/module.json (right):

https://codereview.chromium.org/1569893003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/emulation/module.json:114: "type":
"context-menu-item",
On 2016/01/20 04:30:17, pfeldman wrote:
> You can also conditionally put it into mainMenu/tools via the "condition":
> "remoteFrontend" in the descriptor.

WebInspector.ScreencastApp doesn't create WebInspector.DeviceModeView.Wrapper.
So I added WebInspector.ScreencastApp.ActionDelegate in ScreencastSpp.js.

https://codereview.chromium.org/1569893003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js
(right):

https://codereview.chromium.org/1569893003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js:555:
agent.requestAppBanner();
On 2016/01/20 04:30:17, pfeldman wrote:
> poor indentation.

Removed

https://codereview.chromium.org/1569893003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js:567: if
(node.parent.isRoot()) {
On 2016/01/20 04:30:17, pfeldman wrote:
> Ditto. This context menu is not discoverable. And it is equally important to
> make things work in device mode as well as during remote debugging for
> discoverability. We can place action in the main menu in case of remote
> debugging and into device menu when emulating. But here nobody finds it.

Reverted NavigatorView.js and changed ScreencastApp.js.

[pfeldman, 2016-01-20 18:18:41]

On 2016/01/20 05:37:25, dominickn wrote:
> pfeldman: some more background on the banner implementation. 

Thanks!

> In Chromium, banners are first triggered in AppBannerManager:DidFinishLoad,
> which is called on navigations to pages. As far as I can see, this CL makes
> DevTools call into the same code path by splitting it out of DidFinishLoad and
> into RequestAppBanner, triggering it manually from DevTools and from
> DidFinishLoad on navigations.

That sounds good, but does not explain the changes to the Android port and Java
classes. Are those a part of a different change? For example,

/** Requests the app banner. This method is called from the DevTools. */
 141     public void requestAppBanner() {

in AppBannerManager.java suggests that there is a special DevTools treatment in
the unrelated location.


> Currently, the logging functions are *always* called, but output is only sent
to
> the console if the bypass-app-banner-engagement-checks flag is present. When
the
> flag is off, then nothing should be sent to the console to avoid spamming it.

Got it, in this case, I'd suggest to simply insert a
DevToolsAgentHost::IsDebuggerAttached(web_contents) into the
OutputDeveloperNotShownMessage. It is a public content API that you can use
there. The only (minor) issue is that opening DevTools post-failure prior to the
navigation would not surface the error.

[dominickn, 2016-01-21 02:26:30]

On 2016/01/20 18:18:41, pfeldman wrote:
> That sounds good, but does not explain the changes to the Android port and
Java
> classes. Are those a part of a different change? For example,
> 
> /** Requests the app banner. This method is called from the DevTools. */
>  141     public void requestAppBanner() {
> 
> in AppBannerManager.java suggests that there is a special DevTools treatment
in
> the unrelated location.

On Android, the banner flow begins in C++ native code (app_banner_manager.cc),
but the C++ AppBannerManager is owned by the Java AppBannerManager, making
the Java class the only way to get to the native code. Triggering the banner
flow
manually requires a call through the JNI from TabWebContentsDelegateAndroid
to the Java AppBannerManager, which then calls RequestAppBanner in the native
code.

I think it's been a long term fixme to switch around this ownership on the
Android
side - the implementation there has been around much longer than the desktop
version, which is why it's like this.

> Got it, in this case, I'd suggest to simply insert a
> DevToolsAgentHost::IsDebuggerAttached(web_contents) into the
> OutputDeveloperNotShownMessage. It is a public content API that you can use
> there. The only (minor) issue is that opening DevTools post-failure prior to
the
> navigation would not surface the error.

sgtm - thanks for pointing out this method! In that case, you can remove
is_debug_mode entirely from everywhere, and in
OutputDeveloperNotShownMessage, have it check
bypass flag || DevToolsAgentHost::IsDebuggerAttached(web_contents).
That's a much nicer way of doing this.

[horo, 2016-01-21 05:59:11]

> sgtm - thanks for pointing out this method! In that case, you can remove
> is_debug_mode entirely from everywhere, and in
> OutputDeveloperNotShownMessage, have it check
> bypass flag || DevToolsAgentHost::IsDebuggerAttached(web_contents).
> That's a much nicer way of doing this.

IsDebuggerAttached() returns true when DevTools window is opened.
So even when IsDebuggerAttached() is true, we should not always show such error
messages.

We should show them only when the developer is interested in Manifest.
So I think we need is_debug_mode flag.

[pfeldman, 2016-01-21 18:29:49]

> On Android, the banner flow begins in C++ native code (app_banner_manager.cc),
> but the C++ AppBannerManager is owned by the Java AppBannerManager, making
> the Java class the only way to get to the native code. Triggering the banner
> flow
> manually requires a call through the JNI from TabWebContentsDelegateAndroid
> to the Java AppBannerManager, which then calls RequestAppBanner in the native
> code.

Sorry, I still don't get it. If banner flows begins in app_banner_manager.cc,
why can't we manually kick it via calling one of its methods? I.e. merge into
the existing control flow.

[pfeldman, 2016-01-21 18:45:48]

> IsDebuggerAttached() returns true when DevTools window is opened.
> So even when IsDebuggerAttached() is true, we should not always show such
error
> messages.
> 
> We should show them only when the developer is interested in Manifest.
> So I think we need is_debug_mode flag.

Oh, that's right. But it seems like plumbing is_debug to the method of form

OutputDeveloperDebugMessage(a, b, is_debug) {
    if (!is_debug)
        return;
    ....
}

it only adding to the technical dept. Can we make step back and reflect on why
the flags are necessary only in certain modes? Can we make the code not go there
when it does not have and and yell in case it had to, but failed? It might
involve changing the severity of messages and being less spammy, etc?

[dominickn, 2016-01-22 03:40:22]

On 2016/01/21 18:29:49, pfeldman wrote:
> Sorry, I still don't get it. If banner flows begins in app_banner_manager.cc,
> why can't we manually kick it via calling one of its methods? I.e. merge into
> the existing control flow.

{TabWebContentsDelegateAndroid,Browser}::RequestAppBanner are the key methods
here.

On desktop, the C++ app banner manager inherits from WebContentsUserData, so it
can be fetched
directly from a WebContents pointer. That means Browser::RequestAppBanner can
get the manager
directly and call its RequestAppBanner method.

On Android, the C++ app banner manager isn't a WebContentsUserData. Instead, the
only way to
get it is to tell the Java AppBannerManager (its owner) to tell the C++ manager
to do something.
So TabWebContentsDelegateAndroid::RequestAppBanner has to fetch the Java manager
through
the JNI and tell it to call RequestAppBanner on the C++ manager. That's where
the extra method
on the Java side class comes from.

I hope that makes sense. Like I said before, it's been a long term TODO to fix
this ownership
given that banners are on all platforms now. When that happens, the extra Java
change in this CL
can go away. I've filed https://crbug.com/580392 to track this refactoring.


On 2016/01/21 18:45:48, pfeldman wrote:
> Oh, that's right. But it seems like plumbing is_debug to the method of form
> 
> OutputDeveloperDebugMessage(a, b, is_debug) {
>     if (!is_debug)
>         return;
>     ....
> }
> 
> it only adding to the technical dept. Can we make step back and reflect on why
> the flags are necessary only in certain modes? Can we make the code not go
there
> when it does not have and and yell in case it had to, but failed? It might
> involve changing the severity of messages and being less spammy, etc?

Hmm, then I'm in favour of the implementation as it currently stands.

The tricky thing here is that app banners have *many* requirements (valid URL,
valid manifest, valid icon, active service worker, secure origin, Play store
ID/URL).
Failing to meet any of these stops the banner from appearing. Without the
extensive logging, it's mysterious as to why app banners aren't triggering;
that's
why there are so many error strings in debug_log.cc.

Additionally, app banners require users to have visited the site some number of
times
before they will be offered - this avoids showing banners to users who haven't
really engaged with a site. This is why the bypass-app-banner-engagement-checks
flag
exists; it allows developers who are testing banners to skip the engagement
requirements
and ensure that when users are engaged, the banner will work right. Hence, debug
messages only appear when the bypass flag is on currently.

Finally, the debug messages get sent from both the manager and the data fetcher.
That
makes an if guard unwieldy.

Once the Java banner manager ownership refactoring happens, I think all the
debug logging
can be moved purely into the data fetcher (right now the Android Java manager is
in charge
of retrieving Play Store details, so it's important to be able to log errors
from there). At that
stage we can have a DebugLog object on the data fetcher that handles all of the
debugging logic nicely.

[pfeldman, 2016-01-22 04:11:03]

Ok, it all makes sense now, thank you for your patience. It is a shame this
patch has to incorporate compromises on both these fronts - I was hoping to
eliminate at least of one of them :)

@horo, @dominickn: wrt "So even when IsDebuggerAttached() is true, we should not
always show such error messages.". What is the negative side effect of this? If
there are errors, it would be nice for us to be proactive and show those to the
user even though app banner installation happened on reload. If there are no
errors, it'll be silent. Is that right?

[dominickn, 2016-01-22 04:22:03]

On 2016/01/22 04:11:03, pfeldman wrote:
> Ok, it all makes sense now, thank you for your patience. It is a shame this
> patch has to incorporate compromises on both these fronts - I was hoping to
> eliminate at least of one of them :)

Agreed - I'll try and prioritize the refactoring which should clean up both of
these compromises :)

> @horo, @dominickn: wrt "So even when IsDebuggerAttached() is true, we should
not
> always show such error messages.". What is the negative side effect of this?
If
> there are errors, it would be nice for us to be proactive and show those to
the
> user even though app banner installation happened on reload. If there are no
> errors, it'll be silent. Is that right?

Assuming that we use IsDebuggerAttached, when DevTools is open the only time
there
will be no errors is if a banner is correctly and fully implemented. Every
single other site
opened in DevTools and reloaded will see some message, usually along the lines
of
"App banner not shown: {insecure origin | no manifest | no service worker}."
This could
be confusing if the developer doesn't know about the app banner feature or is
working
on something unrelated.

[pfeldman, 2016-01-22 05:24:30]

I see. My worry is that user never forces install and hence never sees the
banner-related errors.

If we could come up with a strong enough signal of the fact that user wants a
banner, we could proactively show errors and hence bring the most value. For
example:

- there is manifest (weakest signal)
- there is manifest and service worker (moderate)
- there is manifest with short_name (very strong)
- there is images section (moderate)
- images contain 144x144 (very strong)

I'd enable error reporting for the moderate+ cases in case DevTools is there.
Error message would also instruct on how to force install for subsequent tries.
That way we are proacrive, have sane signal-to-noise and boost discoverability.
Wdyt?

[pfeldman, 2016-01-22 05:28:58]

devtools lgtm since what i suggest is incremental wrt this change.

[horo, 2016-01-22 08:48:39]

On 2016/01/22 05:28:58, pfeldman wrote:
> devtools lgtm since what i suggest is incremental wrt this change.

Thank you.

> Wdyt?

I think that detecting whether the developer is interested in a banner or not
may be difficult, because manifest files may be used only for push messages.

[horo, 2016-01-22 08:49:42]

horo@chromium.org changed reviewers:
+ jochen@chromium.org

[horo, 2016-01-22 08:49:43]

jochen@

Could you please review chrome/browser/ui/browser.* and
components/web_contents_delegate_android/web_contents_delegate_android.*?

[horo, 2016-01-22 11:48:48]

Description was changed from

==========
Add "Request app banner" context menu in DevTools

BUG=540491
Demo:
https://drive.google.com/file/d/0B6skYAFVnosEYjgtaDM4cm1DZ1U/view?usp=sharing

When the user clicks "Request app banner" context menu, Chrome tries to open
"add to homescreen" banner. And if error happens, it will show the detailed
information about why the app banner is not opened even if
"bypass-app-banner-engagement-checks" flag is not set.

RequestAppBanner method from DevTools is handled like this:
 Android:
  PageHandler::RequestAppBanner()
   -> TabWebContentsDelegateAndroid::RequestAppBanner()
    -> Java TabWebContentsDelegateAndroid.requestAppBanner()
     -> Java Tab.requestAppBanner()
      -> Java AppBannerManager.requestAppBanner()
       -> nativeRequestAppBanner()
        -> AppBannerManagerAndroid::RequestAppBanner()
         -> AppBannerManager::TriggerAppBannerFetch()
         
 Desktop:
  PageHandler::RequestAppBanner()
   -> Browser::RequestAppBanner()
    -> AppBannerManager::TriggerAppBannerFetch()
==========

to

==========
Add "Request app banner" context menu in DevTools

BUG=540491
Demo:
https://drive.google.com/file/d/0B6skYAFVnosEc1RQRm9KYmxCeXM/view?usp=sharing

When the user clicks "Request app banner" context menu, Chrome tries to open
"add to homescreen" banner. And if error happens, it will show the detailed
information about why the app banner is not opened even if
"bypass-app-banner-engagement-checks" flag is not set.

RequestAppBanner method from DevTools is handled like this:
 Android:
  PageHandler::RequestAppBanner()
   -> TabWebContentsDelegateAndroid::RequestAppBanner()
    -> Java TabWebContentsDelegateAndroid.requestAppBanner()
     -> Java Tab.requestAppBanner()
      -> Java AppBannerManager.requestAppBanner()
       -> nativeRequestAppBanner()
        -> AppBannerManagerAndroid::RequestAppBanner()
         -> AppBannerManager::TriggerAppBannerFetch()
         
 Desktop:
  PageHandler::RequestAppBanner()
   -> Browser::RequestAppBanner()
    -> AppBannerManager::TriggerAppBannerFetch()
==========

[jochen (gone - plz use gerrit), 2016-01-25 09:53:23]

lgtm

[horo, 2016-01-25 09:58:18]

The CQ bit was checked by horo@chromium.org

[horo, 2016-01-25 09:58:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/1569893003/#ps140001
(title: "incorporated pfeldman's comment")

[horo, 2016-01-25 09:58:34]

Thank you!

[commit-bot: I haz the power, 2016-01-25 09:58:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1569893003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1569893003/140001

[commit-bot: I haz the power, 2016-01-25 13:23:01]

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

[commit-bot: I haz the power, 2016-01-25 13:23:02]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[horo, 2016-01-25 13:26:12]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2016-01-25 13:26:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1569893003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1569893003/140001

[commit-bot: I haz the power, 2016-01-25 14:36:53]

Description was changed from

==========
Add "Request app banner" context menu in DevTools

BUG=540491
Demo:
https://drive.google.com/file/d/0B6skYAFVnosEc1RQRm9KYmxCeXM/view?usp=sharing

When the user clicks "Request app banner" context menu, Chrome tries to open
"add to homescreen" banner. And if error happens, it will show the detailed
information about why the app banner is not opened even if
"bypass-app-banner-engagement-checks" flag is not set.

RequestAppBanner method from DevTools is handled like this:
 Android:
  PageHandler::RequestAppBanner()
   -> TabWebContentsDelegateAndroid::RequestAppBanner()
    -> Java TabWebContentsDelegateAndroid.requestAppBanner()
     -> Java Tab.requestAppBanner()
      -> Java AppBannerManager.requestAppBanner()
       -> nativeRequestAppBanner()
        -> AppBannerManagerAndroid::RequestAppBanner()
         -> AppBannerManager::TriggerAppBannerFetch()
         
 Desktop:
  PageHandler::RequestAppBanner()
   -> Browser::RequestAppBanner()
    -> AppBannerManager::TriggerAppBannerFetch()
==========

to

==========
Add "Request app banner" context menu in DevTools

BUG=540491
Demo:
https://drive.google.com/file/d/0B6skYAFVnosEc1RQRm9KYmxCeXM/view?usp=sharing

When the user clicks "Request app banner" context menu, Chrome tries to open
"add to homescreen" banner. And if error happens, it will show the detailed
information about why the app banner is not opened even if
"bypass-app-banner-engagement-checks" flag is not set.

RequestAppBanner method from DevTools is handled like this:
 Android:
  PageHandler::RequestAppBanner()
   -> TabWebContentsDelegateAndroid::RequestAppBanner()
    -> Java TabWebContentsDelegateAndroid.requestAppBanner()
     -> Java Tab.requestAppBanner()
      -> Java AppBannerManager.requestAppBanner()
       -> nativeRequestAppBanner()
        -> AppBannerManagerAndroid::RequestAppBanner()
         -> AppBannerManager::TriggerAppBannerFetch()
         
 Desktop:
  PageHandler::RequestAppBanner()
   -> Browser::RequestAppBanner()
    -> AppBannerManager::TriggerAppBannerFetch()
==========

[commit-bot: I haz the power, 2016-01-25 14:36:54]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-01-25 14:38:16]

Description was changed from

==========
Add "Request app banner" context menu in DevTools

BUG=540491
Demo:
https://drive.google.com/file/d/0B6skYAFVnosEc1RQRm9KYmxCeXM/view?usp=sharing

When the user clicks "Request app banner" context menu, Chrome tries to open
"add to homescreen" banner. And if error happens, it will show the detailed
information about why the app banner is not opened even if
"bypass-app-banner-engagement-checks" flag is not set.

RequestAppBanner method from DevTools is handled like this:
 Android:
  PageHandler::RequestAppBanner()
   -> TabWebContentsDelegateAndroid::RequestAppBanner()
    -> Java TabWebContentsDelegateAndroid.requestAppBanner()
     -> Java Tab.requestAppBanner()
      -> Java AppBannerManager.requestAppBanner()
       -> nativeRequestAppBanner()
        -> AppBannerManagerAndroid::RequestAppBanner()
         -> AppBannerManager::TriggerAppBannerFetch()
         
 Desktop:
  PageHandler::RequestAppBanner()
   -> Browser::RequestAppBanner()
    -> AppBannerManager::TriggerAppBannerFetch()
==========

to

==========
Add "Request app banner" context menu in DevTools

BUG=540491
Demo:
https://drive.google.com/file/d/0B6skYAFVnosEc1RQRm9KYmxCeXM/view?usp=sharing

When the user clicks "Request app banner" context menu, Chrome tries to open
"add to homescreen" banner. And if error happens, it will show the detailed
information about why the app banner is not opened even if
"bypass-app-banner-engagement-checks" flag is not set.

RequestAppBanner method from DevTools is handled like this:
 Android:
  PageHandler::RequestAppBanner()
   -> TabWebContentsDelegateAndroid::RequestAppBanner()
    -> Java TabWebContentsDelegateAndroid.requestAppBanner()
     -> Java Tab.requestAppBanner()
      -> Java AppBannerManager.requestAppBanner()
       -> nativeRequestAppBanner()
        -> AppBannerManagerAndroid::RequestAppBanner()
         -> AppBannerManager::TriggerAppBannerFetch()

 Desktop:
  PageHandler::RequestAppBanner()
   -> Browser::RequestAppBanner()
    -> AppBannerManager::TriggerAppBannerFetch()

Committed: https://crrev.com/9638ad271db786762315556b7c2bc9c0a55c8e76
Cr-Commit-Position: refs/heads/master@{#371239}
==========

[commit-bot: I haz the power, 2016-01-25 14:38:18]

Patchset 7 (id:??) landed as
https://crrev.com/9638ad271db786762315556b7c2bc9c0a55c8e76
Cr-Commit-Position: refs/heads/master@{#371239}