This should already be the case. Assigning to ryansturm to verify.

Hmm. This is interesting. I suppose for a page that is automatically being downloaded, we should disable all previews entirely. However, for a page that is manually downloaded, we should download the page as requested. I think this is pretty complicated to decide between the two an I don't understand the cases when the page is automatically downloaded.

I believe loading the page with PREVIEWS_NO_TRANSFORM will prevent this is largely accessible through ReloadType::DISABLE_LOFI_MODE. However, offline previews does not know about the PREVIEWS_NO_TRANSFORM flag (this check could be added in offline_page_request_job.cc GetNetworkState(). Note, that PREVIEWS_NO_TRANSFORM should be set by the mechanism that loads offline pages in the background when automatic downloading is occurring.

Does the background offliner run on effectively slow networks (2G) or only on 3G+? Certainly there could be a speed change that could affect the decision, but that might be somewhat rare.

Our BackgroundLoaderOffliner and PrerenderingOffliner run on all network types, including 2G, 3G, 4G, and WiFi.

Hmm. I'll look into a fix. Can you give me pointers to where the offliners (the ones that happen automatically) begin their navigations?

BackgroundLoaderOffliner: https://cs.chromium.org/chromium/src/chrome/browser/android/offline_pages/background_loader_offliner.cc?q=LoadAndSave+package:%5Echromium$&l=193

PrerenderingOffliner:
https://cs.chromium.org/chromium/src/chrome/browser/android/offline_pages/prerendering_offliner.cc?q=LoadAndSave+package:%5Echromium$&l=251

My proposal is to change the LoadURL() call in BackgroundLoaderContents::LoadPage to be LoadURLWithParams. Additionally, I will add a disable_previews bool to LoadURLParams and NavigationEntryImpl. 


PREVIEWS_NO_TRANSFORM will be set here:
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigator_impl.cc?type=cs&l=366

Additionally, PreviewsDecider ShouldAllowPreview will check for PREVIEWS_NO_TRANSFORM on the request via a content/ interface. Note: LoFi and LitePages already check this, so this final part would generalize that code for Offline and future previews.

We should also change the prerenderer (prerendering_loader.cc, prerendering_offliner.cc) since it is still likely in use for several milestones to come.

re: Comment 7

Looks like some state needs to plumbed in, but it should be possible.

+dimich, bengr

I doubt I will be able to land a full fix for this until at least M-61. I outlined an approach above that I partially coded up here (note: this doesn't build): https://codereview.chromium.org/2882333002/.

If this is high priority, it would be good to get a new owner for this.

Offline Pages is already shipping, it would be great if we were not broken.
Are the page previews already shipping?  If not, what milestone do they plan to ship?  It may not be too bad if the user has to turn on previews manually, and if they do then they get previvew versions of offline pages.  However, it would be bad if we ran a finch experiment breaking offline pages.

LoFi and Weblite have been launched since ~M48 and ~M52, which are the two previews that will likely cause this problem.

Since WebContents is a base::SupportsUserData, we can easily add an object to it when we create one for offlining. Would the previews be able to query it from WebContents? This way, no plumbing is needed.

In fact, the BackgroundLoaderOffliner already has one, we would just need to add it to the PrerenderingOffliner, and you could easily check.

Unfortunately, these decisions are made on a the IO thread, not the UI thread and the abstraction layers are all content-based, so there isn't a clean way to opaquely get information from the UI thread content/ path to the IO thread content/ path. The previews reload type is converted to a previews state in content/ and added to the main frame URLReques. If the previews state is PREVIEWS_UNSPECIFIED, ChromeResourceDispatcherHostDelegate::GetPreviewsState will return whichever previews state it deems appropriate.

petewil/dimich: If you really need this by M-59 you should take ownership of this issue and do the work yourselves. We can help out, but in the M-61/62 timeframe.

cc +mdw (he asked to be added to cc)

Hi folks, I just became aware of this issue (thanks Pete!).

I am not sure this is an urgent need for M59, but that depends on whether we're planning to ramp up the previews triggering rate. If so, I'd be worried that we're going to break more offline pages (though, the intersection of previews triggering rate and pages being offlined may be small enough that it's OK for now). +talo and +aposner to confirm.

For this bug we're talking about any kind of treatment to the page that might make the rendered offline page not useful to the user. This includes WebLite Previews (server-side) and Client-Side Previews (client-side). I do not think Lo-Fi (client- or server-side) is a real problem. HD Previews is not either.

From the discussion with Pete and the Paquete team yeaterday, I think there are four cases to consider:

(1) A user manually downloads a page (hitting the download button in the three-dots menu).

(2) A page is downloaded in the background via the 'Download later' button on the Dino page.

(3) A page is downloaded via the 'Download link' button in the link context menu.

(4) A page is automatically offlined in the background (for tab restore).

For (1), the user is looking at the page they are downloading so we might imagine they will understand what is going to happen. We *could* warn the user if they attempt to download a Preview page, but it's a funky UX flow and I'm not sure we want to add another UI action. We could instead automatically download the full page, but that will consume mobile data and may fail on a slow network (which is why the Preview was shown in the first place). My thinking is that we just save what's been rendered even if it's a Preview.

For (2) and (3), my thinking is that we should prefer not to use Previews (but possibly Lo-Fi) for these cases, on the presumption that the user will likely view the resulting page in an offline state. (I'm also curious whether we should consider using the Offline Page Service for these cases, since presumably they will be lighter-weight and faster/more robust to load.)

For (4), this is s best-effort thing, and we don't want to impose additional overhead, so what gets offlined should be whatever is rendered in the tab, even if it's a Preview.

Agreed that this shouldn't be blocking for M59 given the current coverage rate, though Ariel should have the latest numbers!

The planned behavior sgtm!

Agree with M59 (assuming triggering rate is not very high).

As for behaviors, for (1) I agree the user understands they see Preview, except the one really unfortunate case when it is the Preview that stops loading content below fold. Since user does not see that, they are likely disappointed when snapshot only contains the top portion. It effectively makes offline page not much better than a screenshot.

This specific kind of preview is somewhat troublesome. Others (like using weblite and LoFi) are fine, since they are visible to users and therefore do not surprise.

Is this specific kind of Preview shipping in M59?

If we defer the loading of content that's below the fold, we will provide an API to force-continue loading that content. An in general I think we should provide a way to back out of not-yet-visible interventions when possible.

Otherwise, I agree with comment #18.

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

commit 61b63cbc244f348331c91d78402eb843bd185408
Author: petewil <petewil@chromium.org>
Date: Sat Jun 10 00:58:41 2017

Add UMA to determine how often we offline a page with previews.

We suspect that page previews may be causing problems with offline
pages.  To understand the severity of the problem, we'll add a new
histogram that measures how often we turn on previews while offlining
a page.

BUG= 703875 

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

[modify] https://crrev.com/61b63cbc244f348331c91d78402eb843bd185408/chrome/browser/loader/chrome_navigation_data.cc
[modify] https://crrev.com/61b63cbc244f348331c91d78402eb843bd185408/chrome/browser/loader/chrome_navigation_data.h
[modify] https://crrev.com/61b63cbc244f348331c91d78402eb843bd185408/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
[modify] https://crrev.com/61b63cbc244f348331c91d78402eb843bd185408/chrome/browser/offline_pages/background_loader_offliner.cc
[modify] https://crrev.com/61b63cbc244f348331c91d78402eb843bd185408/chrome/browser/offline_pages/background_loader_offliner_unittest.cc
[modify] https://crrev.com/61b63cbc244f348331c91d78402eb843bd185408/content/public/common/previews_state.h
[modify] https://crrev.com/61b63cbc244f348331c91d78402eb843bd185408/tools/metrics/histograms/histograms.xml

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

commit c008404b493771a604b34d85040bd8cb09e1342c
Author: Pete Williamson <petewil@chromium.org>
Date: Thu Jun 29 18:13:32 2017

Mark OffliningPreviewStatus as a namespaced histogram

The original change list didn't mark it as a namespaced histogram,
so data in the namespace was not visible in the UMA dashboard.

Bug:  703875 
Change-Id: Ica303c6fd454227a1468cbba0b86677b52007747
Reviewed-on: https://chromium-review.googlesource.com/552479
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Peter Williamson <petewil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483424}
[modify] https://crrev.com/c008404b493771a604b34d85040bd8cb09e1342c/tools/metrics/histograms/histograms.xml

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

commit 89aa1402e01e28ebe8a14180dff714cc8b15a34f
Author: Pete Williamson <petewil@chromium.org>
Date: Fri Jul 07 19:58:41 2017

Prevent Previews from interfering with Offline Pages

Some previews can cause problems for offlining. We should disable
those previews.  This approach uses a NavigationDelegate and a
WebContentsDelegate to allow the background offliner to disable
previews that could cause problems for offlining.

Bug:  703875 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I519fae1b6f98da99c89ff97cb4b201c60a92b176
Reviewed-on: https://chromium-review.googlesource.com/544586
Commit-Queue: Peter Williamson <petewil@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Cathy Li <chili@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485014}
[modify] https://crrev.com/89aa1402e01e28ebe8a14180dff714cc8b15a34f/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
[modify] https://crrev.com/89aa1402e01e28ebe8a14180dff714cc8b15a34f/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h
[modify] https://crrev.com/89aa1402e01e28ebe8a14180dff714cc8b15a34f/components/offline_pages/content/background_loader/background_loader_contents.cc
[modify] https://crrev.com/89aa1402e01e28ebe8a14180dff714cc8b15a34f/components/offline_pages/content/background_loader/background_loader_contents.h
[modify] https://crrev.com/89aa1402e01e28ebe8a14180dff714cc8b15a34f/components/offline_pages/content/background_loader/background_loader_contents_unittest.cc
[modify] https://crrev.com/89aa1402e01e28ebe8a14180dff714cc8b15a34f/content/browser/frame_host/navigator_delegate.cc
[modify] https://crrev.com/89aa1402e01e28ebe8a14180dff714cc8b15a34f/content/browser/frame_host/navigator_delegate.h
[modify] https://crrev.com/89aa1402e01e28ebe8a14180dff714cc8b15a34f/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/89aa1402e01e28ebe8a14180dff714cc8b15a34f/content/browser/loader/mojo_async_resource_handler_unittest.cc
[modify] https://crrev.com/89aa1402e01e28ebe8a14180dff714cc8b15a34f/content/browser/loader/resource_dispatcher_host_browsertest.cc
[modify] https://crrev.com/89aa1402e01e28ebe8a14180dff714cc8b15a34f/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/89aa1402e01e28ebe8a14180dff714cc8b15a34f/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/89aa1402e01e28ebe8a14180dff714cc8b15a34f/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/89aa1402e01e28ebe8a14180dff714cc8b15a34f/content/public/browser/resource_dispatcher_host_delegate.cc
[modify] https://crrev.com/89aa1402e01e28ebe8a14180dff714cc8b15a34f/content/public/browser/resource_dispatcher_host_delegate.h
[modify] https://crrev.com/89aa1402e01e28ebe8a14180dff714cc8b15a34f/content/public/browser/web_contents_delegate.h
[modify] https://crrev.com/89aa1402e01e28ebe8a14180dff714cc8b15a34f/content/public/common/previews_state.h

Is this still an issue? Can this bug be closed?

Fixed as of July 2017.