164006 - wyciwyg: should not appear in tab title

Status: VERIFIED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[[e-mail]]

Using Build ID: 2002082104
Steps to reproduce problem:
1. Open the supplied URL in a new tab

Expected results: tab title is (Untitled)

Actual results: tab title is wyciwyg://0/about:blank

Similar considerations apply to other wyciwyg: URLs.

Comment 1

[Jo Hermans]

I'm not seeing this on build 2002072104 (Windows NT). Not even when I type
'wyciwyg://0/about:blank' into the locationbar directly. The location-bar is
always 'about:blank' and the tab-title is '(Untitled)'.

a regression ?

Comment 2

[[e-mail]]

Attachment: Proposed patch

Comment 3

[[e-mail]]

The regression was "caused" by caillon's tab title fix.

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Better fix

We should just hook directly into window.XULBrowserWindow to avoid duplicating
the code to strip out this sort of thing.  Jag, can you review this?

Comment 5

[[e-mail]]

Comment on attachment 96399 [details] [diff] [review]
Better fix

Surely the XULBrowserWindow is a per-window object, not a per-tab object, so
this will go wrong if the tab loads in the background? I like contentTitle
though!

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 96399 [details] [diff] [review]
Better fix

> Surely the XULBrowserWindow is a per-window object, not a per-tab object, so
this will go wrong if the tab loads in the background?

Duh.  Maybe if I actually used that pref, I'd remember about it...

Comment 7

[jag (Peter Annema)]

That, and I'm not too fond of that direct hookup to XULBrowserWindow. Can you
factor the url clean-up code out? 

Comment 8

[[e-mail]]

Attachment: factor the url clean-up code out

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Factored into c++

I was actually thinking more of something like this.  nsIURIFixup seems like
the perfect place to put this code.

Comment 10

[rpotts (gone)]

hey chris,
here are some comments on the latest patch...

I'm a little afraid about removing nsDocShell::CreateURIFixup(...)... With the
patch, '\r\n' characters will no longer be stripped before calling
NS_NewURI(...) if a uri-fixup component is not available...

Along the same lines, i wonder if calls through gURIFixup (in the JS) should be
made within a try/catch block... just in case no uri-fixup component is available...

Other than that, i think the patch looks good!

-- rick

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Addresses rpotts' comments

Comment 12

[Boris Zbarsky [:bzbarsky]]

+        NS_IF_ADDREF(*aReturn);

You know *aReturn is not null... see the NS_ENSURE_ARG_POINTER(aURI); you call.

+    if (slashIndex == -1)

kNotFound, not -1

+    nsCAutoString newUri;

Substring() is your friend

+    rv = NS_NewURI(aReturn, newUri.get());

since NS_NewURI takes an nsACString.  In fact the version that takes char* wraps
it in a depedent string and calls the other version.

You need to pass along the charset from the wyciwyg URI to the new URI you create.

> Index: docshell/base/nsIURIFixup.idl

Document the @return for the function too?

The navigator.js code should have a try/catch around the fixup stuff.  For
example, right now saving foobar://baz via the navigator.js codepath will throw
and uncaught exception.  It should catch and not call saveURL (and maybe alert
something sane?).

I'd move the try/catch out to include the getService() call in
nsBrowserStatusHandler.js... if that fails, do we really want to show "" as the uri?

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Address bz's comments

Comment 14

[Boris Zbarsky [:bzbarsky]]

+    rv = NS_NewURI(aReturn, Substring(path, slashIndex + 1, pathLength - 1),
charset.get());

Shouldn't that be pathLength - slashIndex - 1 ?  What happens if slashIndex ==
pathLength, btw?

Should have caught this last night, but you should document that
createExposableURI will throw exceptions in various instances (eg unknown scheme).

> +                           Substring(path, slashIndex + 1, pathLength),
charset.get());

Same length issues as above here.

-            if (!loc)
-              loc = ({ spec: "" });

Why remove this code?  Is mCurrentBrowser.currentURI guaranteed non-null?

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Better patch :-)

Hmm, somehow I managed to confuse myself into thinking that Substring took two
offsets instead of an offset and a length.  That's fixed, and exceptions
documented.

Also, I explained this to bz on IRC, but I'll do so here as well: I removed the
location code precisely _because_ we aren't guaranteed to get an nsIURI: If
we're going to be null, let's be null and not some fake believe URI.  |loc|
gets passed into onLocationChange in nsBrowserStatusHandler.js, where it
(aLocation) does get checked for nullness.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 97906 [details] [diff] [review]
Better patch :-)

sr=bzbarsky

Comment 17

[jag (Peter Annema)]

Comment on attachment 97906 [details] [diff] [review]
Better patch :-)

NS_ENSURE_TRUE(!uriString.IsEmpty(), NS_ERROR_FAILURE);

Other than that, r=jag

Comment 18

[Christopher Aillon (sabbatical, not receiving bugmail)]

Fixed

Comment 19

[[e-mail]]

Spun off bug 168509.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

this caused Bug 181724

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

note: bug to port this to toolkit is bug 169826