Mandoline: Support about:blank URL.

Mandoline: Support about:blank URL.

This CL adds a Fetcher implementation to provide responses for about: URLs.

BUG=504558
Committed: https://crrev.com/3e4aaaa45c528d4ab0670331a6c0ebfc4f3ab8e6
Cr-Commit-Position: refs/heads/master@{#341978}

[yzshen1, 2015-08-04 19:18:50]

yzshen@chromium.org changed reviewers:
+ sky@chromium.org

[yzshen1, 2015-08-04 19:18:51]

Hi, Scott.
Would you please take a look? Thanks!

[sky, 2015-08-04 20:04:56]

It would be nice to add test coverage too.

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.cc
File mojo/runner/about_fetcher.cc (right):

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.c...
mojo/runner/about_fetcher.cc:73: if (skip != 0 && response_->body.is_valid()) {
Don't you need to make sure response_ is valid? AFAICT you never set body, is it
ever valid?

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.c...
mojo/runner/about_fetcher.cc:91: return response_->mime_type;
Check response?

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.c...
mojo/runner/about_fetcher.cc:99: return
shell::BlockingPeekLine(response_->body.get(), line, kMaxShebangLength,
Similar comments to that of AsURLResponse.

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.h
File mojo/runner/about_fetcher.h (right):

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.h...
mojo/runner/about_fetcher.h:20: class AboutFetcher : public shell::Fetcher {
The rest of the fetchers are in mojo/shell. Is there a reason this should be in
a different place?

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.h...
mojo/runner/about_fetcher.h:32: // scuccess, the ownership of this object is
passed to the loader callback;
scuccess->success

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.h...
mojo/runner/about_fetcher.h:33: // otherwise, the callback is runned with a
nullptr and this object is
runned->run

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/context.cc
File mojo/runner/context.cc (right):

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/context.cc#newc...
mojo/runner/context.cc:342: new AboutFetcher(url, loader_callback);
Document AboutFetcher deletes itself. Also, do we always want this behavior? By
that I mean we definitely want to support about:blank, but it's not clear that
about:blank should always be text/html.

[yzshen1, 2015-08-05 16:11:08]

Thanks Scott!
Please take another look.

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.cc
File mojo/runner/about_fetcher.cc (right):

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.c...
mojo/runner/about_fetcher.cc:73: if (skip != 0 && response_->body.is_valid()) {
On 2015/08/04 20:04:55, sky wrote:
> Don't you need to make sure response_ is valid?
If |response_| is not valid, this object won't be passed to |loader_callback_|,
according to the Fetcher interface definition.
So nobody should access this method in that case.


> AFAICT you never set body, is it ever valid?
You are right. Body is never set for about:blank.
I was thinking we will probably support more about: URLs than just about:blank
in this future, so this if-block could be useful.

What do you think? I could remove the whole if-block if that makes more sense.
Thanks!

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.c...
mojo/runner/about_fetcher.cc:91: return response_->mime_type;
On 2015/08/04 20:04:55, sky wrote:
> Check response?

Please see my previous comment.

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.c...
mojo/runner/about_fetcher.cc:99: return
shell::BlockingPeekLine(response_->body.get(), line, kMaxShebangLength,
On 2015/08/04 20:04:55, sky wrote:
> Similar comments to that of AsURLResponse.

Please see my previous comment.

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.h
File mojo/runner/about_fetcher.h (right):

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.h...
mojo/runner/about_fetcher.h:20: class AboutFetcher : public shell::Fetcher {
On 2015/08/04 20:04:56, sky wrote:
> The rest of the fetchers are in mojo/shell. Is there a reason this should be
in
> a different place?

Because all the other fetchers are used by mojo/shell/application_manager, while
this one is used by mojo/runner/context

My understanding is that an implementation of an interface lives at where it is
used, instead of where the interface is defined.

What do you think? Thanks!

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.h...
mojo/runner/about_fetcher.h:32: // scuccess, the ownership of this object is
passed to the loader callback;
On 2015/08/04 20:04:56, sky wrote:
> scuccess->success

Done.

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.h...
mojo/runner/about_fetcher.h:33: // otherwise, the callback is runned with a
nullptr and this object is
On 2015/08/04 20:04:55, sky wrote:
> runned->run

Done. Sorry.

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/context.cc
File mojo/runner/context.cc (right):

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/context.cc#newc...
mojo/runner/context.cc:342: new AboutFetcher(url, loader_callback);
On 2015/08/04 20:04:56, sky wrote:
> Document AboutFetcher deletes itself.
Done.

> Also, do we always want this behavior? By
> that I mean we definitely want to support about:blank, but it's not clear that
> about:blank should always be text/html.
In what cases about:blank shouldn't be text/html?
I am not sure I understand. Do you mean we need more than one way to handle
about:blank?

Or do you mean we need different behavior for other "about:" URLs? That is
possible. We could have different MIME types or register different applications
for different "about:" URLs.

[sky, 2015-08-05 17:35:49]

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.cc
File mojo/runner/about_fetcher.cc (right):

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.c...
mojo/runner/about_fetcher.cc:73: if (skip != 0 && response_->body.is_valid()) {
On 2015/08/05 16:11:08, yzshen1 wrote:
> On 2015/08/04 20:04:55, sky wrote:
> > Don't you need to make sure response_ is valid?
> If |response_| is not valid, this object won't be passed to
|loader_callback_|,
> according to the Fetcher interface definition.
> So nobody should access this method in that case.

How about a DCHECK then.

> 
> 
> > AFAICT you never set body, is it ever valid?
> You are right. Body is never set for about:blank.
> I was thinking we will probably support more about: URLs than just about:blank
> in this future, so this if-block could be useful.
> 
> What do you think? I could remove the whole if-block if that makes more sense.

I would rather you wait until body is set. Until then this code is useless.

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.h
File mojo/runner/about_fetcher.h (right):

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.h...
mojo/runner/about_fetcher.h:20: class AboutFetcher : public shell::Fetcher {
On 2015/08/05 16:11:08, yzshen1 wrote:
> On 2015/08/04 20:04:56, sky wrote:
> > The rest of the fetchers are in mojo/shell. Is there a reason this should be
> in
> > a different place?
> 
> Because all the other fetchers are used by mojo/shell/application_manager,
while
> this one is used by mojo/runner/context
> 
> My understanding is that an implementation of an interface lives at where it
is
> used, instead of where the interface is defined.
> 
> What do you think? Thanks!

Ok, makes sense.

https://codereview.chromium.org/1260813007/diff/20001/mojo/runner/about_fetch...
File mojo/runner/about_fetcher.h (right):

https://codereview.chromium.org/1260813007/diff/20001/mojo/runner/about_fetch...
mojo/runner/about_fetcher.h:26: ~AboutFetcher() override;
Can the destructor be moved to private?

[sky, 2015-08-05 17:37:07]

On 2015/08/05 16:11:08, yzshen1 wrote:
> Thanks Scott!
> Please take another look.
> 
> https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.cc
> File mojo/runner/about_fetcher.cc (right):
> 
>
https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.c...
> mojo/runner/about_fetcher.cc:73: if (skip != 0 && response_->body.is_valid())
{
> On 2015/08/04 20:04:55, sky wrote:
> > Don't you need to make sure response_ is valid?
> If |response_| is not valid, this object won't be passed to
|loader_callback_|,
> according to the Fetcher interface definition.
> So nobody should access this method in that case.
> 
> 
> > AFAICT you never set body, is it ever valid?
> You are right. Body is never set for about:blank.
> I was thinking we will probably support more about: URLs than just about:blank
> in this future, so this if-block could be useful.
> 
> What do you think? I could remove the whole if-block if that makes more sense.
> Thanks!
> 
>
https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.c...
> mojo/runner/about_fetcher.cc:91: return response_->mime_type;
> On 2015/08/04 20:04:55, sky wrote:
> > Check response?
> 
> Please see my previous comment.
> 
>
https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.c...
> mojo/runner/about_fetcher.cc:99: return
> shell::BlockingPeekLine(response_->body.get(), line, kMaxShebangLength,
> On 2015/08/04 20:04:55, sky wrote:
> > Similar comments to that of AsURLResponse.
> 
> Please see my previous comment.
> 
> https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.h
> File mojo/runner/about_fetcher.h (right):
> 
>
https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.h...
> mojo/runner/about_fetcher.h:20: class AboutFetcher : public shell::Fetcher {
> On 2015/08/04 20:04:56, sky wrote:
> > The rest of the fetchers are in mojo/shell. Is there a reason this should be
> in
> > a different place?
> 
> Because all the other fetchers are used by mojo/shell/application_manager,
while
> this one is used by mojo/runner/context
> 
> My understanding is that an implementation of an interface lives at where it
is
> used, instead of where the interface is defined.
> 
> What do you think? Thanks!
> 
>
https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.h...
> mojo/runner/about_fetcher.h:32: // scuccess, the ownership of this object is
> passed to the loader callback;
> On 2015/08/04 20:04:56, sky wrote:
> > scuccess->success
> 
> Done.
> 
>
https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.h...
> mojo/runner/about_fetcher.h:33: // otherwise, the callback is runned with a
> nullptr and this object is
> On 2015/08/04 20:04:55, sky wrote:
> > runned->run
> 
> Done. Sorry.
> 
> https://codereview.chromium.org/1260813007/diff/1/mojo/runner/context.cc
> File mojo/runner/context.cc (right):
> 
>
https://codereview.chromium.org/1260813007/diff/1/mojo/runner/context.cc#newc...
> mojo/runner/context.cc:342: new AboutFetcher(url, loader_callback);
> On 2015/08/04 20:04:56, sky wrote:
> > Document AboutFetcher deletes itself.
> Done.
> 
> > Also, do we always want this behavior? By
> > that I mean we definitely want to support about:blank, but it's not clear
that
> > about:blank should always be text/html.
> In what cases about:blank shouldn't be text/html?
> I am not sure I understand. Do you mean we need more than one way to handle
> about:blank?

This patch is effectively a way to make about:blank be handled by html_viewer.
Another option is a trivial mojo app specifically for about:blank that renders
nothing. That's way I said I'm not sure we would always want this behavior. I
suppose it's fine for now.

> 
> Or do you mean we need different behavior for other "about:" URLs? That is
> possible. We could have different MIME types or register different
applications
> for different "about:" URLs.

[yzshen1, 2015-08-05 18:00:27]

Thanks Scott.

Please take another look.

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.cc
File mojo/runner/about_fetcher.cc (right):

https://codereview.chromium.org/1260813007/diff/1/mojo/runner/about_fetcher.c...
mojo/runner/about_fetcher.cc:73: if (skip != 0 && response_->body.is_valid()) {
On 2015/08/05 17:35:49, sky wrote:
> On 2015/08/05 16:11:08, yzshen1 wrote:
> > On 2015/08/04 20:04:55, sky wrote:
> > > Don't you need to make sure response_ is valid?
> > If |response_| is not valid, this object won't be passed to
> |loader_callback_|,
> > according to the Fetcher interface definition.
> > So nobody should access this method in that case.
> 
> How about a DCHECK then.
> 
> > 
> > 
> > > AFAICT you never set body, is it ever valid?
> > You are right. Body is never set for about:blank.
> > I was thinking we will probably support more about: URLs than just
about:blank
> > in this future, so this if-block could be useful.
> > 
> > What do you think? I could remove the whole if-block if that makes more
sense.
> 
> I would rather you wait until body is set. Until then this code is useless.

Done.

https://codereview.chromium.org/1260813007/diff/20001/mojo/runner/about_fetch...
File mojo/runner/about_fetcher.h (right):

https://codereview.chromium.org/1260813007/diff/20001/mojo/runner/about_fetch...
mojo/runner/about_fetcher.h:26: ~AboutFetcher() override;
On 2015/08/05 17:35:49, sky wrote:
> Can the destructor be moved to private?

Done.
I also moved the constructor and added a public static method Start(). I think
it is more clear that way.

[sky, 2015-08-05 18:12:34]

LGTM

[yzshen1, 2015-08-05 18:28:12]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2015-08-05 18:28:12]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1260813007/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-08-05 18:29:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1260813007/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1260813007/60001

[commit-bot: I haz the power, 2015-08-05 19:17:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-05 19:17:05]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yzshen1, 2015-08-05 20:27:44]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2015-08-05 20:27:45]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1260813007/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-08-05 20:29:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1260813007/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1260813007/80001

[commit-bot: I haz the power, 2015-08-05 21:27:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-05 21:27:58]

Patchset 5 (id:??) landed as
https://crrev.com/3e4aaaa45c528d4ab0670331a6c0ebfc4f3ab8e6
Cr-Commit-Position: refs/heads/master@{#341978}