Implement chrome.experimental.downloads.search()

DownloadsSearchFunction (DSF) uses a templatized helper method to parse the json options and set up a DownloadQuery.

When 'id' is specified, DSF uses DownloadManager.GetDownloadItem() instead of DM.Search().

DownloadQuery uses templatized Field interfaces/classes to sort and filter a vector of DownloadItem*s by a complex set of criteria.

Adding a simple field to DownloadItem/DownloadQuery now requires adding 1 line to each of
0. DownloadItemToJson()
1. DSF::ParseArgs to call Parse<>
2. DownloadQuery::DownloadQuery to call SORT_FIELD
3. DownloadItem in extension_api.json
4. DownloadQuery in extension_api.json

[benjhayden, 2011-09-30 15:21:35]

Sorry, Asanka, feel free to weigh in here.

[Randy Smith (Not in Mondays), 2011-09-30 17:37:34]

This is a fascinating top-level approach question.  I personally find myself
leaning towards the x-macros approach (never knew that name before), but I'm not
sure I have the proper grounding in Chrome style to say that for sure.  But my
gut says that it's probably more Chromely.  Chris, could you weigh in on this
one?

My only concern on the javascript approach is one of performance, both of
getting a (potentially large) list of downloads over to the JS context, and of
doing all the processing in JS.  If you can get some numbers saying that that's
not a big deal, JS would be fine (I take it that that means you can write parts
of an extension interface in JS?)

