Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1773403004:
Move some offline-related functions into OfflinePageBridge, from BookmarkUtils. (Closed)
|
Created:
4 years, 9 months ago by dewittj Modified:
4 years, 9 months ago CC:
chromium-reviews, noyau+watch_chromium.org, browser-components-watch_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove some offline-related functions into OfflinePageBridge, from BookmarkUtils.
BUG=
Committed: https://crrev.com/07c0438a48e1a4d4fb65debc20ac7e97c2afa6bc
Cr-Commit-Position: refs/heads/master@{#383182}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Move into OfflinePageUtils #
Total comments: 5
Patch Set 3 : fgorski comments #
Total comments: 4
Patch Set 4 : Fixup comment style. #
Messages
Total messages: 18 (6 generated)
Description was changed from ========== Move some offline-related functions into OfflinePageBridge, from BookmarkUtils. BUG= ========== to ========== Move some offline-related functions into OfflinePageBridge, from BookmarkUtils. BUG= ==========
dewittj@chromium.org changed reviewers: + fgorski@chromium.org, ianwen@chromium.org
https://codereview.chromium.org/1773403004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/1773403004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:492: public SnackbarController createSnackbarControllerForFreeUpSpaceButton( Could you move it to offline page utils?
I don't like that idea, since then it'd just be a standalone static function which always needs a pointer to OfflinePageBridge. It seems cleaner to just hang it off the bridge. Ideally, we'd be able to separate the concerns of the snackbar in a better way, but this patch is intended to simplify BookmarkUtils rather than refactor our code a great deal.
Thanks for making BookmarkUtils simpler! https://chromiumcodereview.appspot.com/1773403004/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://chromiumcodereview.appspot.com/1773403004/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:492: public SnackbarController createSnackbarControllerForFreeUpSpaceButton( +1 what fgorski@ said. Tranditionally in Clank bridges are considered as "model" layer, and they mostly only fetches stuff from native. Also this method should be static.
OK, I have moved it over to Utils. PTAL
https://codereview.chromium.org/1773403004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java (right): https://codereview.chromium.org/1773403004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:157: snackbarManager.showSnackbar(snackbar); I believe you should probably destroy the bookmark model at some point in this method, given that it is no longer passed down for later destruction. https://codereview.chromium.org/1773403004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:244: BookmarkModel model = new BookmarkModel(); Can we use: OfflinePageBridge.getForProfile(profile); And drop the model from this method altogether?
https://codereview.chromium.org/1773403004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java (right): https://codereview.chromium.org/1773403004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:157: snackbarManager.showSnackbar(snackbar); On 2016/03/21 17:28:24, fgorski wrote: > I believe you should probably destroy the bookmark model at some point in this > method, given that it is no longer passed down for later destruction. Done. https://codereview.chromium.org/1773403004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:244: BookmarkModel model = new BookmarkModel(); On 2016/03/21 17:28:24, fgorski wrote: > Can we use: OfflinePageBridge.getForProfile(profile); > And drop the model from this method altogether? I thought about that, but wanted to ensure that we use the same method for getting profile as Bookmarks. If bookmarks is guaranteed to forever use GetLastUsedProfile, then we are okay, but if that ever changes, then the current way would be less brittle.
lgtm https://codereview.chromium.org/1773403004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java (right): https://codereview.chromium.org/1773403004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:244: BookmarkModel model = new BookmarkModel(); On 2016/03/23 15:45:11, dewittj wrote: > On 2016/03/21 17:28:24, fgorski wrote: > > Can we use: OfflinePageBridge.getForProfile(profile); > > And drop the model from this method altogether? > > I thought about that, but wanted to ensure that we use the same method for > getting profile as Bookmarks. If bookmarks is guaranteed to forever use > GetLastUsedProfile, then we are okay, but if that ever changes, then the current > way would be less brittle. Acknowledged. https://codereview.chromium.org/1773403004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java (right): https://codereview.chromium.org/1773403004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:99: * NOTE: This method calls |destroy| on the BookmarkModel that is passed to it. nit: BookmarkModel#destroy() is the java way
bookmark stuff lgtm with nits. https://codereview.chromium.org/1773403004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java (right): https://codereview.chromium.org/1773403004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:170: * NOTE: This callback will call |destroy| on the passed-in bookmark model. BookmarkModel#destroy().
https://codereview.chromium.org/1773403004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java (right): https://codereview.chromium.org/1773403004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:99: * NOTE: This method calls |destroy| on the BookmarkModel that is passed to it. On 2016/03/24 15:43:38, fgorski wrote: > nit: BookmarkModel#destroy() is the java way Done. https://codereview.chromium.org/1773403004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:170: * NOTE: This callback will call |destroy| on the passed-in bookmark model. On 2016/03/24 20:34:51, Ian Wen wrote: > BookmarkModel#destroy(). Done.
The CQ bit was checked by dewittj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ianwen@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/1773403004/#ps60001 (title: "Fixup comment style.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1773403004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773403004/60001
Message was sent while issue was closed.
Description was changed from ========== Move some offline-related functions into OfflinePageBridge, from BookmarkUtils. BUG= ========== to ========== Move some offline-related functions into OfflinePageBridge, from BookmarkUtils. BUG= ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Move some offline-related functions into OfflinePageBridge, from BookmarkUtils. BUG= ========== to ========== Move some offline-related functions into OfflinePageBridge, from BookmarkUtils. BUG= Committed: https://crrev.com/07c0438a48e1a4d4fb65debc20ac7e97c2afa6bc Cr-Commit-Position: refs/heads/master@{#383182} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/07c0438a48e1a4d4fb65debc20ac7e97c2afa6bc Cr-Commit-Position: refs/heads/master@{#383182} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
