Rewrite DownloadsApiTest in C++.

Rewrite DownloadsApiTest in C++.
This uses much less magic than the javascript version, so it's more robust and easier to debug and fix.

Aaron is the primary reviewer since most of this is extensions-specific.
Randy, none of this is particularly downloads-specific. You're welcome to review or mute.

ExtensionDownloadsEventRouter sends a chrome::NOTIFICATION_EXTENSION_DOWNLOADS_EVENT notification whenever it fires an event.
DownloadsEventsListener is-a content::NotificationObserver that listens for chrome::NOTIFICATION_EXTENSION_DOWNLOADS_EVENT and logs them. DEL facilitates waiting for specific events by running the message loops.
BrowserContext::GetFileSystemContext() is used to create HTML5 FileSystem Files.
Empty extensions are used because DownloadsDownloadFunction relies on its host permissions mechanisms.

I considered grouping all of the new TEST_Fs in a single large TEST_F in order to amortize start-up cost, but that would have required a comment explaining how to disable sub-sections, and it would have complicated time-outs. Besides, most bots can parallelize.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144807
Reverted due to word length mismatch bug in downloads_api_unittest.cc:1581: https://chromiumcodereview.appspot.com/10700024/
Trying again with %d and static_cast<int>().

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

[benjhayden, 2012-06-15 17:14:40]

PTAL

[benjhayden, 2012-06-18 15:57:05]

I updated the CL description to be a little more helpful.
I still need to debug some of the tests on windows, but I don't expect this CL
to change much. It works very well on *nix.

[Randy Smith (Not in Mondays), 2012-06-18 18:42:58]

For whatever reason, I was inspired to do a thorough review
(chrome/browser/download/ only).  High level questions/comments:

* I think it's worthwhile thinking about where to put in line breaks in the
tests to group related actions; it's just easier to read code with occasional
line breaks (like text with paragraph breaks).

* I'd suggest you have at least one test that confirms that the resulting file
contents are what you expect.  That should be tested by the basic download
tests, but it's worthwhile confirming that the extension system doesn't have any
strange effects in that space.  Maybe the filesystem test, since there's a large
end-to-end path length for things to go wrong along?