One other high level approach question: My memory is that you were going to
revamp the search functionality within download_manager.cc as part of this work,
but this CL just uses DownloadManager::SearchDownloads() to return a full list
of persistent downloads.  Are you still planning to do that work?  We have two
many download search/getter functions already, and I'd like to reduce that
number rather than increase it (wherever they're implemented).

[benjhayden, 2011-09-30 18:14:50]

On 2011/09/30 17:37:34, rdsmith wrote:
> This is a fascinating top-level approach question.  I personally find myself
> leaning towards the x-macros approach (never knew that name before), but I'm
not
> sure I have the proper grounding in Chrome style to say that for sure.  But my
> gut says that it's probably more Chromely.  Chris, could you weigh in on this
> one?
> 
> My only concern on the javascript approach is one of performance, both of
> getting a (potentially large) list of downloads over to the JS context, and of
> doing all the processing in JS.  If you can get some numbers saying that
that's
> not a big deal, JS would be fine (I take it that that means you can write
parts
> of an extension interface in JS?)

Your next question appears to be another concern on the js approach, so I think
we can remove it from the table.
For curiosity's sake, then, yes, see
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/renderer/resou...
It appears to be mostly glue code.

> 
> One other high level approach question: My memory is that you were going to
> revamp the search functionality within download_manager.cc as part of this
work,
> but this CL just uses DownloadManager::SearchDownloads() to return a full list
> of persistent downloads.  Are you still planning to do that work?  We have two
> many download search/getter functions already, and I'd like to reduce that
> number rather than increase it (wherever they're implemented).

I was thinking of using the DownloadQuery class, which I linked to illustrate
the x-macros but which doesn't need to use x-macros. Search in this page for
"Search("
http://codereview.chromium.org/7192016/diff/29001/chrome/browser/download/dow...
If Chris prefers this verbose approach over the x-macros, then I'll split
DownloadsSearchFunction::RunInternal out into a DownloadQuery class.

[asanka, 2011-09-30 18:31:45]

On 2011/09/30 18:14:50, b s h wrote:
> On 2011/09/30 17:37:34, rdsmith wrote:
> > My only concern on the javascript approach is one of performance, both of
> > getting a (potentially large) list of downloads over to the JS context, and
of
> > doing all the processing in JS.  If you can get some numbers saying that
> that's
> > not a big deal, JS would be fine (I take it that that means you can write
> parts
> > of an extension interface in JS?)
> 
> Your next question appears to be another concern on the js approach, so I
think
> we can remove it from the table.
> For curiosity's sake, then, yes, see
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/renderer/resou...
> It appears to be mostly glue code.

As an additional data point: an extension would want to poll an active download
for progress frequently by searching with the download ID.

[cbentzel, 2011-09-30 19:11:12]

Sorry to not comment on the X-Macro's approach: my general tendency is to avoid
macros unless it absolutely makes things easier, and this is the general Chrome
style guide approach as well.

I think I side towards having a structured DownloadQuery object which is used,
and do the regexp's and filtering in C++. This would reduce the amount of data
which gets transmitted out to JS, and could be reusable by other functions.

It looks like the existing search function is blocking on the UI thread. Does
this ever have to hit the DB or is all data in-memory? Sorry for being lazy and
not reading myself.

[cbentzel, 2011-09-30 19:13:04]

On Fri, Sep 30, 2011 at 3:11 PM,  <cbentzel@chromium.org> wrote:
> Sorry to not comment on the X-Macro's approach: my general tendency is to
> avoid
> macros unless it absolutely makes things easier, and this is the general
> Chrome
> style guide approach as well.
>
> I think I side towards having a structured DownloadQuery object which is
> used,
> and do the regexp's and filtering in C++. This would reduce the amount of
> data
> which gets transmitted out to JS, and could be reusable by other functions.

I'd also move the DownloadQuery object and implementation outside of
the extension API. That way the extension API is a simple JSON
converter and validator.

>
> It looks like the existing search function is blocking on the UI thread.
> Does
> this ever have to hit the DB or is all data in-memory? Sorry for being lazy
> and
> not reading myself.
>
>
> http://codereview.chromium.org/7825035/
>

[benjhayden, 2011-09-30 19:23:03]

On 2011/09/30 19:13:04, cbentzel wrote:
> On Fri, Sep 30, 2011 at 3:11 PM,  <mailto:cbentzel@chromium.org> wrote:
> > Sorry to not comment on the X-Macro's approach: my general tendency is to
> > avoid
> > macros unless it absolutely makes things easier, and this is the general
> > Chrome
> > style guide approach as well.
> >
> > I think I side towards having a structured DownloadQuery object which is
> > used,
> > and do the regexp's and filtering in C++. This would reduce the amount of
> > data
> > which gets transmitted out to JS, and could be reusable by other functions.
> 
> I'd also move the DownloadQuery object and implementation outside of
> the extension API. That way the extension API is a simple JSON
> converter and validator.
> 
> >
> > It looks like the existing search function is blocking on the UI thread.
> > Does
> > this ever have to hit the DB or is all data in-memory? Sorry for being lazy
> > and
> > not reading myself.
> >
> >
> > http://codereview.chromium.org/7825035/
> >

All that SGTM. The only question left is to figure out whether to do it in
300LoC with x-macros or 1000LoC without. X-macros won't affect the run-time
performance at all, but they will make a significant impact on the line count.

[cbentzel, 2011-09-30 19:30:22]

That much of a line savings may make it worth it. Have you
investigated inline functions/templates as an alternative? I haven't
looked at the code in question to determine if that would be a help.

On Fri, Sep 30, 2011 at 3:23 PM,  <benjhayden@chromium.org> wrote:
> On 2011/09/30 19:13:04, cbentzel wrote:
>>
>> On Fri, Sep 30, 2011 at 3:11 PM,  <mailto:cbentzel@chromium.org> wrote:
>> > Sorry to not comment on the X-Macro's approach: my general tendency is
>> > to
>> > avoid
>> > macros unless it absolutely makes things easier, and this is the general
>> > Chrome
>> > style guide approach as well.
>> >
>> > I think I side towards having a structured DownloadQuery object which is
>> > used,
>> > and do the regexp's and filtering in C++. This would reduce the amount
>> > of
>> > data
>> > which gets transmitted out to JS, and could be reusable by other
>> > functions.
>
>> I'd also move the DownloadQuery object and implementation outside of
>> the extension API. That way the extension API is a simple JSON
>> converter and validator.
>
>> >
>> > It looks like the existing search function is blocking on the UI thread.
>> > Does
>> > this ever have to hit the DB or is all data in-memory? Sorry for being
>> > lazy
>> > and
>> > not reading myself.
>> >
>> >
>> > http://codereview.chromium.org/7825035/
>> >
>
> All that SGTM. The only question left is to figure out whether to do it in
> 300LoC with x-macros or 1000LoC without. X-macros won't affect the run-time
> performance at all, but they will make a significant impact on the line
> count.
>
> http://codereview.chromium.org/7825035/
>

[benjhayden, 2011-09-30 20:18:56]

I hadn't considered encapsulating each field in an object.

template<typename T>
class SearchField {
  SearchField(const char* name, Lambda<T(DownloadItem*)>* accessor);
  bool IsSet() const;
  void Set(T value);
  T value() const;
  bool Matches(DownloadItem* item) { return value() == accessor_(item); }
}

DownloadQuery::fields_ = {new SearchField("id", LAMBDA(item, item->id())),
new SearchField("url", LAMBDA(item, item->original_url().spec()))};

This seems to add significant code and runtime overhead in exchange for a
more explicit API that would be internal to DownloadQuery. I think that it
would also be at least as hairy as the x-macros. I'll think about it more
over the weekend.


On Fri, Sep 30, 2011 at 3:30 PM, Chris Bentzel <cbentzel@chromium.org>wrote:

> That much of a line savings may make it worth it. Have you
> investigated inline functions/templates as an alternative? I haven't
> looked at the code in question to determine if that would be a help.
>
> On Fri, Sep 30, 2011 at 3:23 PM,  <benjhayden@chromium.org> wrote:
> > On 2011/09/30 19:13:04, cbentzel wrote:
> >>
> >> On Fri, Sep 30, 2011 at 3:11 PM,  <mailto:cbentzel@chromium.org> wrote:
> >> > Sorry to not comment on the X-Macro's approach: my general tendency is
> >> > to
> >> > avoid
> >> > macros unless it absolutely makes things easier, and this is the
> general
> >> > Chrome
> >> > style guide approach as well.
> >> >
> >> > I think I side towards having a structured DownloadQuery object which
> is
> >> > used,
> >> > and do the regexp's and filtering in C++. This would reduce the amount
> >> > of
> >> > data
> >> > which gets transmitted out to JS, and could be reusable by other
> >> > functions.
> >
> >> I'd also move the DownloadQuery object and implementation outside of
> >> the extension API. That way the extension API is a simple JSON
> >> converter and validator.
> >
> >> >
> >> > It looks like the existing search function is blocking on the UI
> thread.
> >> > Does
> >> > this ever have to hit the DB or is all data in-memory? Sorry for being
> >> > lazy
> >> > and
> >> > not reading myself.
> >> >
> >> >
> >> > http://codereview.chromium.org/7825035/
> >> >
> >
> > All that SGTM. The only question left is to figure out whether to do it
> in
> > 300LoC with x-macros or 1000LoC without. X-macros won't affect the
> run-time
> > performance at all, but they will make a significant impact on the line
> > count.
> >
> > http://codereview.chromium.org/7825035/
> >
>

[benjhayden, 2011-10-06 21:23:13]

PTAL: x-macros!
The LoC savings aren't quite what I thought they'd be since there are only 6
x-includes, but they're still significant.


On 2011/09/30 20:18:56, b s h wrote:
> I hadn't considered encapsulating each field in an object.
> 
> template<typename T>
> class SearchField {
>   SearchField(const char* name, Lambda<T(DownloadItem*)>* accessor);
>   bool IsSet() const;
>   void Set(T value);
>   T value() const;
>   bool Matches(DownloadItem* item) { return value() == accessor_(item); }
> }
> 
> DownloadQuery::fields_ = {new SearchField("id", LAMBDA(item, item->id())),
> new SearchField("url", LAMBDA(item, item->original_url().spec()))};
> 
> This seems to add significant code and runtime overhead in exchange for a
> more explicit API that would be internal to DownloadQuery. I think that it
> would also be at least as hairy as the x-macros. I'll think about it more
> over the weekend.
> 
> 
> On Fri, Sep 30, 2011 at 3:30 PM, Chris Bentzel <cbentzel@chromium.org>wrote:
> 
> > That much of a line savings may make it worth it. Have you
> > investigated inline functions/templates as an alternative? I haven't
> > looked at the code in question to determine if that would be a help.
> >
> > On Fri, Sep 30, 2011 at 3:23 PM,  <mailto:benjhayden@chromium.org> wrote:
> > > On 2011/09/30 19:13:04, cbentzel wrote:
> > >>
> > >> On Fri, Sep 30, 2011 at 3:11 PM,  <mailto:cbentzel@chromium.org> wrote:
> > >> > Sorry to not comment on the X-Macro's approach: my general tendency is
> > >> > to
> > >> > avoid
> > >> > macros unless it absolutely makes things easier, and this is the
> > general
> > >> > Chrome
> > >> > style guide approach as well.
> > >> >
> > >> > I think I side towards having a structured DownloadQuery object which
> > is
> > >> > used,
> > >> > and do the regexp's and filtering in C++. This would reduce the amount
> > >> > of
> > >> > data
> > >> > which gets transmitted out to JS, and could be reusable by other
> > >> > functions.
> > >
> > >> I'd also move the DownloadQuery object and implementation outside of
> > >> the extension API. That way the extension API is a simple JSON
> > >> converter and validator.
> > >
> > >> >
> > >> > It looks like the existing search function is blocking on the UI
> > thread.
> > >> > Does
> > >> > this ever have to hit the DB or is all data in-memory? Sorry for being
> > >> > lazy
> > >> > and
> > >> > not reading myself.
> > >> >
> > >> >
> > >> > http://codereview.chromium.org/7825035/
> > >> >
> > >
> > > All that SGTM. The only question left is to figure out whether to do it
> > in
> > > 300LoC with x-macros or 1000LoC without. X-macros won't affect the
> > run-time
> > > performance at all, but they will make a significant impact on the line
> > > count.
> > >
> > > http://codereview.chromium.org/7825035/
> > >
> >

[cbentzel, 2011-10-10 14:35:30]

The x-macro stuff is clever and reduces code lines, so in that regards may be a
win.

An alternative approach would be to do something like:


class DownloadQueryField {
 public:
  virtual ~DownloadQueryField();
  virtual bool Matches(const SimpleDownloadItem& item) const = 0;
};

The DownloadQuery would then have something like:

map<std::string, DownloadQueryField*> query_fields_;

The upside of this approach is fewer macros, and it moves some of the logic for
things like has_filename_regex_ into the subclasses. It will probably be easier
to unit test each in isolation as well.

The downside is that key is determined at run-time rather than compile-time as
the macro approach does.

I also haven't thought about what the difficulties would be in constructing a
DownloadQuery object.

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
File content/browser/download/download_manager.cc (right):

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
content/browser/download/download_manager.cc:183: results->clear();
How big is this? Is downloads_ unbounded?

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
File content/browser/download/download_query.cc (right):

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
content/browser/download/download_query.cc:118: class DownloadSearch :
DownloadQuery {
Would it make more sense to have DownloadSearch own or reference a DownloadQuery
rather than be a derived class?

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
File content/browser/download/download_query.h (right):

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
content/browser/download/download_query.h:45: bool
Search(std::vector<DownloadItem*>* items,
The way this is used - it seems like it would make more sense to have a const
std::vector of input and std::vector of output, so not doing the sort/filter in
place. If there's only a few items which pass the DownloadQuery, you are doing
extraneous copies.

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
content/browser/download/download_query.h:50: DownloadQuery&
filter_func(FilterType func);
I haven't really seen this style in Chromium (the ref return on set to chain
setters). It also doesn't really seem to be all that useful - the case I saw
just called name() in a macro, without chaining.

It will also usually be set_filter_func rather that filter_func.

[Randy Smith (Not in Mondays), 2011-10-10 18:04:49]

On 2011/10/10 14:35:30, cbentzel wrote:
> The x-macro stuff is clever and reduces code lines, so in that regards may be
a
> win.
> 
> An alternative approach would be to do something like:
> 
> 
> class DownloadQueryField {
>  public:
>   virtual ~DownloadQueryField();
>   virtual bool Matches(const SimpleDownloadItem& item) const = 0;
> };
> 
> The DownloadQuery would then have something like:
> 
> map<std::string, DownloadQueryField*> query_fields_;
> 
> The upside of this approach is fewer macros, and it moves some of the logic
for
> things like has_filename_regex_ into the subclasses. It will probably be
easier
> to unit test each in isolation as well.

I'm not completely following.  Would the derived classes be defined by an
X-macro style?  Or would you just have subclasses for each type of matching you
needed to do, define them by hand, and maybe do something x-macro'y (or just
have a large case statement) to figure out what type of match class to
instantiate for each field?

> The downside is that key is determined at run-time rather than compile-time as
> the macro approach does.

FWIW, this doesn't bother me much.  I think our performance cost is going to be
doing the scan through downloads_, not in setup.

FWIW, I find the x-macro approach somewhat confusing, so I'd be interested in
exploring other options.  But I do think that the x-macro approach is worth it
for reduced #lines, if that's the only way to get the line number reduction.

> I also haven't thought about what the difficulties would be in constructing a
> DownloadQuery object.
> 
>
http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
> File content/browser/download/download_manager.cc (right):
> 
>
http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
> content/browser/download/download_manager.cc:183: results->clear();
> How big is this? Is downloads_ unbounded?
> 
>
http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
> File content/browser/download/download_query.cc (right):
> 
>
http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
> content/browser/download/download_query.cc:118: class DownloadSearch :
> DownloadQuery {
> Would it make more sense to have DownloadSearch own or reference a
DownloadQuery
> rather than be a derived class?
> 
>
http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
> File content/browser/download/download_query.h (right):
> 
>
http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
> content/browser/download/download_query.h:45: bool
> Search(std::vector<DownloadItem*>* items,
> The way this is used - it seems like it would make more sense to have a const
> std::vector of input and std::vector of output, so not doing the sort/filter
in
> place. If there's only a few items which pass the DownloadQuery, you are doing
> extraneous copies.
> 
>
http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
> content/browser/download/download_query.h:50: DownloadQuery&
> filter_func(FilterType func);
> I haven't really seen this style in Chromium (the ref return on set to chain
> setters). It also doesn't really seem to be all that useful - the case I saw
> just called name() in a macro, without chaining.
> 
> It will also usually be set_filter_func rather that filter_func.

[cbentzel, 2011-10-10 18:07:55]

On Mon, Oct 10, 2011 at 2:04 PM, <rdsmith@chromium.org> wrote:

> On 2011/10/10 14:35:30, cbentzel wrote:
>
>> The x-macro stuff is clever and reduces code lines, so in that regards may
>> be
>>
> a
>
>> win.
>>
>
>  An alternative approach would be to do something like:
>>
>
>
>  class DownloadQueryField {
>>  public:
>>   virtual ~DownloadQueryField();
>>   virtual bool Matches(const SimpleDownloadItem& item) const = 0;
>> };
>>
>
>  The DownloadQuery would then have something like:
>>
>
>  map<std::string, DownloadQueryField*> query_fields_;
>>
>
>  The upside of this approach is fewer macros, and it moves some of the
>> logic
>>
> for
>
>> things like has_filename_regex_ into the subclasses. It will probably be
>>
> easier
>
>> to unit test each in isolation as well.
>>
>
> I'm not completely following.  Would the derived classes be defined by an
> X-macro style?  Or would you just have subclasses for each type of matching
> you
> needed to do, define them by hand, and maybe do something x-macro'y (or
> just
> have a large case statement) to figure out what type of match class to
> instantiate for each field?


Not x-macro'y. I guess I was hoping for a few simple subclasses for match
integer which could be used - but they'd need to figure out which field to
extract from DownloadSimpleItem.


>
>  The downside is that key is determined at run-time rather than
>> compile-time as
>> the macro approach does.
>>
>
> FWIW, this doesn't bother me much.  I think our performance cost is going
> to be
> doing the scan through downloads_, not in setup.
>

Was more worried about correctness than performance.

>
> FWIW, I find the x-macro approach somewhat confusing, so I'd be interested
> in
> exploring other options.  But I do think that the x-macro approach is worth
> it
> for reduced #lines, if that's the only way to get the line number
> reduction.
>
>
>  I also haven't thought about what the difficulties would be in
>> constructing a
>> DownloadQuery object.
>>
>
>
> http://codereview.chromium.**org/7825035/diff/11001/**
>
content/browser/download/**download_manager.cc<http://codereview.chromium.org/7825035/diff/11001/content/browser/download/download_manager.cc>
>
>> File content/browser/download/**download_manager.cc (right):
>>
>
>
> http://codereview.chromium.**org/7825035/diff/11001/**
>
content/browser/download/**download_manager.cc#newcode183<http://codereview.chromium.org/7825035/diff/11001/content/browser/download/download_manager.cc#newcode183>
>
>> content/browser/download/**download_manager.cc:183: results->clear();
>> How big is this? Is downloads_ unbounded?
>>
>
>
> http://codereview.chromium.**org/7825035/diff/11001/**
>
content/browser/download/**download_query.cc<http://codereview.chromium.org/7825035/diff/11001/content/browser/download/download_query.cc>
>
>> File content/browser/download/**download_query.cc (right):
>>
>
>
> http://codereview.chromium.**org/7825035/diff/11001/**
>
content/browser/download/**download_query.cc#newcode118<http://codereview.chromium.org/7825035/diff/11001/content/browser/download/download_query.cc#newcode118>
>
>> content/browser/download/**download_query.cc:118: class DownloadSearch :
>> DownloadQuery {
>> Would it make more sense to have DownloadSearch own or reference a
>>
> DownloadQuery
>
>> rather than be a derived class?
>>
>
>
> http://codereview.chromium.**org/7825035/diff/11001/**
>
content/browser/download/**download_query.h<http://codereview.chromium.org/7825035/diff/11001/content/browser/download/download_query.h>
>
>> File content/browser/download/**download_query.h (right):
>>
>
>
> http://codereview.chromium.**org/7825035/diff/11001/**
>
content/browser/download/**download_query.h#newcode45<http://codereview.chromium.org/7825035/diff/11001/content/browser/download/download_query.h#newcode45>
>
>> content/browser/download/**download_query.h:45: bool
>> Search(std::vector<**DownloadItem*>* items,
>> The way this is used - it seems like it would make more sense to have a
>> const
>> std::vector of input and std::vector of output, so not doing the
>> sort/filter
>>
> in
>
>> place. If there's only a few items which pass the DownloadQuery, you are
>> doing
>> extraneous copies.
>>
>
>
> http://codereview.chromium.**org/7825035/diff/11001/**
>
content/browser/download/**download_query.h#newcode50<http://codereview.chromium.org/7825035/diff/11001/content/browser/download/download_query.h#newcode50>
>
>> content/browser/download/**download_query.h:50: DownloadQuery&
>> filter_func(FilterType func);
>> I haven't really seen this style in Chromium (the ref return on set to
>> chain
>> setters). It also doesn't really seem to be all that useful - the case I
>> saw
>> just called name() in a macro, without chaining.
>>
>
>  It will also usually be set_filter_func rather that filter_func.
>>
>
>
>
>
http://codereview.chromium.**org/7825035/<http://codereview.chromium.org/7825...
>

[Randy Smith (Not in Mondays), 2011-10-10 18:09:42]

A couple of high level comments.

http://codereview.chromium.org/7825035/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api.cc (right):

http://codereview.chromium.org/7825035/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:264: constants::kIdKey,
&get_id_));
What is the behavior if we have both an id and other things in the query from
the extension?

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
content/browser/download/download_manager.h:112: // |query| was mal-formed.
This interface makes me uncomfortable for two reasons: a) Having two output
vectors seems like overkill; I'd rather have the lower level (C++) one and then
translate it into json if needed, and b) returning a string error message at the
DownloadManager level feels wrong--I'd rather return an error code, and
translate it when we know we're actually sending the error message to the user. 

Looking at the current code, it doesn't look like there's a real performance
cost to fixing (a)--we do the translation inline now, but we could do it with a
second scan, and the cost of translating an element to json is going to much
higher than that of a single ++ in a DownloadVector.

[Aaron Boodman, 2011-10-12 08:26:08]

http://codereview.chromium.org/7825035/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api.cc (right):

http://codereview.chromium.org/7825035/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:276: #include
"content/browser/download/download_query_fields.h"
whoa.

http://codereview.chromium.org/7825035/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:290: query_, &error_,
NULL/*c++ results*/, results);
We avoid /* ... */ style comments in Chrome. If you want a param comment like
this we typically do:

Search(foo,
       bar, // blah blah
       baz);

http://codereview.chromium.org/7825035/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api_constants.h (right):

http://codereview.chromium.org/7825035/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api_constants.h:1: // Copyright
(c) 2011 The Chromium Authors. All rights reserved.
The separate constants files are no longer needed. You can put these in your
_api.cc file.

[cbentzel, 2011-10-12 09:26:37]

http://codereview.chromium.org/7825035/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api.cc (right):

http://codereview.chromium.org/7825035/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:276: #include
"content/browser/download/download_query_fields.h"
On 2011/10/12 08:26:08, Aaron Boodman wrote:
> whoa.

Ben is reimplementing the code in a way that won't require this.

[benjhayden, 2011-10-13 14:07:34]

PTAL

http://codereview.chromium.org/7825035/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api.cc (right):

http://codereview.chromium.org/7825035/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:264: constants::kIdKey,
&get_id_));
On 2011/10/10 18:09:42, rdsmith wrote:
> What is the behavior if we have both an id and other things in the query from
> the extension?

