Kill DownloadItem::IsOtr()

Kill DownloadItem::IsOtr()
Consumers should just remember which DownloadManager they got the item from, or else lookup the item from both managers and rely on the id-space sharing.
Also move CreateDownloadItemValue from download_util to download_dom_handler.

Reviewers:
asanka: *
Done:
jam: chrome/browser/automation/*
jhawkins: downloads_dom_handler.cc
rdsmith: *

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151504

[benjhayden, 2012-07-20 16:39:47]

PTAL
This seemed easy, so I'm sure I missed something crucial.

[Randy Smith (Not in Mondays), 2012-07-23 18:19:26]

High level questions I'd like to do a round on before doing a detailed review:

* It looks to me at casual reading that we're initialization incognito download
managers with history entries from regular download managers.  Could you
convince yourself (and then me) that this isn't the case (the code's squirrelly,
so it may well not be) or fix it if it is the case?  (Behavioral concern is that
you'll end up getting regular downloads from incognito download manager and
marking them incognito in the UI, but that behavior, if accurate, is broken
aesthetically too :-}.)

* What is the current behavior of the download shelf across the
incognito/regular border?  I.e. if you start an incognito download, does that
show up on a regular shelf (answer should be "no" :-}), and if you start a
regulard download, does that show up on an incognito shelf (I'm not certain I
know what the answer should be).  We should either be careful not to change
behavior, or make sure that we're clear that a change in behavior is better in
some fashion.

http://codereview.chromium.org/10805020/diff/1011/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/10805020/diff/1011/chrome/browser/automation/a...
chrome/browser/automation/automation_provider.cc:329:
dl_item_value->SetBoolean("is_otr", incognito);
Does this interface make sense, or would it make more sense to project upwards
to consumers of this interface what download manager/profile the download item
comes from?  I haven't looked at the consumer requirements, but having an
argument that's passed in to directly tweak an output value feels wrong at the
producer level--the work of this routine is converting DownloadItem into a
different form.

http://codereview.chromium.org/10805020/diff/1011/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10805020/diff/1011/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:417: }
Why not in DownloadHistory?  That feels like a class that's specialized for
dealing with Download<->History interactions, whereas
ChromeDownloadManagerDelegate is a grab bad; if the functionality fits in a more
specific context, that's probably the better place for it?

I could imagine either code testing the profile otr status in the download
history, or a different (simple) implementation of download history for OTR
contexts (though long term we probably need to share state between otr & non-otr
download history contexts as I think sometimes non-otr contexts allocate fake DB
handles, and we want to avoid collision?)

http://codereview.chromium.org/10805020/diff/1011/chrome/browser/extensions/a...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/10805020/diff/1011/chrome/browser/extensions/a...
chrome/browser/extensions/api/downloads/downloads_api.cc:593: DCHECK(on_record ^
off_record);
Just noting that this is subtly scary.  I went on a standards hunt, and it does
what you'd expect, but only because the explicit integer promotion from bool is
(false,true)->(0,1), which my memory is wasn't always the case.  It's personal
preference (not even a suggestion) but I would probably skip creating the extra
variables, pass in the def of off_record into the DownloadItemToJSON call, and
make the DCHECK more complicated (possibly split it into two separate DCHECKs)
but use logical operators rather than bitwise ones.  But that's a personal style
preference.

[benjhayden, 2012-07-23 20:33:50]

On 2012/07/23 18:19:26, rdsmith wrote:
> High level questions I'd like to do a round on before doing a detailed review:
> 
> * It looks to me at casual reading that we're initialization incognito
download
> managers with history entries from regular download managers.  Could you
> convince yourself (and then me) that this isn't the case (the code's
squirrelly,
> so it may well not be) or fix it if it is the case?  (Behavioral concern is
that
> you'll end up getting regular downloads from incognito download manager and
> marking them incognito in the UI, but that behavior, if accurate, is broken
> aesthetically too :-}.)
> 
> * What is the current behavior of the download shelf across the
> incognito/regular border?  I.e. if you start an incognito download, does that
> show up on a regular shelf (answer should be "no" :-}), and if you start a
> regulard download, does that show up on an incognito shelf (I'm not certain I
> know what the answer should be).  We should either be careful not to change
> behavior, or make sure that we're clear that a change in behavior is better in
> some fashion.

I was hoping I wouldn't need to make the otr manager not take items from the
history in this cl, but it looks like there's no other way.
Hold the review.

> 
>
http://codereview.chromium.org/10805020/diff/1011/chrome/browser/automation/a...
> File chrome/browser/automation/automation_provider.cc (right):
> 
>
http://codereview.chromium.org/10805020/diff/1011/chrome/browser/automation/a...
> chrome/browser/automation/automation_provider.cc:329:
> dl_item_value->SetBoolean("is_otr", incognito);
> Does this interface make sense, or would it make more sense to project upwards
> to consumers of this interface what download manager/profile the download item
> comes from?  I haven't looked at the consumer requirements, but having an
> argument that's passed in to directly tweak an output value feels wrong at the
> producer level--the work of this routine is converting DownloadItem into a
> different form.
> 
>
http://codereview.chromium.org/10805020/diff/1011/chrome/browser/download/chr...
> File chrome/browser/download/chrome_download_manager_delegate.cc (right):
> 
>
http://codereview.chromium.org/10805020/diff/1011/chrome/browser/download/chr...
> chrome/browser/download/chrome_download_manager_delegate.cc:417: }
> Why not in DownloadHistory?  That feels like a class that's specialized for
> dealing with Download<->History interactions, whereas
> ChromeDownloadManagerDelegate is a grab bad; if the functionality fits in a
more
> specific context, that's probably the better place for it?
> 
> I could imagine either code testing the profile otr status in the download
> history, or a different (simple) implementation of download history for OTR
> contexts (though long term we probably need to share state between otr &
non-otr
> download history contexts as I think sometimes non-otr contexts allocate fake
DB
> handles, and we want to avoid collision?)
> 
>
http://codereview.chromium.org/10805020/diff/1011/chrome/browser/extensions/a...
> File chrome/browser/extensions/api/downloads/downloads_api.cc (right):
> 
>
http://codereview.chromium.org/10805020/diff/1011/chrome/browser/extensions/a...
> chrome/browser/extensions/api/downloads/downloads_api.cc:593: DCHECK(on_record
^
> off_record);
> Just noting that this is subtly scary.  I went on a standards hunt, and it
does
> what you'd expect, but only because the explicit integer promotion from bool
is
> (false,true)->(0,1), which my memory is wasn't always the case.  It's personal
> preference (not even a suggestion) but I would probably skip creating the
extra
> variables, pass in the def of off_record into the DownloadItemToJSON call, and
> make the DCHECK more complicated (possibly split it into two separate DCHECKs)
> but use logical operators rather than bitwise ones.  But that's a personal
style
> preference.

[benjhayden, 2012-07-25 20:59:58]

PTAL

http://codereview.chromium.org/10805020/diff/1011/chrome/browser/extensions/a...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/10805020/diff/1011/chrome/browser/extensions/a...
chrome/browser/extensions/api/downloads/downloads_api.cc:593: DCHECK(on_record ^
off_record);
On 2012/07/23 18:19:26, rdsmith wrote:
> Just noting that this is subtly scary.  I went on a standards hunt, and it
does
> what you'd expect, but only because the explicit integer promotion from bool
is
> (false,true)->(0,1), which my memory is wasn't always the case.  It's personal
> preference (not even a suggestion) but I would probably skip creating the
extra
> variables, pass in the def of off_record into the DownloadItemToJSON call, and
> make the DCHECK more complicated (possibly split it into two separate DCHECKs)
> but use logical operators rather than bitwise ones.  But that's a personal
style
> preference.

Done.

[asanka, 2012-08-01 17:11:35]

http://codereview.chromium.org/10805020/diff/29001/chrome/browser/automation/...
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/10805020/diff/29001/chrome/browser/automation/...
chrome/browser/automation/automation_provider.cc:331: bool incognito =
profile->HasOffTheRecordProfile() &&
Is it possible for |profile| here to be the original profile while |download|
belongs to the off-the-record profile?

http://codereview.chromium.org/10805020/diff/29001/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10805020/diff/29001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:430: (profile_ ==
profile_->GetOffTheRecordProfile())) {
Why not profile_->IsOffTheRecord() ?

[benjhayden, 2012-08-01 18:51:55]

Thanks! PTAL.

http://codereview.chromium.org/10805020/diff/29001/chrome/browser/automation/...
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/10805020/diff/29001/chrome/browser/automation/...
chrome/browser/automation/automation_provider.cc:331: bool incognito =
profile->HasOffTheRecordProfile() &&
On 2012/08/01 17:11:35, asanka wrote:
> Is it possible for |profile| here to be the original profile while |download|
> belongs to the off-the-record profile?

I'm not sure, which is effectively the same as "yes" since the question is of
possibility. There are some places in the code that handle on-record profile
objects and off-record downloads because the surrounding context is off-record.
I'm not sure if this is one of those places. Either way, it doesn't seem like
it's this routine's job to enforce incognito.

http://codereview.chromium.org/10805020/diff/29001/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10805020/diff/29001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:430: (profile_ ==
profile_->GetOffTheRecordProfile())) {
On 2012/08/01 17:11:35, asanka wrote:
> Why not profile_->IsOffTheRecord() ?

Done.

[Randy Smith (Not in Mondays), 2012-08-02 17:52:17]

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/automation/...
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/automation/...
chrome/browser/automation/automation_provider.h:160: Profile* profile);
If you did this in response to my earlier comments, I'd like to apologize and
repent.  I think the code will be simpler (and lower level objects won't need to
know about Profiles) if you just pass around a boolean as to whether the
download item is incognito or not.

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/automation/...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/automation/...
chrome/browser/automation/automation_provider_observers.h:1096: Profile*
profile);
It looks like there's an observer created per download item it's needed for, so
it can be created with just the boolean as to whether or not the download item
is incognito.

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:135: if
(profile_->GetOriginalProfile() == profile_) {
Did you consider doing this at the DownloadHistory level?  What was the
arguments for conditionals in CDMD rather than a null DownloadHistory
implementation?

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:138:
download_manager_));
I'm somewhat uncomfortable here.  I'll agree in advance with your (presumed :-})
argument that the lifetime guarantees for the history loading chain are not at
all clear, and thus that we're risking some future change resulting in a use
after free error.  But the lifetime guarantees are (I believe) there--I think
there's a cancellation between DownloadHistory and the DB thread to guarantee
that callbacks don't come back if download_history_ goes away (there at least
used to be).  And I think it's confusing to have multiple code that solves the
same problem.  So I'd rather cleanup the entire stack than just change this
point. 

An additional point is that I'd like to be moving towards fewer classes being
refcounted, and this relies on CDMD being refcounted.

Could you put in a TODO?  Either in place of this change ('lifetime guarantees
confusing; cleanup') or in addition to this change ('duplicates the effect of XX
in YY but is clearer in its effect; cleanup to have a single clear mechanism')?

(Applies throughout file for this change.)

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:625:
json_results->Append(json_item.release());
I wince a little bit at RunDownloadQuery having the incognito/non-incognito
information and throwing it away so that it has to be regenerated here.  It
looks like this is the only place RunDownloadQuery() is called from.  Maybe
refactor so that RunDownloadQuery creates the json so that you don't need to
regenerate the incognito info?

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:887: item,
incognito_));
Won't this produce the wrong result when we're dispatching a regular download to
an incognito profile?

[benjhayden, 2012-08-13 15:08:09]

PTAL

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/automation/...
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/automation/...
chrome/browser/automation/automation_provider.h:160: Profile* profile);
On 2012/08/02 17:52:17, rdsmith wrote:
> If you did this in response to my earlier comments, I'd like to apologize and
> repent.  I think the code will be simpler (and lower level objects won't need
to
> know about Profiles) if you just pass around a boolean as to whether the
> download item is incognito or not.

Done.

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/automation/...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/automation/...
chrome/browser/automation/automation_provider_observers.h:1096: Profile*
profile);
On 2012/08/02 17:52:17, rdsmith wrote:
> It looks like there's an observer created per download item it's needed for,
so
> it can be created with just the boolean as to whether or not the download item
> is incognito.

Done.

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:135: if
(profile_->GetOriginalProfile() == profile_) {
On 2012/08/02 17:52:17, rdsmith wrote:
> Did you consider doing this at the DownloadHistory level?  What was the
> arguments for conditionals in CDMD rather than a null DownloadHistory
> implementation?

I think we agreed to keep the conditionals in CDMD because
DownloadHistory<Observer will reduce the number of calls from CDMD to DH to 2
(construction and CheckVisitedReferrerBefore). Let me know if I misremember.

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:138:
download_manager_));
On 2012/08/02 17:52:17, rdsmith wrote:
> I'm somewhat uncomfortable here.  I'll agree in advance with your (presumed
:-})
> argument that the lifetime guarantees for the history loading chain are not at
> all clear, and thus that we're risking some future change resulting in a use
> after free error.  But the lifetime guarantees are (I believe) there--I think
> there's a cancellation between DownloadHistory and the DB thread to guarantee
> that callbacks don't come back if download_history_ goes away (there at least
> used to be).  And I think it's confusing to have multiple code that solves the
> same problem.  So I'd rather cleanup the entire stack than just change this
> point. 
> 
> An additional point is that I'd like to be moving towards fewer classes being
> refcounted, and this relies on CDMD being refcounted.
> 
> Could you put in a TODO?  Either in place of this change ('lifetime guarantees
> confusing; cleanup') or in addition to this change ('duplicates the effect of
XX
> in YY but is clearer in its effect; cleanup to have a single clear
mechanism')?
> 
> (Applies throughout file for this change.)

This call is disappearing in my DownloadHistory CL.

A TODO for the other former Unretaineds seems unnecessary. We know that
RefCountedThreadSafe is bad. The compiler will let us know if we forget to
update a caller.

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:625:
json_results->Append(json_item.release());
On 2012/08/02 17:52:17, rdsmith wrote:
> I wince a little bit at RunDownloadQuery having the incognito/non-incognito
> information and throwing it away so that it has to be regenerated here.  It
> looks like this is the only place RunDownloadQuery() is called from.  Maybe
> refactor so that RunDownloadQuery creates the json so that you don't need to
> regenerate the incognito info?

The downloads are all mixed together, so that wouldn't actually help. Also,
RunDownloadQuery is about to be used by erase().

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:887: item,
incognito_));
On 2012/08/02 17:52:17, rdsmith wrote:
> Won't this produce the wrong result when we're dispatching a regular download
to
> an incognito profile?

We discussed offline how EDER handles incognito. To summarize, each EDER
observers exactly one manager, which is always either incognito or regular. The
incognito-ness of the download isn't lost until DispatchEventInternal.

[Randy Smith (Not in Mondays), 2012-08-13 18:52:58]

LGTM with below request.

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:135: if
(profile_->GetOriginalProfile() == profile_) {
On 2012/08/13 15:08:09, benjhayden_chromium wrote:
> On 2012/08/02 17:52:17, rdsmith wrote:
> > Did you consider doing this at the DownloadHistory level?  What was the
> > arguments for conditionals in CDMD rather than a null DownloadHistory
> > implementation?
> 
> I think we agreed to keep the conditionals in CDMD because
> DownloadHistory<Observer will reduce the number of calls from CDMD to DH to 2
> (construction and CheckVisitedReferrerBefore). Let me know if I misremember.

Yep, that's my memory too.

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:138:
download_manager_));
On 2012/08/13 15:08:09, benjhayden_chromium wrote:
> On 2012/08/02 17:52:17, rdsmith wrote:
> > I'm somewhat uncomfortable here.  I'll agree in advance with your (presumed
> :-})
> > argument that the lifetime guarantees for the history loading chain are not
at
> > all clear, and thus that we're risking some future change resulting in a use
> > after free error.  But the lifetime guarantees are (I believe) there--I
think
> > there's a cancellation between DownloadHistory and the DB thread to
guarantee
> > that callbacks don't come back if download_history_ goes away (there at
least
> > used to be).  And I think it's confusing to have multiple code that solves
the
> > same problem.  So I'd rather cleanup the entire stack than just change this
> > point. 
> > 
> > An additional point is that I'd like to be moving towards fewer classes
being
> > refcounted, and this relies on CDMD being refcounted.
> > 
> > Could you put in a TODO?  Either in place of this change ('lifetime
guarantees
> > confusing; cleanup') or in addition to this change ('duplicates the effect
of
> XX
> > in YY but is clearer in its effect; cleanup to have a single clear
> mechanism')?
> > 
> > (Applies throughout file for this change.)
> 
> This call is disappearing in my DownloadHistory CL.
> 
> A TODO for the other former Unretaineds seems unnecessary. We know that
> RefCountedThreadSafe is bad. The compiler will let us know if we forget to
> update a caller.

The problem is someone (say a poor idealistic new grad--won't anyone think of
the children??) without background knowledge coming upon this code.  I imagine
they could waste a lot of time trying to figure out how to provide lifetime
guarantees to replace the loss of ref_counted, when they don't need to, because
the underlying code already provides those lifetime guarantees.  One line
comment where the Unretaineds used to be saying "lifetime guarantee also
provided by *Cancel* in DownloadHistory"?  (Or whatever it really should say--I
haven't looked at the code.)

In my ideal world we wouldn't use *Cancel* just because I don't think it's at
all obvious what it's doing; weak pointers strike me as a more understandable,
and more commonly known, lifetime protection mechanism.  That's why I originally
suggested a TODO.  But just some breadcrumb so that someone doesn't waste time
assuming the RCTS nature of CDMD is required at these locations when it isn't
would also be fine.

[benjhayden, 2012-08-13 20:26:24]

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10805020/diff/20006/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:138:
download_manager_));
On 2012/08/13 18:52:58, rdsmith wrote:
> On 2012/08/13 15:08:09, benjhayden_chromium wrote:
> > On 2012/08/02 17:52:17, rdsmith wrote:
> > > I'm somewhat uncomfortable here.  I'll agree in advance with your
(presumed
> > :-})
> > > argument that the lifetime guarantees for the history loading chain are
not
> at
> > > all clear, and thus that we're risking some future change resulting in a
use
> > > after free error.  But the lifetime guarantees are (I believe) there--I
> think
> > > there's a cancellation between DownloadHistory and the DB thread to
> guarantee
> > > that callbacks don't come back if download_history_ goes away (there at
> least
> > > used to be).  And I think it's confusing to have multiple code that solves
> the
> > > same problem.  So I'd rather cleanup the entire stack than just change
this
> > > point. 
> > > 
> > > An additional point is that I'd like to be moving towards fewer classes
> being
> > > refcounted, and this relies on CDMD being refcounted.
> > > 
> > > Could you put in a TODO?  Either in place of this change ('lifetime
> guarantees
> > > confusing; cleanup') or in addition to this change ('duplicates the effect
> of
> > XX
> > > in YY but is clearer in its effect; cleanup to have a single clear
> > mechanism')?
> > > 
> > > (Applies throughout file for this change.)
> > 
> > This call is disappearing in my DownloadHistory CL.
> > 
> > A TODO for the other former Unretaineds seems unnecessary. We know that
> > RefCountedThreadSafe is bad. The compiler will let us know if we forget to
> > update a caller.
> 
> The problem is someone (say a poor idealistic new grad--won't anyone think of
> the children??) without background knowledge coming upon this code.  I imagine
> they could waste a lot of time trying to figure out how to provide lifetime
> guarantees to replace the loss of ref_counted, when they don't need to,
because
> the underlying code already provides those lifetime guarantees.  One line
> comment where the Unretaineds used to be saying "lifetime guarantee also
> provided by *Cancel* in DownloadHistory"?  (Or whatever it really should
say--I
> haven't looked at the code.)
> 
> In my ideal world we wouldn't use *Cancel* just because I don't think it's at
> all obvious what it's doing; weak pointers strike me as a more understandable,
> and more commonly known, lifetime protection mechanism.  That's why I
originally
> suggested a TODO.  But just some breadcrumb so that someone doesn't waste time
> assuming the RCTS nature of CDMD is required at these locations when it isn't
> would also be fine.

Done.

[commit-bot: I haz the power, 2012-08-13 20:26:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10805020/34009

[commit-bot: I haz the power, 2012-08-13 20:26:49]

Presubmit check for 10805020-34009 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/automation
    chrome/browser/ui/webui

Presubmit checks took 2.2s to calculate.

[benjhayden, 2012-08-13 20:39:06]

PTAL

[jam, 2012-08-13 20:40:19]

lgtm

[James Hawkins, 2012-08-13 20:47:53]

LGTM with nits.

http://codereview.chromium.org/10805020/diff/34009/chrome/browser/ui/webui/do...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/10805020/diff/34009/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_dom_handler.cc:118: DictionaryValue*
CreateDownloadItemValue(
nit: Document method and parameters.

http://codereview.chromium.org/10805020/diff/34009/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_dom_handler.cc:124:
file_value->SetInteger("started",
nit: Start of parameter rows must align on the same column.

http://codereview.chromium.org/10805020/diff/34009/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_dom_handler.cc:203: bool IsItemIncognito(
nit: Document method and parameters.

[benjhayden, 2012-08-13 21:49:58]

Thanks, everybody!

Asanka, feel free to wait until tomorrow, I don't want to merge this with
http://codereview.chromium.org/10855135/

http://codereview.chromium.org/10805020/diff/34009/chrome/browser/ui/webui/do...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/10805020/diff/34009/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_dom_handler.cc:118: DictionaryValue*
CreateDownloadItemValue(
On 2012/08/13 20:47:53, James Hawkins wrote:
> nit: Document method and parameters.

Done.

http://codereview.chromium.org/10805020/diff/34009/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_dom_handler.cc:124:
file_value->SetInteger("started",
On 2012/08/13 20:47:53, James Hawkins wrote:
> nit: Start of parameter rows must align on the same column.

Done.

http://codereview.chromium.org/10805020/diff/34009/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_dom_handler.cc:203: bool IsItemIncognito(
On 2012/08/13 20:47:53, James Hawkins wrote:
> nit: Document method and parameters.

Done.

[asanka, 2012-08-13 23:03:57]

LGTM. Comments are nits.

https://chromiumcodereview.appspot.com/10805020/diff/37030/chrome/browser/aut...
File chrome/browser/automation/automation_provider.cc (right):

https://chromiumcodereview.appspot.com/10805020/diff/37030/chrome/browser/aut...
chrome/browser/automation/automation_provider.cc:43: #include
"chrome/browser/download/download_service_factory.h"
Nit: Are these needed?

https://chromiumcodereview.appspot.com/10805020/diff/37030/chrome/browser/dow...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://chromiumcodereview.appspot.com/10805020/diff/37030/chrome/browser/dow...
chrome/browser/download/chrome_download_manager_delegate.cc:135: if
(profile_->GetOriginalProfile() == profile_) {
Nit: Why not !profile_->IsOffTheRecord() ? It seems clearer.

https://chromiumcodereview.appspot.com/10805020/diff/37030/chrome/browser/ext...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://chromiumcodereview.appspot.com/10805020/diff/37030/chrome/browser/ext...
chrome/browser/extensions/api/downloads/downloads_api.cc:335: (profile !=
profile->GetOriginalProfile()))) {
Nit: As above. profile->IsOffTheRecord()?

https://chromiumcodereview.appspot.com/10805020/diff/37030/chrome/browser/ext...
chrome/browser/extensions/api/downloads/downloads_api.cc:978: // There is a one
EDER for each on-reocrd Profile, and a separate EDER for
Nit: on-record

[benjhayden, 2012-08-14 14:30:51]

Thanks, everybody!

http://codereview.chromium.org/10805020/diff/37030/chrome/browser/automation/...
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/10805020/diff/37030/chrome/browser/automation/...
chrome/browser/automation/automation_provider.cc:43: #include
"chrome/browser/download/download_service_factory.h"
On 2012/08/13 23:03:58, asanka wrote:
> Nit: Are these needed?

Done.

http://codereview.chromium.org/10805020/diff/37030/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10805020/diff/37030/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:135: if
(profile_->GetOriginalProfile() == profile_) {
On 2012/08/13 23:03:58, asanka wrote:
> Nit: Why not !profile_->IsOffTheRecord() ? It seems clearer.

Done.

http://codereview.chromium.org/10805020/diff/37030/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/10805020/diff/37030/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:335: (profile !=
profile->GetOriginalProfile()))) {
On 2012/08/13 23:03:58, asanka wrote:
> Nit: As above. profile->IsOffTheRecord()?

Done.

http://codereview.chromium.org/10805020/diff/37030/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:978: // There is a one
EDER for each on-reocrd Profile, and a separate EDER for
On 2012/08/13 23:03:58, asanka wrote:
> Nit: on-record

Done.

[commit-bot: I haz the power, 2012-08-14 14:31:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10805020/21013

[commit-bot: I haz the power, 2012-08-14 17:51:56]

Change committed as 151504