Last Comment Bug 306396 - Don't show wyciwyg URLs in location bar
: Don't show wyciwyg URLs in location bar
Status: VERIFIED FIXED
: fixed1.8.1.1
Product: Camino Graveyard
Classification: Graveyard
Component: Location Bar & Autocomplete (show other bugs)
: unspecified
: PowerPC Mac OS X
-- minor (vote)
: Camino1.5
Assigned To: Stuart Morgan
:
:
Mentors:
javascript:document.write(navigator.u...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-08-29 20:51 PDT by Samuel Sidler (old account; do not CC)
Modified: 2007-11-06 14:24 PST (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
do-it-ourselves fix (1.70 KB, patch)
2006-10-10 21:05 PDT, Stuart Morgan
no flags Details | Diff | Splinter Review
nsIURIFixup version (1.69 KB, patch)
2006-10-11 22:35 PDT, Stuart Morgan
no flags Details | Diff | Splinter Review
adds pref disabling (2.48 KB, patch)
2006-10-12 07:55 PDT, Stuart Morgan
no flags Details | Diff | Splinter Review
more comprehensive hiding (5.57 KB, patch)
2006-10-22 09:15 PDT, Stuart Morgan
hwaara: review+
Details | Diff | Splinter Review
comments addressed (6.20 KB, patch)
2006-10-24 06:55 PDT, Stuart Morgan
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image Samuel Sidler (old account; do not CC) 2005-08-29 20:51:38 PDT
(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 User image Mark Mentovai 2005-08-29 20:55:37 PDT
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 User image Samuel Sidler (old account; do not CC) 2005-08-30 17:02:30 PDT
This isn't important for 1.0... targeting for 1.1.
Comment 3 User image Stuart Morgan 2006-10-10 21:05:25 PDT
Created attachment 241911 [details] [diff] [review]
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 User image Stuart Morgan 2006-10-10 21:08:36 PDT
Pink/Mark, does this seem reasonable, or is jumping through the nsIURIFixup hoops substantially better?
Comment 5 User image Stuart Morgan 2006-10-11 09:58:05 PDT
Comment on attachment 241911 [details] [diff] [review]
do-it-ourselves fix

Not what we want.
Comment 6 User image Stuart Morgan 2006-10-11 22:35:28 PDT
Created attachment 242034 [details] [diff] [review]
nsIURIFixup version

Mmm, code re-use.
Comment 7 User image Stuart Morgan 2006-10-11 22:51:43 PDT
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 User image Stuart Morgan 2006-10-12 07:55:54 PDT
Created attachment 242071 [details] [diff] [review]
adds pref disabling
Comment 9 User image Håkan Waara 2006-10-18 17:56:19 PDT
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 User image Stuart Morgan 2006-10-18 19:39:03 PDT
(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 User image Håkan Waara 2006-10-19 01:11:11 PDT
(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 User image Stuart Morgan 2006-10-19 06:47:11 PDT
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 User image Håkan Waara 2006-10-19 06:54:39 PDT
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 User image Stuart Morgan 2006-10-19 07:01:13 PDT
(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 User image Håkan Waara 2006-10-19 07:14:41 PDT
(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 User image Stuart Morgan 2006-10-22 09:15:44 PDT
Created attachment 243109 [details] [diff] [review]
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 User image Håkan Waara 2006-10-24 04:56:36 PDT
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 User image Stuart Morgan 2006-10-24 06:55:57 PDT
Created attachment 243348 [details] [diff] [review]
comments addressed

Added comments and null checks.

I've filed bug 357820 for the punted part.
Comment 19 User image Mike Pinkerton (not reading bugmail) 2006-10-25 08:01:25 PDT
Comment on attachment 243348 [details] [diff] [review]
comments addressed

sr=pink
Comment 20 User image froodian (Ian Leue) 2006-10-25 12:03:38 PDT
Checked in on 1.8branch and trunk.

Note You need to log in before you can comment on or make changes to this bug.