Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11574006:
Implement chrome.downloads.onDeterminingFilename() (Closed)
|
Created:
8 years ago by benjhayden Modified:
7 years, 9 months ago Reviewers:
asanka, jochen (gone - plz use gerrit), Randy Smith (Not in Mondays), Matt Perry, vabr (Chromium), mkearney1, battre CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement chrome.downloads.onDeterminingFilename() to allow extensions to participate in the download filename determination process.
Docs staged: http://basho.cam.corp.google.com:8000/extensions/downloads.html#event-onDeterminingFilename
Example:
chrome.downloads.onDeterminingFilename.addListener(function(item, suggest) {
suggest({filename: item.filename, overwrite: true});
});
chrome.downloads.onDeterminingFilename.addListener(function(item, suggest) {
window.setTimeout(function() {
suggest({filename: item.mime.split('/')[0] + '/' + item.filename, overwrite: false});
}, 1);
return true; // handling asynchronously
});
BUG=12133
BUG=68108
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185811
Patch Set 1 : @r176065 #
Total comments: 59
Patch Set 2 : @r176347 #
Total comments: 8
Patch Set 3 : @r176665 #
Total comments: 39
Patch Set 4 : @r177190 #
Total comments: 11
Patch Set 5 : @r177662 #
Total comments: 19
Patch Set 6 : @r178299 #
Total comments: 15
Patch Set 7 : @r180415 #
Total comments: 33
Patch Set 8 : @r183850 #Patch Set 9 : @r183850 #
Total comments: 27
Patch Set 10 : @r183850 #
Total comments: 2
Patch Set 11 : @r183850 #Patch Set 12 : @r184166 #
Total comments: 1
Patch Set 13 : @r184427 #Patch Set 14 : @r185012 #
Total comments: 16
Patch Set 15 : @r185012 #Patch Set 16 : @r185591 #Patch Set 17 : @r185787 #Patch Set 18 : @r185803 #Messages
Total messages: 66 (0 generated)
Still not sure if it will be possible to test any of this, but it seems to be working.
Dominic, Vaclac: would you mind looking over downloads_api.cc, downloads_custom_bindings.js, and related files? I tried to re-interpret some of the ideas from the webRequest API, but I'm not sure if there are any edge-cases that my interpretation misses.
Randy: Should DownloadPrefs::IsDownloadPathManaged() prevent extensions from playing a part in filename determination?
Hi Ben, I looked at the files where I left comments. Your question, the edge cases: I could not find any. As far as I understood (feel free to correct me), the protocol seems to be rather simple -- listeners provided by extensions see all download file names being created (so, e.g., no filtering on the URL names), on conflict there is one winner beating others, and there are no guarantees on who this winner is. This seems safe to me. Maybe Dominic recalls some more pitfalls from designing the listener logic for Web Request? Also, if some listener blocks, does that mean that the file cannot be downloaded? How is that presented to the user? Thanks, Vaclav https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:985: // to determine which extension will finish last, so users are advised to try to How precisely are the users advised? Using the ExtensionWarning class? https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1000: DownloadServiceFactory::GetForProfile(profile)->GetExtensionEventRouter(); +2 spaces of indentation https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1016: DownloadServiceFactory::GetForProfile(profile)->GetExtensionEventRouter(); +2 spaces of indentation https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1046: ExtensionDownloadsEventRouterData::Get(item); +2 spaces of indentation https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1071: extensions::UnloadedExtensionInfo>(details)->extension; +2 spaces of indent https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1086: iter = empty_determiners.begin(); +4 spaces of indent nit: please break after "=", not before "iter" https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1131: ExtensionDownloadsEventRouterData::Get(item); +2 spaces of indent https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1137: determiner_iter = filename_determiners_.begin(); +4 spaces nit: please break after "=" https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api.h (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.h:282: typedef std::set<std::string> ExtensionIdSet; According to http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order the typedefs should be first in the private section. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.h:283: typedef base::hash_map<std::string, ExtensionIdSet> DeterminerMap; The lint says: "Add #include for hash_map<>" (I used to think that '? errors' in the Lint column means that linting was broken, but apparently it shows something :).) https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... File chrome/browser/extensions/api/downloads_internal/downloads_internal_api.cc (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads_internal/downloads_internal_api.cc:102: DownloadsInternalDetermineFilenameFunction:: nit: Insert a blank line before the c-tor and d-tor definition. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... File chrome/browser/extensions/api/downloads_internal/downloads_internal_api.h (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads_internal/downloads_internal_api.h:11: : public AsyncExtensionFunction { nit: I would guess +2 more spaces here and on lines 25 and 39, but I might be wrong and the style guide does not mention this. Feel free to ignore this comment. https://codereview.chromium.org/11574006/diff/14053/chrome/renderer/extension... File chrome/renderer/extensions/downloads_custom_bindings.cc (right): https://codereview.chromium.org/11574006/diff/14053/chrome/renderer/extension... chrome/renderer/extensions/downloads_custom_bindings.cc:25: DCHECK(args.Length() == 0); Lint says: "Consider using DCHECK_EQ instead of DCHECK(a == b)" https://codereview.chromium.org/11574006/diff/14053/chrome/renderer/extension... File chrome/renderer/extensions/downloads_custom_bindings.h (right): https://codereview.chromium.org/11574006/diff/14053/chrome/renderer/extension... chrome/renderer/extensions/downloads_custom_bindings.h:20: DISALLOW_COPY_AND_ASSIGN(DownloadsCustomBindings ); Lint says: "Extra space before )" https://codereview.chromium.org/11574006/diff/14053/chrome/renderer/resources... File chrome/renderer/resources/extensions/downloads_custom_bindings.js (right): https://codereview.chromium.org/11574006/diff/14053/chrome/renderer/resources... chrome/renderer/resources/extensions/downloads_custom_bindings.js:37: // Test if any callbacks are registered fur thus event. possibly a typo: "fur thus"
I did a very high-level overview. I feel that some of the function names could be improved (see my comments below). The only major difference I see compared to the WebRequest API implementation is that we made sure that we handle multiple conflicting modifications in a deterministic order. Let me know if you would like me to conduct an in depth review. Best regards, Dominic https://codereview.chromium.org/11574006/diff/14053/chrome/browser/download/c... File chrome/browser/download/chrome_download_manager_delegate.cc (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.cc:669: ~ContinueFilenameDeterminationInfo() {} nit: +2 spaces https://codereview.chromium.org/11574006/diff/14053/chrome/browser/download/c... File chrome/browser/download/chrome_download_manager_delegate.h (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.h:144: bool visited_referrer_before; This and the following parameter are unclear to me without having much background in the download manager. Up to you whether you want to add comments. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.h:209: ContinueFilenameDeterminationInfo continue_info, pass structs as const ref? https://codereview.chromium.org/11574006/diff/14053/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.h:218: ContinueFilenameDeterminationInfo continue_info, pass structs as const ref? https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:184: if (filename.size() < 1) nit: why not if (filename.empty()) ? https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:568: void AddDeterminer(const std::string& determiner_id) { How about "AddPendingDeterminer"? https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:572: bool DetermineFilename( How about "DeterminerCallback" https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:579: determiners_.erase(iter); This looks strange. DetermineFilename should not modify the object |this| based on my judgement of the filename. As mentioned in download_api_internal.cc I think that "DetermineFilename" is not a good name. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:590: } This is "Last callback wins" semantic. In the WebRequest API we tried to be sure to be deterministic by enforcing that the last installed extension that wants to modify the request takes precedence. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api.h (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.h:208: static bool AddFilenameDeterminer( These functions need descriptions. What are FilenameDeterminers? What are the parameters? What are the return values? https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... File chrome/browser/extensions/api/downloads_internal/downloads_internal_api.cc (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads_internal/downloads_internal_api.cc:106: DetermineFilenameParams; nit: whitespaces https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads_internal/downloads_internal_api.cc:116: return ExtensionDownloadsEventRouter::DetermineFilename( Why is this called "DetermineFilename" instead of "DeterminedFilename"? The extension has determined the filename, right? https://codereview.chromium.org/11574006/diff/14053/chrome/renderer/resources... File chrome/renderer/resources/extensions/downloads_custom_bindings.js (right): https://codereview.chromium.org/11574006/diff/14053/chrome/renderer/resources... chrome/renderer/resources/extensions/downloads_custom_bindings.js:62: if (typeof(result) == 'object' && result.filename) { only if you pass a filename, you allow to set overwrite to true? I think this should go into the API.
Thank you both so much for the prompt review! I have a few followup questions below. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/download/c... File chrome/browser/download/chrome_download_manager_delegate.cc (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.cc:669: ~ContinueFilenameDeterminationInfo() {} On 2013/01/11 14:29:37, battre wrote: > nit: +2 spaces Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/download/c... File chrome/browser/download/chrome_download_manager_delegate.h (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.h:144: bool visited_referrer_before; On 2013/01/11 14:29:37, battre wrote: > This and the following parameter are unclear to me without having much > background in the download manager. Up to you whether you want to add comments. I'll let Randy decide this. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.h:209: ContinueFilenameDeterminationInfo continue_info, On 2013/01/11 14:29:37, battre wrote: > pass structs as const ref? Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.h:218: ContinueFilenameDeterminationInfo continue_info, On 2013/01/11 14:29:37, battre wrote: > pass structs as const ref? Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:184: if (filename.size() < 1) On 2013/01/11 14:29:37, battre wrote: > nit: why not > if (filename.empty()) > ? Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:568: void AddDeterminer(const std::string& determiner_id) { On 2013/01/11 14:29:37, battre wrote: > How about "AddPendingDeterminer"? Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:572: bool DetermineFilename( On 2013/01/11 14:29:37, battre wrote: > How about "DeterminerCallback" Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:579: determiners_.erase(iter); On 2013/01/11 14:29:37, battre wrote: > This looks strange. DetermineFilename should not modify the object |this| based > on my judgement of the filename. As mentioned in download_api_internal.cc I > think that "DetermineFilename" is not a good name. This is the method that runs when any onDeterminingFilename listener returns. When called in response to the *last* listener returning, it needs to call either filename_no_change_ or filename_change_. I changed this method to detect the last listener using a flag for each listener. Is there a better way to detect when the last listener has returned? https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:590: } On 2013/01/11 14:29:37, battre wrote: > This is "Last callback wins" semantic. In the WebRequest API we tried to be sure > to be deterministic by enforcing that the last installed extension that wants to > modify the request takes precedence. Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:985: // to determine which extension will finish last, so users are advised to try to On 2013/01/11 12:43:40, vabr (Chromium) wrote: > How precisely are the users advised? Using the ExtensionWarning class? Oh, I was just going to make a note in downloads.idl, the developer documentation. ExtensionWarning says it puts a badge on the hotdog menu. I don't know if this situation is severe enough to bug users like that. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1000: DownloadServiceFactory::GetForProfile(profile)->GetExtensionEventRouter(); On 2013/01/11 12:43:40, vabr (Chromium) wrote: > +2 spaces of indentation Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1016: DownloadServiceFactory::GetForProfile(profile)->GetExtensionEventRouter(); On 2013/01/11 12:43:40, vabr (Chromium) wrote: > +2 spaces of indentation Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1046: ExtensionDownloadsEventRouterData::Get(item); On 2013/01/11 12:43:40, vabr (Chromium) wrote: > +2 spaces of indentation Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1071: extensions::UnloadedExtensionInfo>(details)->extension; On 2013/01/11 12:43:40, vabr (Chromium) wrote: > +2 spaces of indent Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1086: iter = empty_determiners.begin(); On 2013/01/11 12:43:40, vabr (Chromium) wrote: > +4 spaces of indent > nit: please break after "=", not before "iter" Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1131: ExtensionDownloadsEventRouterData::Get(item); On 2013/01/11 12:43:40, vabr (Chromium) wrote: > +2 spaces of indent Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1137: determiner_iter = filename_determiners_.begin(); On 2013/01/11 12:43:40, vabr (Chromium) wrote: > +4 spaces > nit: please break after "=" Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api.h (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.h:208: static bool AddFilenameDeterminer( On 2013/01/11 14:29:37, battre wrote: > These functions need descriptions. What are FilenameDeterminers? What are the > parameters? What are the return values? Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.h:282: typedef std::set<std::string> ExtensionIdSet; On 2013/01/11 12:43:40, vabr (Chromium) wrote: > According to > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order > the typedefs should be first in the private section. Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.h:283: typedef base::hash_map<std::string, ExtensionIdSet> DeterminerMap; On 2013/01/11 12:43:40, vabr (Chromium) wrote: > The lint says: "Add #include for hash_map<>" > > (I used to think that '? errors' in the Lint column means that linting was > broken, but apparently it shows something :).) I added base/hash_tables.h, but got a compiler warning when I added <hash_map> https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... File chrome/browser/extensions/api/downloads_internal/downloads_internal_api.cc (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads_internal/downloads_internal_api.cc:102: DownloadsInternalDetermineFilenameFunction:: On 2013/01/11 12:43:40, vabr (Chromium) wrote: > nit: Insert a blank line before the c-tor and d-tor definition. Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads_internal/downloads_internal_api.cc:106: DetermineFilenameParams; On 2013/01/11 14:29:37, battre wrote: > nit: whitespaces Done. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads_internal/downloads_internal_api.cc:116: return ExtensionDownloadsEventRouter::DetermineFilename( On 2013/01/11 14:29:37, battre wrote: > Why is this called "DetermineFilename" instead of "DeterminedFilename"? The > extension has determined the filename, right? Actually, there's still most of the filename determination process left to do, including prepending the default downloads directory, making the filename safe for windows (prepending "nul" with an underscore, changing "foo.lnk" to "foo.download", etc.), dangerous filename detection, and deduplication (inserting " (1)" before the extension), not to mention the fact that another extension may override with a different name. A more accurate name might be "ContinueFilenameDeterminationProcess", but that seemed a bit too long. Still open to alternative names. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... File chrome/browser/extensions/api/downloads_internal/downloads_internal_api.h (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads_internal/downloads_internal_api.h:11: : public AsyncExtensionFunction { On 2013/01/11 12:43:40, vabr (Chromium) wrote: > nit: I would guess +2 more spaces here and on lines 25 and 39, but I might be > wrong and the style guide does not mention this. Feel free to ignore this > comment. Done. https://codereview.chromium.org/11574006/diff/14053/chrome/renderer/extension... File chrome/renderer/extensions/downloads_custom_bindings.cc (right): https://codereview.chromium.org/11574006/diff/14053/chrome/renderer/extension... chrome/renderer/extensions/downloads_custom_bindings.cc:25: DCHECK(args.Length() == 0); On 2013/01/11 12:43:40, vabr (Chromium) wrote: > Lint says: "Consider using DCHECK_EQ instead of DCHECK(a == b)" Done. https://codereview.chromium.org/11574006/diff/14053/chrome/renderer/extension... File chrome/renderer/extensions/downloads_custom_bindings.h (right): https://codereview.chromium.org/11574006/diff/14053/chrome/renderer/extension... chrome/renderer/extensions/downloads_custom_bindings.h:20: DISALLOW_COPY_AND_ASSIGN(DownloadsCustomBindings ); On 2013/01/11 12:43:40, vabr (Chromium) wrote: > Lint says: "Extra space before )" Done. https://codereview.chromium.org/11574006/diff/14053/chrome/renderer/resources... File chrome/renderer/resources/extensions/downloads_custom_bindings.js (right): https://codereview.chromium.org/11574006/diff/14053/chrome/renderer/resources... chrome/renderer/resources/extensions/downloads_custom_bindings.js:37: // Test if any callbacks are registered fur thus event. On 2013/01/11 12:43:40, vabr (Chromium) wrote: > possibly a typo: "fur thus" Done. https://codereview.chromium.org/11574006/diff/14053/chrome/renderer/resources... chrome/renderer/resources/extensions/downloads_custom_bindings.js:62: if (typeof(result) == 'object' && result.filename) { On 2013/01/11 14:29:37, battre wrote: > only if you pass a filename, you allow to set overwrite to true? I think this > should go into the API. Done.
Only looked at chrome/browser/download/ https://codereview.chromium.org/11574006/diff/48002/chrome/browser/download/c... File chrome/browser/download/chrome_download_manager_delegate.cc (right): https://codereview.chromium.org/11574006/diff/48002/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.cc:728: // last_download_path/music/music/bar.mp3. I understand why this comment is here, but I think it's better if it was in OnExtensionsChangingFilename(). We are potentially overriding the path and filename based on the extension anyway. https://codereview.chromium.org/11574006/diff/48002/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.cc:772: filename_determined.Run(); Why not just call OnExtensionsDeterminedFilename()? Looks like the callback is only needed if going through the ExtensionDownloadsEventRouter. https://codereview.chromium.org/11574006/diff/48002/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.cc:785: if (!download || (download->GetState() != DownloadItem::IN_PROGRESS)) We need to invoke the DownloadTargetCallback regardless of whether the download is interrupted or not. https://codereview.chromium.org/11574006/diff/48002/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.cc:808: if (!download || (download->GetState() != DownloadItem::IN_PROGRESS)) As above.
https://codereview.chromium.org/11574006/diff/48002/chrome/browser/download/c... File chrome/browser/download/chrome_download_manager_delegate.cc (right): https://codereview.chromium.org/11574006/diff/48002/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.cc:728: // last_download_path/music/music/bar.mp3. On 2013/01/11 21:56:54, asanka wrote: > I understand why this comment is here, but I think it's better if it was in > OnExtensionsChangingFilename(). We are potentially overriding the path and > filename based on the extension anyway. Done. https://codereview.chromium.org/11574006/diff/48002/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.cc:772: filename_determined.Run(); On 2013/01/11 21:56:54, asanka wrote: > Why not just call OnExtensionsDeterminedFilename()? Looks like the callback is > only needed if going through the ExtensionDownloadsEventRouter. If OnExtensionsDeterminedFilename's signature changes, we only need to change the base::Bind, not 3 different places. https://codereview.chromium.org/11574006/diff/48002/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.cc:785: if (!download || (download->GetState() != DownloadItem::IN_PROGRESS)) On 2013/01/11 21:56:54, asanka wrote: > We need to invoke the DownloadTargetCallback regardless of whether the download > is interrupted or not. Why? That's not what SubstituteDriveDownloadPathCallback, OnPathReservationAvailable, and OnTargetPathDetermined do. Should I fix them? https://codereview.chromium.org/11574006/diff/48002/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.cc:808: if (!download || (download->GetState() != DownloadItem::IN_PROGRESS)) On 2013/01/11 21:56:54, asanka wrote: > As above. ditto.
Note that I didn't review, nor do I feel comfortable reviewing, the security aspects of setting up the new API--you should make sure someone else does that. Similarly, I looked at downloads_custom_bindings.js and concluded that what it was doing doesn't really have much to do with core downloads, and hence I wasn't the right reviewer. https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... File chrome/browser/download/chrome_download_manager_delegate.cc (right): https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.cc:788: changed_filename)); Where are we stripping out .. and similar? https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... File chrome/browser/download/chrome_download_manager_delegate.h (left): https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.h:244: scoped_ptr<ExtensionDownloadsEventRouter> extension_event_router_; So do you remember why we wanted to move EDER to DS? After having read through this CL, I'd be inclined to leave it on CDMD. Specifically, I don't see that anything outside of CDMD is currently referencing it (maybe we have plans?) and this CL means that CDMD needs to reference it, so minimal visibility would make it (from the C++ perspective) an implementation detail of CDMD. WDYT? https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... File chrome/browser/download/chrome_download_manager_delegate.h (right): https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.h:138: struct ContinueFilenameDeterminationInfo { References to this structure below are references :-}, so you just need a forward decl not a full defn. https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.h:222: I'm concerned about the names of these two functions and EDER::OnDownloadFilenameDetermined. (If you don't care about my philosophy and just want my request, skip to the last paragraph :-}). I think of "On*" as being either notifications or returns from async calls--i.e. when information is passed in without expectation of a response. In contrast, I think of action method names (DetermineDownloadFilename) as being used to initiate actions (that may or may not return results). I'm also uncomfortable with the second of the two names above, since it's used when the extension decides not to determine the filename and when it does and we've sanitized it. So I'd like to request something like: * EDER::DetermineDownloadFilename (or DetermineFilename, or something fancier) * CDMD::OnExtensionSpecifyingFilename * CDMD::OnFilenameDetermined (since it's not clear by who). WDYT? https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:190: (filename[1] == L'.')) Doesn't this allow for, e.g. "phantom_subdir/../../../../etc/passwd"? https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:582: bool overwrite) { How likely is it that we'll get a response we didn't expect from a different extension_id+sub_event_id? I'm wondering if it's reasonable just to count down responses here. https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1070: // The method by which extensions hook into the filename determination process nit: I'd find this comment easier to read if it had paragraph breaks between the three chunks of actions being done: The addition of the listener, the call out to the extensions, and the return from the extensions. (When I did my own summary, I actually ended up doing four, splitting the fan in from the EDERD -> CDMD callback.) https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1092: // conflict. If I read the code above, it's not the last one responding, it's the last one registered (because of the determiner_index_ check). Am I confused? https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1099: if (include_incognito && profile->HasOffTheRecordProfile()) { My confusion, your opportunity to enlighten :-}: If we're an extension attached to an incognito profile, we get visibility into the corresponding regular profile. The reverse is not true. So it makes sense to me that (elsewhere) we dispatch regular events to incognito EDERs. But this looks like we're installing listeners on incognito profiles in response to non-incognito requests, which is Bad. What am I missing? (I'd expect the bool to be called "include regular" and for the test to test that and whether the profile *is* incognito, not whether it has a corresponding incognito profile.) https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1100: AddFilenameDeterminer(profile->GetOffTheRecordProfile(), Ignoring return value? That at least should have a comment (also for other such calls). https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1110: return false; What is the larger implication about false returns from these functions? Doesn't it imply a bug in the internal downloads function? (Except for DetermineFilename, when the item could have been completed or something silly like that.) And if there are bugs in our code, even in Javascript, doesn't that deserve a DCHECK? https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1147: DownloadItem* item = GetDownloadIfInProgress( Is there any issue here with downloads resumption? E.g. an interrupt occurs while file determination is in progress, so we drop responses from the extensions on the floor, and then resume the download with a EDERD still attached to it in some intermediate state WRT responses from the extensions? My inclination would be to reset the EDERD when we transition out of IN_PROGRESS, but that may have gotchas I don't see. https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1180: iter = determiners_.erase(iter); What about download items that are waiting for responses from extensions? Do we need to cleanup their EDERD? This and the comment above makes me a little tempted to suggest you not use the UserData aspect of the download item but instead synchronize on a structure in the EDER based on download id. That'd allow you to not worry about what happens to the download item at all; you can just pass the result of all this back to the CDMD where it can decide whether to drop it on the floor all at once. If we don't do that, we're going to have to be thinking fairly carefully about races as we go through this code, and I think it'll be more fragile in the future too. https://codereview.chromium.org/11574006/diff/55003/chrome/common/extensions/... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/11574006/diff/55003/chrome/common/extensions/... chrome/common/extensions/api/downloads.idl:22: // is unset. Sorry, probably still confused about syntax, but doesn't this imply that the filename may be unset, and thus should have a ? after the type name? https://codereview.chromium.org/11574006/diff/55003/chrome/common/extensions/... chrome/common/extensions/api/downloads.idl:439: static void onDeterminingFilename(DownloadItem downloadItem); This is solidly and definitely me not understanding the syntax, but if this function can return something, should it be declared void?
I'll address Randy's comments tomorrow. Still working on incognito and browser testing, so please ignore the occams for now.
Hi Ben, LGTM with comments. Vaclav https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:985: // to determine which extension will finish last, so users are advised to try to On 2013/01/11 21:21:27, benjhayden_chromium wrote: > On 2013/01/11 12:43:40, vabr (Chromium) wrote: > > How precisely are the users advised? Using the ExtensionWarning class? > > Oh, I was just going to make a note in downloads.idl, the developer > documentation. > ExtensionWarning says it puts a badge on the hotdog menu. I don't know if this > situation is severe enough to bug users like that. The analogy in WebRequest is, e.g., when two extension give conflicting redirect instructions (see https://cs.corp.google.com/#chrome/src/chrome/browser/extensions/api/web_requ...). There the ExtensionWarning is used, and I think that might justify using it for clashing download renaming. From the user's point of view, the results of two conflicting extensions doing download renaming might be rather confusing, and we can't really expect them to read the developers' docs. Also, if it was fairly improbable for two extensions to clash on download renaming, then that would be one more reason to use ExtensionWarning and not to worry about its severity. However, I don't feel strongly about this. If Dominic, or another reviewer knowledgeable in extensions, is OK with emitting no warning, then I'm fine with that as well. https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api.h (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.h:283: typedef base::hash_map<std::string, ExtensionIdSet> DeterminerMap; On 2013/01/11 21:21:27, benjhayden_chromium wrote: > On 2013/01/11 12:43:40, vabr (Chromium) wrote: > > The lint says: "Add #include for hash_map<>" > > > > (I used to think that '? errors' in the Lint column means that linting was > > broken, but apparently it shows something :).) > > I added base/hash_tables.h, but got a compiler warning when I added <hash_map> That sounds right, base/hash_tables.h includes <hash_map> anyway, so the template hash_map<> will be defined in one of this file's #includes and lint will be happy. Happy about hash_map, that is, as now it complains about missing an #include declaring vector :).
https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/14053/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:985: // to determine which extension will finish last, so users are advised to try to On 2013/01/11 21:21:27, benjhayden_chromium wrote: > On 2013/01/11 12:43:40, vabr (Chromium) wrote: > > How precisely are the users advised? Using the ExtensionWarning class? > > Oh, I was just going to make a note in downloads.idl, the developer > documentation. > ExtensionWarning says it puts a badge on the hotdog menu. I don't know if this > situation is severe enough to bug users like that. Please don't add a warning badge for this. I think that is too intrusive. We introduced the warning badge because the WebRequest API commands can be critical for privacy/security reasons (e.g. deleting headers that disguise your identity).
On 2013/01/15 08:19:22, vabr (Chromium) wrote:
> Hi Ben,
> LGTM with comments.
>
> Vaclav
Just to clarify that my LGTM was only for the 7 files I reviewed:
downloads_(internal_)?api.{cc|h}
and
downloads_custom_bindings.*
Cheers,
Vaclav
Thanks, Vaclav! Randy: PTAL. https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... File chrome/browser/download/chrome_download_manager_delegate.cc (right): https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.cc:788: changed_filename)); On 2013/01/14 21:02:29, rdsmith wrote: > Where are we stripping out .. and similar? See EDERD::DeterminerCallback > ValidateFilename in downloads_api.cc in the latest PS. https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... File chrome/browser/download/chrome_download_manager_delegate.h (left): https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.h:244: scoped_ptr<ExtensionDownloadsEventRouter> extension_event_router_; On 2013/01/14 21:02:29, rdsmith wrote: > So do you remember why we wanted to move EDER to DS? After having read through > this CL, I'd be inclined to leave it on CDMD. Specifically, I don't see that > anything outside of CDMD is currently referencing it (maybe we have plans?) and > this CL means that CDMD needs to reference it, so minimal visibility would make > it (from the C++ perspective) an implementation detail of CDMD. WDYT? The downloadsInternal API needs to access the EDER. https://codereview.chromium.org/11574006/diff/53009/chrome/browser/extensions... https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... File chrome/browser/download/chrome_download_manager_delegate.h (right): https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.h:138: struct ContinueFilenameDeterminationInfo { On 2013/01/14 21:02:29, rdsmith wrote: > References to this structure below are references :-}, so you just need a > forward decl not a full defn. Done. https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.h:222: On 2013/01/14 21:02:29, rdsmith wrote: > I'm concerned about the names of these two functions and > EDER::OnDownloadFilenameDetermined. (If you don't care about my philosophy and > just want my request, skip to the last paragraph :-}). > > I think of "On*" as being either notifications or returns from async calls--i.e. > when information is passed in without expectation of a response. In contrast, I > think of action method names (DetermineDownloadFilename) as being used to > initiate actions (that may or may not return results). I'm also uncomfortable > with the second of the two names above, since it's used when the extension > decides not to determine the filename and when it does and we've sanitized it. > > So I'd like to request something like: > * EDER::DetermineDownloadFilename (or DetermineFilename, or something fancier) > * CDMD::OnExtensionSpecifyingFilename > * CDMD::OnFilenameDetermined (since it's not clear by who). > > WDYT? I didn't put any thought into naming these methods, so it's good that we're talking about them. I'm getting hung up on the word 'determine', which suggests finality. This is all right in the middle of the determination process, so the filename is far from determined. Maybe the imperfect/progressive 'determining' would fit better? WDYT of these? EDER::DeterminingFilename CDMD::OnExtensionOverridingFilename CDMD::ContinueDeterminingFilename https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:190: (filename[1] == L'.')) On 2013/01/14 21:02:29, rdsmith wrote: > Doesn't this allow for, e.g. "phantom_subdir/../../../../etc/passwd"? Yep, changed it to FilePath::ReferencesParent. https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:582: bool overwrite) { On 2013/01/14 21:02:29, rdsmith wrote: > How likely is it that we'll get a response we didn't expect from a different > extension_id+sub_event_id? I'm wondering if it's reasonable just to count down > responses here. AFAIU, it should be impossible to receive an unexpected DeterminerCallback, however, in order for there to be a deterministic precedence ordering between determiners, the EDERD must know which determiner is calling and which determiner previously set filename. https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1070: // The method by which extensions hook into the filename determination process On 2013/01/14 21:02:29, rdsmith wrote: > nit: I'd find this comment easier to read if it had paragraph breaks between the > three chunks of actions being done: The addition of the listener, the call out > to the extensions, and the return from the extensions. (When I did my own > summary, I actually ended up doing four, splitting the fan in from the EDERD -> > CDMD callback.) Done. https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1092: // conflict. On 2013/01/14 21:02:29, rdsmith wrote: > If I read the code above, it's not the last one responding, it's the last one > registered (because of the determiner_index_ check). Am I confused? Changed the code and updated this comment. https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1099: if (include_incognito && profile->HasOffTheRecordProfile()) { On 2013/01/14 21:02:29, rdsmith wrote: > My confusion, your opportunity to enlighten :-}: If we're an extension attached > to an incognito profile, we get visibility into the corresponding regular > profile. The reverse is not true. So it makes sense to me that (elsewhere) we > dispatch regular events to incognito EDERs. But this looks like we're > installing listeners on incognito profiles in response to non-incognito > requests, which is Bad. What am I missing? > > (I'd expect the bool to be called "include regular" and for the test to test > that and whether the profile *is* incognito, not whether it has a corresponding > incognito profile.) PTAL, this changed. https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1100: AddFilenameDeterminer(profile->GetOffTheRecordProfile(), On 2013/01/14 21:02:29, rdsmith wrote: > Ignoring return value? That at least should have a comment (also for other such > calls). Done. https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1110: return false; On 2013/01/14 21:02:29, rdsmith wrote: > What is the larger implication about false returns from these functions? > Doesn't it imply a bug in the internal downloads function? (Except for > DetermineFilename, when the item could have been completed or something silly > like that.) And if there are bugs in our code, even in Javascript, doesn't that > deserve a DCHECK? Done. https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1147: DownloadItem* item = GetDownloadIfInProgress( On 2013/01/14 21:02:29, rdsmith wrote: > Is there any issue here with downloads resumption? E.g. an interrupt occurs > while file determination is in progress, so we drop responses from the > extensions on the floor, and then resume the download with a EDERD still > attached to it in some intermediate state WRT responses from the extensions? > > My inclination would be to reset the EDERD when we transition out of > IN_PROGRESS, but that may have gotchas I don't see. I added EDERD::ClearPendingDeterminers, and call it both right before the fan-out and right after the fan-in. I'm not worried about leaving unusable (not exactly "leaked") determiners on the EDERD in the rare case of interruption while extensions are responding, but if it becomes a real problem then we can revisit this idea. Are there any other concerns here? https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1180: iter = determiners_.erase(iter); On 2013/01/14 21:02:29, rdsmith wrote: > What about download items that are waiting for responses from extensions? Do we > need to cleanup their EDERD? Done. > > This and the comment above makes me a little tempted to suggest you not use the > UserData aspect of the download item but instead synchronize on a structure in > the EDER based on download id. That'd allow you to not worry about what happens > to the download item at all; you can just pass the result of all this back to > the CDMD where it can decide whether to drop it on the floor all at once. If we > don't do that, we're going to have to be thinking fairly carefully about races > as we go through this code, and I think it'll be more fragile in the future too. I've had plenty of bugs due to parallel datastructures that I'd like to try to avoid this map until we have a specific problem that can't be solved another way. https://codereview.chromium.org/11574006/diff/55003/chrome/common/extensions/... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/11574006/diff/55003/chrome/common/extensions/... chrome/common/extensions/api/downloads.idl:22: // is unset. On 2013/01/14 21:02:29, rdsmith wrote: > Sorry, probably still confused about syntax, but doesn't this imply that the > filename may be unset, and thus should have a ? after the type name? Done. https://codereview.chromium.org/11574006/diff/55003/chrome/common/extensions/... chrome/common/extensions/api/downloads.idl:439: static void onDeterminingFilename(DownloadItem downloadItem); On 2013/01/14 21:02:29, rdsmith wrote: > This is solidly and definitely me not understanding the syntax, but if this > function can return something, should it be declared void? The return values are always ignored in IDL.
https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... File chrome/browser/download/chrome_download_manager_delegate.h (left): https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.h:244: scoped_ptr<ExtensionDownloadsEventRouter> extension_event_router_; On 2013/01/17 02:52:05, benjhayden_chromium wrote: > On 2013/01/14 21:02:29, rdsmith wrote: > > So do you remember why we wanted to move EDER to DS? After having read > through > > this CL, I'd be inclined to leave it on CDMD. Specifically, I don't see that > > anything outside of CDMD is currently referencing it (maybe we have plans?) > and > > this CL means that CDMD needs to reference it, so minimal visibility would > make > > it (from the C++ perspective) an implementation detail of CDMD. WDYT? > > The downloadsInternal API needs to access the EDER. > https://codereview.chromium.org/11574006/diff/53009/chrome/browser/extensions... Glancing at that file, it looks like it's only accessing static functions. So it needs to be able to read the .h file, not access the actual object. What am I missing? (If I'm not missing anything, that means there are unneeded include files in the internal API implementation.) https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... File chrome/browser/download/chrome_download_manager_delegate.h (right): https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.h:222: On 2013/01/17 02:52:05, benjhayden_chromium wrote: > On 2013/01/14 21:02:29, rdsmith wrote: > > I'm concerned about the names of these two functions and > > EDER::OnDownloadFilenameDetermined. (If you don't care about my philosophy > and > > just want my request, skip to the last paragraph :-}). > > > > I think of "On*" as being either notifications or returns from async > calls--i.e. > > when information is passed in without expectation of a response. In contrast, > I > > think of action method names (DetermineDownloadFilename) as being used to > > initiate actions (that may or may not return results). I'm also uncomfortable > > with the second of the two names above, since it's used when the extension > > decides not to determine the filename and when it does and we've sanitized it. > > > > > So I'd like to request something like: > > * EDER::DetermineDownloadFilename (or DetermineFilename, or something fancier) > > * CDMD::OnExtensionSpecifyingFilename > > * CDMD::OnFilenameDetermined (since it's not clear by who). > > > > WDYT? > > I didn't put any thought into naming these methods, so it's good that we're > talking about them. > > I'm getting hung up on the word 'determine', which suggests finality. This is > all right in the middle of the determination process, so the filename is far > from determined. Maybe the imperfect/progressive 'determining' would fit better? > WDYT of these? > EDER::DeterminingFilename > CDMD::OnExtensionOverridingFilename > CDMD::ContinueDeterminingFilename Can we find something that's an imperative? I'm good with ContinueDeterminingFilename (and OnExtensionOverridingFilename) but I'd like "DeterminingFilename" to sound something more like an order the caller gives the routine. (I keep thinking of "CommentOnFilename", but I don't think that's good, just amusing :-}.) What changes to the filename can happen after the extension comes back? I guess if we don't specify overwrite we can get suffixes? MaybeOverrideBaseFilename? https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/55003/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1147: DownloadItem* item = GetDownloadIfInProgress( On 2013/01/17 02:52:05, benjhayden_chromium wrote: > On 2013/01/14 21:02:29, rdsmith wrote: > > Is there any issue here with downloads resumption? E.g. an interrupt occurs > > while file determination is in progress, so we drop responses from the > > extensions on the floor, and then resume the download with a EDERD still > > attached to it in some intermediate state WRT responses from the extensions? > > > > My inclination would be to reset the EDERD when we transition out of > > IN_PROGRESS, but that may have gotchas I don't see. > > I added EDERD::ClearPendingDeterminers, and call it both right before the > fan-out and right after the fan-in. > I'm not worried about leaving unusable (not exactly "leaked") determiners on the > EDERD in the rare case of interruption while extensions are responding, but if > it becomes a real problem then we can revisit this idea. > Are there any other concerns here? I think that's ok. At least, the only problem I see is the leaking of determiners. (The only reason I wouldn't call it a leak is that it'll be destroyed when the DownloadItem is. But that's likely to be at browser shutdown, and we don't really care about things that leak at shutdown, we care about things that take up memory when the browser is running.) https://codereview.chromium.org/11574006/diff/55003/chrome/common/extensions/... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/11574006/diff/55003/chrome/common/extensions/... chrome/common/extensions/api/downloads.idl:22: // is unset. On 2013/01/17 02:52:05, benjhayden_chromium wrote: > On 2013/01/14 21:02:29, rdsmith wrote: > > Sorry, probably still confused about syntax, but doesn't this imply that the > > filename may be unset, and thus should have a ? after the type name? > > Done. Still confused, Milo. Does this mean that the extension will always set filename, but it'll be a null string if the extension doesn't want to override the path? https://codereview.chromium.org/11574006/diff/88001/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/88001/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:574: if (determiners_.empty()) When I first read through this, I spent a couple of minutes being confused about why this statement was here, since a loop traversal should do nothing if the list was empty. I eventually realized that it was a guard against EDERDs that weren't currently involved in a call out to extensions getting their callbacks calls. But I could easily imagine future code writers thinking it was an optimization. Comment? https://codereview.chromium.org/11574006/diff/88001/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:674: FilePath determined_filename_; I may have missed it, but where do you reset these values? I'm specifically thinking of the interrupt->resume case where we call back into this code; if determiner_index_ is somewhat high, won't that result in results from some extensions being dropped on the floor? And if the second time through no-one overrides, won't we re-use determined_filename_? https://codereview.chromium.org/11574006/diff/88001/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:792: FilePath file_path(filename16); nit: Move everything that's only relevant to creating filename16 inside the if, to make the conditionalization clearer. https://codereview.chromium.org/11574006/diff/88001/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1153: // events for incognito/regular downloads. Do not automatically add this So this seems weird to me. My understanding (please correct if wrong) is that everywhere else we give incognito extension full and symmetric visibility to both incognito and regular downloads (within that profile). But it sounds like here we're breaking that symmetry and only allowing interposition for filename determination for non-incognito profiles by non-incognito extensions. Why? https://codereview.chromium.org/11574006/diff/88001/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1190: ExtensionDownloadsEventRouter::DeterminerId::DeterminerId( So there's an officially sanctioned order for methods, which is the same in the .cc file as it is in the .h file. I'm not neurotic about that; if there's some other order that makes sense, go for it. But I don't see why interrupting the EDER methods in order to put in some trivial methods for EDER::DeterminerId would count?
PTAL https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... File chrome/browser/download/chrome_download_manager_delegate.h (left): https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.h:244: scoped_ptr<ExtensionDownloadsEventRouter> extension_event_router_; On 2013/01/17 19:15:42, rdsmith wrote: > On 2013/01/17 02:52:05, benjhayden_chromium wrote: > > On 2013/01/14 21:02:29, rdsmith wrote: > > > So do you remember why we wanted to move EDER to DS? After having read > > through > > > this CL, I'd be inclined to leave it on CDMD. Specifically, I don't see > that > > > anything outside of CDMD is currently referencing it (maybe we have plans?) > > and > > > this CL means that CDMD needs to reference it, so minimal visibility would > > make > > > it (from the C++ perspective) an implementation detail of CDMD. WDYT? > > > > The downloadsInternal API needs to access the EDER. > > > https://codereview.chromium.org/11574006/diff/53009/chrome/browser/extensions... > > Glancing at that file, it looks like it's only accessing static functions. So > it needs to be able to read the .h file, not access the actual object. What am > I missing? The EDER static methods that downloadsInternal calls go to the DownloadServiceFactory to get the EDER instance. > > (If I'm not missing anything, that means there are unneeded include files in the > internal API implementation.) There were. https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... File chrome/browser/download/chrome_download_manager_delegate.h (right): https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.h:222: On 2013/01/17 19:15:42, rdsmith wrote: > On 2013/01/17 02:52:05, benjhayden_chromium wrote: > > On 2013/01/14 21:02:29, rdsmith wrote: > > > I'm concerned about the names of these two functions and > > > EDER::OnDownloadFilenameDetermined. (If you don't care about my philosophy > > and > > > just want my request, skip to the last paragraph :-}). > > > > > > I think of "On*" as being either notifications or returns from async > > calls--i.e. > > > when information is passed in without expectation of a response. In > contrast, > > I > > > think of action method names (DetermineDownloadFilename) as being used to > > > initiate actions (that may or may not return results). I'm also > uncomfortable > > > with the second of the two names above, since it's used when the extension > > > decides not to determine the filename and when it does and we've sanitized > it. > > > > > > > > So I'd like to request something like: > > > * EDER::DetermineDownloadFilename (or DetermineFilename, or something > fancier) > > > * CDMD::OnExtensionSpecifyingFilename > > > * CDMD::OnFilenameDetermined (since it's not clear by who). > > > > > > WDYT? > > > > I didn't put any thought into naming these methods, so it's good that we're > > talking about them. > > > > I'm getting hung up on the word 'determine', which suggests finality. This is > > all right in the middle of the determination process, so the filename is far > > from determined. Maybe the imperfect/progressive 'determining' would fit > better? > > WDYT of these? > > EDER::DeterminingFilename > > CDMD::OnExtensionOverridingFilename > > CDMD::ContinueDeterminingFilename > > Can we find something that's an imperative? I'm good with > ContinueDeterminingFilename (and OnExtensionOverridingFilename) but I'd like > "DeterminingFilename" to sound something more like an order the caller gives the > routine. (I keep thinking of "CommentOnFilename", but I don't think that's > good, just amusing :-}.) > > What changes to the filename can happen after the extension comes back? I guess > if we don't specify overwrite we can get suffixes? MaybeOverrideBaseFilename? All of the other methods on EDER that dispatch events to the renderer start with On. EDER::OnDeterminingFilename? https://codereview.chromium.org/11574006/diff/55003/chrome/common/extensions/... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/11574006/diff/55003/chrome/common/extensions/... chrome/common/extensions/api/downloads.idl:22: // is unset. On 2013/01/17 19:15:42, rdsmith wrote: > On 2013/01/17 02:52:05, benjhayden_chromium wrote: > > On 2013/01/14 21:02:29, rdsmith wrote: > > > Sorry, probably still confused about syntax, but doesn't this imply that the > > > filename may be unset, and thus should have a ? after the type name? > > > > Done. > > Still confused, Milo. Does this mean that the extension will always set > filename, but it'll be a null string if the extension doesn't want to override > the path? Not quite. See about L60 in https://codereview.chromium.org/11574006/diff/70003/chrome/renderer/resources... The handler may return |null|. If I could figure out how to get at this dictionary from javascript, then I'd still only use it to validate what the handler returns if it doesn't return null. Instead I just added a few extra type-checks; see the latest PS. By the time that this information gets to the browser process, yes, there's always a |filename| parameter that may simply be empty. Milo? https://codereview.chromium.org/11574006/diff/88001/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/88001/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:574: if (determiners_.empty()) On 2013/01/17 19:15:42, rdsmith wrote: > When I first read through this, I spent a couple of minutes being confused about > why this statement was here, since a loop traversal should do nothing if the > list was empty. I eventually realized that it was a guard against EDERDs that > weren't currently involved in a call out to extensions getting their callbacks > calls. But I could easily imagine future code writers thinking it was an > optimization. Comment? Done. https://codereview.chromium.org/11574006/diff/88001/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:674: FilePath determined_filename_; On 2013/01/17 19:15:42, rdsmith wrote: > I may have missed it, but where do you reset these values? I'm specifically > thinking of the interrupt->resume case where we call back into this code; if > determiner_index_ is somewhat high, won't that result in results from some > extensions being dropped on the floor? And if the second time through no-one > overrides, won't we re-use determined_filename_? Good catch. Added that to right after where filename_change_ is run. https://codereview.chromium.org/11574006/diff/88001/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:792: FilePath file_path(filename16); On 2013/01/17 19:15:42, rdsmith wrote: > nit: Move everything that's only relevant to creating filename16 inside the if, > to make the conditionalization clearer. No, filename16 is used on L803 of this PS. https://codereview.chromium.org/11574006/diff/88001/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1153: // events for incognito/regular downloads. Do not automatically add this On 2013/01/17 19:15:42, rdsmith wrote: > So this seems weird to me. My understanding (please correct if wrong) is that > everywhere else we give incognito extension full and symmetric visibility to > both incognito and regular downloads (within that profile). But it sounds like > here we're breaking that symmetry and only allowing interposition for filename > determination for non-incognito profiles by non-incognito extensions. Why? Nope, when incognito extensions' renderers call onDeterminingFilename.addListener, the |profile| that is passed here is the incognito profile, so it'll use the incognito EDER. I agree, it does feel a little weird how onDeterminingFIlename works with incognito differently from how the other events work with incognito, but onDeterminingFilename does everything else differently, too. https://codereview.chromium.org/11574006/diff/88001/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1190: ExtensionDownloadsEventRouter::DeterminerId::DeterminerId( On 2013/01/17 19:15:42, rdsmith wrote: > So there's an officially sanctioned order for methods, which is the same in the > .cc file as it is in the .h file. I'm not neurotic about that; if there's some > other order that makes sense, go for it. But I don't see why interrupting the > EDER methods in order to put in some trivial methods for EDER::DeterminerId > would count? Done.
https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... File chrome/browser/download/chrome_download_manager_delegate.h (right): https://codereview.chromium.org/11574006/diff/55003/chrome/browser/download/c... chrome/browser/download/chrome_download_manager_delegate.h:222: On 2013/01/18 20:59:50, benjhayden_chromium wrote: > On 2013/01/17 19:15:42, rdsmith wrote: > > On 2013/01/17 02:52:05, benjhayden_chromium wrote: > > > On 2013/01/14 21:02:29, rdsmith wrote: > > > > I'm concerned about the names of these two functions and > > > > EDER::OnDownloadFilenameDetermined. (If you don't care about my > philosophy > > > and > > > > just want my request, skip to the last paragraph :-}). > > > > > > > > I think of "On*" as being either notifications or returns from async > > > calls--i.e. > > > > when information is passed in without expectation of a response. In > > contrast, > > > I > > > > think of action method names (DetermineDownloadFilename) as being used to > > > > initiate actions (that may or may not return results). I'm also > > uncomfortable > > > > with the second of the two names above, since it's used when the extension > > > > decides not to determine the filename and when it does and we've sanitized > > it. > > > > > > > > > > > So I'd like to request something like: > > > > * EDER::DetermineDownloadFilename (or DetermineFilename, or something > > fancier) > > > > * CDMD::OnExtensionSpecifyingFilename > > > > * CDMD::OnFilenameDetermined (since it's not clear by who). > > > > > > > > WDYT? > > > > > > I didn't put any thought into naming these methods, so it's good that we're > > > talking about them. > > > > > > I'm getting hung up on the word 'determine', which suggests finality. This > is > > > all right in the middle of the determination process, so the filename is far > > > from determined. Maybe the imperfect/progressive 'determining' would fit > > better? > > > WDYT of these? > > > EDER::DeterminingFilename > > > CDMD::OnExtensionOverridingFilename > > > CDMD::ContinueDeterminingFilename > > > > Can we find something that's an imperative? I'm good with > > ContinueDeterminingFilename (and OnExtensionOverridingFilename) but I'd like > > "DeterminingFilename" to sound something more like an order the caller gives > the > > routine. (I keep thinking of "CommentOnFilename", but I don't think that's > > good, just amusing :-}.) > > > > What changes to the filename can happen after the extension comes back? I > guess > > if we don't specify overwrite we can get suffixes? MaybeOverrideBaseFilename? > > All of the other methods on EDER that dispatch events to the renderer start with > On. > EDER::OnDeterminingFilename? Sure. https://codereview.chromium.org/11574006/diff/55003/chrome/common/extensions/... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/11574006/diff/55003/chrome/common/extensions/... chrome/common/extensions/api/downloads.idl:22: // is unset. On 2013/01/18 20:59:50, benjhayden_chromium wrote: > On 2013/01/17 19:15:42, rdsmith wrote: > > On 2013/01/17 02:52:05, benjhayden_chromium wrote: > > > On 2013/01/14 21:02:29, rdsmith wrote: > > > > Sorry, probably still confused about syntax, but doesn't this imply that > the > > > > filename may be unset, and thus should have a ? after the type name? > > > > > > Done. > > > > Still confused, Milo. Does this mean that the extension will always set > > filename, but it'll be a null string if the extension doesn't want to override > > the path? > > Not quite. See about L60 in > https://codereview.chromium.org/11574006/diff/70003/chrome/renderer/resources... > The handler may return |null|. If I could figure out how to get at this > dictionary from javascript, then I'd still only use it to validate what the > handler returns if it doesn't return null. Instead I just added a few extra > type-checks; see the latest PS. > By the time that this information gets to the browser process, yes, there's > always a |filename| parameter that may simply be empty. Ok. > Milo? Bloom county reference/my quoting weirdness: http://www.thecomicstrips.com/store/add_strip.php?iid=78410 https://codereview.chromium.org/11574006/diff/88001/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/88001/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api.cc:1153: // events for incognito/regular downloads. Do not automatically add this On 2013/01/18 20:59:50, benjhayden_chromium wrote: > On 2013/01/17 19:15:42, rdsmith wrote: > > So this seems weird to me. My understanding (please correct if wrong) is that > > everywhere else we give incognito extension full and symmetric visibility to > > both incognito and regular downloads (within that profile). But it sounds > like > > here we're breaking that symmetry and only allowing interposition for filename > > determination for non-incognito profiles by non-incognito extensions. Why? > > Nope, when incognito extensions' renderers call > onDeterminingFilename.addListener, the |profile| that is passed here is the > incognito profile, so it'll use the incognito EDER. > > I agree, it does feel a little weird how onDeterminingFIlename works with > incognito differently from how the other events work with incognito, but > onDeterminingFilename does everything else differently, too. Sounds good. As discussed offline, could you: * Convince yourself that this pattern (probes from incognito extensions seeing both regular and incognito data, but inserted listeners just getting events from their profile) is common and well understood among extensions. * Failing that, check in with someone Wise In the Ways of Extensions to confirm it makes sense to do it this way, and make sure to document it in the extensions API documentation? https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:656: void CheckAllDeterminersCalled() { Suggestion: So I'm a bit uncomfortable with the responsibilities of this routine. I'd phrase the current responsibilities as "Assuming a file name determination is in progress, figure out if it's completed and take appropriate action if it is. Illegal to call if a file name determination is not in progress." I think it would be a simpler routine to understand if the responsibility was "Figure out if a file name determination has completed and take appropriate action if so." This would allow the removal of the test in ExtensionUnload() and I think would make reviewing and maintaining the code clearer. It would require either overloading a variable (one of the callbacks?), having an additional boolean, or possibly (I didn't think this would work, but now I'm not sure why) just pulling the determiners_.empty() clause into this routine. But it's complex code either way, so it's up to you. https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:670: determined_overwrite_ = false; Why do this here rather than in ClearPendingDeterminers()? It seems weird to split out the reset that way. (This would mean that you'd need to either use temp variables or move the call, of course.) https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:685: DeterminerInfo determiner_; Where do you reset this variable? I don't see it as being reset, and that seems important to your logic in DeterminerCallback working properly if there are rounds through this code. https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right): https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2174: IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, Could you put in some line breaks in the following tests so that it's easy for the reader to group related lines into functional chunks? Occasional comments indicating the purposes of the chunks would be good too. https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2353: FilePath(FILE_PATH_LITERAL("../sneaky.txt")), Just because I'm a paranoid control free, could you make this test not start with ".."? (I.e. "subdir/../../sneaky.txt"?) https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2523: override = FilePath(FILE_PATH_LITERAL("C:\\windows\\system32\\mwahaha.dll")); Suggestion: Make these locations you could write to (publicly writable, no file already there?) https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2883: // TODO test incognito=spanning Would it be possible to add a test that runs through the logic twice for a single download item? I ask because that logic is a little tricky, and is going to be probed in the field very rarely, so if there is a bug there it'd be easy for it to hide. https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... File chrome/browser/extensions/api/downloads_internal/downloads_internal_api.cc (right): https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... chrome/browser/extensions/api/downloads_internal/downloads_internal_api.cc:38: bool DownloadsInternalRemoveFilenameDeterminerFunction::RunImpl() { What is the flow back into the extensions on return from these functions? I.e. if a false is returned, what happens? https://codereview.chromium.org/11574006/diff/110005/chrome/common/extensions... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/11574006/diff/110005/chrome/common/extensions... chrome/common/extensions/api/downloads.idl:18: // The download's new target filename. Should we specify that the filename is considered relative to the downloads directory?
https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:656: void CheckAllDeterminersCalled() { On 2013/01/22 19:43:08, rdsmith wrote: > Suggestion: So I'm a bit uncomfortable with the responsibilities of this > routine. I'd phrase the current responsibilities as "Assuming a file name > determination is in progress, figure out if it's completed and take appropriate > action if it is. Illegal to call if a file name determination is not in > progress." I think it would be a simpler routine to understand if the > responsibility was "Figure out if a file name determination has completed and > take appropriate action if so." This would allow the removal of the test in > ExtensionUnload() and I think would make reviewing and maintaining the code > clearer. It would require either overloading a variable (one of the > callbacks?), having an additional boolean, or possibly (I didn't think this > would work, but now I'm not sure why) just pulling the determiners_.empty() > clause into this routine. > > But it's complex code either way, so it's up to you. We discussed this offline, resulting in a comment clarifying the responsibilities of this method and a simplification in ExtensionUnloaded. https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:670: determined_overwrite_ = false; On 2013/01/22 19:43:08, rdsmith wrote: > Why do this here rather than in ClearPendingDeterminers()? It seems weird to > split out the reset that way. (This would mean that you'd need to either use > temp variables or move the call, of course.) Done. https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:685: DeterminerInfo determiner_; On 2013/01/22 19:43:08, rdsmith wrote: > Where do you reset this variable? I don't see it as being reset, and that seems > important to your logic in DeterminerCallback working properly if there are > rounds through this code. Done. https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right): https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2174: IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, On 2013/01/22 19:43:08, rdsmith wrote: > Could you put in some line breaks in the following tests so that it's easy for > the reader to group related lines into functional chunks? Occasional comments > indicating the purposes of the chunks would be good too. Done. https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2353: FilePath(FILE_PATH_LITERAL("../sneaky.txt")), On 2013/01/22 19:43:08, rdsmith wrote: > Just because I'm a paranoid control free, could you make this test not start > with ".."? (I.e. "subdir/../../sneaky.txt"?) Done. https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2523: override = FilePath(FILE_PATH_LITERAL("C:\\windows\\system32\\mwahaha.dll")); On 2013/01/22 19:43:08, rdsmith wrote: > Suggestion: Make these locations you could write to (publicly writable, no file > already there?) Done. https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2883: // TODO test incognito=spanning On 2013/01/22 19:43:08, rdsmith wrote: > Would it be possible to add a test that runs through the logic twice for a > single download item? I ask because that logic is a little tricky, and is going > to be probed in the field very rarely, so if there is a bug there it'd be easy > for it to hide. Done. https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... File chrome/browser/extensions/api/downloads_internal/downloads_internal_api.cc (right): https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... chrome/browser/extensions/api/downloads_internal/downloads_internal_api.cc:38: bool DownloadsInternalRemoveFilenameDeterminerFunction::RunImpl() { On 2013/01/22 19:43:08, rdsmith wrote: > What is the flow back into the extensions on return from these functions? I.e. > if a false is returned, what happens? The "error_" string is printed in red text in the dev tools console. For the way the code is now, that string is empty, so a generic error message is printed. https://codereview.chromium.org/11574006/diff/110005/chrome/common/extensions... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/11574006/diff/110005/chrome/common/extensions... chrome/common/extensions/api/downloads.idl:18: // The download's new target filename. On 2013/01/22 19:43:08, rdsmith wrote: > Should we specify that the filename is considered relative to the downloads > directory? Done.
Remind me as to how to look at the extension documentation changes for this feature? https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right): https://codereview.chromium.org/11574006/diff/110005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2883: // TODO test incognito=spanning On 2013/01/23 21:30:10, benjhayden_chromium wrote: > On 2013/01/22 19:43:08, rdsmith wrote: > > Would it be possible to add a test that runs through the logic twice for a > > single download item? I ask because that logic is a little tricky, and is > going > > to be probed in the field very rarely, so if there is a bug there it'd be easy > > for it to hide. > > Done. I'm not sure you added the test I was looking for, which is ok; I'm not sure it *can* be added in the current situation :-}. I wanted to test out the reentrancy for a single EDERD, which requires running through file name determination twice for the same download, which requires interaction with resumption, which may not be ready for such interaction. Having said that, I'm not sure that the "two downloads in a row" cases that you did add do much--if you think they're useful, feel free to keep them, but if you only put them in for me, feel free to remove them. https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (left): https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:1688: CHECK(StartTestServer()); What controls which tests this should be in and which not? It seems a bit arbitrary where it is in the file. https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right): https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:109: 0, strlen(events::kOnDownloadDeterminingFilename)) == Huh? (If you consider my confusion understandable, add a comment, otherwise, just tell me what's going on.) https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2415: ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, I find myself wondering about the interface here. It's certainly easiest for us to drop the determine filename on the floor in this case, but should we try and find a way to raise an error to the user when they do something about this? Probably it would complicate the interface too much to add another event. But I wanted to at least raise the question. https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2535: override = FilePath(FILE_PATH_LITERAL("C:\\sneaky\\sneaky.dll")); Unless you do a mkdir, I wouldn't expect these directories to exist and be writable. I was thinking more of using /tmp and the windows equivalent; does that not make sense to you? https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2861: ASSERT_TRUE(ExtensionDownloadsEventRouter::RemoveFilenameDeterminer( Do you want to do this after filename determination has started? I think that would test a different set of code, or at least in a different way.
Matt, would you mind helping me figure out how this slightly funky event should work in incognito? It's based on how the webRequest event handlers can return information from javascript to the browser process. The way the code is now, on-record extension renderer processes receive onDeterminingFilename events only from on-record DownloadManagers, and off-record renderers receive onDeterminingFilename events only from off-record DownloadManagers, regardless of spanning/split. * Should off-record renderers receive events from on-record DownloadManagers regardless of spanning/split? * What differences should there be between how onDeterminingFilename works for spanning vs. split extensions? * Is ExtensionFunction::include_incognito() only true when the extension is "incognito":"spanning" and only false when the extension is "incognito":"split"? The relevant methods are ExtensionDownloadsEventRouter::AddFilenameDeterminer ExtensionDownloadsEventRouter::OnDeterminingFilename ExtensionDownloadsEventRouter::DetermineFilename near the bottom of chrome/browser/extensions/api/downloads/downloads_api.cc
PTAL https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (left): https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:1688: CHECK(StartTestServer()); On 2013/01/24 19:12:46, rdsmith wrote: > What controls which tests this should be in and which not? It seems a bit > arbitrary where it is in the file. A test needs to start the test server if it plans on hitting it. I had copy-pasta'd the line, but this test doesn't reference test_server(), so it didn't need it. https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right): https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:109: 0, strlen(events::kOnDownloadDeterminingFilename)) == On 2013/01/24 19:12:46, rdsmith wrote: > Huh? (If you consider my confusion understandable, add a comment, otherwise, > just tell me what's going on.) Sorry, this is 'starts with', which I now see is already a utility function. https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2415: ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, On 2013/01/24 19:12:46, rdsmith wrote: > I find myself wondering about the interface here. It's certainly easiest for us > to drop the determine filename on the floor in this case, but should we try and > find a way to raise an error to the user when they do something about this? > Probably it would complicate the interface too much to add another event. But I > wanted to at least raise the question. When an extension returns an invalid filename, a red error message is printed in the dev tools console. Filename Determination continues because it isn't waiting on any other determiners. I might not be understanding you correctly, but I would think that dropping the download on the floor whenever an extension returns an invalid filename would not be the least-astonishing reaction. Ah, the usual way to return errors to extensions is that they pass a callback to an API function call; if the API function call encounters an error, then chrome.extension.lastError is non-null when the callback is called. In this case, the API function (chrome.downloadsInternal.determineFilename) is not being called by the extension, it's being called by the secret custom bindings, so the extension can't pass a callback to it. https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2535: override = FilePath(FILE_PATH_LITERAL("C:\\sneaky\\sneaky.dll")); On 2013/01/24 19:12:46, rdsmith wrote: > Unless you do a mkdir, I wouldn't expect these directories to exist and be > writable. I was thinking more of using /tmp and the windows equivalent; does > that not make sense to you? These paths are invalid because they're absolute. They're rejected long before chrome checks whether the directory is writable. Besides, the ReservationTracker does mkdirs. https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2861: ASSERT_TRUE(ExtensionDownloadsEventRouter::RemoveFilenameDeterminer( On 2013/01/24 19:12:46, rdsmith wrote: > Do you want to do this after filename determination has started? I think that > would test a different set of code, or at least in a different way. Done.
On 2013/01/24 21:44:03, benjhayden_chromium wrote: > Matt, would you mind helping me figure out how this slightly funky event should > work in incognito? > It's based on how the webRequest event handlers can return information from > javascript to the browser process. > The way the code is now, on-record extension renderer processes receive > onDeterminingFilename events only from on-record DownloadManagers, and > off-record renderers receive onDeterminingFilename events only from off-record > DownloadManagers, regardless of spanning/split. > > * Should off-record renderers receive events from on-record DownloadManagers > regardless of spanning/split? > > * What differences should there be between how onDeterminingFilename works for > spanning vs. split extensions? > > * Is ExtensionFunction::include_incognito() only true when the extension is > "incognito":"spanning" and only false when the extension is "incognito":"split"? > > The relevant methods are > ExtensionDownloadsEventRouter::AddFilenameDeterminer > ExtensionDownloadsEventRouter::OnDeterminingFilename > ExtensionDownloadsEventRouter::DetermineFilename > near the bottom of chrome/browser/extensions/api/downloads/downloads_api.cc One bit of context on this question: Non-incognito downloads are visible to incognito extension probes (via getting all the downloads or asking for information about a particular download by id). So there's a certain perspective from which there's a bit of inconsistency to the current interface. It may be worth it (sending non-incognito events to both incognito and regular extensions invites collisions when they both respond) but I'm curious if it's in keeping with the usual pattern for extensions.
Thanks for the staged docs; skimming them generated no questions. https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right): https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:109: 0, strlen(events::kOnDownloadDeterminingFilename)) == On 2013/01/25 16:21:36, benjhayden_chromium wrote: > On 2013/01/24 19:12:46, rdsmith wrote: > > Huh? (If you consider my confusion understandable, add a comment, otherwise, > > just tell me what's going on.) > > Sorry, this is 'starts with', which I now see is already a utility function. Confusion understanding fail :-}. Why is the filename determination event checked for by prefix, whereas the other events are straight string equals? https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2415: ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, On 2013/01/25 16:21:36, benjhayden_chromium wrote: > On 2013/01/24 19:12:46, rdsmith wrote: > > I find myself wondering about the interface here. It's certainly easiest for > us > > to drop the determine filename on the floor in this case, but should we try > and > > find a way to raise an error to the user when they do something about this? > > Probably it would complicate the interface too much to add another event. But > I > > wanted to at least raise the question. > > When an extension returns an invalid filename, a red error message is printed in > the dev tools console. Filename Determination continues because it isn't waiting > on any other determiners. I might not be understanding you correctly, but I > would think that dropping the download on the floor whenever an extension > returns an invalid filename would not be the least-astonishing reaction. > > Ah, the usual way to return errors to extensions is that they pass a callback to > an API function call; if the API function call encounters an error, then > chrome.extension.lastError is non-null when the callback is called. In this > case, the API function (chrome.downloadsInternal.determineFilename) is not being > called by the extension, it's being called by the secret custom bindings, so the > extension can't pass a callback to it. Ok, that's fine. https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2535: override = FilePath(FILE_PATH_LITERAL("C:\\sneaky\\sneaky.dll")); On 2013/01/25 16:21:36, benjhayden_chromium wrote: > On 2013/01/24 19:12:46, rdsmith wrote: > > Unless you do a mkdir, I wouldn't expect these directories to exist and be > > writable. I was thinking more of using /tmp and the windows equivalent; does > > that not make sense to you? > > These paths are invalid because they're absolute. They're rejected long before > chrome checks whether the directory is writable. Yes, but that's the behavior we're *testing*, so we shouldn't count on it. I'd like to make sure that if the absolute paths aren't rejected, we don't get a false test success because the directories aren't writable. > Besides, the ReservationTracker does mkdirs.
On 2013/01/25 18:21:40, rdsmith wrote: > On 2013/01/24 21:44:03, benjhayden_chromium wrote: > > Matt, would you mind helping me figure out how this slightly funky event > should > > work in incognito? > > It's based on how the webRequest event handlers can return information from > > javascript to the browser process. > > The way the code is now, on-record extension renderer processes receive > > onDeterminingFilename events only from on-record DownloadManagers, and > > off-record renderers receive onDeterminingFilename events only from off-record > > DownloadManagers, regardless of spanning/split. > > > > * Should off-record renderers receive events from on-record DownloadManagers > > regardless of spanning/split? > > > > * What differences should there be between how onDeterminingFilename works > for > > spanning vs. split extensions? > > > > * Is ExtensionFunction::include_incognito() only true when the extension is > > "incognito":"spanning" and only false when the extension is > "incognito":"split"? > > > > The relevant methods are > > ExtensionDownloadsEventRouter::AddFilenameDeterminer > > ExtensionDownloadsEventRouter::OnDeterminingFilename > > ExtensionDownloadsEventRouter::DetermineFilename > > near the bottom of chrome/browser/extensions/api/downloads/downloads_api.cc > > One bit of context on this question: Non-incognito downloads are visible to > incognito extension probes (via getting all the downloads or asking for > information about a particular download by id). So there's a certain > perspective from which there's a bit of inconsistency to the current interface. > It may be worth it (sending non-incognito events to both incognito and regular > extensions invites collisions when they both respond) but I'm curious if it's in > keeping with the usual pattern for extensions. In split mode, each extension instance (one for regular profile and and one for incognito profile) should only see the downloads of the respective profile. In spanning mode, you don't have that problem as there is only one extension that sees everything (assuming that you have incognito permission).
On 2013/01/25 18:29:33, battre wrote: > On 2013/01/25 18:21:40, rdsmith wrote: > > On 2013/01/24 21:44:03, benjhayden_chromium wrote: > > > Matt, would you mind helping me figure out how this slightly funky event > > should > > > work in incognito? > > > It's based on how the webRequest event handlers can return information from > > > javascript to the browser process. > > > The way the code is now, on-record extension renderer processes receive > > > onDeterminingFilename events only from on-record DownloadManagers, and > > > off-record renderers receive onDeterminingFilename events only from > off-record > > > DownloadManagers, regardless of spanning/split. > > > > > > * Should off-record renderers receive events from on-record > DownloadManagers > > > regardless of spanning/split? > > > > > > * What differences should there be between how onDeterminingFilename works > > for > > > spanning vs. split extensions? > > > > > > * Is ExtensionFunction::include_incognito() only true when the extension is > > > "incognito":"spanning" and only false when the extension is > > "incognito":"split"? > > > > > > The relevant methods are > > > ExtensionDownloadsEventRouter::AddFilenameDeterminer > > > ExtensionDownloadsEventRouter::OnDeterminingFilename > > > ExtensionDownloadsEventRouter::DetermineFilename > > > near the bottom of chrome/browser/extensions/api/downloads/downloads_api.cc > > > > One bit of context on this question: Non-incognito downloads are visible to > > incognito extension probes (via getting all the downloads or asking for > > information about a particular download by id). So there's a certain > > perspective from which there's a bit of inconsistency to the current > interface. > > It may be worth it (sending non-incognito events to both incognito and regular > > extensions invites collisions when they both respond) but I'm curious if it's > in > > keeping with the usual pattern for extensions. > > In split mode, each extension instance (one for regular profile and and one for > incognito profile) should only see the downloads of the respective profile. In > spanning mode, you don't have that problem as there is only one extension that > sees everything (assuming that you have incognito permission). To expand on what Dominic said, there's a 2x2 matrix of cases, based on (a) whether the extension is enabled in incognito, and (b) whether the extension is spanning or split. - disabled/spanning: only one extension instance. should see only on-record downloads. - enabled/spanning: only one extension instance. should see both on-record and off-record. - disabled/split: only one extension instance. should see only on-record downloads. - enabled/split: 2 extension instances. each instance should see the downloads of its respective profile. ExtensionFunction::profile() will be the profile of the extension instance. This means it can be the incognito profile for the enabled/split case. include_incognito is false for this case because we don't want information to cross profiles in the split case.
PTAL Dominic, Matt, I think I have incognito split/spanning straight now, but would you care to take a look at downloads_api.cc and downloads_api_unittest.cc to confirm? Thanks! https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right): https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:109: 0, strlen(events::kOnDownloadDeterminingFilename)) == On 2013/01/25 18:22:45, rdsmith wrote: > On 2013/01/25 16:21:36, benjhayden_chromium wrote: > > On 2013/01/24 19:12:46, rdsmith wrote: > > > Huh? (If you consider my confusion understandable, add a comment, > otherwise, > > > just tell me what's going on.) > > > > Sorry, this is 'starts with', which I now see is already a utility function. > > Confusion understanding fail :-}. Why is the filename determination event > checked for by prefix, whereas the other events are straight string equals? The actual name of the event includes the sub_event_id: 'downloads.onDeterminingFilename/0', 'downloads.onDeterminingFilename/1', etc. This is a quirk of how the event works. The webRequest API does the same thing. It's needed to allow downloads_api.cc to keep track of which handlers have responded. Note how downloads_custom_bindings.js makes chrome.Event objects named 'downloads.onDeterminingFilename/0' etc. https://codereview.chromium.org/11574006/diff/124005/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2535: override = FilePath(FILE_PATH_LITERAL("C:\\sneaky\\sneaky.dll")); On 2013/01/25 18:22:45, rdsmith wrote: > On 2013/01/25 16:21:36, benjhayden_chromium wrote: > > On 2013/01/24 19:12:46, rdsmith wrote: > > > Unless you do a mkdir, I wouldn't expect these directories to exist and be > > > writable. I was thinking more of using /tmp and the windows equivalent; > does > > > that not make sense to you? > > > > These paths are invalid because they're absolute. They're rejected long before > > chrome checks whether the directory is writable. > > Yes, but that's the behavior we're *testing*, so we shouldn't count on it. I'd > like to make sure that if the absolute paths aren't rejected, we don't get a > false test success because the directories aren't writable. > > > Besides, the ReservationTracker does mkdirs. > Done.
Dominic, Matt: We'd also like your thoughts on how the rest of the downloads API works with incognito. We'd like the downloads API to be internally consistent and consistent with the other chrome.* APIs when possible, but it's currently a little bit different. search() in incognito:split currently returns both on-record and off-record items to off-record extension renderers, and the rest of the functions can access on-record items from incognito/split. See GetManagers() around line 342 of downloads_api.cc. We did that because the chrome://downloads webui in incognito windows contains DownloadItems from both the on-record and off-record managers and we hope to allow extensions to provide a supplementary UI using browser action buttons and web_accessible_resources tabs. Spanning extensions don't seem to quite fill that role. I couldn't load a web_accessible_resource from a spanning extension in an incognito window. onDeterminingFilename is an automatic mechanism, not related to UI, so it doesn't fire for on-record items in incognito/split extension renderers. So, onDeterminingFilename is like the other chrome.* APIs and unlike the rest of the downloads API in this limited respect. Is this OK?
On 2013/02/04 21:07:55, benjhayden_chromium wrote: > Dominic, Matt: We'd also like your thoughts on how the rest of the downloads API > works with incognito. We'd like the downloads API to be internally consistent > and consistent with the other chrome.* APIs when possible, but it's currently a > little bit different. > > search() in incognito:split currently returns both on-record and off-record > items to off-record extension renderers, and the rest of the functions can > access on-record items from incognito/split. See GetManagers() around line 342 > of downloads_api.cc. > We did that because the chrome://downloads webui in incognito windows contains > DownloadItems from both the on-record and off-record managers and we hope to > allow extensions to provide a supplementary UI using browser action buttons and > web_accessible_resources tabs. Spanning extensions don't seem to quite fill that > role. I couldn't load a web_accessible_resource from a spanning extension in an > incognito window. > onDeterminingFilename is an automatic mechanism, not related to UI, so it > doesn't fire for on-record items in incognito/split extension renderers. So, > onDeterminingFilename is like the other chrome.* APIs and unlike the rest of the > downloads API in this limited respect. > > Is this OK? I think I'd prefer for the API to be internally consistent. If we need to show on-record data in incognito mode, we should do that for all methods/events.
I realize it's a bit late in the game to bring this up, but this CL adds quite a lot of complexity for this feature. Did onDeterminingFilename go through API review? I'm wondering if it's worth the complexity tradeoff. https://codereview.chromium.org/11574006/diff/148001/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/148001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1370: DispatchEvent((event_name + (*iter)).c_str(), false, json); won't this double dispatch for determines with include_incognito=true? We've already dispatched to every event above. https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... chrome/common/extensions/api/downloads.idl:19: // default Downloads directory, possibly containing subdirectories. Absolute How do we handle path separators in a cross-platform manner? On Windows, will "../foo.txt" show up as a back reference given that \ is the path separator? https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... chrome/common/extensions/api/downloads.idl:449: static void onDeterminingFilename(DownloadItem downloadItem); Does this really need to be a synchronous API? What if we did this instead: - Downloads system starts writing to the default filename immediately. - Extensions have a certain amount of time to rename the file to something else.
The last time that the API was reviewed, the mechanism to allow extensions to rename downloads was a simple function, chrome.downloads.setDestination(long id, string filename). In the course of writing this CL, I came to understand that that was technically infeasible. An event that accepts return values from handlers is unusual in the chrome.* APIs except for the webRequest API, but is it that much more complex from an extension writer's point of view than a simple function? It's certainly more complex from an implementation point of view, but I've already overcome that hurdle. There's the maintenance cost, but that's what tests and comments are for. https://codereview.chromium.org/11574006/diff/148001/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/148001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1370: DispatchEvent((event_name + (*iter)).c_str(), false, json); On 2013/02/04 22:30:19, Matt Perry wrote: > won't this double dispatch for determines with include_incognito=true? We've > already dispatched to every event above. That's not what I see happening in DownloadExtensionTest_OnDeterminingFilename_IncognitoSpanning. For incognito downloads, EDER::profile_ is OffTheRecord but profile in AddFilenameDeterminer is *on* the record. So, determiners_ in the incognito EDER will not include the determiner, but the *on* record EDER will. https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... chrome/common/extensions/api/downloads.idl:19: // default Downloads directory, possibly containing subdirectories. Absolute On 2013/02/04 22:30:19, Matt Perry wrote: > How do we handle path separators in a cross-platform manner? On Windows, will > "../foo.txt" show up as a back reference given that \ is the path separator? FilePath allows either / or \ as a path separator on windows. https://code.google.com/p/chromium/codesearch#chrome/src/base/file_path.h&q=f... https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... chrome/common/extensions/api/downloads.idl:449: static void onDeterminingFilename(DownloadItem downloadItem); On 2013/02/04 22:30:19, Matt Perry wrote: > Does this really need to be a synchronous API? What if we did this instead: > - Downloads system starts writing to the default filename immediately. > - Extensions have a certain amount of time to rename the file to something else. The download system does already start writing to a temporary "Unconfirmed XXXXX.crdownload" filename immediately. I think that Asanka explained to me that it would be untenable in the current download system architecture to determine the filename multiple times. How much time? What if the system is being slow? What about when I change the API to allow extensions to pop up a dialog to allow the user to choose whether to overwrite?
https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... chrome/common/extensions/api/downloads.idl:449: static void onDeterminingFilename(DownloadItem downloadItem); On 2013/02/05 15:08:05, benjhayden_chromium wrote: > On 2013/02/04 22:30:19, Matt Perry wrote: > > Does this really need to be a synchronous API? What if we did this instead: > > - Downloads system starts writing to the default filename immediately. > > - Extensions have a certain amount of time to rename the file to something > else. > > The download system does already start writing to a temporary "Unconfirmed > XXXXX.crdownload" filename immediately. > I think that Asanka explained to me that it would be untenable in the current > download system architecture to determine the filename multiple times. > How much time? What if the system is being slow? What about when I change the > API to allow extensions to pop up a dialog to allow the user to choose whether > to overwrite? I'd like to second this point--this type of race strikes me as a really bad idea. The extension writer will not be able to depend on the API to react reliably; whether the rename happens will depend on system loader, user distraction (in the case Ben raises), and the phase of the moon. Let's please not do timing window dependent extension APIs.
Jochen, would you mind reviewing chrome/*.gypi as an owner?
the gypi files lgtm with comments addressed https://codereview.chromium.org/11574006/diff/148001/chrome/chrome_browser_ex... File chrome/chrome_browser_extensions.gypi (right): https://codereview.chromium.org/11574006/diff/148001/chrome/chrome_browser_ex... chrome/chrome_browser_extensions.gypi:205: 'browser/extensions/api/downloads_internal/downloads_internal_api.cc', alphabetize https://codereview.chromium.org/11574006/diff/148001/chrome/chrome_renderer.gypi File chrome/chrome_renderer.gypi (right): https://codereview.chromium.org/11574006/diff/148001/chrome/chrome_renderer.g... chrome/chrome_renderer.gypi:131: 'renderer/extensions/downloads_custom_bindings.cc', alphabetize
On 2013/02/05 15:08:05, benjhayden_chromium wrote: > The last time that the API was reviewed, the mechanism to allow extensions to > rename downloads was a simple function, chrome.downloads.setDestination(long id, > string filename). > In the course of writing this CL, I came to understand that that was technically > infeasible. > An event that accepts return values from handlers is unusual in the chrome.* > APIs except for the webRequest API, but is it that much more complex from an > extension writer's point of view than a simple function? It's certainly more > complex from an implementation point of view, but I've already overcome that > hurdle. There's the maintenance cost, but that's what tests and comments are > for. I'm concerned more about the maintenance cost. It's a lot of code to support 1 method. Are we sure it's worth it? Looking at the bug, maybe adding an API to toggle "prompt user on download" would satisfy the complaints.
https://codereview.chromium.org/11574006/diff/148001/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/148001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1370: DispatchEvent((event_name + (*iter)).c_str(), false, json); On 2013/02/05 15:08:05, benjhayden_chromium wrote: > On 2013/02/04 22:30:19, Matt Perry wrote: > > won't this double dispatch for determines with include_incognito=true? We've > > already dispatched to every event above. > > That's not what I see happening in > DownloadExtensionTest_OnDeterminingFilename_IncognitoSpanning. > For incognito downloads, EDER::profile_ is OffTheRecord but profile in > AddFilenameDeterminer is *on* the record. So, determiners_ in the incognito EDER > will not include the determiner, but the *on* record EDER will. What about split mode? https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... chrome/common/extensions/api/downloads.idl:449: static void onDeterminingFilename(DownloadItem downloadItem); On 2013/02/05 15:08:05, benjhayden_chromium wrote: > On 2013/02/04 22:30:19, Matt Perry wrote: > > Does this really need to be a synchronous API? What if we did this instead: > > - Downloads system starts writing to the default filename immediately. > > - Extensions have a certain amount of time to rename the file to something > else. > > The download system does already start writing to a temporary "Unconfirmed > XXXXX.crdownload" filename immediately. > I think that Asanka explained to me that it would be untenable in the current > download system architecture to determine the filename multiple times. > How much time? What if the system is being slow? OK, sounds like this won't work. > What about when I change the > API to allow extensions to pop up a dialog to allow the user to choose whether > to overwrite? Is the prompt blocking from the JavaScript context's point of view? The onDeterminingFilename event requires a synchronous response - any asynchronous query won't work. https://codereview.chromium.org/11574006/diff/148001/chrome/renderer/resource... File chrome/renderer/resources/extensions/downloads_custom_bindings.js (right): https://codereview.chromium.org/11574006/diff/148001/chrome/renderer/resource... chrome/renderer/resources/extensions/downloads_custom_bindings.js:25: new chrome.Event(eventName, opt_argSchemas, opt_eventOptions); I'm not a JS expert, but I believe it's possible to just do "return new chrome.Event(...);" That way you only use a DownloadsEvent for onDeterminingFilename.
On 2013/02/05 20:45:37, Matt Perry wrote: > On 2013/02/05 15:08:05, benjhayden_chromium wrote: > > The last time that the API was reviewed, the mechanism to allow extensions to > > rename downloads was a simple function, chrome.downloads.setDestination(long > id, > > string filename). > > In the course of writing this CL, I came to understand that that was > technically > > infeasible. > > An event that accepts return values from handlers is unusual in the chrome.* > > APIs except for the webRequest API, but is it that much more complex from an > > extension writer's point of view than a simple function? It's certainly more > > complex from an implementation point of view, but I've already overcome that > > hurdle. There's the maintenance cost, but that's what tests and comments are > > for. > > I'm concerned more about the maintenance cost. It's a lot of code to support 1 > method. Are we sure it's worth it? Looking at the bug, maybe adding an API to > toggle "prompt user on download" would satisfy the complaints. This is a very powerful and desired api feature. This event addresses more feature requests than just 'prompt on duplicate download'. There have been about as many requests for some kind of ability to send downloads to different folders depending on mime type (images/, videos/, pdfs/, etc.). Some users want downloads to just always overwrite. There are several valid things that users might want to do. Writing native code to do them all, or even most of them, would be about as much code as this, would be more difficult to test properly, and would require more options in chrome://settings, which we very much want to avoid.
Please let me know if you ever start to feel like it might be easier to VC. https://codereview.chromium.org/11574006/diff/148001/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/148001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1370: DispatchEvent((event_name + (*iter)).c_str(), false, json); On 2013/02/05 20:49:39, Matt Perry wrote: > On 2013/02/05 15:08:05, benjhayden_chromium wrote: > > On 2013/02/04 22:30:19, Matt Perry wrote: > > > won't this double dispatch for determines with include_incognito=true? We've > > > already dispatched to every event above. > > > > That's not what I see happening in > > DownloadExtensionTest_OnDeterminingFilename_IncognitoSpanning. > > For incognito downloads, EDER::profile_ is OffTheRecord but profile in > > AddFilenameDeterminer is *on* the record. So, determiners_ in the incognito > EDER > > will not include the determiner, but the *on* record EDER will. > > What about split mode? I was given to understand that ExtensionFunction::include_incognito() will never be true for split mode extensions. Is that not correct? https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... chrome/common/extensions/api/downloads.idl:449: static void onDeterminingFilename(DownloadItem downloadItem); On 2013/02/05 20:49:39, Matt Perry wrote: > On 2013/02/05 15:08:05, benjhayden_chromium wrote: > > On 2013/02/04 22:30:19, Matt Perry wrote: > > > Does this really need to be a synchronous API? What if we did this instead: > > > - Downloads system starts writing to the default filename immediately. > > > - Extensions have a certain amount of time to rename the file to something > > else. > > > > The download system does already start writing to a temporary "Unconfirmed > > XXXXX.crdownload" filename immediately. > > I think that Asanka explained to me that it would be untenable in the current > > download system architecture to determine the filename multiple times. > > How much time? What if the system is being slow? > > OK, sounds like this won't work. > > > What about when I change the > > API to allow extensions to pop up a dialog to allow the user to choose whether > > to overwrite? > > Is the prompt blocking from the JavaScript context's point of view? The > onDeterminingFilename event requires a synchronous response - any asynchronous > query won't work. I am planning on augmenting the onDeterminingFilename event in a followup CL to allow extensions to request downloads_custom_bindings.js to pass the handler a callback. This future change will only affect downloads_custom_bindings.js. Handlers may then look like this: chrome.downloads.onDeterminingFilename.addListener(function(item, callback) { chrome.tabs.sendMessage(some_tab_id, item, function(response) { callback(response); }); }, true/*handler is asynchronous*/); https://codereview.chromium.org/11574006/diff/148001/chrome/renderer/resource... File chrome/renderer/resources/extensions/downloads_custom_bindings.js (right): https://codereview.chromium.org/11574006/diff/148001/chrome/renderer/resource... chrome/renderer/resources/extensions/downloads_custom_bindings.js:25: new chrome.Event(eventName, opt_argSchemas, opt_eventOptions); On 2013/02/05 20:49:39, Matt Perry wrote: > I'm not a JS expert, but I believe it's possible to just do "return new > chrome.Event(...);" > > That way you only use a DownloadsEvent for onDeterminingFilename. Unfortunately, registerCustomEvent does 'new DownloadsEvent'. DownloadsEvent is an object class, not a function.
On 2013/02/05 20:57:32, benjhayden_chromium wrote: > On 2013/02/05 20:45:37, Matt Perry wrote: > > On 2013/02/05 15:08:05, benjhayden_chromium wrote: > > > The last time that the API was reviewed, the mechanism to allow extensions > to > > > rename downloads was a simple function, chrome.downloads.setDestination(long > > id, > > > string filename). > > > In the course of writing this CL, I came to understand that that was > > technically > > > infeasible. > > > An event that accepts return values from handlers is unusual in the chrome.* > > > APIs except for the webRequest API, but is it that much more complex from an > > > extension writer's point of view than a simple function? It's certainly more > > > complex from an implementation point of view, but I've already overcome that > > > hurdle. There's the maintenance cost, but that's what tests and comments are > > > for. > > > > I'm concerned more about the maintenance cost. It's a lot of code to support 1 > > method. Are we sure it's worth it? Looking at the bug, maybe adding an API to > > toggle "prompt user on download" would satisfy the complaints. > > This is a very powerful and desired api feature. This event addresses more > feature requests than just 'prompt on duplicate download'. There have been about > as many requests for some kind of ability to send downloads to different folders > depending on mime type (images/, videos/, pdfs/, etc.). Some users want > downloads to just always overwrite. > There are several valid things that users might want to do. Writing native code > to do them all, or even most of them, would be about as much code as this, would > be more difficult to test properly, and would require more options in > chrome://settings, which we very much want to avoid. OK, it sounds like the API has some good use cases. I wasn't around for the original review discussions, so I apologize for bringing up old issues.
> OK, it sounds like the API has some good use cases. I wasn't around for the > original review discussions, so I apologize for bringing up old issues. Not at all! Great questions! I very much appreciate thorough reviews. :-)
https://codereview.chromium.org/11574006/diff/148001/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/148001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1370: DispatchEvent((event_name + (*iter)).c_str(), false, json); On 2013/02/05 21:12:19, benjhayden_chromium wrote: > On 2013/02/05 20:49:39, Matt Perry wrote: > > On 2013/02/05 15:08:05, benjhayden_chromium wrote: > > > On 2013/02/04 22:30:19, Matt Perry wrote: > > > > won't this double dispatch for determines with include_incognito=true? > We've > > > > already dispatched to every event above. > > > > > > That's not what I see happening in > > > DownloadExtensionTest_OnDeterminingFilename_IncognitoSpanning. > > > For incognito downloads, EDER::profile_ is OffTheRecord but profile in > > > AddFilenameDeterminer is *on* the record. So, determiners_ in the incognito > > EDER > > > will not include the determiner, but the *on* record EDER will. > > > > What about split mode? > > I was given to understand that ExtensionFunction::include_incognito() will never > be true for split mode extensions. Is that not correct? That is true. What confuses me, unless I'm misreading it, is this entire block (lines 1350-1372) can only ever dispatch an event that has been dispatched before on line 1347. Lines 1328-1335 add all sub_events in the determiners_ list, and then dispatch those events. Here, in line 1361, we re-add sub_events for which include_incognito is true, and dispatch again for those. What am I missing? https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... chrome/common/extensions/api/downloads.idl:449: static void onDeterminingFilename(DownloadItem downloadItem); On 2013/02/05 21:12:19, benjhayden_chromium wrote: > On 2013/02/05 20:49:39, Matt Perry wrote: > > On 2013/02/05 15:08:05, benjhayden_chromium wrote: > > > On 2013/02/04 22:30:19, Matt Perry wrote: > > > > Does this really need to be a synchronous API? What if we did this > instead: > > > > - Downloads system starts writing to the default filename immediately. > > > > - Extensions have a certain amount of time to rename the file to something > > > else. > > > > > > The download system does already start writing to a temporary "Unconfirmed > > > XXXXX.crdownload" filename immediately. > > > I think that Asanka explained to me that it would be untenable in the > current > > > download system architecture to determine the filename multiple times. > > > How much time? What if the system is being slow? > > > > OK, sounds like this won't work. > > > > > What about when I change the > > > API to allow extensions to pop up a dialog to allow the user to choose > whether > > > to overwrite? > > > > Is the prompt blocking from the JavaScript context's point of view? The > > onDeterminingFilename event requires a synchronous response - any asynchronous > > query won't work. > > I am planning on augmenting the onDeterminingFilename event in a followup CL to > allow extensions to request downloads_custom_bindings.js to pass the handler a > callback. This future change will only affect downloads_custom_bindings.js. > Handlers may then look like this: > chrome.downloads.onDeterminingFilename.addListener(function(item, callback) { > chrome.tabs.sendMessage(some_tab_id, item, function(response) { > callback(response); > }); > }, true/*handler is asynchronous*/); I see. In that case, I'd prefer we always use the callback for consistency. See https://developer.chrome.com/trunk/extensions/runtime.html#event-onMessage for an API that functions similarly (you return true from the event handler to indicate it's asynchronous). https://codereview.chromium.org/11574006/diff/148001/chrome/renderer/resource... File chrome/renderer/resources/extensions/downloads_custom_bindings.js (right): https://codereview.chromium.org/11574006/diff/148001/chrome/renderer/resource... chrome/renderer/resources/extensions/downloads_custom_bindings.js:25: new chrome.Event(eventName, opt_argSchemas, opt_eventOptions); On 2013/02/05 21:12:19, benjhayden_chromium wrote: > On 2013/02/05 20:49:39, Matt Perry wrote: > > I'm not a JS expert, but I believe it's possible to just do "return new > > chrome.Event(...);" > > > > That way you only use a DownloadsEvent for onDeterminingFilename. > > Unfortunately, registerCustomEvent does 'new DownloadsEvent'. DownloadsEvent is > an object class, not a function. I know, but I think it still works. For example, this code works for me: function Parent(x) { this.x_ = x; } function Child(x) { return new Parent(x); } y = new Child(7); // y is a Parent
lgtm... with two small comments (related to each other) https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... chrome/common/extensions/api/downloads.idl:16: // object in order to override a download's target filename. Should this link to filename? I can't see the staged docs at the moment to check the flow, so maybe it isn't necessary. https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... chrome/common/extensions/api/downloads.idl:442: // opportunity to override the target filename. Handlers may return null in Should target filename be a link?
https://codereview.chromium.org/11574006/diff/148001/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/148001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1265: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind( Is there a reason you need to set the install_time on the IO thread? You can get it on the UI thread via extension_service()->extension_prefs()->GetInstallTime(extension->id()); https://codereview.chromium.org/11574006/diff/148001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1549: DispatchEventInternal( I know it's not from this CL, but this is the wrong way to dispatch to incognito profiles. If you look at DispatchEventInternal, you'll see it sets a restrict_to_profile var on the event parameter. You really only want to call DispatchEvent once, with restrict_to_profile NULL in the case when include_incognito is true. The reason for this is that restrict_to_profile is ignored for an incognito-enabled spanning extension. So these events would be dispatched twice to such extensions. https://codereview.chromium.org/11574006/diff/148001/chrome/renderer/resource... File chrome/renderer/resources/extensions/downloads_custom_bindings.js (right): https://codereview.chromium.org/11574006/diff/148001/chrome/renderer/resource... chrome/renderer/resources/extensions/downloads_custom_bindings.js:87: DownloadsEvent.prototype.removeListener = function(cb) { This is only called when the extension explicitly calls it. You should also handle the case where the extension page unloads, otherwise you could have an event registered in the browser that doesn't exist. This can happen if an extension popup adds a DownloadsEvent listener, for example. When the popup goes away, we should remove the filename determiner. The webRequest API handles this by hooking into EventRouter::OnListenerRemoved, which is called on page unload as a result of chrome.Event.prototype.detach_.
Everybody: PTAL. https://codereview.chromium.org/11574006/diff/148001/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/148001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1265: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind( On 2013/02/07 00:10:13, Matt Perry wrote: > Is there a reason you need to set the install_time on the IO thread? You can get > it on the UI thread via > extension_service()->extension_prefs()->GetInstallTime(extension->id()); Done. https://codereview.chromium.org/11574006/diff/148001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1370: DispatchEvent((event_name + (*iter)).c_str(), false, json); On 2013/02/05 21:57:10, Matt Perry wrote: > On 2013/02/05 21:12:19, benjhayden_chromium wrote: > > On 2013/02/05 20:49:39, Matt Perry wrote: > > > On 2013/02/05 15:08:05, benjhayden_chromium wrote: > > > > On 2013/02/04 22:30:19, Matt Perry wrote: > > > > > won't this double dispatch for determines with include_incognito=true? > > We've > > > > > already dispatched to every event above. > > > > > > > > That's not what I see happening in > > > > DownloadExtensionTest_OnDeterminingFilename_IncognitoSpanning. > > > > For incognito downloads, EDER::profile_ is OffTheRecord but profile in > > > > AddFilenameDeterminer is *on* the record. So, determiners_ in the > incognito > > > EDER > > > > will not include the determiner, but the *on* record EDER will. > > > > > > What about split mode? > > > > I was given to understand that ExtensionFunction::include_incognito() will > never > > be true for split mode extensions. Is that not correct? > > That is true. > > What confuses me, unless I'm misreading it, is this entire block (lines > 1350-1372) can only ever dispatch an event that has been dispatched before on > line 1347. Lines 1328-1335 add all sub_events in the determiners_ list, and then > dispatch those events. Here, in line 1361, we re-add sub_events for which > include_incognito is true, and dispatch again for those. What am I missing? Ah! Apologies for being dense, I see it now. https://codereview.chromium.org/11574006/diff/148001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1549: DispatchEventInternal( On 2013/02/07 00:10:13, Matt Perry wrote: > I know it's not from this CL, but this is the wrong way to dispatch to incognito > profiles. If you look at DispatchEventInternal, you'll see it sets a > restrict_to_profile var on the event parameter. You really only want to call > DispatchEvent once, with restrict_to_profile NULL in the case when > include_incognito is true. > > The reason for this is that restrict_to_profile is ignored for an > incognito-enabled spanning extension. So these events would be dispatched twice > to such extensions. Done. https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... chrome/common/extensions/api/downloads.idl:16: // object in order to override a download's target filename. On 2013/02/05 22:00:02, mkearney1 wrote: > Should this link to filename? I can't see the staged docs at the moment to check > the flow, so maybe it isn't necessary. Done. https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... chrome/common/extensions/api/downloads.idl:442: // opportunity to override the target filename. Handlers may return null in On 2013/02/05 22:00:02, mkearney1 wrote: > Should target filename be a link? Done. https://codereview.chromium.org/11574006/diff/148001/chrome/common/extensions... chrome/common/extensions/api/downloads.idl:449: static void onDeterminingFilename(DownloadItem downloadItem); On 2013/02/05 21:57:10, Matt Perry wrote: > On 2013/02/05 21:12:19, benjhayden_chromium wrote: > > On 2013/02/05 20:49:39, Matt Perry wrote: > > > On 2013/02/05 15:08:05, benjhayden_chromium wrote: > > > > On 2013/02/04 22:30:19, Matt Perry wrote: > > > > > Does this really need to be a synchronous API? What if we did this > > instead: > > > > > - Downloads system starts writing to the default filename immediately. > > > > > - Extensions have a certain amount of time to rename the file to > something > > > > else. > > > > > > > > The download system does already start writing to a temporary "Unconfirmed > > > > XXXXX.crdownload" filename immediately. > > > > I think that Asanka explained to me that it would be untenable in the > > current > > > > download system architecture to determine the filename multiple times. > > > > How much time? What if the system is being slow? > > > > > > OK, sounds like this won't work. > > > > > > > What about when I change the > > > > API to allow extensions to pop up a dialog to allow the user to choose > > whether > > > > to overwrite? > > > > > > Is the prompt blocking from the JavaScript context's point of view? The > > > onDeterminingFilename event requires a synchronous response - any > asynchronous > > > query won't work. > > > > I am planning on augmenting the onDeterminingFilename event in a followup CL > to > > allow extensions to request downloads_custom_bindings.js to pass the handler a > > callback. This future change will only affect downloads_custom_bindings.js. > > Handlers may then look like this: > > chrome.downloads.onDeterminingFilename.addListener(function(item, callback) { > > chrome.tabs.sendMessage(some_tab_id, item, function(response) { > > callback(response); > > }); > > }, true/*handler is asynchronous*/); > > I see. In that case, I'd prefer we always use the callback for consistency. See > https://developer.chrome.com/trunk/extensions/runtime.html#event-onMessage for > an API that functions similarly (you return true from the event handler to > indicate it's asynchronous). Done. https://codereview.chromium.org/11574006/diff/148001/chrome/renderer/resource... File chrome/renderer/resources/extensions/downloads_custom_bindings.js (right): https://codereview.chromium.org/11574006/diff/148001/chrome/renderer/resource... chrome/renderer/resources/extensions/downloads_custom_bindings.js:25: new chrome.Event(eventName, opt_argSchemas, opt_eventOptions); On 2013/02/05 21:57:10, Matt Perry wrote: > On 2013/02/05 21:12:19, benjhayden_chromium wrote: > > On 2013/02/05 20:49:39, Matt Perry wrote: > > > I'm not a JS expert, but I believe it's possible to just do "return new > > > chrome.Event(...);" > > > > > > That way you only use a DownloadsEvent for onDeterminingFilename. > > > > Unfortunately, registerCustomEvent does 'new DownloadsEvent'. DownloadsEvent > is > > an object class, not a function. > > I know, but I think it still works. For example, this code works for me: > function Parent(x) { this.x_ = x; } > function Child(x) { return new Parent(x); } > y = new Child(7); // y is a Parent 8-O ... Done. https://codereview.chromium.org/11574006/diff/148001/chrome/renderer/resource... chrome/renderer/resources/extensions/downloads_custom_bindings.js:87: DownloadsEvent.prototype.removeListener = function(cb) { On 2013/02/07 00:10:13, Matt Perry wrote: > This is only called when the extension explicitly calls it. You should also > handle the case where the extension page unloads, otherwise you could have an > event registered in the browser that doesn't exist. This can happen if an > extension popup adds a DownloadsEvent listener, for example. When the popup goes > away, we should remove the filename determiner. > > The webRequest API handles this by hooking into EventRouter::OnListenerRemoved, > which is called on page unload as a result of chrome.Event.prototype.detach_. I already Observe NOTIFICATION_EXTENSION_UNLOADED. Is there a reason that I should use EventRouter::OnListenerRemoved instead?
https://codereview.chromium.org/11574006/diff/148001/chrome/renderer/resource... File chrome/renderer/resources/extensions/downloads_custom_bindings.js (right): https://codereview.chromium.org/11574006/diff/148001/chrome/renderer/resource... chrome/renderer/resources/extensions/downloads_custom_bindings.js:87: DownloadsEvent.prototype.removeListener = function(cb) { On 2013/02/21 22:29:33, benjhayden_chromium wrote: > On 2013/02/07 00:10:13, Matt Perry wrote: > > This is only called when the extension explicitly calls it. You should also > > handle the case where the extension page unloads, otherwise you could have an > > event registered in the browser that doesn't exist. This can happen if an > > extension popup adds a DownloadsEvent listener, for example. When the popup > goes > > away, we should remove the filename determiner. > > > > The webRequest API handles this by hooking into > EventRouter::OnListenerRemoved, > > which is called on page unload as a result of chrome.Event.prototype.detach_. > > I already Observe NOTIFICATION_EXTENSION_UNLOADED. Is there a reason that I > should use EventRouter::OnListenerRemoved instead? Yes - in the case of a popup closing, for example. The extension will not be unloaded, but the listener will be removed.
https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1505: // events with on-record extension renderers. This comment is a bit cryptic to me. The main quirk with what you're doing is that on-record events are not restricted to a profile, even in the split-mode case. I would add a comment to that effect. https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1510: ExtensionDownloadsEventRouter::DownloadsNotificationSource nit: you're already in this scope so you can drop the ExtensionDownloadsEventRouter:: prefix https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1514: content::Source<ExtensionDownloadsEventRouter::DownloadsNotificationSource> same here https://codereview.chromium.org/11574006/diff/199003/chrome/common/extensions... File chrome/common/extensions/docs/examples/api/downloads/downloads_overwrite/bg.js (right): https://codereview.chromium.org/11574006/diff/199003/chrome/common/extensions... chrome/common/extensions/docs/examples/api/downloads/downloads_overwrite/bg.js:8: chrome.downloads.onDeterminingFilename.addListener(function(item) { This needs to be updated. https://codereview.chromium.org/11574006/diff/199003/chrome/renderer/resource... File chrome/renderer/resources/extensions/downloads_custom_bindings.js (right): https://codereview.chromium.org/11574006/diff/199003/chrome/renderer/resource... chrome/renderer/resources/extensions/downloads_custom_bindings.js:60: return; set suggestable to false here too https://codereview.chromium.org/11574006/diff/199003/chrome/renderer/resource... chrome/renderer/resources/extensions/downloads_custom_bindings.js:78: cb.apply(null, [downloadItem, suggest]); I'd recommend requiring the user to return true from the onDeterminingFilename to indicate he wants to call |suggest| asynchronously. If they don't return true, and suggest was not called, have it be an error and call determineFilename yourself. The reason is to make it explicit that the user intended to call |suggest| asynchronously. ex: chrome.downloads.onDeterminingFilename.addListener(function(item, suggest) { if (item.filename.endsWith(".jpg")) { suggest("images/" + item.filename); } else { // handle asynchronously. window.setTimeout(function() { handle(item, suggest); }, 0); return true; } }); Note that if I forget the "else" branch, chrome should call determineFilename for me. The lack of return value helps chrome know when to do so.
https://codereview.chromium.org/11574006/diff/199003/chrome/browser/download/... File chrome/browser/download/chrome_download_manager_delegate.cc (right): https://codereview.chromium.org/11574006/diff/199003/chrome/browser/download/... chrome/browser/download/chrome_download_manager_delegate.cc:619: struct ChromeDownloadManagerDelegate::ContinueFilenameDeterminationInfo { Shall we move this to the top of the source file? https://codereview.chromium.org/11574006/diff/199003/chrome/browser/download/... chrome/browser/download/chrome_download_manager_delegate.cc:738: changed_filename)); Also normalize path separators. Or we might end up with "C:\Users\foo\Downloads/bar/image.jpg" https://codereview.chromium.org/11574006/diff/199003/chrome/browser/download/... chrome/browser/download/chrome_download_manager_delegate.cc:739: net::GenerateSafeFileName(download->GetMimeType(), false, &temp_filename); This could prevent the extension from setting a filename that doesn't have an extension if the MIME type has a default extension. I.e. if the MIME type is 'text/plain' and temp_filename is '/foo/bar', then after calling net::GenerateSafeFileName, temp_filename would be '/foo/bar.txt'. If you just want to make sure that the filename is safe, then you can omit the MIME type. It's only used for generating an extension. https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:193: return !filename.empty() && "foo/" shouldn't be considered a valid filename either. https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:609: found_info = true; Curious: Is it possible for there to be more than one match? https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1293: data->ClearPendingDeterminers(); Is it possible for there to be a pending determiner for this download item? It is possible for a single DownloadItem to determine the download target multiple times due to download resumption. However, it shouldn't be possible for these to overlap. https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right): https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:179: LOG(INFO) << "occam caught " << new_event->Debug(); I'm guessing these LOG statements will be removed.
Last comment from me (I believe). Sorry about zoning out for a while--I wanted to wait for the incognito/regular discussion to finish, which meant I didn't have an obvious reminder that I needed to finish this review :-}. https://codereview.chromium.org/11574006/diff/179007/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right): https://codereview.chromium.org/11574006/diff/179007/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2558: override = base::FilePath(FILE_PATH_LITERAL("C:\\sneaky.dll")); Could you make these file targets be absolute paths in a directory you know you can write to? I want to be testing *just* that we're rejecting absolute paths, and nothing else.
Everybody: PTAL. Docs should still be staged at the usual link. Thanks! https://codereview.chromium.org/11574006/diff/148001/chrome/renderer/resource... File chrome/renderer/resources/extensions/downloads_custom_bindings.js (right): https://codereview.chromium.org/11574006/diff/148001/chrome/renderer/resource... chrome/renderer/resources/extensions/downloads_custom_bindings.js:87: DownloadsEvent.prototype.removeListener = function(cb) { On 2013/02/21 23:07:49, Matt Perry wrote: > On 2013/02/21 22:29:33, benjhayden_chromium wrote: > > On 2013/02/07 00:10:13, Matt Perry wrote: > > > This is only called when the extension explicitly calls it. You should also > > > handle the case where the extension page unloads, otherwise you could have > an > > > event registered in the browser that doesn't exist. This can happen if an > > > extension popup adds a DownloadsEvent listener, for example. When the popup > > goes > > > away, we should remove the filename determiner. > > > > > > The webRequest API handles this by hooking into > > EventRouter::OnListenerRemoved, > > > which is called on page unload as a result of > chrome.Event.prototype.detach_. > > > > I already Observe NOTIFICATION_EXTENSION_UNLOADED. Is there a reason that I > > should use EventRouter::OnListenerRemoved instead? > > Yes - in the case of a popup closing, for example. The extension will not be > unloaded, but the listener will be removed. Done -- I assumed that I should also use OnListenerAdded instead of AddFilenameDeterminer. Is that assumption correct? https://codereview.chromium.org/11574006/diff/199003/chrome/browser/download/... File chrome/browser/download/chrome_download_manager_delegate.cc (right): https://codereview.chromium.org/11574006/diff/199003/chrome/browser/download/... chrome/browser/download/chrome_download_manager_delegate.cc:619: struct ChromeDownloadManagerDelegate::ContinueFilenameDeterminationInfo { On 2013/02/22 18:55:48, asanka wrote: > Shall we move this to the top of the source file? You mean to the anonymous namespace? Can't do -- it's passed between methods, so it needs to be declared in the header, so it should be declared inside the class. Failing that, I thought that closest to its usage would be best. https://codereview.chromium.org/11574006/diff/199003/chrome/browser/download/... chrome/browser/download/chrome_download_manager_delegate.cc:738: changed_filename)); On 2013/02/22 18:55:48, asanka wrote: > Also normalize path separators. Or we might end up with > "C:\Users\foo\Downloads/bar/image.jpg" Done. https://codereview.chromium.org/11574006/diff/199003/chrome/browser/download/... chrome/browser/download/chrome_download_manager_delegate.cc:739: net::GenerateSafeFileName(download->GetMimeType(), false, &temp_filename); On 2013/02/22 18:55:48, asanka wrote: > This could prevent the extension from setting a filename that doesn't have an > extension if the MIME type has a default extension. I.e. if the MIME type is > 'text/plain' and temp_filename is '/foo/bar', then after calling > net::GenerateSafeFileName, temp_filename would be '/foo/bar.txt'. > > If you just want to make sure that the filename is safe, then you can omit the > MIME type. It's only used for generating an extension. Done. https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:193: return !filename.empty() && On 2013/02/22 18:55:48, asanka wrote: > "foo/" shouldn't be considered a valid filename either. I believe that the next line handles that case. https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:609: found_info = true; On 2013/02/22 18:55:48, asanka wrote: > Curious: Is it possible for there to be more than one match? I hope not. :-) See AddPendingDeterminer and search for "already_added" in the newest patchset. https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1293: data->ClearPendingDeterminers(); On 2013/02/22 18:55:48, asanka wrote: > Is it possible for there to be a pending determiner for this download item? > > It is possible for a single DownloadItem to determine the download target > multiple times due to download resumption. However, it shouldn't be possible for > these to overlap. Yep, Randy suggested clearing pending determiners here just for the resumption filename re-determination case. If an onDeterminingFilename handler is holding off on suggest()ing a filename, and the user pauses and resumes a download in a way that triggers re-determination, then this will ClearPendingDeterminers. https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1505: // events with on-record extension renderers. On 2013/02/22 01:43:43, Matt Perry wrote: > This comment is a bit cryptic to me. > > The main quirk with what you're doing is that on-record events are not > restricted to a profile, even in the split-mode case. I would add a comment to > that effect. Done. https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1510: ExtensionDownloadsEventRouter::DownloadsNotificationSource On 2013/02/22 01:43:43, Matt Perry wrote: > nit: you're already in this scope so you can drop the > ExtensionDownloadsEventRouter:: prefix Done. https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1514: content::Source<ExtensionDownloadsEventRouter::DownloadsNotificationSource> On 2013/02/22 01:43:43, Matt Perry wrote: > same here Done. https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right): https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:179: LOG(INFO) << "occam caught " << new_event->Debug(); On 2013/02/22 18:55:48, asanka wrote: > I'm guessing these LOG statements will be removed. Done. https://codereview.chromium.org/11574006/diff/199003/chrome/common/extensions... File chrome/common/extensions/docs/examples/api/downloads/downloads_overwrite/bg.js (right): https://codereview.chromium.org/11574006/diff/199003/chrome/common/extensions... chrome/common/extensions/docs/examples/api/downloads/downloads_overwrite/bg.js:8: chrome.downloads.onDeterminingFilename.addListener(function(item) { On 2013/02/22 01:43:43, Matt Perry wrote: > This needs to be updated. Done. https://codereview.chromium.org/11574006/diff/199003/chrome/renderer/resource... File chrome/renderer/resources/extensions/downloads_custom_bindings.js (right): https://codereview.chromium.org/11574006/diff/199003/chrome/renderer/resource... chrome/renderer/resources/extensions/downloads_custom_bindings.js:60: return; On 2013/02/22 01:43:43, Matt Perry wrote: > set suggestable to false here too Done. https://codereview.chromium.org/11574006/diff/199003/chrome/renderer/resource... chrome/renderer/resources/extensions/downloads_custom_bindings.js:78: cb.apply(null, [downloadItem, suggest]); On 2013/02/22 01:43:43, Matt Perry wrote: > I'd recommend requiring the user to return true from the onDeterminingFilename > to indicate he wants to call |suggest| asynchronously. If they don't return > true, and suggest was not called, have it be an error and call determineFilename > yourself. > > The reason is to make it explicit that the user intended to call |suggest| > asynchronously. > > ex: > chrome.downloads.onDeterminingFilename.addListener(function(item, suggest) { > if (item.filename.endsWith(".jpg")) { > suggest("images/" + item.filename); > } else { > // handle asynchronously. > window.setTimeout(function() { handle(item, suggest); }, 0); > return true; > } > }); > > Note that if I forget the "else" branch, chrome should call determineFilename > for me. The lack of return value helps chrome know when to do so. Done. https://codereview.chromium.org/11574006/diff/179007/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right): https://codereview.chromium.org/11574006/diff/179007/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:2558: override = base::FilePath(FILE_PATH_LITERAL("C:\\sneaky.dll")); On 2013/02/22 19:09:11, rdsmith wrote: > Could you make these file targets be absolute paths in a directory you know you > can write to? I want to be testing *just* that we're rejecting absolute paths, > and nothing else. Done.
lgtm
https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/199003/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:193: return !filename.empty() && On 2013/02/22 20:43:23, benjhayden_chromium wrote: > On 2013/02/22 18:55:48, asanka wrote: > > "foo/" shouldn't be considered a valid filename either. > > I believe that the next line handles that case. For "foo/" BaseName() returns "foo".
Arg... You're going to hate me. I just realized this won't play nice with event
pages. We don't actually want to remove determiners when listeners are removed,
because listeners are removed each time event pages unload.
Also, the sub_event_id thing will result in us accumulating an endless amount of
determiners as event pages reload. I'm sorry, I should've realized this much
sooner.
I think the best solution is to just drop the sub_event_id property. This should
also simplify the code a bit. The only problem is that multiple event listeners
in a single extension will race against each other - but multiple determiners
don't play nicely anyway, so maybe this isn't a big loss.
Basically, follow the omnibox.onInputChanged pattern. You can simplify your
custom bindings by registering an "argument massager"[1] which takes the
download_id and feeds it into the event's suggest callback.
You should also drop the code that listens for OnListenerAdded/Removed. Instead,
just check EventRouter::HasEventListener("downloads.onDeterminingFilename") in
ExtensionDownloadsEventRouter::OnDeterminingFilename.
[1] See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...
https://codereview.chromium.org/11574006/diff/200012/chrome/renderer/resource...
File chrome/renderer/resources/extensions/downloads_custom_bindings.js (right):
https://codereview.chromium.org/11574006/diff/200012/chrome/renderer/resource...
chrome/renderer/resources/extensions/downloads_custom_bindings.js:58: return;
Also print an error (console.error) explaining how suggest can only be called
once.
On 2013/02/22 22:53:06, Matt Perry wrote:
> Arg... You're going to hate me. I just realized this won't play nice with
event
> pages. We don't actually want to remove determiners when listeners are
removed,
> because listeners are removed each time event pages unload.
> Also, the sub_event_id thing will result in us accumulating an endless amount
of
> determiners as event pages reload. I'm sorry, I should've realized this much
> sooner.
Yeah, I noticed a while ago that it doesn't play nice with event pages, but I
didn't know if that would be acceptable or if there was a way around it. I
should have mentioned it.
>
> I think the best solution is to just drop the sub_event_id property. This
should
> also simplify the code a bit. The only problem is that multiple event
listeners
> in a single extension will race against each other - but multiple determiners
> don't play nicely anyway, so maybe this isn't a big loss.
>
> Basically, follow the omnibox.onInputChanged pattern. You can simplify your
> custom bindings by registering an "argument massager"[1] which takes the
> download_id and feeds it into the event's suggest callback.
Does the argument massager's |dispatch| return the handler's return value? Can I
say 'if (dispatch(...) !== true) suggestCallback();'?
Should I use registerCustomEvent/DownloadsEvent to log an error when an
extension registers more than one listener? Is registerCustomEvent compatible
with registerArgumentMassager?
>
> You should also drop the code that listens for OnListenerAdded/Removed.
Instead,
> just check EventRouter::HasEventListener("downloads.onDeterminingFilename") in
> ExtensionDownloadsEventRouter::OnDeterminingFilename.
So, just to double-check, HasEventListener is sensitive to event pages?
Are there plans to switch the webRequest API over to HasEventListener?
Could I add comments to the web request implementation warning against copying
it?
>
> [1] See
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...
>
>
https://codereview.chromium.org/11574006/diff/200012/chrome/renderer/resource...
> File chrome/renderer/resources/extensions/downloads_custom_bindings.js
(right):
>
>
https://codereview.chromium.org/11574006/diff/200012/chrome/renderer/resource...
> chrome/renderer/resources/extensions/downloads_custom_bindings.js:58: return;
> Also print an error (console.error) explaining how suggest can only be called
> once.
On 2013/02/25 16:28:06, benjhayden_chromium wrote: > On 2013/02/22 22:53:06, Matt Perry wrote: > > Arg... You're going to hate me. I just realized this won't play nice with > event > > pages. We don't actually want to remove determiners when listeners are > removed, > > because listeners are removed each time event pages unload. > > Also, the sub_event_id thing will result in us accumulating an endless amount > of > > determiners as event pages reload. I'm sorry, I should've realized this much > > sooner. > > Yeah, I noticed a while ago that it doesn't play nice with event pages, but I > didn't know if that would be acceptable or if there was a way around it. I > should have mentioned it. > > > > > I think the best solution is to just drop the sub_event_id property. This > should > > also simplify the code a bit. The only problem is that multiple event > listeners > > in a single extension will race against each other - but multiple determiners > > don't play nicely anyway, so maybe this isn't a big loss. > > > > Basically, follow the omnibox.onInputChanged pattern. You can simplify your > > custom bindings by registering an "argument massager"[1] which takes the > > download_id and feeds it into the event's suggest callback. > > Does the argument massager's |dispatch| return the handler's return value? Can I > say 'if (dispatch(...) !== true) suggestCallback();'? Looks like it doesn't - but it's easy enough to fix. Open up event.js line 210: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re... and update dispatchArgs to return result. result.results will be an array of all the return values for all event handlers that were called. For an example of its usage, see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re... > > Should I use registerCustomEvent/DownloadsEvent to log an error when an > extension registers more than one listener? Is registerCustomEvent compatible > with registerArgumentMassager? That's not a bad idea. I'll leave it up to you. (Printing a warning on multiple calls to the suggest() callback works too, IMO.) If you use registerCustomEvent, you can skip argumentMassager, because the former is more powerful. > > > > You should also drop the code that listens for OnListenerAdded/Removed. > Instead, > > just check EventRouter::HasEventListener("downloads.onDeterminingFilename") in > > > ExtensionDownloadsEventRouter::OnDeterminingFilename. > > So, just to double-check, HasEventListener is sensitive to event pages? Yes. > Are there plans to switch the webRequest API over to HasEventListener? No - we just make it an error for extensions to use webRequest with event pages. We're hoping declarativeWebRequest will replace it. > Could I add comments to the web request implementation warning against copying > it? That would be great, thanks! > > > > [1] See > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re... > > > > > https://codereview.chromium.org/11574006/diff/200012/chrome/renderer/resource... > > File chrome/renderer/resources/extensions/downloads_custom_bindings.js > (right): > > > > > https://codereview.chromium.org/11574006/diff/200012/chrome/renderer/resource... > > chrome/renderer/resources/extensions/downloads_custom_bindings.js:58: return; > > Also print an error (console.error) explaining how suggest can only be called > > once.
Matt: PTAL
Almost there! Thanks for all your patience. I think the code ended up a lot cleaner with the previous simplifications. https://codereview.chromium.org/11574006/diff/211001/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/211001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:577: void DeterminerRemoved(const std::string& extension_id) { This is now the same as ExtensionUnloaded. https://codereview.chromium.org/11574006/diff/211001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:593: for (int index = 0; index < static_cast<int>(determiners_.size()); nit: use size_t index and avoid the case https://codereview.chromium.org/11574006/diff/211001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:609: for (int index = 0; index < static_cast<int>(determiners_.size()); ditto https://codereview.chromium.org/11574006/diff/211001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:613: // maxListeners=1 in downloads.idl and suggestCallback in Does maxListeners work across pages? E.g. if an extension has a popup and an event page, can both register a listener? https://codereview.chromium.org/11574006/diff/211001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1224: for (extensions::EventListenerMap::ListenerList::const_iterator list_iter = It might be easier for you to use Event::will_dispatch_callback. Call BroadcastEvent first, and it will call that callback for each event that it dispatches. If it is never called, you can just call no_change.Run(). Otherwise, set up the PendingDeterminers. The main advantage is that you don't have to worry about a logic mismatch between this and BroadcastEvent. (i.e. BroadcastEvent might decide not to dispatch the event based on some other criteria, and you'd be forever waiting for the event to respond.) will_dispatch_callback is called synchronously, so you can pass it values from the stack in order to get your results. https://codereview.chromium.org/11574006/diff/211001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1232: !extension->incognito_split_mode())) { I think you want extension_service->CanCrossIncognito(extension). That will factor in whether the extension was enabled in incognito mode. https://codereview.chromium.org/11574006/diff/211001/chrome/common/extensions... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/11574006/diff/211001/chrome/common/extensions... chrome/common/extensions/api/downloads.idl:332: callback SuggestFilenameCallback = void(optional FilenameSuggestion suggestion); nit: line length https://codereview.chromium.org/11574006/diff/211001/chrome/renderer/resource... File chrome/renderer/resources/extensions/downloads_custom_bindings.js (right): https://codereview.chromium.org/11574006/diff/211001/chrome/renderer/resource... chrome/renderer/resources/extensions/downloads_custom_bindings.js:38: var ext_async = (results && nit: extAsync
PTAL. Thank you so much for your help! https://codereview.chromium.org/11574006/diff/211001/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/11574006/diff/211001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:577: void DeterminerRemoved(const std::string& extension_id) { On 2013/02/27 22:33:47, Matt Perry wrote: > This is now the same as ExtensionUnloaded. Done. https://codereview.chromium.org/11574006/diff/211001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:593: for (int index = 0; index < static_cast<int>(determiners_.size()); On 2013/02/27 22:33:47, Matt Perry wrote: > nit: use size_t index and avoid the case Done. https://codereview.chromium.org/11574006/diff/211001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:609: for (int index = 0; index < static_cast<int>(determiners_.size()); On 2013/02/27 22:33:47, Matt Perry wrote: > ditto Done. https://codereview.chromium.org/11574006/diff/211001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:613: // maxListeners=1 in downloads.idl and suggestCallback in On 2013/02/27 22:33:47, Matt Perry wrote: > Does maxListeners work across pages? E.g. if an extension has a popup and an > event page, can both register a listener? No and yes. Added DeterminerAlreadyReported() and delayed clearing determiners_ and made DetermineFilename() return a helpful error message. https://codereview.chromium.org/11574006/diff/211001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1224: for (extensions::EventListenerMap::ListenerList::const_iterator list_iter = On 2013/02/27 22:33:47, Matt Perry wrote: > It might be easier for you to use Event::will_dispatch_callback. Call > BroadcastEvent first, and it will call that callback for each event that it > dispatches. If it is never called, you can just call no_change.Run(). Otherwise, > set up the PendingDeterminers. The main advantage is that you don't have to > worry about a logic mismatch between this and BroadcastEvent. (i.e. > BroadcastEvent might decide not to dispatch the event based on some other > criteria, and you'd be forever waiting for the event to respond.) > > will_dispatch_callback is called synchronously, so you can pass it values from > the stack in order to get your results. Done. https://codereview.chromium.org/11574006/diff/211001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:1232: !extension->incognito_split_mode())) { On 2013/02/27 22:33:47, Matt Perry wrote: > I think you want extension_service->CanCrossIncognito(extension). That will > factor in whether the extension was enabled in incognito mode. moot https://codereview.chromium.org/11574006/diff/211001/chrome/common/extensions... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/11574006/diff/211001/chrome/common/extensions... chrome/common/extensions/api/downloads.idl:332: callback SuggestFilenameCallback = void(optional FilenameSuggestion suggestion); On 2013/02/27 22:33:47, Matt Perry wrote: > nit: line length Done. https://codereview.chromium.org/11574006/diff/211001/chrome/renderer/resource... File chrome/renderer/resources/extensions/downloads_custom_bindings.js (right): https://codereview.chromium.org/11574006/diff/211001/chrome/renderer/resource... chrome/renderer/resources/extensions/downloads_custom_bindings.js:38: var ext_async = (results && On 2013/02/27 22:33:47, Matt Perry wrote: > nit: extAsync Done.
LGTM! Thanks again for putting up with all my comments.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/11574006/231003
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/11574006/232009
Retried try job too often on win_rel for step(s) sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/11574006/232009
Message was sent while issue was closed.
Change committed as 185811 |