search() will only return items that match the query. If the item whose id is
specified in the query fails another filter in the query, then no items will be
returned. If item 42 is complete, then search({id:42, state:"in_progress"})
returns [].

http://codereview.chromium.org/7825035/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:276: #include
"content/browser/download/download_query_fields.h"
On 2011/10/12 08:26:08, Aaron Boodman wrote:
> whoa.

Done.

http://codereview.chromium.org/7825035/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:276: #include
"content/browser/download/download_query_fields.h"
On 2011/10/12 09:26:37, cbentzel wrote:
> On 2011/10/12 08:26:08, Aaron Boodman wrote:
> > whoa.
> 
> Ben is reimplementing the code in a way that won't require this. 

Done.

http://codereview.chromium.org/7825035/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:290: query_, &error_,
NULL/*c++ results*/, results);
On 2011/10/12 08:26:08, Aaron Boodman wrote:
> We avoid /* ... */ style comments in Chrome. If you want a param comment like
> this we typically do:
> 
> Search(foo,
>        bar, // blah blah
>        baz);

Done.

http://codereview.chromium.org/7825035/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api_constants.h (right):

http://codereview.chromium.org/7825035/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api_constants.h:1: // Copyright
(c) 2011 The Chromium Authors. All rights reserved.
On 2011/10/12 08:26:08, Aaron Boodman wrote:
> The separate constants files are no longer needed. You can put these in your
> _api.cc file.

