Don't show wyciwyg URLs in location bar

VERIFIED FIXED in Camino1.5

Status

Camino Graveyard
Location Bar & Autocomplete
--
minor
VERIFIED FIXED
12 years ago
10 years ago

People

(Reporter: Samuel Sidler (old account; do not CC), Assigned: Stuart Morgan)

Tracking

({fixed1.8.1.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1.1

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(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

12 years ago
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
(Reporter)

Comment 2

12 years ago
This isn't important for 1.0... targeting for 1.1.
Target Milestone: --- → Camino1.1

Updated

11 years ago
(Reporter)

Updated

11 years ago
Assignee: mikepinkerton → nobody
QA Contact: location.bar
(Assignee)

Comment 3

11 years ago
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.
Assignee: nobody → stuart.morgan
Status: NEW → ASSIGNED
Attachment #241911 - Flags: review?
(Assignee)

Comment 4

11 years ago
Pink/Mark, does this seem reasonable, or is jumping through the nsIURIFixup hoops substantially better?
(Assignee)

Comment 5

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

Not what we want.
Attachment #241911 - Attachment is obsolete: true
Attachment #241911 - Flags: review?
(Assignee)

Comment 6

11 years ago
Created attachment 242034 [details] [diff] [review]
nsIURIFixup version

Mmm, code re-use.
Attachment #242034 - Flags: review?
(Assignee)

Comment 7

11 years ago
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).
(Assignee)

Comment 8

11 years ago
Created attachment 242071 [details] [diff] [review]
adds pref disabling
Attachment #242034 - Attachment is obsolete: true
Attachment #242071 - Flags: review?
Attachment #242034 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #242071 - Flags: review? → review?(hwaara)

Comment 9

11 years ago
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?
(Assignee)

Comment 10

11 years ago
(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

11 years ago
(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.
(Assignee)

Comment 12

11 years ago
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.
Attachment #242071 - Flags: review?(hwaara)

Comment 13

11 years ago
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...
(Assignee)

Comment 14

11 years ago
(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

11 years ago
(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.
(Assignee)

Comment 16

11 years ago
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.
Attachment #242071 - Attachment is obsolete: true
Attachment #243109 - Flags: review?(hwaara)

Comment 17

11 years ago
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.
Attachment #243109 - Flags: review?(hwaara) → review+
(Assignee)

Comment 18

11 years ago
Created attachment 243348 [details] [diff] [review]
comments addressed

Added comments and null checks.

I've filed bug 357820 for the punted part.
Attachment #243109 - Attachment is obsolete: true
Attachment #243348 - Flags: superreview?(mikepinkerton)
Comment on attachment 243348 [details] [diff] [review]
comments addressed

sr=pink
Attachment #243348 - Flags: superreview?(mikepinkerton) → superreview+
Whiteboard: [needs checkin]

Comment 20

11 years ago
Checked in on 1.8branch and trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [needs checkin]

Updated

11 years ago
Keywords: fixed1.8.1.1

Updated

11 years ago
Keywords: fixed1.8.1
(Reporter)

Updated

10 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.