Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9651020:
Pass content-type resources to web intents. Goes through download, then invokes the p… (Closed)
|
Created:
8 years, 9 months ago by Greg Billock Modified:
8 years, 8 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, rdsmith+dwatch_chromium.org, benwells Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPass content-type resources to web intents. Goes through download, then invokes the picker.
Code to handle policy and intent payload creation is embedder-side.
New API for dispatching the browser-initiated internal intent added to content.
BUG=105732
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=131535
Patch Set 1 #Patch Set 2 : Add ability to send unserialized intent data from browser process. #
Total comments: 14
Patch Set 3 : Move dispatch logic client-side. #
Total comments: 5
Patch Set 4 : Internal dispatcher checked in. #Patch Set 5 : Take out policy changes #Patch Set 6 : Add security policy hole-punch for blob file. #Patch Set 7 : Move code embedder-side #
Total comments: 4
Patch Set 8 : Move to an internal Create method. #
Total comments: 11
Patch Set 9 : Move static method location #
Total comments: 4
Patch Set 10 : Add TODO. #Patch Set 11 : Improve security policy invocation #Patch Set 12 : Fix string handling for getExtra. #Patch Set 13 : Move Create method to impl cc file #
Total comments: 8
Patch Set 14 : Rebase to head #
Total comments: 2
Patch Set 15 : Pass FilePath #
Total comments: 2
Patch Set 16 : Remove includes #Messages
Total messages: 44 (0 generated)
This code now works for a main use case, but needs some discussion. Mechanically, I'm inclined to split off the parts adding the ability to pass unserialized data through to the renderer to make this easier to read. There's also additional information needed there for MessagePort support as it exists now. Second, I'm interested in your views of what to do about Blobs. This code is very close to supporting Blob passing, which I think we want to ultimately do. In that case, we can put the url in the extras, but we do need to do some work for non-binary types (so the data comes in as a string in that case. (According to what we think will be the interchange spec.) I'm kind of inclined to check in the support for simply passing the URL, and adding Blob support later. That's pretty simple and exercises the pathway. It is as good as registerContentHandler, and so gets some immediate wins. On the other hand, Blob/file content is the ultimate goal, so maybe I should wrap it all up here. I've also made some adjustments here to allow us to handle feed URLs. This code will support the use case of registering an app like Google Reader to subscribe to feeds, as well as the docs use case of handing types like application/ms-word and such. (i.e., all RCH). Shall I keep those, or save that for a separate change?
http://codereview.chromium.org/9651020/diff/2001/content/browser/renderer_hos... File content/browser/renderer_host/buffered_resource_handler.cc (right): http://codereview.chromium.org/9651020/diff/2001/content/browser/renderer_hos... content/browser/renderer_host/buffered_resource_handler.cc:186: /* are these comments intended?
I'm mostly just looking at the blob handling aspects of this. The closest example of what's being done here can be found in extension_page_capture_api.cc. In that usage, a temp file is created and a reference to that file is given to script in the form of a WebCore::Blob, when that Blob is gc'd the backing temp file is deleted. It's not clear to me you either want/need the automagic deletion stuff? See other comments below... Another question, how does PostMessage come into play for this? https://chromiumcodereview.appspot.com/9651020/diff/2001/chrome/browser/downl... File chrome/browser/download/chrome_download_manager_delegate.cc (right): https://chromiumcodereview.appspot.com/9651020/diff/2001/chrome/browser/downl... chrome/browser/download/chrome_download_manager_delegate.cc:98: // TODO: Check for explicit user-caused download? Should this also check for mimes for which there actually are registered 'intent' handlers? Seems like the "to webintent or not to webintent" decision making needs to refinement? https://chromiumcodereview.appspot.com/9651020/diff/2001/chrome/browser/downl... chrome/browser/download/chrome_download_manager_delegate.cc:228: content::WebIntentsDispatcher* CreateWebIntentsDispatcherFor( At this point, we've fully downloaded a file to the Downloads folder and now we want to provide a reference to that file to the 'intent handler' script as a 'blob'... is that right? Some questions... * Does/should this file be visible in the downloads tray? * Should this file be automagically deleted at some point or should it remain in the users Downloads folder? Depending on the answers, things would probably be different in how to handle it. I think these answers to those questions, YES and NO, make for the simplest implementation with regard to blob handling. In particular the answer to the second question (dont automagically delete) makes the real difference. Assuming for the moment we don't automagically delete these files... I think you just want to send the full file path (not a blob url) to the WebIntentHost running in the recipient renderer process. And to use the WebBlob::createFromFile(full_file_path, file_size) method to create the blob, that method expects a FilePath as input. And that's it... no need to muck with blob storage controller and such. If we do want automagic deletion... things would get a little more complicated... let me know if that's the behavior you really want. https://chromiumcodereview.appspot.com/9651020/diff/2001/chrome/browser/downl... chrome/browser/download/chrome_download_manager_delegate.cc:243: base::Time::Now()); just use base::Time() here to indicate "doesn't matter" https://chromiumcodereview.appspot.com/9651020/diff/2001/chrome/browser/downl... chrome/browser/download/chrome_download_manager_delegate.cc:295: // Update text/completion indicator on shelf. just curious... what will the shelf indicator say about this file? https://chromiumcodereview.appspot.com/9651020/diff/2001/content/renderer/web... File content/renderer/web_intents_host.cc (right): https://chromiumcodereview.appspot.com/9651020/diff/2001/content/renderer/web... content/renderer/web_intents_host.cc:61: WebBlob web_blob = WebBlob::createFromFile(intent.blob_url, i think this method is expecting a full file path as input, not a url
https://chromiumcodereview.appspot.com/9651020/diff/2001/chrome/browser/downl... File chrome/browser/download/chrome_download_manager_delegate.cc (right): https://chromiumcodereview.appspot.com/9651020/diff/2001/chrome/browser/downl... chrome/browser/download/chrome_download_manager_delegate.cc:98: // TODO: Check for explicit user-caused download? Yes, it needs refinement. Checking the registry for registered handlers is probably in the future. The current mission is to support RCH-style pluggability for a whitelist of types. We may need a policy decision on whether we should handle those with web intents when we know there are available applications. In other words, this is more of a dummy proof of concept than what's intended as the actual policy. On 2012/03/12 21:16:12, michaeln wrote: > Should this also check for mimes for which there actually are registered > 'intent' handlers? Seems like the "to webintent or not to webintent" decision > making needs to refinement? https://chromiumcodereview.appspot.com/9651020/diff/2001/chrome/browser/downl... chrome/browser/download/chrome_download_manager_delegate.cc:228: content::WebIntentsDispatcher* CreateWebIntentsDispatcherFor( On 2012/03/12 21:16:12, michaeln wrote: > At this point, we've fully downloaded a file to the Downloads folder and now we > want to provide a reference to that file to the 'intent handler' script as a > 'blob'... is that right? Yes, that's right. > > Some questions... > > * Does/should this file be visible in the downloads tray? I think so, yes. This is to provide an affordance to the user while the file is downloading (cancel, resume, etc.) and also a fallback handling if they don't choose to do anything with the file using intents. (They can still have the downloaded file there.) > * Should this file be automagically deleted at some point or should it remain in > the users Downloads folder? I'm inclined to leave it in the downloads folder, but this policy, like the web-intents-triggering one, we should probably have reviewed. > Depending on the answers, things would probably be different in how to handle > it. I think these answers to those questions, YES and NO, make for the simplest > implementation with regard to blob handling. In particular the answer to the > second question (dont automagically delete) makes the real difference. > > Assuming for the moment we don't automagically delete these files... > > I think you just want to send the full file path (not a blob url) to the > WebIntentHost running in the recipient renderer process. And to use the > WebBlob::createFromFile(full_file_path, file_size) method to create the blob, > that method expects a FilePath as input. And that's it... no need to muck with > blob storage controller and such. Ah! That'd be great! I'll get the requirements process engaged to decide on the policy, as this would be really straightforward. We'd need some sandboxing access set, which we discussed, but that'd be the only wrinkle I know of. > If we do want automagic deletion... things would get a little more > complicated... let me know if that's the behavior you really want. https://chromiumcodereview.appspot.com/9651020/diff/2001/chrome/browser/downl... chrome/browser/download/chrome_download_manager_delegate.cc:243: base::Time::Now()); On 2012/03/12 21:16:12, michaeln wrote: > just use base::Time() here to indicate "doesn't matter" Done. https://chromiumcodereview.appspot.com/9651020/diff/2001/chrome/browser/downl... chrome/browser/download/chrome_download_manager_delegate.cc:295: // Update text/completion indicator on shelf. On 2012/03/12 21:16:12, michaeln wrote: > just curious... what will the shelf indicator say about this file? Not sure yet... https://chromiumcodereview.appspot.com/9651020/diff/2001/content/browser/rend... File content/browser/renderer_host/buffered_resource_handler.cc (right): https://chromiumcodereview.appspot.com/9651020/diff/2001/content/browser/rend... content/browser/renderer_host/buffered_resource_handler.cc:186: /* On 2012/03/10 01:02:14, darin wrote: > are these comments intended? They're needed to enable feed content type handling. Currently we're "handling" these types by overwriting the type to plain text. https://chromiumcodereview.appspot.com/9651020/diff/2001/content/renderer/web... File content/renderer/web_intents_host.cc (right): https://chromiumcodereview.appspot.com/9651020/diff/2001/content/renderer/web... content/renderer/web_intents_host.cc:61: WebBlob web_blob = WebBlob::createFromFile(intent.blob_url, On 2012/03/12 21:16:12, michaeln wrote: > i think this method is expecting a full file path as input, not a url Yeah, this was essentially because I didn't know what to pass exactly. Now I know the right interface I'll switch this to the right naming. I split off the internal dispatcher and some other changes from here, so I'm going to get those committed and then merge this. I'll get the discussion going in parallel about exactly what to do with requirements.
http://codereview.chromium.org/9651020/diff/2001/chrome/browser/download/chro... File chrome/browser/download/chrome_download_manager_delegate.cc (right): http://codereview.chromium.org/9651020/diff/2001/chrome/browser/download/chro... chrome/browser/download/chrome_download_manager_delegate.cc:93: bool ChromeDownloadManagerDelegate::ShouldWebIntentsHandle( why is this stuff in chrome vs content? it seems generic to the web platform (as opposed to the crx stuff, which is chrome specific)
http://codereview.chromium.org/9651020/diff/2001/chrome/browser/download/chro... File chrome/browser/download/chrome_download_manager_delegate.cc (right): http://codereview.chromium.org/9651020/diff/2001/chrome/browser/download/chro... chrome/browser/download/chrome_download_manager_delegate.cc:93: bool ChromeDownloadManagerDelegate::ShouldWebIntentsHandle( On 2012/03/13 20:05:01, John Abd-El-Malek wrote: > why is this stuff in chrome vs content? it seems generic to the web platform (as > opposed to the crx stuff, which is chrome specific) It could be defined more generically in DownloadManagerDelegate. My current take on the policy I think we'll use is that using web intents for some downloaded content types is a browser-specific policy decision. Tooling this back through the content API to call TabContentsDelegate::WebIntentDispatch would require putting some more tooling in the caller of ShouldOpenDownload. It might end up looking pretty similar, though, i.e. download_item_impl.cc:770 WebIntentsDispatcher* d = delegate_->ShouldOpenWithWebIntents(this); if (d != NULL) { GetWebContents()->delegate->Dispatch(d); Completed(); } Do you think that's a better insertion point? One thing I'm nervous about is that these are all synchronous calls, and if we end up wanting a policy requiring consulting the WebIntentsRegistry, we'll need to make this whole part of the code use a continuation.
On 2012/03/13 20:53:56, Greg Billock wrote: > http://codereview.chromium.org/9651020/diff/2001/chrome/browser/download/chro... > File chrome/browser/download/chrome_download_manager_delegate.cc (right): > > http://codereview.chromium.org/9651020/diff/2001/chrome/browser/download/chro... > chrome/browser/download/chrome_download_manager_delegate.cc:93: bool > ChromeDownloadManagerDelegate::ShouldWebIntentsHandle( > On 2012/03/13 20:05:01, John Abd-El-Malek wrote: > > why is this stuff in chrome vs content? it seems generic to the web platform > (as > > opposed to the crx stuff, which is chrome specific) > > It could be defined more generically in DownloadManagerDelegate. My current take > on the policy I think we'll use is that using web intents for some downloaded > content types is a browser-specific policy decision. Tooling this back through > the content API to call TabContentsDelegate::WebIntentDispatch would require > putting some more tooling in the caller of ShouldOpenDownload. It might end up > looking pretty similar, though, i.e. > > download_item_impl.cc:770 > > WebIntentsDispatcher* d = delegate_->ShouldOpenWithWebIntents(this); > if (d != NULL) { > GetWebContents()->delegate->Dispatch(d); > Completed(); > } > > Do you think that's a better insertion point? One thing I'm nervous about is > that these are all synchronous calls, and if we end up wanting a policy > requiring consulting the WebIntentsRegistry, we'll need to make this whole part > of the code use a continuation. I'm not really following everything you're saying. Whatever code in content is calling these embedder callbacks where you're now doing this work, why not just have that do this webintent work? As for sync/async, how does this code being in content/chrome affect things?
On 2012/03/14 00:49:03, John Abd-El-Malek wrote: > On 2012/03/13 20:53:56, Greg Billock wrote: > > > http://codereview.chromium.org/9651020/diff/2001/chrome/browser/download/chro... > > File chrome/browser/download/chrome_download_manager_delegate.cc (right): > > > > > http://codereview.chromium.org/9651020/diff/2001/chrome/browser/download/chro... > > chrome/browser/download/chrome_download_manager_delegate.cc:93: bool > > ChromeDownloadManagerDelegate::ShouldWebIntentsHandle( > > On 2012/03/13 20:05:01, John Abd-El-Malek wrote: > > > why is this stuff in chrome vs content? it seems generic to the web platform > > (as > > > opposed to the crx stuff, which is chrome specific) > > > > It could be defined more generically in DownloadManagerDelegate. My current > take > > on the policy I think we'll use is that using web intents for some downloaded > > content types is a browser-specific policy decision. Tooling this back through > > the content API to call TabContentsDelegate::WebIntentDispatch would require > > putting some more tooling in the caller of ShouldOpenDownload. It might end up > > looking pretty similar, though, i.e. > > > > download_item_impl.cc:770 > > > > WebIntentsDispatcher* d = delegate_->ShouldOpenWithWebIntents(this); > > if (d != NULL) { > > GetWebContents()->delegate->Dispatch(d); > > Completed(); > > } > > > > Do you think that's a better insertion point? One thing I'm nervous about is > > that these are all synchronous calls, and if we end up wanting a policy > > requiring consulting the WebIntentsRegistry, we'll need to make this whole > part > > of the code use a continuation. > > I'm not really following everything you're saying. > > Whatever code in content is calling these embedder callbacks where you're now > doing this work, why not just have that do this webintent work? As for > sync/async, how does this code being in content/chrome affect things? There's currently no impact on the content-side API at all. All the work is getting done by the delegate during ShouldOpenDownload, an existing API call. We could definitely move to have this work get done within the content code: it'll require a new API, and we have to figure out what that ought to be (just policy? creating the dispatcher?). WRT sync/async, I am arguing for making this decision synchronous, in part just to keep this implementation simpler and more predictable. But a very possible evolution is that we want it to be asynchronous, in order to account for arbitrary intent handlers. That impacts how we should think about the embedder/content divide is all.
On 2012/03/14 16:14:28, Greg Billock wrote: > On 2012/03/14 00:49:03, John Abd-El-Malek wrote: > > On 2012/03/13 20:53:56, Greg Billock wrote: > > > > > > http://codereview.chromium.org/9651020/diff/2001/chrome/browser/download/chro... > > > File chrome/browser/download/chrome_download_manager_delegate.cc (right): > > > > > > > > > http://codereview.chromium.org/9651020/diff/2001/chrome/browser/download/chro... > > > chrome/browser/download/chrome_download_manager_delegate.cc:93: bool > > > ChromeDownloadManagerDelegate::ShouldWebIntentsHandle( > > > On 2012/03/13 20:05:01, John Abd-El-Malek wrote: > > > > why is this stuff in chrome vs content? it seems generic to the web > platform > > > (as > > > > opposed to the crx stuff, which is chrome specific) > > > > > > It could be defined more generically in DownloadManagerDelegate. My current > > take > > > on the policy I think we'll use is that using web intents for some > downloaded > > > content types is a browser-specific policy decision. Tooling this back > through > > > the content API to call TabContentsDelegate::WebIntentDispatch would require > > > putting some more tooling in the caller of ShouldOpenDownload. It might end > up > > > looking pretty similar, though, i.e. > > > > > > download_item_impl.cc:770 > > > > > > WebIntentsDispatcher* d = delegate_->ShouldOpenWithWebIntents(this); > > > if (d != NULL) { > > > GetWebContents()->delegate->Dispatch(d); > > > Completed(); > > > } > > > > > > Do you think that's a better insertion point? One thing I'm nervous about is > > > that these are all synchronous calls, and if we end up wanting a policy > > > requiring consulting the WebIntentsRegistry, we'll need to make this whole > > part > > > of the code use a continuation. > > > > I'm not really following everything you're saying. > > > > Whatever code in content is calling these embedder callbacks where you're now > > doing this work, why not just have that do this webintent work? As for > > sync/async, how does this code being in content/chrome affect things? > > There's currently no impact on the content-side API at all. All the work is > getting done by the delegate during ShouldOpenDownload, an existing API call. I wasn't referring to the content API. What I meant is that if this is generic implementation of web intent stuff that every embedder of content would want, it should be in content and not chrome. otherwise every embedder would have to duplicate this. put another way, the include to "#include "content/browser/intents/internal_web_intents_dispatcher.h" in chrome will give you a checkdeps error > We > could definitely move to have this work get done within the content code: it'll > require a new API, why would this need a new API? content is the one that calls ShouldOpenDownload > and we have to figure out what that ought to be (just policy? > creating the dispatcher?). > > WRT sync/async, I am arguing for making this decision synchronous, in part just > to keep this implementation simpler and more predictable. But a very possible > evolution is that we want it to be asynchronous, in order to account for > arbitrary intent handlers. That impacts how we should think about the > embedder/content divide is all. i have to run so i can't look at the impl in detail, but what is happening or might happen in the future, i.e. disk access or any slow operations?
Moved implementation logic to content code. Still a bit cluttered; I'll merge once the other CL lands...
http://codereview.chromium.org/9651020/diff/14001/chrome/browser/download/chr... File chrome/browser/download/chrome_download_manager_delegate.cc (right): http://codereview.chromium.org/9651020/diff/14001/chrome/browser/download/chr... chrome/browser/download/chrome_download_manager_delegate.cc:218: if (mime_type == "application/rss+xml" || why is this stuff in chrome? http://codereview.chromium.org/9651020/diff/14001/content/browser/renderer_ho... File content/browser/renderer_host/buffered_resource_handler.cc (right): http://codereview.chromium.org/9651020/diff/14001/content/browser/renderer_ho... content/browser/renderer_host/buffered_resource_handler.cc:187: /* ? http://codereview.chromium.org/9651020/diff/14001/content/browser/renderer_ho... content/browser/renderer_host/buffered_resource_handler.cc:368: if (type == "application/rss+xml" || why is this code here? it's hard to understand this. isn't/shouldn't there be a centralized place for mime types that are handled by web intents instead of spreading this across different files?
(Note, I'm in China for a few days, so this is going to stagnate a bit...) http://codereview.chromium.org/9651020/diff/14001/chrome/browser/download/chr... File chrome/browser/download/chrome_download_manager_delegate.cc (right): http://codereview.chromium.org/9651020/diff/14001/chrome/browser/download/chr... chrome/browser/download/chrome_download_manager_delegate.cc:218: if (mime_type == "application/rss+xml" || On 2012/03/16 21:36:26, John Abd-El-Malek wrote: > why is this stuff in chrome? This method is basically a policy delegation to the embedder. The content layer says "hey, do you want me to dispatch this download I just did by issuing a web intent?" I think it's possible that different embedders would have different answers to that question. Specifically, maintaining a whitelist of this kind, or deriving it from a list of installed apps, seems like something that should be done chrome-side. http://codereview.chromium.org/9651020/diff/14001/content/browser/renderer_ho... File content/browser/renderer_host/buffered_resource_handler.cc (right): http://codereview.chromium.org/9651020/diff/14001/content/browser/renderer_ho... content/browser/renderer_host/buffered_resource_handler.cc:368: if (type == "application/rss+xml" || On 2012/03/16 21:36:26, John Abd-El-Malek wrote: > why is this code here? it's hard to understand this. isn't/shouldn't there be a > centralized place for mime types that are handled by web intents instead of > spreading this across different files? This is working around a hack in IsSupportedMimeType, which claims the browser "supports" feed types (which it does by mashing them to text/plain as above). I plan to put these policy changes in separate CLs, but I do plan on supporting feed content types with web intents; I think it'll be a big step forward (see the bug number earlier in the thread).
OK, I've changed this CL in a couple of ways. 1. Removed the internal dispatcher, which is checked in separately. 2. Removed all policy changes. This CL now simply focuses on making it possible to deal with finished downloads using an intent dispatch. Issues to discuss: The current API presumes a synchronous decision. We want to eventually amend this to allow an async delegate decision so that the web intents registry can be consulted. Even now, we would like to make the dispatch async so we can read a text-based download file and put it into the payload instead of using a blob. Should I make the delegate API richer, so that, for instance, it could have access to a webkit_glue::WebIntentData it can fill asynchronously? Should I keep the url in the "extra data"? In registerContentHandler, the handler gets the URL, which might be convenient for some types. (i.e. a feed url, in which the URL is important to allow an app to subscribe to it). This payload preparation should perhaps be delegated... Do I need to put in any logic about detecting a user-initiated download? In that case, presumably the user has decided to download the item, and isn't interested in seeing it get dispatched, opened, etc. That's in the DownloadItemImpl::DownloadStateInfo, but that isn't exposed in the DownloadItem API. Should it be, so the delegate can use information like this in a decision? On 2012/03/18 00:32:21, Greg Billock wrote: > (Note, I'm in China for a few days, so this is going to stagnate a bit...) > > http://codereview.chromium.org/9651020/diff/14001/chrome/browser/download/chr... > File chrome/browser/download/chrome_download_manager_delegate.cc (right): > > http://codereview.chromium.org/9651020/diff/14001/chrome/browser/download/chr... > chrome/browser/download/chrome_download_manager_delegate.cc:218: if (mime_type > == "application/rss+xml" || > On 2012/03/16 21:36:26, John Abd-El-Malek wrote: > > why is this stuff in chrome? > > This method is basically a policy delegation to the embedder. The content layer > says "hey, do you want me to dispatch this download I just did by issuing a web > intent?" I think it's possible that different embedders would have different > answers to that question. Specifically, maintaining a whitelist of this kind, or > deriving it from a list of installed apps, seems like something that should be > done chrome-side. > > http://codereview.chromium.org/9651020/diff/14001/content/browser/renderer_ho... > File content/browser/renderer_host/buffered_resource_handler.cc (right): > > http://codereview.chromium.org/9651020/diff/14001/content/browser/renderer_ho... > content/browser/renderer_host/buffered_resource_handler.cc:368: if (type == > "application/rss+xml" || > On 2012/03/16 21:36:26, John Abd-El-Malek wrote: > > why is this code here? it's hard to understand this. isn't/shouldn't there be > a > > centralized place for mime types that are handled by web intents instead of > > spreading this across different files? > > This is working around a hack in IsSupportedMimeType, which claims the browser > "supports" feed types (which it does by mashing them to text/plain as above). I > plan to put these policy changes in separate CLs, but I do plan on supporting > feed content types with web intents; I think it'll be a big step forward (see > the bug number earlier in the thread).
I apologize profusely for coming so late to the game; I saw this CL when it
first showed up, judged that it wasn't involved in the core download system, and
promptly started ignoring it. But now that I see that it is involved, I no
longer want to ignore it :-{.
Long-term, I'd like to get the DownloadItem out of the business of opening
files. There are enough aspects of opening that are chrome level (things like
extensions, consulting the download prefs for auto-open) that I feel like that
decision "should be" at the chrome level. That doesn't mean that we can't have
logic at the content level for opening, of course, just called from chrome. I'm
happy to do the full refactoring of open with the other downloads refactorings
I'm planning (and get John's ok at that point :-}), but I'd like not to put more
stuff into download_item_impl.cc in the meantime. Can we put the web intent
stuff into a separate file that gets called from DII? By my quick scan, it
doesn't look like a lot of download state is being used for "open as web
intent".
On 2012/03/26 18:48:36, Greg Billock wrote: > OK, I've changed this CL in a couple of ways. > > 1. Removed the internal dispatcher, which is checked in separately. > 2. Removed all policy changes. This CL now simply focuses on making it possible > to deal with finished downloads using an intent dispatch. > > Issues to discuss: > > The current API presumes a synchronous decision. We want to eventually amend > this to allow an async delegate decision so that the web intents registry can be > consulted. Even now, we would like to make the dispatch async so we can read a > text-based download file and put it into the payload instead of using a blob. > Should I make the delegate API richer, so that, for instance, it could have > access to a webkit_glue::WebIntentData it can fill asynchronously? > > Should I keep the url in the "extra data"? In registerContentHandler, the > handler gets the URL, which might be convenient for some types. (i.e. a feed > url, in which the URL is important to allow an app to subscribe to it). This > payload preparation should perhaps be delegated... > > Do I need to put in any logic about detecting a user-initiated download? In that > case, presumably the user has decided to download the item, and isn't interested > in seeing it get dispatched, opened, etc. That's in the > DownloadItemImpl::DownloadStateInfo, but that isn't exposed in the DownloadItem > API. Should it be, so the delegate can use information like this in a decision? Apologies for the lack of clue, but could you give me a better sense of the use case? From the issue, it sounds like you want to hand off particular mime types to particular web apps when they're "activated" in the browser. In what sense is a user-initiated download not an activation? What are the user actions which you'd want to consult to override the web intent mechanism? (Context: I want to get rid of DownloadStateInfo, not make it more visible, so I'm trying to understand your full requirements to see how I think it should fit in with the rest of the downloads system.) > > > On 2012/03/18 00:32:21, Greg Billock wrote: > > (Note, I'm in China for a few days, so this is going to stagnate a bit...) > > > > > http://codereview.chromium.org/9651020/diff/14001/chrome/browser/download/chr... > > File chrome/browser/download/chrome_download_manager_delegate.cc (right): > > > > > http://codereview.chromium.org/9651020/diff/14001/chrome/browser/download/chr... > > chrome/browser/download/chrome_download_manager_delegate.cc:218: if (mime_type > > == "application/rss+xml" || > > On 2012/03/16 21:36:26, John Abd-El-Malek wrote: > > > why is this stuff in chrome? > > > > This method is basically a policy delegation to the embedder. The content > layer > > says "hey, do you want me to dispatch this download I just did by issuing a > web > > intent?" I think it's possible that different embedders would have different > > answers to that question. Specifically, maintaining a whitelist of this kind, > or > > deriving it from a list of installed apps, seems like something that should be > > done chrome-side. > > > > > http://codereview.chromium.org/9651020/diff/14001/content/browser/renderer_ho... > > File content/browser/renderer_host/buffered_resource_handler.cc (right): > > > > > http://codereview.chromium.org/9651020/diff/14001/content/browser/renderer_ho... > > content/browser/renderer_host/buffered_resource_handler.cc:368: if (type == > > "application/rss+xml" || > > On 2012/03/16 21:36:26, John Abd-El-Malek wrote: > > > why is this code here? it's hard to understand this. isn't/shouldn't there > be > > a > > > centralized place for mime types that are handled by web intents instead of > > > spreading this across different files? > > > > This is working around a hack in IsSupportedMimeType, which claims the browser > > "supports" feed types (which it does by mashing them to text/plain as above). > I > > plan to put these policy changes in separate CLs, but I do plan on supporting > > feed content types with web intents; I think it'll be a big step forward (see > > the bug number earlier in the thread).
On 2012/03/26 21:22:11, rdsmith wrote:
> I apologize profusely for coming so late to the game; I saw this CL when it
> first showed up, judged that it wasn't involved in the core download system,
and
> promptly started ignoring it. But now that I see that it is involved, I no
> longer want to ignore it :-{.
>
> Long-term, I'd like to get the DownloadItem out of the business of opening
> files. There are enough aspects of opening that are chrome level (things like
> extensions, consulting the download prefs for auto-open) that I feel like that
> decision "should be" at the chrome level. That doesn't mean that we can't
have
> logic at the content level for opening, of course, just called from chrome.
I'm
> happy to do the full refactoring of open with the other downloads refactorings
> I'm planning (and get John's ok at that point :-}), but I'd like not to put
more
> stuff into download_item_impl.cc in the meantime. Can we put the web intent
> stuff into a separate file that gets called from DII? By my quick scan, it
> doesn't look like a lot of download state is being used for "open as web
> intent".
No, there's not a lot of use, and I'd be happy to put this in another file if
that'll make the refactoring easier. I want to merge this to the M19 branch,
though, so perhaps given that the overall complexity is easier to keep it
in-place? Let me know.
The big use of the download item state is asking the delegate about the policy
decision. The rest is content-side code currently, as per what is discussed
up-thread with John. If you've been talking about moving all of this stuff
chrome-side, though, perhaps that changes the equation of what to do with
this...
On the use case question... the use case is when someone clicks on the RSS link
in a blog or on a link to a word doc, we want to be able to dispatch that to a
web app handler. (i.e. Bloglines, Reader, QuickOffice, Office365).
The "explicit download gesture" I'm thinking of is if someone uses the context
menu to "save link as" or something similar, that's an action I'd interpret more
as "no, really, I just want to save this" rather than "open this" the way a
navigational click would be. Is that not what the explicit gesture on the
download state means, though? If not, perhaps we should write that down and pass
it through the chain.
(Getting rid of DownloadStateInfo is fine with me, really I just want to give
the delegate enough information about provenance to make an informed decision
about whether to dispatch the event with an intent. If we want to keep this "no
really the user said just download it" within content and not bother the
delegate at all, though, I think that'd be fair.)
On 2012/03/26 22:05:15, Greg Billock wrote:
> On 2012/03/26 21:22:11, rdsmith wrote:
> > I apologize profusely for coming so late to the game; I saw this CL when it
> > first showed up, judged that it wasn't involved in the core download system,
> and
> > promptly started ignoring it. But now that I see that it is involved, I no
> > longer want to ignore it :-{.
> >
> > Long-term, I'd like to get the DownloadItem out of the business of opening
> > files. There are enough aspects of opening that are chrome level (things
like
> > extensions, consulting the download prefs for auto-open) that I feel like
that
> > decision "should be" at the chrome level. That doesn't mean that we can't
> have
> > logic at the content level for opening, of course, just called from chrome.
> I'm
> > happy to do the full refactoring of open with the other downloads
refactorings
> > I'm planning (and get John's ok at that point :-}), but I'd like not to put
> more
> > stuff into download_item_impl.cc in the meantime. Can we put the web intent
> > stuff into a separate file that gets called from DII? By my quick scan, it
> > doesn't look like a lot of download state is being used for "open as web
> > intent".
>
> No, there's not a lot of use, and I'd be happy to put this in another file if
> that'll make the refactoring easier. I want to merge this to the M19 branch,
> though, so perhaps given that the overall complexity is easier to keep it
> in-place? Let me know.
I always end up wrestling with my conscience around branch points :-{. I
suspect John and I are going to need to talk about how the open stuff gets split
out between chrome and content when I get to that refactoring, which suggests we
not try to "lead" that change now, so let's try to put the code in in a way
that's as compatible as possible with what's here now.
> The big use of the download item state is asking the delegate about the policy
> decision. The rest is content-side code currently, as per what is discussed
> up-thread with John. If you've been talking about moving all of this stuff
> chrome-side, though, perhaps that changes the equation of what to do with
> this...
How about this? (John, obviously your opinion is strongly requested.) We're
currently delegating both the "ShouldOpen" decision and the Open functionality
to the chrome layer, it's just that in the one case we're using the
DownloadManagerDelegate and in the other the ContentBrowserClient. Why don't
just unify them on the DownloadManagerDelegate, add a pointer to the
DownloadItem being opened to the interface, and have
ChromeDownloadManagerDelegate::OpenItem() call into a content level utility
routine to do the web intent opening? That way we don't need extra tests on the
DownloadManagerDelegate, just an extra bit of functionality to do a download
open.
An alternative that would also be in line with the current code would be to have
ChromeDownloadManagerDelegate::ShouldOpenDownload test whatever needs to be
tested to make the policy decision, and take over responsibility for opening the
download (again calling into content level routines for implementation) if the
download should be opened by web intent? That requires no extra interfaces on
DownloadManagerDelegate.
> On the use case question... the use case is when someone clicks on the RSS
link
> in a blog or on a link to a word doc, we want to be able to dispatch that to a
> web app handler. (i.e. Bloglines, Reader, QuickOffice, Office365).
>
> The "explicit download gesture" I'm thinking of is if someone uses the context
> menu to "save link as" or something similar, that's an action I'd interpret
more
> as "no, really, I just want to save this" rather than "open this" the way a
> navigational click would be.
Ah, gotcha. That makes sense.
> Is that not what the explicit gesture on the
> download state means, though?
Nope; it means it's in response to a user click of some sort, rather than
something the web page did on its own (e.g. navigate to a download link in an
iframe, so a download starts without any intervention by the user).
> If not, perhaps we should write that down and pass it through the chain.
I wince a bit at this, just because we've already got quite a bit of random
(garbage) state on the DownloadItem. But if you need the information, you need
the information. Long-term I'd really like to have an enum that describes where
the download came from, which you could use (which might be the right way to go:
setup the enum at the chrome layer, make sure it's opaque at the content layer,
and just populate it with the download sources that you need. You can see
ChromeDownloadSource and DownloadSource for the current (chrome & content)
lists; I don't think you can uses those, because they overlap.
> (Getting rid of DownloadStateInfo is fine with me, really I just want to give
> the delegate enough information about provenance to make an informed decision
> about whether to dispatch the event with an intent. If we want to keep this
"no
> really the user said just download it" within content and not bother the
> delegate at all, though, I think that'd be fair.)
I'm not sure I understand this thought? The delegate needs to make the go/no-go
on web intent, so it needs to have the user intent information, doesn't it?
OK, this is updated as per our discussion. The logic of interest is in chrome_download_manager_delegate, where the policy is queried and the intent payload created. I added a new API to content which allows for browser-initiated intents to be dispatched. This will come in handy for both content-side initiation and embedder-side initiation. (We plan on more and more of these happening as time goes on.) A lot of this code is just setting up some new intent payload internals machinery. I would be happy to split that out if desired; the actual interesting LoC here is quite low. Aside: although this CL does kind of establish a whitelist policy, intersection with upstream policy means that it will never actually get executed. We need some more changes to let feed urls pass through with unscathed MIME types to trigger this policy.
http://codereview.chromium.org/9651020/diff/30001/content/public/browser/web_... File content/public/browser/web_intents_dispatcher.h (right): http://codereview.chromium.org/9651020/diff/30001/content/public/browser/web_... content/public/browser/web_intents_dispatcher.h:66: CONTENT_EXPORT void DispatchInternalIntent( perhaps this should be a static Create inside WebIntensDispatcher? i.e. static WebIntensDispatcher* Create(const webkit_glue::WebIntentData& data); then whoever calls this can call WebIntentDispatch themselves. i say this because this is really a creator function, and this is the pattern we use in the rest of the content api http://codereview.chromium.org/9651020/diff/30001/content/renderer/web_intent... File content/renderer/web_intents_host.cc (right): http://codereview.chromium.org/9651020/diff/30001/content/renderer/web_intent... content/renderer/web_intents_host.cc:134: void getExtra(const CppArgumentList& args, CppVariant* result) { nit: all these functions need to follow the chrome naming convention, i.e. GetExtra
http://codereview.chromium.org/9651020/diff/30001/content/public/browser/web_... File content/public/browser/web_intents_dispatcher.h (right): http://codereview.chromium.org/9651020/diff/30001/content/public/browser/web_... content/public/browser/web_intents_dispatcher.h:66: CONTENT_EXPORT void DispatchInternalIntent( Sounds good. I was trying to keep dispatching within content, but it really is a one-liner. On 2012/03/29 02:33:51, John Abd-El-Malek wrote: > perhaps this should be a static Create inside WebIntensDispatcher? i.e. > > static WebIntensDispatcher* Create(const webkit_glue::WebIntentData& data); > > then whoever calls this can call WebIntentDispatch themselves. > > i say this because this is really a creator function, and this is the pattern we > use in the rest of the content api http://codereview.chromium.org/9651020/diff/30001/content/renderer/web_intent... File content/renderer/web_intents_host.cc (right): http://codereview.chromium.org/9651020/diff/30001/content/renderer/web_intent... content/renderer/web_intents_host.cc:134: void getExtra(const CppArgumentList& args, CppVariant* result) { On 2012/03/29 02:33:51, John Abd-El-Malek wrote: > nit: all these functions need to follow the chrome naming convention, i.e. > GetExtra Done.
lgtm http://codereview.chromium.org/9651020/diff/32003/content/browser/intents/int... File content/browser/intents/internal_web_intents_dispatcher.cc (right): http://codereview.chromium.org/9651020/diff/32003/content/browser/intents/int... content/browser/intents/internal_web_intents_dispatcher.cc:65: content::WebIntentsDispatcher* WebIntentsDispatcher::Create( nit: by convention we put this in the to of the file. also, since this is defined in web_intents_dispatcher.h, by convention we put the static function in web_intents_dispatcher_impl.cc http://codereview.chromium.org/9651020/diff/32003/content/public/browser/web_... File content/public/browser/web_intents_dispatcher.h (right): http://codereview.chromium.org/9651020/diff/32003/content/public/browser/web_... content/public/browser/web_intents_dispatcher.h:65: static WebIntentsDispatcher* Create(const webkit_glue::WebIntentData& data); nit: by convention this is at the top of the class (after the typedef)
http://codereview.chromium.org/9651020/diff/32003/content/browser/intents/int... File content/browser/intents/internal_web_intents_dispatcher.cc (right): http://codereview.chromium.org/9651020/diff/32003/content/browser/intents/int... content/browser/intents/internal_web_intents_dispatcher.cc:65: content::WebIntentsDispatcher* WebIntentsDispatcher::Create( I can't put an impl file in the public/browser dir, since it can't refer to the browser/intents dep. Shall I make a new file alongside this one? Or just keep the impl in this file at the top? On 2012/03/29 17:59:19, John Abd-El-Malek wrote: > nit: by convention we put this in the to of the file. also, since this is > defined in web_intents_dispatcher.h, by convention we put the static function in > web_intents_dispatcher_impl.cc http://codereview.chromium.org/9651020/diff/32003/content/public/browser/web_... File content/public/browser/web_intents_dispatcher.h (right): http://codereview.chromium.org/9651020/diff/32003/content/public/browser/web_... content/public/browser/web_intents_dispatcher.h:65: static WebIntentsDispatcher* Create(const webkit_glue::WebIntentData& data); On 2012/03/29 17:59:19, John Abd-El-Malek wrote: > nit: by convention this is at the top of the class (after the typedef) Done.
This looks basically fine, and I'm willing for it to go in as is. But if we can find ways to address my two points below I'd be happier. (I.e. "LGTM, but." :-}) http://codereview.chromium.org/9651020/diff/32003/chrome/browser/download/chr... File chrome/browser/download/chrome_download_manager_delegate.cc (right): http://codereview.chromium.org/9651020/diff/32003/chrome/browser/download/chr... chrome/browser/download/chrome_download_manager_delegate.cc:285: item->GetWebContents(), dispatcher); Is there no way to get the web contents delegate more directly? In addition to the "reaching through content to get to a chrome" pattern that John raised, this creates another dependency on the DownloadItem knowing the tab from which it was initiated, which is an artificial concept at best (e.g. A download item loaded from history that's then resumed, a download that occurs in opening a new tab that's then closed because it's not a real navigation (not absolutely sure this last case occurs, but I think it does)). http://codereview.chromium.org/9651020/diff/32003/chrome/browser/download/chr... File chrome/browser/download/chrome_download_manager_delegate.h (right): http://codereview.chromium.org/9651020/diff/32003/chrome/browser/download/chr... chrome/browser/download/chrome_download_manager_delegate.h:154: void OpenWithWebIntent(const content::DownloadItem* item); Any reason why these can't be file-local functions rather than in the chrome_download_manager_delegate.h interface?
lgtm https://chromiumcodereview.appspot.com/9651020/diff/36003/content/browser/int... File content/browser/intents/intent_injector.cc (right): https://chromiumcodereview.appspot.com/9651020/diff/36003/content/browser/int... content/browser/intents/intent_injector.cc:73: policy->GrantReadFile(child_id, are read rights ever revoked, should they be at some point? nit: it's more common usage i think... ChildProcessSecurityPolicyImpl::GetInstance()->DoSomething( ....); https://chromiumcodereview.appspot.com/9651020/diff/36003/content/renderer/we... File content/renderer/web_intents_host.cc (right): https://chromiumcodereview.appspot.com/9651020/diff/36003/content/renderer/we... content/renderer/web_intents_host.cc:63: data_obj = v8::Local<v8::Value>::New(web_blob.toV8Value()); i have no clue if wrapping the return value of toV8Value() in a v8::Local does ;)
http://codereview.chromium.org/9651020/diff/32003/chrome/browser/download/chr... File chrome/browser/download/chrome_download_manager_delegate.cc (right): http://codereview.chromium.org/9651020/diff/32003/chrome/browser/download/chr... chrome/browser/download/chrome_download_manager_delegate.cc:285: item->GetWebContents(), dispatcher); Perhaps a better thing to do here is Browser* active = BrowserList::GetLastActiveWithProfile(profile_); active->WebIntentDispatch(null, dispatcher); But currently that won't work well with the browser: we really want that webcontents. I'll add a TODO here to get this working. On 2012/03/29 18:47:24, rdsmith wrote: > Is there no way to get the web contents delegate more directly? In addition to > the "reaching through content to get to a chrome" pattern that John raised, this > creates another dependency on the DownloadItem knowing the tab from which it was > initiated, which is an artificial concept at best (e.g. A download item loaded > from history that's then resumed, a download that occurs in opening a new tab > that's then closed because it's not a real navigation (not absolutely sure this > last case occurs, but I think it does)). http://codereview.chromium.org/9651020/diff/32003/chrome/browser/download/chr... File chrome/browser/download/chrome_download_manager_delegate.h (right): http://codereview.chromium.org/9651020/diff/32003/chrome/browser/download/chr... chrome/browser/download/chrome_download_manager_delegate.h:154: void OpenWithWebIntent(const content::DownloadItem* item); I like having the policy function be instance-based. If the opener needs the profile, it's plausible that it be instance-based, too (although it could take it as a parameter). On 2012/03/29 18:47:24, rdsmith wrote: > Any reason why these can't be file-local functions rather than in the > chrome_download_manager_delegate.h interface?
https://chromiumcodereview.appspot.com/9651020/diff/36003/content/browser/int... File content/browser/intents/intent_injector.cc (right): https://chromiumcodereview.appspot.com/9651020/diff/36003/content/browser/int... content/browser/intents/intent_injector.cc:73: policy->GrantReadFile(child_id, On 2012/03/29 20:51:57, michaeln wrote: > are read rights ever revoked, should they be at some point? I should do that when the injector is destroyed? I'd need to write down the child_id, but that's not a problem. > > nit: it's more common usage i think... > ChildProcessSecurityPolicyImpl::GetInstance()->DoSomething( > ....); Fixed. https://chromiumcodereview.appspot.com/9651020/diff/36003/content/renderer/we... File content/renderer/web_intents_host.cc (right): https://chromiumcodereview.appspot.com/9651020/diff/36003/content/renderer/we... content/renderer/web_intents_host.cc:63: data_obj = v8::Local<v8::Value>::New(web_blob.toV8Value()); On 2012/03/29 20:51:57, michaeln wrote: > i have no clue if wrapping the return value of toV8Value() in a v8::Local does > ;) Do I need to store this WebBlob in the object? That is, will stack allocating it cause horror? My mental image was that toV8Value wraps the internal value, but maybe that's not enough to make the lifespan right. For simple values, it works out, but for this one, it may not (the handle wouldn't be persisted to the toNPVariant binding onto the data_val_?
Similar usage in page_capture_custom_bindings.cc would indicate that your fine as is. I am by no means an expert on v8::Voodoo<>, but i think the code you have is sufficient to keep the blob alive for as long as the v8::Voodoo<Value::Handle> is alive. Those constructs should addref/release the underlying refcounted BlobData (leap of faith). If you have any doubts, should be easy to verify in a debugger. > Do I need to store this WebBlob in the object? That is, will stack allocating it > cause horror? My mental image was that toV8Value wraps the internal value, but > maybe that's not enough to make the lifespan right. > > For simple values, it works out, but for this one, it may not (the handle > wouldn't be persisted to the toNPVariant binding onto the data_val_?
Sounds good; thanks. LGTM. On 2012/03/29 20:53:00, Greg Billock wrote: > http://codereview.chromium.org/9651020/diff/32003/chrome/browser/download/chr... > File chrome/browser/download/chrome_download_manager_delegate.cc (right): > > http://codereview.chromium.org/9651020/diff/32003/chrome/browser/download/chr... > chrome/browser/download/chrome_download_manager_delegate.cc:285: > item->GetWebContents(), dispatcher); > Perhaps a better thing to do here is > > Browser* active = > BrowserList::GetLastActiveWithProfile(profile_); > active->WebIntentDispatch(null, dispatcher); > > But currently that won't work well with the browser: we really want that > webcontents. I'll add a TODO here to get this working. > > On 2012/03/29 18:47:24, rdsmith wrote: > > Is there no way to get the web contents delegate more directly? In addition > to > > the "reaching through content to get to a chrome" pattern that John raised, > this > > creates another dependency on the DownloadItem knowing the tab from which it > was > > initiated, which is an artificial concept at best (e.g. A download item loaded > > from history that's then resumed, a download that occurs in opening a new tab > > that's then closed because it's not a real navigation (not absolutely sure > this > > last case occurs, but I think it does)). > > http://codereview.chromium.org/9651020/diff/32003/chrome/browser/download/chr... > File chrome/browser/download/chrome_download_manager_delegate.h (right): > > http://codereview.chromium.org/9651020/diff/32003/chrome/browser/download/chr... > chrome/browser/download/chrome_download_manager_delegate.h:154: void > OpenWithWebIntent(const content::DownloadItem* item); > I like having the policy function be instance-based. If the opener needs the > profile, it's plausible that it be instance-based, too (although it could take > it as a parameter). > > On 2012/03/29 18:47:24, rdsmith wrote: > > Any reason why these can't be file-local functions rather than in the > > chrome_download_manager_delegate.h interface?
http://codereview.chromium.org/9651020/diff/32003/content/browser/intents/int... File content/browser/intents/internal_web_intents_dispatcher.cc (right): http://codereview.chromium.org/9651020/diff/32003/content/browser/intents/int... content/browser/intents/internal_web_intents_dispatcher.cc:65: content::WebIntentsDispatcher* WebIntentsDispatcher::Create( On 2012/03/29 18:45:08, Greg Billock wrote: > I can't put an impl file in the public/browser dir, since it can't refer to the > browser/intents dep. Shall I make a new file alongside this one? Or just keep > the impl in this file at the top? I meant in the impl file in content\browser, i.e. content\browser\intents\web_intents_dispatcher_impl.cc > > On 2012/03/29 17:59:19, John Abd-El-Malek wrote: > > nit: by convention we put this in the to of the file. also, since this is > > defined in web_intents_dispatcher.h, by convention we put the static function > in > > web_intents_dispatcher_impl.cc >
On 2012/03/29 20:53:00, Greg Billock wrote: > http://codereview.chromium.org/9651020/diff/32003/chrome/browser/download/chr... > File chrome/browser/download/chrome_download_manager_delegate.cc (right): > > http://codereview.chromium.org/9651020/diff/32003/chrome/browser/download/chr... > chrome/browser/download/chrome_download_manager_delegate.cc:285: > item->GetWebContents(), dispatcher); > Perhaps a better thing to do here is > > Browser* active = > BrowserList::GetLastActiveWithProfile(profile_); > active->WebIntentDispatch(null, dispatcher); You want to avoid this pattern, since it causes grief for Android (they don't use the Browser* object).
On 2012/03/30 16:12:11, John Abd-El-Malek wrote: > On 2012/03/29 20:53:00, Greg Billock wrote: > > > http://codereview.chromium.org/9651020/diff/32003/chrome/browser/download/chr... > > File chrome/browser/download/chrome_download_manager_delegate.cc (right): > > > > > http://codereview.chromium.org/9651020/diff/32003/chrome/browser/download/chr... > > chrome/browser/download/chrome_download_manager_delegate.cc:285: > > item->GetWebContents(), dispatcher); > > Perhaps a better thing to do here is > > > > Browser* active = > > BrowserList::GetLastActiveWithProfile(profile_); > > active->WebIntentDispatch(null, dispatcher); > > You want to avoid this pattern, since it causes grief for Android (they don't > use the Browser* object). Thanks. I didn't know that. I fixed up string handling for getExtra now (tested with a separate policy CL). No substantive change. I'm planning on checking this in. Speak up if there's anything else you think belongs in the change.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/9651020/50001
Presubmit check for 9651020-50001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
webkit
Presubmit checks took 1.9s to calculate.
http://codereview.chromium.org/9651020/diff/32003/content/browser/intents/int... File content/browser/intents/internal_web_intents_dispatcher.cc (right): http://codereview.chromium.org/9651020/diff/32003/content/browser/intents/int... content/browser/intents/internal_web_intents_dispatcher.cc:65: content::WebIntentsDispatcher* WebIntentsDispatcher::Create( ping?
http://codereview.chromium.org/9651020/diff/32003/content/browser/intents/int... File content/browser/intents/internal_web_intents_dispatcher.cc (right): http://codereview.chromium.org/9651020/diff/32003/content/browser/intents/int... content/browser/intents/internal_web_intents_dispatcher.cc:65: content::WebIntentsDispatcher* WebIntentsDispatcher::Create( On 2012/04/02 15:05:08, John Abd-El-Malek wrote: > ping? Done.
http://codereview.chromium.org/9651020/diff/48005/webkit/glue/web_intent_data.h File webkit/glue/web_intent_data.h (right): http://codereview.chromium.org/9651020/diff/48005/webkit/glue/web_intent_data... webkit/glue/web_intent_data.h:26: // SerializedScriptObject. nit: SerializedScriptObject -> WebSerializedScriptValue http://codereview.chromium.org/9651020/diff/48005/webkit/glue/web_intent_data... webkit/glue/web_intent_data.h:28: // Any extra key-value pair metadata. (Not serialized.) what does "Not serialized" mean? http://codereview.chromium.org/9651020/diff/48005/webkit/glue/web_intent_data... webkit/glue/web_intent_data.h:35: std::string blob_file; Is this the blob URL? Should you be using GURL instead? Or, is this really a filename? In which case, it should be using FilePath instead, right? http://codereview.chromium.org/9651020/diff/48005/webkit/glue/web_intent_data... webkit/glue/web_intent_data.h:37: int64 blob_length; this is a byte count? http://codereview.chromium.org/9651020/diff/47017/content/browser/intents/web... File content/browser/intents/web_intents_dispatcher_impl.cc (right): http://codereview.chromium.org/9651020/diff/47017/content/browser/intents/web... content/browser/intents/web_intents_dispatcher_impl.cc:18: content::WebIntentsDispatcher* WebIntentsDispatcher::Create( nit: no need for "content::" when inside the content namespace
http://codereview.chromium.org/9651020/diff/48005/webkit/glue/web_intent_data.h File webkit/glue/web_intent_data.h (right): http://codereview.chromium.org/9651020/diff/48005/webkit/glue/web_intent_data... webkit/glue/web_intent_data.h:26: // SerializedScriptObject. On 2012/04/02 16:35:43, darin wrote: > nit: SerializedScriptObject -> WebSerializedScriptValue Done. http://codereview.chromium.org/9651020/diff/48005/webkit/glue/web_intent_data... webkit/glue/web_intent_data.h:28: // Any extra key-value pair metadata. (Not serialized.) On 2012/04/02 16:35:43, darin wrote: > what does "Not serialized" mean? Meaning the raw string values. I added a note that |data| is serialized. http://codereview.chromium.org/9651020/diff/48005/webkit/glue/web_intent_data... webkit/glue/web_intent_data.h:35: std::string blob_file; On 2012/04/02 16:35:43, darin wrote: > Is this the blob URL? Should you be using GURL instead? Or, is this really a > filename? In which case, it should be using FilePath instead, right? Right. Fixing... (for some reason I assumed FilePath wasn't IPC-congruous, although of course it is) http://codereview.chromium.org/9651020/diff/48005/webkit/glue/web_intent_data... webkit/glue/web_intent_data.h:37: int64 blob_length; On 2012/04/02 16:35:43, darin wrote: > this is a byte count? Yes. Basically this and the above are WebBlob::createFromFile args. I'll note that explicitly. http://codereview.chromium.org/9651020/diff/47017/content/browser/intents/web... File content/browser/intents/web_intents_dispatcher_impl.cc (right): http://codereview.chromium.org/9651020/diff/47017/content/browser/intents/web... content/browser/intents/web_intents_dispatcher_impl.cc:18: content::WebIntentsDispatcher* WebIntentsDispatcher::Create( On 2012/04/02 16:35:43, darin wrote: > nit: no need for "content::" when inside the content namespace Done.
http://codereview.chromium.org/9651020/diff/47018/content/browser/intents/int... File content/browser/intents/internal_web_intents_dispatcher.cc (right): http://codereview.chromium.org/9651020/diff/47018/content/browser/intents/int... content/browser/intents/internal_web_intents_dispatcher.cc:9: #include "content/public/browser/web_contents_delegate.h" nit: i think these aren't needed anymore
http://codereview.chromium.org/9651020/diff/47018/content/browser/intents/int... File content/browser/intents/internal_web_intents_dispatcher.cc (right): http://codereview.chromium.org/9651020/diff/47018/content/browser/intents/int... content/browser/intents/internal_web_intents_dispatcher.cc:9: #include "content/public/browser/web_contents_delegate.h" On 2012/04/03 01:59:18, John Abd-El-Malek wrote: > nit: i think these aren't needed anymore Oops! Thanks.
btw if it's not clear, still lgtm
On 2012/04/04 00:43:55, John Abd-El-Malek wrote: > btw if it's not clear, still lgtm Darin, can you look at this for webkit/*
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/9651020/50008
Change committed as 131535 |