* Could you say something in a test comment about why you didn't copy over all
the tests from the .js file?  I could be wrong, but there seem to be several
that used to be present that aren't there.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
File chrome/browser/download/download_extension_api.cc (right):

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_api.cc:1113: profile_ = NULL;
So this means that you'll not handle the notifications that are generated for
individual download items during the shutdown process.  Is that the behavior you
want?  Can you give me a sense as to why?  (Not saying I object, just that I
want to make sure we explore the implications.)

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_api.cc:1135:
content::Details<std::string>(&json_args));
This is two different mechanisms for notifying about changes to a download item,
on top of the DownloadItem observer mechanism.  Would it make sense to, rather
than doing both of these, make the ExtensionDownloadsEventRouter automatically
become a DownloadManager observer in its constructor?  And maybe find some way
(this is probably a large hassle in C++, but I figure it can't hurt to ask) to
hook the renderer event dispatch in C++ to avoid the extra NotificationService
notification?

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
File chrome/browser/download/download_extension_test.cc (right):

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:68: static const int
kTimeOutMs = -1;
The comment and the value (which I take to mean "infinite") don't seem like
they're in alignment?

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:76:
STLDeleteElements(&events_);
Shouldn't this have a registrar_.Remove()?

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:88:
args_(base::JSONReader::Read(json_args)),
Is there some location you can point to for the format of these args?  It's hard
to verify the below code without knowing what the args are supposed to look
like.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:109:
!right_list->GetDictionary(0, &right_dict))
Does this mean the Equals function returns false if either of the lists are
empty?  (This may be just my confusion as to args format; see above.)

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:112: iter.HasNext();
iter.Advance()) {
nit: indent?  (May be just formatting in Rietveld.)

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:169: // This is public to
accommodate the HTML5 FileSystem Files event.
I found this comment confusing when I first read it.  Maybe "This is public to
allow creating an indirect callback to this function from the HTML5 FileSystem
test below."?

I'll also note that you could get around the entire problem by consing up a
NotificationSource and NotificationDetails by hand and using Observer directly. 
Admittedly hacky, and I don't have a preference one way or the other, but
mentioning it anyway.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:273: void LoadExtension(const
char* name) {
I find this confusing; reasonable to put a comment in explaining what this does?
 I'm specifically confused as to why we're redirecting from a function name that
implies (to me) non-incognito to an incognito function.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:315: std::string GetURL(const
char* path) {
Why not test_server()->GetURL?

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:339: }
I have mixed feelings about On/Off record state being stateful in the contexts
of the tests; my leaning would have been to plumb a flag/pass in the correct
profile to all functions whose behavior would have been different in the two
cases so that you don't have to keep state in your mind as you scan the test
code.  OTOH, I could imagine that could end up being a lot of confusing
plumbing.  So I'll raise the issue, but leave it up to you which way you go.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:476: // on-record profile to
match real-life behavior.
Why does it match real-life behavior?  Does this same thing happen if an
extension runs in incognito mode?  If not, how are we testing incognito mode
running of functions?

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:636: void FileCreated(const
std::string& url, const base::Closure& done,
Could you put two comments here, one that these functions are listed in reverse
order of execution for reference reasons (side note: I debated asking for
prototypes + ordering by execution order, and decided that was overkill, but I
wouldn't object if you decided you wanted to do it), and another before each
function just saying what it does?  Alternatively, a longer comment just going
over the thread hopping and operations done on each thread at the top.  I think
it'll make it easier to read for people who don't know the code.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:658: &CreateFileOnIOThread,
operation, done, root.spec() + filename)));
nit: indent?  (Again not sure about Rietveld formatting.)

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1127: off_item_arg =
DownloadItemIdAsArgList(off_item);
Why reverse the order?  Any particular reason?  If it's subtle, it probably
deserves a comment.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1246:
ASSERT_FALSE(WaitFor(100, extension_event_names::kOnDownloadErased, "-1"));
What is this actually testing?  Just that 100 != infinity?

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1251:
LoadExtension("downloads_split");
What does this do/what is it for?  Some documentation wouldn't be amiss. 
Possibly it's worth abstracting this behind a base function that's named
something more explanatory, and having a longer comment at that base function?

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1266:
ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
Doesn't this result in a hung test if the event never happens?  (Possibly same
comment as question about -1 timeout above.)

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1289:
DownloadExtensionTest_Download_Incognito) {
If you're testing incognito, shouldn't part of that test be that it's not
visible from non-incognito contexts?

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1582: // The item may or may
not be interrupted before the onCreated event fires.
I'd add an explanatory line here or below (that in the "before" case the WaitFor
below is just checking that the event that already happened is more specific
than you've already tested for.)

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1582: // The item may or may
not be interrupted before the onCreated event fires.
Also, this pattern seems to occur repeatedly in the file; any way to abstract
it?

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1607:
DownloadExtensionTest_Download_Headers) {
It looks like this test is only testing that the system doesn't break when you
put in the "headers" arg (i.e. it doesn't look like we verify, in this test,
that the headers arg made it through to the server.)  That right?

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1652:
IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
I think it's generally a good idea to have a line or two of comment before each
test describing what the test is actually testing.  When the test is really
simple and the name is clear, that's not needed, but otherwise I think it's a
useful pattern.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1659: // the downloads
extension api exposes the failure to extensions.
As well as that the api really does propagate the headers all the way through.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1950: // NOTE: chrome
disallows creating HTML5 FileSystem Files in incognito.
Should we test that?

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_test_utils.cc (right):

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_test_utils.cc:222: return result;
Why?

[benjhayden, 2012-06-19 15:01:59]

PTAL
Still debugging windows.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
File chrome/browser/download/download_extension_api.cc (right):

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_api.cc:1113: profile_ = NULL;
On 2012/06/18 18:42:58, rdsmith wrote:
> So this means that you'll not handle the notifications that are generated for
> individual download items during the shutdown process.  Is that the behavior
you
> want?  Can you give me a sense as to why?  (Not saying I object, just that I
> want to make sure we explore the implications.)

When I made ChromeDownloadManagerDelegate own ExtensionDownloadsEventRouter,
DispatchEvent() started crashing. Either profile_ or
profile_->GetExtensionEventRouter() had already been destroyed by the time EDER
observed the shutdown events.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_api.cc:1135:
content::Details<std::string>(&json_args));
On 2012/06/18 18:42:58, rdsmith wrote:
> This is two different mechanisms for notifying about changes to a download
item,
> on top of the DownloadItem observer mechanism.  Would it make sense to, rather
> than doing both of these, make the ExtensionDownloadsEventRouter automatically
> become a DownloadManager observer in its constructor?  And maybe find some way
> (this is probably a large hassle in C++, but I figure it can't hurt to ask) to
> hook the renderer event dispatch in C++ to avoid the extra NotificationService
> notification?

EDER is-a DM::O already. How would making it a DM::O make it unnecessary for
EDER to give events to the extension system?

I could add a delegate to EDER so that EDER could call out to the testcase
directly, but that would require exposing CDMD's EDER somehow, which seems like
bad encapsulation.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
File chrome/browser/download/download_extension_test.cc (right):

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:68: static const int
kTimeOutMs = -1;
On 2012/06/18 18:42:58, rdsmith wrote:
> The comment and the value (which I take to mean "infinite") don't seem like
> they're in alignment?

Done.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:76:
STLDeleteElements(&events_);
On 2012/06/18 18:42:58, rdsmith wrote:
> Shouldn't this have a registrar_.Remove()?

Done.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:88:
args_(base::JSONReader::Read(json_args)),
On 2012/06/18 18:42:58, rdsmith wrote:
> Is there some location you can point to for the format of these args?  It's
hard
> to verify the below code without knowing what the args are supposed to look
> like.

The structure of args_ depends on event_name. See the DispatchEvent() calls in
ExtensionDownloadsEventRouter, or {experimental_,}downloads.json.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:109:
!right_list->GetDictionary(0, &right_dict))
On 2012/06/18 18:42:58, rdsmith wrote:
> Does this mean the Equals function returns false if either of the lists are
> empty?  (This may be just my confusion as to args format; see above.)

