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

Issue 610824 link

Starred by 1 user
Status: Fixed
Owner:
fgor...@chromium.org
Closed: Sep 2016
Cc:
jianli@chromium.org
dewittj@chromium.org
█ dim...@chromium.org
fgor...@chromium.org
dougarnett@chromium.org
█ romax@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Extract OfflinePageModel interface

Project Member Reported by fgor...@chromium.org, May 10 2016 
Extract OfflinePageModel interface to enable better testability and break circular dependencies that happen, when adding functionality, e.g. OfflinePageStorageManager.

Plan:
* Extract callbacks and enums
* Extract async methods to separate interface
* Once all clients converted to async, switch all of them to the interface
* Hide the implementation
* Create a fake implementation for testing.
Project Member

Comment 1 by bugdroid1@chromium.org, May 11 2016 

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

commit fe8a72da89b1941cccdd54a0da548540360286a6
Author: fgorski <fgorski@chromium.org>
Date: Wed May 11 20:24:49 2016

[Offline pages] Extracting callbacks from the Offline Page Model

This patch:
* Moves the callbacks and enums from offline page model
* Updates all of the consumers of the callbacks and enums

BUG= 610824 

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

[modify] https://crrev.com/fe8a72da89b1941cccdd54a0da548540360286a6/chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc
[modify] https://crrev.com/fe8a72da89b1941cccdd54a0da548540360286a6/chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
[modify] https://crrev.com/fe8a72da89b1941cccdd54a0da548540360286a6/components/offline_pages.gypi
[modify] https://crrev.com/fe8a72da89b1941cccdd54a0da548540360286a6/components/offline_pages/BUILD.gn
[modify] https://crrev.com/fe8a72da89b1941cccdd54a0da548540360286a6/components/offline_pages/offline_page_model.cc
[modify] https://crrev.com/fe8a72da89b1941cccdd54a0da548540360286a6/components/offline_pages/offline_page_model.h
[modify] https://crrev.com/fe8a72da89b1941cccdd54a0da548540360286a6/components/offline_pages/offline_page_model_unittest.cc
[modify] https://crrev.com/fe8a72da89b1941cccdd54a0da548540360286a6/components/offline_pages/offline_page_storage_manager.cc
[modify] https://crrev.com/fe8a72da89b1941cccdd54a0da548540360286a6/components/offline_pages/offline_page_storage_manager_unittest.cc
[add] https://crrev.com/fe8a72da89b1941cccdd54a0da548540360286a6/components/offline_pages/offline_page_types.h

Comment 2 by dougarnett@chromium.org, May 27 2016

Cc: dougarnett@chromium.org
I had opened separate bugs for pieces of this one that we could either treat as dups or sub-bugs: 

Split interface and impl:
https://bugs.chromium.org/p/chromium/issues/detail?id=614864

Some cleanups identified in: https://bugs.chromium.org/p/chromium/issues/detail?id=615165
Project Member

Comment 3 by bugdroid1@chromium.org, May 31 2016 

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

commit 76f2e5a378d9d1ab6daf69cb77723adf6d2c40eb
Author: dougarnett <dougarnett@chromium.org>
Date: Tue May 31 19:07:09 2016

Splits the OfflinePageModel into and interface and and implementation class.

Tried to make mostly straightforward split for this iteration. Some static members and methods
were interesting deal with and we might consider better location/resolution to follow-up on.

BUG= 614864 , 610824 

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

[modify] https://crrev.com/76f2e5a378d9d1ab6daf69cb77723adf6d2c40eb/chrome/browser/android/offline_pages/offline_page_model_factory.cc
[modify] https://crrev.com/76f2e5a378d9d1ab6daf69cb77723adf6d2c40eb/chrome/browser/android/offline_pages/test_offline_page_model_builder.cc
[modify] https://crrev.com/76f2e5a378d9d1ab6daf69cb77723adf6d2c40eb/components/components_tests.gyp
[modify] https://crrev.com/76f2e5a378d9d1ab6daf69cb77723adf6d2c40eb/components/offline_pages.gypi
[modify] https://crrev.com/76f2e5a378d9d1ab6daf69cb77723adf6d2c40eb/components/offline_pages/BUILD.gn
[modify] https://crrev.com/76f2e5a378d9d1ab6daf69cb77723adf6d2c40eb/components/offline_pages/offline_page_model.cc
[modify] https://crrev.com/76f2e5a378d9d1ab6daf69cb77723adf6d2c40eb/components/offline_pages/offline_page_model.h
[add] https://crrev.com/76f2e5a378d9d1ab6daf69cb77723adf6d2c40eb/components/offline_pages/offline_page_model_impl.cc
[add] https://crrev.com/76f2e5a378d9d1ab6daf69cb77723adf6d2c40eb/components/offline_pages/offline_page_model_impl.h
[rename] https://crrev.com/76f2e5a378d9d1ab6daf69cb77723adf6d2c40eb/components/offline_pages/offline_page_model_impl_unittest.cc

Comment 4 by dougarnett@chromium.org, Jun 2 2016 

Re: * Create a fake implementation for testing.

Patch https://codereview.chromium.org/2016313003/ added an empty implementation as StubOfflinePageModel. It also has an example of extending this Stub with the SavePage method mocked as MockOfflinePageModel in chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc

Comment 5 by dougarnett@chromium.org, Jun 2 2016

Cc: romax@chromium.org fgor...@chromium.org dim...@chromium.org
 Issue 615165  has been merged into this issue.

Comment 7 by dewittj@chromium.org, Sep 23 2016

Status: Fixed (was: Assigned)
►

Sign in to add a comment