Done.

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
File content/browser/download/download_manager.cc (right):

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
content/browser/download/download_manager.cc:183: results->clear();
On 2011/10/10 14:35:30, cbentzel wrote:
> How big is this? Is downloads_ unbounded?

AFAIK yes.

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
content/browser/download/download_manager.h:112: // |query| was mal-formed.
On 2011/10/10 18:09:42, rdsmith wrote:
> This interface makes me uncomfortable for two reasons: a) Having two output
> vectors seems like overkill; I'd rather have the lower level (C++) one and
then
> translate it into json if needed, and b) returning a string error message at
the
> DownloadManager level feels wrong--I'd rather return an error code, and
> translate it when we know we're actually sending the error message to the
user. 
> 
> Looking at the current code, it doesn't look like there's a real performance
> cost to fixing (a)--we do the translation inline now, but we could do it with
a
> second scan, and the cost of translating an element to json is going to much
> higher than that of a single ++ in a DownloadVector.

Done.

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
File content/browser/download/download_query.cc (right):

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
content/browser/download/download_query.cc:118: class DownloadSearch :
DownloadQuery {
On 2011/10/10 14:35:30, cbentzel wrote:
> Would it make more sense to have DownloadSearch own or reference a
DownloadQuery
> rather than be a derived class?

The Field classes allowed me to merge DSearch into DQuery.

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
File content/browser/download/download_query.h (right):

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
content/browser/download/download_query.h:45: bool
Search(std::vector<DownloadItem*>* items,
On 2011/10/10 14:35:30, cbentzel wrote:
> The way this is used - it seems like it would make more sense to have a const
> std::vector of input and std::vector of output, so not doing the sort/filter
in
> place. If there's only a few items which pass the DownloadQuery, you are doing
> extraneous copies.

Done.

http://codereview.chromium.org/7825035/diff/11001/content/browser/download/do...
content/browser/download/download_query.h:50: DownloadQuery&
filter_func(FilterType func);
On 2011/10/10 14:35:30, cbentzel wrote:
> I haven't really seen this style in Chromium (the ref return on set to chain
> setters). It also doesn't really seem to be all that useful - the case I saw
> just called name() in a macro, without chaining.
> 
> It will also usually be set_filter_func rather that filter_func.

Done.

[cbentzel, 2011-10-13 15:22:23]

I haven't done a thorough review here, but mainly wanted to send a quick
response that I am happy with how DownloadQuery is implemented now.

The macros are pretty small and less-hairy now, and if necessary we could remove
them for a little bit more boilerplate by explicitly authoring each derived
class.

I'm not sure if there's a way to avoid the reinterpret_cast [which might be OK
as a static_cast?] during the Set.

I definitely think you should write unit tests for the DownloadQuery, and with
the API it has it should be somewhat straightforward.

http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
File content/browser/download/download_query.cc (right):

http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
content/browser/download/download_query.cc:241: #define SORT_FIELD(name, type,
expr) \
I'd add a helper function called SetSortField(name, SortFieldInterface*), call
it in this macro, and have it DCHECK that name is not already contained inside
of sort_fields_. Ditto for filter.