For onCreated and onChanged events, yes.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:112: iter.HasNext();
iter.Advance()) {
On 2012/06/18 18:42:58, rdsmith wrote:
> nit: indent?  (May be just formatting in Rietveld.)

Done.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:169: // This is public to
accommodate the HTML5 FileSystem Files event.
On 2012/06/18 18:42:58, rdsmith wrote:
> I found this comment confusing when I first read it.  Maybe "This is public to
> allow creating an indirect callback to this function from the HTML5 FileSystem
> test below."?
> 
> I'll also note that you could get around the entire problem by consing up a
> NotificationSource and NotificationDetails by hand and using Observer
directly. 
> Admittedly hacky, and I don't have a preference one way or the other, but
> mentioning it anyway.

Done.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:273: void LoadExtension(const
char* name) {
On 2012/06/18 18:42:58, rdsmith wrote:
> I find this confusing; reasonable to put a comment in explaining what this
does?
>  I'm specifically confused as to why we're redirecting from a function name
that
> implies (to me) non-incognito to an incognito function.

Done.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:315: std::string GetURL(const
char* path) {
On 2012/06/18 18:42:58, rdsmith wrote:
> Why not test_server()->GetURL?

Done.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:339: }
On 2012/06/18 18:42:58, rdsmith wrote:
> I have mixed feelings about On/Off record state being stateful in the contexts
> of the tests; my leaning would have been to plumb a flag/pass in the correct
> profile to all functions whose behavior would have been different in the two
> cases so that you don't have to keep state in your mind as you scan the test
> code.  OTOH, I could imagine that could end up being a lot of confusing
> plumbing.  So I'll raise the issue, but leave it up to you which way you go.

I originally started with passing flags everywhere. Besides the mysterious
nameless boolean problem (which could be solved with an enum), there was the
problem of copy-pasting bits of test code around and forgetting to update each
and every little boolean. I could store the boolean in each TEST_F, but that's
unnecessary code duplication.
Reading through the code, I like how not every line of test code needs to think
about incognito. When you tell a story, you don't set the scene over and over in
every sentence.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:476: // on-record profile to
match real-life behavior.
On 2012/06/18 18:42:58, rdsmith wrote:
> Why does it match real-life behavior?  Does this same thing happen if an
> extension runs in incognito mode?  If not, how are we testing incognito mode
> running of functions?

Yes, ExtensionFunctions get the on-record profile even when in incognito
contexts.
ExtensionFunctions use include_incognito() to determine whether to act like
they're in incognito contexts. You'd have to ask Aaron why.
I tell the functions that they're in incognito by passing
extension_function_test_utils::INCLUDE_INCOGNITO (from GetFlags()) through
ExtensionFunction::include_incognito().

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:636: void FileCreated(const
std::string& url, const base::Closure& done,
On 2012/06/18 18:42:58, rdsmith wrote:
> Could you put two comments here, one that these functions are listed in
reverse
> order of execution for reference reasons (side note: I debated asking for
> prototypes + ordering by execution order, and decided that was overkill, but I
> wouldn't object if you decided you wanted to do it), and another before each
> function just saying what it does?  Alternatively, a longer comment just going
> over the thread hopping and operations done on each thread at the top.  I
think
> it'll make it easier to read for people who don't know the code. 

Done.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:658: &CreateFileOnIOThread,
operation, done, root.spec() + filename)));
On 2012/06/18 18:42:58, rdsmith wrote:
> nit: indent?  (Again not sure about Rietveld formatting.)

Looks right to me. The previous line ends with a paren, so this line is indented
+4.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1127: off_item_arg =
DownloadItemIdAsArgList(off_item);
On 2012/06/18 18:42:58, rdsmith wrote:
> Why reverse the order?  Any particular reason?  If it's subtle, it probably
> deserves a comment.

Done.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1246:
ASSERT_FALSE(WaitFor(100, extension_event_names::kOnDownloadErased, "-1"));
On 2012/06/18 18:42:58, rdsmith wrote:
> What is this actually testing?  Just that 100 != infinity?

This is actually testing that DownloadsEventsListener times out quickly enough
for an event that isn't going to happen.
Do you have a more specific question?

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1251:
LoadExtension("downloads_split");
On 2012/06/18 18:42:58, rdsmith wrote:
> What does this do/what is it for?  Some documentation wouldn't be amiss. 
> Possibly it's worth abstracting this behind a base function that's named
> something more explanatory, and having a longer comment at that base function?

Done.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1266:
ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
On 2012/06/18 18:42:58, rdsmith wrote:
> Doesn't this result in a hung test if the event never happens?  (Possibly same
> comment as question about -1 timeout above.)

