Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1470403002:
[Offline pages] Filter out offline pages without matching bookmarks in UI (Closed)
|
Created:
5 years ago by fgorski Modified:
5 years ago Reviewers:
Kibeom Kim (inactive) CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline pages] Filter out offline pages without matching bookmarks in UI
Making sure that when offline page filter is applied, all of the
bookmark IDs are checked against the bookmark model, before being
used by UI.
BUG=537806
Committed: https://crrev.com/f2d8e24f06b57fa0edc5dcd999516e3343d8a769
Cr-Commit-Position: refs/heads/master@{#362185}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fixing bug id #Messages
Total messages: 19 (7 generated)
Description was changed from ========== [Offline pages] Filter out offline pages without matching bookmarks in UI Making sure that when offline page filter is applied, all of the bookmark IDs are checked against the bookmark model, before being used by UI. BUG=560424 ========== to ========== [Offline pages] Filter out offline pages without matching bookmarks in UI Making sure that when offline page filter is applied, all of the bookmark IDs are checked against the bookmark model, before being used by UI. BUG=537806 ==========
fgorski@chromium.org changed reviewers: + kkimlabs@chromium.org
PTAL
https://codereview.chromium.org/1470403002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java (right): https://codereview.chromium.org/1470403002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:333: // http://crbug.com/560424 I'll update the bug ID.
Is this because mOfflinePageBridge.getAllPages() returns everything including those were marked for deletion?
On 2015/11/25 20:00:46, Kibeom Kim wrote: > Is this because mOfflinePageBridge.getAllPages() returns everything including > those were marked for deletion? No, that is actually filtered out, but I think that a bookmark deleted by sync, or an updated ID of a bookmark could still be a problem. (It all is a bit async I believe). + we recently had a case where we would not notify of offline page deletion in ever case.
On 2015/11/25 20:51:56, fgorski wrote: > ... > + we recently had a case where we would not notify of offline page deletion > in ever case. Oh, interesting.. is there a bug or CL I can look into? I thought that BookmarkModel is expected to be updated only on main thread even on sync. Since disk saving logic is not synced, I think it's possible that Offline page's item doesn't exist on bookmark model, but for that case, probably the cleanup code better belongs to offline page model init path.
On 2015/11/25 21:19:42, Kibeom Kim wrote: > On 2015/11/25 20:51:56, fgorski wrote: > > ... > > + we recently had a case where we would not notify of offline page deletion > > in ever case. > > Oh, interesting.. is there a bug or CL I can look into? CL for properly propagating deleted pages: https://codereview.chromium.org/1460413002/ > > I thought that BookmarkModel is expected to be updated only on main thread even > on sync. Since disk saving logic is not synced, I think it's possible that > Offline page's item doesn't exist on bookmark model, but for that case, probably > the cleanup code better belongs to offline page model init path. I am working on another patch that will redo the logic for clean up, but it will take a while before I can put it in and it is unlikely to merge into M48. https://codereview.chromium.org/1480723002/
lgtm
The CQ bit was checked by fgorski@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1470403002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1470403002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) ios_rel_device_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by fgorski@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1470403002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1470403002/20001
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Filter out offline pages without matching bookmarks in UI Making sure that when offline page filter is applied, all of the bookmark IDs are checked against the bookmark model, before being used by UI. BUG=537806 ========== to ========== [Offline pages] Filter out offline pages without matching bookmarks in UI Making sure that when offline page filter is applied, all of the bookmark IDs are checked against the bookmark model, before being used by UI. BUG=537806 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Filter out offline pages without matching bookmarks in UI Making sure that when offline page filter is applied, all of the bookmark IDs are checked against the bookmark model, before being used by UI. BUG=537806 ========== to ========== [Offline pages] Filter out offline pages without matching bookmarks in UI Making sure that when offline page filter is applied, all of the bookmark IDs are checked against the bookmark model, before being used by UI. BUG=537806 Committed: https://crrev.com/f2d8e24f06b57fa0edc5dcd999516e3343d8a769 Cr-Commit-Position: refs/heads/master@{#362185} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f2d8e24f06b57fa0edc5dcd999516e3343d8a769 Cr-Commit-Position: refs/heads/master@{#362185} |