http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
content/browser/download/download_query.cc:307: FilterFieldInterface*
field_interface = filter_fields_[name];
Use find here - this will add name to the map if it doesn't already exist.

http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
File content/browser/download/download_query.h (right):

http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
content/browser/download/download_query.h:60: // Ignore these mostly-private
interfaces.
Why aren't these private?

http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
content/browser/download/download_query.h:72: STLValueDeleter<SortFields>
delete_sort_fields_;
Are the delete_sort_fields_, delete_filter_fields_ used at all?

http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
content/browser/download/download_query.h:80: // The following macros facilitate
using anonymous expressions (lambdas) to
Are these needed anymore?

[benjhayden, 2011-10-13 17:08:56]

On 2011/10/13 15:22:23, cbentzel wrote:
> I haven't done a thorough review here, but mainly wanted to send a quick
> response that I am happy with how DownloadQuery is implemented now.
> 
> The macros are pretty small and less-hairy now, and if necessary we could
remove
> them for a little bit more boilerplate by explicitly authoring each derived
> class.

22*7 + 13*7 = 245 more lines of boilerplate, plus whitespace, plus making it
7-14 times harder to add fields.

> 
> I'm not sure if there's a way to avoid the reinterpret_cast [which might be OK
> as a static_cast?] during the Set.

