DownloadQuery filters and sorts DownloadItems.

content/browser/download/download_query.h

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_QUERY_H_
+#define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_QUERY_H_
+#pragma once
+
+#include <map>
+#include <string>
+#include <vector>
+
+#include "base/callback.h"
+#include "content/browser/download/download_item.h"
+
+// Filter and sort a vector of DownloadItem*s.
+//
+// The following example copies from |all_items| to |results| those
+// DownloadItem*s whose start time is not 0 or whose id is odd, sorts primarily
+// by bytes received ascending and secondarily by url descending, and limits the
+// results to 20 items. Any number of filter fields or sort rules is
+// permissible. If all sort fields compare two DownloadItems equivalently, then
+// they are sorted by their id ascending or db_handle ascending.
+//
+// DownloadQuery query;

[cbentzel, 2011/12/05 21:56:26]

Nice use of sample code to indicate how this works.

[benjhayden, 2011/12/09 19:29:15]

Done.

+// CHECK(query.Filter(FILTER_FIELD_START_TIME, 0));
+// bool FilterOutOddDownloads(const DownloadItem& item) {
+//   return 0 == (item.GetId() % 2);
+// }
+// CHECK(query.Filter(base::Bind(&FilterOutOddDownloads)));
+// query.Sort(SORT_FIELD_BYTES_RECEIVED, ASCENDING);
+// query.Sort(SORT_FIELD_URL, DESCENDING);
+// query.Limit(20);
+// DownloadVector all_items, results;
+// query.Search(all_items.begin(), all_items.end(), &results);
+class CONTENT_EXPORT DownloadQuery {

[cbentzel, 2011/12/05 21:56:26]

Does this need to be in content/public?

[benjhayden, 2011/12/09 19:29:15]

What's the rule for what goes in content/public? There are several exported
things that aren't in public.

[Randy Smith (Not in Mondays), 2011/12/12 18:17:54]

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.

[benjhayden, 2011/12/12 19:52:10]

Done.

+ public:
+  DownloadQuery();
+  ~DownloadQuery();
+
+  // FilterType is a Callback that takes a DownloadItem and returns true if the
+  // item matches the filter and false otherwise.
+  // query.Filter(base::Bind(&YourFilterFunction));
+  typedef base::Callback<bool(const DownloadItem&)> FilterType;

[cbentzel, 2011/12/05 21:56:26]

Nit: typedefs and enums come before methods - including constructor/destructor.

[benjhayden, 2011/12/09 19:29:15]

Done.

+
+  enum FilterFieldName {
+    FILTER_FIELD_BYTES_RECEIVED,       // int
+    FILTER_FIELD_DANGER_ACCEPTED,      // bool
+    FILTER_FIELD_FILENAME,             // string16
+    FILTER_FIELD_FILENAME_REGEX,       // string16
+    FILTER_FIELD_MIME,                 // std::string
+    FILTER_FIELD_PAUSED,               // bool
+    FILTER_FIELD_QUERY,                // string16
+    FILTER_FIELD_STARTED_AFTER,        // int
+    FILTER_FIELD_STARTED_BEFORE,       // int
+    FILTER_FIELD_START_TIME,           // int

[Randy Smith (Not in Mondays), 2011/12/05 20:25:42]

nit: Mention somewhere that times are in ms?

[benjhayden, 2011/12/09 19:29:15]

Done.

+    FILTER_FIELD_TOTAL_BYTES,          // int
+    FILTER_FIELD_TOTAL_BYTES_GREATER,  // int
+    FILTER_FIELD_TOTAL_BYTES_LESS,     // int
+    FILTER_FIELD_URL,                  // std::string
+    FILTER_FIELD_URL_REGEX,            // std::string
+  };
+
+  // Returns false if |name| is unrecognized or if |value| is the wrong type or

[cbentzel, 2011/12/05 21:56:26]

I'd change this comment to indicate that it Adds a new filter first, and then
talk about error codes.

[benjhayden, 2011/12/09 19:29:15]

Done.

+  // malformed. Otherwise, adds a new filter of type |name| and with value
+  // |value|, and returns true. Search() will filter out all DownloadItem*s that
+  // do not match all filters. Multiple instances of the same FilterFieldName
+  // are allowed, so you can pass two regexes to Filter(URL_REGEX,...) in order
+  // to Search() for items whose url matches both regexes. You can also pass two
+  // different DownloadStates to Filter(), which will cause Search() to filter
+  // out all items.

[cbentzel, 2011/12/05 21:56:26]

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.

[benjhayden, 2011/12/09 19:29:15]

Done.

+  bool Filter(FilterFieldName name, int value);

[cbentzel, 2011/12/05 21:56:26]

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.

[benjhayden, 2011/12/09 19:29:15]

Done.

[Randy Smith (Not in Mondays), 2011/12/12 18:17:54]

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.

[cbentzel, 2011/12/12 19:08:29]

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.

[Randy Smith (Not in Mondays), 2011/12/12 19:19:29]

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.

[benjhayden, 2011/12/12 19:52:10]

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.

[benjhayden, 2011/12/12 19:52:10]

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.

[Randy Smith (Not in Mondays), 2011/12/12 21:01:38]

That's ok by me.  But let's please add them if we get non-test clients that want
them.
I'm good with that (so long as you avoid the aforementioned leak :-}).

[Randy Smith (Not in Mondays), 2011/12/12 21:01:38]

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.
*nod*

[benjhayden, 2011/12/12 21:28:06]

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".

[Randy Smith (Not in Mondays), 2011/12/12 21:31:57]

Ok, I'm good with passing by reference.  I just wanted to be a good lemming.

+  bool Filter(FilterFieldName name, bool value);

[cbentzel, 2011/12/05 21:56:26]

Should these be called AddFilter, to indicate that a filter is being prepared
but not executed at the time of the call?

[benjhayden, 2011/12/09 19:29:15]

Done.

+  bool Filter(FilterFieldName name, const std::string& value);
+  bool Filter(FilterFieldName name, const string16& value);
+  void Filter(DownloadStateInfo::DangerType danger);
+  void Filter(DownloadItem::DownloadState state);
+  void Filter(const FilterType& filter);
+
+  enum SortFieldName {
+    SORT_FIELD_BYTES_RECEIVED,
+    SORT_FIELD_DANGER,
+    SORT_FIELD_DANGER_ACCEPTED,
+    SORT_FIELD_FILENAME,
+    SORT_FIELD_MIME,
+    SORT_FIELD_PAUSED,
+    SORT_FIELD_START_TIME,
+    SORT_FIELD_STATE,
+    SORT_FIELD_TOTAL_BYTES,
+    SORT_FIELD_URL,
+  };
+
+  enum SortFieldDirection {
+    ASCENDING,
+    DESCENDING,
+  };
+
+  // Adds a new sort field named |name| and sets its direction to |direction|.
+  // After filtering DownloadItem*s, Search() will sort the results primarily by
+  // the sort field from the first call to Sort(), secondarily by the sort field
+  // from the second call to Sort(), and so on. For example, if the
+  // InputIterator passed to Search() yields four DownloadItems {id:0, error:0,
+  // start_time:0}, {id:1, error:0, start_time:1}, {id:2, error:1,
+  // start_time:0}, {id:3, error:1, start_time:1}, and Sort is called twice,
+  // once with (SORT_FIELD_ERROR, ASCENDING) then with (SORT_FIELD_START_TIME,
+  // DESCENDING), then Search() will return items ordered 1,0,3,2.
+  void Sort(SortFieldName name, SortFieldDirection direction);
+
+  // Limit the size of search results to |limit|.
+  void Limit(size_t limit) { limit_ = limit; }
+
+  typedef std::vector<DownloadItem*> DownloadVector;

[cbentzel, 2011/12/05 21:56:26]

typedef's etc move to the top.

[benjhayden, 2011/12/09 19:29:15]

Done.

+
+  // Filters DownloadItem*s from |iter| to |last| into |results|, sorts
+  // |results|, and limits the size of |results|. |results| must be non-NULL.
+  template <typename InputIterator>
+  void Search(InputIterator iter, const InputIterator last,
+              DownloadVector* results) const;

[cbentzel, 2011/12/05 21:56:26]

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]. 

[benjhayden, 2011/12/09 19:29:15]

So, it cannot use an OutputIterator, right?

[cbentzel, 2011/12/09 19:38:58]

Correct. I should have removed that comment.

[benjhayden, 2011/12/12 19:52:10]

Done.

+
+  // Returns true if |item| matches all set filter fields; returns false if any
+  // filter fields have been set that the item does not match.
+  bool Matches(const DownloadItem& item) const;

[cbentzel, 2011/12/05 21:56:26]

Does this need to be public? Seems like a private method.

[benjhayden, 2011/12/09 19:29:15]

Done.

+
+ private:
+  struct SortField;
+  class DownloadComparator;
+  typedef std::vector<FilterType> FilterFieldVector;
+  typedef std::vector<SortField> SortFieldVector;
+
+  bool FilterRegex(const std::string& regex_str,
+      const base::Callback<std::string(const DownloadItem&)>& accessor);
+
+  FilterFieldVector filter_fields_;
+  SortFieldVector sort_fields_;
+  size_t limit_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadQuery);
+};
+
+#endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_QUERY_H_