Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1428833005:
MD Downloads: track downloads in C++, dispatch discrete JS updates (Closed)
DescriptionMD Downloads: track downloads in C++, dispatch discrete JS updates
Instead of both the C++ and the JS needing to create sparse and dense
representations of download items (and reconcile the differences in some
cases), just keep a sorted representation in C++ and send discrete,
positioned updates to the JS. This makes the C++ act more like a
controller and makes the JS simply respond to commands in a performant
way.
R=asanka@chromium.org,dpapad@chromium.org
BUG=526577
Committed: https://crrev.com/099dfdd09d2dd8e2d8228c955a89069fc971d625
Cr-Commit-Position: refs/heads/master@{#362063}
Committed: https://crrev.com/85f052423a3e2476f1afe4fdc11dfd42ef4439dd
Cr-Commit-Position: refs/heads/master@{#362790}
Patch Set 1 : moar testz #
Total comments: 14
Patch Set 2 : asdf #
Total comments: 44
Patch Set 3 : dpapad@ + asanka@ feedback + self-review #
Total comments: 6
Patch Set 4 : dpapad@ review #Patch Set 5 : silly windows #
Total comments: 2
Patch Set 6 : trim test #Patch Set 7 : asanka@ review #Patch Set 8 : GN test fixes #Patch Set 9 : gyp shufflin #Patch Set 10 : include_dirs? #Patch Set 11 : removing gtestjs #Patch Set 12 : merge #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 58 (28 generated)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controlled and makes the JS simply respond to commands in a performant way. R=asanka@chromium.org BUG=526577 ========== to ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controlled and makes the JS simply respond to commands in a performant way. R=dpapad@chromium.org BUG=526577 ==========
dbeam@chromium.org changed reviewers: + dpapad@chromium.org - asanka@chromium.org
+dpapad@ for sanity checking
https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_downloads/action_service.js (right): https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/resource... chrome/browser/resources/md_downloads/action_service.js:82: function trim(s) { return s.trim(); } Nit: Add @param, @return? https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/resource... chrome/browser/resources/md_downloads/action_service.js:84: searchText ? searchText.split(/"([^"]*)"/).map(trim) : []); Nit: Use isSearching instead of implicit conversion to boolean? chrome.send('getDownloads', this.isSearching() ? searchText.split(/"([^"]*)"/).map(trim) : []); https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_downloads/manager.js (right): https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/resource... chrome/browser/resources/md_downloads/manager.js:145: /** @return {downloads.Manager} */ @return {!downloads.Manager} https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/resource... chrome/browser/resources/md_downloads/manager.js:147: return document.querySelector('downloads-manager'); Can the result of this call be cached, to avoid searching document every time get() is called?
Overall looks good. https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_downloads/manager.js (right): https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/resource... chrome/browser/resources/md_downloads/manager.js:162: Manager.size = function() { I couldn't find any uses of Manager.size(). Does it need to be exposed? https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc (right): https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:242: DCHECK(download_item->GetDangerType() == Is this DCHECK needed given that there is already a NOT_REACHED at GetDangerTypeString()? https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:381: void DownloadsListTracker::InsertItems(const SortedSet::iterator& start, Nit: Can we improve a bit the consistency of function naming? Specifically I mean the following: - If a method simply calls to JS (without doing any other book-keeping), prefix it with "Call" (CallUpdateItem, CallClearAl already do that). - If a method does book-keeping besides calling to JS then no need for the prefix. Following the above convention InsertItems => CallInsertItems
Description was changed from ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controlled and makes the JS simply respond to commands in a performant way. R=dpapad@chromium.org BUG=526577 ========== to ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controller and makes the JS simply respond to commands in a performant way. R=dpapad@chromium.org BUG=526577 ==========
https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_downloads/action_service.js (right): https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/resource... chrome/browser/resources/md_downloads/action_service.js:82: function trim(s) { return s.trim(); } On 2015/11/20 21:55:15, dpapad wrote: > Nit: Add @param, @return? Done. https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/resource... chrome/browser/resources/md_downloads/action_service.js:84: searchText ? searchText.split(/"([^"]*)"/).map(trim) : []); On 2015/11/20 21:55:15, dpapad wrote: > Nit: Use isSearching instead of implicit conversion to boolean? > > chrome.send('getDownloads', > this.isSearching() ? searchText.split(/"([^"]*)"/).map(trim) : []); ehhhhhhhhh https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_downloads/manager.js (right): https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/resource... chrome/browser/resources/md_downloads/manager.js:145: /** @return {downloads.Manager} */ On 2015/11/20 21:55:15, dpapad wrote: > @return {!downloads.Manager} technically this can return null if the component is loaded but not instantiated/attached (as I was doing in testing). anyways, programmatically enforcing via assert() now https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/resource... chrome/browser/resources/md_downloads/manager.js:147: return document.querySelector('downloads-manager'); On 2015/11/20 21:55:15, dpapad wrote: > Can the result of this call be cached, to avoid searching document every time > get() is called? this seems like a premature optimization that may cause head banging in tests https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/resource... chrome/browser/resources/md_downloads/manager.js:162: Manager.size = function() { On 2015/11/20 23:00:19, dpapad wrote: > I couldn't find any uses of Manager.size(). Does it need to be exposed? Done. https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc (right): https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:242: DCHECK(download_item->GetDangerType() == On 2015/11/20 23:00:19, dpapad wrote: > Is this DCHECK needed given that there is already a NOT_REACHED at > GetDangerTypeString()? Done. https://codereview.chromium.org/1428833005/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:381: void DownloadsListTracker::InsertItems(const SortedSet::iterator& start, On 2015/11/20 23:00:19, dpapad wrote: > Nit: Can we improve a bit the consistency of function naming? Specifically I > mean the following: > > - If a method simply calls to JS (without doing any other book-keeping), prefix > it with "Call" (CallUpdateItem, CallClearAl already do that). > - If a method does book-keeping besides calling to JS then no need for the > prefix. > > Following the above convention > InsertItems => CallInsertItems Done.
dbeam@chromium.org changed reviewers: + asanka@chromium.org
+asanka@ for his mad downloads knowledge see also: https://docs.google.com/document/d/1XkUDOc6085tir4D5yYEyjL2GsIGBslJBHXiNQDzJa...
Description was changed from ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controller and makes the JS simply respond to commands in a performant way. R=dpapad@chromium.org BUG=526577 ========== to ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controller and makes the JS simply respond to commands in a performant way. R=asanka@chromium.org,dpapad@chromium.org BUG=526577 ==========
ping asanka@
(and dpapad@ for that matter)
LGTM https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_downloads/action_service.js (right): https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_downloads/action_service.js:88: var terms = searchText ? searchText.split(/"([^"]*)"/).map(trim) : []; Is it worth adding tests for the regular expression? This could be a unit_test (no DOM), similar to https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res.... https://codereview.chromium.org/1428833005/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/md_downloads/layout_tests.js (right): https://codereview.chromium.org/1428833005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/md_downloads/layout_tests.js:18: manager.insertItems_(0, [{ Nit: Can you avoid referring to private method by using Manager.insertItems() instead? This way you also test the method that is actually called by the C++ side.
https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_downloads/action_service.js (right): https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_downloads/action_service.js:48: return (this.searchText_ || '').length > 0; !!this.searchText_ or do you folks dislike that expression? https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc (right): https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:34: using DownloadVector = std::vector<DownloadItem*>; Should this be: using DownloadVector = DownloadManager::DownloadVector ? https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:56: default: I prefer avoiding the `default` label and instead adding case labels for the NOT_DANGEROUS and MAYBE_DANGEROUS_CONTENT cases. Those cases can be handled the same way you are handling the default case below. This way if we introduce a new danger type, we'd be forced by the compiler to update this mapping. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:81: should_show_(base::Bind(&DownloadsListTracker::ShouldShow, Is this what git-cl format does? https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:176: base::Time start_time = download_item->GetStartTime(); If you're gonna store it in a variable, do so above the two other calls to GetStartTime(). Although I'd be okay with continuing to call GetStartTime(). https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:178: file_value->SetString("date_string", date_string); Nit: Better naming for 'started', 'since_string', and 'date_string'? https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:204: file_value->SetString("by_ext_name", extension->name()); This gets overwritten below. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:212: file_name = base::i18n::GetDisplayStringInLTRDirectionality(file_name); Is this how we handle filenames? What if the filename needs to be displayed RTL? https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:354: query.AddFilter(should_show_); Shall we break up ShouldShow into two? This is a pretty expensive predicate since each time it's invoked it runs DownloadQuery on a small array. It'll be much more efficient to just apply the search terms to this query rather than build and run a query for each download item. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:374: const SortedSet::iterator& end) { This method only seems to be called once and only at the start where start,end are begin()/end(). We don't actually want to 'insert', right? We want to set the entire list of visible items to be the list [start,end). I was initially a bit confused about why we need a method like this to insert a batch of downloads, and then I realized we just want to insert all of sorted_visible_items_. Can we simplify? https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_downloads/downloads_list_tracker.h (right): https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.h:65: content::DownloadItem* GetItemForTesting(size_t index); const method? https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.h:109: // When this is true, all changes to downloads that affect the page are send *sent? https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc (right): https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc:50: for (auto* list_item : *list) { Nit: No braces https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc:86: content::DownloadItem* item) const override { Take a const DownloadItem& or const DownloadItem* https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc:101: testing::Mock::VerifyAndClear(mock_item); Expectations are verified when the mock items are destroyed in the STLDeleteElements() call below. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc:124: return CreateMock(mock_items_.size(), mock_items_.size()); Nit: GetStartTime() is Time::FromDoubleT(static_cast<double>(mock_item_.size())) ? Why pass through double at all if the time value is integral? https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc:277: } Tests for incognito?
https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_downloads/action_service.js (right): https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_downloads/action_service.js:48: return (this.searchText_ || '').length > 0; On 2015/11/24 22:08:22, asanka wrote: > !!this.searchText_ > > or do you folks dislike that expression? Done. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_downloads/action_service.js:88: var terms = searchText ? searchText.split(/"([^"]*)"/).map(trim) : []; On 2015/11/24 18:42:33, dpapad wrote: > Is it worth adding tests for the regular expression? This could be a unit_test > (no DOM), similar to > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res.... Done. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc (right): https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:34: using DownloadVector = std::vector<DownloadItem*>; On 2015/11/24 22:08:22, asanka wrote: > Should this be: > > using DownloadVector = DownloadManager::DownloadVector > > ? Done. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:56: default: On 2015/11/24 22:08:22, asanka wrote: > I prefer avoiding the `default` label and instead adding case labels for the > NOT_DANGEROUS and MAYBE_DANGEROUS_CONTENT cases. Those cases can be handled the > same way you are handling the default case below. > > This way if we introduce a new danger type, we'd be forced by the compiler to > update this mapping. Done. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:81: should_show_(base::Bind(&DownloadsListTracker::ShouldShow, On 2015/11/24 22:08:22, asanka wrote: > Is this what git-cl format does? nope, it's what dan-fat-finger format does. Done. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:176: base::Time start_time = download_item->GetStartTime(); On 2015/11/24 22:08:22, asanka wrote: > If you're gonna store it in a variable, do so above the two other calls to > GetStartTime(). > > Although I'd be okay with continuing to call GetStartTime(). Done. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:178: file_value->SetString("date_string", date_string); On 2015/11/24 22:08:22, asanka wrote: > Nit: Better naming for 'started', 'since_string', and 'date_string'? Let's do that separately. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:204: file_value->SetString("by_ext_name", extension->name()); On 2015/11/24 22:08:22, asanka wrote: > This gets overwritten below. Done. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:212: file_name = base::i18n::GetDisplayStringInLTRDirectionality(file_name); On 2015/11/24 22:08:22, asanka wrote: > Is this how we handle filenames? What if the filename needs to be displayed RTL? ¯\_(ツ)_/¯ ask benjhayden@: https://chromiumcodereview.appspot.com/10805020/diff/21013/chrome/browser/ui/... fwiw: removing this line and running RTL (and pasting in RTL translations for the file name) all seem to have similar results. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:354: query.AddFilter(should_show_); On 2015/11/24 22:08:22, asanka wrote: > Shall we break up ShouldShow into two? This is a pretty expensive predicate > since each time it's invoked it runs DownloadQuery on a small array. It'll be > much more efficient to just apply the search terms to this query rather than > build and run a query for each download item. let me know if you find what I did acceptable. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:374: const SortedSet::iterator& end) { On 2015/11/24 22:08:22, asanka wrote: > This method only seems to be called once and only at the start where start,end > are begin()/end(). We don't actually want to 'insert', right? We want to set the > entire list of visible items to be the list [start,end). > > I was initially a bit confused about why we need a method like this to insert a > batch of downloads, and then I realized we just want to insert all of > sorted_visible_items_. Can we simplify? yes, but the follow-up patch will insert chunks. i'll simplify until then. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_downloads/downloads_list_tracker.h (right): https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.h:65: content::DownloadItem* GetItemForTesting(size_t index); On 2015/11/24 22:08:22, asanka wrote: > const method? Done. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.h:109: // When this is true, all changes to downloads that affect the page are send On 2015/11/24 22:08:22, asanka wrote: > *sent? Done. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc (right): https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc:50: for (auto* list_item : *list) { On 2015/11/24 22:08:22, asanka wrote: > Nit: No braces "In general, curly braces are not required for single-line statements, but they are allowed if you like them; conditional or loop statements with complex conditions or statements may be more readable with curly braces." https://google.github.io/styleguide/cppguide.html#Conditionals i've never fully understood why something in the "Conditionals" section of the C++ style guide would affect loops, so ambiguity abounds. will do it if it makes you happy, though... https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc:86: content::DownloadItem* item) const override { On 2015/11/24 22:08:22, asanka wrote: > Take a const DownloadItem& or const DownloadItem* this is overriding the method in DownloadsListTracker, which has multiple cases that currently require a non-const DownloadItem* (DownloadItemModel(), DownloadManager::GetDownload(), DownloadedByExtension::Get()). i could try to make const versions or update the downstream code to do this, if you really want, but I don't think it's worth it https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc:101: testing::Mock::VerifyAndClear(mock_item); On 2015/11/24 22:08:22, asanka wrote: > Expectations are verified when the mock items are destroyed in the > STLDeleteElements() call below. when a base::Time() is created via mock_item->GetStartTime(), gmock whines about the object being leaked in ~mock_item. this fixes that. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc:124: return CreateMock(mock_items_.size(), mock_items_.size()); On 2015/11/24 22:08:22, asanka wrote: > Nit: GetStartTime() is Time::FromDoubleT(static_cast<double>(mock_item_.size())) > ? > > Why pass through double at all if the time value is integral? Done. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc:277: } On 2015/11/24 22:08:22, asanka wrote: > Tests for incognito? will get to these soon https://codereview.chromium.org/1428833005/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/md_downloads/layout_tests.js (right): https://codereview.chromium.org/1428833005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/md_downloads/layout_tests.js:18: manager.insertItems_(0, [{ On 2015/11/24 18:42:33, dpapad wrote: > Nit: Can you avoid referring to private method by using Manager.insertItems() > instead? This way you also test the method that is actually called by the C++ > side. Done.
Patchset #3 (id:120001) has been deleted
https://codereview.chromium.org/1428833005/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_downloads/action_service.js (right): https://codereview.chromium.org/1428833005/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_downloads/action_service.js:32: function notEmpty(s) { return s.length > 0; } Nit: We are re-defining the trim() and notEpmty() functions every time splitTerms() is called. How about moving the outside of splitTerms (but still within the cr.definde('downloads', function() {...}). https://codereview.chromium.org/1428833005/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_downloads/action_service.js:69: return !!this.searchText_; If you follow the comment above, then you can re-use the notEmpty() here, as notEmpty(this.searchText_); https://codereview.chromium.org/1428833005/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_downloads/action_service_unittest.gtestjs (right): https://codereview.chromium.org/1428833005/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_downloads/action_service_unittest.gtestjs:3: // found in the LICENSE file. Nit (optional): I don't think that this file has to be named as *.gtestjs. You can just name it as *.js which makes most code editors to properly highlight the code as JS.
https://codereview.chromium.org/1428833005/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_downloads/action_service.js (right): https://codereview.chromium.org/1428833005/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_downloads/action_service.js:32: function notEmpty(s) { return s.length > 0; } On 2015/11/25 17:50:47, dpapad wrote: > Nit: We are re-defining the trim() and notEpmty() functions every time > splitTerms() is called. How about moving the outside of splitTerms (but still > within the cr.definde('downloads', function() {...}). Done. https://codereview.chromium.org/1428833005/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_downloads/action_service.js:69: return !!this.searchText_; On 2015/11/25 17:50:47, dpapad wrote: > If you follow the comment above, then you can re-use the notEmpty() here, as > notEmpty(this.searchText_); changed a little, but basically doing this https://codereview.chromium.org/1428833005/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_downloads/action_service_unittest.gtestjs (right): https://codereview.chromium.org/1428833005/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_downloads/action_service_unittest.gtestjs:3: // found in the LICENSE file. On 2015/11/25 17:50:47, dpapad wrote: > Nit (optional): I don't think that this file has to be named as *.gtestjs. You > can just name it as *.js which makes most code editors to properly highlight the > code as JS. Done. https://codereview.chromium.org/1428833005/diff/180001/chrome/browser/downloa... File chrome/browser/download/download_query.cc (right): https://codereview.chromium.org/1428833005/diff/180001/chrome/browser/downloa... chrome/browser/download/download_query.cc:215: return true; asanka@: note ^
lgtm https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc (right): https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:354: query.AddFilter(should_show_); On 2015/11/25 04:06:36, Dan Beam wrote: > On 2015/11/24 22:08:22, asanka wrote: > > Shall we break up ShouldShow into two? This is a pretty expensive predicate > > since each time it's invoked it runs DownloadQuery on a small array. It'll be > > much more efficient to just apply the search terms to this query rather than > > build and run a query for each download item. > > let me know if you find what I did acceptable. Yeah. I think this is good. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc:374: const SortedSet::iterator& end) { On 2015/11/25 04:06:36, Dan Beam wrote: > On 2015/11/24 22:08:22, asanka wrote: > > This method only seems to be called once and only at the start where start,end > > are begin()/end(). We don't actually want to 'insert', right? We want to set > the > > entire list of visible items to be the list [start,end). > > > > I was initially a bit confused about why we need a method like this to insert > a > > batch of downloads, and then I realized we just want to insert all of > > sorted_visible_items_. Can we simplify? > > yes, but the follow-up patch will insert chunks. i'll simplify until then. Acknowledged. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc (right): https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc:50: for (auto* list_item : *list) { On 2015/11/25 04:06:36, Dan Beam wrote: > On 2015/11/24 22:08:22, asanka wrote: > > Nit: No braces > > "In general, curly braces are not required for single-line statements, but they > are allowed if you like them; conditional or loop statements with complex > conditions or statements may be more readable with curly braces." > > https://google.github.io/styleguide/cppguide.html#Conditionals > > i've never fully understood why something in the "Conditionals" section of the > C++ style guide would affect loops, so ambiguity abounds. will do it if it > makes you happy, though... Hehe. https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc:124: return CreateMock(mock_items_.size(), mock_items_.size()); On 2015/11/25 04:06:36, Dan Beam wrote: > On 2015/11/24 22:08:22, asanka wrote: > > Nit: GetStartTime() is > Time::FromDoubleT(static_cast<double>(mock_item_.size())) > > ? > > > > Why pass through double at all if the time value is integral? > > Done. Better, but something like : CreateMock(mock_items_.size(), base::Time::UnixEpoch() + base::TimeDelta::FromHours(mock_items_.size())); where CreateMock(..) takes a base::Time would make it clearer what the code is trying to do. But I don't feel strongly about it. https://codereview.chromium.org/1428833005/diff/180001/chrome/browser/downloa... File chrome/browser/download/download_query.cc (right): https://codereview.chromium.org/1428833005/diff/180001/chrome/browser/downloa... chrome/browser/download/download_query.cc:215: return true; On 2015/11/25 at 20:23:23, Dan Beam wrote: > asanka@: note ^ Noted.
https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc (right): https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc:124: return CreateMock(mock_items_.size(), mock_items_.size()); On 2015/11/25 20:49:06, asanka wrote: > On 2015/11/25 04:06:36, Dan Beam wrote: > > On 2015/11/24 22:08:22, asanka wrote: > > > Nit: GetStartTime() is > > Time::FromDoubleT(static_cast<double>(mock_item_.size())) > > > ? > > > > > > Why pass through double at all if the time value is integral? > > > > Done. > > Better, but something like : > > CreateMock(mock_items_.size(), base::Time::UnixEpoch() + > base::TimeDelta::FromHours(mock_items_.size())); > > where CreateMock(..) takes a base::Time would make it clearer what the code is > trying to do. But I don't feel strongly about it. Done.
https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc (right): https://codereview.chromium.org/1428833005/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc:277: } On 2015/11/25 04:06:36, Dan Beam wrote: > On 2015/11/24 22:08:22, asanka wrote: > > Tests for incognito? > > will get to these soon ... in a follow-up CL. there's a pretty decent chance this CL itself gets reverted, so I want to try now.
The CQ bit was checked by dbeam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org, asanka@chromium.org Link to the patchset: https://codereview.chromium.org/1428833005/#ps180002 (title: "asanka@ review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1428833005/180002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1428833005/180002
Message was sent while issue was closed.
Description was changed from ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controller and makes the JS simply respond to commands in a performant way. R=asanka@chromium.org,dpapad@chromium.org BUG=526577 ========== to ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controller and makes the JS simply respond to commands in a performant way. R=asanka@chromium.org,dpapad@chromium.org BUG=526577 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180002)
Message was sent while issue was closed.
Description was changed from ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controller and makes the JS simply respond to commands in a performant way. R=asanka@chromium.org,dpapad@chromium.org BUG=526577 ========== to ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controller and makes the JS simply respond to commands in a performant way. R=asanka@chromium.org,dpapad@chromium.org BUG=526577 Committed: https://crrev.com/099dfdd09d2dd8e2d8228c955a89069fc971d625 Cr-Commit-Position: refs/heads/master@{#362063} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/099dfdd09d2dd8e2d8228c955a89069fc971d625 Cr-Commit-Position: refs/heads/master@{#362063}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:180002) has been created in https://codereview.chromium.org/1480263002/ by vabr@chromium.org. The reason for reverting is: This broke Android GN. More info on the bug. BUG=563357.
Message was sent while issue was closed.
Description was changed from ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controller and makes the JS simply respond to commands in a performant way. R=asanka@chromium.org,dpapad@chromium.org BUG=526577 Committed: https://crrev.com/099dfdd09d2dd8e2d8228c955a89069fc971d625 Cr-Commit-Position: refs/heads/master@{#362063} ========== to ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controller and makes the JS simply respond to commands in a performant way. R=asanka@chromium.org,dpapad@chromium.org BUG=526577 Committed: https://crrev.com/099dfdd09d2dd8e2d8228c955a89069fc971d625 Cr-Commit-Position: refs/heads/master@{#362063} ==========
Description was changed from ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controller and makes the JS simply respond to commands in a performant way. R=asanka@chromium.org,dpapad@chromium.org BUG=526577 Committed: https://crrev.com/099dfdd09d2dd8e2d8228c955a89069fc971d625 Cr-Commit-Position: refs/heads/master@{#362063} ========== to ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controller and makes the JS simply respond to commands in a performant way. R=asanka@chromium.org,dpapad@chromium.org BUG=526577,563357 Committed: https://crrev.com/099dfdd09d2dd8e2d8228c955a89069fc971d625 Cr-Commit-Position: refs/heads/master@{#362063} ==========
dbeam@chromium.org changed reviewers: + sky@chromium.org
+sky@ for: chrome/chrome_browser_ui.gypi chrome/chrome_tests_unit.gypi chrome/test/BUILD.gn
LGTM
The CQ bit was checked by dbeam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org, dpapad@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1428833005/#ps250001 (title: "gyp shufflin")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1428833005/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1428833005/250001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controller and makes the JS simply respond to commands in a performant way. R=asanka@chromium.org,dpapad@chromium.org BUG=526577,563357 Committed: https://crrev.com/099dfdd09d2dd8e2d8228c955a89069fc971d625 Cr-Commit-Position: refs/heads/master@{#362063} ========== to ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controller and makes the JS simply respond to commands in a performant way. R=asanka@chromium.org,dpapad@chromium.org BUG=526577 Committed: https://crrev.com/099dfdd09d2dd8e2d8228c955a89069fc971d625 Cr-Commit-Position: refs/heads/master@{#362063} ==========
split the js unit tests to here: https://codereview.chromium.org/1491773002
Patchset #12 (id:310001) has been deleted
The CQ bit was checked by dbeam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org, sky@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/1428833005/#ps330001 (title: "merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1428833005/330001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1428833005/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1428833005/330001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1428833005/330001
Message was sent while issue was closed.
Description was changed from ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controller and makes the JS simply respond to commands in a performant way. R=asanka@chromium.org,dpapad@chromium.org BUG=526577 Committed: https://crrev.com/099dfdd09d2dd8e2d8228c955a89069fc971d625 Cr-Commit-Position: refs/heads/master@{#362063} ========== to ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controller and makes the JS simply respond to commands in a performant way. R=asanka@chromium.org,dpapad@chromium.org BUG=526577 Committed: https://crrev.com/099dfdd09d2dd8e2d8228c955a89069fc971d625 Cr-Commit-Position: refs/heads/master@{#362063} ==========
Message was sent while issue was closed.
Committed patchset #12 (id:330001)
Message was sent while issue was closed.
Description was changed from ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controller and makes the JS simply respond to commands in a performant way. R=asanka@chromium.org,dpapad@chromium.org BUG=526577 Committed: https://crrev.com/099dfdd09d2dd8e2d8228c955a89069fc971d625 Cr-Commit-Position: refs/heads/master@{#362063} ========== to ========== MD Downloads: track downloads in C++, dispatch discrete JS updates Instead of both the C++ and the JS needing to create sparse and dense representations of download items (and reconcile the differences in some cases), just keep a sorted representation in C++ and send discrete, positioned updates to the JS. This makes the C++ act more like a controller and makes the JS simply respond to commands in a performant way. R=asanka@chromium.org,dpapad@chromium.org BUG=526577 Committed: https://crrev.com/099dfdd09d2dd8e2d8228c955a89069fc971d625 Cr-Commit-Position: refs/heads/master@{#362063} Committed: https://crrev.com/85f052423a3e2476f1afe4fdc11dfd42ef4439dd Cr-Commit-Position: refs/heads/master@{#362790} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/85f052423a3e2476f1afe4fdc11dfd42ef4439dd Cr-Commit-Position: refs/heads/master@{#362790}
Message was sent while issue was closed.
https://codereview.chromium.org/1428833005/diff/330001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler.cc (left): https://codereview.chromium.org/1428833005/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler.cc:637: GetMainNotifierManager()->CheckForHistoryFilesRemoval(); ^ well, I dropped this on the floor |