C++ doesn't support virtual template methods.
I could overload FilterFieldInterface::Set() with all the possible ValueTypes
and make them all return false by default, but that would not be very DRY. I
could use an x-macro for the ValueTypes... :-)
static_cast does seem to work here.

> 
> I definitely think you should write unit tests for the DownloadQuery, and with
> the API it has it should be somewhat straightforward.

I will definitely do that in this CL. I still want to see if Randy likes this
approach or has any other ideas that might change the fundamental structure of
this cl.

> 
>
http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
> File content/browser/download/download_query.cc (right):
> 
>
http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
> content/browser/download/download_query.cc:241: #define SORT_FIELD(name, type,
> expr) \
> I'd add a helper function called SetSortField(name, SortFieldInterface*), call
> it in this macro, and have it DCHECK that name is not already contained inside
> of sort_fields_. Ditto for filter.
> 
>
http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
> content/browser/download/download_query.cc:307: FilterFieldInterface*
> field_interface = filter_fields_[name];
> Use find here - this will add name to the map if it doesn't already exist.
> 
>
http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
> File content/browser/download/download_query.h (right):
> 
>
http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
> content/browser/download/download_query.h:60: // Ignore these mostly-private
> interfaces.
> Why aren't these private?
> 
>
http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
> content/browser/download/download_query.h:72: STLValueDeleter<SortFields>
> delete_sort_fields_;
> Are the delete_sort_fields_, delete_filter_fields_ used at all?
> 
>
http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
> content/browser/download/download_query.h:80: // The following macros
facilitate
> using anonymous expressions (lambdas) to
> Are these needed anymore?

[benjhayden, 2011-10-13 17:19:47]

PTAL

http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
File content/browser/download/download_query.cc (right):

http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
content/browser/download/download_query.cc:241: #define SORT_FIELD(name, type,
expr) \
On 2011/10/13 15:22:24, cbentzel wrote:
> I'd add a helper function called SetSortField(name, SortFieldInterface*), call
> it in this macro, and have it DCHECK that name is not already contained inside
> of sort_fields_. Ditto for filter. 

Done.

http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
content/browser/download/download_query.cc:307: FilterFieldInterface*
field_interface = filter_fields_[name];
On 2011/10/13 15:22:24, cbentzel wrote:
> Use find here - this will add name to the map if it doesn't already exist.

Done.

http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
File content/browser/download/download_query.h (right):

http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
content/browser/download/download_query.h:60: // Ignore these mostly-private
interfaces.
On 2011/10/13 15:22:24, cbentzel wrote:
> Why aren't these private? 

DownloadComparator, FilterField<>, and SortField<> need them.

http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
content/browser/download/download_query.h:72: STLValueDeleter<SortFields>
delete_sort_fields_;
On 2011/10/13 15:22:24, cbentzel wrote:
> Are the delete_sort_fields_, delete_filter_fields_ used at all?

Yes. DQ's ctor sets them up with a pointer to sort_fields_ and filter_fields_
respectively. They automatically call STLDeleteValues when the DQ goes out of
scope. They're "magic" like scoped_ptr.

http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
content/browser/download/download_query.h:80: // The following macros facilitate
using anonymous expressions (lambdas) to
On 2011/10/13 15:22:24, cbentzel wrote:
> Are these needed anymore? 

They're still as necessary as they were. If this approach LGTY, I'll use them to
rewrite all 4 of the existing search methods in DownloadManager, which can't
quite be implemented without a filter Callback. I could implement those filter
callbacks without these macros, but the filter implementations would be somewhat
far away from their use, and less readable imho. These macros aren't strictly
necessary, but I think they're nice to have.

[Randy Smith (Not in Mondays), 2011-10-13 23:23:10]

I like this general pathway, but I do have one high level question.  It seems to
me as if the generic sort infrastructure (download_query*) is more targeted at
the extension usage than at programatic usage, in that it generally takes
strings to identify fields.  Part of me feels like fields should be identified
by constants/enums rather than strings at a level that's targeted at
programmatic usage, and the extension system should translate.  I don't feel
strongly about this--I think the extension system  is going to be the primary
customer for the queries--but using constants/enums make's it a bit easier to
not accidentally do the wrong thing when calling from a programatic context. 
(It's a bit better performance wise too, but I'm not sure how important that
is.)  WDYT?

The stuff below is just random stuff I noticed as I was reading through for a
high level overview--it should in no way be taken as a fine grained reading of
the CL.  Given that I like the basic approach, are you ready for a fine-grained
reading?

http://codereview.chromium.org/7825035/diff/36008/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api.cc (right):

http://codereview.chromium.org/7825035/diff/36008/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:142: item->safety_state()
== DownloadItem::DANGEROUS_BUT_VALIDATED);
If it's not dangerous, should this value be set at all?  I'd naively think it
not being present in the dictionary would make more sense.  (Happy to go with
your preference, just raising the issue in case you hadn't thought about it.)

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
File content/browser/download/download_manager.cc (right):

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
content/browser/download/download_manager.cc:186: query.Search(all_items,
results);
Potentially tacky, but may be worth considering: Can you just pass a generic
iterator of some sort to Search to avoid having to copy all the DownloadItems?

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
File content/browser/download/download_query.cc (right):

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
content/browser/download/download_query.cc:37: // right, or 1 when left < right.
That seems backward from the usual custom (e.g. qsort).  Why do it that way?

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
File content/browser/download/download_query.h (right):

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
content/browser/download/download_query.h:86: // even lvalues.
What's the reason for this restriction?  Is there any way to lift it?  It seems
like an uncomfortable restriction on callers.

[benjhayden, 2011-10-14 19:31:02]

Before the nitty gritty, WDYT of the DownloadItemInterface for testing?

http://codereview.chromium.org/7825035/diff/36008/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api.cc (right):

http://codereview.chromium.org/7825035/diff/36008/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:142: item->safety_state()
== DownloadItem::DANGEROUS_BUT_VALIDATED);
On 2011/10/13 23:23:11, rdsmith wrote:
> If it's not dangerous, should this value be set at all?  I'd naively think it
> not being present in the dictionary would make more sense.  (Happy to go with
> your preference, just raising the issue in case you hadn't thought about it.)

I hadn't thought about it, that makes sense.

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
File content/browser/download/download_manager.cc (right):

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
content/browser/download/download_manager.cc:186: query.Search(all_items,
results);
On 2011/10/13 23:23:11, rdsmith wrote:
> Potentially tacky, but may be worth considering: Can you just pass a generic
> iterator of some sort to Search to avoid having to copy all the DownloadItems?

Done.

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
File content/browser/download/download_query.cc (right):

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
content/browser/download/download_query.cc:37: // right, or 1 when left < right.
On 2011/10/13 23:23:11, rdsmith wrote:
> That seems backward from the usual custom (e.g. qsort).  Why do it that way?

Ah, I meant to check the convention. Done.

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
File content/browser/download/download_query.h (right):

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
content/browser/download/download_query.h:86: // even lvalues.
On 2011/10/13 23:23:11, rdsmith wrote:
> What's the reason for this restriction?  Is there any way to lift it?  It
seems
> like an uncomfortable restriction on callers.

The reason is that the macro implementation uses the variable names as both
arguments to base::Bind() and method parameter names. There are a few ways to
lift it:
0. Require the caller to specify both parameter names and values: DQF2(name0,
value0()[], name1, value1()[], item.foo() == name0 && item.bar() == name1)
1. Always use the same parameter names, allow the user to only specify values:
DQF2(value0()[], value1()[], item.foo() == param0 && item.bar() == param1)
2. Use python and its lambda and keyword args. :-)
query.filter = lambda item, name0=otr(): item.otr() == name0

