New issue
Advanced search Search tips

Issue 649148 link

Starred by 2 users
Status: Fixed
Owner:
ryansturm@chromium.org
Closed: Nov 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 655239



Sign in to add a comment

Fix OfflinePreviews UMA logic that was recently regressed

Project Member Reported by ryansturm@chromium.org, Sep 21 2016 
https://crrev.com/f68c52f414b7b785720faf53458df2517e921c9e regressed the is_offline_preview() logic by moving the time the variable is set to DidFinishNavigation. The only consumer of is_offline_preview() is in a different DidFinishNavigation that occurs before the OfflinePreviewsTabHelper::DidFinishNavigation

Comment 1 by ryansturm@chromium.org, Oct 12 2016

Blocking: 655239
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 27 2016 

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

commit 21364834a65e541369e8895d93695dfaf114056b
Author: ryansturm <ryansturm@chromium.org>
Date: Thu Oct 27 01:13:01 2016

Showing previews UI for Offline Previews

This changes the functionality of is_offline_preview() in OfflinePageTabHelper to check the provisional information as well.

Additionally, this CL addresses other consumers of this information that
want access to it in DidFinishNavigation. Specifically,
PreviewsPageLoadMetricsObserver will access the is_offline_previews bit
as will PreviewsInfoBarHelper.

This also prevents showing the offline pages snackbar and replaces it with
the previews infobar. This leaves the Offline omnibox and other UI
features.

BUG=615564, 649148 

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

[modify] https://crrev.com/21364834a65e541369e8895d93695dfaf114056b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
[modify] https://crrev.com/21364834a65e541369e8895d93695dfaf114056b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java
[modify] https://crrev.com/21364834a65e541369e8895d93695dfaf114056b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
[modify] https://crrev.com/21364834a65e541369e8895d93695dfaf114056b/chrome/browser/android/offline_pages/offline_page_bridge.cc
[modify] https://crrev.com/21364834a65e541369e8895d93695dfaf114056b/chrome/browser/android/offline_pages/offline_page_bridge.h
[modify] https://crrev.com/21364834a65e541369e8895d93695dfaf114056b/chrome/browser/android/offline_pages/offline_page_tab_helper.cc
[modify] https://crrev.com/21364834a65e541369e8895d93695dfaf114056b/chrome/browser/android/offline_pages/offline_page_tab_helper.h
[modify] https://crrev.com/21364834a65e541369e8895d93695dfaf114056b/chrome/browser/android/offline_pages/offline_page_utils.cc
[modify] https://crrev.com/21364834a65e541369e8895d93695dfaf114056b/chrome/browser/android/offline_pages/offline_page_utils.h
[modify] https://crrev.com/21364834a65e541369e8895d93695dfaf114056b/chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc
[modify] https://crrev.com/21364834a65e541369e8895d93695dfaf114056b/chrome/browser/previews/previews_infobar_delegate.cc
[modify] https://crrev.com/21364834a65e541369e8895d93695dfaf114056b/chrome/browser/previews/previews_infobar_tab_helper.cc
[modify] https://crrev.com/21364834a65e541369e8895d93695dfaf114056b/chrome/browser/previews/previews_infobar_tab_helper.h
[modify] https://crrev.com/21364834a65e541369e8895d93695dfaf114056b/chrome/browser/previews/previews_infobar_tab_helper_unittest.cc

Comment 3 by ryansturm@chromium.org, Oct 27 2016

Status: Fixed (was: Started)
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 27 2016 

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

commit 7d6eaf7adcb3f61835051ae104fee8f61f2144ca
Author: alancutter <alancutter@chromium.org>
Date: Thu Oct 27 04:25:22 2016