Yep. See my new comments on kTimeOutMs.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1289:
DownloadExtensionTest_Download_Incognito) {
On 2012/06/18 18:42:58, rdsmith wrote:
> If you're testing incognito, shouldn't part of that test be that it's not
> visible from non-incognito contexts?

That's tested in SearchIncognito.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1582: // The item may or may
not be interrupted before the onCreated event fires.
On 2012/06/18 18:42:58, rdsmith wrote:
> I'd add an explanatory line here or below (that in the "before" case the
WaitFor
> below is just checking that the event that already happened is more specific
> than you've already tested for.)

Done.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1582: // The item may or may
not be interrupted before the onCreated event fires.
On 2012/06/18 18:42:58, rdsmith wrote:
> Also, this pattern seems to occur repeatedly in the file; any way to abstract
> it?

Done.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1607:
DownloadExtensionTest_Download_Headers) {
On 2012/06/18 18:42:58, rdsmith wrote:
> It looks like this test is only testing that the system doesn't break when you
> put in the "headers" arg (i.e. it doesn't look like we verify, in this test,
> that the headers arg made it through to the server.)  That right?

The test server verifies the expected_headers, and only completes the download
if the headers are there.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1652:
IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
On 2012/06/18 18:42:58, rdsmith wrote:
> I think it's generally a good idea to have a line or two of comment before
each
> test describing what the test is actually testing.  When the test is really
> simple and the name is clear, that's not needed, but otherwise I think it's a
> useful pattern.

Done.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1659: // the downloads
extension api exposes the failure to extensions.
On 2012/06/18 18:42:58, rdsmith wrote:
> As well as that the api really does propagate the headers all the way through.

No, that's tested by DownloadExtensionTest_Download_Headers

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1950: // NOTE: chrome
disallows creating HTML5 FileSystem Files in incognito.
On 2012/06/18 18:42:58, rdsmith wrote:
> Should we test that?

What would it buy to test that HTML5 Files are disallowed in incognito?
I suppose it would let us know if/when we need to start testing downloading
html5 files in incognito.
Is that what you were thinking?

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_test_utils.cc (right):

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_test_utils.cc:222: return result;
On 2012/06/18 18:42:58, rdsmith wrote:
> Why?

Done.

[Randy Smith (Not in Mondays), 2012-06-19 19:23:22]

Review still in process.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
File chrome/browser/download/download_extension_api.cc (right):

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_api.cc:1113: profile_ = NULL;
On 2012/06/19 15:01:59, benjhayden_chromium wrote:
> On 2012/06/18 18:42:58, rdsmith wrote:
> > So this means that you'll not handle the notifications that are generated
for
> > individual download items during the shutdown process.  Is that the behavior
> you
> > want?  Can you give me a sense as to why?  (Not saying I object, just that I
> > want to make sure we explore the implications.)
> 
> When I made ChromeDownloadManagerDelegate own ExtensionDownloadsEventRouter,
> DispatchEvent() started crashing. Either profile_ or
> profile_->GetExtensionEventRouter() had already been destroyed by the time
EDER
> observed the shutdown events.

Sure, that's a problem, but you've chosen one particular way of solving it. 
Another way might be to make sure that whatever notifications show up after
DownloadManager::Shutdown() are noticed as being spurious and dropped on the
floor (I don't know how we'd do that because I don't know what such
notifications would show up--we shouldn't be generating any notifications after
that point.  So this would take some debugging.)  But I'm not sure that's the
right thing to do either, so I'm asking the behavioral question.  From the
viewpoint of an extension writer, would getting notifications from the
DownloadItems as they're being torn down by the DownloadManager during shutdown
be valuable?

I could argue "Yes!  If we don't get those notifications, we won't know that the
download items were actually interrupted/cancelled/whatever, and that's vital
for tracking their life cycle."  I could also argue "No!  By the time we're
calling DownloadManager::Shutdown, the profile is on its way out, and all
extensions have been torn down, so it doesn't make any difference."  (I don't
know that that's true, but it's plausible.)  Since I don't know what the right
argument is, I'm asking the Download extensions owner :-}.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_api.cc:1135:
content::Details<std::string>(&json_args));
On 2012/06/19 15:01:59, benjhayden_chromium wrote:
> On 2012/06/18 18:42:58, rdsmith wrote:
> > This is two different mechanisms for notifying about changes to a download
> item,
> > on top of the DownloadItem observer mechanism.  Would it make sense to,
rather
> > than doing both of these, make the ExtensionDownloadsEventRouter
automatically
> > become a DownloadManager observer in its constructor?  And maybe find some
way
> > (this is probably a large hassle in C++, but I figure it can't hurt to ask)
to
> > hook the renderer event dispatch in C++ to avoid the extra
NotificationService
> > notification?
> 
> EDER is-a DM::O already. How would making it a DM::O make it unnecessary for
> EDER to give events to the extension system?

Sorry, this is my cluelessness; I didn't notice that we were already inside
ExtensionDownloadsEventRouter (should it have its own file?).  Ignore that
suggestion.

> I could add a delegate to EDER so that EDER could call out to the testcase
> directly, but that would require exposing CDMD's EDER somehow, which seems
like
> bad encapsulation.

Yeah.  Both directions seem messy to me, but I certainly agree with you that the
EDER delegate is no neater than what you've done.  Ok, I yield :-}.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
File chrome/browser/download/download_extension_test.cc (right):

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:658: &CreateFileOnIOThread,
operation, done, root.spec() + filename)));
On 2012/06/19 15:01:59, benjhayden_chromium wrote:
> On 2012/06/18 18:42:58, rdsmith wrote:
> > nit: indent?  (Again not sure about Rietveld formatting.)
> 
> Looks right to me. The previous line ends with a paren, so this line is
indented
> +4.

