Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
694303002:
Allow local file to run though content handler. (Closed)
|
Created:
6 years, 1 month ago by qsr Modified:
6 years, 1 month ago CC:
ben+mojo_chromium.org, darin (slow to review), mojo-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://github.com/domokit/mojo.git@master Project:
mojo Visibility:
Public. |
DescriptionAllow local file to run though content handler.
This CL allows local file to use content handler. For this it aggregates the 2 code paths when reading a local file or retrieving a file
from the network.
R=aa@chromium.org
Committed: https://chromium.googlesource.com/external/mojo/+/da86e22cf5ae8a97c458db996e5669d653e0e5ea
Patch Set 1 #Patch Set 2 : Remove mime mapping #
Total comments: 17
Patch Set 3 : Follow review + rebase #Patch Set 4 : Fix spurious warnings #Patch Set 5 : Small refactor #
Total comments: 4
Patch Set 6 : Follow review #Patch Set 7 : Delay running the callback in AsPath as the code doesn't support re-entrency. #
Total comments: 7
Patch Set 8 : Follow review #Patch Set 9 : Fix constants. #
Total comments: 6
Patch Set 10 : Follow review #
Total comments: 4
Patch Set 11 : Follow review #
Messages
Total messages: 33 (2 generated)
abarth@chromium.org changed reviewers: + abarth@chromium.org
Should we wait until we're done implementing #! ? It's not clear to me how a static mapping from file extension to mime type scales.
On 2014/11/03 15:08:25, abarth wrote: > Should we wait until we're done implementing #! ? It's not clear to me how a > static mapping from file extension to mime type scales. #! only works for new mojo specific type though. If I want to get a png file, a pdf file, or an html file there will be not #! I think we all agree with want #! for any new type, but if we want to be able to support the old ones, and load those from disk, we need some static mapping.
On 2014/11/03 at 15:39:18, qsr wrote: > I think we all agree with want #! for any new type, but if we want to be able to support the old ones, and load those from disk, we need some static mapping. I'm not sure we want to support all those file types from disk. Why not just host them on a web server?
On 2014/11/03 15:40:56, abarth wrote: > On 2014/11/03 at 15:39:18, qsr wrote: > > I think we all agree with want #! for any new type, but if we want to be able > to support the old ones, and load those from disk, we need some static mapping. > > I'm not sure we want to support all those file types from disk. Why not just > host them on a web server? Are we then saying that file:// url won't be correctly supported in mojo? I mean, the main part of this change is to allow to load application that needs content handler from disk. I added the mime type because that's the only way to have content that is not .so right now. So I could remove that part, but I do not see any reason will want to restrict file:// url to be from specific mojo app and not any usual content.
Adam, can you explain your position a little more? I agree that new content written for Mojo should use the #! syntax. But, what about existing content like pdf, html, and images. Surely we want to support such content. Are you saying we just don't want to support it from local files? (I think I disagree). Are you saying that this patch is not the correct way to implement (I think I agree).
On 2014/11/03 at 16:10:38, qsr wrote: > Are we then saying that file:// url won't be correctly supported in mojo? Yes, I don't think we should support file URLs. We need some way of loading the network_service off disk, but I don't see why we need a way to load PNGs off disk. > I mean, the main part of this change is to allow to load application that needs content handler from disk. I don't think we should add support for that use case at this time. On 2014/11/03 at 17:01:06, aa wrote: > Adam, can you explain your position a little more? Once we add support for sniffing content types from file extensions, we won't be able to remove it. I don't see a big need to add support for that feature at this time, so I'd rather we waited until we're forced into it. > I agree that new content written for Mojo should use the #! syntax. But, what about existing content like pdf, html, and images. That content should be served from web servers. > Surely we want to support such content. Are you saying we just don't want to support it from local files? Correct. I don't think we should support the file URL scheme. > (I think I disagree). Are you saying that this patch is not the correct way to implement (I think I agree). I'm saying that once we add a mapping from file extensions to mime types, we won't be able to remove it. Such a mapping is not desirable, so we shouldn't add it unless absolutely necessary. We can just use a web server instead.
> > I mean, the main part of this change is to allow to load application that > needs content handler from disk. > > I don't think we should add support for that use case at this time. I need this to be able to write a network service that is a java + .so application so that we can remove the network service from the android shell and so remove the network service from the mojo repo.
Removed the mime mapping from this patch.
On 2014/11/03 at 17:22:00, qsr wrote: > Removed the mime mapping from this patch. Thanks!
I think we need to add a bunch of content type sniffing (e.g., in the manner of https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_snif...) to mojo_shell just to support the web. Once we do that it seems we will get some local files for free. I would like to not go out of our way to break that local file support for a number of reasons: - It makes development of Mojo easier. Running a web server to do local development is a pain. - It makes embedding Mojo easier, safer, cheaper. Embedder should not need to run an entire web server to support local files. - Philosophically, I feel that mojo_shell should be able to support local files as first-class citizens. As to whether we add extension->mimetype mapping, I don't know enough to say. I would be happy to start with just the sniffable content types and see how far that takes us. I could also imagine flags to mojo-shell to do mappings of specific files to specific content types.
On 2014/11/03 17:33:59, Aaron Boodman wrote: > I think we need to add a bunch of content type sniffing (e.g., in the manner of > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_snif...) > to mojo_shell just to support the web. > > Once we do that it seems we will get some local files for free. > > I would like to not go out of our way to break that local file support for a > number of reasons: > > - It makes development of Mojo easier. Running a web server to do local > development is a pain. > - It makes embedding Mojo easier, safer, cheaper. Embedder should not need to > run an entire web server to support local files. > - Philosophically, I feel that mojo_shell should be able to support local files > as first-class citizens. > > As to whether we add extension->mimetype mapping, I don't know enough to say. I > would be happy to start with just the sniffable content types and see how far > that takes us. I could also imagine flags to mojo-shell to do mappings of > specific files to specific content types. I'm working on local-file pain reduction - http://code.google.com/p/chromium/issues/detail?id=426899 Step 1 is enabling data pipe peeking (https://codereview.chromium.org/696543003/), and step 2 is adding support for a #!mojo:content-handler-url "shebang". The support for peeking should also enable "sniffing" a response's data type.
https://codereview.chromium.org/694303002/diff/20001/mojo/common/data_pipe_ut... File mojo/common/data_pipe_utils.cc (right): https://codereview.chromium.org/694303002/diff/20001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:67: void* buffer; = nullptr; I know the code above does not do it. That code is wrong :). https://codereview.chromium.org/694303002/diff/20001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:68: uint32_t buffer_num_bytes; = 0; https://codereview.chromium.org/694303002/diff/20001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:92: // If the consumder handle was closed, then treat as EOF. typo: consumer https://codereview.chromium.org/694303002/diff/20001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:136: void CopyFromFile(const base::FilePath& source, Can you add a unit test for this? https://codereview.chromium.org/694303002/diff/20001/mojo/common/data_pipe_ut... File mojo/common/data_pipe_utils.h (right): https://codereview.chromium.org/694303002/diff/20001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.h:35: const base::Callback<void(bool /*success*/)>& callback); This /* syntax */ is something I don't feel that I have seen in Chromium sources before, and don't really like. Oh well, you didn't introduce it. https://codereview.chromium.org/694303002/diff/20001/mojo/shell/dynamic_appli... File mojo/shell/dynamic_application_loader.cc (right): https://codereview.chromium.org/694303002/diff/20001/mojo/shell/dynamic_appli... mojo/shell/dynamic_application_loader.cc:35: class LoaderResponse { Is there some better way to factor this? It seems weird that we have the Loader/LocalLoader/RemoteLoader split, but then this shared LoaderResponse that contains smarts about how to handle both local and remote results. Can the impl of converting to URL response go in the two loader implementations somehow?
There is a few changed due to the #! feature. The food news is that if a prepend a png file with #!mojo://png_viewer\n I can read a local png file. https://codereview.chromium.org/694303002/diff/20001/mojo/common/data_pipe_ut... File mojo/common/data_pipe_utils.cc (right): https://codereview.chromium.org/694303002/diff/20001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:67: void* buffer; On 2014/11/03 19:20:15, Aaron Boodman wrote: > = nullptr; I know the code above does not do it. That code is wrong :). Done. But I do not understand why you say this code is wrong. The data is initialized (through BeginWriteDataRaw) before being used. So, yes the buffer is not initialized until then, but why would the code be wrong? https://codereview.chromium.org/694303002/diff/20001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:68: uint32_t buffer_num_bytes; On 2014/11/03 19:20:14, Aaron Boodman wrote: > = 0; Done. https://codereview.chromium.org/694303002/diff/20001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:92: // If the consumder handle was closed, then treat as EOF. On 2014/11/03 19:20:14, Aaron Boodman wrote: > typo: consumer Done. https://codereview.chromium.org/694303002/diff/20001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:136: void CopyFromFile(const base::FilePath& source, On 2014/11/03 19:20:15, Aaron Boodman wrote: > Can you add a unit test for this? Done. https://codereview.chromium.org/694303002/diff/20001/mojo/common/data_pipe_ut... File mojo/common/data_pipe_utils.h (right): https://codereview.chromium.org/694303002/diff/20001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.h:35: const base::Callback<void(bool /*success*/)>& callback); On 2014/11/03 19:20:15, Aaron Boodman wrote: > This /* syntax */ is something I don't feel that I have seen in Chromium sources > before, and don't really like. > > Oh well, you didn't introduce it. Acknowledged. https://codereview.chromium.org/694303002/diff/20001/mojo/shell/dynamic_appli... File mojo/shell/dynamic_application_loader.cc (right): https://codereview.chromium.org/694303002/diff/20001/mojo/shell/dynamic_appli... mojo/shell/dynamic_application_loader.cc:35: class LoaderResponse { On 2014/11/03 19:20:15, Aaron Boodman wrote: > Is there some better way to factor this? > > It seems weird that we have the Loader/LocalLoader/RemoteLoader split, but then > this shared LoaderResponse that contains smarts about how to handle both local > and remote results. > > Can the impl of converting to URL response go in the two loader implementations > somehow? Not easily. But I can definitively make this class virtual and have 2 implementation for the 2 cases. It wll prevent all of those tests.
hansmuller@chromium.org changed reviewers: + hansmuller@chromium.org
https://codereview.chromium.org/694303002/diff/80001/mojo/shell/dynamic_appli... File mojo/shell/dynamic_application_loader.cc (right): https://codereview.chromium.org/694303002/diff/80001/mojo/shell/dynamic_appli... mojo/shell/dynamic_application_loader.cc:54: shebang.substr(2, std::string::npos), base::TRIM_ALL, &url_string); Is this necessary? The URL parser is required to trim whitespace (http://dev.w3.org/html5/spec-LC/urls.html#parsing-urls). https://codereview.chromium.org/694303002/diff/80001/mojo/shell/dynamic_appli... mojo/shell/dynamic_application_loader.cc:266: // TODO(aa): Santify check that the thing we got looks vaguely like a mojo Santify => Sanity
https://codereview.chromium.org/694303002/diff/80001/mojo/shell/dynamic_appli... File mojo/shell/dynamic_application_loader.cc (right): https://codereview.chromium.org/694303002/diff/80001/mojo/shell/dynamic_appli... mojo/shell/dynamic_application_loader.cc:54: shebang.substr(2, std::string::npos), base::TRIM_ALL, &url_string); On 2014/11/04 16:36:57, hansmuller wrote: > Is this necessary? The URL parser is required to trim whitespace > (http://dev.w3.org/html5/spec-LC/urls.html#parsing-urls). Hum. Did that to try to correct another issue I had. Did not remove it. Thanks. Removed now. https://codereview.chromium.org/694303002/diff/80001/mojo/shell/dynamic_appli... mojo/shell/dynamic_application_loader.cc:266: // TODO(aa): Santify check that the thing we got looks vaguely like a mojo On 2014/11/04 16:36:57, hansmuller wrote: > Santify => Sanity Done.
https://codereview.chromium.org/694303002/diff/20001/mojo/common/data_pipe_ut... File mojo/common/data_pipe_utils.cc (right): https://codereview.chromium.org/694303002/diff/20001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:67: void* buffer; On 2014/11/04 11:58:34, qsr wrote: > On 2014/11/03 19:20:15, Aaron Boodman wrote: > > = nullptr; I know the code above does not do it. That code is wrong :). > > Done. But I do not understand why you say this code is wrong. The data is > initialized (through BeginWriteDataRaw) before being used. So, yes the buffer is > not initialized until then, but why would the code be wrong? Sorry, I was exaggerating for comedic effect. As if to say that failing to initializing a local if morally, or ethically wrong. In truth, it is wrong, at least in the stylistic sense: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Local_Variables https://codereview.chromium.org/694303002/diff/20001/mojo/shell/dynamic_appli... File mojo/shell/dynamic_application_loader.cc (right): https://codereview.chromium.org/694303002/diff/20001/mojo/shell/dynamic_appli... mojo/shell/dynamic_application_loader.cc:35: class LoaderResponse { On 2014/11/04 11:58:34, qsr wrote: > On 2014/11/03 19:20:15, Aaron Boodman wrote: > > Is there some better way to factor this? > > > > It seems weird that we have the Loader/LocalLoader/RemoteLoader split, but > then > > this shared LoaderResponse that contains smarts about how to handle both local > > and remote results. > > > > Can the impl of converting to URL response go in the two loader > implementations > > somehow? > > Not easily. But I can definitively make this class virtual and have 2 > implementation for the 2 cases. It wll prevent all of those tests. Really? It seems like all the methods of this class could become abstract on Loader, then implemented in each of the two loader impls. What am I missing? https://codereview.chromium.org/694303002/diff/20001/mojo/shell/dynamic_appli... mojo/shell/dynamic_application_loader.cc:63: base::Bind(&IgnoreResult)); I think you can also do this? base::Callback<void(bool)> foo; common::CopyFromFile(..., foo);
https://codereview.chromium.org/694303002/diff/20001/mojo/shell/dynamic_appli... File mojo/shell/dynamic_application_loader.cc (right): https://codereview.chromium.org/694303002/diff/20001/mojo/shell/dynamic_appli... mojo/shell/dynamic_application_loader.cc:35: class LoaderResponse { On 2014/11/04 18:05:01, Aaron Boodman wrote: > On 2014/11/04 11:58:34, qsr wrote: > > On 2014/11/03 19:20:15, Aaron Boodman wrote: > > > Is there some better way to factor this? > > > > > > It seems weird that we have the Loader/LocalLoader/RemoteLoader split, but > > then > > > this shared LoaderResponse that contains smarts about how to handle both > local > > > and remote results. > > > > > > Can the impl of converting to URL response go in the two loader > > implementations > > > somehow? > > > > Not easily. But I can definitively make this class virtual and have 2 > > implementation for the 2 cases. It wll prevent all of those tests. > > Really? It seems like all the methods of this class could become abstract on > Loader, then implemented in each of the two loader impls. What am I missing? We could do this, but it would mean adding the field of this class to each of the loader. Those fields would only be valid in a certain phase of the loader, and the associated method would only be called in this phase. This makes harder to use the API, and I find it quite confusing. Using an external object for the response that is created when needed and use as a parameter of the OnResponse method seems way more natural for me. https://codereview.chromium.org/694303002/diff/20001/mojo/shell/dynamic_appli... mojo/shell/dynamic_application_loader.cc:63: base::Bind(&IgnoreResult)); On 2014/11/04 18:05:00, Aaron Boodman wrote: > I think you can also do this? > > base::Callback<void(bool)> foo; > common::CopyFromFile(..., foo); Not really. See the comment in base/task_runner_util.h:30 and http://crbug.com/162712 The bug is quite old, but given that the comment is still there, I prefer to consider that the API should not allow for null callbacks.
Sorry to push back on this, but having the two different abstractions seems really over complicated to me. How about having a single Loader class that contains basically what's in the the Loader and Response base classes now. But then it takes as a parameter a Request interface, of which we have Local and Network implementations. These would expose the AsPath(), AsURLResponse(), MimeType(), Peek*(), and so-on. https://codereview.chromium.org/694303002/diff/120001/mojo/shell/dynamic_appl... File mojo/shell/dynamic_application_loader.cc (right): https://codereview.chromium.org/694303002/diff/120001/mojo/shell/dynamic_appl... mojo/shell/dynamic_application_loader.cc:83: if (skip) { I believe this will be a warning on some compilers: http://msdn.microsoft.com/en-us/library/b6801kcy.aspx Typically Chromium code will explicitly compare to zero. https://codereview.chromium.org/694303002/diff/120001/mojo/shell/dynamic_appl... mojo/shell/dynamic_application_loader.cc:84: DCHECK(ReadDataRaw(response_->body.get(), This function call is going to get compiled out in optimized builds. https://codereview.chromium.org/694303002/diff/120001/mojo/shell/dynamic_appl... mojo/shell/dynamic_application_loader.cc:169: GURL url_; The order is: static methods, static fields, non-static methods, non-static fields.
https://codereview.chromium.org/694303002/diff/120001/mojo/shell/dynamic_appl... File mojo/shell/dynamic_application_loader.cc (right): https://codereview.chromium.org/694303002/diff/120001/mojo/shell/dynamic_appl... mojo/shell/dynamic_application_loader.cc:169: GURL url_; On 2014/11/06 02:46:03, Aaron Boodman wrote: > The order is: > > static methods, static fields, non-static methods, non-static fields. Oops, got this slightly wrong. See here: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O.... Anyway, these fields should be below these methods.
https://codereview.chromium.org/694303002/diff/120001/mojo/shell/dynamic_appl... File mojo/shell/dynamic_application_loader.cc (right): https://codereview.chromium.org/694303002/diff/120001/mojo/shell/dynamic_appl... mojo/shell/dynamic_application_loader.cc:83: if (skip) { On 2014/11/06 02:46:03, Aaron Boodman wrote: > I believe this will be a warning on some compilers: > > http://msdn.microsoft.com/en-us/library/b6801kcy.aspx > > Typically Chromium code will explicitly compare to zero. Done. https://codereview.chromium.org/694303002/diff/120001/mojo/shell/dynamic_appl... mojo/shell/dynamic_application_loader.cc:84: DCHECK(ReadDataRaw(response_->body.get(), On 2014/11/06 02:46:03, Aaron Boodman wrote: > This function call is going to get compiled out in optimized builds. Done. https://codereview.chromium.org/694303002/diff/120001/mojo/shell/dynamic_appl... mojo/shell/dynamic_application_loader.cc:169: GURL url_; On 2014/11/06 02:46:03, Aaron Boodman wrote: > The order is: > > static methods, static fields, non-static methods, non-static fields. Done.
gentle ping?
https://codereview.chromium.org/694303002/diff/160001/mojo/shell/dynamic_appl... File mojo/shell/dynamic_application_loader.cc (right): https://codereview.chromium.org/694303002/diff/160001/mojo/shell/dynamic_appl... mojo/shell/dynamic_application_loader.cc:52: bool PeekContentHandler(std::string* mojo_shebang, I would prefer this function in loader, and make LoaderRequest a pure interface. https://codereview.chromium.org/694303002/diff/160001/mojo/shell/dynamic_appl... mojo/shell/dynamic_application_loader.cc:339: void Load(const GURL& url, NetworkServicePtr* network_service) { second param never used. https://codereview.chromium.org/694303002/diff/160001/mojo/shell/dynamic_appl... mojo/shell/dynamic_application_loader.cc:355: if (!network_service_->get()) { Put this back in DAL::Load() so that we share one instance across all loaders. https://codereview.chromium.org/694303002/diff/160001/mojo/shell/dynamic_appl... mojo/shell/dynamic_application_loader.cc:357: GURL("mojo:network_service"), network_service_); network_service_ is a pointer, so I guess this pipe will never be closed. https://codereview.chromium.org/694303002/diff/160001/mojo/shell/dynamic_appl... mojo/shell/dynamic_application_loader.cc:404: runner_factory_.get(), &network_service_, this is always null.
Looking at this, I still think that basic inheritance is even simpler here:
Loader
protected
Load()
HasMojoMagic() = 0
PeekFirstLine() = 0
MimeType() = 0
AsURLResponse() = 0
AsPath() = 0
private
PeekMagic()
RunLibrary()
RunAsContentHandler()
LocalLoader
public
LocalLoader(url):
PostTask(&Load)
private
// impls of all the abstract methods
NetworkLoader(url)
public
Fetch(&OnFetch)
private
OnFetch:
Load()
// impls of all the abstract methods
... but I'm OK with the shape you have now too.
On 2014/11/07 08:57:56, Aaron Boodman wrote: > Looking at this, I still think that basic inheritance is even simpler here: Ok, did that.
https://codereview.chromium.org/694303002/diff/160001/mojo/shell/dynamic_appl... File mojo/shell/dynamic_application_loader.cc (right): https://codereview.chromium.org/694303002/diff/160001/mojo/shell/dynamic_appl... mojo/shell/dynamic_application_loader.cc:357: GURL("mojo:network_service"), network_service_); On 2014/11/07 08:56:27, Aaron Boodman wrote: > network_service_ is a pointer, so I guess this pipe will never be closed. I mixed 2 things here, sorry. network_service_ should not exist, and we should use the network_service provided by the DynamicApplicationLoader::Load
much better. https://codereview.chromium.org/694303002/diff/180001/mojo/shell/dynamic_appl... File mojo/shell/dynamic_application_loader.cc (right): https://codereview.chromium.org/694303002/diff/180001/mojo/shell/dynamic_appl... mojo/shell/dynamic_application_loader.cc:81: void OnResponse(bool success) { Rename this Load(). Rename LoaderComplete() to ReportComplete(). Remove the param from this method and have callers just choose to either call Load() or ReportComplete() as necessary. https://codereview.chromium.org/694303002/diff/180001/mojo/shell/dynamic_appl... mojo/shell/dynamic_application_loader.cc:88: { Not your deal, but remove these braces. No need.
https://codereview.chromium.org/694303002/diff/180001/mojo/shell/dynamic_appl... File mojo/shell/dynamic_application_loader.cc (right): https://codereview.chromium.org/694303002/diff/180001/mojo/shell/dynamic_appl... mojo/shell/dynamic_application_loader.cc:81: void OnResponse(bool success) { On 2014/11/07 09:44:36, Aaron Boodman wrote: > Rename this Load(). > > Rename LoaderComplete() to ReportComplete(). > > Remove the param from this method and have callers just choose to either call > Load() or ReportComplete() as necessary. Done. https://codereview.chromium.org/694303002/diff/180001/mojo/shell/dynamic_appl... mojo/shell/dynamic_application_loader.cc:88: { On 2014/11/07 09:44:36, Aaron Boodman wrote: > Not your deal, but remove these braces. No need. Done.
lgtm
Message was sent while issue was closed.
Committed patchset #11 (id:200001) manually as da86e22cf5ae8a97c458db996e5669d653e0e5ea (presubmit successful).
Message was sent while issue was closed.
Thanks |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
