Prepending view-source: does not load the source of the userscript.

Prepending view-source: does not load the source of the userscript. Instead,
Chrome attempts to install the userscript.

BUG=Prepending "view-source:" to userscripts' URLs is ignored
TEST=Load userscript with "view-source:" pre-pended. Expected: source code is 
seen. Load userscript w/o "view-source:". Expected: dialog appears to install
userscript.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135271

[chebert, 2012-03-26 23:01:00]

This won't compile.

One of the files was missed in my git-cl upload, so you can hold off on
reviewing for now.

[darin (slow to review), 2012-03-27 03:20:49]

I think there should be a better way to implement this.  Passing the virtual URL
along through all these layers is wrong.  In addition to implementing this as an
"allows downloading" flag, it might also be nice to find a way to reduce layers
that need to be plumbed.

http://codereview.chromium.org/9808029/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/9808029/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/buffered_resource_handler.cc:352: if
(request_->virtual_url().SchemeIs(chrome::kViewSourceScheme))
while we can solve this bug by passing around the virtual URL, i'm not so sure
that we want to do that here.  i think it may make more sense to have a flag
that controls whether or not downloading is permitted.

http://codereview.chromium.org/9808029/diff/2001/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9808029/diff/2001/content/renderer/render_view...
content/renderer/render_view_impl.cc:916:
request.setVirtualURL(params.virtual_url);
we don't want to have to teach WebKit about virtual URLs either.

http://codereview.chromium.org/9808029/diff/2001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/9808029/diff/2001/net/url_request/url_request....
net/url_request/url_request.cc:348: void URLRequest::set_virtual_url(const GURL&
virtual_url) {
we definitely don't want to teach the network stack about virtual URLs.  that
would be a pretty major layering violation.

[darin (slow to review), 2012-03-27 03:23:47]

See also RenderView::willSendRequest.  That gives you a way to tag additional
fields on the WebURLRequest without having to modify WebKit.

http://codereview.chromium.org/9808029/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/9808029/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/buffered_resource_handler.cc:374:
ResourceRequestInfoImpl* info =
each URLRequest created by the ResourceDispatcherHost has an associated
ResourceRequestInfoImpl.  I would expect to find a flag to allow downloading
here.

[chebert, 2012-03-31 21:03:10]

On 2012/03/27 03:23:47, darin wrote:
> See also RenderView::willSendRequest.  That gives you a way to tag additional
> fields on the WebURLRequest without having to modify WebKit.
> 
>
http://codereview.chromium.org/9808029/diff/2001/content/browser/renderer_hos...
> File content/browser/renderer_host/buffered_resource_handler.cc (right):
> 
>
http://codereview.chromium.org/9808029/diff/2001/content/browser/renderer_hos...
> content/browser/renderer_host/buffered_resource_handler.cc:374:
> ResourceRequestInfoImpl* info =
> each URLRequest created by the ResourceDispatcherHost has an associated
> ResourceRequestInfoImpl.  I would expect to find a flag to allow downloading
> here.

Thanks for all of your help! I think the code looks much better now. :)

[chebert, 2012-03-31 21:03:18]

http://codereview.chromium.org/9808029/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/9808029/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/buffered_resource_handler.cc:352: if
(request_->virtual_url().SchemeIs(chrome::kViewSourceScheme))
On 2012/03/27 03:20:49, darin wrote:
> while we can solve this bug by passing around the virtual URL, i'm not so sure
> that we want to do that here.  i think it may make more sense to have a flag
> that controls whether or not downloading is permitted.

Done.

http://codereview.chromium.org/9808029/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/buffered_resource_handler.cc:374:
ResourceRequestInfoImpl* info =
On 2012/03/27 03:23:47, darin wrote:
> each URLRequest created by the ResourceDispatcherHost has an associated
> ResourceRequestInfoImpl.  I would expect to find a flag to allow downloading
> here.

Done.

http://codereview.chromium.org/9808029/diff/2001/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9808029/diff/2001/content/renderer/render_view...
content/renderer/render_view_impl.cc:916:
request.setVirtualURL(params.virtual_url);
On 2012/03/27 03:20:49, darin wrote:
> we don't want to have to teach WebKit about virtual URLs either.

Done.

