Mojo content handler shebang support

Mojo content handler shebang support

If an application's content begins with #!mojo:content-handler-url, use the specified content-handler and skip the whole "shebang".


BUG=426899
R=aa@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/eb1a5f99c88eddc67c1951059c18cbc2363a434c

[hansmuller, 2014-10-30 23:50:58]

hansmuller@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[hansmuller, 2014-10-31 00:06:14]

PTAL.

[viettrungluu, 2014-10-31 23:30:23]

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils.cc
File mojo/common/data_pipe_utils.cc (right):

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:70: // that there's no point in trying again.
Please add a TODO for yourself, since this is pretty hacky.

(Maybe also file a bug, in which case link to the bug in your TODO.)

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:71: 
nit: no blank line here

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:72: class PeekSleeper {
For some reason, having this helper class seems like overkill to me.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:73: public:
indent one space

(consider running clang-format)

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:74: PeekSleeper(MojoTimeTicks deadline)
explicit

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:77: max_sleep_micros_(1000 * 10) {
This is a constant, so it should be declared as such.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:96: private:
indent one space

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:102: enum PeekStatus {kSuccess, kFail,
kKeepReading};
space after {, before }

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:103: typedef PeekStatus (*PeekFunc)(const void*,
uint32_t, size_t, std::string*);
I find the fact that the size_t argument has varying meaning rather disturbing.

Possibly, you should use base::Callback() and then base::Bind() (partially
binding the size_t argument) instead, though admittedly that's more heavyweight.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:108: 
no blank line

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:114: CHECK(value);
Probably a DCHECK is sufficient.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:122: while(result == MOJO_RESULT_OK) {
Maybe write this as a do-while instead?

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:126: BeginReadDataRaw(source, &buffer,
&num_bytes, MOJO_READ_DATA_FLAG_NONE);
So if this is called not at the beginning of the buffer, it may never get the
data that you want, even if it's available. :/

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:130: if (EndReadDataRaw(source, 0) !=
MOJO_RESULT_OK)
Probably just do a CHECK_NE.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:139: 
no blank line

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:155: std::string s(p, buffer_num_bytes);
It seems quite painful to build an entire string just so you can do s.find() on
it.

Possibly you should use StringPiece, for maybe you should just write a loop:

for (size_t i = 0; i < std::min(buffer_num_bytes, max_line_length); i++) {
  if (p[i] == '\n') {
    *line = std::string(p, n);
    return PeekStatus::kSuccess;
  }
}
return (i > max_line_length) ? PeekStatus::kFail : PeekStatus:kKeepReading;

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:158: *line = s.substr(0, n);
This won't include the '\n', will it? I find that slightly odd.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:167: uint32 buffer_num_bytes,
fix indentation

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:180: 
nit: one fewer blank line

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils.h
File mojo/common/data_pipe_utils.h (right):

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.h:38: // Return true and the first newline
terminated line from source. Return false
nit:
s/Return/Returns/g

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.h:47: // Return true and the first bytes_length
bytes from source. Return false
...

[hansmuller, 2014-11-01 00:06:05]

I've made all of the suggested changes save one. This is just a checkpoint.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils.cc
File mojo/common/data_pipe_utils.cc (right):

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:70: // that there's no point in trying again.
On 2014/10/31 23:30:23, viettrungluu wrote:
> Please add a TODO for yourself, since this is pretty hacky.
> 
> (Maybe also file a bug, in which case link to the bug in your TODO.)

Done.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:71: 
On 2014/10/31 23:30:22, viettrungluu wrote:
> nit: no blank line here

Done.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:72: class PeekSleeper {
On 2014/10/31 23:30:22, viettrungluu wrote:
> For some reason, having this helper class seems like overkill to me.

I initially wrote the code without it. Moving the sleep code to a helper class
made BlockingPeekHelper much easier to read.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:73: public:
On 2014/10/31 23:30:23, viettrungluu wrote:
> indent one space
> 
> (consider running clang-format)

Done.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:74: PeekSleeper(MojoTimeTicks deadline)
On 2014/10/31 23:30:23, viettrungluu wrote:
> explicit

Done.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:77: max_sleep_micros_(1000 * 10) {
On 2014/10/31 23:30:22, viettrungluu wrote:
> This is a constant, so it should be declared as such.

Done.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:96: private:
On 2014/10/31 23:30:23, viettrungluu wrote:
> indent one space

Done.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:102: enum PeekStatus {kSuccess, kFail,
kKeepReading};
On 2014/10/31 23:30:23, viettrungluu wrote:
> space after {, before }

Done.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:108: 
On 2014/10/31 23:30:23, viettrungluu wrote:
> no blank line

Done.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:114: CHECK(value);
On 2014/10/31 23:30:23, viettrungluu wrote:
> Probably a DCHECK is sufficient.

Done.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:139: 
On 2014/10/31 23:30:23, viettrungluu wrote:
> no blank line

Done.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:155: std::string s(p, buffer_num_bytes);
On 2014/10/31 23:30:23, viettrungluu wrote:
> It seems quite painful to build an entire string just so you can do s.find()
on
> it.
> 
> Possibly you should use StringPiece, for maybe you should just write a loop:
> 
> for (size_t i = 0; i < std::min(buffer_num_bytes, max_line_length); i++) {
>   if (p[i] == '\n') {
>     *line = std::string(p, n);
>     return PeekStatus::kSuccess;
>   }
> }
> return (i > max_line_length) ? PeekStatus::kFail : PeekStatus:kKeepReading;

Since you went to the trouble of writing a loop, I've just used it.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:158: *line = s.substr(0, n);
On 2014/10/31 23:30:23, viettrungluu wrote:
> This won't include the '\n', will it? I find that slightly odd.

Yes he line return value doesn't include the terminating newline. It didn't seem
odd to me but I don't have a strong feeling about it. I will include it.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:167: uint32 buffer_num_bytes,
On 2014/10/31 23:30:23, viettrungluu wrote:
> fix indentation

Done.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:180: 
On 2014/10/31 23:30:23, viettrungluu wrote:
> nit: one fewer blank line

Done.

[hansmuller, 2014-11-03 16:34:55]

I've made all of the suggested changes, including refactoring
BlockingPeekHelper. PTAL.

[viettrungluu, 2014-11-03 18:36:56]

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils.cc
File mojo/common/data_pipe_utils.cc (right):

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:122: while(result == MOJO_RESULT_OK) {
On 2014/10/31 23:30:23, viettrungluu wrote:
> Maybe write this as a do-while instead?

You missed this.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:126: BeginReadDataRaw(source, &buffer,
&num_bytes, MOJO_READ_DATA_FLAG_NONE);
On 2014/10/31 23:30:23, viettrungluu wrote:
> So if this is called not at the beginning of the buffer, it may never get the
> data that you want, even if it's available. :/

And this....

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:130: if (EndReadDataRaw(source, 0) !=
MOJO_RESULT_OK)
On 2014/10/31 23:30:23, viettrungluu wrote:
> Probably just do a CHECK_NE.

...

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:158: *line = s.substr(0, n);
On 2014/11/01 00:06:05, hansmuller wrote:
> On 2014/10/31 23:30:23, viettrungluu wrote:
> > This won't include the '\n', will it? I find that slightly odd.
> 
> Yes he line return value doesn't include the terminating newline. It didn't
seem
> odd to me but I don't have a strong feeling about it. I will include it.

I guess for "peek", it's less odd, but you should have it to be parallel to a
hypothetical "read" (which would either include the \n or at least dispose of
it).

https://codereview.chromium.org/696543003/diff/60001/mojo/common/data_pipe_ut...
File mojo/common/data_pipe_utils.cc (right):

https://codereview.chromium.org/696543003/diff/60001/mojo/common/data_pipe_ut...
mojo/common/data_pipe_utils.cc:99: const MojoTimeTicks max_sleep_micros_; //
microseconds
By "const", I meant a static const, named "kMaxSleepMicros".

(deadline_ can be a non-static const.)

https://codereview.chromium.org/696543003/diff/60001/mojo/common/data_pipe_ut...
mojo/common/data_pipe_utils.cc:100: };
DISALLOW_COPY_AND_ASSIGN

[hansmuller, 2014-11-03 23:24:24]

hansmuller@chromium.org changed reviewers:
+ aa@chromium.org

[hansmuller, 2014-11-03 23:24:25]

I moved the data pipe peek support from the mojo:common to mojo:shell, since the
code isn't intended to be general purpose, for now it's just for finding the
content-handler shebang.

DynamicApplicationLoader::OnComplete() now uses the shebang content-handler, if
it can find one.

Aaron - please review the DynamicApplicationLoader changes.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils.cc
File mojo/common/data_pipe_utils.cc (right):

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:103: typedef PeekStatus (*PeekFunc)(const void*,
uint32_t, size_t, std::string*);
On 2014/10/31 23:30:23, viettrungluu wrote:
> I find the fact that the size_t argument has varying meaning rather
disturbing.
> 
> Possibly, you should use base::Callback() and then base::Bind() (partially
> binding the size_t argument) instead, though admittedly that's more
heavyweight.

Done.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:122: while(result == MOJO_RESULT_OK) {
On 2014/11/03 18:36:56, viettrungluu wrote:
> On 2014/10/31 23:30:23, viettrungluu wrote:
> > Maybe write this as a do-while instead?
> 
> You missed this.

Done.

https://codereview.chromium.org/696543003/diff/1/mojo/common/data_pipe_utils....
mojo/common/data_pipe_utils.cc:130: if (EndReadDataRaw(source, 0) !=
MOJO_RESULT_OK)
On 2014/11/03 18:36:56, viettrungluu wrote:
> On 2014/10/31 23:30:23, viettrungluu wrote:
> > Probably just do a CHECK_NE.
> 
> ...

Done.

https://codereview.chromium.org/696543003/diff/60001/mojo/common/data_pipe_ut...
File mojo/common/data_pipe_utils.cc (right):

https://codereview.chromium.org/696543003/diff/60001/mojo/common/data_pipe_ut...
mojo/common/data_pipe_utils.cc:99: const MojoTimeTicks max_sleep_micros_; //
microseconds
On 2014/11/03 18:36:56, viettrungluu wrote:
> By "const", I meant a static const, named "kMaxSleepMicros".
> 
> (deadline_ can be a non-static const.)

Done.

https://codereview.chromium.org/696543003/diff/60001/mojo/common/data_pipe_ut...
mojo/common/data_pipe_utils.cc:100: };
On 2014/11/03 18:36:56, viettrungluu wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Aaron Boodman, 2014-11-03 23:50:37]

https://codereview.chromium.org/696543003/diff/80001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/696543003/diff/80001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:166: const std::string
kMojoMagic("#!mojo:");
No real reason to make this a string, const char* works.

https://codereview.chromium.org/696543003/diff/80001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:167: const MojoDeadline kPeekTimeout =
1000 * 5; // 5ms
urgh. Can you put a TODO on this to fix once peeking is supported properly?

https://codereview.chromium.org/696543003/diff/80001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:185: void OnLoadComplete(URLResponsePtr
response) {
Note that this is about to move around some:

https://codereview.chromium.org/694303002/

https://codereview.chromium.org/696543003/diff/80001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:199: // TBD(hansmuller):
content-handlers should skip the shebang themselves.
I'm not sure I agree with this statement.

How about we just assume it is the shell's responsibility for now, and change it
if/when necessary. Unless you have some specific reason you think apps should do
it?

https://codereview.chromium.org/696543003/diff/80001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:220: LOG(WARNING) << "Failed to find
content handler for " << response->url
Let's just remove this. I don't think it warrants a warning, and the presubmit
script will keep whining if we leave it as INFO.

[hansmuller, 2014-11-04 00:26:03]

Thanks for looking at this.  I've made the suggested changes.

https://codereview.chromium.org/696543003/diff/80001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/696543003/diff/80001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:166: const std::string
kMojoMagic("#!mojo:");
On 2014/11/03 23:50:36, Aaron Boodman wrote:
> No real reason to make this a string, const char* works.

Done.

https://codereview.chromium.org/696543003/diff/80001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:167: const MojoDeadline kPeekTimeout =
1000 * 5; // 5ms
On 2014/11/03 23:50:36, Aaron Boodman wrote:
> urgh. Can you put a TODO on this to fix once peeking is supported properly?

Done. I've defeated the timeout for now, it's MOJO_DEADLINE_INDEFINITE.

https://codereview.chromium.org/696543003/diff/80001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:185: void OnLoadComplete(URLResponsePtr
response) {
On 2014/11/03 23:50:36, Aaron Boodman wrote:
> Note that this is about to move around some:
> 
> https://codereview.chromium.org/694303002/

It looks like the shebang code will merge with qsr's CL without too much
trouble.

https://codereview.chromium.org/696543003/diff/80001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:199: // TBD(hansmuller):
content-handlers should skip the shebang themselves.
On 2014/11/03 23:50:36, Aaron Boodman wrote:
> I'm not sure I agree with this statement.
> 
> How about we just assume it is the shell's responsibility for now, and change
it
> if/when necessary. Unless you have some specific reason you think apps should
do
> it?

I thought that content-handlers would check the shebang as sanity check. The
shebang is optional so that probably doesn't make sense. I will remove the
comment.

https://codereview.chromium.org/696543003/diff/80001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:220: LOG(WARNING) << "Failed to find
content handler for " << response->url
On 2014/11/03 23:50:36, Aaron Boodman wrote:
> Let's just remove this. I don't think it warrants a warning, and the presubmit
> script will keep whining if we leave it as INFO.

Done.

[Aaron Boodman, 2014-11-04 00:27:21]

lgtm

[hansmuller, 2014-11-04 00:44:06]

The CQ bit was checked by hansmuller@chromium.org

[hansmuller, 2014-11-04 00:44:09]

The CQ bit was unchecked by hansmuller@chromium.org

[hansmuller, 2014-11-04 00:59:09]

Committed patchset #6 (id:100001) manually as
eb1a5f99c88eddc67c1951059c18cbc2363a434c (presubmit successful).