DevTools: make app banner debug messages report with different severity

DevTools: make app banner debug messages report with different severity

BUG=540491
Committed: https://crrev.com/90ec49bfb97fced1cd822de1e13b2284afaa1485
Cr-Commit-Position: refs/heads/master@{#383759}

[pfeldman, 2016-03-25 20:20:10]

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

[pfeldman, 2016-03-25 20:37:36]

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

[dgozman, 2016-03-25 22:30:22]

lgtm with nits

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

https://codereview.chromium.org/1835063002/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_debug_log.h:9: #include
"content/public/browser/render_frame_host.h"
Not used.

https://codereview.chromium.org/1835063002/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_debug_log.h:19: enum
OutputDeveloperMessageCode {
enum class?

[pfeldman, 2016-03-25 23:42:09]

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

https://codereview.chromium.org/1835063002/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_debug_log.h:9: #include
"content/public/browser/render_frame_host.h"
On 2016/03/25 22:30:21, dgozman wrote:
> Not used.

Done.

https://codereview.chromium.org/1835063002/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_debug_log.h:19: enum
OutputDeveloperMessageCode {
On 2016/03/25 22:30:21, dgozman wrote:
> enum class?

Ack.

[dominickn, 2016-03-27 22:32:06]

Can you clarify what changes this makes over https://crrev.com/1569893003, and
add a bug number? I'm not sure what the "additional flags" reference means.

FYI, you'll need to rebase this CL when https://crrev.com/1829643002 lands, as
that adds a new banner debug message.

This CL also seems to be making two orthogonal changes - perhaps consider
splitting it into:

a. refactor of app banner debug messaging
b. changes to devtools

Additionally, I'm not entirely convinced that the extra complexity of adding an
enum/switch for the error message is really worth it. The old way wasn't
perfect, but for example, if a future error message requires more than one
parameter, it'll require a new log method.

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

https://codereview.chromium.org/1835063002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_debug_log.cc:44: "%s application ignored: not
supported on Android";
Nit: now that the prefix for all of the messages is "App banner not shown: ",
the double colon here isn't great. Change this message to "%s application is not
supported on Android"

https://codereview.chromium.org/1835063002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_debug_log.cc:46: "play application ignored: no
id provided";
Nit: "no Play store ID provided"

https://codereview.chromium.org/1835063002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_debug_log.cc:48: "play application ignored:
app URL and id fields were specified in the "
Nit: "a Play app URL and Play store ID were specified in the manifest, but they
do not match"

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

https://codereview.chromium.org/1835063002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_debug_log.h:17: 
Nit: update this comment to reference error message codes. Remove the extra
newline.

https://codereview.chromium.org/1835063002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_debug_log.h:44: // and the engagement checks
have been bypassed.
Nit: this comment is a copy-paste of the above. Make it reference the param
argument.

https://codereview.chromium.org/1835063002/diff/80001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/1835063002/diff/80001/content/public/browser/...
content/public/browser/web_contents_delegate.h:534: virtual void
RequestAppBannerFromDevTools(content::WebContents* web_contents);
I think you also need to change the method in
chrome/browser/android/tab_web_contents_delegate_android.cc, which overrides the
old version. Otherwise this won't work on Android.

[pfeldman, 2016-03-28 18:38:36]

Description was changed from

==========
DevTools: request app banner from DevTools with no additional flags.
==========

to

==========
DevTools: make app banner debug messages report with different severity
==========

[pfeldman, 2016-03-28 18:39:31]

>> I'm not sure what the "additional flags" reference means.

No flags are required now, whereas "enable-add-to-shelf" was previously required
during mobile emulation.

>> FYI, you'll need to rebase this CL when https://crrev.com/1829643002 lands,
as that adds a new banner debug message.

Sounds good.

>> perhaps consider splitting it into:
>> a. refactor of app banner debug messaging
>> b. changes to devtools

Since this one has comments on messages, I'll convert it into "a" and upload "b"
separately.

>> Additionally, I'm not entirely convinced that the extra complexity of adding
an enum/switch for the error message is really worth it. The old way wasn't
perfect, but for example, if a future error message requires more than one
parameter, it'll require a new log method.

I would leave this as is, but the main point of the message refactoring was to
add support for different severity levels depending on the error code. I
understand that it won't scale to 2 params, but one would be able to overload
another method for it if / when the time comes.

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

https://codereview.chromium.org/1835063002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_debug_log.cc:44: "%s application ignored: not
supported on Android";
On 2016/03/27 22:32:05, dominickn wrote:
> Nit: now that the prefix for all of the messages is "App banner not shown: ",
> the double colon here isn't great. Change this message to "%s application is
not
> supported on Android"

Done.

https://codereview.chromium.org/1835063002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_debug_log.cc:46: "play application ignored: no
id provided";
On 2016/03/27 22:32:05, dominickn wrote:
> Nit: "no Play store ID provided"

Done.

https://codereview.chromium.org/1835063002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_debug_log.cc:48: "play application ignored:
app URL and id fields were specified in the "
On 2016/03/27 22:32:05, dominickn wrote:
> Nit: "a Play app URL and Play store ID were specified in the manifest, but
they
> do not match"

Done.

https://codereview.chromium.org/1835063002/diff/80001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/1835063002/diff/80001/content/public/browser/...
content/public/browser/web_contents_delegate.h:534: virtual void
RequestAppBannerFromDevTools(content::WebContents* web_contents);
On 2016/03/27 22:32:06, dominickn wrote:
> I think you also need to change the method in
> chrome/browser/android/tab_web_contents_delegate_android.cc, which overrides
the
> old version. Otherwise this won't work on Android.

Done.

[paulirish, 2016-03-28 20:54:22]

Description was changed from

==========
DevTools: make app banner debug messages report with different severity
==========

to

==========
DevTools: make app banner debug messages report with different severity

BUG=540491
==========

[dominickn, 2016-03-28 23:09:57]

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

[dominickn, 2016-03-28 23:09:57]

lgtm % trybots

+dfalcantara for chrome/browser/android OWNERs

On 2016/03/28 18:39:31, pfeldman wrote:
> >> I'm not sure what the "additional flags" reference means.
> 
> No flags are required now, whereas "enable-add-to-shelf" was previously
required
> during mobile emulation.

Thanks for clarifying :)

> I would leave this as is, but the main point of the message refactoring was to
> add support for different severity levels depending on the error code. I
> understand that it won't scale to 2 params, but one would be able to overload
> another method for it if / when the time comes.

Acknowledged.

[gone, 2016-03-28 23:50:18]

android/ lgtm.

[pfeldman, 2016-03-29 04:12:47]

The CQ bit was checked by pfeldman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-29 04:13:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1835063002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1835063002/100001

[commit-bot: I haz the power, 2016-03-29 04:52:46]

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

[commit-bot: I haz the power, 2016-03-29 04:52:47]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[pfeldman, 2016-03-29 17:00:01]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-03-29 17:00:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, dominickn@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/1835063002/#ps120001
(title: "win fixed")

[commit-bot: I haz the power, 2016-03-29 17:00:14]

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

[commit-bot: I haz the power, 2016-03-29 17:48:11]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-29 17:50:18]

Description was changed from

==========
DevTools: make app banner debug messages report with different severity

BUG=540491
==========

to

==========
DevTools: make app banner debug messages report with different severity

BUG=540491

Committed: https://crrev.com/90ec49bfb97fced1cd822de1e13b2284afaa1485
Cr-Commit-Position: refs/heads/master@{#383759}
==========

[commit-bot: I haz the power, 2016-03-29 17:50:19]

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