Since some parts of chrome use named variables for function parameters for
clarity instead of using "param/*meaning*/,", I thought that DM's Search methods
could do the same:
bool otr = otr();
query.Set("filter", DQF1(otr, item.otr() == otr));

[benjhayden, 2011-10-18 19:11:09]

+Andy

[Randy Smith (Not in Mondays), 2011-10-18 23:58:45]

Here's another round of comments with more detail.  The primary top-level thing
to think about is how to document this so that it's as understandable as
possible to someone coming upon it for the first time.  Willing to give that a
try?

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
File content/browser/download/download_query.cc (right):

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
content/browser/download/download_query.cc:82:
sort_fields_.find(order_term_value);
I wince when I see this.  Performance isn't vital for this functionality (I hope
:-}) but this adds another factor of n in the midst of your n log n operation. 
Is there any way to arrange things such that when we get to doing the
comparison, we're just chugging on down a list of sort key objects, each one of
which we can call Compare() on?

(I only have vague ideas in this direction, but I could imagine template methods
that are templated on the field enum, or static (lazily created) callback
instances that are looked up by field enum and then pushed on the list.)

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
content/browser/download/download_query.cc:145: explicit SortField(const char*
name) : name_(name) {}
What's the purpose for having the name field on these interfaces?  The various
methods need to know which field their getting/comparing by some other method,
and we're switching over to defining fields by enums.

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
content/browser/download/download_query.cc:167: base::SplitString(value, L' ',
&terms_);
This feels like more smarts than I'd expect at this level--what's the use case? 
Is it reasonable to layer this case on top of this class, rather than inside it?

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
content/browser/download/download_query.cc:253: #define SORT_FIELD(name, type,
expr) \
It took me a couple of read throughs through these macros to follow what was
going on, and one issue was remembering which things were macros arguments as I
read.  Would it make sense to you to call out the macro arguments by (say)
having them have an underscore prepended to them?  (Or something else--I'm not
wedded to the underscore.)

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
content/browser/download/download_query.cc:370:
DownloadComparator(order_by_fields_, sort_fields_));
partial sort, so we don't take quite as big a hit where the results of the
matches will be large, but we're limiting?

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
File content/browser/download/download_query.h (right):

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
content/browser/download/download_query.h:22: // CHECK(query.Set("startTime",
0));
Is this the ideal name for it?  "Set" is a generic verb, and doesn't in and of
itself say "and we'll be filtering for DownloadItems where the field matches
this value.

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
content/browser/download/download_query.h:26: // CHECK(query.Set("filter",
base::Bind(&FilterOutOddDownloads)));
I'd be inclined to make filter a separate method; it's doing something
noticeably different from Set.  Set looks layered on top of filter, but (for
instnace) it makes sense to call filter with several different values, and each
new set value will presumably just override the old one.

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
content/browser/download/download_query.h:48: bool OrderBy(const std::string&
order_by);
So one of my thoughts about suggesting that we go with field enums rather than
names was making this more programmatic.  I could imagine something like Set in
that some method takes a specific field name and a forward/reverse boolean, and
that specifies the nth sort key.  This means you can't reset the DownoadQuery,
but I think that's fine; these'll be constructed pretty much on the fly anyway.

http://codereview.chromium.org/7825035/diff/36008/content/browser/download/do...
content/browser/download/download_query.h:86: // even lvalues.
On 2011/10/14 19:31:02, b s h wrote:
> On 2011/10/13 23:23:11, rdsmith wrote:
> > What's the reason for this restriction?  Is there any way to lift it?  It
> seems
> > like an uncomfortable restriction on callers.
> 
> The reason is that the macro implementation uses the variable names as both
> arguments to base::Bind() and method parameter names. There are a few ways to
> lift it:
> 0. Require the caller to specify both parameter names and values: DQF2(name0,
> value0()[], name1, value1()[], item.foo() == name0 && item.bar() == name1)
> 1. Always use the same parameter names, allow the user to only specify values:
> DQF2(value0()[], value1()[], item.foo() == param0 && item.bar() == param1)
> 2. Use python and its lambda and keyword args. :-)
> query.filter = lambda item, name0=otr(): item.otr() == name0
> 
> Since some parts of chrome use named variables for function parameters for
> clarity instead of using "param/*meaning*/,", I thought that DM's Search
methods
> could do the same:
> bool otr = otr();
> query.Set("filter", DQF1(otr, item.otr() == otr));

Hmmm.  All reasonable options, but I find myself wondering if the macro magic
(here) is worth it.  If the user wants this functionality, they can create a
class with a method and use base::Bind themselves; it requires them to
understand how to use base::Bind, but I don't think the cost in terms of lines
of code is that extreme.  And macros bad :-}.  Maybe just put a couple of lines
of example code near query.Set("filter"...) doc to show people how they should
be thinking if they want to do this?

[cbentzel, 2011-10-20 18:18:53]

