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

Issue 879801 link

Starred by 1 user
Status: Fixed
Owner:
carlosk@chromium.org
Closed: Sep 10
Cc:
dewittj@chromium.org
█ dim...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Offline Pages namespaces with tab restriction must save tab id into ClientId::id for their pages

Project Member Reported by carlosk@chromium.org, Sep 1 
This is an un-reported requirement that is required for the tab-restricted policy to actually work (otherwise no pages from this namespace would ever be loaded (see OnGetPagesByURLDone implementation [1]).

As we recently considered adding another namespace that would be tab-restricted, we would have been struck by this problem should this convention be forgotten.

I'll start by documenting the respective policy members to clarify this requirement but we might want to think about a better way to handle this.

[1] https://cs.chromium.org/chromium/src/chrome/browser/offline_pages/offline_page_utils.cc?q=OnGetPagesByURLDone
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 6 

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

commit 78525669b83bd8ce96b965b7c77bf4d33e52481b
Author: Carlos Knippschild <carlosk@chromium.org>
Date: Thu Sep 06 23:45:47 2018

Clarify offline pages policy for tab restriction

Offline Pages namespaces can be configured to only allow its pages to be
presented in their original tabs. This is enforced using an undeclared
convention that pages for these namespaces will store the tab id value
in the ClientId::id field for their pages. This change makes this
convention more explicit.

Bug:  879801 
Change-Id: I3ab3d871abb41d6de9112c55d8b3d8b6491c7386
Reviewed-on: https://chromium-review.googlesource.com/1200389
Commit-Queue: Carlos Knippschild <carlosk@chromium.org>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589369}
[modify] https://crrev.com/78525669b83bd8ce96b965b7c77bf4d33e52481b/chrome/browser/offline_pages/offline_page_utils.cc
[modify] https://crrev.com/78525669b83bd8ce96b965b7c77bf4d33e52481b/components/offline_pages/core/client_policy_controller.cc
[modify] https://crrev.com/78525669b83bd8ce96b965b7c77bf4d33e52481b/components/offline_pages/core/client_policy_controller.h
[modify] https://crrev.com/78525669b83bd8ce96b965b7c77bf4d33e52481b/components/offline_pages/core/client_policy_controller_unittest.cc
[modify] https://crrev.com/78525669b83bd8ce96b965b7c77bf4d33e52481b/components/offline_pages/core/offline_page_client_policy.h

Comment 2 by dim...@chromium.org, Sep 10

Owner: carlosk@chromium.org
Status: Assigned (was: Untriaged)

Comment 3 by carlosk@chromium.org, Sep 10

Cc: dewittj@chromium.org
Status: Fixed (was: Assigned)
The landed change already makes it clear enough for when another namespace is created to use that feature. Should we actually have more cases where it's used we might then consider a more thought out solution.
►

Sign in to add a comment