Revert of Showing previews UI for Offline Previews (patchset #15 id:280001 of https://codereview.chromium.org/2362033002/ )

Reason for revert:
The following tests are failing on Android Tests:
org.chromium.chrome.browser.offlinepages.OfflinePageTabObserverTest#testOnConnectionTypeChanged_pageNotLoaded
org.chromium.chrome.browser.offlinepages.OfflinePageTabObserverTest#testOnDestroyed
org.chromium.chrome.browser.offlinepages.OfflinePageTabObserverTest#testOnPageLoadFinished_notConnected
org.chromium.chrome.browser.offlinepages.OfflinePageTabObserverTest#testOnPageLoadFinished
org.chromium.chrome.browser.offlinepages.OfflinePageTabObserverTest#testShowSnackbar_ignoreEventsAfterShownOnce
org.chromium.chrome.browser.offlinepages.OfflinePageTabObserverTest#testAddObserverForTab
org.chromium.chrome.browser.offlinepages.OfflinePageTabObserverTest#testOnUrlUpdated_whenSnackbarShown
org.chromium.chrome.browser.offlinepages.OfflinePageTabObserverTest#testOnHidden_afterSnackbarShown
org.chromium.chrome.browser.offlinepages.OfflinePageTabObserverTest#testOnConnectionTypeChanged_notConnected
org.chromium.chrome.browser.offlinepages.OfflinePageTabObserverTest#testShowSnackbar_onConnectionTypeChanged
org.chromium.chrome.browser.offlinepages.OfflinePageTabObserverTest#testAddObserverForTab_whenConnected
org.chromium.chrome.browser.offlinepages.OfflinePageTabObserverTest#testShowSnackbar_onShown
org.chromium.chrome.browser.offlinepages.OfflinePageTabObserverTest#testShowSnackbar_onPageLoadFinished

https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Tests/builds/33841

Original issue's description:
> Showing previews UI for Offline Previews
>
> This changes the functionality of is_offline_preview() in OfflinePageTabHelper to check the provisional information as well.
>
> Additionally, this CL addresses other consumers of this information that
> want access to it in DidFinishNavigation. Specifically,
> PreviewsPageLoadMetricsObserver will access the is_offline_previews bit
> as will PreviewsInfoBarHelper.
>
> This also prevents showing the offline pages snackbar and replaces it with
> the previews infobar. This leaves the Offline omnibox and other UI
> features.
>
> BUG=615564, 649148 
>
> Committed: https://crrev.com/21364834a65e541369e8895d93695dfaf114056b
> Cr-Commit-Position: refs/heads/master@{#427902}

TBR=bmcquade@chromium.org,jianli@chromium.org,megjablon@chromium.org,ryansturm@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=615564, 649148 

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

[modify] https://crrev.com/7d6eaf7adcb3f61835051ae104fee8f61f2144ca/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
[modify] https://crrev.com/7d6eaf7adcb3f61835051ae104fee8f61f2144ca/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java
[modify] https://crrev.com/7d6eaf7adcb3f61835051ae104fee8f61f2144ca/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
[modify] https://crrev.com/7d6eaf7adcb3f61835051ae104fee8f61f2144ca/chrome/browser/android/offline_pages/offline_page_bridge.cc
[modify] https://crrev.com/7d6eaf7adcb3f61835051ae104fee8f61f2144ca/chrome/browser/android/offline_pages/offline_page_bridge.h
[modify] https://crrev.com/7d6eaf7adcb3f61835051ae104fee8f61f2144ca/chrome/browser/android/offline_pages/offline_page_tab_helper.cc
[modify] https://crrev.com/7d6eaf7adcb3f61835051ae104fee8f61f2144ca/chrome/browser/android/offline_pages/offline_page_tab_helper.h
[modify] https://crrev.com/7d6eaf7adcb3f61835051ae104fee8f61f2144ca/chrome/browser/android/offline_pages/offline_page_utils.cc
[modify] https://crrev.com/7d6eaf7adcb3f61835051ae104fee8f61f2144ca/chrome/browser/android/offline_pages/offline_page_utils.h
[modify] https://crrev.com/7d6eaf7adcb3f61835051ae104fee8f61f2144ca/chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc
[modify] https://crrev.com/7d6eaf7adcb3f61835051ae104fee8f61f2144ca/chrome/browser/previews/previews_infobar_delegate.cc
[modify] https://crrev.com/7d6eaf7adcb3f61835051ae104fee8f61f2144ca/chrome/browser/previews/previews_infobar_tab_helper.cc
[modify] https://crrev.com/7d6eaf7adcb3f61835051ae104fee8f61f2144ca/chrome/browser/previews/previews_infobar_tab_helper.h
[modify] https://crrev.com/7d6eaf7adcb3f61835051ae104fee8f61f2144ca/chrome/browser/previews/previews_infobar_tab_helper_unittest.cc

Comment 5 by ryansturm@chromium.org, Oct 27 2016

Status: Started (was: Fixed)
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 27 2016 

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

commit e28ec590f2f8f5103fb724c587d3abc1a6782cb8
Author: ryansturm <ryansturm@chromium.org>
Date: Thu Oct 27 20:26:39 2016

Showing previews UI for Offline Previews

This changes the functionality of is_offline_preview() in OfflinePageTabHelper to check the provisional information as well.

Additionally, this CL addresses other consumers of this information that
want access to it in DidFinishNavigation. Specifically,
PreviewsPageLoadMetricsObserver will access the is_offline_previews bit
as will PreviewsInfoBarHelper.

This also prevents showing the offline pages snackbar and replaces it with
the previews infobar. This leaves the Offline omnibox and other UI
features.

BUG=615564, 649148 

Committed: https://crrev.com/21364834a65e541369e8895d93695dfaf114056b
Review-Url: https://codereview.chromium.org/2362033002
Cr-Original-Commit-Position: refs/heads/master@{#427902}
Cr-Commit-Position: refs/heads/master@{#428124}

[modify] https://crrev.com/e28ec590f2f8f5103fb724c587d3abc1a6782cb8/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
[modify] https://crrev.com/e28ec590f2f8f5103fb724c587d3abc1a6782cb8/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java
[modify] https://crrev.com/e28ec590f2f8f5103fb724c587d3abc1a6782cb8/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
[modify] https://crrev.com/e28ec590f2f8f5103fb724c587d3abc1a6782cb8/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java
[modify] https://crrev.com/e28ec590f2f8f5103fb724c587d3abc1a6782cb8/chrome/browser/android/offline_pages/offline_page_bridge.cc
[modify] https://crrev.com/e28ec590f2f8f5103fb724c587d3abc1a6782cb8/chrome/browser/android/offline_pages/offline_page_bridge.h
[modify] https://crrev.com/e28ec590f2f8f5103fb724c587d3abc1a6782cb8/chrome/browser/android/offline_pages/offline_page_tab_helper.cc
[modify] https://crrev.com/e28ec590f2f8f5103fb724c587d3abc1a6782cb8/chrome/browser/android/offline_pages/offline_page_tab_helper.h
[modify] https://crrev.com/e28ec590f2f8f5103fb724c587d3abc1a6782cb8/chrome/browser/android/offline_pages/offline_page_utils.cc
[modify] https://crrev.com/e28ec590f2f8f5103fb724c587d3abc1a6782cb8/chrome/browser/android/offline_pages/offline_page_utils.h
[modify] https://crrev.com/e28ec590f2f8f5103fb724c587d3abc1a6782cb8/chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc
[modify] https://crrev.com/e28ec590f2f8f5103fb724c587d3abc1a6782cb8/chrome/browser/previews/previews_infobar_delegate.cc
[modify] https://crrev.com/e28ec590f2f8f5103fb724c587d3abc1a6782cb8/chrome/browser/previews/previews_infobar_tab_helper.cc
[modify] https://crrev.com/e28ec590f2f8f5103fb724c587d3abc1a6782cb8/chrome/browser/previews/previews_infobar_tab_helper.h
[modify] https://crrev.com/e28ec590f2f8f5103fb724c587d3abc1a6782cb8/chrome/browser/previews/previews_infobar_tab_helper_unittest.cc
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 28 2016 

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

commit a438c4bcb58e59d72727dbf83a2e9251a949a9a2
Author: phoglund <phoglund@chromium.org>
Date: Fri Oct 28 09:40:31 2016

Revert of Showing previews UI for Offline Previews (patchset #17 id:320001 of https://codereview.chromium.org/2362033002/ )

Reason for revert:
Speculative revert: might have broken a bunch of infobar tests. This CL is the only infobar-related one in the blamelist.  https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Tests%20%28dbg%29/builds/36738

For instance org.chromium.chrome.browser.infobar.InfoBarContainerTest#testCloseTabOnAdd:

junit.framework.AssertionFailedError: expected:<2> but was:<1>
	at org.chromium.chrome.browser.infobar.InfoBarContainerTest.addInfoBarToCurrentTab(InfoBarContainerTest.java:103)
	at org.chromium.chrome.browser.infobar.InfoBarContainerTest.testCloseTabOnAdd(InfoBarContainerTest.java:223)

Original issue's description:
> Showing previews UI for Offline Previews
>
> This changes the functionality of is_offline_preview() in OfflinePageTabHelper to check the provisional information as well.
>
> Additionally, this CL addresses other consumers of this information that
> want access to it in DidFinishNavigation. Specifically,
> PreviewsPageLoadMetricsObserver will access the is_offline_previews bit
> as will PreviewsInfoBarHelper.
>
> This also prevents showing the offline pages snackbar and replaces it with
> the previews infobar. This leaves the Offline omnibox and other UI
> features.
>
> BUG=615564, 649148 
>
> Committed: https://crrev.com/21364834a65e541369e8895d93695dfaf114056b
> Committed: https://crrev.com/e28ec590f2f8f5103fb724c587d3abc1a6782cb8
> Cr-Original-Commit-Position: refs/heads/master@{#427902}
> Cr-Commit-Position: refs/heads/master@{#428124}

TBR=bmcquade@chromium.org,jianli@chromium.org,megjablon@chromium.org,dewittj@chromium.org,ryansturm@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=615564, 649148 

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

[modify] https://crrev.com/a438c4bcb58e59d72727dbf83a2e9251a949a9a2/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
[modify] https://crrev.com/a438c4bcb58e59d72727dbf83a2e9251a949a9a2/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java
[modify] https://crrev.com/a438c4bcb58e59d72727dbf83a2e9251a949a9a2/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
[modify] https://crrev.com/a438c4bcb58e59d72727dbf83a2e9251a949a9a2/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java
[modify] https://crrev.com/a438c4bcb58e59d72727dbf83a2e9251a949a9a2/chrome/browser/android/offline_pages/offline_page_bridge.cc
[modify] https://crrev.com/a438c4bcb58e59d72727dbf83a2e9251a949a9a2/chrome/browser/android/offline_pages/offline_page_bridge.h
[modify] https://crrev.com/a438c4bcb58e59d72727dbf83a2e9251a949a9a2/chrome/browser/android/offline_pages/offline_page_tab_helper.cc
[modify] https://crrev.com/a438c4bcb58e59d72727dbf83a2e9251a949a9a2/chrome/browser/android/offline_pages/offline_page_tab_helper.h
[modify] https://crrev.com/a438c4bcb58e59d72727dbf83a2e9251a949a9a2/chrome/browser/android/offline_pages/offline_page_utils.cc
[modify] https://crrev.com/a438c4bcb58e59d72727dbf83a2e9251a949a9a2/chrome/browser/android/offline_pages/offline_page_utils.h
[modify] https://crrev.com/a438c4bcb58e59d72727dbf83a2e9251a949a9a2/chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc
[modify] https://crrev.com/a438c4bcb58e59d72727dbf83a2e9251a949a9a2/chrome/browser/previews/previews_infobar_delegate.cc
[modify] https://crrev.com/a438c4bcb58e59d72727dbf83a2e9251a949a9a2/chrome/browser/previews/previews_infobar_tab_helper.cc
[modify] https://crrev.com/a438c4bcb58e59d72727dbf83a2e9251a949a9a2/chrome/browser/previews/previews_infobar_tab_helper.h
[modify] https://crrev.com/a438c4bcb58e59d72727dbf83a2e9251a949a9a2/chrome/browser/previews/previews_infobar_tab_helper_unittest.cc
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 28 2016 

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

commit ad9e85d3ae876b9531b7850d120a3f363ea0ad34
Author: ryansturm <ryansturm@chromium.org>
Date: Fri Oct 28 17:28:06 2016

Showing previews UI for Offline Previews

This changes the functionality of is_offline_preview() in
OfflinePageTabHelper to check the provisional information as well.

Additionally, this CL addresses other consumers of this information that
want access to it in DidFinishNavigation. Specifically,
PreviewsPageLoadMetricsObserver will access the is_offline_previews bit
as will PreviewsInfoBarHelper.

This also prevents showing the offline pages snackbar and replaces it
with
the previews infobar. This leaves the Offline omnibox and other UI
features.

This is a re-land of https://codereview.chromium.org/2362033002/

BUG=615564,  649148 

TBR=bmcquade

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

[modify] https://crrev.com/ad9e85d3ae876b9531b7850d120a3f363ea0ad34/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
[modify] https://crrev.com/ad9e85d3ae876b9531b7850d120a3f363ea0ad34/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java
[modify] https://crrev.com/ad9e85d3ae876b9531b7850d120a3f363ea0ad34/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
[modify] https://crrev.com/ad9e85d3ae876b9531b7850d120a3f363ea0ad34/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java
[modify] https://crrev.com/ad9e85d3ae876b9531b7850d120a3f363ea0ad34/chrome/browser/android/offline_pages/offline_page_bridge.cc
[modify] https://crrev.com/ad9e85d3ae876b9531b7850d120a3f363ea0ad34/chrome/browser/android/offline_pages/offline_page_bridge.h
[modify] https://crrev.com/ad9e85d3ae876b9531b7850d120a3f363ea0ad34/chrome/browser/android/offline_pages/offline_page_tab_helper.cc
[modify] https://crrev.com/ad9e85d3ae876b9531b7850d120a3f363ea0ad34/chrome/browser/android/offline_pages/offline_page_tab_helper.h
[modify] https://crrev.com/ad9e85d3ae876b9531b7850d120a3f363ea0ad34/chrome/browser/android/offline_pages/offline_page_utils.cc
[modify] https://crrev.com/ad9e85d3ae876b9531b7850d120a3f363ea0ad34/chrome/browser/android/offline_pages/offline_page_utils.h
[modify] https://crrev.com/ad9e85d3ae876b9531b7850d120a3f363ea0ad34/chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc
[modify] https://crrev.com/ad9e85d3ae876b9531b7850d120a3f363ea0ad34/chrome/browser/previews/previews_infobar_delegate.cc
[modify] https://crrev.com/ad9e85d3ae876b9531b7850d120a3f363ea0ad34/chrome/browser/previews/previews_infobar_tab_helper.cc
[modify] https://crrev.com/ad9e85d3ae876b9531b7850d120a3f363ea0ad34/chrome/browser/previews/previews_infobar_tab_helper.h
[modify] https://crrev.com/ad9e85d3ae876b9531b7850d120a3f363ea0ad34/chrome/browser/previews/previews_infobar_tab_helper_unittest.cc

Comment 9 by ryansturm@chromium.org, Nov 10 2016

Status: Fixed (was: Started)
►

Sign in to add a comment