306396 - Don't show wyciwyg URLs in location bar

Status: VERIFIED FIXED

(Camino Graveyard :: Location Bar & Autocomplete, defect)

Description

[Samuel Sidler (old account; do not CC)]

(The URL for this bug is a testcase in another one which will loop indefinitely.)

Sometimes Camino shows wyciwyg URLs. Like FF/SM, we shouldn't show them at all
to the user.

That is all.

Comment 1

[Mark Mentovai]

Firefox (and Seamonkey, too, but elsewhere) fixes up URLs with nsIURIFixup:

http://lxr.mozilla.org/mozilla/source/browser/base/content/browser.js#3442
http://lxr.mozilla.org/mozilla/source/docshell/base/nsDefaultURIFixup.cpp#71

Comment 2

[Samuel Sidler (old account; do not CC)]

This isn't important for 1.0... targeting for 1.1.

Comment 3

[Stuart Morgan]

Attachment: do-it-ourselves fix

Pulling in another component just to do a simple string manipulation and get a side-effect I'm not sure we want (hiding user:password in URLs) didn't seem especially compelling, so this reimplements a very small wheel.

Comment 4

[Stuart Morgan]

Pink/Mark, does this seem reasonable, or is jumping through the nsIURIFixup hoops substantially better?

Comment 5

[Stuart Morgan]

Comment on attachment 241911 [details] [diff] [review]
do-it-ourselves fix

Not what we want.

Comment 6

[Stuart Morgan]

Attachment: nsIURIFixup version

Mmm, code re-use.

Comment 7

[Stuart Morgan]

I think we'll actually want to set browser.fixup.hide_user_pass to false in camino.js though; having that on and using fixup could result in some weird behaviors (like reloading and bookmarking of user:pass-ified URLs not working).

Comment 8

[Stuart Morgan]

Attachment: adds pref disabling

Comment 9

[Håkan Waara]

Comment on attachment 242071 [details] [diff] [review]
adds pref disabling

Are you sure that we want to do the fixup at this level, before giving it to all the observers, instead of *in* one of the front-end observers?

Comment 10

[Stuart Morgan]

(In reply to comment #9)
> (From update of attachment 242071 [details] [diff] [review] [edit])
> Are you sure that we want to do the fixup at this level, before giving it to
> all the observers, instead of *in* one of the front-end observers?

Not 100% sure, but I did consider it.  My feeling is that wyciwyg URLs are purely a Gecko internal construct that higher levels shouldn't have to know about, so the best thing to do is get rid of them at the lowest level we can.  Can you think of a reason anything in the chrome layer should know about them?

Comment 11

[Håkan Waara]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 242071 [details] [diff] [review] [edit] [edit])
> > Are you sure that we want to do the fixup at this level, before giving it to
> > all the observers, instead of *in* one of the front-end observers?
> 
> Not 100% sure, but I did consider it.  My feeling is that wyciwyg URLs are
> purely a Gecko internal construct that higher levels shouldn't have to know
> about, so the best thing to do is get rid of them at the lowest level we can. 
> Can you think of a reason anything in the chrome layer should know about them?

I don't know offhand, but I'm thinking like this: Is history a browser listener? Then it will depend on the URIs we give it, and this patch would break some URIs. Download manager?

I'd check skim the classes that implement the informal browserlistener protocol 
to see what they do with their URIs.

Comment 12

[Stuart Morgan]

Comment on attachment 242071 [details] [diff] [review]
adds pref disabling

I just realized that this actually isn't comprehensive enough, since requesting the current URI (as is done by history, bookmarks, etc.) all follows a non-fixed-up path, so bookmarking a wyciwyg url suddenly makes the full url visible, which isn't good.

Bookmarks clearly should never be bookmarking a wyciwyg URL, since that's not necessarily a stable location.  I'm not sure about history; I can look into it more.  However, since this testcase breaks back, I'm not sure how obvious the right thing will be.

Comment 13

[Håkan Waara]

If only the location bar was a textfield of text that we didn't rely on everywhere (cf getCurrentURI), then you could simply do the fixup as the last step...

Comment 14

[Stuart Morgan]

(In reply to comment #13)
> If only the location bar was a textfield of text that we didn't rely on
> everywhere (cf getCurrentURI), then you could simply do the fixup as the last
> step...

That pretty much is the case, which is why my current patch isn't good enough.  We don't *want* this to happen only for the URL bar, but also visible history, bookmark info, etc.  Anywhere the user can see the URL, it should be fixed.

The question is whether to do that at a minimal number of choke points, or just before every single time a URL becomes visible, which will turn into an error-prone whack-a-mole game.  Unless we can find specific places Camino really needs it, I want to do the former.

Comment 15

[Håkan Waara]

(In reply to comment #14)
> (In reply to comment #13)
> > If only the location bar was a textfield of text that we didn't rely on
> > everywhere (cf getCurrentURI), then you could simply do the fixup as the last
> > step...
> 
> That pretty much is the case, which is why my current patch isn't good enough. 
> We don't *want* this to happen only for the URL bar, but also visible history,
> bookmark info, etc.  Anywhere the user can see the URL, it should be fixed.
> 
> The question is whether to do that at a minimal number of choke points, or just
> before every single time a URL becomes visible, which will turn into an
> error-prone whack-a-mole game.  Unless we can find specific places Camino
> really needs it, I want to do the former.

I think former sounds like the best plan. Maybe you could define some kind of clear distinction between the real URI and the fixed-up URI in our code, possibly by renaming getCurrentURI and creating a sister method to it that will return a cleaned-up URI? Just an idea.

Comment 16

[Stuart Morgan]

Attachment: more comprehensive hiding

I looked at all the places we GetSpec, and fixed up almost all the ones that lead to visible strings.  I didn't make a new method because there's nothing in the Camino source using getCurrentURI that should have to know about wyciwyg.

The 'almost' is history--the one part of Camino that has a legitimate need to deal with wyciwyg--which I'm punting. That means that wyciwyg will show up in the session history menu.  I'm punting that because:
- It's not clear to me where exactly the fixup would go to just change the visible string.
- It's not clear to me what the right behavior even is, since FF2 shows wyciwyg there.
- We have deeper issues in that my understanding of wyciwyg is that it's intended to make going 'back' to a wyciwyg (document.write) page work, and that fails for test case in this bug in Camino (with or without this patch), but not in FF2.
So history is a bigger, nastier problem, and I don't want to hold up fixing the more obvious visible stuff for that. We should land this (or something like it) and file a follow-up bug for tackling the interaction between wyciwyg and Camino's history system.

Comment 17

[Håkan Waara]

Comment on attachment 243109 [details] [diff] [review]
more comprehensive hiding

Looks good to me. 

* I want either a visible comment to getCurrentURI, that it is a fixed-up URI that is for purely display purposes, and should not be used for passing back to gecko (for example), or that it's renamed to something to make it explicit.

* As you say, getCurrentURI seems to be used in only OK places right now. The only place I was unsure of is http://landfill.mozilla.org/mxr-test/mozilla/source/camino/src/browser/BrowserWrapper.mm#767

I think we should already have the favicon for the non-wyciwyg-case of the URI though.

* Oh, and the exposableURI object could use a null-check before being used, just for safety.

Comment 18

[Stuart Morgan]

Attachment: comments addressed

Added comments and null checks.

I've filed bug 357820 for the punted part.

Comment 19

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 243348 [details] [diff] [review]
comments addressed

sr=pink

Comment 20

[froodian (Ian Leue)]

Checked in on 1.8branch and trunk.