Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2176013002:
Add support for OfflinePagesDownloadBridge. (Closed)
|
Created:
4 years, 5 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. |
DescriptionAdd support for OfflinePagesDownloadBridge.
This adds initial DownloadUIAdapter and DownloadUIItem.
Adapter is a class that caches filtered subset of OfflinePages that should be
shown in DownloadsUI. DownloadUIItem mimics DownloadItem and is a collection of fields
needed for UI.
BUG=630817
Committed: https://crrev.com/d35eb5bffa7c8ab10a33ec736644c5e91df3e32b
Cr-Commit-Position: refs/heads/master@{#407923}
Patch Set 1 #
Total comments: 29
Patch Set 2 : CR feedback and tests. #
Total comments: 36
Patch Set 3 : More comments and compile fixes #
Total comments: 1
Patch Set 4 : gypi fix #Dependent Patchsets: Messages
Total messages: 25 (13 generated)
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... File components/offline_pages/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.cc:26: void DownloadUIAdapter::AddObserver(Observer* observer) { Consider DCHECKing the observer. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.cc:115: nit: this empty line probably not needed. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.cc:122: std::string guid = item->guid; this can probably be const & https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.cc:123: if (items_.find(guid) == items_.end()) { This code does not do the validation of duplicate GUIDs like the one above. turning added_guids (and probably changed_guids) to a set and checking against it allows for making it here. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.cc:125: items_[guid] = std::move(item); shouldn't this happen for both changed and added case? After all we want to update the cache. You should probably move it after the if statement https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.cc:133: ItemAdded(*((*items_.find(guid)).second.get()))); How about the two options below for readability. FOR_EACH_OBSERVER(Observer, observers_, ItemAdded(*(items_.find(guid)->second.get())) OR: for (auto& guid : added_guids) { const DownloadUIItem& item = *(items_.find(guid)->second.get()); FOR_EACH_OBSERVER(Observer, observers_, ItemAdded(item)); } Applies below as well. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... File components/offline_pages/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.h:50: DownloadUIAdapter(OfflinePageModel* model); Make this constructor explicit, as it accepts only a single parameter. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.h:62: const DownloadUIItem* GetItem(std::string guid) const; const std::string& guid https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.h:71: nit: empty line not needed. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.h:77: void NotifyOnLoad(Observer* observer); perhaps rename: NotifyItemsLoaded? https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... File components/offline_pages/downloads/download_ui_item.h (right): https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_item.h:30: base::FilePath target_path; having an extra method: GetOfflineURL() might be helpful to add (now or later) for cases where we want to use download item to launch offline page. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_item.h:33: std::string mime_type; Based on comment from Dan in my code, we don't need that for offline page items. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_item.h:36: base::Time start_time_ms; if it is a base::Time, then _ms is not appropriate here.
https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... File components/offline_pages/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.cc:121: DownloadUIItemsMap::const_iterator it = items_.find(item->guid); this is not used.
https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... File components/offline_pages/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.cc:26: void DownloadUIAdapter::AddObserver(Observer* observer) { On 2016/07/25 17:09:56, fgorski wrote: > Consider DCHECKing the observer. Done, not sure it is useful since it'll fail fast anyways. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.cc:115: On 2016/07/25 17:09:56, fgorski wrote: > nit: this empty line probably not needed. Done. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.cc:121: DownloadUIItemsMap::const_iterator it = items_.find(item->guid); On 2016/07/25 17:28:41, fgorski wrote: > this is not used. Done. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.cc:122: std::string guid = item->guid; On 2016/07/25 17:09:56, fgorski wrote: > this can probably be const & Done. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.cc:123: if (items_.find(guid) == items_.end()) { On 2016/07/25 17:09:56, fgorski wrote: > This code does not do the validation of duplicate GUIDs like the one above. > > turning added_guids (and probably changed_guids) to a set and checking against > it allows for making it here. It's not clear to me what this method should enforce uniqueness of entries for the same page here (or de-dupe anything), especially if it was added and changed after that etc. It feels it should be done at the point where OnOfflinePagesChanged is originating. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.cc:125: items_[guid] = std::move(item); On 2016/07/25 17:09:56, fgorski wrote: > shouldn't this happen for both changed and added case? > After all we want to update the cache. > > You should probably move it after the if statement Done. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.cc:133: ItemAdded(*((*items_.find(guid)).second.get()))); On 2016/07/25 17:09:56, fgorski wrote: > How about the two options below for readability. > > FOR_EACH_OBSERVER(Observer, observers_, > ItemAdded(*(items_.find(guid)->second.get())) > > OR: > for (auto& guid : added_guids) { > const DownloadUIItem& item = *(items_.find(guid)->second.get()); > FOR_EACH_OBSERVER(Observer, observers_, ItemAdded(item)); > } > > Applies below as well. Done. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... File components/offline_pages/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.h:50: DownloadUIAdapter(OfflinePageModel* model); On 2016/07/25 17:09:57, fgorski wrote: > Make this constructor explicit, as it accepts only a single parameter. Done. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.h:62: const DownloadUIItem* GetItem(std::string guid) const; On 2016/07/25 17:09:56, fgorski wrote: > const std::string& guid Done. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.h:71: On 2016/07/25 17:09:56, fgorski wrote: > nit: empty line not needed. Done. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.h:77: void NotifyOnLoad(Observer* observer); On 2016/07/25 17:09:57, fgorski wrote: > perhaps rename: NotifyItemsLoaded? Done. https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... File components/offline_pages/downloads/download_ui_item.h (right): https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_item.h:30: base::FilePath target_path; On 2016/07/25 17:09:57, fgorski wrote: > having an extra method: GetOfflineURL() might be helpful to add (now or later) > for cases where we want to use download item to launch offline page. I thought UI won't launch anything, instead they will pass "open" command to us and we'll open or whatever semantically is needed for the download item... https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_item.h:33: std::string mime_type; On 2016/07/25 17:09:57, fgorski wrote: > Based on comment from Dan in my code, we don't need that for offline page items. Removed, we might need it later (to be a package). https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_item.h:36: base::Time start_time_ms; On 2016/07/25 17:09:57, fgorski wrote: > if it is a base::Time, then _ms is not appropriate here. Done.
dimich@chromium.org changed reviewers: + blundell@chromium.org
+Colin Blundell as OWNER for components/BUILD.gn change. Thanks!
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
//components/BUILD.gn lgtm
https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... File components/offline_pages/downloads/download_ui_item.h (right): https://codereview.chromium.org/2176013002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_item.h:30: base::FilePath target_path; On 2016/07/26 00:41:25, Dmitry Titov wrote: > On 2016/07/25 17:09:57, fgorski wrote: > > having an extra method: GetOfflineURL() might be helpful to add (now or later) > > for cases where we want to use download item to launch offline page. > > I thought UI won't launch anything, instead they will pass "open" command to us > and we'll open or whatever semantically is needed for the download item... Acknowledged. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:102: DCHECK(items_.find(new_item->guid) == items_.end()); I like the idea below: const std::string& guid = page.client_id.id; It would allow you to first do the DCHECK and then new up an object and put it in the map. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:128: added_guids.push_back(guid); nit: Can you move this line up or down? It feels like it breaks apart 2 lines that are much more related to each other. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:131: // Only one added page Interesting. Do we even need the update to be in the interface? <Update>I just saw your comment in test. We can discuss that today</Update> https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.h:7: nit: #include <map>, <memory>, <string>, <stdint.h> There are other places in the code where these are missing. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.h:44: virtual void ItemDeleted(std::string guid) = 0; Caught that later yesterday, sorry. This should also be const& https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter_unittest.cc (right): https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:7: #include <stdint.h> #include <map> #include <memory> #include <string> #include <vector> https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:82: for (const auto& page : pages) { nit: {} not needed. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:107: std::unique_ptr<DownloadUIAdapter> adapter; I think this should still be adapter_ and exposed through adapter() https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:109: std::map<int64_t, OfflinePageItem> pages; same here. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:115: DISALLOW_COPY_AND_ASSIGN(MockOfflinePageModel); nit: fix alignment please. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:160: void DownloadUIAdapterTest::TearDown() { You don't need this method if it is empty. You will use the base class implementation. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... File components/offline_pages/downloads/download_ui_item.cc (right): https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_item.cc:7: #include "net/base/filename_util.h" I believe this is what is breaking the build. Good news of course is that if we don't convert FP to URL we don't need this here ;) https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... File components/offline_pages/downloads/download_ui_item.h (right): https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_item.h:8: #include <string> #include <stdint.h>
4 more comments (as my local compiler complains)... Overall things look good. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter_unittest.cc (right): https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:62: void AddObserver(Observer* observer) { mark as override https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:67: void RemoveObserver(Observer* observer) { mark as override https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:74: void GetAllPages(const MultipleOfflinePageItemCallback& callback) { mark as override https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:193: EXPECT_EQ(1, model->pages.size()); 1UL (as size() is size_t)
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...
https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:102: DCHECK(items_.find(new_item->guid) == items_.end()); On 2016/07/26 16:25:21, fgorski wrote: > I like the idea below: > const std::string& guid = page.client_id.id; > > It would allow you to first do the DCHECK and then new up an object and put it > in the map. Done. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:128: added_guids.push_back(guid); On 2016/07/26 16:25:21, fgorski wrote: > nit: Can you move this line up or down? It feels like it breaks apart 2 lines > that are much more related to each other. Done. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:131: // Only one added page On 2016/07/26 16:25:21, fgorski wrote: > Interesting. Do we even need the update to be in the interface? > > <Update>I just saw your comment in test. We can discuss that today</Update> I'd have it in interface. We will need it soon (indicating some progress or even change in response to user actions in UI. We could add it later but it's nice to have interfaces early even if they are not 100% implemented. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.h:7: On 2016/07/26 16:25:21, fgorski wrote: > nit: #include <map>, <memory>, <string>, <stdint.h> > > There are other places in the code where these are missing. Done. Don't think I need stdint.h here... https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.h:44: virtual void ItemDeleted(std::string guid) = 0; On 2016/07/26 16:25:21, fgorski wrote: > Caught that later yesterday, sorry. > This should also be const& Done. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter_unittest.cc (right): https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:7: #include <stdint.h> On 2016/07/26 16:25:21, fgorski wrote: > #include <map> > #include <memory> > #include <string> > #include <vector> Done. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:62: void AddObserver(Observer* observer) { On 2016/07/26 18:03:59, fgorski wrote: > mark as override Done. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:67: void RemoveObserver(Observer* observer) { On 2016/07/26 18:03:59, fgorski wrote: > mark as override Done. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:74: void GetAllPages(const MultipleOfflinePageItemCallback& callback) { On 2016/07/26 18:03:59, fgorski wrote: > mark as override Done. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:82: for (const auto& page : pages) { On 2016/07/26 16:25:21, fgorski wrote: > nit: {} not needed. Done. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:107: std::unique_ptr<DownloadUIAdapter> adapter; On 2016/07/26 16:25:21, fgorski wrote: > I think this should still be adapter_ and exposed through adapter() I'd like to keep it as this in test code, since it's accessed a lot, making test cases clearer... I do the same in the test class below as well, making members that are accesses in tests just public... https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:109: std::map<int64_t, OfflinePageItem> pages; On 2016/07/26 16:25:21, fgorski wrote: > same here. Same here. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:115: DISALLOW_COPY_AND_ASSIGN(MockOfflinePageModel); On 2016/07/26 16:25:21, fgorski wrote: > nit: fix alignment please. Done. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:160: void DownloadUIAdapterTest::TearDown() { On 2016/07/26 16:25:21, fgorski wrote: > You don't need this method if it is empty. > You will use the base class implementation. Done. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:193: EXPECT_EQ(1, model->pages.size()); On 2016/07/26 18:03:59, fgorski wrote: > 1UL (as size() is size_t) Done. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... File components/offline_pages/downloads/download_ui_item.cc (right): https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_item.cc:7: #include "net/base/filename_util.h" On 2016/07/26 16:25:21, fgorski wrote: > I believe this is what is breaking the build. Good news of course is that if we > don't convert FP to URL we don't need this here ;) Done. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... File components/offline_pages/downloads/download_ui_item.h (right): https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_item.h:8: #include <string> On 2016/07/26 16:25:21, fgorski wrote: > #include <stdint.h> Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm mod gypi file problem https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter_unittest.cc (right): https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:107: std::unique_ptr<DownloadUIAdapter> adapter; On 2016/07/26 19:56:48, Dmitry Titov wrote: > On 2016/07/26 16:25:21, fgorski wrote: > > I think this should still be adapter_ and exposed through adapter() > > I'd like to keep it as this in test code, since it's accessed a lot, making test > cases clearer... > > I do the same in the test class below as well, making members that are accesses > in tests just public... Acknowledged. https://codereview.chromium.org/2176013002/diff/20001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:109: std::map<int64_t, OfflinePageItem> pages; On 2016/07/26 19:56:48, Dmitry Titov wrote: > On 2016/07/26 16:25:21, fgorski wrote: > > same here. > > Same here. Acknowledged. https://codereview.chromium.org/2176013002/diff/40001/components/offline_page... File components/offline_pages.gypi (right): https://codereview.chromium.org/2176013002/diff/40001/components/offline_page... components/offline_pages.gypi:103: 'download_ui_adapter.cc', compilation issue caused by lack of path
The CQ bit was checked by dimich@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from blundell@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2176013002/#ps60001 (title: "gypi fix")
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 #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add support for OfflinePagesDownloadBridge. This adds initial DownloadUIAdapter and DownloadUIItem. Adapter is a class that caches filtered subset of OfflinePages that should be shown in DownloadsUI. DownloadUIItem mimics DownloadItem and is a collection of fields needed for UI. BUG=630817 ========== to ========== Add support for OfflinePagesDownloadBridge. This adds initial DownloadUIAdapter and DownloadUIItem. Adapter is a class that caches filtered subset of OfflinePages that should be shown in DownloadsUI. DownloadUIItem mimics DownloadItem and is a collection of fields needed for UI. BUG=630817 Committed: https://crrev.com/d35eb5bffa7c8ab10a33ec736644c5e91df3e32b Cr-Commit-Position: refs/heads/master@{#407923} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d35eb5bffa7c8ab10a33ec736644c5e91df3e32b Cr-Commit-Position: refs/heads/master@{#407923} |
