Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1970423002:
[Offline Pages] Clear offline pages when user clears cache. (Closed)
Description[Offline Pages] Clear offline pages when user clears cache.
Since offline pages are no longer associated with bookmark, we're considering them as 'cache' instead of 'site data'. Hence making the change to correct the behavior when removing browsing data, move REMOVE_OFFLINE_PAGE_DATA from REMOVE_SITE_DATA and add REMOVE_CACHE as a condition when deciding to remove offline pages.
BUG=611225
Committed: https://crrev.com/4150bae6fb7993b3675e8430dc65fd941268aa0b
Cr-Commit-Position: refs/heads/master@{#393702}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Adding comments. #
Total comments: 1
Patch Set 3 : fix typo #Patch Set 4 : removing unused enum #Patch Set 5 : typo #
Messages
Total messages: 21 (7 generated)
romax@chromium.org changed reviewers: + fgorski@chromium.org, michaeln@chromium.org
Removing REMOVE_OFFLINE_PAGE from REMOVE_SITE_DATA, and if user is clearing cache, we clear offline page. Since we would treat offline pages as cache for now(user cannot control the behaviors of offline pages). In this case, when user tries to clear browsing data -> 'Cached images and files', we would clear offline pages, instead of 'Cookies and site data'.
msramek@chromium.org changed reviewers: + dvadym@chromium.org, msramek@chromium.org
Drive-by! Clarifying question: Does this just turn the offlined page into a regular bookmark, or does it remove the entry completely? This probably LG, as I agree that offline pages can be seen as a kind of cache, but please let's double-check with Vadym who suggested in crbug.com/541758 that "cookies and site data" be used.
Please explain the change properly in the issue description, and address the comment. https://codereview.chromium.org/1970423002/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover.h (left): https://codereview.chromium.org/1970423002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.h:138: REMOVE_OFFLINE_PAGE_DATA | So we are never setting this one? That is OK, provided you add a comment next to the definition of this enum saying that it is superseded by REMOVE_CACHE for the time being.
On 2016/05/13 08:31:16, msramek wrote: > Drive-by! > > Clarifying question: Does this just turn the offlined page into a regular > bookmark, or does it remove the entry completely? > > This probably LG, as I agree that offline pages can be seen as a kind of cache, > but please let's double-check with Vadym who suggested in crbug.com/541758 that > "cookies and site data" be used. Let me try to make it clear... So some time ago we were making offline pages with bookmarks, so that user has full control about offline pages. However for now we're decoupling offline pages from bookmarks, instead we have some 'automatic' ways to save pages offline, which makes user has little control (and that's why we're calling it a cache). So this change is going to make offline pages called as cache when user tries to clear storage spaces, instead of site data. I think it's more about definition of site data. And I don't know why I have no permission to access the bug you linked. What's it about?
Description was changed from ========== [Offline Pages] Clear offline pages when user clears cache. BUG=611225 ========== to ========== [Offline Pages] Clear offline pages when user clears cache. Since offline pages are no longer associated with bookmark, we're considering them as 'cache' instead of 'site data'. Hence making the change to correct the behavior when removing browsing data, move REMOVE_OFFLINE_PAGE_DATA from REMOVE_SITE_DATA and add REMOVE_CACHE as a condition when deciding to remove offline pages. BUG=611225 ==========
Addressing comments https://codereview.chromium.org/1970423002/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover.h (left): https://codereview.chromium.org/1970423002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.h:138: REMOVE_OFFLINE_PAGE_DATA | On 2016/05/13 16:14:57, fgorski wrote: > So we are never setting this one? > > That is OK, provided you add a comment next to the definition of this enum > saying that it is superseded by REMOVE_CACHE for the time being. yes we'll not set this one, instead we use REMOVE_CACHE. Added comments above the enum to explain.
lgtm with a comment about a comment https://codereview.chromium.org/1970423002/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.h (right): https://codereview.chromium.org/1970423002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.h:117: // REMOVE_OFFLINE_PAGE_DATA is superceded by REMOVE_CACHE since https://brendanscott.wordpress.com/2011/08/08/supercede-v-supersede-mirriam-w... superseded
lgtm, since the flag isn't used anywhere you could remove it altogether, it's easy enough to restore the flag should the need arise
On 2016/05/13 20:03:49, michaeln wrote: > lgtm, since the flag isn't used anywhere you could remove it altogether, it's > easy enough to restore the flag should the need arise Another idea is to define REMOVE_CACHE = REMOVE_NET_CACHE | REMOVE_OFFLINE_PAGE_DATA So its an aggregate like "site data" is.
On 2016/05/13 17:29:14, romax wrote: > On 2016/05/13 08:31:16, msramek wrote: > > Drive-by! > > > > Clarifying question: Does this just turn the offlined page into a regular > > bookmark, or does it remove the entry completely? > > > > This probably LG, as I agree that offline pages can be seen as a kind of > cache, > > but please let's double-check with Vadym who suggested in crbug.com/541758 > that > > "cookies and site data" be used. > > Let me try to make it clear... > So some time ago we were making offline pages with bookmarks, so that user has > full control about offline pages. However for now we're decoupling offline pages > from bookmarks, instead we have some 'automatic' ways to save pages offline, > which makes user has little control (and that's why we're calling it a cache). > So this change is going to make offline pages called as cache when user tries to > clear storage spaces, instead of site data. I think it's more about definition > of site data. And I don't know why I have no permission to access the bug you > linked. What's it about? If the saving is automatic, then it indeed does sound like cache. Site data is more about things that are saved by website's explicit action, many of those are even directly accessible by JS. The bug is narrowly ACL'd, because it's a privacy review. I can cc you to it. LGTM.
On 2016/05/13 20:07:47, michaeln wrote: > On 2016/05/13 20:03:49, michaeln wrote: > > lgtm, since the flag isn't used anywhere you could remove it altogether, it's > > easy enough to restore the flag should the need arise > > Another idea is to define > > REMOVE_CACHE = REMOVE_NET_CACHE | REMOVE_OFFLINE_PAGE_DATA > > So its an aggregate like "site data" is. +1 Since ::RemoveImpl is a huge messy method, it helps readability if the enum is more granular. I would prefer an aggregate instead of merging the two.
I agree that making an aggregate would increase readability, however renaming current REMOVE_CACHE to REMOVE_NET_CACHE and adding REMOVE_OFFLINE_PAGE_DATA in seems an overkill to me, since I'm sure offline pages won't be considered as cache for a long time. But renaming might introduce more confusion and complexity. So I'll go with removing the REMOVE_OFFLINE_PAGE_DATA and keep the comments in the remover_impl which might help.
The CQ bit was checked by romax@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, michaeln@chromium.org, msramek@chromium.org Link to the patchset: https://codereview.chromium.org/1970423002/#ps80001 (title: "typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1970423002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1970423002/80001
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Clear offline pages when user clears cache. Since offline pages are no longer associated with bookmark, we're considering them as 'cache' instead of 'site data'. Hence making the change to correct the behavior when removing browsing data, move REMOVE_OFFLINE_PAGE_DATA from REMOVE_SITE_DATA and add REMOVE_CACHE as a condition when deciding to remove offline pages. BUG=611225 ========== to ========== [Offline Pages] Clear offline pages when user clears cache. Since offline pages are no longer associated with bookmark, we're considering them as 'cache' instead of 'site data'. Hence making the change to correct the behavior when removing browsing data, move REMOVE_OFFLINE_PAGE_DATA from REMOVE_SITE_DATA and add REMOVE_CACHE as a condition when deciding to remove offline pages. BUG=611225 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Clear offline pages when user clears cache. Since offline pages are no longer associated with bookmark, we're considering them as 'cache' instead of 'site data'. Hence making the change to correct the behavior when removing browsing data, move REMOVE_OFFLINE_PAGE_DATA from REMOVE_SITE_DATA and add REMOVE_CACHE as a condition when deciding to remove offline pages. BUG=611225 ========== to ========== [Offline Pages] Clear offline pages when user clears cache. Since offline pages are no longer associated with bookmark, we're considering them as 'cache' instead of 'site data'. Hence making the change to correct the behavior when removing browsing data, move REMOVE_OFFLINE_PAGE_DATA from REMOVE_SITE_DATA and add REMOVE_CACHE as a condition when deciding to remove offline pages. BUG=611225 Committed: https://crrev.com/4150bae6fb7993b3675e8430dc65fd941268aa0b Cr-Commit-Position: refs/heads/master@{#393702} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4150bae6fb7993b3675e8430dc65fd941268aa0b Cr-Commit-Position: refs/heads/master@{#393702} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
