Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1969233002:
[OfflinePages] Make website settings popup rely on asynchronous API (Closed)
|
Created:
4 years, 7 months ago by chili Modified:
4 years, 7 months ago CC:
chromium-reviews, fgorski+watch_chromium.org, romax+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[OfflinePages] Make website settings popup rely on asynchronous API
BUG=607670
Committed: https://crrev.com/1119715ee9e4d99a54002f891fccb92090880ae6
Cr-Commit-Position: refs/heads/master@{#394469}
Patch Set 1 #Patch Set 2 : update #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 15
Patch Set 6 : Comment response #
Total comments: 16
Patch Set 7 : Resolving code review comments and running git cl format #
Messages
Total messages: 24 (9 generated)
Description was changed from ========== [OfflinePages] Make website settings popup rely on asynchronous API BUG=607670 ========== to ========== [OfflinePages] Make website settings popup rely on asynchronous API BUG=607670 ==========
chili@chromium.org changed reviewers: + dewittj@chromium.org
dewittj@chromium.org changed reviewers: + fgorski@chromium.org
+fgorski who wrote the original website settings code for offline pages. https://codereview.chromium.org/1969233002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/1969233002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:358: final String offlineUrl, final MultipleOfflinePageItemCallback callback) { Let's just remove this method, since we have the constraint that only one offline page can point to a local file:// URL. https://codereview.chromium.org/1969233002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:379: * matching {@link OfflinePageItem}, if any. nit: please mention that null is possible in the callback. https://codereview.chromium.org/1969233002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1969233002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java:908: OfflinePageBridge offlinePageBridge = OfflinePageBridge.getForProfile(tab.getProfile()); The bridge can be null if offline pages is not enabled, so either need to check for null or check that the feature is enabled before using the bridge. https://codereview.chromium.org/1969233002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_bridge.cc (right): https://codereview.chromium.org/1969233002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_bridge.cc:58: ScopedJavaLocalRef<jobject> ToJavaOfflinePageItem( nit: I think I prefer this as CreateJavaOfflinePageItem. https://codereview.chromium.org/1969233002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_bridge.cc:128: ScopedJavaGlobalRef<jobject> emptyResult; should this be ScopedJavaLocalRef? https://codereview.chromium.org/1969233002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_bridge.cc:131: Java_SingleOfflinePageItemCallback_onResult( This code would be simpler if you just rename emptyResult to j_result and in this if statement you can say j_result = ToJavaOfflinePageItem(env, *result); Then you only have one place to call Java_SingleOfflinePageItemCallback_onResult.
https://codereview.chromium.org/1969233002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/1969233002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:358: final String offlineUrl, final MultipleOfflinePageItemCallback callback) { On 2016/05/16 21:07:46, dewittj wrote: > Let's just remove this method, since we have the constraint that only one > offline page can point to a local file:// URL. Done. https://codereview.chromium.org/1969233002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:379: * matching {@link OfflinePageItem}, if any. On 2016/05/16 21:07:46, dewittj wrote: > nit: please mention that null is possible in the callback. Assuming you mean to add @Nullable reference in callback? https://codereview.chromium.org/1969233002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1969233002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java:908: OfflinePageBridge offlinePageBridge = OfflinePageBridge.getForProfile(tab.getProfile()); On 2016/05/16 21:07:46, dewittj wrote: > The bridge can be null if offline pages is not enabled, so either need to check > for null or check that the feature is enabled before using the bridge. Checking for null now. How do I check for enabled features? https://codereview.chromium.org/1969233002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_bridge.cc (right): https://codereview.chromium.org/1969233002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_bridge.cc:58: ScopedJavaLocalRef<jobject> ToJavaOfflinePageItem( On 2016/05/16 21:07:46, dewittj wrote: > nit: I think I prefer this as CreateJavaOfflinePageItem. I renamed this to 'ToJavaOfflinePageItem' to be consistent with the previous method 'ToJavaOfflinePageList'. Should we also rename that one to 'CreateJavaOfflinePageList' ? https://codereview.chromium.org/1969233002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_bridge.cc:128: ScopedJavaGlobalRef<jobject> emptyResult; On 2016/05/16 21:07:46, dewittj wrote: > should this be ScopedJavaLocalRef? Done. https://codereview.chromium.org/1969233002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_bridge.cc:131: Java_SingleOfflinePageItemCallback_onResult( On 2016/05/16 21:07:46, dewittj wrote: > This code would be simpler if you just rename emptyResult to j_result and in > this if statement you can say j_result = ToJavaOfflinePageItem(env, *result); > > Then you only have one place to call > Java_SingleOfflinePageItemCallback_onResult. I did start with that in a previous iteration. Perhaps due to lack of C++fu, it didn't compile with some error like "unable to copy ScopedJavaGlobalRef to ScopedJavaGlobalRef." Will try again.
mostly looks good. https://codereview.chromium.org/1969233002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1969233002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java:908: OfflinePageBridge offlinePageBridge = OfflinePageBridge.getForProfile(tab.getProfile()); On 2016/05/16 23:06:41, chili wrote: > On 2016/05/16 21:07:46, dewittj wrote: > > The bridge can be null if offline pages is not enabled, so either need to > check > > for null or check that the feature is enabled before using the bridge. > > Checking for null now. How do I check for enabled features? In case of offline pages: OfflinePageBridge#isOfflinePagesEnabled and you dive deep from there. https://codereview.chromium.org/1969233002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/1969233002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:355: * matching {@link OfflinePageItem} if found. Will pass back null if not. nit: <code>null</code> https://codereview.chromium.org/1969233002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1969233002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java:921: if (item != null) { did you remove the comment on purpose? // Get formatted creation date and original URL of the offline copy. https://codereview.chromium.org/1969233002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java:928: new WebsiteSettingsPopup(activity, tab.getProfile(), webContents, tab.getWebContents() does not require final at the top of the method https://codereview.chromium.org/1969233002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_bridge.cc (right): https://codereview.chromium.org/1969233002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bridge.cc:57: nit: remove extra empty line. https://codereview.chromium.org/1969233002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bridge.cc:130: if (result) { nit: you don't need braces if you have one line if followed by one line body... But this is optional, so {yourself}: healthcare reform joke coming: If you like your braces you can keep them... https://codereview.chromium.org/1969233002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bridge.cc:131: j_result = ToJavaOfflinePageItem(env, *result); nit: indentation should be 2 spaces... like below, run git cl format. https://codereview.chromium.org/1969233002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bridge.cc:318: GURL(ConvertJavaStringToUTF8(env, j_offline_url)), nit: run git cl format... too many spaces here. https://codereview.chromium.org/1969233002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_bridge.h (right): https://codereview.chromium.org/1969233002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bridge.h:116: base::android::ScopedJavaLocalRef<jobject> CreateOfflinePageItem( did you mean to remove this? I think you are deleting the method.
https://codereview.chromium.org/1969233002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/1969233002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:379: * matching {@link OfflinePageItem}, if any. On 2016/05/16 23:06:41, chili wrote: > On 2016/05/16 21:07:46, dewittj wrote: > > nit: please mention that null is possible in the callback. > > Assuming you mean to add @Nullable reference in callback? I was thinking in this comment. But you did that too :) https://codereview.chromium.org/1969233002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_bridge.cc (right): https://codereview.chromium.org/1969233002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_bridge.cc:58: ScopedJavaLocalRef<jobject> ToJavaOfflinePageItem( On 2016/05/16 23:06:41, chili wrote: > On 2016/05/16 21:07:46, dewittj wrote: > > nit: I think I prefer this as CreateJavaOfflinePageItem. > > I renamed this to 'ToJavaOfflinePageItem' to be consistent with the previous > method 'ToJavaOfflinePageList'. Should we also rename that one to > 'CreateJavaOfflinePageList' ? I guess it is a little different to me, since ToJavaOfflinePageList actually doesn't create a list. I leave it up to you :)
https://codereview.chromium.org/1969233002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/1969233002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:355: * matching {@link OfflinePageItem} if found. Will pass back null if not. On 2016/05/17 04:12:25, fgorski wrote: > nit: <code>null</code> Done. https://codereview.chromium.org/1969233002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1969233002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java:921: if (item != null) { On 2016/05/17 04:12:25, fgorski wrote: > did you remove the comment on purpose? > > // Get formatted creation date and original URL of the offline copy. I did because it didn't seem to add any information that the code doesn't already document. I added it back now. https://codereview.chromium.org/1969233002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java:928: new WebsiteSettingsPopup(activity, tab.getProfile(), webContents, On 2016/05/17 04:12:25, fgorski wrote: > tab.getWebContents() does not require final at the top of the method It does if I set it as a variable and use it inside this callback (anonymous class). Alternatively, I can remove it altogether and only access tab https://codereview.chromium.org/1969233002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_bridge.cc (right): https://codereview.chromium.org/1969233002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bridge.cc:57: On 2016/05/17 04:12:25, fgorski wrote: > nit: remove extra empty line. Done. https://codereview.chromium.org/1969233002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bridge.cc:130: if (result) { On 2016/05/17 04:12:25, fgorski wrote: > nit: you don't need braces if you have one line if followed by one line body... > > But this is optional, so {yourself}: healthcare reform joke coming: > If you like your braces you can keep them... Habit from Java >_> https://codereview.chromium.org/1969233002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bridge.cc:131: j_result = ToJavaOfflinePageItem(env, *result); On 2016/05/17 04:12:25, fgorski wrote: > nit: indentation should be 2 spaces... like below, run git cl format. Done. https://codereview.chromium.org/1969233002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bridge.cc:318: GURL(ConvertJavaStringToUTF8(env, j_offline_url)), On 2016/05/17 04:12:25, fgorski wrote: > nit: run git cl format... too many spaces here. More touch-ups to vim config... https://codereview.chromium.org/1969233002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_bridge.h (right): https://codereview.chromium.org/1969233002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bridge.h:116: base::android::ScopedJavaLocalRef<jobject> CreateOfflinePageItem( On 2016/05/17 04:12:25, fgorski wrote: > did you mean to remove this? I think you are deleting the method. Done.
lgtm
lgtm
The CQ bit was checked by chili@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1969233002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1969233002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
chili@chromium.org changed reviewers: + tedchoc@chromium.org
Add Ted C to reviewers for OWNERS approval
owners - lgtm
The CQ bit was checked by chili@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1969233002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1969233002/120001
Message was sent while issue was closed.
Description was changed from ========== [OfflinePages] Make website settings popup rely on asynchronous API BUG=607670 ========== to ========== [OfflinePages] Make website settings popup rely on asynchronous API BUG=607670 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [OfflinePages] Make website settings popup rely on asynchronous API BUG=607670 ========== to ========== [OfflinePages] Make website settings popup rely on asynchronous API BUG=607670 Committed: https://crrev.com/1119715ee9e4d99a54002f891fccb92090880ae6 Cr-Commit-Position: refs/heads/master@{#394469} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/1119715ee9e4d99a54002f891fccb92090880ae6 Cr-Commit-Position: refs/heads/master@{#394469} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
