Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2221293002:
Implement deleteItem and openItem on OfflinePageDownloadBridge. (Closed)
|
Created:
4 years, 4 months ago by Dmitry Titov Modified:
4 years, 4 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+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. |
DescriptionImplement deleteItem and openItem on OfflinePageDownloadBridge.
BUG=630817
Committed: https://crrev.com/c9623e1995016f0d61c2333426fad9e3adc231e2
Cr-Commit-Position: refs/heads/master@{#411802}
Patch Set 1 #Patch Set 2 : cleanup #
Total comments: 24
Patch Set 3 : CR feedback #Patch Set 4 : cleanup #
Total comments: 16
Patch Set 5 : CR feedback #
Total comments: 7
Patch Set 6 : more updates #
Messages
Total messages: 29 (17 generated)
The CQ bit was checked by dimich@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dimich@chromium.org changed reviewers: + fgorski@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
please add tests to the adapter on C++ side after you fix compilation. I'll happily discuss my comments tomorrow. https://codereview.chromium.org/2221293002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2221293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:20: using base::android::ConvertUTF8ToJavaString; please add missing using statement here: using base::android::JavaParamRef; https://codereview.chromium.org/2221293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:78: const JavaParamRef<jobject>& j_result_obj) { because of this. https://codereview.chromium.org/2221293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:88: const base::android::JavaParamRef<jstring>& j_guid) { remove base::android:: from here, and other JavaParamRef https://codereview.chromium.org/2221293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:97: const base::android::JavaParamRef<jstring>& j_guid) { including here. https://codereview.chromium.org/2221293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:105: const base::android::JavaParamRef<jstring>& j_guid) { and here. https://codereview.chromium.org/2221293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:110: return ConvertUTF8ToJavaString(env, url); It should simply work if url is empty, I don't think you need 2 return statements here. https://codereview.chromium.org/2221293002/diff/20001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2221293002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:100: model_->GetAllPages( On an off chance that you didn't consider it yet. What was the disadvantage of putting offline_id, id or item_id on the download UI item? This would save you going back to the DB and bulk reading all offline pages every time user is interacting with UI and expects it to be responsive. I'd consider that and ability to internally distinguish between requests and offline pages. This is so that this method looks up in only one place and knows where to go from there. https://codereview.chromium.org/2221293002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:112: offline_url = (*it).second->offline_url; How about dropping the internal variable and doing this? return it->second->offline_url; return std::string(); https://codereview.chromium.org/2221293002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:174: void DownloadUIAdapter::OnDeleteItemByGuid( this method needs git cl format. Please run it. https://codereview.chromium.org/2221293002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:191: // Do nothing here. TODO: Add UMA for Download UI actions. https://codereview.chromium.org/2221293002/diff/20001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2221293002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.h:76: std::string GetOfflineUrlByGuid(guid); this is where it does not compile. https://codereview.chromium.org/2221293002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.h:77: nit: remove empty line
The CQ bit was checked by dimich@chromium.org to run a CQ dry run
The CQ bit was unchecked by dimich@chromium.org
The CQ bit was checked by dimich@chromium.org to run a CQ dry run
The CQ bit was unchecked by dimich@chromium.org
The CQ bit was checked by dimich@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2221293002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2221293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:20: using base::android::ConvertUTF8ToJavaString; On 2016/08/09 04:00:50, fgorski wrote: > please add missing using statement here: > using base::android::JavaParamRef; Done. https://codereview.chromium.org/2221293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:78: const JavaParamRef<jobject>& j_result_obj) { On 2016/08/09 04:00:50, fgorski wrote: > because of this. Acknowledged. https://codereview.chromium.org/2221293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:88: const base::android::JavaParamRef<jstring>& j_guid) { On 2016/08/09 04:00:50, fgorski wrote: > remove base::android:: from here, and other JavaParamRef Done. https://codereview.chromium.org/2221293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:97: const base::android::JavaParamRef<jstring>& j_guid) { On 2016/08/09 04:00:50, fgorski wrote: > including here. Done. https://codereview.chromium.org/2221293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:105: const base::android::JavaParamRef<jstring>& j_guid) { On 2016/08/09 04:00:50, fgorski wrote: > and here. Done. https://codereview.chromium.org/2221293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:110: return ConvertUTF8ToJavaString(env, url); On 2016/08/09 04:00:50, fgorski wrote: > It should simply work if url is empty, I don't think you need 2 return > statements here. Done. https://codereview.chromium.org/2221293002/diff/20001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2221293002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:100: model_->GetAllPages( On 2016/08/09 04:00:50, fgorski wrote: > On an off chance that you didn't consider it yet. What was the disadvantage of > putting offline_id, id or item_id on the download UI item? This would save you > going back to the DB and bulk reading all offline pages every time user is > interacting with UI and expects it to be responsive. > > I'd consider that and ability to internally distinguish between requests and > offline pages. This is so that this method looks up in only one place and knows > where to go from there. Done, by adding ItemInfo internal struct of whihc DownloadUIItem is a field, and additional info is added to ItemInfo. This way, there is no leakage of stuff into DownloadUIItem. https://codereview.chromium.org/2221293002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:112: offline_url = (*it).second->offline_url; On 2016/08/09 04:00:50, fgorski wrote: > How about dropping the internal variable and doing this? > return it->second->offline_url; > return std::string(); Done. https://codereview.chromium.org/2221293002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:174: void DownloadUIAdapter::OnDeleteItemByGuid( On 2016/08/09 04:00:50, fgorski wrote: > this method needs git cl format. Please run it. Done. https://codereview.chromium.org/2221293002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:191: // Do nothing here. On 2016/08/09 04:00:50, fgorski wrote: > TODO: Add UMA for Download UI actions. Done. https://codereview.chromium.org/2221293002/diff/20001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2221293002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.h:76: std::string GetOfflineUrlByGuid(guid); On 2016/08/09 04:00:51, fgorski wrote: > this is where it does not compile. Done. https://codereview.chromium.org/2221293002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.h:77: On 2016/08/09 04:00:51, fgorski wrote: > nit: remove empty line Done.
dimich@chromium.org changed reviewers: + dfalcantara@chromium.org
+ Dan to take a look at incoming methods on the bridge and as OWNER of the junit test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2221293002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2221293002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:111: return ConvertUTF8ToJavaString(env, url); you know what, I think depending on what you want to get on the other side, (null vs. empty string), you might have had it correct the first time. I remember mtreib using the if() to return null I think instead of empty string. https://codereview.chromium.org/1514833002 Feel free to revert this to the previous code, which was correct if you wanted null on Java side instead of an empty string. https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:86: if (item.second->ui_item->guid == client_id.id) { Since items_ are keyed with guid: items_.find(client_id.id) is possible and more efficient. https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:102: DownloadUIAdapter::GetItem(const std::string& guid) const { nit: it that indentation what git cl format delivers here? https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:131: return it->second->offline_url.spec(); why do you prefer string over GURL? If this is because of Java/Android, appropriate place to convert is the bridge. GURL would be more general and applicable on other platforms. https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.h:10: #include <string> #include <vector> https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.h:85: ~ItemInfo(); = default; would probably do here. https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.h:87: std::unique_ptr<DownloadUIItem> ui_item; do we need to do anything about copying of these objects?
PTAL https://codereview.chromium.org/2221293002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2221293002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:111: return ConvertUTF8ToJavaString(env, url); On 2016/08/12 15:43:52, fgorski wrote: > you know what, I think depending on what you want to get on the other side, > (null vs. empty string), you might have had it correct the first time. > I remember mtreib using the if() to return null I think instead of empty string. > https://codereview.chromium.org/1514833002 > > Feel free to revert this to the previous code, which was correct if you wanted > null on Java side instead of an empty string. I check for empty string on Java side and not for null. I think it's better to return null indeed and check for null rather than a content of a string. https://codereview.chromium.org/2221293002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:111: return ConvertUTF8ToJavaString(env, url); On 2016/08/12 15:43:52, fgorski wrote: > you know what, I think depending on what you want to get on the other side, > (null vs. empty string), you might have had it correct the first time. > I remember mtreib using the if() to return null I think instead of empty string. > https://codereview.chromium.org/1514833002 > > Feel free to revert this to the previous code, which was correct if you wanted > null on Java side instead of an empty string. reverted back tand check for null. needed it for GURL anyways. https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:86: if (item.second->ui_item->guid == client_id.id) { On 2016/08/12 15:43:52, fgorski wrote: > Since items_ are keyed with guid: > items_.find(client_id.id) is possible and more efficient. Done. https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:102: DownloadUIAdapter::GetItem(const std::string& guid) const { On 2016/08/12 15:43:52, fgorski wrote: > nit: it that indentation what git cl format delivers here? I've run it again. https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:131: return it->second->offline_url.spec(); On 2016/08/12 15:43:52, fgorski wrote: > why do you prefer string over GURL? > If this is because of Java/Android, appropriate place to convert is the bridge. > GURL would be more general and applicable on other platforms. Done. https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.h:10: #include <string> On 2016/08/12 15:43:52, fgorski wrote: > #include <vector> Done. https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.h:85: ~ItemInfo(); On 2016/08/12 15:43:52, fgorski wrote: > = default; would probably do here. Apparently, the 'complex types must have non-inline destructors' according to clang. https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.h:87: std::unique_ptr<DownloadUIItem> ui_item; On 2016/08/12 15:43:52, fgorski wrote: > do we need to do anything about copying of these objects? Done.
lgtm after small fixes and optional nits. https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2221293002/diff/60001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:102: DownloadUIAdapter::GetItem(const std::string& guid) const { On 2016/08/12 19:43:37, Dmitry Titov wrote: > On 2016/08/12 15:43:52, fgorski wrote: > > nit: it that indentation what git cl format delivers here? > > I've run it again. Acknowledged. https://codereview.chromium.org/2221293002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2221293002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:37: env, j_result_obj, ConvertUTF8ToJavaString(env, item->guid).obj(), nit: there is a patch out removing the .obj() https://codereview.chromium.org/2237943002/ you probably want to clean this one as well (only applies to your new code though) https://codereview.chromium.org/2221293002/diff/80001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2221293002/diff/80001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:196: std::string name_space = client_id.name_space; nit: would const & work? Otherwise you are making a copy. https://codereview.chromium.org/2221293002/diff/80001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2221293002/diff/80001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.h:85: ItemInfo(const OfflinePageItem& page); mark explicit, please
https://codereview.chromium.org/2221293002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2221293002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:37: env, j_result_obj, ConvertUTF8ToJavaString(env, item->guid).obj(), On 2016/08/12 20:01:14, fgorski wrote: > nit: there is a patch out removing the .obj() > https://codereview.chromium.org/2237943002/ > > you probably want to clean this one as well (only applies to your new code > though) Acknowledged. https://codereview.chromium.org/2221293002/diff/80001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2221293002/diff/80001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:196: std::string name_space = client_id.name_space; On 2016/08/12 20:01:14, fgorski wrote: > nit: would const & work? Otherwise you are making a copy. Done. https://codereview.chromium.org/2221293002/diff/80001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2221293002/diff/80001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.h:85: ItemInfo(const OfflinePageItem& page); On 2016/08/12 20:01:14, fgorski wrote: > mark explicit, please Done.
Bridge bits lgtm. Might be worth hooking into OfflinePageItemWrapper#open() and OfflinePageItemWrapper#delete(), but I can tackle that later if needed. https://chromiumcodereview.appspot.com/2221293002/diff/80001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java (right): https://chromiumcodereview.appspot.com/2221293002/diff/80001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:141: tabDelegate.createNewTab(params, TabLaunchType.FROM_LINK, null); I might end up having to tweak this work like Bookmarks later, depending on what the transition between the downloads page and the browser Activity looks like. Definitely a polish thing, though.
The CQ bit was checked by dimich@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2221293002/#ps100001 (title: "more updates")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Implement deleteItem and openItem on OfflinePageDownloadBridge. BUG=630817 ========== to ========== Implement deleteItem and openItem on OfflinePageDownloadBridge. BUG=630817 Committed: https://crrev.com/c9623e1995016f0d61c2333426fad9e3adc231e2 Cr-Commit-Position: refs/heads/master@{#411802} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c9623e1995016f0d61c2333426fad9e3adc231e2 Cr-Commit-Position: refs/heads/master@{#411802} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