I like that more of the parsing/validation is happening in the extension
functions instead of in the DownloadQuery. 

Still concerned about the number of macros and templates but it's starting to
wear off.

http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
File content/browser/download/download_query.h (right):

http://codereview.chromium.org/7825035/diff/34001/content/browser/download/do...
content/browser/download/download_query.h:80: // The following macros facilitate
using anonymous expressions (lambdas) to
On 2011/10/13 17:19:47, b s h wrote:
> On 2011/10/13 15:22:24, cbentzel wrote:
> > Are these needed anymore? 
> 
> They're still as necessary as they were. If this approach LGTY, I'll use them
to
> rewrite all 4 of the existing search methods in DownloadManager, which can't
> quite be implemented without a filter Callback. I could implement those filter
> callbacks without these macros, but the filter implementations would be
somewhat
> far away from their use, and less readable imho. These macros aren't strictly
> necessary, but I think they're nice to have.

Perhaps I'm blind, but I don't see them being used in this CL.

http://codereview.chromium.org/7825035/diff/44001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api.cc (right):

http://codereview.chromium.org/7825035/diff/44001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:113: case
DownloadItem::REMOVING:  // fall through
Is this an intentional fall through here? It may get NOTREACHED.

http://codereview.chromium.org/7825035/diff/44001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:169: bool
GetJsonValue(base::DictionaryValue* json, const char* name, int* value) {
This seems like a good use of template specialization.

http://codereview.chromium.org/7825035/diff/44001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:413: bool
DownloadsSearchFunction::Parse(
I think this could be a non-member function if it changed to 

template <typename ValueType>
std::string ParseQueryField(DownloadQuery* query,  
                            base::DictionaryValue* json,
                            const char* json_name,
                            DownloadQuery::FilterFieldName)

where the return value is an error string, and empty indicates no error.

That will help make it so you can do a forward declare in the header, and remove
the declaration from the header.

http://codereview.chromium.org/7825035/diff/44001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api.h (right):

http://codereview.chromium.org/7825035/diff/44001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.h:133: class
DownloadsSearchFunction : public SyncDownloadsFunction {
Should this be an AsyncDownloadsFunction function? Do we know how long this is
going to take? I'd worry about this blocking the UI thread, although I suppose
it should be only accessing the in-memory representation instead of hitting the
db/disk.

http://codereview.chromium.org/7825035/diff/44001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.h:140: virtual bool
ParseArgs() OVERRIDE;
For testing purposes, it might be nice to have a 

bool ParseArgsIntoQuery(DownloadQuery* query) 

function to validate that the parser works correctly. 

ParseArgs could just be a simple wrapper around that.

http://codereview.chromium.org/7825035/diff/44001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.h:148:
download_util::DownloadQuery query_;
Could you make this a scoped_ptr so you can just forward declare DownloadQuery?
It does mean that FilterFieldName would need to move out into a separate
header/class, although I think that Parse could be done separately.

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
content/browser/download/download_item.h:47: class CONTENT_EXPORT DownloadItem :
public DownloadItemInterface {
Naming issue: we usually don't have Interface appended to interfaces. More
commonly it's something like

class DownloadItemImpl : public DownloadItem {
};

Another advantage of this change is that most of the consumers of DownloadItem
only care about the interface, so you won't have to change them.

It also might be a good idea to do this very simple refactor in a separate CL.

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
File content/browser/download/download_item_interface.h (right):

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
content/browser/download/download_item_interface.h:68: virtual
~DownloadItemInterface() {}
Comments for these methods should move from DownloadItem to
DownloadItemInterface.

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
File content/browser/download/download_manager.cc (right):

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
content/browser/download/download_manager.cc:836: DownloadItem* download = new
DownloadItem(this, entries->at(i), -1 - i);
Why -1 - i?

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
content/browser/download/download_manager.h:66: }  // namespace content
Nit: end namespaces not needed for forward declaration cases.

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
File content/browser/download/download_query.cc (right):

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
content/browser/download/download_query.cc:328: #define INSTANTIATE_SET(type) \
Dumb question: Why do you need to do the instantiation here? Isn't this handled
at the call sites?

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
File content/browser/download/download_query.h (right):

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
content/browser/download/download_query.h:34: typedef std::vector<DownloadItem*>
DownloadItems;
Should this be DownloadItemInterface?

[asanka, 2011-10-20 19:29:16]

http://codereview.chromium.org/7825035/diff/44001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api.cc (right):

http://codereview.chromium.org/7825035/diff/44001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:456: if
(query_json->HasKey(constants::kOrderByKey)) {
The extensions API doc should be updated to mention that the 'orderBy' value
could be a space separated list.

http://codereview.chromium.org/7825035/diff/44001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api.h (right):

http://codereview.chromium.org/7825035/diff/44001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.h:18: #include
"content/browser/download/download_query.h"
Nit: ordering.

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
File content/browser/download/download_query.cc (right):

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
content/browser/download/download_query.cc:86: static int get_start_time(const
DownloadItem& item) {
Nit: These aren't trivial accessors.

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
content/browser/download/download_query.cc:281: // enumerated in INSTANTIATE_SET
below.
Have you considered lazy initialization for these, or would that make things
much more complicated?  Lazy initialization would let you have a set of
FilterFields and SortFields that contain the minimal set of objects necessary
for this query.

In addition, you can remove the lookup in DownloadComparator and remove
is_set().

I suspect that only a few of these fields would be used for any single query.

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
content/browser/download/download_query.cc:325:
static_cast<FilterField<ValueType>*>(iter->second)->Set(value);
Would this do the correct thing for something like
query_.Set(SORT_FIELD_FILENAME, 1u)?  It should return false without changing
the query.

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
File content/browser/download/download_query.h (right):

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
content/browser/download/download_query.h:23: // CHECK(query.Set("startTime",
0));
Nit: Field names aren't strings.

http://codereview.chromium.org/7825035/diff/44001/content/browser/download/do...
content/browser/download/download_query.h:144: // query.Set("filter",
DOWNLOAD_QUERY_FILTER2(otr, foo,
Nit: Field name isn't a string.

[Aaron Boodman, 2011-10-24 05:07:47]

Can you please put this in chrome/browser/download/downloads_extension_api.*

Also: You don't need the separate constants file. That precedent was set ages
ago because of a certain Chrome project that needed it. That project no longer
needs it, so we can stop.