http://codereview.chromium.org/9808029/diff/2001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/9808029/diff/2001/net/url_request/url_request....
net/url_request/url_request.cc:348: void URLRequest::set_virtual_url(const GURL&
virtual_url) {
On 2012/03/27 03:20:49, darin wrote:
> we definitely don't want to teach the network stack about virtual URLs.  that
> would be a pretty major layering violation.

Done.

http://codereview.chromium.org/9808029/diff/9011/content/renderer/render_view...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/9808029/diff/9011/content/renderer/render_view...
content/renderer/render_view_impl.h:1144: bool pending_allow_download_;
I wasn't sure if it was a good idea to add member data or not.

Since the navigation_params message is lost after OnNavigate(), I wasn't sure
what else I could do.

[chebert, 2012-03-31 21:19:12]

Also note:
If you are running a build of 128744 or later, you might have to comment out the
lines in download_resource_handler.cc like I did in patch 2. (They are currently
in the process of reverting this change. See
http://codereview.chromium.org/9963028/ ).

[darin (slow to review), 2012-04-03 21:56:20]

http://codereview.chromium.org/9808029/diff/9011/content/common/resource_mess...
File content/common/resource_messages.h (right):

http://codereview.chromium.org/9808029/diff/9011/content/common/resource_mess...
content/common/resource_messages.h:62: IPC_STRUCT_MEMBER(bool, allow_download)
any rhyme or reason for inserting this field here rather than just appending it
to the end?

http://codereview.chromium.org/9808029/diff/9011/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/9808029/diff/9011/content/common/view_messages...
content/common/view_messages.h:593: IPC_STRUCT_MEMBER(bool, allow_download)
ditto... why not just append?

http://codereview.chromium.org/9808029/diff/9011/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9808029/diff/9011/content/renderer/render_view...
content/renderer/render_view_impl.cc:890: *pending_navigation_params_.get() =
params;
it seems like pending_allow_download_ should not be necessary given that
pending_navigation_params_ contains the same information.

http://codereview.chromium.org/9808029/diff/9011/content/renderer/render_view...
content/renderer/render_view_impl.cc:2600: DocumentState* document_state) {
here, it seems that you could copy the allow_download flag.

[chebert, 2012-04-03 22:13:22]

http://codereview.chromium.org/9808029/diff/9011/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/9808029/diff/9011/content/common/view_messages...
content/common/view_messages.h:593: IPC_STRUCT_MEMBER(bool, allow_download)
On 2012/04/03 21:56:20, darin wrote:
> ditto... why not just append?

I was thinking that the allow_download field was set specifically based on the
url, so I wanted to put it close to that. Other than that, I'm not really
attached to putting it here, so I will move it to the end.

http://codereview.chromium.org/9808029/diff/9011/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9808029/diff/9011/content/renderer/render_view...
content/renderer/render_view_impl.cc:890: *pending_navigation_params_.get() =
params;
On 2012/04/03 21:56:20, darin wrote:
> it seems like pending_allow_download_ should not be necessary given that
> pending_navigation_params_ contains the same information.

There is a call to pending_navigation_params_.reset() at the end of this method.
(I originally tried to do it this way.)

http://codereview.chromium.org/9808029/diff/9011/content/renderer/render_view...
content/renderer/render_view_impl.cc:2600: DocumentState* document_state) {
On 2012/04/03 21:56:20, darin wrote:
> here, it seems that you could copy the allow_download flag.

Would it be better to put the assignment here than in the OnNavigate() function?

[chebert, 2012-04-07 01:06:44]

Alright. I reviewed the code and what you suggested, and I *think* I understand
what you were saying. At any rate, I think the code is cleaner now. Hopefully it
is up to par.

Also, is the reason for appending the Member to the end of the struct to avoid
confusion for coders who are already familiar with this? (I'm trying to make
better design decisions for future CL's.)

Thanks for your help!

[chebert, 2012-04-24 20:45:29]

brettw:

Would you mind taking a look at this for me?

[darin (slow to review), 2012-04-26 21:54:49]

LGTM w/ nits fixed

https://chromiumcodereview.appspot.com/9808029/diff/20001/content/browser/web...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/9808029/diff/20001/content/browser/web...
content/browser/web_contents/web_contents_impl.cc:218: // We only want to allow
downloading if "view-source:" is not pre-pended.
nit: no need to repeat the code so precisely in this comment.  maybe just
simplify to "// Avoid downloading when in view-source mode."

nit: use entry.IsViewSourceMode() here instead.

[commit-bot: I haz the power, 2012-04-27 00:51:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hebert.christopherj@chromium.org/98080...

[commit-bot: I haz the power, 2012-04-27 01:47:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hebert.christopherj@chromium.org/98080...

[commit-bot: I haz the power, 2012-04-27 23:59:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hebert.christopherj@chromium.org/98080...

[commit-bot: I haz the power, 2012-04-28 01:11:02]

Try job failure for 9808029-39001 (retry) on linux_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-04-28 01:12:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hebert.christopherj@chromium.org/98080...

[commit-bot: I haz the power, 2012-04-28 01:14:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hebert.christopherj@chromium.org/98080...

[commit-bot: I haz the power, 2012-04-28 02:13:23]

Try job failure for 9808029-39001 (retry) on linux_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-05-04 00:02:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hebert.christopherj@chromium.org/98080...

[commit-bot: I haz the power, 2012-05-04 00:18:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hebert.christopherj@chromium.org/98080...

[commit-bot: I haz the power, 2012-05-04 01:47:13]

Change committed as 135271