First cut at "content handling" support in Mojo.

First cut at "content handling" support in Mojo.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288013

Reverted (see http://crrev.com/288043) because this closed the tree on some builders.  They had compile errors and could not find service_provider.mojom-internal.h.  See the following builds for examples:

http://build.chromium.org/p/chromium.chrome/buildstatus?builder=Google%20Chrome%20Linux&number=36866
http://build.chromium.org/p/chromium.chrome/buildstatus?builder=Google%20Chrome%20ChromeOS&number=70451

[Aaron Boodman, 2014-08-03 02:34:36]

A few of the tests still don't pass, but I think this is ready for a first look.
I need to ask Dave about the tests on monday.

[darin (slow to review), 2014-08-03 06:12:02]

https://codereview.chromium.org/423963004/diff/20001/mojo/public/interfaces/s...
File mojo/public/interfaces/service_provider/content_handler.mojom (right):

https://codereview.chromium.org/423963004/diff/20001/mojo/public/interfaces/s...
mojo/public/interfaces/service_provider/content_handler.mojom:11:
handle<data_pipe_consumer> content,
Are you thinking we can get away with not supplying the URLResponse here? Some
response headers (e.g., Content-Type) will matter to content handlers. An app
might be registered as a content handler for N mime types. It might also find
the charset hint from the response headers useful. But, perhaps it is possible
to provide something less than a full URLResponse data structure here.

Also, is it useful / good (from a security perspective) to provide the chain of
redirects to the content handler?

[Aaron Boodman, 2014-08-03 11:11:40]

https://codereview.chromium.org/423963004/diff/20001/mojo/public/interfaces/s...
File mojo/public/interfaces/service_provider/content_handler.mojom (right):

https://codereview.chromium.org/423963004/diff/20001/mojo/public/interfaces/s...
mojo/public/interfaces/service_provider/content_handler.mojom:11:
handle<data_pipe_consumer> content,
On 2014/08/03 06:12:02, darin wrote:
> Are you thinking we can get away with not supplying the URLResponse here? Some
> response headers (e.g., Content-Type) will matter to content handlers. An app
> might be registered as a content handler for N mime types. It might also find
> the charset hint from the response headers useful. But, perhaps it is possible
> to provide something less than a full URLResponse data structure here.

No, we will need the full URLResponse or something close to it. I encountered
some dependency issues using it, changed it to this data pipe temporarily, then
forgot to go back and figure that out. I will look at it again, maybe in a
separate change.

> Also, is it useful / good (from a security perspective) to provide the chain
of
> redirects to the content handler?

I'm not sure. Currently I don't see anything in URLResponse that exposes the
redirect chain - am I missing it? Does Blink need the redirect chain for any
reason?

[darin (slow to review), 2014-08-03 15:13:40]

We could move ContentHandler to mojo/services/public/... allowing it to
depend on URLResponse.

I'm not sure if we'll need the redirect chain info. That was just something
I was wondering.

-Darin
On Aug 3, 2014 4:11 AM, <aa@chromium.org> wrote:

>
> https://codereview.chromium.org/423963004/diff/20001/mojo/
> public/interfaces/service_provider/content_handler.mojom
> File mojo/public/interfaces/service_provider/content_handler.mojom
> (right):
>
> https://codereview.chromium.org/423963004/diff/20001/mojo/
> public/interfaces/service_provider/content_handler.mojom#newcode11
> mojo/public/interfaces/service_provider/content_handler.mojom:11:
> handle<data_pipe_consumer> content,
> On 2014/08/03 06:12:02, darin wrote:
>
>> Are you thinking we can get away with not supplying the URLResponse
>>
> here? Some
>
>> response headers (e.g., Content-Type) will matter to content handlers.
>>
> An app
>
>> might be registered as a content handler for N mime types. It might
>>
> also find
>
>> the charset hint from the response headers useful. But, perhaps it is
>>
> possible
>
>> to provide something less than a full URLResponse data structure here.
>>
>
> No, we will need the full URLResponse or something close to it. I
> encountered some dependency issues using it, changed it to this data
> pipe temporarily, then forgot to go back and figure that out. I will
> look at it again, maybe in a separate change.
>
>  Also, is it useful / good (from a security perspective) to provide the
>>
> chain of
>
>> redirects to the content handler?
>>
>
> I'm not sure. Currently I don't see anything in URLResponse that exposes
> the redirect chain - am I missing it? Does Blink need the redirect chain
> for any reason?
>
> https://codereview.chromium.org/423963004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Aaron Boodman, 2014-08-04 16:40:41]

+davemoore for ServiceManager, ServiceLoader, and DynamicServiceLoader in
particular.

Dave, I will ask you about some of the DSL tests in person today.

[Aaron Boodman, 2014-08-04 18:24:40]

I have updated ContentHandler to use URLResponse instead of data pipe. Pray I do
not update it further.

[DaveMoore, 2014-08-04 21:50:52]

https://codereview.chromium.org/423963004/diff/40001/mojo/service_manager/ser...
File mojo/service_manager/service_manager.h (right):

https://codereview.chromium.org/423963004/diff/40001/mojo/service_manager/ser...
mojo/service_manager/service_manager.h:98: typedef std::map<GURL,
ContentHandlerConnection*> ContentHandlerMap;
Nit: We should have consistent naming of the types. This would be
URLToContentHandlerMap.

https://codereview.chromium.org/423963004/diff/40001/mojo/service_manager/ser...
mojo/service_manager/service_manager.h:131: ContentHandlerMap content_handlers_;
Nit: url_to_content_handler_

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/context.cc
File mojo/shell/context.cc (right):

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/context.cc#ne...
mojo/shell/context.cc:77: if (shell_handle.is_valid()) {
Nit: no braces

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/context.cc#ne...
mojo/shell/context.cc:107: "text/plain",
GURL("mojo://mojo_content_handler_demo/"));
mojo_content_handler_demo?

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/dynamic_servi...
File mojo/shell/dynamic_service_loader.cc (right):

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader.cc:60: if (resolved_url.SchemeIsFile()) {
Nit: no braces

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader.cc:83: if (!url_loader_) {
Nit: no braces

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader.cc:133: if (!path_exists) {
Nit: no braces

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader.cc:138: if (!shell_handle.is_valid()) {
Nit: no braces

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/dynamic_servi...
File mojo/shell/dynamic_service_loader.h (right):

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader.h:46: typedef std::map<std::string, GURL>
ContentHandlerMap;
Nit: MimeTypeToURLMap or MimeTypeToContentHandlerMap. I prefer the former,
mostly because there's a type called ContentHandler but this isn't one of those.

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/network_servi...
File mojo/shell/network_service_loader.cc (right):

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/network_servi...
mojo/shell/network_service_loader.cc:37: if (!shell_handle.is_valid()) {
Nit: no braces

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/ui_service_lo...
File mojo/shell/ui_service_loader_android.cc (right):

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/ui_service_lo...
mojo/shell/ui_service_loader_android.cc:52: if (!shell_handle.is_valid()) {
Nit: no braces

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/view_manager_...
File mojo/shell/view_manager_loader.cc (right):

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/view_manager_...
mojo/shell/view_manager_loader.cc:28: if (!shell_handle.is_valid()) {
Nit: no braces

[Aaron Boodman, 2014-08-05 05:44:55]

https://codereview.chromium.org/423963004/diff/40001/mojo/service_manager/ser...
File mojo/service_manager/service_manager.h (right):

https://codereview.chromium.org/423963004/diff/40001/mojo/service_manager/ser...
mojo/service_manager/service_manager.h:98: typedef std::map<GURL,
ContentHandlerConnection*> ContentHandlerMap;
On 2014/08/04 21:50:51, DaveMoore wrote:
> Nit: We should have consistent naming of the types. This would be
> URLToContentHandlerMap.

Done.

https://codereview.chromium.org/423963004/diff/40001/mojo/service_manager/ser...
mojo/service_manager/service_manager.h:131: ContentHandlerMap content_handlers_;
On 2014/08/04 21:50:51, DaveMoore wrote:
> Nit: url_to_content_handler_

Done.

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/context.cc
File mojo/shell/context.cc (right):

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/context.cc#ne...
mojo/shell/context.cc:77: if (shell_handle.is_valid()) {
On 2014/08/04 21:50:51, DaveMoore wrote:
> Nit: no braces

Done.

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/context.cc#ne...
mojo/shell/context.cc:107: "text/plain",
GURL("mojo://mojo_content_handler_demo/"));
On 2014/08/04 21:50:51, DaveMoore wrote:
> mojo_content_handler_demo?

I don't understand this comment, but I've removed this line in favor of a
--content-handlers switch.

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/dynamic_servi...
File mojo/shell/dynamic_service_loader.cc (right):

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader.cc:60: if (resolved_url.SchemeIsFile()) {
On 2014/08/04 21:50:51, DaveMoore wrote:
> Nit: no braces

Done.

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader.cc:83: if (!url_loader_) {
On 2014/08/04 21:50:51, DaveMoore wrote:
> Nit: no braces

Done.

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader.cc:133: if (!path_exists) {
On 2014/08/04 21:50:52, DaveMoore wrote:
> Nit: no braces

Done.

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader.cc:138: if (!shell_handle.is_valid()) {
On 2014/08/04 21:50:52, DaveMoore wrote:
> Nit: no braces

Done.

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/dynamic_servi...
File mojo/shell/dynamic_service_loader.h (right):

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader.h:46: typedef std::map<std::string, GURL>
ContentHandlerMap;
On 2014/08/04 21:50:52, DaveMoore wrote:
> Nit: MimeTypeToURLMap or MimeTypeToContentHandlerMap. I prefer the former,
> mostly because there's a type called ContentHandler but this isn't one of
those.

Done.

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/network_servi...
File mojo/shell/network_service_loader.cc (right):

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/network_servi...
mojo/shell/network_service_loader.cc:37: if (!shell_handle.is_valid()) {
On 2014/08/04 21:50:52, DaveMoore wrote:
> Nit: no braces

Done.

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/ui_service_lo...
File mojo/shell/ui_service_loader_android.cc (right):

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/ui_service_lo...
mojo/shell/ui_service_loader_android.cc:52: if (!shell_handle.is_valid()) {
On 2014/08/04 21:50:52, DaveMoore wrote:
> Nit: no braces

Done.

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/view_manager_...
File mojo/shell/view_manager_loader.cc (right):

https://codereview.chromium.org/423963004/diff/40001/mojo/shell/view_manager_...
mojo/shell/view_manager_loader.cc:28: if (!shell_handle.is_valid()) {
On 2014/08/04 21:50:52, DaveMoore wrote:
> Nit: no braces

Done.

[tim (not reviewing), 2014-08-05 20:45:55]

https://codereview.chromium.org/423963004/diff/60001/mojo/service_manager/ser...
File mojo/service_manager/service_manager.cc (right):

https://codereview.chromium.org/423963004/diff/60001/mojo/service_manager/ser...
mojo/service_manager/service_manager.cc:166:
STLDeleteValues(&url_to_content_handler_);
Are connections you're adding here for content handlers still represented by a
ServiceManager::ShellImpl <> ApplicationImpl?  The idea with
TerminateShellConnections (and how it's used in
https://codereview.chromium.org/394903005/) is that the Application side watches
its ShellPtr for a pipe error to Quits itself.  TerminateShellConnections is
used (by tests as of that CL) to trigger that pipe error, and wait for KeepAlive
to notice that no connections remain and kill the shell loop. "no connections
remain" from KeepAlive's perspective right now means that the
keep_alive_counter() hits 0, which right now mostly translates to all of the
InProcessDynamicServiceRunners having completed / gone away.

[Aaron Boodman, 2014-08-05 22:45:35]

https://codereview.chromium.org/423963004/diff/60001/mojo/service_manager/ser...
File mojo/service_manager/service_manager.cc (right):

https://codereview.chromium.org/423963004/diff/60001/mojo/service_manager/ser...
mojo/service_manager/service_manager.cc:166:
STLDeleteValues(&url_to_content_handler_);
Yes, content handlers are also normal mojo applications, so they will show up in
url_to_shell_impl_. This map is just a cache of the ContentHandler interface.

[Aaron Boodman, 2014-08-05 23:22:16]

tim, can you please review the changes to the tests?

[tim (not reviewing), 2014-08-05 23:52:37]

https://codereview.chromium.org/423963004/diff/80001/mojo/shell/dynamic_servi...
File mojo/shell/dynamic_service_loader.cc (right):

https://codereview.chromium.org/423963004/diff/80001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader.cc:132: // InProcessDynamicServiceRunner to be
destroyed so that the shell will exit.
What if you don't create the InProcessDynamicServiceRunner?  Previously we were
creating the runner before we knew if the path existed, and that created the
KeepAlive problem...

https://codereview.chromium.org/423963004/diff/80001/mojo/shell/dynamic_servi...
File mojo/shell/dynamic_service_loader_unittest.cc (right):

https://codereview.chromium.org/423963004/diff/80001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader_unittest.cc:25: explicit
TestDynamicServiceRunner(Context* context, TestState* state)
nit - don't need explicit

https://codereview.chromium.org/423963004/diff/80001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader_unittest.cc:34:
InProcessDynamicServiceRunner::Start(app_path, service_handle.Pass(),
What is using a real InProcessDSR buying us?

[tim (not reviewing), 2014-08-06 00:12:35]

https://codereview.chromium.org/423963004/diff/120001/mojo/shell/dynamic_serv...
File mojo/shell/dynamic_service_loader.cc (right):

https://codereview.chromium.org/423963004/diff/120001/mojo/shell/dynamic_serv...
mojo/shell/dynamic_service_loader.cc:137: if (!path_exists)
I wasn't looking at the latest patch set, sorry.  I see.  Hm.. it seems
shell::Run was trying to account for this by allocating a KeepAlive, but even
that probably doesn't work given the async nature of Connect.  Maybe we want a
KeepAlive to exist until a runner is either created, or an attempt fails (like
this case)?  Thinking...

[Aaron Boodman, 2014-08-06 00:37:23]

https://codereview.chromium.org/423963004/diff/80001/mojo/shell/dynamic_servi...
File mojo/shell/dynamic_service_loader.cc (right):

https://codereview.chromium.org/423963004/diff/80001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader.cc:132: // InProcessDynamicServiceRunner to be
destroyed so that the shell will exit.
(please see latest version of patch, but it's the same idea)

On 2014/08/05 23:52:37, timsteele wrote:
> What if you don't create the InProcessDynamicServiceRunner?  Previously we
were
> creating the runner before we knew if the path existed, and that created the
> KeepAlive problem...

If I don't create IPDSR, then the shell hangs in
ShellTestBaseTest::ConnectInvalidService because we never tell the message loop
QuitWhenIdle(), because a KeepAlive never gets destroyed, because we never
create (or destroy) any IPDSR.

https://codereview.chromium.org/423963004/diff/80001/mojo/shell/dynamic_servi...
File mojo/shell/dynamic_service_loader_unittest.cc (right):

https://codereview.chromium.org/423963004/diff/80001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader_unittest.cc:34:
InProcessDynamicServiceRunner::Start(app_path, service_handle.Pass(),
On 2014/08/05 23:52:37, timsteele wrote:
> What is using a real InProcessDSR buying us?

It's not needed with the current patch, I had the change in by mistake. I've
reverted it.

I think it was needed in one version of the code in order to get
app_completed_callback to run. But I can't remember exactly anymore. I think I
need dinner.

[Aaron Boodman, 2014-08-06 00:39:25]

On 2014/08/06 00:12:35, timsteele wrote:
>
https://codereview.chromium.org/423963004/diff/120001/mojo/shell/dynamic_serv...
> File mojo/shell/dynamic_service_loader.cc (right):
> 
>
https://codereview.chromium.org/423963004/diff/120001/mojo/shell/dynamic_serv...
> mojo/shell/dynamic_service_loader.cc:137: if (!path_exists)
> I wasn't looking at the latest patch set, sorry.  I see.  Hm.. it seems
> shell::Run was trying to account for this by allocating a KeepAlive, but even
> that probably doesn't work given the async nature of Connect.  Maybe we want a
> KeepAlive to exist until a runner is either created, or an attempt fails (like
> this case)?  Thinking...

No idea. I'm not really sure what the shutdown code is trying to achieve,
because to me it seems reasonable that shell runs forever, waiting for
connections, until somebody external quits it.

[tim (not reviewing), 2014-08-06 05:57:52]

On 2014/08/06 00:39:25, Aaron Boodman wrote:
> On 2014/08/06 00:12:35, timsteele wrote:
> >
>
https://codereview.chromium.org/423963004/diff/120001/mojo/shell/dynamic_serv...
> > File mojo/shell/dynamic_service_loader.cc (right):
> > 
> >
>
https://codereview.chromium.org/423963004/diff/120001/mojo/shell/dynamic_serv...
> > mojo/shell/dynamic_service_loader.cc:137: if (!path_exists)
> > I wasn't looking at the latest patch set, sorry.  I see.  Hm.. it seems
> > shell::Run was trying to account for this by allocating a KeepAlive, but
even
> > that probably doesn't work given the async nature of Connect.  Maybe we want
a
> > KeepAlive to exist until a runner is either created, or an attempt fails
(like
> > this case)?  Thinking...
> 
> No idea. I'm not really sure what the shutdown code is trying to achieve,
> because to me it seems reasonable that shell runs forever, waiting for
> connections, until somebody external quits it.

Hm. At the same time, if you run ./mojo_shell mojo:mojo_foo_app, you probably
expect it to finish when foo_app finishes. e.g mojo_wget just sits there after
completing and there's nothing you can do but ctrl-C. That's one of my goals for
shutdown.

The more immediately relevant goal is for tests, because being able to fully
quit everything is (I'd argue) essential for running more than one (in-process)
test that brings up the shell. Even if we call it an error that a particular app
that was started as part of a test didn't terminate itself cleanly (and I want
to do that... see todo in shell_test_base_unittest).

[Aaron Boodman, 2014-08-06 19:50:05]

On Tue, Aug 5, 2014 at 10:57 PM, <tim@chromium.org> wrote:

> On 2014/08/06 00:39:25, Aaron Boodman wrote:
>
>> On 2014/08/06 00:12:35, timsteele wrote:
>> >
>>
>
> https://codereview.chromium.org/423963004/diff/120001/
> mojo/shell/dynamic_service_loader.cc
>
>> > File mojo/shell/dynamic_service_loader.cc (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/423963004/diff/120001/
> mojo/shell/dynamic_service_loader.cc#newcode137
>
>> > mojo/shell/dynamic_service_loader.cc:137: if (!path_exists)
>> > I wasn't looking at the latest patch set, sorry.  I see.  Hm.. it seems
>> > shell::Run was trying to account for this by allocating a KeepAlive, but
>>
> even
>
>> > that probably doesn't work given the async nature of Connect.  Maybe we
>> want
>>
> a
>
>> > KeepAlive to exist until a runner is either created, or an attempt fails
>>
> (like
>
>> > this case)?  Thinking...
>>
>
>  No idea. I'm not really sure what the shutdown code is trying to achieve,
>> because to me it seems reasonable that shell runs forever, waiting for
>> connections, until somebody external quits it.
>>
>
> Hm. At the same time, if you run ./mojo_shell mojo:mojo_foo_app, you
> probably
> expect it to finish when foo_app finishes. e.g mojo_wget just sits there
> after
> completing and there's nothing you can do but ctrl-C. That's one of my
> goals for
> shutdown.
>
> The more immediately relevant goal is for tests, because being able to
> fully
> quit everything is (I'd argue) essential for running more than one
> (in-process)
> test that brings up the shell. Even if we call it an error that a
> particular app
> that was started as part of a test didn't terminate itself cleanly (and I
> want
> to do that... see todo in shell_test_base_unittest).
>

Makes sense. Sounds like we really want to wrap each call to Connect() in a
keep alive, right from the time that it is received. Currently the keep
alive is only started when the DSO is loaded. Sounds like a separate change
though.

https://codereview.chromium.org/423963004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaveMoore, 2014-08-06 20:50:46]

https://codereview.chromium.org/423963004/diff/160001/mojo/shell/dbus_service...
File mojo/shell/dbus_service_loader_linux.cc (right):

https://codereview.chromium.org/423963004/diff/160001/mojo/shell/dbus_service...
mojo/shell/dbus_service_loader_linux.cc:160: if (!shell_handle.is_valid()) {
Nit: no braces

https://codereview.chromium.org/423963004/diff/160001/mojo/shell/dynamic_serv...
File mojo/shell/dynamic_service_loader.h (right):

https://codereview.chromium.org/423963004/diff/160001/mojo/shell/dynamic_serv...
mojo/shell/dynamic_service_loader.h:35: const GURL& content_handler_url);
SetContentHandlerURL() or SetContentHandlerURLForMimeType()

Also, I think this may belong on the ServiceManager, not within the
DynamicServiceLoader

[Aaron Boodman, 2014-08-06 22:49:30]

https://codereview.chromium.org/423963004/diff/160001/mojo/shell/dbus_service...
File mojo/shell/dbus_service_loader_linux.cc (right):

https://codereview.chromium.org/423963004/diff/160001/mojo/shell/dbus_service...
mojo/shell/dbus_service_loader_linux.cc:160: if (!shell_handle.is_valid()) {
On 2014/08/06 20:50:46, DaveMoore wrote:
> Nit: no braces

Done.

https://codereview.chromium.org/423963004/diff/160001/mojo/shell/dynamic_serv...
File mojo/shell/dynamic_service_loader.h (right):

https://codereview.chromium.org/423963004/diff/160001/mojo/shell/dynamic_serv...
mojo/shell/dynamic_service_loader.h:35: const GURL& content_handler_url);
On 2014/08/06 20:50:46, DaveMoore wrote:
> SetContentHandlerURL() or SetContentHandlerURLForMimeType()
> 
> Also, I think this may belong on the ServiceManager, not within the
> DynamicServiceLoader

I looked at doing this, but it makes ownership of the |response| param  to
LoadServiceCallbacks::LoadWithContentHandler() conditional on the success of the
function:

bool LoadServiceCallbacksImpl::LoadWithContentHandler(
    URLResponsePtr* response) {

  MimeTypeToContentHandler::iterator iter = 
      mime_type_to_content_handler_.find(response->mime_type);
  if (iter == mime_type_to_content_handler_.end()) {
    // Caller retains ownership of |response|.
    return;
  }

  // We take ownership of |response|.
  URLResponsePtr owned_response = response->Pass();

  ...
}

There are a few more speculative problems I can forsee with having this in SM:

1) When figuring out the right handler requires sniffing the beginning of the
data (as it does on the web), then LoadWithContentHandler() will need to consume
some of the response. But if it fails, then it needs to return that consumed
data back to the caller somehow so that the caller can use it to run the DSO.

2) I suspect that finding the right content handler isn't going to be as much of
a global map of mimetype->handler as it looks like in this change. Think of this
as temporary scaffolding. The mapping of content type to handler might not even
live in the shell ... it might be a feature of the window manager, since really
that's where it's needed. When applications are just randomly wanting to 

Anyway, I'd be happy to just go ahead with your idea see how the code evolves if
it wasn't more complex. But since it is more complex I'd prefer to stick with
the current design for now (pending Darin's comments, anyway).

[Aaron Boodman, 2014-08-06 22:50:18]

On 2014/08/06 22:49:30, Aaron Boodman wrote:
> 2) I suspect that finding the right content handler isn't going to be as much
of
> a global map of mimetype->handler as it looks like in this change. Think of
this
> as temporary scaffolding. The mapping of content type to handler might not
even
> live in the shell ... it might be a feature of the window manager, since
really
> that's where it's needed. When applications are just randomly wanting to 

... connect, they might not want this dynamic matchmaking behavior.

[darin (slow to review), 2014-08-06 23:10:02]

LGTM

https://codereview.chromium.org/423963004/diff/160001/mojo/mojo_examples.gypi
File mojo/mojo_examples.gypi (right):

https://codereview.chromium.org/423963004/diff/160001/mojo/mojo_examples.gypi...
mojo/mojo_examples.gypi:148: 'target_name': 'mojo_content_handler_demo',
you probably want to add this to the "mojo" target.

https://codereview.chromium.org/423963004/diff/160001/mojo/service_manager/se...
File mojo/service_manager/service_loader.h (right):

https://codereview.chromium.org/423963004/diff/160001/mojo/service_manager/se...
mojo/service_manager/service_loader.h:28: class LoadServiceCallbacks : public
base::RefCounted<LoadServiceCallbacks> {
nit: you might consider just naming this class Callbacks since "LoadService" is
a bit redundant with the containing class name ServiceLoader. Another option is
to rename the LoadService method to just be Load and name this callbacks class
LoadCallbacks. That way you preserve the relationship between the method and the
callbacks class, and you get to have shorter names.

https://codereview.chromium.org/423963004/diff/160001/mojo/service_manager/se...
mojo/service_manager/service_loader.h:71: virtual void
LoadService(ServiceManager* service_manager,
nit: this method could just be called Load

https://codereview.chromium.org/423963004/diff/160001/mojo/service_manager/se...
File mojo/service_manager/service_manager.cc (right):

https://codereview.chromium.org/423963004/diff/160001/mojo/service_manager/se...
mojo/service_manager/service_manager.cc:136: MessagePipe pipe;
nit: these 3 lines of code can be replaced with mojo::ConnectToService(). See
connect.h.

https://codereview.chromium.org/423963004/diff/160001/mojo/services/public/in...
File mojo/services/public/interfaces/content_handler/content_handler.mojom
(right):

https://codereview.chromium.org/423963004/diff/160001/mojo/services/public/in...
mojo/services/public/interfaces/content_handler/content_handler.mojom:10:
interface ContentHandler {
by the way, I'm not 100% sold on the name "OnConnect". it doesn't seem to go
with ContentHandler. I get the relationship with ConnectToApplication, so I
guess it works. Maybe we need to think more about these names. That can happen
after this CL lands too.

https://codereview.chromium.org/423963004/diff/160001/mojo/services/public/in...
mojo/services/public/interfaces/content_handler/content_handler.mojom:11:
OnConnect(string url,
Isn't this |url| parameter redundant? The URLResponse should have an identical
|url| field.

https://codereview.chromium.org/423963004/diff/160001/mojo/shell/dynamic_serv...
File mojo/shell/dynamic_service_loader.cc (right):

https://codereview.chromium.org/423963004/diff/160001/mojo/shell/dynamic_serv...
mojo/shell/dynamic_service_loader.cc:46: void
DynamicServiceLoader::set_content_handler(
nit: perhaps this method should be called RegisterContentHandler? it seems to be
registering a mime type -> content handler mapping.

[Aaron Boodman, 2014-08-07 00:03:12]

https://codereview.chromium.org/423963004/diff/160001/mojo/mojo_examples.gypi
File mojo/mojo_examples.gypi (right):

https://codereview.chromium.org/423963004/diff/160001/mojo/mojo_examples.gypi...
mojo/mojo_examples.gypi:148: 'target_name': 'mojo_content_handler_demo',
On 2014/08/06 23:10:01, darin wrote:
> you probably want to add this to the "mojo" target.

Thanks. done.

https://codereview.chromium.org/423963004/diff/160001/mojo/service_manager/se...
File mojo/service_manager/service_loader.h (right):

https://codereview.chromium.org/423963004/diff/160001/mojo/service_manager/se...
mojo/service_manager/service_loader.h:28: class LoadServiceCallbacks : public
base::RefCounted<LoadServiceCallbacks> {
On 2014/08/06 23:10:02, darin wrote:
> nit: you might consider just naming this class Callbacks since "LoadService"
is
> a bit redundant with the containing class name ServiceLoader. Another option
is
> to rename the LoadService method to just be Load and name this callbacks class
> LoadCallbacks. That way you preserve the relationship between the method and
the
> callbacks class, and you get to have shorter names.

I like the second idea. Done.

https://codereview.chromium.org/423963004/diff/160001/mojo/service_manager/se...
mojo/service_manager/service_loader.h:71: virtual void
LoadService(ServiceManager* service_manager,
On 2014/08/06 23:10:01, darin wrote:
> nit: this method could just be called Load

Done.

https://codereview.chromium.org/423963004/diff/160001/mojo/service_manager/se...
File mojo/service_manager/service_manager.cc (right):

https://codereview.chromium.org/423963004/diff/160001/mojo/service_manager/se...
mojo/service_manager/service_manager.cc:136: MessagePipe pipe;
On 2014/08/06 23:10:02, darin wrote:
> nit: these 3 lines of code can be replaced with mojo::ConnectToService(). See
> connect.h.

Done.

https://codereview.chromium.org/423963004/diff/160001/mojo/services/public/in...
File mojo/services/public/interfaces/content_handler/content_handler.mojom
(right):

https://codereview.chromium.org/423963004/diff/160001/mojo/services/public/in...
mojo/services/public/interfaces/content_handler/content_handler.mojom:10:
interface ContentHandler {
On 2014/08/06 23:10:02, darin wrote:
> by the way, I'm not 100% sold on the name "OnConnect". it doesn't seem to go
> with ContentHandler. I get the relationship with ConnectToApplication, so I
> guess it works. Maybe we need to think more about these names. That can happen
> after this CL lands too.

Yeah. I am not 100% with it either. There is also the issue that in the future,
I think we will want to invoke this interface but not actually pass content in
order to handle offline applications.

For example I imagine that apps will be able to tell the system that they can
handle a URL space. In that case, when we connect to them, we might end up
calling this interface, but not doing a network request.

I think this is really more of a DelegatedApplication or something. But I
figured I'd let it be for now and probably things would become more clear as we
move forward.

https://codereview.chromium.org/423963004/diff/160001/mojo/services/public/in...
mojo/services/public/interfaces/content_handler/content_handler.mojom:11:
OnConnect(string url,
On 2014/08/06 23:10:02, darin wrote:
> Isn't this |url| parameter redundant? The URLResponse should have an identical
> |url| field.

No, because redirects. Also, as I said above in the future we might not pass
|response| here.

https://codereview.chromium.org/423963004/diff/160001/mojo/shell/dynamic_serv...
File mojo/shell/dynamic_service_loader.cc (right):

https://codereview.chromium.org/423963004/diff/160001/mojo/shell/dynamic_serv...
mojo/shell/dynamic_service_loader.cc:46: void
DynamicServiceLoader::set_content_handler(
On 2014/08/06 23:10:02, darin wrote:
> nit: perhaps this method should be called RegisterContentHandler? it seems to
be
> registering a mime type -> content handler mapping.

Fair enough. Done.

[Aaron Boodman, 2014-08-07 00:58:47]

The CQ bit was checked by aa@chromium.org

[commit-bot: I haz the power, 2014-08-07 01:01:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aa@chromium.org/423963004/220001

[darin (slow to review), 2014-08-07 01:27:03]

The URLResponse's url field is the post-redirect URL. Isn't that the same
as the URL passed to OnConnect.

URL interception, a la ServiceWorker, would likely just happen at the
NetworkService level. There would always be an URLResponse it seems. URL
interception would be about vending a "fake" URLResponse, no?

-Darin
On Aug 6, 2014 5:03 PM, <aa@chromium.org> wrote:

>
> https://codereview.chromium.org/423963004/diff/160001/
> mojo/mojo_examples.gypi
> File mojo/mojo_examples.gypi (right):
>
> https://codereview.chromium.org/423963004/diff/160001/
> mojo/mojo_examples.gypi#newcode148
> mojo/mojo_examples.gypi:148: 'target_name': 'mojo_content_handler_demo',
> On 2014/08/06 23:10:01, darin wrote:
>
>> you probably want to add this to the "mojo" target.
>>
>
> Thanks. done.
>
> https://codereview.chromium.org/423963004/diff/160001/
> mojo/service_manager/service_loader.h
> File mojo/service_manager/service_loader.h (right):
>
> https://codereview.chromium.org/423963004/diff/160001/
> mojo/service_manager/service_loader.h#newcode28
> mojo/service_manager/service_loader.h:28: class LoadServiceCallbacks :
> public base::RefCounted<LoadServiceCallbacks> {
> On 2014/08/06 23:10:02, darin wrote:
>
>> nit: you might consider just naming this class Callbacks since
>>
> "LoadService" is
>
>> a bit redundant with the containing class name ServiceLoader. Another
>>
> option is
>
>> to rename the LoadService method to just be Load and name this
>>
> callbacks class
>
>> LoadCallbacks. That way you preserve the relationship between the
>>
> method and the
>
>> callbacks class, and you get to have shorter names.
>>
>
> I like the second idea. Done.
>
> https://codereview.chromium.org/423963004/diff/160001/
> mojo/service_manager/service_loader.h#newcode71
> mojo/service_manager/service_loader.h:71: virtual void
> LoadService(ServiceManager* service_manager,
> On 2014/08/06 23:10:01, darin wrote:
>
>> nit: this method could just be called Load
>>
>
> Done.
>
> https://codereview.chromium.org/423963004/diff/160001/
> mojo/service_manager/service_manager.cc
> File mojo/service_manager/service_manager.cc (right):
>
> https://codereview.chromium.org/423963004/diff/160001/
> mojo/service_manager/service_manager.cc#newcode136
> mojo/service_manager/service_manager.cc:136: MessagePipe pipe;
> On 2014/08/06 23:10:02, darin wrote:
>
>> nit: these 3 lines of code can be replaced with
>>
> mojo::ConnectToService(). See
>
>> connect.h.
>>
>
> Done.
>
> https://codereview.chromium.org/423963004/diff/160001/
> mojo/services/public/interfaces/content_handler/content_handler.mojom
> File
> mojo/services/public/interfaces/content_handler/content_handler.mojom
> (right):
>
> https://codereview.chromium.org/423963004/diff/160001/
> mojo/services/public/interfaces/content_handler/content_handler.mojom#
> newcode10
> mojo/services/public/interfaces/content_handler/content_handler.mojom:10:
> interface ContentHandler {
> On 2014/08/06 23:10:02, darin wrote:
>
>> by the way, I'm not 100% sold on the name "OnConnect". it doesn't seem
>>
> to go
>
>> with ContentHandler. I get the relationship with ConnectToApplication,
>>
> so I
>
>> guess it works. Maybe we need to think more about these names. That
>>
> can happen
>
>> after this CL lands too.
>>
>
> Yeah. I am not 100% with it either. There is also the issue that in the
> future, I think we will want to invoke this interface but not actually
> pass content in order to handle offline applications.
>
> For example I imagine that apps will be able to tell the system that
> they can handle a URL space. In that case, when we connect to them, we
> might end up calling this interface, but not doing a network request.
>
> I think this is really more of a DelegatedApplication or something. But
> I figured I'd let it be for now and probably things would become more
> clear as we move forward.
>
> https://codereview.chromium.org/423963004/diff/160001/
> mojo/services/public/interfaces/content_handler/content_handler.mojom#
> newcode11
> mojo/services/public/interfaces/content_handler/content_handler.mojom:11:
> OnConnect(string url,
> On 2014/08/06 23:10:02, darin wrote:
>
>> Isn't this |url| parameter redundant? The URLResponse should have an
>>
> identical
>
>> |url| field.
>>
>
> No, because redirects. Also, as I said above in the future we might not
> pass |response| here.
>
> https://codereview.chromium.org/423963004/diff/160001/
> mojo/shell/dynamic_service_loader.cc
> File mojo/shell/dynamic_service_loader.cc (right):
>
> https://codereview.chromium.org/423963004/diff/160001/
> mojo/shell/dynamic_service_loader.cc#newcode46
> mojo/shell/dynamic_service_loader.cc:46: void
> DynamicServiceLoader::set_content_handler(
> On 2014/08/06 23:10:02, darin wrote:
>
>> nit: perhaps this method should be called RegisterContentHandler? it
>>
> seems to be
>
>> registering a mime type -> content handler mapping.
>>
>
> Fair enough. Done.
>
> https://codereview.chromium.org/423963004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Aaron Boodman, 2014-08-07 01:40:23]

The CQ bit was checked by aa@chromium.org

[commit-bot: I haz the power, 2014-08-07 01:42:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aa@chromium.org/423963004/240001

[Aaron Boodman, 2014-08-07 06:04:36]

The CQ bit was checked by aa@chromium.org

[commit-bot: I haz the power, 2014-08-07 06:05:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aa@chromium.org/423963004/290001

[commit-bot: I haz the power, 2014-08-07 08:23:15]

Change committed as 288013

[Roger Tawa OOO till Jul 10th, 2014-08-07 15:10:08]

Aaron: I had to revert this because it broke the tree.  I updated description of
this CL with details.

[Aaron Boodman, 2014-08-08 04:33:59]

Sorry, I forgot to reply to this.

On 2014/08/07 01:27:03, darin wrote:
> The URLResponse's url field is the post-redirect URL. Isn't that the same
> as the URL passed to OnConnect.

No, because of redirects. The URL that gets sent to OnConnect is (currently) the
URL that was originally requested. For example if you request
mojo://mojo_html_viewer and we translate that to http://localhost:4444/... I
send the mojo URL to OnConnect.

In some cases I could imagine that maybe the place that an app was deployed is
not known to the original author of the app, and that they would want to know
that ... ? Maybe I'm making things up.

> URL interception, a la ServiceWorker, would likely just happen at the
> NetworkService level. There would always be an URLResponse it seems. URL
> interception would be about vending a "fake" URLResponse, no?

I think maybe you are thinking about how offline connections look to the
requesting application? But I'm talking about how they look to the receiving
application.

Say an HTML SW registers to handle all of https://foo/*. User navigates to
https://foo/bar, which we don't have in the cache. What do we send to OnConnect
for the HTMLViewer application in that case? I was thinking we wouldn't send
anything, and HTMLViewer would look at the request and spin up the SW.

Sounds like you had a different direction in mind...?

> -Darin
> On Aug 6, 2014 5:03 PM, <mailto:aa@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/423963004/diff/160001/
> > mojo/mojo_examples.gypi
> > File mojo/mojo_examples.gypi (right):
> >
> > https://codereview.chromium.org/423963004/diff/160001/
> > mojo/mojo_examples.gypi#newcode148
> > mojo/mojo_examples.gypi:148: 'target_name': 'mojo_content_handler_demo',
> > On 2014/08/06 23:10:01, darin wrote:
> >
> >> you probably want to add this to the "mojo" target.
> >>
> >
> > Thanks. done.
> >
> > https://codereview.chromium.org/423963004/diff/160001/
> > mojo/service_manager/service_loader.h
> > File mojo/service_manager/service_loader.h (right):
> >
> > https://codereview.chromium.org/423963004/diff/160001/
> > mojo/service_manager/service_loader.h#newcode28
> > mojo/service_manager/service_loader.h:28: class LoadServiceCallbacks :
> > public base::RefCounted<LoadServiceCallbacks> {
> > On 2014/08/06 23:10:02, darin wrote:
> >
> >> nit: you might consider just naming this class Callbacks since
> >>
> > "LoadService" is
> >
> >> a bit redundant with the containing class name ServiceLoader. Another
> >>
> > option is
> >
> >> to rename the LoadService method to just be Load and name this
> >>
> > callbacks class
> >
> >> LoadCallbacks. That way you preserve the relationship between the
> >>
> > method and the
> >
> >> callbacks class, and you get to have shorter names.
> >>
> >
> > I like the second idea. Done.
> >
> > https://codereview.chromium.org/423963004/diff/160001/
> > mojo/service_manager/service_loader.h#newcode71
> > mojo/service_manager/service_loader.h:71: virtual void
> > LoadService(ServiceManager* service_manager,
> > On 2014/08/06 23:10:01, darin wrote:
> >
> >> nit: this method could just be called Load
> >>
> >
> > Done.
> >
> > https://codereview.chromium.org/423963004/diff/160001/
> > mojo/service_manager/service_manager.cc
> > File mojo/service_manager/service_manager.cc (right):
> >
> > https://codereview.chromium.org/423963004/diff/160001/
> > mojo/service_manager/service_manager.cc#newcode136
> > mojo/service_manager/service_manager.cc:136: MessagePipe pipe;
> > On 2014/08/06 23:10:02, darin wrote:
> >
> >> nit: these 3 lines of code can be replaced with
> >>
> > mojo::ConnectToService(). See
> >
> >> connect.h.
> >>
> >
> > Done.
> >
> > https://codereview.chromium.org/423963004/diff/160001/
> > mojo/services/public/interfaces/content_handler/content_handler.mojom
> > File
> > mojo/services/public/interfaces/content_handler/content_handler.mojom
> > (right):
> >
> > https://codereview.chromium.org/423963004/diff/160001/
> > mojo/services/public/interfaces/content_handler/content_handler.mojom#
> > newcode10
> > mojo/services/public/interfaces/content_handler/content_handler.mojom:10:
> > interface ContentHandler {
> > On 2014/08/06 23:10:02, darin wrote:
> >
> >> by the way, I'm not 100% sold on the name "OnConnect". it doesn't seem
> >>
> > to go
> >
> >> with ContentHandler. I get the relationship with ConnectToApplication,
> >>
> > so I
> >
> >> guess it works. Maybe we need to think more about these names. That
> >>
> > can happen
> >
> >> after this CL lands too.
> >>
> >
> > Yeah. I am not 100% with it either. There is also the issue that in the
> > future, I think we will want to invoke this interface but not actually
> > pass content in order to handle offline applications.
> >
> > For example I imagine that apps will be able to tell the system that
> > they can handle a URL space. In that case, when we connect to them, we
> > might end up calling this interface, but not doing a network request.
> >
> > I think this is really more of a DelegatedApplication or something. But
> > I figured I'd let it be for now and probably things would become more
> > clear as we move forward.
> >
> > https://codereview.chromium.org/423963004/diff/160001/
> > mojo/services/public/interfaces/content_handler/content_handler.mojom#
> > newcode11
> > mojo/services/public/interfaces/content_handler/content_handler.mojom:11:
> > OnConnect(string url,
> > On 2014/08/06 23:10:02, darin wrote:
> >
> >> Isn't this |url| parameter redundant? The URLResponse should have an
> >>
> > identical
> >
> >> |url| field.
> >>
> >
> > No, because redirects. Also, as I said above in the future we might not
> > pass |response| here.
> >
> > https://codereview.chromium.org/423963004/diff/160001/
> > mojo/shell/dynamic_service_loader.cc
> > File mojo/shell/dynamic_service_loader.cc (right):
> >
> > https://codereview.chromium.org/423963004/diff/160001/
> > mojo/shell/dynamic_service_loader.cc#newcode46
> > mojo/shell/dynamic_service_loader.cc:46: void
> > DynamicServiceLoader::set_content_handler(
> > On 2014/08/06 23:10:02, darin wrote:
> >
> >> nit: perhaps this method should be called RegisterContentHandler? it
> >>
> > seems to be
> >
> >> registering a mime type -> content handler mapping.
> >>
> >
> > Fair enough. Done.
> >
> > https://codereview.chromium.org/423963004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-08-08 04:45:55]

On 2014/08/08 04:33:59, Aaron Boodman wrote:
> Sorry, I forgot to reply to this.
> 
> On 2014/08/07 01:27:03, darin wrote:
> > The URLResponse's url field is the post-redirect URL. Isn't that the same
> > as the URL passed to OnConnect.
> 
> No, because of redirects. The URL that gets sent to OnConnect is (currently)
the
> URL that was originally requested. For example if you request
> mojo://mojo_html_viewer and we translate that to http://localhost:4444/... I
> send the mojo URL to OnConnect.

Oh, I see the issue now. That's not really a redirect in the HTTP sense. That's
URL rewriting that happens early on. If the network stack actually understood
mojo:// URLs, then there would be no mapping or redirecting happening here. A
URLRequestMojoJob would know how to resolve and load data for a mojo:// URL.
It would be the truth, the origin, etc.

When you mention redirects, I was thinking more in terms of https://foo.com/
redirecting to https://bar.com/. I'm not sure the caller of ConnectToApp needs
to observe such redirects, and I'm not sure the app at https://bar.com/ needs
to know that the caller of ConnectToApp thinks they are talking to
https://foo.com/. I'm not sure what problems sharing that information solves.

Mainly though I'm arguing from the principle that foo.com could just be a
proxy, man-in-the-middle forwarding everything to bar.com anyways such that
bar.com doesn't realize the real client thinks they are talking to foo.com, and
the real client doesn't know they are really talking to bar.com. Since that is
possible, knowing that a redirect occurred doesn't really tell you who you are
really talking to, and moreover, the client might not want to talk to bar.com.
That would (IMO wrongly) prevent foo.com from delegating work to bar.com. Maybe
it is more efficient to implement the service that way, etc.


> In some cases I could imagine that maybe the place that an app was deployed is
> not known to the original author of the app, and that they would want to know
> that ... ? Maybe I'm making things up.
> 
> > URL interception, a la ServiceWorker, would likely just happen at the
> > NetworkService level. There would always be an URLResponse it seems. URL
> > interception would be about vending a "fake" URLResponse, no?
> 
> I think maybe you are thinking about how offline connections look to the
> requesting application? But I'm talking about how they look to the receiving
> application.
> 
> Say an HTML SW registers to handle all of https://foo/*. User navigates to
> https://foo/bar, which we don't have in the cache. What do we send to
OnConnect
> for the HTMLViewer application in that case? I was thinking we wouldn't send
> anything, and HTMLViewer would look at the request and spin up the SW.

I was thinking there would be a completely different interface for network
interception. An app that has registered to intercept network requests would get
invoked via the NetworkService. It would get a chance to vend a fake
URLResponse.
Then, to the rest of the system, including the ConnectToApplication
implementation,
we'd just use that URLResponse and do everything as we normally do.

-Darin



> 
> Sounds like you had a different direction in mind...?

[Aaron Boodman, 2014-08-08 05:16:52]

On Thu, Aug 7, 2014 at 9:45 PM, <darin@chromium.org> wrote:

> On 2014/08/08 04:33:59, Aaron Boodman wrote:
>
>> Sorry, I forgot to reply to this.
>>
>
>  On 2014/08/07 01:27:03, darin wrote:
>> > The URLResponse's url field is the post-redirect URL. Isn't that the
>> same
>> > as the URL passed to OnConnect.
>>
>
>  No, because of redirects. The URL that gets sent to OnConnect is
>> (currently)
>>
> the
>
>> URL that was originally requested. For example if you request
>> mojo://mojo_html_viewer and we translate that to http://localhost:4444/...
>> I
>> send the mojo URL to OnConnect.
>>
>
> Oh, I see the issue now. That's not really a redirect in the HTTP sense.


I know. I was just using that as an example of how the current code goes
out of its way to always tells the receiving app the URL that was
originally requested, no matter how it is munged along the pipeline. I
meant HTTP redirects too though.


> When you mention redirects, I was thinking more in terms of
> https://foo.com/
> redirecting to https://bar.com/. I'm not sure the caller of ConnectToApp
> needs
> to observe such redirects, and I'm not sure the app at https://bar.com/
> needs
> to know that the caller of ConnectToApp thinks they are talking to
> https://foo.com/. I'm not sure what problems sharing that information
> solves.


Mainly though I'm arguing from the principle that foo.com could just be a
> proxy, man-in-the-middle forwarding everything to bar.com anyways such
> that
> bar.com doesn't realize the real client thinks they are talking to foo.com,
> and
> the real client doesn't know they are really talking to bar.com. Since
> that is
> possible, knowing that a redirect occurred doesn't really tell you who you
> are
> really talking to, and moreover, the client might not want to talk to
> bar.com.
> That would (IMO wrongly) prevent foo.com from delegating work to bar.com.
> Maybe
> it is more efficient to implement the service that way, etc.


I like this argument, thanks. I will stop worrying about the redirect issue.


>  > URL interception, a la ServiceWorker, would likely just happen at the
>> > NetworkService level. There would always be an URLResponse it seems. URL
>> > interception would be about vending a "fake" URLResponse, no?
>>
>
>  I think maybe you are thinking about how offline connections look to the
>> requesting application? But I'm talking about how they look to the
>> receiving
>> application.
>>
>
>  Say an HTML SW registers to handle all of https://foo/*. User navigates
>> to
>> https://foo/bar, which we don't have in the cache. What do we send to
>>
> OnConnect
>
>> for the HTMLViewer application in that case? I was thinking we wouldn't
>> send
>> anything, and HTMLViewer would look at the request and spin up the SW.
>>
>
> I was thinking there would be a completely different interface for network
> interception. An app that has registered to intercept network requests
> would get
> invoked via the NetworkService. It would get a chance to vend a fake
> URLResponse.
> Then, to the rest of the system, including the ConnectToApplication
> implementation,
> we'd just use that URLResponse and do everything as we normally do.
>

I'm not thinking about the use case of simulating HTTP responses -- I'm
thinking about navigation.

If I have an app https://foo/bar, I want it to be able to register control
of https://foo/*, such that when a node is navigated to https://foo/baz,
somehow https://foo/bar is invoked and gets control of that node. I don't
think that a fake HTTP response should be required here.

- a


> -Darin
>
>
>
>
>
>  Sounds like you had a different direction in mind...?
>>
>
>
>
> https://codereview.chromium.org/423963004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-08-08 05:29:24]

On 2014/08/08 05:16:52, Aaron Boodman wrote:
...
> >  > URL interception, a la ServiceWorker, would likely just happen at the
> >> > NetworkService level. There would always be an URLResponse it seems. URL
> >> > interception would be about vending a "fake" URLResponse, no?
> >>
> >
> >  I think maybe you are thinking about how offline connections look to the
> >> requesting application? But I'm talking about how they look to the
> >> receiving
> >> application.
> >>
> >
> >  Say an HTML SW registers to handle all of https://foo/*. User navigates
> >> to
> >> https://foo/bar, which we don't have in the cache. What do we send to
> >>
> > OnConnect
> >
> >> for the HTMLViewer application in that case? I was thinking we wouldn't
> >> send
> >> anything, and HTMLViewer would look at the request and spin up the SW.
> >>
> >
> > I was thinking there would be a completely different interface for network
> > interception. An app that has registered to intercept network requests
> > would get
> > invoked via the NetworkService. It would get a chance to vend a fake
> > URLResponse.
> > Then, to the rest of the system, including the ConnectToApplication
> > implementation,
> > we'd just use that URLResponse and do everything as we normally do.
> >
> 
> I'm not thinking about the use case of simulating HTTP responses -- I'm
> thinking about navigation.
> 
> If I have an app https://foo/bar, I want it to be able to register control
> of https://foo/*, such that when a node is navigated to https://foo/baz,
> somehow https://foo/bar is invoked and gets control of that node. I don't
> think that a fake HTTP response should be required here.

Oh, I see. Perhaps both forms of interception are interesting. For example,
if you wanted https://foo/baz to act like a PDF, then you would just fake
the network response, but just blanket taking over connections to an URL
could certainly be useful and much more efficient and less hacky than
generating fake responses. OK. Maybe that is still expressed as a different
interface? Or, maybe this just points to a reason why "content handler" might
not be the best name for the interface.

-Darin

[Aaron Boodman, 2014-08-08 17:12:59]

On 2014/08/08 05:29:24, darin wrote:
> Oh, I see. Perhaps both forms of interception are interesting. For example,
> if you wanted https://foo/baz to act like a PDF, then you would just fake
> the network response, but just blanket taking over connections to an URL
> could certainly be useful and much more efficient and less hacky than
> generating fake responses. OK. Maybe that is still expressed as a different
> interface? Or, maybe this just points to a reason why "content handler" might
> not be the best name for the interface.

Yeah, these are the sorts of issues I've been dancing around: connection,
navigation, and content handling are all tightly related and interconnected
concepts. I've gone back and forth about having 1, 2, or 3 interfaces for them
multiple times :). I'm still not happy with what we have, but I thought maybe
I'd make more progress by trying to move on to the next step.

[darin (slow to review), 2014-08-08 19:46:35]

On Fri, Aug 8, 2014 at 10:12 AM, <aa@chromium.org> wrote:

> On 2014/08/08 05:29:24, darin wrote:
>
>> Oh, I see. Perhaps both forms of interception are interesting. For
>> example,
>> if you wanted https://foo/baz to act like a PDF, then you would just fake
>> the network response, but just blanket taking over connections to an URL
>> could certainly be useful and much more efficient and less hacky than
>> generating fake responses. OK. Maybe that is still expressed as a
>> different
>> interface? Or, maybe this just points to a reason why "content handler"
>> might
>> not be the best name for the interface.
>>
>
> Yeah, these are the sorts of issues I've been dancing around: connection,
> navigation, and content handling are all tightly related and interconnected
> concepts. I've gone back and forth about having 1, 2, or 3 interfaces for
> them
> multiple times :). I'm still not happy with what we have, but I thought
> maybe
> I'd make more progress by trying to move on to the next step.
>

Sounds like a good plan to me!
-Darin


>
> https://codereview.chromium.org/423963004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.