Right--did the alignment wrong in my head.  Ooops.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1246:
ASSERT_FALSE(WaitFor(100, extension_event_names::kOnDownloadErased, "-1"));
On 2012/06/19 15:01:59, benjhayden_chromium wrote:
> On 2012/06/18 18:42:58, rdsmith wrote:
> > What is this actually testing?  Just that 100 != infinity?
> 
> This is actually testing that DownloadsEventsListener times out quickly enough
> for an event that isn't going to happen.
> Do you have a more specific question?

What you look to be testing is that WaitFor(100, ...) actually times out. 
There's no definition of "quickly enough"; it's just a binary thing.  Thus my
question as to whether it's just that 100 != infinity (to WaitFor(); it clearly
doesn't to other entities :-}).  Is that right?  

If so, it's primarily the words "quickly enough" that I dislike; you're just
testing that the timeout functionality works.  (I wince a bit at testing it and
then not using it, especially in a browser_test, as that just takes time.  But
it's the, to me, misleading comment that provoked my question.)

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1251:
LoadExtension("downloads_split");
On 2012/06/19 15:01:59, benjhayden_chromium wrote:
> On 2012/06/18 18:42:58, rdsmith wrote:
> > What does this do/what is it for?  Some documentation wouldn't be amiss. 
> > Possibly it's worth abstracting this behind a base function that's named
> > something more explanatory, and having a longer comment at that base
function?
> 
> Done.

As discussed, maybe a comment in the downloads_split extension explaining its
purpose in life?

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1950: // NOTE: chrome
disallows creating HTML5 FileSystem Files in incognito.
On 2012/06/19 15:01:59, benjhayden_chromium wrote:
> On 2012/06/18 18:42:58, rdsmith wrote:
> > Should we test that?
> 
> What would it buy to test that HTML5 Files are disallowed in incognito?
> I suppose it would let us know if/when we need to start testing downloading
> html5 files in incognito.
> Is that what you were thinking?

In a sorta vague fuzzy background way, yeah.  I don't consider it high priority.

http://codereview.chromium.org/10542038/diff/28001/chrome/browser/download/do...
File chrome/browser/download/download_extension_test.cc (right):

http://codereview.chromium.org/10542038/diff/28001/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:76: // bots.
This comment is targeted at the WaitFor() routine.  You probably want to start
out saying "Default value for WaitFor() timeout." and then say the rest of what
you have said.

(And I unfortunately agree with your argument.  The abstract pattern I try to do
here is "Is there some other event that I'm fairly sure will happen, even if
whatever I'm testing for doesn't, which in a success case will always be after
the event I'm testing for?"  If so, I wait for that other event, then test for
success afterwards.  But sometimes there's no such event :-{.)

http://codereview.chromium.org/10542038/diff/28001/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:354: return
test_server()->GetURL(path).spec();
Personal preference would be to expand this inline to make clear what's
happening; "it's only one line".  But it's up to you.

[benjhayden, 2012-06-21 17:50:48]

PTAL

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
File chrome/browser/download/download_extension_api.cc (right):

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_api.cc:1113: profile_ = NULL;
On 2012/06/19 19:23:23, rdsmith wrote:
> On 2012/06/19 15:01:59, benjhayden_chromium wrote:
> > On 2012/06/18 18:42:58, rdsmith wrote:
> > > So this means that you'll not handle the notifications that are generated
> for
> > > individual download items during the shutdown process.  Is that the
behavior
> > you
> > > want?  Can you give me a sense as to why?  (Not saying I object, just that
I
> > > want to make sure we explore the implications.)
> > 
> > When I made ChromeDownloadManagerDelegate own ExtensionDownloadsEventRouter,
> > DispatchEvent() started crashing. Either profile_ or
> > profile_->GetExtensionEventRouter() had already been destroyed by the time
> EDER
> > observed the shutdown events.
> 
> Sure, that's a problem, but you've chosen one particular way of solving it. 
> Another way might be to make sure that whatever notifications show up after
> DownloadManager::Shutdown() are noticed as being spurious and dropped on the
> floor (I don't know how we'd do that because I don't know what such
> notifications would show up--we shouldn't be generating any notifications
after
> that point.  So this would take some debugging.)  But I'm not sure that's the
> right thing to do either, so I'm asking the behavioral question.  From the
> viewpoint of an extension writer, would getting notifications from the
> DownloadItems as they're being torn down by the DownloadManager during
shutdown
> be valuable?
> 
> I could argue "Yes!  If we don't get those notifications, we won't know that
the
> download items were actually interrupted/cancelled/whatever, and that's vital
> for tracking their life cycle."  I could also argue "No!  By the time we're
> calling DownloadManager::Shutdown, the profile is on its way out, and all
> extensions have been torn down, so it doesn't make any difference."  (I don't
> know that that's true, but it's plausible.)  Since I don't know what the right
> argument is, I'm asking the Download extensions owner :-}.
> 

An extension that wants to keep track of the state of some items should call
search() when it starts up.
I am inclined to do the simple thing until it proves insufficient somehow, then
consider addressing the particular insufficiency. This route does not preclude
figuring out how to fire the shutdown events at some point.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
File chrome/browser/download/download_extension_test.cc (right):

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1246:
ASSERT_FALSE(WaitFor(100, extension_event_names::kOnDownloadErased, "-1"));
On 2012/06/19 19:23:23, rdsmith wrote:
> On 2012/06/19 15:01:59, benjhayden_chromium wrote:
> > On 2012/06/18 18:42:58, rdsmith wrote:
> > > What is this actually testing?  Just that 100 != infinity?
> > 
> > This is actually testing that DownloadsEventsListener times out quickly
enough
> > for an event that isn't going to happen.
> > Do you have a more specific question?
> 
> What you look to be testing is that WaitFor(100, ...) actually times out. 
> There's no definition of "quickly enough"; it's just a binary thing.  Thus my
> question as to whether it's just that 100 != infinity (to WaitFor(); it
clearly
> doesn't to other entities :-}).  Is that right?  
> 
> If so, it's primarily the words "quickly enough" that I dislike; you're just
> testing that the timeout functionality works.  (I wince a bit at testing it
and
> then not using it, especially in a browser_test, as that just takes time.  But
> it's the, to me, misleading comment that provoked my question.)
> 

The test doesn't use the timeout, and I can always add it back if I need it, so
I took it out.

http://codereview.chromium.org/10542038/diff/24002/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:1251:
LoadExtension("downloads_split");
On 2012/06/19 19:23:23, rdsmith wrote:
> On 2012/06/19 15:01:59, benjhayden_chromium wrote:
> > On 2012/06/18 18:42:58, rdsmith wrote:
> > > What does this do/what is it for?  Some documentation wouldn't be amiss. 
> > > Possibly it's worth abstracting this behind a base function that's named
> > > something more explanatory, and having a longer comment at that base
> function?
> > 
> > Done.
> 
> As discussed, maybe a comment in the downloads_split extension explaining its
> purpose in life?

Done.

http://codereview.chromium.org/10542038/diff/28001/chrome/browser/download/do...
File chrome/browser/download/download_extension_test.cc (right):

http://codereview.chromium.org/10542038/diff/28001/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:76: // bots.
On 2012/06/19 19:23:23, rdsmith wrote:
> This comment is targeted at the WaitFor() routine.  You probably want to start
> out saying "Default value for WaitFor() timeout." and then say the rest of
what
> you have said.
> 
> (And I unfortunately agree with your argument.  The abstract pattern I try to
do
> here is "Is there some other event that I'm fairly sure will happen, even if
> whatever I'm testing for doesn't, which in a success case will always be after
> the event I'm testing for?"  If so, I wait for that other event, then test for
> success afterwards.  But sometimes there's no such event :-{.)

Done.

http://codereview.chromium.org/10542038/diff/28001/chrome/browser/download/do...
chrome/browser/download/download_extension_test.cc:354: return
test_server()->GetURL(path).spec();
On 2012/06/19 19:23:23, rdsmith wrote:
> Personal preference would be to expand this inline to make clear what's
> happening; "it's only one line".  But it's up to you.

Done.

[benjhayden, 2012-06-21 21:03:45]

Note to self: investigate lifetimes of ExtensionSystem, EDER, Profile, etc. I've
already added the log statements, I just need to run some manual experiments.

[benjhayden, 2012-06-25 21:24:49]

On 2012/06/21 21:03:45, benjhayden_chromium wrote:
> Note to self: investigate lifetimes of ExtensionSystem, EDER, Profile, etc.
I've
> already added the log statements, I just need to run some manual experiments.

Of course, as soon as I try to look at it, it vanishes. I can't repro the bug
that I was seeing anymore, so I took out the part that you objected to.

PTAL

[Randy Smith (Not in Mondays), 2012-06-26 16:16:31]

LGTM.  Thanks!

[commit-bot: I haz the power, 2012-06-28 16:50:45]

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

[commit-bot: I haz the power, 2012-06-28 16:52:30]

Try job failure for 10542038-90001 on mac_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-06-28 19:50:53]

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

[commit-bot: I haz the power, 2012-06-28 21:47:10]

Change committed as 144807

[commit-bot: I haz the power, 2012-06-29 15:42:33]

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

[commit-bot: I haz the power, 2012-06-29 17:30:36]

Change committed as 144921