New issue
Advanced search Search tips

Issue 666165 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2016
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Prevent in-page navigations from stopping the app banner pipeline

Project Member Reported by dominickn@chromium.org, Nov 17 2016

Issue description

Triggering a fragment navigation or history.pushState action whilst the app banner pipeline is checking a page's eligibility will stop the pipeline. This can cause racey behaviour with banners. We should ignore these in-page navigations.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/803300091bf7d275a9456b0362e3a9ddb9f61abb

commit 803300091bf7d275a9456b0362e3a9ddb9f61abb
Author: dominickn <dominickn@chromium.org>
Date: Thu Nov 17 06:02:37 2016

Prevent in-page navigations from stopping the app banner pipeline.

Triggering a fragment navigation or history.pushState action whilst the
app banner pipeline is checking a page's eligibility will prevent the
banner from being shown even if the page is eligible. This is because
any main frame navigation currently stops the checking pipeline to
prevent the banner from being shown after a navigation.

This CL addresses the issue by ignoring in-page navigations when the
banner pipeline is active.

BUG= 666165 

Review-Url: https://codereview.chromium.org/2507163003
Cr-Commit-Position: refs/heads/master@{#432778}

[modify] https://crrev.com/803300091bf7d275a9456b0362e3a9ddb9f61abb/chrome/browser/banners/app_banner_manager.cc

Labels: Merge-Request-55
Requesting merge of #1 to M55. This has baked in Canary and I've verified the change manually.

Comment 3 by dimu@chromium.org, Nov 18 2016

Labels: -Merge-Request-55 Merge-Approved-55 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M55 (branch: 2883)
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 18 2016

Labels: -merge-approved-55 merge-merged-2883
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/65fc2a2547f3ce03aedf8bedbbec685029a76c2c

commit 65fc2a2547f3ce03aedf8bedbbec685029a76c2c
Author: Dominick Ng <dominickn@chromium.org>
Date: Fri Nov 18 18:59:02 2016

Prevent in-page navigations from stopping the app banner pipeline.

Triggering a fragment navigation or history.pushState action whilst the
app banner pipeline is checking a page's eligibility will prevent the
banner from being shown even if the page is eligible. This is because
any main frame navigation currently stops the checking pipeline to
prevent the banner from being shown after a navigation.

This CL addresses the issue by ignoring in-page navigations when the
banner pipeline is active.

BUG= 666165 

Review-Url: https://codereview.chromium.org/2507163003
Cr-Commit-Position: refs/heads/master@{#432778}
(cherry picked from commit 803300091bf7d275a9456b0362e3a9ddb9f61abb)

Review URL: https://codereview.chromium.org/2515793002 .

Cr-Commit-Position: refs/branch-heads/2883@{#613}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/65fc2a2547f3ce03aedf8bedbbec685029a76c2c/chrome/browser/banners/app_banner_manager.cc

Status: Fixed (was: Started)

Sign in to add a comment