Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8601012:
DownloadQuery filters and sorts DownloadItems. (Closed)
|
Created:
9 years, 1 month ago by benjhayden Modified:
9 years ago CC:
chromium-reviews, jam, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, rdsmith+dwatch_chromium.org, Paweł Hajdan Jr. Visibility:
Public. |
DescriptionDownloadQuery filters and sorts DownloadItems
DownloadQuery will be used first by the DownloadsSearchFunction in download_extension_api.h/cc to implement the chrome.experimental.downloads.search() function outlined in http://goo.gl/6hO1n
BUG=12133
TEST=content_unittests DownloadQueryTest
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114460
Patch Set 1 : DownloadQuery #
Total comments: 26
Patch Set 2 : Callbacks #
Total comments: 20
Patch Set 3 : comments #
Total comments: 9
Patch Set 4 : tests #
Total comments: 55
Patch Set 5 : " #
Total comments: 4
Patch Set 6 : const #
Total comments: 6
Patch Set 7 : comments #
Total comments: 2
Patch Set 8 : kill dbhandle #
Total comments: 8
Patch Set 9 : ... #Patch Set 10 : include #
Messages
Total messages: 38 (0 generated)
PTAL. Please overlook the test's incompleteness for now. I'll write some more tests while awaiting your review. I'm sure you'll have plenty of comments on download_query.h/cc. I'll send out a PTAL when I think that the test is complete enough so that you can tell me what I missed.
On 2011/11/22 19:54:06, b s h wrote: > PTAL. > Please overlook the test's incompleteness for now. I'll write some more tests > while awaiting your review. I'm sure you'll have plenty of comments on > download_query.h/cc. I'll send out a PTAL when I think that the test is complete > enough so that you can tell me what I missed. Just for context, what's the relationship between this CL and the earlier one that was about sorting/searching?
I split http://codereview.chromium.org/7825035/ into the DownloadItemABC/Impl/Mock cl, this one, and one or two others, so I will not be committing 7825035. There'll be another cl that implements DownloadsSearchFunction (downloads.search()), and maybe another cl before or after that one, depending on how that cl and this one go.
Non-test comments. This looks pretty good (though guideline comments as to the various bits of magic you're doing would be a good thing :-}). http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... File content/browser/download/download_query.cc (right): http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.cc:96: return item.GetFullPath().LossyDisplayName(); A comment here as to why we're using the LossyDisplayName for something that's targeted at programs rather than users would be welcome; it's confusing to read over it. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.cc:99: template <typename ValueType_> nit: I think it would be useful to have comments indicating when something's an ABC and something is actually intiantiable. I'd go further and say a comment at the top of this file sketching out your class hierarchy would go a long way to making life easier for future readers. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.cc:102: typedef ValueType_ ValueType; Just for my education, what's the value in type trailing underscore + typedef? http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.cc:180: #define LAMBDA(__name, __base, __method, __expr, __ctor_arg_type, __ctor_arg) ({ \ nit: This line looks long--I suspect > 80 columns? http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.cc:180: #define LAMBDA(__name, __base, __method, __expr, __ctor_arg_type, __ctor_arg) ({ \ nit/suggestion (up to you): I personally prefer lining up the "\" in macro definitions as I find it makes the macros easier to read. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.cc:180: #define LAMBDA(__name, __base, __method, __expr, __ctor_arg_type, __ctor_arg) ({ \ A comment describing the context for this macro (why/how you're going to use it) would be useful; that might be appropriate to include at the top of the file if it's a high level enough concept. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.cc:330: iter->name, iter->direction); Any reason not to do this as the Sort calls are being made? It doesn't matter performance wise, but you could get rid of the OrderTermVector doing it that way (at cost of having order_terms be a member variable instead of a local one) and I think of the prep for the search as naturally happening at those points. (I'm ok with it either way--I don't think it matters for performance, and I have no strong objection to doing it this way. It just feels a little odd.) http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.h:16: namespace download_util { Given that we've got a pretty descriptive top level class name that everything's under, I'm not sure there's any need to put this in download_util. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.h:25: // rules are applied: id ascending then db_handle ascending. nit: I find this comment sentence a little confusing, as if you do (presumed stable :-}) sorts in order, it's the later ones that end up being most primary, whereas I presume you mean here that id ascending and db_handle ascending are least primary. Is there a way to rephrase the comment to eliminate the confusion? http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.h:98: // |name|. (DRY.) nit: What doe the "DRY" add to the comment? (I'm a bit biased as I had forgotten what it mean and had to look it up, but having done that the only reason I can see is as a justification for why you don't include the type of each |name|, and I don't think it's worthwhile putting such justifications in comments. Is there another reason to have it?) http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.h:104: // the first sort field, secondarily by the second sort field, and so on. For nit: I'd say "field in first call to sort"; "first sort field" requires the reader to interpolate an ordering, and while the ordering of Sort() calls is the obvious one to interpolate, it's better not to have to guess. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.h:124: bool Matches(const DownloadItem& item) const; Why is this a public member function?
As I mentioned IRL, I realized that I had partially re-implemented base::Bind/Callback, so I rewrote DownloadQuery to just use those. * Zero macros! * Only one template class, InnerCallback, which can be deleted when the bug in base::Bind/Callback is fixed * Only two template functions * No inheritance forest * An opportunity to brush up on functional programming http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... File content/browser/download/download_query.cc (right): http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.cc:96: return item.GetFullPath().LossyDisplayName(); On 2011/11/23 21:07:34, rdsmith wrote: > A comment here as to why we're using the LossyDisplayName for something that's > targeted at programs rather than users would be welcome; it's confusing to read > over it. Done. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.cc:99: template <typename ValueType_> On 2011/11/23 21:07:34, rdsmith wrote: > nit: I think it would be useful to have comments indicating when something's an > ABC and something is actually intiantiable. I'd go further and say a comment at > the top of this file sketching out your class hierarchy would go a long way to > making life easier for future readers. Done. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.cc:102: typedef ValueType_ ValueType; On 2011/11/23 21:07:34, rdsmith wrote: > Just for my education, what's the value in type trailing underscore + typedef? Short: Done. Long: If any of FilterField's methods were not in-line, then this typedef would be the only way for the out-of-line methods and clients to access the type. You'll also find this pattern in the STL and some other templated classes. This was the case in a former version of this class, but I'll kill it now and resurrect it later if necessary. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.cc:180: #define LAMBDA(__name, __base, __method, __expr, __ctor_arg_type, __ctor_arg) ({ \ On 2011/11/23 21:07:34, rdsmith wrote: > nit: This line looks long--I suspect > 80 columns? Done. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.cc:180: #define LAMBDA(__name, __base, __method, __expr, __ctor_arg_type, __ctor_arg) ({ \ On 2011/11/23 21:07:34, rdsmith wrote: > A comment describing the context for this macro (why/how you're going to use it) > would be useful; that might be appropriate to include at the top of the file if > it's a high level enough concept. Done. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.cc:180: #define LAMBDA(__name, __base, __method, __expr, __ctor_arg_type, __ctor_arg) ({ \ On 2011/11/23 21:07:34, rdsmith wrote: > nit/suggestion (up to you): I personally prefer lining up the "\" in macro > definitions as I find it makes the macros easier to read. Done. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.cc:330: iter->name, iter->direction); On 2011/11/23 21:07:34, rdsmith wrote: > Any reason not to do this as the Sort calls are being made? It doesn't matter > performance wise, but you could get rid of the OrderTermVector doing it that way > (at cost of having order_terms be a member variable instead of a local one) and > I think of the prep for the search as naturally happening at those points. > > (I'm ok with it either way--I don't think it matters for performance, and I have > no strong objection to doing it this way. It just feels a little odd.) I meant to do that and forgot. It required moving DownloadComparator into DownloadQuery, but Done. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.h:16: namespace download_util { On 2011/11/23 21:07:34, rdsmith wrote: > Given that we've got a pretty descriptive top level class name that everything's > under, I'm not sure there's any need to put this in download_util. Done. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.h:25: // rules are applied: id ascending then db_handle ascending. On 2011/11/23 21:07:34, rdsmith wrote: > nit: I find this comment sentence a little confusing, as if you do (presumed > stable :-}) sorts in order, it's the later ones that end up being most primary, > whereas I presume you mean here that id ascending and db_handle ascending are > least primary. Is there a way to rephrase the comment to eliminate the > confusion? Done. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.h:98: // |name|. (DRY.) On 2011/11/23 21:07:34, rdsmith wrote: > nit: What doe the "DRY" add to the comment? (I'm a bit biased as I had > forgotten what it mean and had to look it up, but having done that the only > reason I can see is as a justification for why you don't include the type of > each |name|, and I don't think it's worthwhile putting such justifications in > comments. Is there another reason to have it?) Done. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.h:104: // the first sort field, secondarily by the second sort field, and so on. For On 2011/11/23 21:07:34, rdsmith wrote: > nit: I'd say "field in first call to sort"; "first sort field" requires the > reader to interpolate an ordering, and while the ordering of Sort() calls is the > obvious one to interpolate, it's better not to have to guess. Done. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.h:124: bool Matches(const DownloadItem& item) const; On 2011/11/23 21:07:34, rdsmith wrote: > Why is this a public member function? If you pass an id to search(), then DownloadSearchFunction will lookup the item by id instead of using DownloadQuery::Search(). In order to see if that item matches the other filter fields, DSF could either construct a vector and add the one item to it, or it could call Matches(). So, basically, Matches() is a shortcut for constructing a vector and adding one item to it and calling Search(). It might also facilitate the test.
Wow. This is nice. I presume you already have C++ readability, but if you don't, this could probably get it for you. I'd like to see some performance tests so that we get an estimate of what our per-filter cost is, and what our per-item cost is. I'm debating how much I'd like to see in the way of unit tests. I'd suggest that you think of it in terms of code coverage; would you current unit tests cover the large majority of lines in download_query.cc? I'm afraid they'd miss a lot of the small functions (though it looks like they test the core logic well). http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... File content/browser/download/download_query.cc (right): http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.cc:29: static bool MatchesQuery(const string16& value, const DownloadItem& item) { nit, suggestion: Comment before this set of functions indicate what the general usage for them is (e.g. "helper functions for accessing non-simple attributes of DownloadItem"?) http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.cc:84: // callback is nullified when the outer callback is Run. nit: If there's an issue number, it should be in the comment. http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.cc:102: // Returns true if |item| matches the filter specified by |value|, |signum|, and nit: I think this would be more easily comprehensible if ComparisonType was defined right before this function; any reason not to? I also find |signum| non-pneumonic. Maybe |comptype| or |comparison_type|? http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.cc:137: icu::RegexMatcher match(pattern, input, 0, status); This looks like we're going to recompile the pattern on every DownloadItem. Is there a way to compile the pattern only once? This isn't top priority, just a useful pre-optimization if we can do it. http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.cc:419: case SORT_FIELD_ERROR: break; // TODO Why are these no-ops? I think of it as friendlier to return an error for something that we haven't implemented than to pretend that it works. http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.h:39: typedef base::Callback<bool(const DownloadItem&)> FilterType; A comment here or on the Filter() interface that uses FilterType giving a one line description of how to use this? Currently all you've got is the example, and it's easy to miss the part of the example that maps to FilterType. http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.h:91: // malformed. Otherwise, makes a new filter named |name|, sets its value to nit, suggestion: "makes" -> "adds"? http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.h:91: // malformed. Otherwise, makes a new filter named |name|, sets its value to nit: "filter named |name|" implies that the name is arbitrary, which it's not. Maybe "makes a new filter of type |name|"? (Note that the current wording implies that you can't have multiple filters of the same name, which I don't think is true.) http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.h:93: // do not match all filters. See the implementation for the type of each nit: I dislike pointing people at the implementation for details of the interface, especially when the implementation is complicated and potentially confusing. Any reason not to put the types in as comments in the enum above? http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.h:103: // Makes a new sort field named |name| and sets its direction to |direction|. nit, suggestion: "Makes" -> "Adds"?
I have C++ readability in google's internal reckoning. Do you know if that translates to chrome? If not, I'll investigate tomorrow. I'll see about the performance tests tomorrow. I think I will shoot for 100% coverage. Can you show me how to calculate coverage? http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... File content/browser/download/download_query.cc (right): http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.cc:29: static bool MatchesQuery(const string16& value, const DownloadItem& item) { On 2011/11/30 19:26:17, rdsmith wrote: > nit, suggestion: Comment before this set of functions indicate what the general > usage for them is (e.g. "helper functions for accessing non-simple attributes of > DownloadItem"?) Done. http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.cc:84: // callback is nullified when the outer callback is Run. On 2011/11/30 19:26:17, rdsmith wrote: > nit: If there's an issue number, it should be in the comment. There isn't a bug, it's just one CL: http://codereview.chromium.org/8738001/ http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.cc:102: // Returns true if |item| matches the filter specified by |value|, |signum|, and On 2011/11/30 19:26:17, rdsmith wrote: > nit: I think this would be more easily comprehensible if ComparisonType was > defined right before this function; any reason not to? Done. > > I also find |signum| non-pneumonic. Maybe |comptype| or |comparison_type|? "cmptype" ok? ComparisonType was originally called Signum because it replaced the 0/1/-1 in Compare<>(). I missed this parameter when I renamed Signum. http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.cc:137: icu::RegexMatcher match(pattern, input, 0, status); On 2011/11/30 19:26:17, rdsmith wrote: > This looks like we're going to recompile the pattern on every DownloadItem. Is > there a way to compile the pattern only once? This isn't top priority, just a > useful pre-optimization if we can do it. Done. http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.cc:419: case SORT_FIELD_ERROR: break; // TODO On 2011/11/30 19:26:17, rdsmith wrote: > Why are these no-ops? I think of it as friendlier to return an error for > something that we haven't implemented than to pretend that it works. Sort() doesn't return anything, so I'll make it NOTIMPLEMENTED(). http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.h:39: typedef base::Callback<bool(const DownloadItem&)> FilterType; On 2011/11/30 19:26:17, rdsmith wrote: > A comment here or on the Filter() interface that uses FilterType giving a one > line description of how to use this? Currently all you've got is the example, > and it's easy to miss the part of the example that maps to FilterType. Done. http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.h:91: // malformed. Otherwise, makes a new filter named |name|, sets its value to On 2011/11/30 19:26:17, rdsmith wrote: > nit, suggestion: "makes" -> "adds"? Done. http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.h:91: // malformed. Otherwise, makes a new filter named |name|, sets its value to On 2011/11/30 19:26:17, rdsmith wrote: > nit: "filter named |name|" implies that the name is arbitrary, which it's not. > Maybe "makes a new filter of type |name|"? (Note that the current wording > implies that you can't have multiple filters of the same name, which I don't > think is true.) Done. http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.h:93: // do not match all filters. See the implementation for the type of each On 2011/11/30 19:26:17, rdsmith wrote: > nit: I dislike pointing people at the implementation for details of the > interface, especially when the implementation is complicated and potentially > confusing. Any reason not to put the types in as comments in the enum above? Done. http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... content/browser/download/download_query.h:103: // Makes a new sort field named |name| and sets its direction to |direction|. On 2011/11/30 19:26:17, rdsmith wrote: > nit, suggestion: "Makes" -> "Adds"? Done.
On 2011/11/30 22:13:49, b s h wrote: > I have C++ readability in google's internal reckoning. Do you know if that > translates to chrome? If not, I'll investigate tomorrow. C++ readability is only relevant in the Google context. It's just something you can get (if you jump through the right hoops) on chrome. I haven't managed it yet, so I'm on the lookout for classes that would do the job. So you don't need to worry about it. > I'll see about the performance tests tomorrow. > > I think I will shoot for 100% coverage. Can you show me how to calculate > coverage? Sure. It's a bit complex, but getting simpler all the time :-}. The quick summary is: * Make sure you've sync'd past r111801 * Patch in http://codereview.chromium.org/8598003. * Follow the directions at http://go/chrome-tc ("Running Coverage Builds locally") except use the target coverage_build, not coverage. * Execute (from 'src') the command: python tools/code_coverage/coverage_posix.py --directory out/Debug --src_root . 'content_tests[DownloadQueryTest.*]' * Execute (from 'src') the command: tools/code_coverage/croc.py -c build/common.croc -c build/linux/chrome_linux.croc -i out/Debug/coverage.info -r .. --html out/Debug/coverage_download_unittests_html That should produce some interesting html to lookout in the named directory; if you drill down you should be able to get line-by-line on your unit test. I'm sure the above will break somehow, but that's good, you can be my alpha-tester :-}. > > http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... > File content/browser/download/download_query.cc (right): > > http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... > content/browser/download/download_query.cc:29: static bool MatchesQuery(const > string16& value, const DownloadItem& item) { > On 2011/11/30 19:26:17, rdsmith wrote: > > nit, suggestion: Comment before this set of functions indicate what the > general > > usage for them is (e.g. "helper functions for accessing non-simple attributes > of > > DownloadItem"?) > > Done. > > http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... > content/browser/download/download_query.cc:84: // callback is nullified when the > outer callback is Run. > On 2011/11/30 19:26:17, rdsmith wrote: > > nit: If there's an issue number, it should be in the comment. > > There isn't a bug, it's just one CL: http://codereview.chromium.org/8738001/ > > http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... > content/browser/download/download_query.cc:102: // Returns true if |item| > matches the filter specified by |value|, |signum|, and > On 2011/11/30 19:26:17, rdsmith wrote: > > nit: I think this would be more easily comprehensible if ComparisonType was > > defined right before this function; any reason not to? > > Done. > > > > > I also find |signum| non-pneumonic. Maybe |comptype| or |comparison_type|? > > "cmptype" ok? > > ComparisonType was originally called Signum because it replaced the 0/1/-1 in > Compare<>(). I missed this parameter when I renamed Signum. > > http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... > content/browser/download/download_query.cc:137: icu::RegexMatcher match(pattern, > input, 0, status); > On 2011/11/30 19:26:17, rdsmith wrote: > > This looks like we're going to recompile the pattern on every DownloadItem. > Is > > there a way to compile the pattern only once? This isn't top priority, just a > > useful pre-optimization if we can do it. > > Done. > > http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... > content/browser/download/download_query.cc:419: case SORT_FIELD_ERROR: break; // > TODO > On 2011/11/30 19:26:17, rdsmith wrote: > > Why are these no-ops? I think of it as friendlier to return an error for > > something that we haven't implemented than to pretend that it works. > > Sort() doesn't return anything, so I'll make it NOTIMPLEMENTED(). > > http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... > File content/browser/download/download_query.h (right): > > http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... > content/browser/download/download_query.h:39: typedef base::Callback<bool(const > DownloadItem&)> FilterType; > On 2011/11/30 19:26:17, rdsmith wrote: > > A comment here or on the Filter() interface that uses FilterType giving a one > > line description of how to use this? Currently all you've got is the example, > > and it's easy to miss the part of the example that maps to FilterType. > > Done. > > http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... > content/browser/download/download_query.h:91: // malformed. Otherwise, makes a > new filter named |name|, sets its value to > On 2011/11/30 19:26:17, rdsmith wrote: > > nit, suggestion: "makes" -> "adds"? > > Done. > > http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... > content/browser/download/download_query.h:91: // malformed. Otherwise, makes a > new filter named |name|, sets its value to > On 2011/11/30 19:26:17, rdsmith wrote: > > nit: "filter named |name|" implies that the name is arbitrary, which it's not. > > > Maybe "makes a new filter of type |name|"? (Note that the current wording > > implies that you can't have multiple filters of the same name, which I don't > > think is true.) > > Done. > > http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... > content/browser/download/download_query.h:93: // do not match all filters. See > the implementation for the type of each > On 2011/11/30 19:26:17, rdsmith wrote: > > nit: I dislike pointing people at the implementation for details of the > > interface, especially when the implementation is complicated and potentially > > confusing. Any reason not to put the types in as comments in the enum above? > > Done. > > http://codereview.chromium.org/8601012/diff/25001/content/browser/download/do... > content/browser/download/download_query.h:103: // Makes a new sort field named > |name| and sets its direction to |direction|. > On 2011/11/30 19:26:17, rdsmith wrote: > > nit, suggestion: "Makes" -> "Adds"? > > Done.
I won't be able to get to this review until Friday, but feel free to land it without me looking at it. On Wed, Nov 30, 2011 at 5:27 PM, <rdsmith@chromium.org> wrote: > On 2011/11/30 22:13:49, b s h wrote: > >> I have C++ readability in google's internal reckoning. Do you know if that >> translates to chrome? If not, I'll investigate tomorrow. >> > > C++ readability is only relevant in the Google context. It's just > something you > can get (if you jump through the right hoops) on chrome. I haven't > managed it > yet, so I'm on the lookout for classes that would do the job. So you > don't need > to worry about it. > > > I'll see about the performance tests tomorrow. >> > > I think I will shoot for 100% coverage. Can you show me how to calculate >> coverage? >> > > Sure. It's a bit complex, but getting simpler all the time :-}. The quick > summary is: > * Make sure you've sync'd past r111801 > * Patch in http://codereview.chromium.**org/8598003<http://codereview.chromium.org/8598003> > . > * Follow the directions at http://go/chrome-tc ("Running Coverage Builds > locally") except use the target coverage_build, not coverage. > * Execute (from 'src') the command: > > python tools/code_coverage/coverage_**posix.py --directory > out/Debug > --src_root . 'content_tests[**DownloadQueryTest.*]' > > * Execute (from 'src') the command: > > tools/code_coverage/croc.py -c build/common.croc -c > build/linux/chrome_linux.croc -i out/Debug/coverage.info -r .. --html > out/Debug/coverage_download_**unittests_html > > That should produce some interesting html to lookout in the named > directory; if > you drill down you should be able to get line-by-line on your unit test. > > I'm sure the above will break somehow, but that's good, you can be my > alpha-tester :-}. > > > > > http://codereview.chromium.**org/8601012/diff/25001/** > content/browser/download/**download_query.cc<http://codereview.chromium.org/8601012/diff/25001/content/browser/download/download_query.cc> > >> File content/browser/download/**download_query.cc (right): >> > > > http://codereview.chromium.**org/8601012/diff/25001/** > content/browser/download/**download_query.cc#newcode29<http://codereview.chromium.org/8601012/diff/25001/content/browser/download/download_query.cc#newcode29> > >> content/browser/download/**download_query.cc:29: static bool >> MatchesQuery(const >> string16& value, const DownloadItem& item) { >> On 2011/11/30 19:26:17, rdsmith wrote: >> > nit, suggestion: Comment before this set of functions indicate what the >> general >> > usage for them is (e.g. "helper functions for accessing non-simple >> > attributes > >> of >> > DownloadItem"?) >> > > Done. >> > > > http://codereview.chromium.**org/8601012/diff/25001/** > content/browser/download/**download_query.cc#newcode84<http://codereview.chromium.org/8601012/diff/25001/content/browser/download/download_query.cc#newcode84> > >> content/browser/download/**download_query.cc:84: // callback is >> nullified when >> > the > >> outer callback is Run. >> On 2011/11/30 19:26:17, rdsmith wrote: >> > nit: If there's an issue number, it should be in the comment. >> > > There isn't a bug, it's just one CL: http://codereview.chromium.** >> org/8738001/ <http://codereview.chromium.org/8738001/> >> > > > http://codereview.chromium.**org/8601012/diff/25001/** > content/browser/download/**download_query.cc#newcode102<http://codereview.chromium.org/8601012/diff/25001/content/browser/download/download_query.cc#newcode102> > >> content/browser/download/**download_query.cc:102: // Returns true if >> |item| >> matches the filter specified by |value|, |signum|, and >> On 2011/11/30 19:26:17, rdsmith wrote: >> > nit: I think this would be more easily comprehensible if ComparisonType >> was >> > defined right before this function; any reason not to? >> > > Done. >> > > > >> > I also find |signum| non-pneumonic. Maybe |comptype| or >> |comparison_type|? >> > > "cmptype" ok? >> > > ComparisonType was originally called Signum because it replaced the >> 0/1/-1 in >> Compare<>(). I missed this parameter when I renamed Signum. >> > > > http://codereview.chromium.**org/8601012/diff/25001/** > content/browser/download/**download_query.cc#newcode137<http://codereview.chromium.org/8601012/diff/25001/content/browser/download/download_query.cc#newcode137> > >> content/browser/download/**download_query.cc:137: icu::RegexMatcher >> > match(pattern, > >> input, 0, status); >> On 2011/11/30 19:26:17, rdsmith wrote: >> > This looks like we're going to recompile the pattern on every >> DownloadItem. >> Is >> > there a way to compile the pattern only once? This isn't top priority, >> just >> > a > >> > useful pre-optimization if we can do it. >> > > Done. >> > > > http://codereview.chromium.**org/8601012/diff/25001/** > content/browser/download/**download_query.cc#newcode419<http://codereview.chromium.org/8601012/diff/25001/content/browser/download/download_query.cc#newcode419> > >> content/browser/download/**download_query.cc:419: case SORT_FIELD_ERROR: >> break; >> > // > >> TODO >> On 2011/11/30 19:26:17, rdsmith wrote: >> > Why are these no-ops? I think of it as friendlier to return an error >> for >> > something that we haven't implemented than to pretend that it works. >> > > Sort() doesn't return anything, so I'll make it NOTIMPLEMENTED(). >> > > > http://codereview.chromium.**org/8601012/diff/25001/** > content/browser/download/**download_query.h<http://codereview.chromium.org/8601012/diff/25001/content/browser/download/download_query.h> > >> File content/browser/download/**download_query.h (right): >> > > > http://codereview.chromium.**org/8601012/diff/25001/** > content/browser/download/**download_query.h#newcode39<http://codereview.chromium.org/8601012/diff/25001/content/browser/download/download_query.h#newcode39> > >> content/browser/download/**download_query.h:39: typedef >> > base::Callback<bool(const > >> DownloadItem&)> FilterType; >> On 2011/11/30 19:26:17, rdsmith wrote: >> > A comment here or on the Filter() interface that uses FilterType giving >> a >> > one > >> > line description of how to use this? Currently all you've got is the >> > example, > >> > and it's easy to miss the part of the example that maps to FilterType. >> > > Done. >> > > > http://codereview.chromium.**org/8601012/diff/25001/** > content/browser/download/**download_query.h#newcode91<http://codereview.chromium.org/8601012/diff/25001/content/browser/download/download_query.h#newcode91> > >> content/browser/download/**download_query.h:91: // malformed. Otherwise, >> makes a >> new filter named |name|, sets its value to >> On 2011/11/30 19:26:17, rdsmith wrote: >> > nit, suggestion: "makes" -> "adds"? >> > > Done. >> > > > http://codereview.chromium.**org/8601012/diff/25001/** > content/browser/download/**download_query.h#newcode91<http://codereview.chromium.org/8601012/diff/25001/content/browser/download/download_query.h#newcode91> > >> content/browser/download/**download_query.h:91: // malformed. Otherwise, >> makes a >> new filter named |name|, sets its value to >> On 2011/11/30 19:26:17, rdsmith wrote: >> > nit: "filter named |name|" implies that the name is arbitrary, which >> it's >> > not. > > > Maybe "makes a new filter of type |name|"? (Note that the current >> wording >> > implies that you can't have multiple filters of the same name, which I >> don't >> > think is true.) >> > > Done. >> > > > http://codereview.chromium.**org/8601012/diff/25001/** > content/browser/download/**download_query.h#newcode93<http://codereview.chromium.org/8601012/diff/25001/content/browser/download/download_query.h#newcode93> > >> content/browser/download/**download_query.h:93: // do not match all >> filters. >> > See > >> the implementation for the type of each >> On 2011/11/30 19:26:17, rdsmith wrote: >> > nit: I dislike pointing people at the implementation for details of the >> > interface, especially when the implementation is complicated and >> potentially >> > confusing. Any reason not to put the types in as comments in the enum >> > above? > > Done. >> > > > http://codereview.chromium.**org/8601012/diff/25001/** > content/browser/download/**download_query.h#newcode103<http://codereview.chromium.org/8601012/diff/25001/content/browser/download/download_query.h#newcode103> > >> content/browser/download/**download_query.h:103: // Makes a new sort >> field named >> |name| and sets its direction to |direction|. >> On 2011/11/30 19:26:17, rdsmith wrote: >> > nit, suggestion: "Makes" -> "Adds"? >> > > Done. >> > > > http://codereview.chromium.**org/8601012/<http://codereview.chromium.org/8601... >
Will do another round on tests when they're ready. But beyond that, looks good. http://codereview.chromium.org/8601012/diff/30001/content/browser/download/do... File content/browser/download/download_query.cc (right): http://codereview.chromium.org/8601012/diff/30001/content/browser/download/do... content/browser/download/download_query.cc:54: } How do these two routines relate to what actually shows up on the file system? My memory is that we decided that the extension system couldn't do arbitrary filename setting, but could set file names relative to the download directory. Is there a way (for filtering, sorting, getting) for them to get those relative file names? Are what they get always lossy, or can they get the filename in all the file system pathname glory? I'm thinking we want some way for the extensions to have visibility to the (relative) real pathnames. http://codereview.chromium.org/8601012/diff/30001/content/browser/download/do... content/browser/download/download_query.cc:56: static std::string GetUrl(const DownloadItem& item) { Do we have long term plans about giving the extensions access to the rest of the URL-related information? (Referrer, redirect chain?) (Not a question directly relevant to this CL.) http://codereview.chromium.org/8601012/diff/30001/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/30001/content/browser/download/do... content/browser/download/download_query.h:47: FILTER_FIELD_BYTES_RECEIVED, // int nit, suggestion: I find this type of annotation easier to read if the comments are lined up. http://codereview.chromium.org/8601012/diff/30001/content/browser/download/do... content/browser/download/download_query.h:108: // Adds a new sort field named |name| and sets its direction to |direction|. nit: I'm still tripping over the connotations in the comment of "name" vs. "type". When I see that something is "named" something, I take the name as being a pure label, and not having semantic meaning, which isn't true here.
The new tests cover every line except the NOTREACHED branches, which are only there to make the compiler happy. I ripped out the NOTIMPLEMENTED features. I'm sure that I haven't properly commented how the two super tests work, so questions and suggestions are welcome. http://codereview.chromium.org/8601012/diff/30001/content/browser/download/do... File content/browser/download/download_query.cc (right): http://codereview.chromium.org/8601012/diff/30001/content/browser/download/do... content/browser/download/download_query.cc:54: } On 2011/12/01 19:24:30, rdsmith wrote: > How do these two routines relate to what actually shows up on the file system? > My memory is that we decided that the extension system couldn't do arbitrary > filename setting, but could set file names relative to the download directory. > Is there a way (for filtering, sorting, getting) for them to get those relative > file names? Are what they get always lossy, or can they get the filename in all > the file system pathname glory? I'm thinking we want some way for the > extensions to have visibility to the (relative) real pathnames. I don't understand all the differences between lossy names and glorious names. Maybe Asanka does. Using relative names instead of absolute names sgtm, but not for this CL. See http://crbug.com/106080 http://codereview.chromium.org/8601012/diff/30001/content/browser/download/do... content/browser/download/download_query.cc:56: static std::string GetUrl(const DownloadItem& item) { On 2011/12/01 19:24:30, rdsmith wrote: > Do we have long term plans about giving the extensions access to the rest of the > URL-related information? (Referrer, redirect chain?) > > (Not a question directly relevant to this CL.) See http://crbug.com/106084 http://codereview.chromium.org/8601012/diff/30001/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/30001/content/browser/download/do... content/browser/download/download_query.h:47: FILTER_FIELD_BYTES_RECEIVED, // int On 2011/12/01 19:24:30, rdsmith wrote: > nit, suggestion: I find this type of annotation easier to read if the comments > are lined up. Done. http://codereview.chromium.org/8601012/diff/30001/content/browser/download/do... content/browser/download/download_query.h:108: // Adds a new sort field named |name| and sets its direction to |direction|. On 2011/12/01 19:24:30, rdsmith wrote: > nit: I'm still tripping over the connotations in the comment of "name" vs. > "type". When I see that something is "named" something, I take the name as > being a pure label, and not having semantic meaning, which isn't true here. I'm open to suggestions. There's a Callback typedef (currently FilterType), an enum (currently FilterFieldName), and a method (currently Filter) that all need different but similar names. This parameter's name is tied to the enum's name. If I rename the enum to FilterType, then what should I rename the Callback typdef?
http://codereview.chromium.org/8601012/diff/30001/content/browser/download/do... File content/browser/download/download_query.cc (right): http://codereview.chromium.org/8601012/diff/30001/content/browser/download/do... content/browser/download/download_query.cc:54: } On 2011/12/02 22:31:53, b s h wrote: > I don't understand all the differences between lossy names and glorious names. > Maybe Asanka does. On OS_POSIX the contents of the FilePath is interpreted as a string encoded in the active codepage. It's not guaranteed that this string could be translated to Unicode without loss. Hence LossyDisplayName(). This is less of an issue on Windows, where a FilePath is already a UTF-16 string, and on Mac and ChromeOS where the active codepage is UTF-8.
Quick note before I review the unit tests: I don't see a perftest here (I'm used to perf tests having name *_perftest.cc). Did you fold that into the unit tests/are still working on it/have forgotten about it/leaving it for another CL?
On 2011/12/05 20:02:42, rdsmith wrote: > Quick note before I review the unit tests: I don't see a perftest here (I'm used > to perf tests having name *_perftest.cc). Did you fold that into the unit > tests/are still working on it/have forgotten about it/leaving it for another CL? Sorry, still working on it.
http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:56: FILTER_FIELD_START_TIME, // int nit: Mention somewhere that times are in ms? http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... File content/browser/download/download_query_unittest.cc (right): http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query_unittest.cc:49: DownloadVector all_items_; Why have all_items_ separate from mocks_? It's not a big deal, but it seems as if you could just have all_items_ and delete at end from there. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query_unittest.cc:56: EXPECT_CALL(mock(_index), _method).WillRepeatedly(_value) nit, suggestion: My personal reaction to this is that it'd be more readable without the macro (presuming the audience has familiarity w/ gmock). http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query_unittest.cc:94: MOCK(0, GetStartTime(), Return(base::Time::FromDoubleT(0.001))); I get nervous whenever I see code that expects some level of precisions from doubles. Would you be willing to do FromInt(1),FromInt(2) and compare against 1000/2000? http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query_unittest.cc:129: bool IdNotEqual(int not_id, const DownloadItem& item) { nit: I think Chrome custom is for all helper functions for a file to be in a block at the beginning. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query_unittest.cc:134: TEST_F(DownloadQueryTest, AllFilters) { This is a good test for testing all the filters at once, but I'm afraid I also think you should also have a test that tests each one individually. If this test ever rots such that the result wouldn't change if you removed a filter, that filter could then rot. Having a separate test per-filter makes that harder (and also means it's a bit easier to figure out how to add a new thing to filter on, which is almost certainly going to happen repeatedly). Same comment applies to sort test below. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query_unittest.cc:147: MOCK(i, GetId(), Return(i)); Here is a case where I feel some urge to ask you to put in a specialized macro. It would take the method to mock, index of that filter, the value to return at that index, and the type to return at any other index. I'd also suggest putting in a constant/define for the number of items, and putting in an assert/test at the end of the mock list to make it harder to add a new filter/entry in the list without adding a new mock. This is a bit of overkill, but I'm trying to make it as easy as possible for someone who doesn't know the test to add a new filter type to DownloadQuery. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query_unittest.cc:186: Search(); Is it possible to search on a query multiple times? If so, you could search after putting in each new filter and confirm that your results dropped by one each time. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query_unittest.cc:192: // mock br d da fn m p st s tb u id dbh I think if you're going to have this comment here (and I like it), you should have a full spelling out of each abbreviation. Maybe mock Bytes received danger type Safety state ?
This looks really cool! Nice improvement. I'll let Randy do the detailed review, I primarily focused on the class interface. http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.h:16: namespace download_util { On 2011/11/23 21:07:34, rdsmith wrote: > Given that we've got a pretty descriptive top level class name that everything's > under, I'm not sure there's any need to put this in download_util. Have there been any thoughts about moving content/browser/downloads into the downloads namespace? http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... File content/browser/download/download_query.cc (right): http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.cc:138: if (left_value > right_value) return GT; Do you need to return GT/LT/EQ? Could you just return bool and do the LT operator like sort functors? http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.cc:201: case FILTER_FIELD_TOTAL_BYTES_GREATER: Should bytes filter fields be size_t or check >= 0? http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:25: // DownloadQuery query; Nice use of sample code to indicate how this works. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:36: class CONTENT_EXPORT DownloadQuery { Does this need to be in content/public? http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:44: typedef base::Callback<bool(const DownloadItem&)> FilterType; Nit: typedefs and enums come before methods - including constructor/destructor. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:64: // Returns false if |name| is unrecognized or if |value| is the wrong type or I'd change this comment to indicate that it Adds a new filter first, and then talk about error codes. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:71: // out all items. Thanks for spelling out that this won't check that filter could always generate some item - we don't want to check that two FILENAME_REGEX's are mutually exclusive, for example. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:72: bool Filter(FilterFieldName name, int value); Perhaps instead of all the overloading, do bool Filter(FilterFieldName name, const Value* value) Internally, this will check if value->GetType() matches the expected type. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:73: bool Filter(FilterFieldName name, bool value); Should these be called AddFilter, to indicate that a filter is being prepared but not executed at the time of the call? http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:112: typedef std::vector<DownloadItem*> DownloadVector; typedef's etc move to the top. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:118: DownloadVector* results) const; This could use an OutputIterator such as an insert_iterator or back_insert_iterator as an argument, instead of requiring that it goes into a DownloadVector. [Note: I wrote that before looking at the implementation. There may be performance implications due to the partial_sort on the result, and partial_sort requires random-access iterators IIRC]. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:122: bool Matches(const DownloadItem& item) const; Does this need to be public? Seems like a private method.
Search() usually takes about 46 nanoseconds per item per filter when all the filters are just 'return true'. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... File content/browser/download/download_query.cc (right): http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.cc:138: if (left_value > right_value) return GT; On 2011/12/05 21:56:26, cbentzel wrote: > Do you need to return GT/LT/EQ? Could you just return bool and do the LT > operator like sort functors? Yes to the first, no to the second. :-) Compare() is called per sort field by the DownloadComparator (which is the sort functor) in the context of possibly several other sort fields. There are two bits of information that Compare() returns to the Comparator: whether this (accessor) sort field of these two items is different, and if they're different, whether the left is less than or greater than the right. If the two items are equivalent, then the Comparator continues comparing other sort fields; if they're different, then it returns true or false (depending on the sort field's direction) regardless of subsequent sort fields. I could make Compare() return a different enum {KEEP_GOING, RETURN_TRUE, RETURN_FALSE}, but it reduces to ComparisonType. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.cc:201: case FILTER_FIELD_TOTAL_BYTES_GREATER: On 2011/12/05 21:56:26, cbentzel wrote: > Should bytes filter fields be size_t or check >= 0? If we're allowing users to specify conflicting filters, I'd rather also allow them to specify singular filters that filter out nothing or everything instead of risking forgetting to prevent the check at the caller and documenting the explosiveness. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:25: // DownloadQuery query; On 2011/12/05 21:56:26, cbentzel wrote: > Nice use of sample code to indicate how this works. Done. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:36: class CONTENT_EXPORT DownloadQuery { On 2011/12/05 21:56:26, cbentzel wrote: > Does this need to be in content/public? What's the rule for what goes in content/public? There are several exported things that aren't in public. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:44: typedef base::Callback<bool(const DownloadItem&)> FilterType; On 2011/12/05 21:56:26, cbentzel wrote: > Nit: typedefs and enums come before methods - including constructor/destructor. Done. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:56: FILTER_FIELD_START_TIME, // int On 2011/12/05 20:25:42, rdsmith wrote: > nit: Mention somewhere that times are in ms? Done. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:64: // Returns false if |name| is unrecognized or if |value| is the wrong type or On 2011/12/05 21:56:26, cbentzel wrote: > I'd change this comment to indicate that it Adds a new filter first, and then > talk about error codes. Done. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:71: // out all items. On 2011/12/05 21:56:26, cbentzel wrote: > Thanks for spelling out that this won't check that filter could always generate > some item - we don't want to check that two FILENAME_REGEX's are mutually > exclusive, for example. Done. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:72: bool Filter(FilterFieldName name, int value); On 2011/12/05 21:56:26, cbentzel wrote: > Perhaps instead of all the overloading, do > > bool Filter(FilterFieldName name, const Value* value) > > Internally, this will check if value->GetType() matches the expected type. Done. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:73: bool Filter(FilterFieldName name, bool value); On 2011/12/05 21:56:26, cbentzel wrote: > Should these be called AddFilter, to indicate that a filter is being prepared > but not executed at the time of the call? Done. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:112: typedef std::vector<DownloadItem*> DownloadVector; On 2011/12/05 21:56:26, cbentzel wrote: > typedef's etc move to the top. Done. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:118: DownloadVector* results) const; On 2011/12/05 21:56:26, cbentzel wrote: > This could use an OutputIterator such as an insert_iterator or > back_insert_iterator as an argument, instead of requiring that it goes into a > DownloadVector. > > [Note: I wrote that before looking at the implementation. There may be > performance implications due to the partial_sort on the result, and partial_sort > requires random-access iterators IIRC]. So, it cannot use an OutputIterator, right? http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:122: bool Matches(const DownloadItem& item) const; On 2011/12/05 21:56:26, cbentzel wrote: > Does this need to be public? Seems like a private method. Done. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... File content/browser/download/download_query_unittest.cc (right): http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query_unittest.cc:49: DownloadVector all_items_; On 2011/12/05 20:25:42, rdsmith wrote: > Why have all_items_ separate from mocks_? It's not a big deal, but it seems as > if you could just have all_items_ and delete at end from there. I didn't want to do that because it would require either instantiating Search<>() with vector<MockDI*> (which is a different type from vector<DI*>) or moving Search<>() into the header, but it looks like I can move half of Search<>() into the header and keep the other half in the cc. PTAL. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query_unittest.cc:56: EXPECT_CALL(mock(_index), _method).WillRepeatedly(_value) On 2011/12/05 20:25:42, rdsmith wrote: > nit, suggestion: My personal reaction to this is that it'd be more readable > without the macro (presuming the audience has familiarity w/ gmock). Done. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query_unittest.cc:94: MOCK(0, GetStartTime(), Return(base::Time::FromDoubleT(0.001))); On 2011/12/05 20:25:42, rdsmith wrote: > I get nervous whenever I see code that expects some level of precisions from > doubles. Would you be willing to do FromInt(1),FromInt(2) and compare against > 1000/2000? Done. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query_unittest.cc:129: bool IdNotEqual(int not_id, const DownloadItem& item) { On 2011/12/05 20:25:42, rdsmith wrote: > nit: I think Chrome custom is for all helper functions for a file to be in a > block at the beginning. Done. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query_unittest.cc:134: TEST_F(DownloadQueryTest, AllFilters) { On 2011/12/05 20:25:42, rdsmith wrote: > This is a good test for testing all the filters at once, but I'm afraid I also > think you should also have a test that tests each one individually. If this > test ever rots such that the result wouldn't change if you removed a filter, > that filter could then rot. Having a separate test per-filter makes that harder > (and also means it's a bit easier to figure out how to add a new thing to filter > on, which is almost certainly going to happen repeatedly). > > Same comment applies to sort test below. How does this look now with the EXPECT in a loop? http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query_unittest.cc:147: MOCK(i, GetId(), Return(i)); On 2011/12/05 20:25:42, rdsmith wrote: > Here is a case where I feel some urge to ask you to put in a specialized macro. > It would take the method to mock, index of that filter, the value to return at > that index, and the type to return at any other index. I'd also suggest > putting in a constant/define for the number of items, and putting in an > assert/test at the end of the mock list to make it harder to add a new > filter/entry in the list without adding a new mock. > > This is a bit of overkill, but I'm trying to make it as easy as possible for > someone who doesn't know the test to add a new filter type to DownloadQuery. Done. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query_unittest.cc:186: Search(); On 2011/12/05 20:25:42, rdsmith wrote: > Is it possible to search on a query multiple times? If so, you could search > after putting in each new filter and confirm that your results dropped by one > each time. Done. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query_unittest.cc:192: // mock br d da fn m p st s tb u id dbh On 2011/12/05 20:25:42, rdsmith wrote: > I think if you're going to have this comment here (and I like it), you should > have a full spelling out of each abbreviation. Maybe > > mock > Bytes received > danger type > Safety state > ? Done.
I'm happy with the interface now. I'll remove myself from the reviewers list and let rdsmith+asanka provide the L G T Ms. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:118: DownloadVector* results) const; On 2011/12/09 19:29:15, b s h wrote: > On 2011/12/05 21:56:26, cbentzel wrote: > > This could use an OutputIterator such as an insert_iterator or > > back_insert_iterator as an argument, instead of requiring that it goes into a > > DownloadVector. > > > > [Note: I wrote that before looking at the implementation. There may be > > performance implications due to the partial_sort on the result, and > partial_sort > > requires random-access iterators IIRC]. > > So, it cannot use an OutputIterator, right? Correct. I should have removed that comment. http://codereview.chromium.org/8601012/diff/45002/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/45002/content/browser/download/do... content/browser/download/download_query.h:122: DownloadVector* results) { Could this be const? http://codereview.chromium.org/8601012/diff/45002/content/browser/download/do... content/browser/download/download_query.h:139: void FinishSearch(DownloadVector* results); Could this be const as well?
I'm happy with the interface now. I'll remove myself from the reviewers list and let rdsmith+asanka provide the L G T Ms. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:118: DownloadVector* results) const; On 2011/12/09 19:29:15, b s h wrote: > On 2011/12/05 21:56:26, cbentzel wrote: > > This could use an OutputIterator such as an insert_iterator or > > back_insert_iterator as an argument, instead of requiring that it goes into a > > DownloadVector. > > > > [Note: I wrote that before looking at the implementation. There may be > > performance implications due to the partial_sort on the result, and > partial_sort > > requires random-access iterators IIRC]. > > So, it cannot use an OutputIterator, right? Correct. I should have removed that comment. http://codereview.chromium.org/8601012/diff/45002/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/45002/content/browser/download/do... content/browser/download/download_query.h:122: DownloadVector* results) { Could this be const? http://codereview.chromium.org/8601012/diff/45002/content/browser/download/do... content/browser/download/download_query.h:139: void FinishSearch(DownloadVector* results); Could this be const as well?
http://codereview.chromium.org/8601012/diff/45002/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/45002/content/browser/download/do... content/browser/download/download_query.h:122: DownloadVector* results) { On 2011/12/09 19:38:58, cbentzel wrote: > Could this be const? Done. http://codereview.chromium.org/8601012/diff/45002/content/browser/download/do... content/browser/download/download_query.h:139: void FinishSearch(DownloadVector* results); On 2011/12/09 19:38:58, cbentzel wrote: > Could this be const as well? Done.
http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/7001/content/browser/download/dow... content/browser/download/download_query.h:16: namespace download_util { On 2011/12/05 21:56:26, cbentzel wrote: > On 2011/11/23 21:07:34, rdsmith wrote: > > Given that we've got a pretty descriptive top level class name that > everything's > > under, I'm not sure there's any need to put this in download_util. > > Have there been any thoughts about moving content/browser/downloads into the > downloads namespace? We've thought about it, but it's never seemed needed. Any thoughts for what determines when something should have its own namespace? http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:36: class CONTENT_EXPORT DownloadQuery { On 2011/12/09 19:29:15, b s h wrote: > On 2011/12/05 21:56:26, cbentzel wrote: > > Does this need to be in content/public? > > What's the rule for what goes in content/public? There are several exported > things that aren't in public. This is a good question that I don't know the answer to. I think I'm going to say: If the DownloadManager isn't in content/public, we shouldn't put stuff "lower level" than the DownloadManager in content/public. Having said that, I suspect at some point we're going to need to put a whole lot of stuff over into that directory. I'll ping jam@ about it, but don't worry about it for this CL. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:72: bool Filter(FilterFieldName name, int value); On 2011/12/05 21:56:26, cbentzel wrote: > Perhaps instead of all the overloading, do > > bool Filter(FilterFieldName name, const Value* value) > > Internally, this will check if value->GetType() matches the expected type. Does it work to make this a reference rather than a pointer? My concern is to make this class as easy as possible for the client to use; before this change, if they were comparing against an int or string, they could just pass that value. Now, they have to pass the address of the value, which may require creating a variable. (Note that I would consider "work" to mean that "AddFilter(<name>, 5);" or "AddFilter(<name>, "field match value")" would work.) If for some reason we need a pointer, tell me, and I'll see if I can get Chris to change his mind about the original request. http://codereview.chromium.org/8601012/diff/43003/content/browser/download/do... File content/browser/download/download_query_unittest.cc (right): http://codereview.chromium.org/8601012/diff/43003/content/browser/download/do... content/browser/download/download_query_unittest.cc:115: #define SWITCH2(_index, _col1, _ret1, _default) \ nit: A sentence or two of comment documentation? http://codereview.chromium.org/8601012/diff/43003/content/browser/download/do... content/browser/download/download_query_unittest.cc:137: // it fails, except one item, which matches all the filters without exception. This comment doesn't match (to me, at least) with the existence of SWITCH3/4--those macros make it sound like, at least for a few of these, you're doing something more complex. Put in comments explaining the complexity? http://codereview.chromium.org/8601012/diff/43003/content/browser/download/do... content/browser/download/download_query_unittest.cc:367: EXPECT_CALL(mock(1), GetId()).WillRepeatedly(Return(0)); Having the same id makes me nervous--we don't different ids in this code right now, but eventually we will. Maybe include ids that would sort opposite to the DB handle sort? (And why does this test have ids specified and the other ones don't?)
Sorry, one other request: I find the issue description (and the subject) a bit terse; could you expand it? Even just "Created the DownloadQuery class as a helper for the (coming) DownloadSearch function to hold the guts of sorting and searching download items." would be good by me.
http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:72: bool Filter(FilterFieldName name, int value); On 2011/12/12 18:17:54, rdsmith wrote: > On 2011/12/05 21:56:26, cbentzel wrote: > > Perhaps instead of all the overloading, do > > > > bool Filter(FilterFieldName name, const Value* value) > > > > Internally, this will check if value->GetType() matches the expected type. > > Does it work to make this a reference rather than a pointer? My concern is to > make this class as easy as possible for the client to use; before this change, > if they were comparing against an int or string, they could just pass that > value. Now, they have to pass the address of the value, which may require > creating a variable. (Note that I would consider "work" to mean that > "AddFilter(<name>, 5);" or "AddFilter(<name>, "field match value")" would work.) > > If for some reason we need a pointer, tell me, and I'll see if I can get Chris > to change his mind about the original request. Reference would work. I think that most of the consumers [extension function implementations] will end up extracting Value's from the JSON, which is why I suggested this - so a more natural fit for most production code consumers at the potential cost of test consumers. Usually Value's are passed around by pointers however.
http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:72: bool Filter(FilterFieldName name, int value); On 2011/12/12 19:08:29, cbentzel wrote: > On 2011/12/12 18:17:54, rdsmith wrote: > > On 2011/12/05 21:56:26, cbentzel wrote: > > > Perhaps instead of all the overloading, do > > > > > > bool Filter(FilterFieldName name, const Value* value) > > > > > > Internally, this will check if value->GetType() matches the expected type. > > > > Does it work to make this a reference rather than a pointer? My concern is to > > make this class as easy as possible for the client to use; before this change, > > if they were comparing against an int or string, they could just pass that > > value. Now, they have to pass the address of the value, which may require > > creating a variable. (Note that I would consider "work" to mean that > > "AddFilter(<name>, 5);" or "AddFilter(<name>, "field match value")" would > work.) > > > > If for some reason we need a pointer, tell me, and I'll see if I can get Chris > > to change his mind about the original request. > > Reference would work. I think that most of the consumers [extension function > implementations] will end up extracting Value's from the JSON, which is why I > suggested this - so a more natural fit for most production code consumers at the > potential cost of test consumers. > > Usually Value's are passed around by pointers however. I'm a bit reluctant to target DownloadQuery directly at extensions; it may well have C++ targets as well. I've envisions the extensions layer as changing impedances from C++ to JS.
http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:36: class CONTENT_EXPORT DownloadQuery { On 2011/12/12 18:17:54, rdsmith wrote: > On 2011/12/09 19:29:15, b s h wrote: > > On 2011/12/05 21:56:26, cbentzel wrote: > > > Does this need to be in content/public? > > > > What's the rule for what goes in content/public? There are several exported > > things that aren't in public. > > This is a good question that I don't know the answer to. I think I'm going to > say: If the DownloadManager isn't in content/public, we shouldn't put stuff > "lower level" than the DownloadManager in content/public. Having said that, I > suspect at some point we're going to need to put a whole lot of stuff over into > that directory. I'll ping jam@ about it, but don't worry about it for this CL. Done. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:72: bool Filter(FilterFieldName name, int value); On 2011/12/12 18:17:54, rdsmith wrote: > On 2011/12/05 21:56:26, cbentzel wrote: > > Perhaps instead of all the overloading, do > > > > bool Filter(FilterFieldName name, const Value* value) > > > > Internally, this will check if value->GetType() matches the expected type. > > Does it work to make this a reference rather than a pointer? My concern is to > make this class as easy as possible for the client to use; before this change, > if they were comparing against an int or string, they could just pass that > value. Now, they have to pass the address of the value, which may require > creating a variable. (Note that I would consider "work" to mean that > "AddFilter(<name>, 5);" or "AddFilter(<name>, "field match value")" would work.) > > If for some reason we need a pointer, tell me, and I'll see if I can get Chris > to change his mind about the original request. base::Values don't support the C++ copy constructor or assignment, so they also don't have methods that return references or stack copies of Value objects, so you're going to need a variable no matter what. I changed it to a const ref anyway to document functionality and avoid the null check. This also reminded me about a memory leak where the test would pass base::Value::CreateFooValue() directly to AddFilter, instead of via scoped_ptr. DownloadQuery didn't and shouldn't take ownership of the Value. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:72: bool Filter(FilterFieldName name, int value); On 2011/12/12 19:19:29, rdsmith wrote: > On 2011/12/12 19:08:29, cbentzel wrote: > > On 2011/12/12 18:17:54, rdsmith wrote: > > > On 2011/12/05 21:56:26, cbentzel wrote: > > > > Perhaps instead of all the overloading, do > > > > > > > > bool Filter(FilterFieldName name, const Value* value) > > > > > > > > Internally, this will check if value->GetType() matches the expected type. > > > > > > Does it work to make this a reference rather than a pointer? My concern is > to > > > make this class as easy as possible for the client to use; before this > change, > > > if they were comparing against an int or string, they could just pass that > > > value. Now, they have to pass the address of the value, which may require > > > creating a variable. (Note that I would consider "work" to mean that > > > "AddFilter(<name>, 5);" or "AddFilter(<name>, "field match value")" would > > work.) > > > > > > If for some reason we need a pointer, tell me, and I'll see if I can get > Chris > > > to change his mind about the original request. > > > > Reference would work. I think that most of the consumers [extension function > > implementations] will end up extracting Value's from the JSON, which is why I > > suggested this - so a more natural fit for most production code consumers at > the > > potential cost of test consumers. > > > > Usually Value's are passed around by pointers however. > > I'm a bit reluctant to target DownloadQuery directly at extensions; it may well > have C++ targets as well. I've envisions the extensions layer as changing > impedances from C++ to JS. How would you feel about adding helper methods like this to DownloadQuery when we have a specific use-case for them? bool AddFilter(FilterType type, int cpp_value) { scoped_ptr<base::Value> json_value(base::Value::CreateIntegerValue(cpp_value)); return AddFilter(type, *json_value.get()); } It does seem strange to convert int->Value->int within the same class, but it makes AddFilter(FilterType,...) simpler and cleaner. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:118: DownloadVector* results) const; On 2011/12/09 19:38:58, cbentzel wrote: > On 2011/12/09 19:29:15, b s h wrote: > > On 2011/12/05 21:56:26, cbentzel wrote: > > > This could use an OutputIterator such as an insert_iterator or > > > back_insert_iterator as an argument, instead of requiring that it goes into > a > > > DownloadVector. > > > > > > [Note: I wrote that before looking at the implementation. There may be > > > performance implications due to the partial_sort on the result, and > > partial_sort > > > requires random-access iterators IIRC]. > > > > So, it cannot use an OutputIterator, right? > > Correct. I should have removed that comment. Done. http://codereview.chromium.org/8601012/diff/43003/content/browser/download/do... File content/browser/download/download_query_unittest.cc (right): http://codereview.chromium.org/8601012/diff/43003/content/browser/download/do... content/browser/download/download_query_unittest.cc:115: #define SWITCH2(_index, _col1, _ret1, _default) \ On 2011/12/12 18:17:54, rdsmith wrote: > nit: A sentence or two of comment documentation? Done. http://codereview.chromium.org/8601012/diff/43003/content/browser/download/do... content/browser/download/download_query_unittest.cc:137: // it fails, except one item, which matches all the filters without exception. On 2011/12/12 18:17:54, rdsmith wrote: > This comment doesn't match (to me, at least) with the existence of > SWITCH3/4--those macros make it sound like, at least for a few of these, you're > doing something more complex. Put in comments explaining the complexity? Done. http://codereview.chromium.org/8601012/diff/43003/content/browser/download/do... content/browser/download/download_query_unittest.cc:367: EXPECT_CALL(mock(1), GetId()).WillRepeatedly(Return(0)); On 2011/12/12 18:17:54, rdsmith wrote: > Having the same id makes me nervous--we don't different ids in this code right > now, but eventually we will. Maybe include ids that would sort opposite to the > DB handle sort? (And why does this test have ids specified and the other ones > don't?) Sorting by id doesn't kick in unless all the sorters return EQ. That's tested by DownloadQuerySortId above (which also specifies ids...). The db handle sorting doesn't kick in unless all the sorters return EQ *and* the ids are equal. These two implicit sorters (id and db_handle) are just used to stabilize the sort operation. I could use the address of the DownloadItem instead of its id and db_handle, but that seemed more magical to me. If that doesn't help, feel free to roll over and let's chat irl before I need to go home to let the pup out at 4:30.
LGTM given the sort change as we discussed. As I mention, I'd vote in favor of Value * if that's the usual pattern, but I won't fight if you feel strongly. http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:72: bool Filter(FilterFieldName name, int value); On 2011/12/12 19:52:10, b s h wrote: > On 2011/12/12 19:19:29, rdsmith wrote: > > On 2011/12/12 19:08:29, cbentzel wrote: > > > On 2011/12/12 18:17:54, rdsmith wrote: > > > > On 2011/12/05 21:56:26, cbentzel wrote: > > > > > Perhaps instead of all the overloading, do > > > > > > > > > > bool Filter(FilterFieldName name, const Value* value) > > > > > > > > > > Internally, this will check if value->GetType() matches the expected > type. > > > > > > > > Does it work to make this a reference rather than a pointer? My concern > is > > to > > > > make this class as easy as possible for the client to use; before this > > change, > > > > if they were comparing against an int or string, they could just pass that > > > > value. Now, they have to pass the address of the value, which may require > > > > creating a variable. (Note that I would consider "work" to mean that > > > > "AddFilter(<name>, 5);" or "AddFilter(<name>, "field match value")" would > > > work.) > > > > > > > > If for some reason we need a pointer, tell me, and I'll see if I can get > > Chris > > > > to change his mind about the original request. > > > > > > Reference would work. I think that most of the consumers [extension function > > > implementations] will end up extracting Value's from the JSON, which is why > I > > > suggested this - so a more natural fit for most production code consumers at > > the > > > potential cost of test consumers. > > > > > > Usually Value's are passed around by pointers however. > > > > I'm a bit reluctant to target DownloadQuery directly at extensions; it may > well > > have C++ targets as well. I've envisions the extensions layer as changing > > impedances from C++ to JS. > > How would you feel about adding helper methods like this to DownloadQuery when > we have a specific use-case for them? > bool AddFilter(FilterType type, int cpp_value) { > scoped_ptr<base::Value> json_value(base::Value::CreateIntegerValue(cpp_value)); > return AddFilter(type, *json_value.get()); > } That's ok by me. But let's please add them if we get non-test clients that want them. > It does seem strange to convert int->Value->int within the same class, but it > makes AddFilter(FilterType,...) simpler and cleaner. I'm good with that (so long as you avoid the aforementioned leak :-}). http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:72: bool Filter(FilterFieldName name, int value); On 2011/12/12 19:52:10, b s h wrote: > On 2011/12/12 18:17:54, rdsmith wrote: > > On 2011/12/05 21:56:26, cbentzel wrote: > > > Perhaps instead of all the overloading, do > > > > > > bool Filter(FilterFieldName name, const Value* value) > > > > > > Internally, this will check if value->GetType() matches the expected type. > > > > Does it work to make this a reference rather than a pointer? My concern is to > > make this class as easy as possible for the client to use; before this change, > > if they were comparing against an int or string, they could just pass that > > value. Now, they have to pass the address of the value, which may require > > creating a variable. (Note that I would consider "work" to mean that > > "AddFilter(<name>, 5);" or "AddFilter(<name>, "field match value")" would > work.) > > > > If for some reason we need a pointer, tell me, and I'll see if I can get Chris > > to change his mind about the original request. > > base::Values don't support the C++ copy constructor or assignment, so they also > don't have methods that return references or stack copies of Value objects, so > you're going to need a variable no matter what. > I changed it to a const ref anyway to document functionality and avoid the null > check. My inclination would be not to do this, based on Chris' comment that this isn't how Value's are usually passed around. We should be consistent with the code base unless we have a strong reason not to be. > This also reminded me about a memory leak where the test would pass > base::Value::CreateFooValue() directly to AddFilter, instead of via scoped_ptr. > DownloadQuery didn't and shouldn't take ownership of the Value. *nod* http://codereview.chromium.org/8601012/diff/51003/content/browser/download/do... File content/browser/download/download_query_unittest.cc (right): http://codereview.chromium.org/8601012/diff/51003/content/browser/download/do... content/browser/download/download_query_unittest.cc:377: EXPECT_CALL(mock(1), GetId()).WillRepeatedly(Return(0)); From our offline conversation: Given that we should never have two identical ids, I'd rather put in an assertion for that and make the ID the only backing sort, and skip the DB handle sort. (If the ids are identical, the DB handles are likely to be identical anyway). To be fair (WRT the assertions): Is there any reason the user couldn't have the same download item listed twice in the input collection? We (you :-}) should make an explicit choice as to whether you want to allow that or not, but if you do, that probably means *not* putting in assertions that we don't have identical download ids. But I still think getting rid of the DB handle backing sort makes sense.
http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:72: bool Filter(FilterFieldName name, int value); On 2011/12/12 21:01:38, rdsmith wrote: > On 2011/12/12 19:52:10, b s h wrote: > > On 2011/12/12 18:17:54, rdsmith wrote: > > > On 2011/12/05 21:56:26, cbentzel wrote: > > > > Perhaps instead of all the overloading, do > > > > > > > > bool Filter(FilterFieldName name, const Value* value) > > > > > > > > Internally, this will check if value->GetType() matches the expected type. > > > > > > Does it work to make this a reference rather than a pointer? My concern is > to > > > make this class as easy as possible for the client to use; before this > change, > > > if they were comparing against an int or string, they could just pass that > > > value. Now, they have to pass the address of the value, which may require > > > creating a variable. (Note that I would consider "work" to mean that > > > "AddFilter(<name>, 5);" or "AddFilter(<name>, "field match value")" would > > work.) > > > > > > If for some reason we need a pointer, tell me, and I'll see if I can get > Chris > > > to change his mind about the original request. > > > > base::Values don't support the C++ copy constructor or assignment, so they > also > > don't have methods that return references or stack copies of Value objects, so > > you're going to need a variable no matter what. > > I changed it to a const ref anyway to document functionality and avoid the > null > > check. > > My inclination would be not to do this, based on Chris' comment that this isn't > how Value's are usually passed around. We should be consistent with the code > base unless we have a strong reason not to be. There are more than half as many const Value& as const Value*. It's used in the situations that warrant it, it's just that the public interface isn't optimized for it. http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=%22c... http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=%22c... The strong reason here is the same reason behind passing anything by const&: to document and enforce proper usage. Passing pointers makes ownership and nullability ambiguous, requiring comments that somebody will probably forget to read (probably me). This seems like a harder rule than "pass Values by pointer only". > > > This also reminded me about a memory leak where the test would pass > > base::Value::CreateFooValue() directly to AddFilter, instead of via > scoped_ptr. > > DownloadQuery didn't and shouldn't take ownership of the Value. > > *nod* http://codereview.chromium.org/8601012/diff/51003/content/browser/download/do... File content/browser/download/download_query_unittest.cc (right): http://codereview.chromium.org/8601012/diff/51003/content/browser/download/do... content/browser/download/download_query_unittest.cc:377: EXPECT_CALL(mock(1), GetId()).WillRepeatedly(Return(0)); On 2011/12/12 21:01:38, rdsmith wrote: > From our offline conversation: Given that we should never have two identical > ids, I'd rather put in an assertion for that and make the ID the only backing > sort, and skip the DB handle sort. (If the ids are identical, the DB handles > are likely to be identical anyway). Done. > > To be fair (WRT the assertions): Is there any reason the user couldn't have the > same download item listed twice in the input collection? We (you :-}) should > make an explicit choice as to whether you want to allow that or not, but if you > do, that probably means *not* putting in assertions that we don't have identical > download ids. But I still think getting rid of the DB handle backing sort makes > sense. The primary (perhaps only) caller of DownloadQuery::Search should be DownloadManager, which will have bigger problems if it has more than one item with the same id.
http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/36001/content/browser/download/do... content/browser/download/download_query.h:72: bool Filter(FilterFieldName name, int value); On 2011/12/12 21:28:06, b s h wrote: > On 2011/12/12 21:01:38, rdsmith wrote: > > On 2011/12/12 19:52:10, b s h wrote: > > > On 2011/12/12 18:17:54, rdsmith wrote: > > > > On 2011/12/05 21:56:26, cbentzel wrote: > > > > > Perhaps instead of all the overloading, do > > > > > > > > > > bool Filter(FilterFieldName name, const Value* value) > > > > > > > > > > Internally, this will check if value->GetType() matches the expected > type. > > > > > > > > Does it work to make this a reference rather than a pointer? My concern > is > > to > > > > make this class as easy as possible for the client to use; before this > > change, > > > > if they were comparing against an int or string, they could just pass that > > > > value. Now, they have to pass the address of the value, which may require > > > > creating a variable. (Note that I would consider "work" to mean that > > > > "AddFilter(<name>, 5);" or "AddFilter(<name>, "field match value")" would > > > work.) > > > > > > > > If for some reason we need a pointer, tell me, and I'll see if I can get > > Chris > > > > to change his mind about the original request. > > > > > > base::Values don't support the C++ copy constructor or assignment, so they > > also > > > don't have methods that return references or stack copies of Value objects, > so > > > you're going to need a variable no matter what. > > > I changed it to a const ref anyway to document functionality and avoid the > > null > > > check. > > > > My inclination would be not to do this, based on Chris' comment that this > isn't > > how Value's are usually passed around. We should be consistent with the code > > base unless we have a strong reason not to be. > > There are more than half as many const Value& as const Value*. It's used in the > situations that warrant it, it's just that the public interface isn't optimized > for it. > http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=%252... > http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=%252... > The strong reason here is the same reason behind passing anything by const&: to > document and enforce proper usage. Passing pointers makes ownership and > nullability ambiguous, requiring comments that somebody will probably forget to > read (probably me). This seems like a harder rule than "pass Values by pointer > only". > > > > > > This also reminded me about a memory leak where the test would pass > > > base::Value::CreateFooValue() directly to AddFilter, instead of via > > scoped_ptr. > > > DownloadQuery didn't and shouldn't take ownership of the Value. > > > > *nod* > Ok, I'm good with passing by reference. I just wanted to be a good lemming.
LGTM. Comments are nits. http://codereview.chromium.org/8601012/diff/50004/content/browser/download/do... File content/browser/download/download_query.cc (right): http://codereview.chromium.org/8601012/diff/50004/content/browser/download/do... content/browser/download/download_query.cc:338: direction, &GetFilename)); Nit: indentation. Here and below. http://codereview.chromium.org/8601012/diff/50004/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/50004/content/browser/download/do... content/browser/download/download_query.h:31: // base::Value::CreateIntegerValue(0))); Does this example still hold? base::Value::Create*Value() returns a Value*. http://codereview.chromium.org/8601012/diff/50004/content/content_browser.gypi File content/content_browser.gypi (right): http://codereview.chromium.org/8601012/diff/50004/content/content_browser.gyp... content/content_browser.gypi:166: 'browser/download/download_query.cc', Nit: The sources list should be in alphabetical order. http://codereview.chromium.org/8601012/diff/50004/content/content_tests.gypi File content/content_tests.gypi (right): http://codereview.chromium.org/8601012/diff/50004/content/content_tests.gypi#... content/content_tests.gypi:187: 'browser/download/download_query_unittest.cc', Nit: sort.
Thank you, everybody, for your help and your patience! I'll submit as soon as the trybots are green. I can probably mail out DownloadsSearchFunction this week. http://codereview.chromium.org/8601012/diff/50004/content/browser/download/do... File content/browser/download/download_query.cc (right): http://codereview.chromium.org/8601012/diff/50004/content/browser/download/do... content/browser/download/download_query.cc:338: direction, &GetFilename)); On 2011/12/13 16:17:12, asanka wrote: > Nit: indentation. Here and below. Done. http://codereview.chromium.org/8601012/diff/50004/content/browser/download/do... File content/browser/download/download_query.h (right): http://codereview.chromium.org/8601012/diff/50004/content/browser/download/do... content/browser/download/download_query.h:31: // base::Value::CreateIntegerValue(0))); On 2011/12/13 16:17:12, asanka wrote: > Does this example still hold? base::Value::Create*Value() returns a Value*. Done. http://codereview.chromium.org/8601012/diff/50004/content/content_browser.gypi File content/content_browser.gypi (right): http://codereview.chromium.org/8601012/diff/50004/content/content_browser.gyp... content/content_browser.gypi:166: 'browser/download/download_query.cc', On 2011/12/13 16:17:12, asanka wrote: > Nit: The sources list should be in alphabetical order. Done. http://codereview.chromium.org/8601012/diff/50004/content/content_tests.gypi File content/content_tests.gypi (right): http://codereview.chromium.org/8601012/diff/50004/content/content_tests.gypi#... content/content_tests.gypi:187: 'browser/download/download_query_unittest.cc', On 2011/12/13 16:17:12, asanka wrote: > Nit: sort. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/8601012/67006
Try job failure for 8601012-67006 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
No LGTM from valid reviewers yet.
No LGTM from valid reviewers yet.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/8601012/72001
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is mac_rel, revision is 114424, job name was 8601012-72001 (previous was lost) (previous was lost) (previous was lost) (retry).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/8601012/72001
Change committed as 114460 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
