78810 - CreateFixupURI does not handle view-source:www.cnn.com correctly

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

In NS 4.x, view-source:www.cnn.com will show the source of http://www.cnn.com

In Mozilla it will not.  This is because CreateFixupURI will convert the above
to http://view-source:www.cnn.com ... it should convert it to
view-source:http://www.cnn.com

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: patch to fix this

Comment 2

[Boris Zbarsky [:bzbarsky]]

reviews?

Comment 3

[timeless]

about:view-source:about:view-source:http://bugzilla.mozilla.org/showattachment.
cgi?attach_id=33118
[mind you it's an eclectic case and doesn't behave particularly well]

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm... good point....

We need to be smarter about matching.  Better patch coming up.  :)

Comment 5

[Boris Zbarsky [:bzbarsky]]

hmm... Thinking a bit more.

We need to remove the view-source: part before calling NS_NewURI; otherwise
NS_NewURI will return no error.  On the other hand, if we do remove it,
NS_NewURI will return no error for things like
view-source:http://www.mozilla.org and we will just load http://www.mozilla.org
without viewsourcing it.  So NS_NewURI could use some changes, maybe.

So this actually needs some thought.  My original approach is really no good. 
Doron, it may make sense to workaround this bug in view source for the time
being.

Comment 6

[Doron Rosenberg (IBM)]

hmm. I can override it by calling it up from the js which you already coded ;)
for the time being.

Comment 7

[Judson Valeski]

-> 0.9.3

Comment 8

[Adam Lock]

Retargetting

Comment 9

[Adam Lock]

Attachment: New patch for this, some notes for follow

Comment 10

[Adam Lock]

I've submitted a new patch to fix view-source.

This patch works by lopping off the "view-source:" from the front of the string
and calling the nsDefaultURIFixup::CreateFixupURI again before rebuilding the
result. This ensures stuff like view-source:www.cnn.com is correctly fixed up.

Reviews please?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 54678 [details] [diff] [review]
New patch for this, some notes for follow

>+    	nsAutoString uriSansViewSource;

weird indentation...

>+        nsCOMPtr<nsIURI> uri;
>+        uriString.Mid(uriSansViewSource, 12, uriString.Length() - 12);
>+        nsresult rv = CreateFixupURI(uriSansViewSource.get(), getter_AddRefs(uri));

Would something like:

nsCOMPtr<nsIURI> uri;
nsresult rv = 
  CreateFixupURI(PromiseFlatString(Substring(uriString, 
                                             12, 
                                             uriString.Length() - 12)).get(),
                 getter_AddRefs(uri));

not be better?  That way you don't need the uriSansViewSource temporary variable
an you avoid allocating, copying, and the like.

>+        uriString.AssignWithConversion("view-source:");

use |uriString.Assign(NS_LITERAL_STRING("view-source:"));|

>+        uriString.AppendWithConversion(spec.get());

I think a |.Append(NS_ConvertASCIItoUCS2(spec))| may be better here.

Other than that, looks good.

Comment 12

[Adam Lock]

Attachment: Patch with revised string stuff. Submitting this for sr

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 54871 [details] [diff] [review]
Patch with revised string stuff. Submitting this for sr

r=bzbarsky

Comment 14

[rpotts (gone)]

do we need to worry about these 'malformed' view-source URIs hitting the keyword 
server?

it seems like we want to limit the 'fixup' to prepending 'http://'  i'm not even 
sure if we want to tack on the 'www' and 'com' ...

-- rick

Comment 15

[Adam Lock]

Rick, the code for tacking on www. & .com is still in nsWebShell and nothing to
do with the nsDefaultURIFixup.

I understand what you're saying about the keyword service so I'll try and work
out a patch that disables it cleanly for view-source.

Comment 16

[rpotts (gone)]

yeah...  at some point i want to remove the remaining stuff from nsWebShell and
move it all into the URIFixup component...

so i think that we need to specify a way that we can do 'limited' fixups for
this class of URI string...

-- rick

Comment 17

[Adam Lock]

Attachment: Patch disable keyword service via a flag (notes to follow)

Comment 18

[Adam Lock]

This patch adds a flags parameter to the nsIURIFixup::CreateFixupURI method.

If this method is called with FIXUP_FLAG_NO_KEYWORD it will skip the keyword
lookup on the URI. I ensure this flag is set when I call the method during
view-source: fixup.

Ultimately this interface should also to the post-fixup to tack on the www. &
.com but that's another issue altogether.

r/sr= please?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 56092 [details] [diff] [review]
Patch disable keyword service via a flag (notes to follow)

r=bzbarsky (good thing we have no calls to
createFixupURI anywhere in the chrome.  :) )

Comment 20

[Adam Lock]

Attachment: Flipped the default for keyword lookup. Keyword lookup must now be enabled by a flag

Comment 21

[Christopher Blizzard (:blizzard)]

Comment on attachment 56736 [details] [diff] [review]
Flipped the default for keyword lookup. Keyword lookup must now be enabled by a flag

r/sr=blizzard

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 56736 [details] [diff] [review]
Flipped the default for keyword lookup. Keyword lookup must now be enabled by a flag

look good
r=bzbarsky

Comment 23

[Adam Lock]

Fix is in.