New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 679822 link

Starred by 1 user
Status: WontFix
Owner:
█ chili@chromium.org
Last visit 22 days ago
Closed: Mar 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 679844
issue 680363


Sign in to add a comment

Clean up unused policy handles from Offliners

Project Member Reported by chili@chromium.org, Jan 10 2017 
Both prerendering_offliner and background_loader_offliner requires a OfflinePolicy object to be passed in for construction, yet this is never used/stored.

Should remove to clean up code

Comment 1 by chili@chromium.org, Jan 10 2017

Components: UI>Browser>Offline
Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 10 2017 

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

commit 73b5ae5acf62a62a0782eef02086bd7b2b2b3fe4
Author: chili <chili@chromium.org>
Date: Tue Jan 10 21:15:16 2017

[Offline Pages] Remove need for OfflinePolicy when constructing an Offliner.

BUG= 679822 

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

[modify] https://crrev.com/73b5ae5acf62a62a0782eef02086bd7b2b2b3fe4/chrome/browser/android/offline_pages/background_loader_offliner.cc
[modify] https://crrev.com/73b5ae5acf62a62a0782eef02086bd7b2b2b3fe4/chrome/browser/android/offline_pages/background_loader_offliner.h
[modify] https://crrev.com/73b5ae5acf62a62a0782eef02086bd7b2b2b3fe4/chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc
[modify] https://crrev.com/73b5ae5acf62a62a0782eef02086bd7b2b2b3fe4/chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc
[modify] https://crrev.com/73b5ae5acf62a62a0782eef02086bd7b2b2b3fe4/chrome/browser/android/offline_pages/prerendering_offliner.cc
[modify] https://crrev.com/73b5ae5acf62a62a0782eef02086bd7b2b2b3fe4/chrome/browser/android/offline_pages/prerendering_offliner.h
[modify] https://crrev.com/73b5ae5acf62a62a0782eef02086bd7b2b2b3fe4/chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc
[modify] https://crrev.com/73b5ae5acf62a62a0782eef02086bd7b2b2b3fe4/chrome/browser/android/offline_pages/request_coordinator_factory.cc
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 10 2017 

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

commit 8dbcb8957be2763be561ea510da5c4ed9c442da0
Author: chili <chili@chromium.org>
Date: Tue Jan 10 21:33:12 2017

Revert of [Offline Pages] Remove need for OfflinePolicy when constructing an Offliner. (patchset #1 id:1 of https://codereview.chromium.org/2624523003/ )

Reason for revert:
The policy might be used shortly. This may be too soon

Original issue's description:
> [Offline Pages] Remove need for OfflinePolicy when constructing an Offliner.
>
> BUG= 679822 
>
> Review-Url: https://codereview.chromium.org/2624523003
> Cr-Commit-Position: refs/heads/master@{#442687}
> Committed: https://chromium.googlesource.com/chromium/src/+/73b5ae5acf62a62a0782eef02086bd7b2b2b3fe4

TBR=dimich@chromium.org,petewil@chromium.org,dougarnett@chromium.org,dewittj@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 679822 

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

[modify] https://crrev.com/8dbcb8957be2763be561ea510da5c4ed9c442da0/chrome/browser/android/offline_pages/background_loader_offliner.cc
[modify] https://crrev.com/8dbcb8957be2763be561ea510da5c4ed9c442da0/chrome/browser/android/offline_pages/background_loader_offliner.h
[modify] https://crrev.com/8dbcb8957be2763be561ea510da5c4ed9c442da0/chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc
[modify] https://crrev.com/8dbcb8957be2763be561ea510da5c4ed9c442da0/chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc
[modify] https://crrev.com/8dbcb8957be2763be561ea510da5c4ed9c442da0/chrome/browser/android/offline_pages/prerendering_offliner.cc
[modify] https://crrev.com/8dbcb8957be2763be561ea510da5c4ed9c442da0/chrome/browser/android/offline_pages/prerendering_offliner.h
[modify] https://crrev.com/8dbcb8957be2763be561ea510da5c4ed9c442da0/chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc
[modify] https://crrev.com/8dbcb8957be2763be561ea510da5c4ed9c442da0/chrome/browser/android/offline_pages/request_coordinator_factory.cc

Comment 4 by kbr@chromium.org, Jan 14 2017

Blockedon: 679844

Comment 5 by romax@chromium.org, Jan 26 2017

Blockedon: 680363
if we decide to land the patch for 'snapshot on last timeout retry' we might have to keep the policy handle in offliner. adding a 'blocked on'.

Comment 6 by romax@chromium.org, Feb 28 2017 

Update:

680363 actually needs the policy handle to work, so we might not be able to remove the policy handle unless we're going to have a larger refactoring.
I'd suggest to close the bug as wontfix... :(

Comment 7 by chili@chromium.org, Mar 1 2017

Status: WontFix (was: Started)
The policy handles are now used, per comment #6
►

Sign in to add a comment