[Webapps]: Clear AppBannerManager::page_requested_prompt_ at start of banner flow

[Webapps]: Clear AppBannerManager::page_requested_prompt_ at start of banner flow

This CL fixes a bug where a web page is not capable of cancelling the app banner
if the tab was previously navigated to a page which requested the banner in the
beforeinstallprompt() JS handler.

BUG=688902
TEST=AppBannerManagerBrowserTest.CancelBannerAfterPromptInHandler

Review-Url: https://codereview.chromium.org/2677853002
Cr-Commit-Position: refs/heads/master@{#451689}
Committed: https://chromium.googlesource.com/chromium/src/+/e082e1d1591776aff10d468bc7057dfa3e49abea

[pkotwicz, 2017-02-06 03:32:42]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2017-02-06 03:34:16]

Description was changed from

==========
[Webapps]: Clear AppBannerManager::page_requested_prompt_ at start of banner
flow

This CL fixes a bug where a web page is not capable of cancelling the app banner
if the tab was previously navigated to a page which requested the banner in the
beforeinstallprompt JS handler.

BUG=688902
TEST=
==========

to

==========
[Webapps]: Clear AppBannerManager::page_requested_prompt_ at start of banner
flow

This CL fixes a bug where a web page is not capable of cancelling the app banner
if the tab was previously navigated to a page which requested the banner in the
beforeinstallprompt() JS handler.

BUG=688902
TEST=AppBannerManagerBrowserTest.CancelBannerAfterPromptInHandler
==========

[pkotwicz, 2017-02-06 03:37:26]

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

[pkotwicz, 2017-02-06 03:37:26]

Dominick, can you please take a look?

[dominickn, 2017-02-06 05:22:01]

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

https://codereview.chromium.org/2677853002/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:12: #include
"base/strings/string_number_conversions.h"
#include "base/macros.h" for DISALLOW_COPY_AND_ASSIGN

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

https://codereview.chromium.org/2677853002/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_browsertest.cc:32: public
content::WebContentsUserData<AppBannerManagerTest> {
Not sure if it's necessary to have this full WebContentsUserData in this test (I
can't find another test file that declares a WebContentsUserData subclass). The
right way to do this is to probably move the std::unique_ptr<AppBannerManager>
up a level and pass it in the raw AppBannerManagerTest as a pointer argument.

https://codereview.chromium.org/2677853002/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_browsertest.cc:75:
AppBannerManager::WebContentsDestroyed();
WebContentsDestroyed() calls Stop(), which sets will_show_ and calls the quit
closure. Why is this needed? (At the very least, why is will_show_ reset?)

[pkotwicz, 2017-02-09 00:57:37]

Dominick, can you please take another look?

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

https://codereview.chromium.org/2677853002/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_browsertest.cc:32: public
content::WebContentsUserData<AppBannerManagerTest> {
Passing in AppBannerManagerTest to RunBannerTest() is another way of writing my
test. I like this way better because it is closer to how the production code
works.

https://codereview.chromium.org/2677853002/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_browsertest.cc:75:
AppBannerManager::WebContentsDestroyed();
AppBannerManager::WebContentsDestroyed() always calls Stop() regardless of
whether the banner was shown or not.

I am resetting |will_show_| to avoid failing on the assertion on line 57.

Previously this was not an issue because AppBannerManagerTest's lifetime was
super short so AppBannerManagerTest::WebContentsDestroyed() was not called in
tests

[dominickn, 2017-02-09 04:49:04]

lgtm, but I still would prefer passing an AppBannerManager to RunBannerTest
rather than making AppBannerManagerTest a WebContentsUserData.

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

https://codereview.chromium.org/2677853002/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_browsertest.cc:32: public
content::WebContentsUserData<AppBannerManagerTest> {
On 2017/02/09 00:57:37, pkotwicz wrote:
> Passing in AppBannerManagerTest to RunBannerTest() is another way of writing
my
> test. I like this way better because it is closer to how the production code
> works.
> 

I'm not really convinced of the closer to how production code works argument (if
we want to do that we'd also simulate user input and so forth).

https://codereview.chromium.org/2677853002/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_browsertest.cc:75:
AppBannerManager::WebContentsDestroyed();
On 2017/02/09 00:57:37, pkotwicz wrote:
> AppBannerManager::WebContentsDestroyed() always calls Stop() regardless of
> whether the banner was shown or not.
> 
> I am resetting |will_show_| to avoid failing on the assertion on line 57.
> 
> Previously this was not an issue because AppBannerManagerTest's lifetime was
> super short so AppBannerManagerTest::WebContentsDestroyed() was not called in
> tests

I'm not a huge fan of this (this is partially why I had such a short lifetime
for the test manager previously), but I don't want to block this CL.

[pkotwicz, 2017-02-09 16:17:03]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2017-02-09 16:17:11]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2017-02-20 04:48:43]

Dominick, can you please take another look?

I now pass the AppBannerManagerTest pointer to RunBannerTest()

[dominickn, 2017-02-20 04:57:47]

Still lgtm. Thanks!

[pkotwicz, 2017-02-21 01:20:57]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2017-02-21 01:21:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 02:28:43]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487640057778340,
"parent_rev": "1efb7901eafb3fa00849b5f380ef8409c663b1d2", "commit_rev":
"e082e1d1591776aff10d468bc7057dfa3e49abea"}

[commit-bot: I haz the power, 2017-02-21 02:29:19]

Description was changed from

==========
[Webapps]: Clear AppBannerManager::page_requested_prompt_ at start of banner
flow

This CL fixes a bug where a web page is not capable of cancelling the app banner
if the tab was previously navigated to a page which requested the banner in the
beforeinstallprompt() JS handler.

BUG=688902
TEST=AppBannerManagerBrowserTest.CancelBannerAfterPromptInHandler
==========

to

==========
[Webapps]: Clear AppBannerManager::page_requested_prompt_ at start of banner
flow

This CL fixes a bug where a web page is not capable of cancelling the app banner
if the tab was previously navigated to a page which requested the banner in the
beforeinstallprompt() JS handler.

BUG=688902
TEST=AppBannerManagerBrowserTest.CancelBannerAfterPromptInHandler

Review-Url: https://codereview.chromium.org/2677853002
Cr-Commit-Position: refs/heads/master@{#451689}
Committed:
https://chromium.googlesource.com/chromium/src/+/e082e1d1591776aff10d468bc705...
==========

[commit-bot: I haz the power, 2017-02-21 02:29:20]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e082e1d1591776aff10d468bc705...