Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8203005:
Implement chrome.experimental.downloads.onChanged (Closed)
|
Created:
9 years, 2 months ago by benjhayden Modified:
8 years, 10 months ago CC:
chromium-reviews, Aaron Boodman, Erik does not do reviews, mihaip+watch_chromium.org Visibility:
Public. |
DescriptionImplement chrome.experimental.downloads.onChanged
ExtensionDownloadsEventRouter now also observes all DownloadItems and dispatches onChanged events.
Download.OnChanged records the percentage of OnDownloadUpdated() calls that are propagated as onChanged events instead of being suppressed.
BUG=12133
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121912
Patch Set 1 : " #
Total comments: 7
Patch Set 2 : mixin #
Total comments: 4
Patch Set 3 : on_changed_stats_,OnChangedStat #
Total comments: 2
Patch Set 4 : " #
Total comments: 9
Patch Set 5 : merge #Patch Set 6 : comments #Patch Set 7 : merge #Patch Set 8 : merge #Patch Set 9 : merge + apitest rewrite #Patch Set 10 : try re-enabling apitest on mac #Patch Set 11 : try to fix downloadInterrupted #Patch Set 12 : merge #Patch Set 13 : filename windows #
Total comments: 24
Patch Set 14 : comments #Patch Set 15 : merge #
Total comments: 2
Patch Set 16 : fix post tests #Patch Set 17 : merge #Patch Set 18 : debug the new post handler functionality #
Total comments: 4
Patch Set 19 : comments #
Total comments: 1
Patch Set 20 : kill console.logs #
Total comments: 26
Patch Set 21 : fix expected_body #
Total comments: 2
Patch Set 22 : comments #Patch Set 23 : const downloadId #
Total comments: 8
Patch Set 24 : comments #
Total comments: 6
Patch Set 25 : re-disable apitest on mac -- see 9392013 #
Total comments: 2
Patch Set 26 : indentation #Patch Set 27 : retry #
Messages
Total messages: 43 (0 generated)
Gonna hold off on a really detailed review until I get your response to the mix-in idea below. But this looks reasonable so far. http://codereview.chromium.org/8203005/diff/3004/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_downloads_api.cc (right): http://codereview.chromium.org/8203005/diff/3004/chrome/browser/extensions/ex... chrome/browser/extensions/extension_downloads_api.cc:441: void ExtensionDownloadsEventRouter::ItemObserver::OnDownloadUpdated( This amount of work on each DownloadItem updated strengthens my interest in getting rid of updates on size changes and shifting over to polling. Is that on your agenda? My sense is that we've agreed we want to do it but that no one's taken ownership yet, but if that's wrong I want to know :-}. We might alternatively push determining the delta down into the DownloadItem class and have OnDownloadUpdated include an argument indicating which fields have updated. That'd save this level of deltaing. WDYT? http://codereview.chromium.org/8203005/diff/3004/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_downloads_api.h (right): http://codereview.chromium.org/8203005/diff/3004/chrome/browser/extensions/ex... chrome/browser/extensions/extension_downloads_api.h:270: // Observes a single DownloadItem and dispatches onChanged events. Might want a comment here as to why it's not a mix-in on the base class. I'm almost tempted to suggest you do that instead; I think fewer classes is better than more, and it'd save you some of the inter-class interaction at the cost of having to keep a more complex datastructure of the last state seen.
PTAL http://codereview.chromium.org/8203005/diff/3004/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_downloads_api.cc (right): http://codereview.chromium.org/8203005/diff/3004/chrome/browser/extensions/ex... chrome/browser/extensions/extension_downloads_api.cc:441: void ExtensionDownloadsEventRouter::ItemObserver::OnDownloadUpdated( On 2011/10/12 18:46:30, rdsmith wrote: > This amount of work on each DownloadItem updated strengthens my interest in > getting rid of updates on size changes and shifting over to polling. Is that on > your agenda? My sense is that we've agreed we want to do it but that no one's > taken ownership yet, but if that's wrong I want to know :-}. This isn't quite as much work as it looks like. The for-loop is over 10-20 fields, this whole method only does a few small mallocs, and there's just a dash of string processing in delta->Set(), which is almost never called more than 3 times per OnDownloadUpdated(). If you're talking about polling for bytesReceived changes only, then yes. I think it makes good API sense to dispatch an event for other changes both at the extension level and the DownloadItem::Observer level. Shifting DownloadItem over to requiring its observers to poll for bytesReceived events sounds to me like part of the refactoring task slated for next quarter. I think that we can wire up the new ui as it is now, but, yes, not dispatching bytesReceived will be a Good Thing. > > We might alternatively push determining the delta down into the DownloadItem > class and have OnDownloadUpdated include an argument indicating which fields > have updated. That'd save this level of deltaing. WDYT? I think that having DownloadItem tell its observers what changed is the Right Thing To Do, even though it will almost certainly require more code than this approach. I thought that we had dismissed it a while ago for some reason? http://codereview.chromium.org/8203005/diff/3004/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_downloads_api.h (right): http://codereview.chromium.org/8203005/diff/3004/chrome/browser/extensions/ex... chrome/browser/extensions/extension_downloads_api.h:270: // Observes a single DownloadItem and dispatches onChanged events. On 2011/10/12 18:46:30, rdsmith wrote: > Might want a comment here as to why it's not a mix-in on the base class. I'm > almost tempted to suggest you do that instead; I think fewer classes is better > than more, and it'd save you some of the inter-class interaction at the cost of > having to keep a more complex datastructure of the last state seen. Done.
I'm finding myself wondering a bit about keeping a JSONified copy of all the DownloadItems around when the full list (with history & etc.) can grow without bound. If I were certain that entries from the history couldn't ever fire changed notifications, I'd suggest you look into keeping the full information only for the in-progress ones. But I think even history DIs can fire changed notifications, specifically if their file goes away because of user action and we notice. So I guess I'd just ask you to think about this problem in the back of your mind (maybe do some back of the envelope calculations about how much memory we're going to use?) and see if we can come up with a solution for it long term. Beyond that, and with the suggested changes below, LGTM (and you could probably argue me out of the changes below if you wanted to). http://codereview.chromium.org/8203005/diff/3004/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_downloads_api.cc (right): http://codereview.chromium.org/8203005/diff/3004/chrome/browser/extensions/ex... chrome/browser/extensions/extension_downloads_api.cc:441: void ExtensionDownloadsEventRouter::ItemObserver::OnDownloadUpdated( On 2011/10/13 16:30:29, b s h wrote: > On 2011/10/12 18:46:30, rdsmith wrote: > > This amount of work on each DownloadItem updated strengthens my interest in > > getting rid of updates on size changes and shifting over to polling. Is that > on > > your agenda? My sense is that we've agreed we want to do it but that no one's > > taken ownership yet, but if that's wrong I want to know :-}. > > This isn't quite as much work as it looks like. The for-loop is over 10-20 > fields, this whole method only does a few small mallocs, and there's just a dash > of string processing in delta->Set(), which is almost never called more than 3 > times per OnDownloadUpdated(). > If you're talking about polling for bytesReceived changes only, then yes. I > think it makes good API sense to dispatch an event for other changes both at the > extension level and the DownloadItem::Observer level. Yep, that's what I meant. > Shifting DownloadItem over to requiring its observers to poll for bytesReceived > events sounds to me like part of the refactoring task slated for next quarter. I > think that we can wire up the new ui as it is now, but, yes, not dispatching > bytesReceived will be a Good Thing. Hmmm. Ok, I'll put it on the list. Could you put some performance tests in at some point for the extensions so that we have some idea whether/how much we'll be in pain from doing the bytesReceived dispatch? My memory of the code is that we've got a gratuitous doubling of messages (we both have a timer loop that fires off events, and we update on every size change explicitly). > > We might alternatively push determining the delta down into the DownloadItem > > class and have OnDownloadUpdated include an argument indicating which fields > > have updated. That'd save this level of deltaing. WDYT? > > I think that having DownloadItem tell its observers what changed is the Right > Thing To Do, even though it will almost certainly require more code than this > approach. I thought that we had dismissed it a while ago for some reason? Yeah; I think I was just being resistant to widespread code changes. I think given that this code isn't that many lines, I'll put it on the list for the future. I don't actually want to send a delta from the base object (though I'm open to the argument), but at the moment, at least, I'm open to saying "this is why we're doing the update". Note that that updating will end up using a symbol for each (conceptual) field that's updated (based on my shoot-without-removing-gun-from-holster as to the design), which we might usefully combine with the symbols I asked you for in the other CL (to refer to the DownloadItem fields for querying). http://codereview.chromium.org/8203005/diff/6001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_downloads_api.cc (right): http://codereview.chromium.org/8203005/diff/6001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_downloads_api.cc:434: return; I feel a bit uncomfortable about keeping a DownloadItem* pointer around after getting a REMOVING notification. I suspect that the code'll get rid of it in the ModelChanged() event, but this notification is a specific request to let go of any pointers to the item we might have, and I think it'll produce less fragile code to do it here. http://codereview.chromium.org/8203005/diff/6001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_downloads_api.h (right): http://codereview.chromium.org/8203005/diff/6001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_downloads_api.h:273: typedef std::set<int> DownloadIdSet; This doesn't look like it's used in this file--should it be in the .cc file?
PTAL WDYT of the SimpleDownloadItem idea below for killing the cached jsons? http://codereview.chromium.org/8203005/diff/3004/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_downloads_api.cc (right): http://codereview.chromium.org/8203005/diff/3004/chrome/browser/extensions/ex... chrome/browser/extensions/extension_downloads_api.cc:441: void ExtensionDownloadsEventRouter::ItemObserver::OnDownloadUpdated( On 2011/10/13 23:42:59, rdsmith wrote: > On 2011/10/13 16:30:29, b s h wrote: > > On 2011/10/12 18:46:30, rdsmith wrote: > > > This amount of work on each DownloadItem updated strengthens my interest in > > > getting rid of updates on size changes and shifting over to polling. Is > that > > on > > > your agenda? My sense is that we've agreed we want to do it but that no > one's > > > taken ownership yet, but if that's wrong I want to know :-}. > > > > This isn't quite as much work as it looks like. The for-loop is over 10-20 > > fields, this whole method only does a few small mallocs, and there's just a > dash > > of string processing in delta->Set(), which is almost never called more than 3 > > times per OnDownloadUpdated(). > > If you're talking about polling for bytesReceived changes only, then yes. I > > think it makes good API sense to dispatch an event for other changes both at > the > > extension level and the DownloadItem::Observer level. > > Yep, that's what I meant. > > > Shifting DownloadItem over to requiring its observers to poll for > bytesReceived > > events sounds to me like part of the refactoring task slated for next quarter. > I > > think that we can wire up the new ui as it is now, but, yes, not dispatching > > bytesReceived will be a Good Thing. > > Hmmm. Ok, I'll put it on the list. Could you put some performance tests in at > some point for the extensions so that we have some idea whether/how much we'll > be in pain from doing the bytesReceived dispatch? My memory of the code is that > we've got a gratuitous doubling of messages (we both have a timer loop that > fires off events, and we update on every size change explicitly). I just added a uma stat to count the number of OnDownloadUpdated calls that do/don't trigger a downloads.onChanged event. (2 enum buckets) I'll put an AI on my whiteboard for writing a better perf test. > > > > We might alternatively push determining the delta down into the DownloadItem > > > class and have OnDownloadUpdated include an argument indicating which fields > > > have updated. That'd save this level of deltaing. WDYT? > > > > I think that having DownloadItem tell its observers what changed is the Right > > Thing To Do, even though it will almost certainly require more code than this > > approach. I thought that we had dismissed it a while ago for some reason? > > Yeah; I think I was just being resistant to widespread code changes. I think > given that this code isn't that many lines, I'll put it on the list for the > future. I don't actually want to send a delta from the base object (though I'm > open to the argument), but at the moment, at least, I'm open to saying "this is > why we're doing the update". > > Note that that updating will end up using a symbol for each (conceptual) field > that's updated (based on my shoot-without-removing-gun-from-holster as to the > design), which we might usefully combine with the symbols I asked you for in the > other CL (to refer to the DownloadItem fields for querying). We could also resurrect the SimpleDownloadItem, but this time as part of the DownloadItem[Interface] hierarchy. DownloadItemInterface -> SimpleDownloadItem -> DownloadItem So, SimpleDownloadItem could be a dumb copyable struct-of-POD that would just be a cheaper version of the cached jsons. We could also merge SimpleDownloadItem with DownloadPersistentStoreInfo. Things to think about for another CL. http://codereview.chromium.org/8203005/diff/6001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_downloads_api.cc (right): http://codereview.chromium.org/8203005/diff/6001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_downloads_api.cc:434: return; On 2011/10/13 23:42:59, rdsmith wrote: > I feel a bit uncomfortable about keeping a DownloadItem* pointer around after > getting a REMOVING notification. I suspect that the code'll get rid of it in > the ModelChanged() event, but this notification is a specific request to let go > of any pointers to the item we might have, and I think it'll produce less > fragile code to do it here. Done. http://codereview.chromium.org/8203005/diff/6001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_downloads_api.h (right): http://codereview.chromium.org/8203005/diff/6001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_downloads_api.h:273: typedef std::set<int> DownloadIdSet; On 2011/10/13 23:42:59, rdsmith wrote: > This doesn't look like it's used in this file--should it be in the .cc file? Done.
The new Download.OnChanged stat appears to be false (indicating a suppressed OnDownloadUpdated) at least as often as it is true. That's just for small safe quick files; larger/dangerous files will probably be somewhat higher. Would the stat be more useful as a pair of counters [suppressions, fires], recorded when the item is removed from the EDER either for onErased or ~EDER? This would require another item map in EDER, and might be laggy for users who never close chrome or clear downloads.
+Chris in case you're interested.
On 2011/10/17 20:21:22, b s h wrote: > The new Download.OnChanged stat appears to be false (indicating a suppressed > OnDownloadUpdated) at least as often as it is true. That's just for small safe > quick files; larger/dangerous files will probably be somewhat higher. Would the > stat be more useful as a pair of counters [suppressions, fires], recorded when > the item is removed from the EDER either for onErased or ~EDER? This would > require another item map in EDER, and might be laggy for users who never close > chrome or clear downloads. If you'd like to do something like that I'm ok with it, but I don't have a strong urge on my own account. I sorta figure that what you've done gives us actionable information--i.e. we should look at why we're sending gratuitous OnDownloadUpdated notifications (and I don't think we'd have to look very hard). If we took care of the low hanging fruit and still were getting gratuitous notifications, being able to split things out by size of download could be useful.
http://codereview.chromium.org/8203005/diff/3004/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_downloads_api.cc (right): http://codereview.chromium.org/8203005/diff/3004/chrome/browser/extensions/ex... chrome/browser/extensions/extension_downloads_api.cc:441: void ExtensionDownloadsEventRouter::ItemObserver::OnDownloadUpdated( On 2011/10/17 19:14:00, b s h wrote: > I just added a uma stat to count the number of OnDownloadUpdated calls that > do/don't trigger a downloads.onChanged event. (2 enum buckets) > I'll put an AI on my whiteboard for writing a better perf test. Sounds good; thanks. > We could also resurrect the SimpleDownloadItem, but this time as part of the > DownloadItem[Interface] hierarchy. > DownloadItemInterface -> SimpleDownloadItem -> DownloadItem > So, SimpleDownloadItem could be a dumb copyable struct-of-POD that would just be > a cheaper version of the cached jsons. We could also merge SimpleDownloadItem > with DownloadPersistentStoreInfo. > Things to think about for another CL. Possibly. My shoot from the hip reaction is that it's not worthwhile making the hierarchy more complex to save a constant factor--the thing I'm really worried about is having to keep extra bytes around on a per-history entry basis. But we're already doing that, so maybe reducing the total amount would be worth it? If we could combine it with DownloadPersistentStoreInfo I'd be in favor and my only issue would be whether it was the best place to put our energies. (I have a dream of combining DownloadPersistentStoreInfo and DownloadCreateInfo too, though that's probably not practical. Still, it makes some sense to me that "amount of information needed to start a download" would be approximately the same as "amount of information needed to restart a download", and the latter is where DPSI is going. http://codereview.chromium.org/8203005/diff/19001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_downloads_api.cc (right): http://codereview.chromium.org/8203005/diff/19001/chrome/browser/extensions/e... chrome/browser/extensions/extension_downloads_api.cc:443: ExtensionDownloadsEventRouter::OnChangedStat::OnChangedStat() I don't have a strong feeling on this, but I think it'd be a better code complexity/information tradeoff to just record the number of times you get an OnDownloadUpdated and the number of times you fire an OnChanged event. It doesn't show you the distribution across downloads, but as mentioned, that's not vital information; just the fact that we're getting the gratuitous notifications is the primary information we need. http://codereview.chromium.org/8203005/diff/19001/content/browser/download/do... File content/browser/download/download_stats.cc (right): http://codereview.chromium.org/8203005/diff/19001/content/browser/download/do... content/browser/download/download_stats.cc:234: UMA_HISTOGRAM_PERCENTAGE("Download.OnChanged", percent); I'd make this name a bit more detailed (SubstantiveUpdatedNotificationPercent?)
I took a very brief look. Why is the extension browser test disabled for onChanged?
On 2011/10/19 19:51:02, cbentzel wrote: > I took a very brief look. > > Why is the extension browser test disabled for onChanged? It depends on http://codereview.chromium.org/8060042/ which causes the downloads to actually be added to the history_downloads_ map, which is necessary for the ExtensionDownloadsEventRouter to see the downloads.
On 2011/10/19 19:51:02, cbentzel wrote: > I took a very brief look. > > Why is the extension browser test disabled for onChanged? It depends on http://codereview.chromium.org/8060042/ which causes the downloads to actually be added to the history_downloads_ map, which is necessary for the ExtensionDownloadsEventRouter to see the downloads.
So, this CL was going to wait for the apitest to be re-enabled, but it sounds like that might take a while. How would you feel about getting this in now so that it doesn't rot and take up space on my task list?
On 2011/12/01 19:25:59, b s h wrote: > So, this CL was going to wait for the apitest to be re-enabled, but it sounds > like that might take a while. > How would you feel about getting this in now so that it doesn't rot and take up > space on my task list? Might take a while because Asanka's busy with the Safebrowsing UI stuff? I'm hopeful he'll free up next week. I'd really prefer not to commit changes that don't have corresponding tests. If you're waiting on Asanka, you could also take over the test work from him and get that in.
PTAL. The apitest is re-enabled for win, linux.
Remind me of the context here; was this just pushed aside by higher priority items, or was there something specific you were waiting on? And is the apitest you mention the one that Chris tried to enable and had to revert? If so, what was the gotcha? http://codereview.chromium.org/8203005/diff/40001/chrome/browser/download/dow... File chrome/browser/download/download_extension_api.cc (right): http://codereview.chromium.org/8203005/diff/40001/chrome/browser/download/dow... chrome/browser/download/download_extension_api.cc:719: // notice its absence in ModelChanged() and fire an onErased. Idea (i.e. the notch below "suggestion"; just something I'd like you to think about): The obvious alternative to this is to fire the OnErased from here. The argument to fire the OnErased() from ModelChanged() is pretty clear; it's where you have to fire the OnCreated() from, and that's a natural symmetry. But if you fire the OnErased from here, you can keep the downloads_ data structure cleaner, because you won't have the special meaning of "NULL". Given the choice between clean procedural code and clean data structures, I lean towards clean data structures, as the data structures need to be utilized and understood from multiple different sections of the code, and putting a comment in ModelChanged() (when a download is erased) pointing here mostly deals with the dangers of the unclean code. http://codereview.chromium.org/8203005/diff/40001/chrome/browser/download/dow... chrome/browser/download/download_extension_api.cc:737: if (new_json->Get(*key, &new_value) && So this isn't performance critical code, but (:-}): Did you consider using a DictionaryValue::Iterator to drop this from O(n log n) to O(n) (I'm presuming the Get is log n). There isn't a lot of value in doing it the more performing way, but I don't see the argument on the other side; I think the code would still be as clear. http://codereview.chromium.org/8203005/diff/40001/chrome/browser/download/dow... chrome/browser/download/download_extension_api.cc:783: DownloadIdSet new_set; // current_set - prev_set; semi-nit: I think this section would be clearer if you separated out the "construction" of the two sets. They're basically identical, and a hypothetical reader reading through the code only really needs to read through the C++ syntax for creating a new set that's the different of two old sets once. It might even be worthwhile creating a static helper, and turning this into two lines worth of code. http://codereview.chromium.org/8203005/diff/40001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/40001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:52: // TODO(benjhayden): Test onErased using remove(). Why not as part of this CL, since it fires the OnErased() notification?
On 2012/01/15 18:47:13, rdsmith wrote: > Remind me of the context here; was this just pushed aside by higher priority > items, or was there something specific you were waiting on? And is the apitest > you mention the one that Chris tried to enable and had to revert? If so, what > was the gotcha? This cl depended on re-enabling the apitest, which depended on cancel(). Chris re-enabled the apitest on linux and windows, and is still hunting the problem on mac afaik. > > http://codereview.chromium.org/8203005/diff/40001/chrome/browser/download/dow... > File chrome/browser/download/download_extension_api.cc (right): > > http://codereview.chromium.org/8203005/diff/40001/chrome/browser/download/dow... > chrome/browser/download/download_extension_api.cc:719: // notice its absence in > ModelChanged() and fire an onErased. > Idea (i.e. the notch below "suggestion"; just something I'd like you to think > about): The obvious alternative to this is to fire the OnErased from here. The > argument to fire the OnErased() from ModelChanged() is pretty clear; it's where > you have to fire the OnCreated() from, and that's a natural symmetry. But if > you fire the OnErased from here, you can keep the downloads_ data structure > cleaner, because you won't have the special meaning of "NULL". Given the choice > between clean procedural code and clean data structures, I lean towards clean > data structures, as the data structures need to be utilized and understood from > multiple different sections of the code, and putting a comment in ModelChanged() > (when a download is erased) pointing here mostly deals with the dangers of the > unclean code. > > http://codereview.chromium.org/8203005/diff/40001/chrome/browser/download/dow... > chrome/browser/download/download_extension_api.cc:737: if (new_json->Get(*key, > &new_value) && > So this isn't performance critical code, but (:-}): Did you consider using a > DictionaryValue::Iterator to drop this from O(n log n) to O(n) (I'm presuming > the Get is log n). There isn't a lot of value in doing it the more performing > way, but I don't see the argument on the other side; I think the code would > still be as clear. > > http://codereview.chromium.org/8203005/diff/40001/chrome/browser/download/dow... > chrome/browser/download/download_extension_api.cc:783: DownloadIdSet new_set; > // current_set - prev_set; > semi-nit: I think this section would be clearer if you separated out the > "construction" of the two sets. They're basically identical, and a hypothetical > reader reading through the code only really needs to read through the C++ syntax > for creating a new set that's the different of two old sets once. > > It might even be worthwhile creating a static helper, and turning this into two > lines worth of code. > > http://codereview.chromium.org/8203005/diff/40001/chrome/test/data/extensions... > File chrome/test/data/extensions/api_test/downloads/test.js (right): > > http://codereview.chromium.org/8203005/diff/40001/chrome/test/data/extensions... > chrome/test/data/extensions/api_test/downloads/test.js:52: // TODO(benjhayden): > Test onErased using remove(). > Why not as part of this CL, since it fires the OnErased() notification?
PTAL I think I've fixed the race conditions in the test on mac, but I'll re-disable it on mac if the waterfall disagrees. http://codereview.chromium.org/8203005/diff/40001/chrome/browser/download/dow... File chrome/browser/download/download_extension_api.cc (right): http://codereview.chromium.org/8203005/diff/40001/chrome/browser/download/dow... chrome/browser/download/download_extension_api.cc:719: // notice its absence in ModelChanged() and fire an onErased. On 2012/01/15 18:47:13, rdsmith wrote: > Idea (i.e. the notch below "suggestion"; just something I'd like you to think > about): The obvious alternative to this is to fire the OnErased from here. The > argument to fire the OnErased() from ModelChanged() is pretty clear; it's where > you have to fire the OnCreated() from, and that's a natural symmetry. But if > you fire the OnErased from here, you can keep the downloads_ data structure > cleaner, because you won't have the special meaning of "NULL". Given the choice > between clean procedural code and clean data structures, I lean towards clean > data structures, as the data structures need to be utilized and understood from > multiple different sections of the code, and putting a comment in ModelChanged() > (when a download is erased) pointing here mostly deals with the dangers of the > unclean code. Done. http://codereview.chromium.org/8203005/diff/40001/chrome/browser/download/dow... chrome/browser/download/download_extension_api.cc:737: if (new_json->Get(*key, &new_value) && On 2012/01/15 18:47:13, rdsmith wrote: > So this isn't performance critical code, but (:-}): Did you consider using a > DictionaryValue::Iterator to drop this from O(n log n) to O(n) (I'm presuming > the Get is log n). There isn't a lot of value in doing it the more performing > way, but I don't see the argument on the other side; I think the code would > still be as clear. Done. http://codereview.chromium.org/8203005/diff/40001/chrome/browser/download/dow... chrome/browser/download/download_extension_api.cc:783: DownloadIdSet new_set; // current_set - prev_set; On 2012/01/15 18:47:13, rdsmith wrote: > semi-nit: I think this section would be clearer if you separated out the > "construction" of the two sets. They're basically identical, and a hypothetical > reader reading through the code only really needs to read through the C++ syntax > for creating a new set that's the different of two old sets once. > > It might even be worthwhile creating a static helper, and turning this into two > lines worth of code. erased_set isn't necessary now that the onErased is fired in OnDownloadUpdated(). http://codereview.chromium.org/8203005/diff/40001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/40001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:52: // TODO(benjhayden): Test onErased using remove(). On 2012/01/15 18:47:13, rdsmith wrote: > Why not as part of this CL, since it fires the OnErased() notification? I need remove() in order to test onErased. I can't test onErased until I can erase an item.
You've added a lot of tests at the .js level, but those tests only test very basic things about the UI. My understanding was that our pattern was to add functionality tests at the extensions api test level--shouldn't there be such tests for OnChanged() there? (Or at the .js level; I'm not picky.) I've noted this in a couple of places, but: I'd suggest you go through this and think about places where comments might be useful for smart people who don't know the code well. It strikes me as weak in comment density. http://codereview.chromium.org/8203005/diff/40001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/40001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:52: // TODO(benjhayden): Test onErased using remove(). On 2012/02/01 21:42:40, b s h wrote: > On 2012/01/15 18:47:13, rdsmith wrote: > > Why not as part of this CL, since it fires the OnErased() notification? > > I need remove() in order to test onErased. I can't test onErased until I can > erase an item. *snort* Right you are. Who knew? :-} (I.e. that's obvious enough I shouldn't have bothered you with it. Thanks for the explanation.) http://codereview.chromium.org/8203005/diff/66001/chrome/browser/download/dow... File chrome/browser/download/download_extension_api.cc (right): http://codereview.chromium.org/8203005/diff/66001/chrome/browser/download/dow... chrome/browser/download/download_extension_api.cc:911: if (item->GetState() == DownloadItem::REMOVING) { The code would be enhanced by a few comments indicating what each block of code did. If someone doesn't know the Observer interface fairly well, they might be a bit lost. http://codereview.chromium.org/8203005/diff/66001/chrome/browser/download/dow... chrome/browser/download/download_extension_api.cc:931: if (iter.key() != kBytesReceivedKey) { My inclination would be to have changes in BytesReceived show up in the delta but simple not result in an event dispatch. Can you give me a sense as to why you prefer it not to show up (if you do)? http://codereview.chromium.org/8203005/diff/66001/chrome/browser/download/dow... chrome/browser/download/download_extension_api.cc:935: !iter.value().Equals(old_value))) { This looks like it'll leave out cases in which we've dropped keys in the old->new transition. We probably don't have any such cases now, but it's an interface assumption that might get broken silently in the future; we should either comment it or handle the case. http://codereview.chromium.org/8203005/diff/66001/chrome/browser/download/dow... chrome/browser/download/download_extension_api.cc:955: void ExtensionDownloadsEventRouter::ModelChanged() { Again, I think just one or two comments would help this routine to call out the set creation, then finding the set difference in order to dispach the events. http://codereview.chromium.org/8203005/diff/66001/chrome/browser/download/dow... chrome/browser/download/download_extension_api.cc:988: DispatchEvent(extension_event_names::kOnDownloadCreated, item->DeepCopy()); I think it's worthwhile having a comment in this routine somewhere (at end?) indicating why there's not a parallel Download destroyed event dispatch. (I actually mentioned this parenthetically when I suggested the code change.) http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:59: "in test N."); Is there really no way for us to check for this programmatically? That's worlds better than relying on people to look at the test output--after you check this in, that's not going to happen unless one of us suspects a problem. (And if we can check for it programmatically, then we can pull the console logs.) http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:61: chrome.test.runTests([ Could you put in a short comment about what these tests are testing and why? They end up looking pretty sparse reading through them, which I presume is because we're testing the real functionality using the C++ tests, but a comment explaining that would be a win. http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:194: // slashes with underscores). I'd suggest you have a plan sketch (and maybe a bug id) for this before you commit.
= Elaborating on the race conditions in the apitest = == The importance of callbackAdded == The chrome.test JS test framework loops through the functions passed to chrome.test.runTests(). When each function finishes, it looks at how many callbacks are pending. You call chrome.test.callbackAdded() to tell it that a new callback is pending, that function returns a callback (named e.g. callbackComplete) that decrements the count of pending callbacks. When there are no more pending callbacks, and chrome.test.fail() has not been called, then chrome.test declares that function a SUCCESS. chrome.test.callbackAdded() and its callbackCompleted is wrapped by chrome.test.callback(), chrome.test.callbackPass(), chrome.test.callbackFail(), and chrome.test.listenOnce(), but we can call callbackAdded() directly if we need more flexibility. If there's more than one callback in a test function, and any of the callbacks are not surrounded by callbackAdded/callbackComplete, then it's possible for chrome.test's callback count to drop to zero before all your callbacks are called. If that happens, then the test might be declared a success before it's finished, and the next test will start. If an uncounted callback in test N starts a counted callback (calling callbackAdded()) while test N+1 is running, then the pending callback count will seem to jump around, which may trip assertions in chrome.test. == The importance of specificity in event listeners == The onCreated listener in downloadInterrupted() cannot be in a chrome.test.listenOnce() because it is possible that the onCreated events for the items from the previous test functions will not fire until downloadInterrupted begins. You may see the onCreated event listener in downloadInterrupted() log "id: 0", "id: 1", etc. If downloadInterrupted.createdListener called cancel(download_id) the first time that it's called, the DownloadItem for downloadInterrupted.download_id might not have been created yet, so the cancel() will fail and that download's state will never change to 'interrupted' and downloadInterrupted.changedListener will never call its changedCompleted() callback. Event listeners must be careful to ignore events for downloads owned by other test functions.
Please hold the review while I debug more changes to the apitest. I'll say PTAL when ready. I couldn't figure out how to test events in C++. http://codereview.chromium.org/8203005/diff/66001/chrome/browser/download/dow... File chrome/browser/download/download_extension_api.cc (right): http://codereview.chromium.org/8203005/diff/66001/chrome/browser/download/dow... chrome/browser/download/download_extension_api.cc:911: if (item->GetState() == DownloadItem::REMOVING) { On 2012/02/02 18:10:07, rdsmith wrote: > The code would be enhanced by a few comments indicating what each block of code > did. If someone doesn't know the Observer interface fairly well, they might be > a bit lost. Done. http://codereview.chromium.org/8203005/diff/66001/chrome/browser/download/dow... chrome/browser/download/download_extension_api.cc:931: if (iter.key() != kBytesReceivedKey) { On 2012/02/02 18:10:07, rdsmith wrote: > My inclination would be to have changes in BytesReceived show up in the delta > but simple not result in an event dispatch. Can you give me a sense as to why > you prefer it not to show up (if you do)? API purity. If I write an onChanged listener without reading the docs carefully, and I notice the bytesReceived field there, then I might think that onChanged is a good way to listen for bytesReceived events, and wonder why I don't receive them very often, and I'd probably file a bug. If the only way to find bytesReceived changes is the correct way (polling search()), then I'll definitely do it the correct way. http://codereview.chromium.org/8203005/diff/66001/chrome/browser/download/dow... chrome/browser/download/download_extension_api.cc:935: !iter.value().Equals(old_value))) { On 2012/02/02 18:10:07, rdsmith wrote: > This looks like it'll leave out cases in which we've dropped keys in the > old->new transition. We probably don't have any such cases now, but it's an > interface assumption that might get broken silently in the future; we should > either comment it or handle the case. Good catch! http://codereview.chromium.org/8203005/diff/66001/chrome/browser/download/dow... chrome/browser/download/download_extension_api.cc:955: void ExtensionDownloadsEventRouter::ModelChanged() { On 2012/02/02 18:10:07, rdsmith wrote: > Again, I think just one or two comments would help this routine to call out the > set creation, then finding the set difference in order to dispach the events. Done. http://codereview.chromium.org/8203005/diff/66001/chrome/browser/download/dow... chrome/browser/download/download_extension_api.cc:988: DispatchEvent(extension_event_names::kOnDownloadCreated, item->DeepCopy()); On 2012/02/02 18:10:07, rdsmith wrote: > I think it's worthwhile having a comment in this routine somewhere (at end?) > indicating why there's not a parallel Download destroyed event dispatch. (I > actually mentioned this parenthetically when I suggested the code change.) Done. http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:59: "in test N."); On 2012/02/02 18:10:07, rdsmith wrote: > Is there really no way for us to check for this programmatically? That's worlds > better than relying on people to look at the test output--after you check this > in, that's not going to happen unless one of us suspects a problem. > > (And if we can check for it programmatically, then we can pull the console > logs.) chrome.test already tests it programmatically by asserting that the pending callback counter doesn't jump around. However, the race conditions mean that, if you forget to surround a callback with callbackAdded()/callbackCompleted(), then sometimes those assertions will pass and sometimes they'll fail with a less-than-helpful error message. If every callback log()s, then it's much easier to find the untracked callback. http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:61: chrome.test.runTests([ On 2012/02/02 18:10:07, rdsmith wrote: > Could you put in a short comment about what these tests are testing and why? > They end up looking pretty sparse reading through them, which I presume is > because we're testing the real functionality using the C++ tests, but a comment > explaining that would be a win. Um, you might want to have a look at download_extension_test.cc and see what features you think should be tested somewhere and aren't. Comments done. http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:194: // slashes with underscores). On 2012/02/02 18:10:07, rdsmith wrote: > I'd suggest you have a plan sketch (and maybe a bug id) for this before you > commit. Added the bug id, and Asanka has a vague plan if I understand him correctly. I'd rather not write the plan of attack here because it might change, and that kind of discussion seems to belong in the bug thread.
PTAL Not sure why the mac bot can't compile, but my test is passing on linux and windows.
Looks pretty good; I think we're almost done. http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:59: "in test N."); On 2012/02/02 20:40:38, b s h wrote: > On 2012/02/02 18:10:07, rdsmith wrote: > > Is there really no way for us to check for this programmatically? That's > worlds > > better than relying on people to look at the test output--after you check this > > in, that's not going to happen unless one of us suspects a problem. > > > > (And if we can check for it programmatically, then we can pull the console > > logs.) > > chrome.test already tests it programmatically by asserting that the pending > callback counter doesn't jump around. > However, the race conditions mean that, if you forget to surround a callback > with callbackAdded()/callbackCompleted(), then sometimes those assertions will > pass and sometimes they'll fail with a less-than-helpful error message. If every > callback log()s, then it's much easier to find the untracked callback. Would it satisfy your goals to change the above two console.log's to in-file comments and leave the logging below? http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:61: chrome.test.runTests([ On 2012/02/02 20:40:38, b s h wrote: > On 2012/02/02 18:10:07, rdsmith wrote: > > Could you put in a short comment about what these tests are testing and why? > > They end up looking pretty sparse reading through them, which I presume is > > because we're testing the real functionality using the C++ tests, but a > comment > > explaining that would be a win. > > Um, you might want to have a look at download_extension_test.cc and see what > features you think should be tested somewhere and aren't. I think I didn't read as closely as I should have. It looks ok modulo my questions about method and headers elsewhere. > Comments done. http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:194: // slashes with underscores). On 2012/02/02 20:40:38, b s h wrote: > On 2012/02/02 18:10:07, rdsmith wrote: > > I'd suggest you have a plan sketch (and maybe a bug id) for this before you > > commit. > > Added the bug id, and Asanka has a vague plan if I understand him correctly. I'd > rather not write the plan of attack here because it might change, and that kind > of discussion seems to belong in the bug thread. Yep; if there's a bug id I agree the discussion should go in the bug. Thanks. http://codereview.chromium.org/8203005/diff/66004/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/66004/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:106: chrome.test.assertEq(download_id, id); Do we have a test confirming that we get the correct body? It seems like this test would pass if the method arg was dropped on the floor at some point. This same comment applies to downloadHeader as well.
PTAL Chris, you too, please, especially for testserver.py. http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:59: "in test N."); On 2012/02/03 20:02:00, rdsmith wrote: > On 2012/02/02 20:40:38, b s h wrote: > > On 2012/02/02 18:10:07, rdsmith wrote: > > > Is there really no way for us to check for this programmatically? That's > > worlds > > > better than relying on people to look at the test output--after you check > this > > > in, that's not going to happen unless one of us suspects a problem. > > > > > > (And if we can check for it programmatically, then we can pull the console > > > logs.) > > > > chrome.test already tests it programmatically by asserting that the pending > > callback counter doesn't jump around. > > However, the race conditions mean that, if you forget to surround a callback > > with callbackAdded()/callbackCompleted(), then sometimes those assertions will > > pass and sometimes they'll fail with a less-than-helpful error message. If > every > > callback log()s, then it's much easier to find the untracked callback. > > Would it satisfy your goals to change the above two console.log's to in-file > comments and leave the logging below? I think that if I saw test output as verbose as this without knowing why, then I'd probably delete all the print statements before reading all the comments in the file in case any of them explain why I shouldn't. I can turn these logs into comments if you think I'm the only one who would do that. http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:61: chrome.test.runTests([ On 2012/02/03 20:02:00, rdsmith wrote: > On 2012/02/02 20:40:38, b s h wrote: > > On 2012/02/02 18:10:07, rdsmith wrote: > > > Could you put in a short comment about what these tests are testing and why? > > > > They end up looking pretty sparse reading through them, which I presume is > > > because we're testing the real functionality using the C++ tests, but a > > comment > > > explaining that would be a win. > > > > Um, you might want to have a look at download_extension_test.cc and see what > > features you think should be tested somewhere and aren't. > > I think I didn't read as closely as I should have. It looks ok modulo my > questions about method and headers elsewhere. > > > Comments done. > Done. http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:194: // slashes with underscores). On 2012/02/03 20:02:00, rdsmith wrote: > On 2012/02/02 20:40:38, b s h wrote: > > On 2012/02/02 18:10:07, rdsmith wrote: > > > I'd suggest you have a plan sketch (and maybe a bug id) for this before you > > > commit. > > > > Added the bug id, and Asanka has a vague plan if I understand him correctly. > I'd > > rather not write the plan of attack here because it might change, and that > kind > > of discussion seems to belong in the bug thread. > > Yep; if there's a bug id I agree the discussion should go in the bug. Thanks. Done. http://codereview.chromium.org/8203005/diff/66004/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/66004/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:106: chrome.test.assertEq(download_id, id); On 2012/02/03 20:02:00, rdsmith wrote: > Do we have a test confirming that we get the correct body? It seems like this > test would pass if the method arg was dropped on the floor at some point. This > same comment applies to downloadHeader as well. Done.
On 2012/02/10 18:53:07, b s h wrote: > PTAL > > Chris, you too, please, especially for testserver.py. We discussed this in person, but for the record: Don't check in the changes to FileHandler. The reason this was needed was because your test exposed an already-known issue with the download system and how it handles 4xx/5xx response codes. For the PostOnlyFileHandler, it seems reasonable to validate that the data matches expectations. However, I'd just make an expected_post_body=<foo> where foo is a base64 encoded version of the expected body. More general than the path matching. You may also want to have it return a 404 or similar code instead of returning False, which passes the buck to the next handler in the list. Another option is to simply reflect the post-body in the response and have the validation done in the test, not sure if that's acceptable in this case. > > http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... > File chrome/test/data/extensions/api_test/downloads/test.js (right): > > http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... > chrome/test/data/extensions/api_test/downloads/test.js:59: "in test N."); > On 2012/02/03 20:02:00, rdsmith wrote: > > On 2012/02/02 20:40:38, b s h wrote: > > > On 2012/02/02 18:10:07, rdsmith wrote: > > > > Is there really no way for us to check for this programmatically? That's > > > worlds > > > > better than relying on people to look at the test output--after you check > > this > > > > in, that's not going to happen unless one of us suspects a problem. > > > > > > > > (And if we can check for it programmatically, then we can pull the console > > > > logs.) > > > > > > chrome.test already tests it programmatically by asserting that the pending > > > callback counter doesn't jump around. > > > However, the race conditions mean that, if you forget to surround a callback > > > with callbackAdded()/callbackCompleted(), then sometimes those assertions > will > > > pass and sometimes they'll fail with a less-than-helpful error message. If > > every > > > callback log()s, then it's much easier to find the untracked callback. > > > > Would it satisfy your goals to change the above two console.log's to in-file > > comments and leave the logging below? > > I think that if I saw test output as verbose as this without knowing why, then > I'd probably delete all the print statements before reading all the comments in > the file in case any of them explain why I shouldn't. I can turn these logs into > comments if you think I'm the only one who would do that. > > http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... > chrome/test/data/extensions/api_test/downloads/test.js:61: > chrome.test.runTests([ > On 2012/02/03 20:02:00, rdsmith wrote: > > On 2012/02/02 20:40:38, b s h wrote: > > > On 2012/02/02 18:10:07, rdsmith wrote: > > > > Could you put in a short comment about what these tests are testing and > why? > > > > > > They end up looking pretty sparse reading through them, which I presume is > > > > because we're testing the real functionality using the C++ tests, but a > > > comment > > > > explaining that would be a win. > > > > > > Um, you might want to have a look at download_extension_test.cc and see what > > > features you think should be tested somewhere and aren't. > > > > I think I didn't read as closely as I should have. It looks ok modulo my > > questions about method and headers elsewhere. > > > > > Comments done. > > > > Done. > > http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... > chrome/test/data/extensions/api_test/downloads/test.js:194: // slashes with > underscores). > On 2012/02/03 20:02:00, rdsmith wrote: > > On 2012/02/02 20:40:38, b s h wrote: > > > On 2012/02/02 18:10:07, rdsmith wrote: > > > > I'd suggest you have a plan sketch (and maybe a bug id) for this before > you > > > > commit. > > > > > > Added the bug id, and Asanka has a vague plan if I understand him correctly. > > I'd > > > rather not write the plan of attack here because it might change, and that > > kind > > > of discussion seems to belong in the bug thread. > > > > Yep; if there's a bug id I agree the discussion should go in the bug. Thanks. > > Done. > > http://codereview.chromium.org/8203005/diff/66004/chrome/test/data/extensions... > File chrome/test/data/extensions/api_test/downloads/test.js (right): > > http://codereview.chromium.org/8203005/diff/66004/chrome/test/data/extensions... > chrome/test/data/extensions/api_test/downloads/test.js:106: > chrome.test.assertEq(download_id, id); > On 2012/02/03 20:02:00, rdsmith wrote: > > Do we have a test confirming that we get the correct body? It seems like this > > test would pass if the method arg was dropped on the floor at some point. > This > > same comment applies to downloadHeader as well. > > Done.
On 2012/02/10 20:47:37, cbentzel wrote: > On 2012/02/10 18:53:07, b s h wrote: > > PTAL > > > > Chris, you too, please, especially for testserver.py. > > We discussed this in person, but for the record: > > Don't check in the changes to FileHandler. The reason this was needed was > because your test exposed an already-known issue with the download system and > how it handles 4xx/5xx response codes. > > For the PostOnlyFileHandler, it seems reasonable to validate that the data > matches expectations. However, I'd just make an expected_post_body=<foo> where > foo is a base64 encoded version of the expected body. More general than the path > matching. You may also want to have it return a 404 or similar code instead of > returning False, which passes the buck to the next handler in the list. > > Another option is to simply reflect the post-body in the response and have the > validation done in the test, not sure if that's acceptable in this case. PTAL.
Ignore the comment about the "What was the problem" - saw it earlier but it's a pain to discard existing drafts. http://codereview.chromium.org/8203005/diff/83002/chrome/browser/download/dow... File chrome/browser/download/download_extension_apitest.cc (right): http://codereview.chromium.org/8203005/diff/83002/chrome/browser/download/dow... chrome/browser/download/download_extension_apitest.cc:29: IN_PROC_BROWSER_TEST_F(DownloadsApiTest, Downloads) { Did you fix this for OSX? What was the problem? http://codereview.chromium.org/8203005/diff/83002/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/83002/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:52: console.log("I know all these console.logs stink up the place, but please " + Will these actually show up on the output of all bot runs? That's generally considered unacceptable.
http://codereview.chromium.org/8203005/diff/83002/chrome/browser/download/dow... File chrome/browser/download/download_extension_apitest.cc (right): http://codereview.chromium.org/8203005/diff/83002/chrome/browser/download/dow... chrome/browser/download/download_extension_apitest.cc:29: IN_PROC_BROWSER_TEST_F(DownloadsApiTest, Downloads) { On 2012/02/10 21:06:31, cbentzel wrote: > Did you fix this for OSX? What was the problem? It seems to be working on mac trybots. I fixed some races in the apitest, so maybe that was it, or maybe it will fail on the mac waterfall bots, in which case I'll redisable it. http://codereview.chromium.org/8203005/diff/83002/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/83002/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:52: console.log("I know all these console.logs stink up the place, but please " + On 2012/02/10 21:06:31, cbentzel wrote: > Will these actually show up on the output of all bot runs? That's generally > considered unacceptable. Done.
All comments are on the api test. I did not review the implementation of onChanged. http://codereview.chromium.org/8203005/diff/84004/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/84004/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:383: // Test that download() requires a valid url. This comment isn't really needed. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:45: const SAFE_FAST_URL = getURL('slow?0'); Looks like const is disallowed by Javascript style guide. http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... Since this is Chrome-only it may not be an issue, but I don't see any overrides on dev.chromium.org. I actually like this over var, so if you find a counter-example let's go with it. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:47: const POST_URL = 'files/post/downloads/a_zip_file.zip?expected_body=BODY' Why don't you use getURL here like the others? http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:86: const download_id = getNextId(); Nit: camelCase used instead of hacker_style http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:91: if ((delta.id == download_id) && Nit: General style is to do if (delta.id != download_id) return; if (!delta.state || (delta.state.new != downloads.STATE_COMPLETE)) return; to prevent lots o' indentation. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:94: chrome.test.assertEq(downloads.STATE_COMPLETE, delta.state.new); This assert is uneeded - just tested it in the conditional above. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:99: const kExpectedSize = 164; Should be EXPECTED_SIZE in javascript style. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:104: downloads.onChanged.removeListener(changedListener); Cool that this works when being called from within the listener's callback. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:120: function downloadPostWouldFailWithoutMethod() { Why is this test needed? Why wouldn't downloadPostSuccess fail if we tried to do a GET rather than a POST? http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:132: ((delta.state.new == downloads.STATE_INTERRUPTED) || Why do you have the INTERRUPTED case here? It doesn't currently interrupt. You could also point to crbug.com/112342 which is the same root problem. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:184: downloads.download( I don't think this is testing what you want it to test. For example, you aren't providing a body argument to downloads.download. Again, if we correctly treated a 4xx response as a download interruption, then downloadPostSuccess would not work correctly if the POST body were not included, because the PostHandler would reject the request. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:194: // Test the |headers| download option. How is this validating that the headers are actually being propagated through? http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:205: function downloadInterrupted() { Would be nice to have a test which validates that this works when the server dies or some other case that doesn't come from a cancellation. Doesn't need to happen now. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:260: {"url": getURL("slow?0")}, SAFE_FAST_URL
LGTM with one comment below. http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:59: "in test N."); On 2012/02/10 18:53:07, b s h wrote: > On 2012/02/03 20:02:00, rdsmith wrote: > > On 2012/02/02 20:40:38, b s h wrote: > > > On 2012/02/02 18:10:07, rdsmith wrote: > > > > Is there really no way for us to check for this programmatically? That's > > > worlds > > > > better than relying on people to look at the test output--after you check > > this > > > > in, that's not going to happen unless one of us suspects a problem. > > > > > > > > (And if we can check for it programmatically, then we can pull the console > > > > logs.) > > > > > > chrome.test already tests it programmatically by asserting that the pending > > > callback counter doesn't jump around. > > > However, the race conditions mean that, if you forget to surround a callback > > > with callbackAdded()/callbackCompleted(), then sometimes those assertions > will > > > pass and sometimes they'll fail with a less-than-helpful error message. If > > every > > > callback log()s, then it's much easier to find the untracked callback. > > > > Would it satisfy your goals to change the above two console.log's to in-file > > comments and leave the logging below? > > I think that if I saw test output as verbose as this without knowing why, then > I'd probably delete all the print statements before reading all the comments in > the file in case any of them explain why I shouldn't. I can turn these logs into > comments if you think I'm the only one who would do that. It looks like you've nuked the logging; was that in response to this comment thread? I didn't see other comment threads on this topic. (Note that I'm not objecting--I'm happier without the logging. Just confused.) http://codereview.chromium.org/8203005/diff/79004/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/79004/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:135: // why 4XX HTTP errors are not considered interruptions. Could you make sure we have a bug filed on this appropriately with a reference to it here?
PTAL http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/66001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:59: "in test N."); On 2012/02/13 16:13:08, rdsmith wrote: > On 2012/02/10 18:53:07, b s h wrote: > > On 2012/02/03 20:02:00, rdsmith wrote: > > > On 2012/02/02 20:40:38, b s h wrote: > > > > On 2012/02/02 18:10:07, rdsmith wrote: > > > > > Is there really no way for us to check for this programmatically? > That's > > > > worlds > > > > > better than relying on people to look at the test output--after you > check > > > this > > > > > in, that's not going to happen unless one of us suspects a problem. > > > > > > > > > > (And if we can check for it programmatically, then we can pull the > console > > > > > logs.) > > > > > > > > chrome.test already tests it programmatically by asserting that the > pending > > > > callback counter doesn't jump around. > > > > However, the race conditions mean that, if you forget to surround a > callback > > > > with callbackAdded()/callbackCompleted(), then sometimes those assertions > > will > > > > pass and sometimes they'll fail with a less-than-helpful error message. If > > > every > > > > callback log()s, then it's much easier to find the untracked callback. > > > > > > Would it satisfy your goals to change the above two console.log's to in-file > > > comments and leave the logging below? > > > > I think that if I saw test output as verbose as this without knowing why, then > > I'd probably delete all the print statements before reading all the comments > in > > the file in case any of them explain why I shouldn't. I can turn these logs > into > > comments if you think I'm the only one who would do that. > > It looks like you've nuked the logging; was that in response to this comment > thread? I didn't see other comment threads on this topic. > > (Note that I'm not objecting--I'm happier without the logging. Just confused.) Chris said that it was generally not acceptable. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:45: const SAFE_FAST_URL = getURL('slow?0'); On 2012/02/10 22:32:52, cbentzel wrote: > Looks like const is disallowed by Javascript style guide. > > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... > > Since this is Chrome-only it may not be an issue, but I don't see any overrides > on http://dev.chromium.org. > > I actually like this over var, so if you find a counter-example let's go with > it. http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/resources/shar... http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:47: const POST_URL = 'files/post/downloads/a_zip_file.zip?expected_body=BODY' On 2012/02/10 22:32:52, cbentzel wrote: > Why don't you use getURL here like the others? Done. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:86: const download_id = getNextId(); On 2012/02/10 22:32:52, cbentzel wrote: > Nit: camelCase used instead of hacker_style Done. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:91: if ((delta.id == download_id) && On 2012/02/10 22:32:52, cbentzel wrote: > Nit: General style is to do > > if (delta.id != download_id) > return; > if (!delta.state || (delta.state.new != downloads.STATE_COMPLETE)) > return; > > to prevent lots o' indentation. Done. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:94: chrome.test.assertEq(downloads.STATE_COMPLETE, delta.state.new); On 2012/02/10 22:32:52, cbentzel wrote: > This assert is uneeded - just tested it in the conditional above. Done. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:99: const kExpectedSize = 164; On 2012/02/10 22:32:52, cbentzel wrote: > Should be EXPECTED_SIZE in javascript style. Done. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:104: downloads.onChanged.removeListener(changedListener); On 2012/02/10 22:32:52, cbentzel wrote: > Cool that this works when being called from within the listener's callback. Done. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:120: function downloadPostWouldFailWithoutMethod() { On 2012/02/10 22:32:52, cbentzel wrote: > Why is this test needed? Why wouldn't downloadPostSuccess fail if we tried to do > a GET rather than a POST? Done. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:132: ((delta.state.new == downloads.STATE_INTERRUPTED) || On 2012/02/10 22:32:52, cbentzel wrote: > Why do you have the INTERRUPTED case here? It doesn't currently interrupt. > > You could also point to crbug.com/112342 which is the same root problem. Done. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:184: downloads.download( On 2012/02/10 22:32:52, cbentzel wrote: > I don't think this is testing what you want it to test. For example, you aren't > providing a body argument to downloads.download. > > Again, if we correctly treated a 4xx response as a download interruption, then > downloadPostSuccess would not work correctly if the POST body were not included, > because the PostHandler would reject the request. Done. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:194: // Test the |headers| download option. On 2012/02/10 22:32:52, cbentzel wrote: > How is this validating that the headers are actually being propagated through? Done. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:205: function downloadInterrupted() { On 2012/02/10 22:32:52, cbentzel wrote: > Would be nice to have a test which validates that this works when the server > dies or some other case that doesn't come from a cancellation. Doesn't need to > happen now. Done. http://codereview.chromium.org/8203005/diff/83008/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:260: {"url": getURL("slow?0")}, On 2012/02/10 22:32:52, cbentzel wrote: > SAFE_FAST_URL Done. http://codereview.chromium.org/8203005/diff/79004/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/79004/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:135: // why 4XX HTTP errors are not considered interruptions. On 2012/02/13 16:13:08, rdsmith wrote: > Could you make sure we have a bug filed on this appropriately with a reference > to it here? Done.
http://codereview.chromium.org/8203005/diff/86003/net/tools/testserver/testse... File net/tools/testserver/testserver.py (right): http://codereview.chromium.org/8203005/diff/86003/net/tools/testserver/testse... net/tools/testserver/testserver.py:914: _, _, _, _, query_str, _ = urlparse.urlparse(self.path) Feels like expected_headers and expected_body should move into _FileHandlerHelper. May need to change how the body gets consumed here and PostOnlyFileHandler to make that work. Another thought for these tests - could you just use the EchoAllHandler instead? Might need to modify it to report the method used.
http://codereview.chromium.org/8203005/diff/86003/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/86003/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:45: const SAFE_FAST_URL = getURL('slow?0'); Avoid 'const', here and elsewhere. See http://crbug.com/80149 (not really a big deal for test code, but is a style violation.) http://codereview.chromium.org/8203005/diff/86003/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:304: return; Nit: Check delta.error.new? http://codereview.chromium.org/8203005/diff/86003/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:347: (delta.filename.new.indexOf(FILENAME) == -1)) Is .filename intended to be the target (final) path? or can it be the intermediate path while the download is in progress? You might be succeeding here when we see the path to FILENAME.crdownload. Perhaps that is okay. The extension API doc is (at least to me) ambiguous on the precise meaning of .filename. Also .filename is actually a path.
PTAL http://codereview.chromium.org/8203005/diff/86003/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/86003/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:45: const SAFE_FAST_URL = getURL('slow?0'); On 2012/02/13 19:45:47, asanka wrote: > Avoid 'const', here and elsewhere. See http://crbug.com/80149 (not really a big > deal for test code, but is a style violation.) Done. The style guide suggested that the reason to avoid const is that it is unsupported on IE, which is irrelevant here. Perhaps it should also mention that it slows down V8, which is counter-intuitive. http://codereview.chromium.org/8203005/diff/86003/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:304: return; On 2012/02/13 19:45:47, asanka wrote: > Nit: Check delta.error.new? Done. http://codereview.chromium.org/8203005/diff/86003/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:347: (delta.filename.new.indexOf(FILENAME) == -1)) On 2012/02/13 19:45:47, asanka wrote: > Is .filename intended to be the target (final) path? or can it be the > intermediate path while the download is in progress? You might be succeeding > here when we see the path to FILENAME.crdownload. Perhaps that is okay. > > The extension API doc is (at least to me) ambiguous on the precise meaning of > .filename. Also .filename is actually a path. Yeah, there might be some more discussion to be had about |filename| before launching, but not for this CL, especially when we don't know if search() or onChanged will ever be launched... http://codereview.chromium.org/8203005/diff/86003/net/tools/testserver/testse... File net/tools/testserver/testserver.py (right): http://codereview.chromium.org/8203005/diff/86003/net/tools/testserver/testse... net/tools/testserver/testserver.py:914: _, _, _, _, query_str, _ = urlparse.urlparse(self.path) On 2012/02/13 19:23:38, cbentzel wrote: > Feels like expected_headers and expected_body should move into > _FileHandlerHelper. May need to change how the body gets consumed here and > PostOnlyFileHandler to make that work. Done. > > Another thought for these tests - could you just use the EchoAllHandler instead? > Might need to modify it to report the method used. The downloads extension api does not provide any way to access the contents of downloaded files. I'd rather not depend on an unrelated API such as the HTML5 File api unless absolutely necessary in case that API changes, and so that I don't have to figure out how.
LGTM I'd consider re-enabling the DownloadsApiTest.Downloads test on OSX in a separate CL, so if it leads to hangs a revert would only impact that instead of do collateral damage on onChanged. http://codereview.chromium.org/8203005/diff/91003/net/tools/testserver/testse... File net/tools/testserver/testserver.py (right): http://codereview.chromium.org/8203005/diff/91003/net/tools/testserver/testse... net/tools/testserver/testserver.py:930: if expected_body and request_body not in expected_body: if request_body not in expected_body: should be sufficient.
Splitting re-enabling the apitest on mac out into a separate CL momentarily. http://codereview.chromium.org/8203005/diff/91003/net/tools/testserver/testse... File net/tools/testserver/testserver.py (right): http://codereview.chromium.org/8203005/diff/91003/net/tools/testserver/testse... net/tools/testserver/testserver.py:930: if expected_body and request_body not in expected_body: On 2012/02/13 21:58:01, cbentzel wrote: > if request_body not in expected_body: should be sufficient. There are tests that do not specify expected_body, so expected_body will be []. No possible request_body is in [], so this code would return 404 to those tests instead of succeeding.
Still LGTM http://codereview.chromium.org/8203005/diff/91003/net/tools/testserver/testse... File net/tools/testserver/testserver.py (right): http://codereview.chromium.org/8203005/diff/91003/net/tools/testserver/testse... net/tools/testserver/testserver.py:929: expected_body = query_dict.get('expected_body', []) Should the expected_body argument be base64 encoded? Perhaps too much effort now. http://codereview.chromium.org/8203005/diff/91003/net/tools/testserver/testse... net/tools/testserver/testserver.py:930: if expected_body and request_body not in expected_body: On 2012/02/13 22:05:51, b s h wrote: > On 2012/02/13 21:58:01, cbentzel wrote: > > if request_body not in expected_body: should be sufficient. > > There are tests that do not specify expected_body, so expected_body will be []. > No possible request_body is in [], so this code would return 404 to those tests > instead of succeeding. Yeah, my mistake. You could do expected_body = query_dict.get('expected_body', [request_body]) but that would probably be less clear than this.
LGTM. http://codereview.chromium.org/8203005/diff/88015/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/88015/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:52: 'expected_body=BODY'); Nit: indentation
Thanks, everybody! I'll hit the CQ when the trybots come back, then I'll mail out http://codereview.chromium.org/9392013/ http://codereview.chromium.org/8203005/diff/91003/net/tools/testserver/testse... File net/tools/testserver/testserver.py (right): http://codereview.chromium.org/8203005/diff/91003/net/tools/testserver/testse... net/tools/testserver/testserver.py:929: expected_body = query_dict.get('expected_body', []) On 2012/02/14 00:58:46, cbentzel wrote: > Should the expected_body argument be base64 encoded? Perhaps too much effort > now. If somebody has a need for it, they can add it. http://codereview.chromium.org/8203005/diff/91003/net/tools/testserver/testse... net/tools/testserver/testserver.py:930: if expected_body and request_body not in expected_body: On 2012/02/14 00:58:46, cbentzel wrote: > On 2012/02/13 22:05:51, b s h wrote: > > On 2012/02/13 21:58:01, cbentzel wrote: > > > if request_body not in expected_body: should be sufficient. > > > > There are tests that do not specify expected_body, so expected_body will be > []. > > No possible request_body is in [], so this code would return 404 to those > tests > > instead of succeeding. > > Yeah, my mistake. > > You could do > > expected_body = query_dict.get('expected_body', [request_body]) > > but that would probably be less clear than this. It doesn't sound like there's a strong preference, so I'll leave it how it is. http://codereview.chromium.org/8203005/diff/88015/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/8203005/diff/88015/chrome/test/data/extensions... chrome/test/data/extensions/api_test/downloads/test.js:52: 'expected_body=BODY'); On 2012/02/14 15:37:22, asanka wrote: > Nit: indentation Done.
On 2012/02/14 15:52:11, b s h wrote: > Thanks, everybody! > > I'll hit the CQ when the trybots come back, then I'll mail out > http://codereview.chromium.org/9392013/ You know that with the proper git magic you can mail out new CLs that are dependent on CLs that haven't landed yet? (With the diffs only showing the changes since the base CL?) Ping me if that isn't a tool in your toolbox and you'd like it to be. > > http://codereview.chromium.org/8203005/diff/91003/net/tools/testserver/testse... > File net/tools/testserver/testserver.py (right): > > http://codereview.chromium.org/8203005/diff/91003/net/tools/testserver/testse... > net/tools/testserver/testserver.py:929: expected_body = > query_dict.get('expected_body', []) > On 2012/02/14 00:58:46, cbentzel wrote: > > Should the expected_body argument be base64 encoded? Perhaps too much effort > > now. > > If somebody has a need for it, they can add it. > > http://codereview.chromium.org/8203005/diff/91003/net/tools/testserver/testse... > net/tools/testserver/testserver.py:930: if expected_body and request_body not in > expected_body: > On 2012/02/14 00:58:46, cbentzel wrote: > > On 2012/02/13 22:05:51, b s h wrote: > > > On 2012/02/13 21:58:01, cbentzel wrote: > > > > if request_body not in expected_body: should be sufficient. > > > > > > There are tests that do not specify expected_body, so expected_body will be > > []. > > > No possible request_body is in [], so this code would return 404 to those > > tests > > > instead of succeeding. > > > > Yeah, my mistake. > > > > You could do > > > > expected_body = query_dict.get('expected_body', [request_body]) > > > > but that would probably be less clear than this. > > It doesn't sound like there's a strong preference, so I'll leave it how it is. > > http://codereview.chromium.org/8203005/diff/88015/chrome/test/data/extensions... > File chrome/test/data/extensions/api_test/downloads/test.js (right): > > http://codereview.chromium.org/8203005/diff/88015/chrome/test/data/extensions... > chrome/test/data/extensions/api_test/downloads/test.js:52: > 'expected_body=BODY'); > On 2012/02/14 15:37:22, asanka wrote: > > Nit: indentation > > Done.
It doesn't depend on this CL in terms of the code. I just wanted to keep myself from accidentally committing that change before this one makes it all the way through. And I have two other CLs waiting for review. My CLs still change a lot during review, so I'm not sure that such magic would actually speed things up. On Tue, Feb 14, 2012 at 10:55 AM, <rdsmith@chromium.org> wrote: > On 2012/02/14 15:52:11, b s h wrote: > >> Thanks, everybody! >> > > I'll hit the CQ when the trybots come back, then I'll mail out >> http://codereview.chromium.**org/9392013/<http://codereview.chromium.org/9392... >> > > You know that with the proper git magic you can mail out new CLs that are > dependent on CLs that haven't landed yet? (With the diffs only showing the > changes since the base CL?) Ping me if that isn't a tool in your toolbox > and > you'd like it to be. > > > > > http://codereview.chromium.**org/8203005/diff/91003/net/** > tools/testserver/testserver.py<http://codereview.chromium.org/8203005/diff/91003/net/tools/testserver/testserver.py> > >> File net/tools/testserver/**testserver.py (right): >> > > > http://codereview.chromium.**org/8203005/diff/91003/net/** > tools/testserver/testserver.**py#newcode929<http://codereview.chromium.org/8203005/diff/91003/net/tools/testserver/testserver.py#newcode929> > >> net/tools/testserver/**testserver.py:929: expected_body = >> query_dict.get('expected_body'**, []) >> On 2012/02/14 00:58:46, cbentzel wrote: >> > Should the expected_body argument be base64 encoded? Perhaps too much >> effort >> > now. >> > > If somebody has a need for it, they can add it. >> > > > http://codereview.chromium.**org/8203005/diff/91003/net/** > tools/testserver/testserver.**py#newcode930<http://codereview.chromium.org/8203005/diff/91003/net/tools/testserver/testserver.py#newcode930> > >> net/tools/testserver/**testserver.py:930: if expected_body and >> request_body not >> > in > >> expected_body: >> On 2012/02/14 00:58:46, cbentzel wrote: >> > On 2012/02/13 22:05:51, b s h wrote: >> > > On 2012/02/13 21:58:01, cbentzel wrote: >> > > > if request_body not in expected_body: should be sufficient. >> > > >> > > There are tests that do not specify expected_body, so expected_body >> will >> > be > >> > []. >> > > No possible request_body is in [], so this code would return 404 to >> those >> > tests >> > > instead of succeeding. >> > >> > Yeah, my mistake. >> > >> > You could do >> > >> > expected_body = query_dict.get('expected_body'**, [request_body]) >> > >> > but that would probably be less clear than this. >> > > It doesn't sound like there's a strong preference, so I'll leave it how >> it is. >> > > > http://codereview.chromium.**org/8203005/diff/88015/chrome/** > test/data/extensions/api_test/**downloads/test.js<http://codereview.chromium.org/8203005/diff/88015/chrome/test/data/extensions/api_test/downloads/test.js> > >> File chrome/test/data/extensions/**api_test/downloads/test.js (right): >> > > > http://codereview.chromium.**org/8203005/diff/88015/chrome/** > test/data/extensions/api_test/**downloads/test.js#newcode52<http://codereview.chromium.org/8203005/diff/88015/chrome/test/data/extensions/api_test/downloads/test.js#newcode52> > >> chrome/test/data/extensions/**api_test/downloads/test.js:52: >> 'expected_body=BODY'); >> On 2012/02/14 15:37:22, asanka wrote: >> > Nit: indentation >> > > Done. >> > > > > http://codereview.chromium.**org/8203005/<http://codereview.chromium.org/8203... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/8203005/95001
Change committed as 121912 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
