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

Issue 641053 link

Starred by 1 user
Status: Fixed
Owner:
█ chili@chromium.org
Last visit 22 days ago
Closed: Oct 2016
Cc:
dewittj@chromium.org
█ dim...@chromium.org
fgor...@chromium.org
vitaliii@chromium.org
treib@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Introduce functions to classify namespaces

Project Member Reported by fgor...@chromium.org, Aug 25 2016 
Based on this comment:
Any chance you can make a function NamespaceIsUsedForDownloads (or similar) in the client_namespace_constants file so that anyone adding a namespace knows whether to consider this case?

Comment 1 by fgor...@chromium.org, Aug 25 2016

Description: Show this description

Comment 2 by fgor...@chromium.org, Aug 29 2016

Cc: dim...@chromium.org
Another idea: How about we handle that through policy.

Comment 3 by dim...@chromium.org, Aug 29 2016 

As a long-term plan, we need to reduce usage of namespaces and check against policy bits.

Comment 4 by dim...@chromium.org, Aug 29 2016

Labels: -Pri-3 Pri-2
Owner: chili@chromium.org
Status: Assigned (was: Untriaged)

Comment 5 by chili@chromium.org, Aug 30 2016 

What kind of features do we have currently?  Can you provide a list of features and the namespaces that apply?

For example, 
Downloads
 - async
 - bookmark
 - download
 - ?

??

Comment 6 by dim...@chromium.org, Aug 30 2016 

There is no ready list... I'd go through the places in code that use constants like kAsyncNamespace and compiled a list. The usage can be deducted from context of those calls, but I'd recommend to attempt to clarify (and maybe dedupe), for example:

Usage here is under "IsUserRequestedPage()" kind of method: https://cs.chromium.org/chromium/src/components/offline_pages/offline_page_model_impl.cc?rcl=1472570131&l=1064

However the only usage of that method is in check of whether we need to remove the page when user clears caches: 
https://cs.chromium.org/chromium/src/components/offline_pages/offline_page_model_impl.cc?rcl=1472570131&l=397

From this usage, I'd be inclined to at least name the new bit in the policy as "RemovePageOnChromeCacheReset" rather than "UserRequested", to more directly reflect what it is used for.

Comment 8 by treib@chromium.org, Sep 20 2016

Cc: treib@chromium.org vitaliii@chromium.org
Project Member

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

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

commit f3de4523eff1856d2fe99c80a76bb7692d1e8beb
Author: chili <chili@chromium.org>
Date: Thu Oct 06 02:33:11 2016

[Offline pages] Use the new policy bits

BUG= 641053 

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

[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/background/request_coordinator.cc
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/background/request_coordinator.h
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/downloads/download_notifying_observer.cc
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/downloads/download_notifying_observer.h
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/downloads/download_notifying_observer_unittest.cc
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/downloads/download_ui_adapter.cc
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/downloads/download_ui_adapter.h
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/downloads/download_ui_adapter_unittest.cc
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/offline_page_model_impl.cc
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/offline_page_model_impl.h

Comment 10 by chili@chromium.org, Oct 26 2016

Status: Fixed (was: Assigned)
We have some work to do to unify the Client policy objects (currently there's one on both the request_coordinator and the offline_page_model), but the core work for the original bug is completed.
Project Member

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

Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f3de4523eff1856d2fe99c80a76bb7692d1e8beb

commit f3de4523eff1856d2fe99c80a76bb7692d1e8beb
Author: chili <chili@chromium.org>
Date: Thu Oct 06 02:33:11 2016

[Offline pages] Use the new policy bits

BUG= 641053 

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

[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/background/request_coordinator.cc
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/background/request_coordinator.h
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/downloads/download_notifying_observer.cc
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/downloads/download_notifying_observer.h
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/downloads/download_notifying_observer_unittest.cc
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/downloads/download_ui_adapter.cc
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/downloads/download_ui_adapter.h
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/downloads/download_ui_adapter_unittest.cc
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/offline_page_model_impl.cc
[modify] https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb/components/offline_pages/offline_page_model_impl.h

Comment 12 by dimu@google.com, Nov 4 2016

Labels: -merge-merged-2840
[Automated comment] removing mislabelled merge-merged-2840
►

Sign in to add a comment