Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1440623004:
[Enhanced Bookmark]Rewrite initialization logic (Closed)
Description[Enhanced Bookmark]Rewrite initialization logic
It proves to be really hard to change the first content users see in
bookmark manager on both phones and tablets. This is because EBManager
does not initialize itself the same as other native pages.
This CL proposes a different approach about initializing the bookmark
manager. In EBManager's constructor, it will always initialize itself in
the loading mode. And after the constructor returns, EBPage and
EBActivity call updateForUrl() with an initial url that is determined by
some complicated logics in EBUtils. This procedure aligns with
NativePageFactory's logic.
To achieve this goal, several changes have been made:
1. Loading state no longer carries a url. Instead mInitialUrl is created
to catch that info.
2. Detecting bookmark model loaded state has been made simple by
introducing doAfterBookmarkModelLoaded in BookmarksBridge.
3. Now both EBActivity and EBPage need to call EBmanager#updateForUrl.
4. EBPage no longer calls tab#loadUrl if this url change is originally
initiated by tab itself.
BUG=554595
Committed: https://crrev.com/27f0872193b01629cbd041d07a78fc014970e03a
Cr-Commit-Position: refs/heads/master@{#362910}
Patch Set 1 #
Total comments: 2
Patch Set 2 : decouple this CL with experiments #
Total comments: 11
Patch Set 3 : address newt's comments #
Total comments: 2
Patch Set 4 : rewrote a function #Patch Set 5 : Rewrite the initilization code for EB #
Total comments: 6
Patch Set 6 : address fgorski's comments #
Total comments: 29
Patch Set 7 : respond to newt's comments #
Total comments: 14
Patch Set 8 : Added another class: EBUIState. All EB will be remaned to B this month. #
Total comments: 20
Patch Set 9 : nits and renamings #
Total comments: 1
Patch Set 10 : #Patch Set 11 : Thank you, findbug #Messages
Total messages: 40 (12 generated)
ianwen@chromium.org changed reviewers: + kkimlabs@chromium.org, newt@chromium.org
PTAL. :)
lgtm I think it's more urgent to fix the bugs introduced by https://codereview.chromium.org/1425293009 , which are already merged to M47, so let's merge this to M47 first. This is a small self-contained CL so I think it should be OK. https://codereview.chromium.org/1440623004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java (right): https://codereview.chromium.org/1440623004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:281: public static void saveUrlToPreference(Context context, String url) { nit: -public? https://codereview.chromium.org/1440623004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:289: public static String getUrlForLastTime(Context context) { nit: -public?
PTAL the new patch.
Description was changed from ========== [Enhanced Bookmark]Restore user's preference on Tablet On Tablet the bookmark manager is opened via a url navigation and the url is never changed even if we save user's previous bookmark page. This CL fixes it. This CL also fixes a critical bug introduced in https://codereview.chromium.org/1425293009, where tablet users might never be able to navigate to all bookmarks section. BUG=554595 ========== to ========== [Enhanced Bookmark]Restore user's preference on Tablet On Tablet the bookmark manager is opened via a url navigation and the url is never changed even if we save user's previous bookmark page. This CL fixes it. This CL also fixes a critical bug introduced in https://codereview.chromium.org/1425293009, where tablet users might never be able to navigate to all bookmarks section. BUG=554595 ==========
https://codereview.chromium.org/1440623004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java (right): https://codereview.chromium.org/1440623004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:53: private static final String PREF_LAST_USED_URL = "enhanced_bookmark_last_used_url"; Weren't you planning to change the name of this pref to effectively "reset" it for all users? https://codereview.chromium.org/1440623004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:265: if (!isEnhancedBookmarkEnabled()) { side note: can we get rid of this code soon?? https://codereview.chromium.org/1440623004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:269: if (TextUtils.isEmpty(getLastUsedUrl(activity))) { Store the value of getLastUsedUrl(activity) and reuse it on line 289. Looking up values in shared prefs isn't free. https://codereview.chromium.org/1440623004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:271: EnhancedBookmarksModel model = new EnhancedBookmarksModel(); This is a lot of nesting. It would be more readable to extract a method called "getUrlToLoad()" or something, that returns a String. And then this method would simply say: if (DeviceFormFactor.isTablet(activity)) { openBookmark(activity, getUrlToLoad(activity)); } else { ... } https://codereview.chromium.org/1440623004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:277: + model.getMobileFolderId().toString(); Is mobileFolderId a constant? Could we just use that constant rather than creating a bookmark model bridge object?
PTAL. https://codereview.chromium.org/1440623004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java (right): https://codereview.chromium.org/1440623004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:53: private static final String PREF_LAST_USED_URL = "enhanced_bookmark_last_used_url"; On 2015/11/12 02:29:39, newt wrote: > Weren't you planning to change the name of this pref to effectively "reset" it > for all users? Talked offline. It is not handled at least in this CL. https://codereview.chromium.org/1440623004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:265: if (!isEnhancedBookmarkEnabled()) { On 2015/11/12 02:29:39, newt wrote: > side note: can we get rid of this code soon?? In December I will start removing the old code. One blocking issue is the bookmark widget. https://codereview.chromium.org/1440623004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:269: if (TextUtils.isEmpty(getLastUsedUrl(activity))) { On 2015/11/12 02:29:39, newt wrote: > Store the value of getLastUsedUrl(activity) and reuse it on line 289. Looking up > values in shared prefs isn't free. Done. https://codereview.chromium.org/1440623004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:271: EnhancedBookmarksModel model = new EnhancedBookmarksModel(); On 2015/11/12 02:29:39, newt wrote: > This is a lot of nesting. It would be more readable to extract a method called > "getUrlToLoad()" or something, that returns a String. And then this method would > simply say: > > if (DeviceFormFactor.isTablet(activity)) { > openBookmark(activity, getUrlToLoad(activity)); > } else { > ... > } Done. https://codereview.chromium.org/1440623004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:277: + model.getMobileFolderId().toString(); On 2015/11/12 02:29:39, newt wrote: > Is mobileFolderId a constant? Could we just use that constant rather than > creating a bookmark model bridge object? mobileFolderId is somewhat constant in the current code. All permanent nodes' ids are determined in bookmark_model::generate_next_node_id(), and is subject to the sequence we call it. I wouldn't recommend us assume this constant number (though we know it is likely to be 3) but in future if they change code in bookmark_model we have to support it here, which is not ideal. :(
lgtm with suggestion https://codereview.chromium.org/1440623004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java (right): https://codereview.chromium.org/1440623004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:265: if (!isEnhancedBookmarkEnabled()) { On 2015/11/12 18:43:24, Ian Wen wrote: > On 2015/11/12 02:29:39, newt wrote: > > side note: can we get rid of this code soon?? > > In December I will start removing the old code. One blocking issue is the > bookmark widget. The bookmark widget is already gone! :D https://codereview.chromium.org/1415973005 https://codereview.chromium.org/1440623004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java (right): https://codereview.chromium.org/1440623004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:279: if (TextUtils.isEmpty(lastUsedUrl)) { Suggestion for a cleaner method structure: String lastUsedUrl = getLastUsedUrl(activity); if (!TextUtils.isEmpty(lastUsedUrl)) { return lastUsedUrl; } EnhancedBookmarksModel model = new EnhancedBookmarksModel(); String url = null; if (model.isBookmarkModelLoaded() && VariationsAssociatedData(...)) { url = UrlConstants.BOOKMARKS_FOLDER_URL + model.getMobileFolderId().toString(); } else { url = UrlConstants.BOOKMARKS_URL; } model.destroy() return url; ---- or ---- EnhancedBookmarksModel model = new EnhancedBookmarksModel(); try { if (model.isBookmarkModelLoaded() && VariationsAssociatedData(...)) { return UrlConstants.BOOKMARKS_FOLDER_URL + model.getMobileFolderId().toString(); } else { return UrlConstants.BOOKMARKS_URL; } } finally { model.destroy() }
I just realize after this CL the experiment will still be broken, on phone only because we are not reading the experiment for phone. I think I have a better fix, which involves changing a bit code in NativePageFactory. https://codereview.chromium.org/1440623004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java (right): https://codereview.chromium.org/1440623004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:279: if (TextUtils.isEmpty(lastUsedUrl)) { On 2015/11/12 19:07:53, newt wrote: > Suggestion for a cleaner method structure: > > String lastUsedUrl = getLastUsedUrl(activity); > if (!TextUtils.isEmpty(lastUsedUrl)) { > return lastUsedUrl; > } > > EnhancedBookmarksModel model = new EnhancedBookmarksModel(); > String url = null; > if (model.isBookmarkModelLoaded() && > VariationsAssociatedData(...)) { > url = UrlConstants.BOOKMARKS_FOLDER_URL + > model.getMobileFolderId().toString(); > } else { > url = UrlConstants.BOOKMARKS_URL; > } > model.destroy() > return url; > > ---- or ---- > > EnhancedBookmarksModel model = new EnhancedBookmarksModel(); > try { > if (model.isBookmarkModelLoaded() && > VariationsAssociatedData(...)) { > return UrlConstants.BOOKMARKS_FOLDER_URL + > model.getMobileFolderId().toString(); > } else { > return UrlConstants.BOOKMARKS_URL; > } > } finally { > model.destroy() > } Done.
Description was changed from ========== [Enhanced Bookmark]Restore user's preference on Tablet On Tablet the bookmark manager is opened via a url navigation and the url is never changed even if we save user's previous bookmark page. This CL fixes it. This CL also fixes a critical bug introduced in https://codereview.chromium.org/1425293009, where tablet users might never be able to navigate to all bookmarks section. BUG=554595 ========== to ========== [Enhanced Bookmark]Rewrite initialization logic It proves to be really hard to change the first content users see in bookmark manager on both phones and tablets. This is because EBManager does not initialize itself the same as other native pages. This CL proposes a different approach about initializing the bookmark manager. In EBManager's constructor, it will always initialize itself in the loading mode. And after the constructor returns, EBPage and EBActivity call updateForUrl() with an initial url that is determined by some complicated logics in EBUtils. This procedure aligns with NativePageFactory's logic. To achieve this goal, several changes have been made: 1. Loading state no longer carries a url. Instead mInitialUrl is created to catch that info. 2. Detecting bookmark model loaded state has been made simple by introducing doAfterBookmarkModelLoaded in BookmarksBridge. 3. Now both EBActivity and EBPage need to call EBmanager#updateForUrl. 4. EBPage no longer calls tab#loadUrl if this url change is originally initiated by tab itself. BUG=554595 ==========
Description was changed from ========== [Enhanced Bookmark]Rewrite initialization logic It proves to be really hard to change the first content users see in bookmark manager on both phones and tablets. This is because EBManager does not initialize itself the same as other native pages. This CL proposes a different approach about initializing the bookmark manager. In EBManager's constructor, it will always initialize itself in the loading mode. And after the constructor returns, EBPage and EBActivity call updateForUrl() with an initial url that is determined by some complicated logics in EBUtils. This procedure aligns with NativePageFactory's logic. To achieve this goal, several changes have been made: 1. Loading state no longer carries a url. Instead mInitialUrl is created to catch that info. 2. Detecting bookmark model loaded state has been made simple by introducing doAfterBookmarkModelLoaded in BookmarksBridge. 3. Now both EBActivity and EBPage need to call EBmanager#updateForUrl. 4. EBPage no longer calls tab#loadUrl if this url change is originally initiated by tab itself. BUG=554595 ========== to ========== [Enhanced Bookmark]Rewrite initialization logic It proves to be really hard to change the first content users see in bookmark manager on both phones and tablets. This is because EBManager does not initialize itself the same as other native pages. This CL proposes a different approach about initializing the bookmark manager. In EBManager's constructor, it will always initialize itself in the loading mode. And after the constructor returns, EBPage and EBActivity call updateForUrl() with an initial url that is determined by some complicated logics in EBUtils. This procedure aligns with NativePageFactory's logic. To achieve this goal, several changes have been made: 1. Loading state no longer carries a url. Instead mInitialUrl is created to catch that info. 2. Detecting bookmark model loaded state has been made simple by introducing doAfterBookmarkModelLoaded in BookmarksBridge. 3. Now both EBActivity and EBPage need to call EBmanager#updateForUrl. 4. EBPage no longer calls tab#loadUrl if this url change is originally initiated by tab itself. BUG=554595 ==========
ianwen@chromium.org changed reviewers: + fgorski@chromium.org, jianli@chromium.org
Hey guys, I rewrote some of the initialization logic for EnhancedBookmarks, and it may affects OfflinePage as well. Please refer to the CL's description for design overview. TLDR for jianli@ and fgorski@: 1. On tablet, user's last url is now restored in bookmark manager. 2. When OfflinePage is enabled and network is out, OfflinePage's filter will be shown first, regardless of user's previous url. I spent a long time testing this patch on both N5 and N7, with different loading state and Offline page on/off. If I did not miss any cases, the change gives correct result.
https://codereview.chromium.org/1440623004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java (right): https://codereview.chromium.org/1440623004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:273: EnhancedBookmarkUtils.setLastUsedUrl(mActivity, state.mUrl); This may be something that we don't want to fix as part of this patch, but here it goes: If we are offline, we would like to show offline filter (to make the bookmarks page immediately usefule), but not persist this setting - as this is useful when offline. I don't think it will work here. Also, in case you are restoring the previous setting, this code is still saving it, even though it hasn't change, if I understand correctly. https://codereview.chromium.org/1440623004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:479: encodeUrl(UrlConstants.BOOKMARKS_FILTER_URL, filter.toString()), bookmarkModel); Why would this be the only place you are making such change? https://codereview.chromium.org/1440623004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java (right): https://codereview.chromium.org/1440623004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:302: return UrlConstants.BOOKMARKS_FILTER_URL I think you could have static methods on UIState to produce the URLs and then use them there and here consistently.
https://codereview.chromium.org/1440623004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java (right): https://codereview.chromium.org/1440623004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:273: EnhancedBookmarkUtils.setLastUsedUrl(mActivity, state.mUrl); On 2015/11/13 17:06:27, fgorski wrote: > This may be something that we don't want to fix as part of this patch, but here > it goes: > > If we are offline, we would like to show offline filter (to make the bookmarks > page immediately usefule), but not persist this setting - as this is useful when > offline. I don't think it will work here. > > Also, in case you are restoring the previous setting, this code is still saving > it, even though it hasn't change, if I understand correctly. Okay I see. How about I add a query in the url like "?should_persist=true" and then in UIState we parse it. Ideally the goal is to make the state of EB to fully reproducable given a URL.
https://codereview.chromium.org/1440623004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java (right): https://codereview.chromium.org/1440623004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:273: EnhancedBookmarkUtils.setLastUsedUrl(mActivity, state.mUrl); On 2015/11/13 19:58:59, Ian Wen wrote: > On 2015/11/13 17:06:27, fgorski wrote: > > This may be something that we don't want to fix as part of this patch, but > here > > it goes: > > > > If we are offline, we would like to show offline filter (to make the bookmarks > > page immediately usefule), but not persist this setting - as this is useful > when > > offline. I don't think it will work here. > > > > Also, in case you are restoring the previous setting, this code is still > saving > > it, even though it hasn't change, if I understand correctly. > > Okay I see. How about I add a query in the url like "?should_persist=true" and > then in UIState we parse it. Ideally the goal is to make the state of EB to > fully reproducable given a URL. Sounds reasonable to me, but it may require solid testing, as this affects the URL.
newt@ or kkimlabs@ this CL looks quite different than my first try. PTAL. fgorski@ please check the new patchset and see if it follows what you guys want. https://codereview.chromium.org/1440623004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java (right): https://codereview.chromium.org/1440623004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:479: encodeUrl(UrlConstants.BOOKMARKS_FILTER_URL, filter.toString()), bookmarkModel); On 2015/11/13 17:06:27, fgorski wrote: > Why would this be the only place you are making such change? Acknowledged. Added several tests to cover all cases.
On 2015/11/17 00:54:18, Ian Wen wrote: > newt@ or kkimlabs@ this CL looks quite different than my first try. PTAL. > > fgorski@ please check the new patchset and see if it follows what you guys want. > > https://codereview.chromium.org/1440623004/diff/80001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java > (right): > > https://codereview.chromium.org/1440623004/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:479: > encodeUrl(UrlConstants.BOOKMARKS_FILTER_URL, filter.toString()), bookmarkModel); > On 2015/11/13 17:06:27, fgorski wrote: > > Why would this be the only place you are making such change? > > Acknowledged. Added several tests to cover all cases. If you intend to merge it to M48, you are still breaking the offline page behavior with this patch I think (opening the saved offline filter when offline). what is the plan here?
On 2015/11/17 23:15:30, fgorski wrote: > On 2015/11/17 00:54:18, Ian Wen wrote: > > newt@ or kkimlabs@ this CL looks quite different than my first try. PTAL. > > > > fgorski@ please check the new patchset and see if it follows what you guys > want. > > > > > https://codereview.chromium.org/1440623004/diff/80001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java > > (right): > > > > > https://codereview.chromium.org/1440623004/diff/80001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:479: > > encodeUrl(UrlConstants.BOOKMARKS_FILTER_URL, filter.toString()), > bookmarkModel); > > On 2015/11/13 17:06:27, fgorski wrote: > > > Why would this be the only place you are making such change? > > > > Acknowledged. Added several tests to cover all cases. > > If you intend to merge it to M48, you are still breaking the offline page > behavior with this patch I think (opening the saved offline filter when > offline). > what is the plan here? I think this is too big to be merged to M48 and it might just land on M49. Also why does it break offline page? It opens offline page filter when the device is not connected.
On 2015/11/17 23:20:38, Ian Wen wrote: > On 2015/11/17 23:15:30, fgorski wrote: > > On 2015/11/17 00:54:18, Ian Wen wrote: > > > newt@ or kkimlabs@ this CL looks quite different than my first try. PTAL. > > > > > > fgorski@ please check the new patchset and see if it follows what you guys > > want. > > > > > > > > > https://codereview.chromium.org/1440623004/diff/80001/chrome/android/java/src... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java > > > (right): > > > > > > > > > https://codereview.chromium.org/1440623004/diff/80001/chrome/android/java/src... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:479: > > > encodeUrl(UrlConstants.BOOKMARKS_FILTER_URL, filter.toString()), > > bookmarkModel); > > > On 2015/11/13 17:06:27, fgorski wrote: > > > > Why would this be the only place you are making such change? > > > > > > Acknowledged. Added several tests to cover all cases. > > > > If you intend to merge it to M48, you are still breaking the offline page > > behavior with this patch I think (opening the saved offline filter when > > offline). > > what is the plan here? > > I think this is too big to be merged to M48 and it might just land on M49. > > Also why does it break offline page? It opens offline page filter when the > device is not connected. Well in that case it works for me. lgtm.
looks good to me :) just few minor stuff https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkPage.java (right): https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkPage.java:113: if (url.equals(mCurrentUrl)) return; Q: 1. how does this interact with refresh? 2. What problem does it solve? https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java (right): https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:266: public static boolean shouldShowOfflinePageAtFirst(EnhancedBookmarksModel model, private? https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:312: .equals("mobile")) { I reverted mobile experiment, let's exclude from this patch. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:353: public static String encodeUrl(String prefix, String suffix, Boolean shouldPersist) { I guess we can use String.valueOf(boolean) at #356 to avoid Boolean object here?
https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/bookmark/BookmarksBridge.java (right): https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/bookmark/BookmarksBridge.java:210: * If bookmark model is loaded, execute the runnable immediately; otherwise schedule the s/execute/executes/ javadocs should describe what the method does (e.g. "Enables the lasers"), rather than sound like a command. (inline comments, by constrast, are usually written as commands, e.g. "Log a warning") https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/bookmark/BookmarksBridge.java:214: public boolean doAfterBookmarkModelLoaded(final Runnable runnable) { s/do/run/ to use consistent terminology https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java (right): https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:304: if (shouldShowOfflinePageAtFirst(model, activity.getApplicationContext())) { Why getApplicationContext()? Activity is a Context too. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:326: public static void setLastUsedUrl(Context context, String url) { Make all of these private unless they need to be public. Or make them package private and mark with @VisibleForTesting is they're used in tests. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:344: Uri.Builder builder = Uri.parse(prefix).buildUpon(); This should just forward to the other encodeUrl() method, e.g. encodeUrl(prefix, suffix, true); https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:350: * Encodes the suffix as path and append it to the prefix. This comment doesn't make any sense. Higher level: why does this method exist? Why might someone call it? What does it return? https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:356: builder.appendQueryParameter(URI_PERSIST_QUERY_NAME, shouldPersist.toString()); if (!shouldPersist) { builder.appendQueryParameter(URI_PERSIST_QUERY_NAME, "0"); } https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:361: * Gets the suffix of a url and decode it. Also confused by this comment https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:363: public static String decodePath(String url) { maybe call this "getPathFromUrl()" or even "getLastPathSegment()" because that's what it's really doing. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:376: return Boolean.parseBoolean(booleanString); This will crash if it's not parsable. I'd instead check for equality against the expected value: String shouldPersist = uri.getQueryParameter(URI_PERSIST_QUERY_NAME); return !("0".equals(shouldPersist));
newt@: thanks for the awesome comments. All addressed. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/bookmark/BookmarksBridge.java (right): https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/bookmark/BookmarksBridge.java:210: * If bookmark model is loaded, execute the runnable immediately; otherwise schedule the On 2015/11/19 23:29:28, newt wrote: > s/execute/executes/ > > javadocs should describe what the method does (e.g. "Enables the lasers"), > rather than sound like a command. (inline comments, by constrast, are usually > written as commands, e.g. "Log a warning") Done. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/bookmark/BookmarksBridge.java:214: public boolean doAfterBookmarkModelLoaded(final Runnable runnable) { On 2015/11/19 23:29:28, newt wrote: > s/do/run/ to use consistent terminology Done. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkPage.java (right): https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkPage.java:113: if (url.equals(mCurrentUrl)) return; On 2015/11/19 19:18:19, Kibeom Kim (OOO) wrote: > Q: 1. how does this interact with refresh? 2. What problem does it solve? It eliminates the loop that EB changes its url and it tells chrome tab. Chrome tab will try to load EB again to that url. Refresh will do nothing here (WAI?) https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java (right): https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:266: public static boolean shouldShowOfflinePageAtFirst(EnhancedBookmarksModel model, On 2015/11/19 19:18:19, Kibeom Kim (OOO) wrote: > private? Done. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:304: if (shouldShowOfflinePageAtFirst(model, activity.getApplicationContext())) { On 2015/11/19 23:29:28, newt wrote: > Why getApplicationContext()? Activity is a Context too. I copied the implementation that the offline page folks wrote. And in terms of implementation details an activity is indeed sufficient here. Done. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:312: .equals("mobile")) { On 2015/11/19 19:18:19, Kibeom Kim (OOO) wrote: > I reverted mobile experiment, let's exclude from this patch. Done. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:326: public static void setLastUsedUrl(Context context, String url) { On 2015/11/19 23:29:29, newt wrote: > Make all of these private unless they need to be public. Or make them package > private and mark with @VisibleForTesting is they're used in tests. Done. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:344: Uri.Builder builder = Uri.parse(prefix).buildUpon(); On 2015/11/19 23:29:28, newt wrote: > This should just forward to the other encodeUrl() method, e.g. > > encodeUrl(prefix, suffix, true); Done. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:350: * Encodes the suffix as path and append it to the prefix. On 2015/11/19 23:29:28, newt wrote: > This comment doesn't make any sense. Higher level: why does this method exist? > Why might someone call it? What does it return? Rewrote it and added the purpose of the function. Done. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:353: public static String encodeUrl(String prefix, String suffix, Boolean shouldPersist) { On 2015/11/19 19:18:19, Kibeom Kim (OOO) wrote: > I guess we can use String.valueOf(boolean) at #356 to avoid Boolean object here? Did it in newt's way. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:356: builder.appendQueryParameter(URI_PERSIST_QUERY_NAME, shouldPersist.toString()); On 2015/11/19 23:29:29, newt wrote: > if (!shouldPersist) { > builder.appendQueryParameter(URI_PERSIST_QUERY_NAME, "0"); > } Done. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:361: * Gets the suffix of a url and decode it. On 2015/11/19 23:29:28, newt wrote: > Also confused by this comment Rewrote it. Hope it is now better. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:363: public static String decodePath(String url) { On 2015/11/19 23:29:28, newt wrote: > maybe call this "getPathFromUrl()" or even "getLastPathSegment()" because that's > what it's really doing. Done. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:376: return Boolean.parseBoolean(booleanString); On 2015/11/19 23:29:28, newt wrote: > This will crash if it's not parsable. I'd instead check for equality against the > expected value: > > String shouldPersist = uri.getQueryParameter(URI_PERSIST_QUERY_NAME); > return !("0".equals(shouldPersist)); Done.
Thanks for the clean-ups. I still have some high level comments that I think could make this code significantly clearer. https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkPage.java (right): https://codereview.chromium.org/1440623004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkPage.java:113: if (url.equals(mCurrentUrl)) return; > Refresh will do nothing here (WAI?) Actually, refresh will cause the page to be recreated. This happens for all native pages. See "candidateForReuse" in Tab.maybeShowNativePage(): https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... https://codereview.chromium.org/1440623004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/bookmark/BookmarksBridge.java (right): https://codereview.chromium.org/1440623004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/bookmark/BookmarksBridge.java:210: * Schedule a runnable to run after the bookmark model is loaded. If the s/Schedule/Schedules/ Same deal: javadocs should describe what the method does (e.g. "Enables the lasers"), rather than sound like a command. (inline comments, by constrast, are usually written as commands, e.g. "Log a warning") https://codereview.chromium.org/1440623004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java (right): https://codereview.chromium.org/1440623004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:434: static class UIState { Since UIState is in charge of parsing URLs, I think it should also be in charge of generating URLs. That will keep all the bookmark URL logic encapsulated inside of UIState, rather than being partially in UIState and partially in EnhancedBookmarkUtils. (Alternatively, you could create a new BookmarkUrls class and move all the URL creating and parsing logic into there.) That means moving encodePathToUrl(), getPathFromUrl(), getShouldPersistFromUrl() into UIState. All these methods, except encodePathToUrl(), can be private. getPathFromUrl() can probably be removed altogether and just replaced with a direct call to Uri.getLastPathSegment(). For clarify, I think you should break encodePathToUrl() into two simple methods: String createFolderUrl(BookmarkId folderId); String createFilterUrl(EnhancedBookmarkFilter filter, boolean shouldPersist); I think that would make bookmark URL handling simpler, clearer, and better encapsulated. https://codereview.chromium.org/1440623004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:514: private boolean isValid(EnhancedBookmarksModel bookmarkModel) { does this need to be updated to handle filter URLs? https://codereview.chromium.org/1440623004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:530: return 31 * mUrl.hashCode() + mState; This will crash is mUrl is null. It might be simplest to make mUrl = "" by default. Then you don't need to change the equals() method either. https://codereview.chromium.org/1440623004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:538: return mState == other.mState && mUrl.equals(other.mUrl); I'd remove the line above and change this to: return mState == other.mState && TextUtils.equals(mUrl, other.mUrl); https://codereview.chromium.org/1440623004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java (right): https://codereview.chromium.org/1440623004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:292: EnhancedBookmarkFilter.OFFLINE_PAGES.toString(), false); FWIW, I don't recommend using OFFLINE_PAGES.toString(), since if someone renames the OFFLINE_PAGES constant, this URL will unexpectedly change. OFFLINE_PAGES.toString() is a bit reflection-y in this regard -- changes that seem perfectly safe (e.g. renaming a constant) may break the code. A better solution would be to add a member field to EnhancedBookmarkFilter: public enum EnhancedBookmarkFilter { OFFLINE_PAGES("offline"); public final String value; private EnhancedBookmarkFilter(String value) { this.value = value; } } Then in places like you, you can use OFFLINE_PAGES.value. Alternatively, you could add a big impossible-to-miss comment in EnhancedBookmarkFilter saying that the values of these constants should never be changed. This isn't directly related to this CL though. Just an idea for future cleanup. https://codereview.chromium.org/1440623004/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkTest.java (right): https://codereview.chromium.org/1440623004/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkTest.java:198: String url = EnhancedBookmarkUtils.encodePathToUrl(UrlConstants.BOOKMARKS_FOLDER_URL, What exactly are you testing here? These five tests collectively test several (two?) different features. I think you could cover the same ground with two or three more focused tests: - one test that thoroughly tests URL creation and parsing. e.g. a list of statements like assertEquals("chrome-native://bookmarks/filter/OFFLINE_PAGES", UIState.createFilterUrl(EnhancedBookmarkFilter.OFFLINE_PAGES)) - one or two tests for whether URLs with shouldPersist=true/false are persisted. e.g. load a hard-coded URL, such as "chrome-native://bookmarks/filter/OFFLINE_PAGES?persist=0", then check the value of EnhancedBookmarkUtils.getLastUsedUrl(). Or the tests could be a bit more integration-y: instead of checking the value of EnhancedBookmarkUtils.getLastUsedUrl(), you could load the bookmark manager again and see what the loaded URL is.
https://chromiumcodereview.appspot.com/1440623004/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/bookmark/BookmarksBridge.java (right): https://chromiumcodereview.appspot.com/1440623004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/bookmark/BookmarksBridge.java:210: * Schedule a runnable to run after the bookmark model is loaded. If the On 2015/12/01 19:53:20, newt wrote: > s/Schedule/Schedules/ > > Same deal: javadocs should describe what the method does (e.g. "Enables the > lasers"), rather than sound like a command. (inline comments, by constrast, are > usually written as commands, e.g. "Log a warning") Done. https://chromiumcodereview.appspot.com/1440623004/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java (right): https://chromiumcodereview.appspot.com/1440623004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:434: static class UIState { On 2015/12/01 19:53:20, newt wrote: > Since UIState is in charge of parsing URLs, I think it should also be in charge > of generating URLs. That will keep all the bookmark URL logic encapsulated > inside of UIState, rather than being partially in UIState and partially in > EnhancedBookmarkUtils. (Alternatively, you could create a new BookmarkUrls class > and move all the URL creating and parsing logic into there.) > > That means moving encodePathToUrl(), getPathFromUrl(), getShouldPersistFromUrl() > into UIState. All these methods, except encodePathToUrl(), can be private. > getPathFromUrl() can probably be removed altogether and just replaced with a > direct call to Uri.getLastPathSegment(). > > For clarify, I think you should break encodePathToUrl() into two simple methods: > > String createFolderUrl(BookmarkId folderId); > String createFilterUrl(EnhancedBookmarkFilter filter, boolean shouldPersist); > > I think that would make bookmark URL handling simpler, clearer, and better > encapsulated. Done. https://chromiumcodereview.appspot.com/1440623004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:514: private boolean isValid(EnhancedBookmarksModel bookmarkModel) { On 2015/12/01 19:53:20, newt wrote: > does this need to be updated to handle filter URLs? Not needed for now, but it's more correct to add Filter here as well. Done. https://chromiumcodereview.appspot.com/1440623004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:530: return 31 * mUrl.hashCode() + mState; On 2015/12/01 19:53:20, newt wrote: > This will crash is mUrl is null. It might be simplest to make mUrl = "" by > default. Then you don't need to change the equals() method either. Done. https://chromiumcodereview.appspot.com/1440623004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:538: return mState == other.mState && mUrl.equals(other.mUrl); On 2015/12/01 19:53:20, newt wrote: > I'd remove the line above and change this to: > > return mState == other.mState && TextUtils.equals(mUrl, other.mUrl); Done. https://chromiumcodereview.appspot.com/1440623004/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java (right): https://chromiumcodereview.appspot.com/1440623004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:292: EnhancedBookmarkFilter.OFFLINE_PAGES.toString(), false); On 2015/12/01 19:53:20, newt wrote: > FWIW, I don't recommend using OFFLINE_PAGES.toString(), since if someone renames > the OFFLINE_PAGES constant, this URL will unexpectedly change. > OFFLINE_PAGES.toString() is a bit reflection-y in this regard -- changes that > seem perfectly safe (e.g. renaming a constant) may break the code. > > A better solution would be to add a member field to EnhancedBookmarkFilter: > > public enum EnhancedBookmarkFilter { > OFFLINE_PAGES("offline"); > > public final String value; > > private EnhancedBookmarkFilter(String value) { > this.value = value; > } > } > > Then in places like you, you can use OFFLINE_PAGES.value. Alternatively, you > could add a big impossible-to-miss comment in EnhancedBookmarkFilter saying that > the values of these constants should never be changed. > > This isn't directly related to this CL though. Just an idea for future cleanup. Done. https://chromiumcodereview.appspot.com/1440623004/diff/120001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkTest.java (right): https://chromiumcodereview.appspot.com/1440623004/diff/120001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkTest.java:198: String url = EnhancedBookmarkUtils.encodePathToUrl(UrlConstants.BOOKMARKS_FOLDER_URL, On 2015/12/01 19:53:20, newt wrote: > What exactly are you testing here? These five tests collectively test several > (two?) different features. I think you could cover the same ground with two or > three more focused tests: > > - one test that thoroughly tests URL creation and parsing. e.g. a list of > statements like assertEquals("chrome-native://bookmarks/filter/OFFLINE_PAGES", > UIState.createFilterUrl(EnhancedBookmarkFilter.OFFLINE_PAGES)) > > - one or two tests for whether URLs with shouldPersist=true/false are > persisted. e.g. load a hard-coded URL, such as > "chrome-native://bookmarks/filter/OFFLINE_PAGES?persist=0", then check the value > of EnhancedBookmarkUtils.getLastUsedUrl(). Or the tests could be a bit more > integration-y: instead of checking the value of > EnhancedBookmarkUtils.getLastUsedUrl(), you could load the bookmark manager > again and see what the loaded URL is. Goal is to test: 1. If bookmark manager can be successfully opened in most common case. 2. If bookmark manager can load the offline page correctly if the user navigated to it last time. 3. If bookmark manager can load the offline page, but the url is not persisted for this visit. 4. If the URL composition helper functions work correctly. Each of them now corresponds to a test case.
This looks much better now! Last few comments I promise :) https://chromiumcodereview.appspot.com/1440623004/diff/140001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkFilter.java (right): https://chromiumcodereview.appspot.com/1440623004/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkFilter.java:11: OFFLINE_PAGES("OFFLINE_PAGES"); Is it too late to change the value to something more elegant, such as, "offline"? https://chromiumcodereview.appspot.com/1440623004/diff/140001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java (right): https://chromiumcodereview.appspot.com/1440623004/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:372: || currentUIState == EnhancedBookmarkUIState.STATE_FOLDER : nit: ":" should go at the beginning of the next line https://chromiumcodereview.appspot.com/1440623004/diff/140001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java (right): https://chromiumcodereview.appspot.com/1440623004/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java:19: static final String URI_PERSIST_QUERY_NAME = "should_persist"; maybe just call this "persist" https://chromiumcodereview.appspot.com/1440623004/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java:61: static EnhancedBookmarkUIState createStateFromUrl(String url, you could change this to accept a Uri, to eliminate duplicate Uri parsing in this class https://chromiumcodereview.appspot.com/1440623004/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java:101: if (mFolder == null) return false; simpler: return mFolder != null && bookmarkModel.doesBookmarkExist(mFolder) && !mFolder.equals(bookmarkModel.getRootFolderId()); https://chromiumcodereview.appspot.com/1440623004/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java:112: private static boolean shouldPersist(String url) { Change the parameter to a Uri to avoid duplicate parsing in createStateFromUrl() https://chromiumcodereview.appspot.com/1440623004/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java:115: if (queryString == null) return true; don't need this line https://chromiumcodereview.appspot.com/1440623004/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java:131: static String createFolderUrl(BookmarkId folderId) { Could you reorder the methods in this class in a logical way? E.g. put all the static methods together near the top, put the private methods at the bottom, etc https://chromiumcodereview.appspot.com/1440623004/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java:145: @VisibleForTesting remove @VisibleForTesting https://chromiumcodereview.appspot.com/1440623004/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java:146: private static String encodePathToUrl(String url, String suffix, boolean shouldPersist) { rename this "createUrl(String baseUrl, String pathSuffix, boolean shouldPersist)"?
https://codereview.chromium.org/1440623004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkFilter.java (right): https://codereview.chromium.org/1440623004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkFilter.java:11: OFFLINE_PAGES("OFFLINE_PAGES"); On 2015/12/02 16:21:12, newt wrote: > Is it too late to change the value to something more elegant, such as, > "offline"? I wouldn't change it without the offline page folks taking a look here. I wonder how large the offline page's user base is. +fgorski can you confirm? Anyway if we want to change it we can always change the string after this patch. https://codereview.chromium.org/1440623004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java (right): https://codereview.chromium.org/1440623004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:372: || currentUIState == EnhancedBookmarkUIState.STATE_FOLDER : On 2015/12/02 16:21:12, newt wrote: > nit: ":" should go at the beginning of the next line Done. https://codereview.chromium.org/1440623004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java (right): https://codereview.chromium.org/1440623004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java:19: static final String URI_PERSIST_QUERY_NAME = "should_persist"; On 2015/12/02 16:21:13, newt wrote: > maybe just call this "persist" Done. https://codereview.chromium.org/1440623004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java:61: static EnhancedBookmarkUIState createStateFromUrl(String url, On 2015/12/02 16:21:12, newt wrote: > you could change this to accept a Uri, to eliminate duplicate Uri parsing in > this class Done. https://codereview.chromium.org/1440623004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java:101: if (mFolder == null) return false; On 2015/12/02 16:21:13, newt wrote: > simpler: > > return mFolder != null && bookmarkModel.doesBookmarkExist(mFolder) > && !mFolder.equals(bookmarkModel.getRootFolderId()); Done. https://codereview.chromium.org/1440623004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java:112: private static boolean shouldPersist(String url) { On 2015/12/02 16:21:13, newt wrote: > Change the parameter to a Uri to avoid duplicate parsing in createStateFromUrl() Done. https://codereview.chromium.org/1440623004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java:115: if (queryString == null) return true; On 2015/12/02 16:21:13, newt wrote: > don't need this line Done. https://codereview.chromium.org/1440623004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java:131: static String createFolderUrl(BookmarkId folderId) { On 2015/12/02 16:21:13, newt wrote: > Could you reorder the methods in this class in a logical way? E.g. put all the > static methods together near the top, put the private methods at the bottom, etc Done. https://codereview.chromium.org/1440623004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java:145: @VisibleForTesting On 2015/12/02 16:21:12, newt wrote: > remove @VisibleForTesting Done. https://codereview.chromium.org/1440623004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java:146: private static String encodePathToUrl(String url, String suffix, boolean shouldPersist) { On 2015/12/02 16:21:13, newt wrote: > rename this "createUrl(String baseUrl, String pathSuffix, boolean > shouldPersist)"? Done.
lgtm after comment :) Feel free to land this and then maybe address the OFFLINE_PAGES->offline renaming as a separate CL. https://codereview.chromium.org/1440623004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java (right): https://codereview.chromium.org/1440623004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUIState.java:115: private static Uri createUrl(String url, String suffix, boolean shouldPersist) { s/url/baseUrl/ and s/suffix/pathSuffix ? I think those parameter names are clearer.
The CQ bit was checked by ianwen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkimlabs@chromium.org, fgorski@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1440623004/#ps180001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440623004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440623004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by ianwen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org, kkimlabs@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/1440623004/#ps200001 (title: "Thank you, findbug")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440623004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440623004/200001
Message was sent while issue was closed.
Description was changed from ========== [Enhanced Bookmark]Rewrite initialization logic It proves to be really hard to change the first content users see in bookmark manager on both phones and tablets. This is because EBManager does not initialize itself the same as other native pages. This CL proposes a different approach about initializing the bookmark manager. In EBManager's constructor, it will always initialize itself in the loading mode. And after the constructor returns, EBPage and EBActivity call updateForUrl() with an initial url that is determined by some complicated logics in EBUtils. This procedure aligns with NativePageFactory's logic. To achieve this goal, several changes have been made: 1. Loading state no longer carries a url. Instead mInitialUrl is created to catch that info. 2. Detecting bookmark model loaded state has been made simple by introducing doAfterBookmarkModelLoaded in BookmarksBridge. 3. Now both EBActivity and EBPage need to call EBmanager#updateForUrl. 4. EBPage no longer calls tab#loadUrl if this url change is originally initiated by tab itself. BUG=554595 ========== to ========== [Enhanced Bookmark]Rewrite initialization logic It proves to be really hard to change the first content users see in bookmark manager on both phones and tablets. This is because EBManager does not initialize itself the same as other native pages. This CL proposes a different approach about initializing the bookmark manager. In EBManager's constructor, it will always initialize itself in the loading mode. And after the constructor returns, EBPage and EBActivity call updateForUrl() with an initial url that is determined by some complicated logics in EBUtils. This procedure aligns with NativePageFactory's logic. To achieve this goal, several changes have been made: 1. Loading state no longer carries a url. Instead mInitialUrl is created to catch that info. 2. Detecting bookmark model loaded state has been made simple by introducing doAfterBookmarkModelLoaded in BookmarksBridge. 3. Now both EBActivity and EBPage need to call EBmanager#updateForUrl. 4. EBPage no longer calls tab#loadUrl if this url change is originally initiated by tab itself. BUG=554595 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [Enhanced Bookmark]Rewrite initialization logic It proves to be really hard to change the first content users see in bookmark manager on both phones and tablets. This is because EBManager does not initialize itself the same as other native pages. This CL proposes a different approach about initializing the bookmark manager. In EBManager's constructor, it will always initialize itself in the loading mode. And after the constructor returns, EBPage and EBActivity call updateForUrl() with an initial url that is determined by some complicated logics in EBUtils. This procedure aligns with NativePageFactory's logic. To achieve this goal, several changes have been made: 1. Loading state no longer carries a url. Instead mInitialUrl is created to catch that info. 2. Detecting bookmark model loaded state has been made simple by introducing doAfterBookmarkModelLoaded in BookmarksBridge. 3. Now both EBActivity and EBPage need to call EBmanager#updateForUrl. 4. EBPage no longer calls tab#loadUrl if this url change is originally initiated by tab itself. BUG=554595 ========== to ========== [Enhanced Bookmark]Rewrite initialization logic It proves to be really hard to change the first content users see in bookmark manager on both phones and tablets. This is because EBManager does not initialize itself the same as other native pages. This CL proposes a different approach about initializing the bookmark manager. In EBManager's constructor, it will always initialize itself in the loading mode. And after the constructor returns, EBPage and EBActivity call updateForUrl() with an initial url that is determined by some complicated logics in EBUtils. This procedure aligns with NativePageFactory's logic. To achieve this goal, several changes have been made: 1. Loading state no longer carries a url. Instead mInitialUrl is created to catch that info. 2. Detecting bookmark model loaded state has been made simple by introducing doAfterBookmarkModelLoaded in BookmarksBridge. 3. Now both EBActivity and EBPage need to call EBmanager#updateForUrl. 4. EBPage no longer calls tab#loadUrl if this url change is originally initiated by tab itself. BUG=554595 Committed: https://crrev.com/27f0872193b01629cbd041d07a78fc014970e03a Cr-Commit-Position: refs/heads/master@{#362910} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/27f0872193b01629cbd041d07a78fc014970e03a Cr-Commit-Position: refs/heads/master@{#362910} |
