wyciwyg: should not appear in tab title

VERIFIED FIXED

Status

SeaMonkey
Tabbed Browser
VERIFIED FIXED
16 years ago
10 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: Christopher Aillon (sabbatical, not receiving bugmail))

Tracking

({polish, regression})

Trunk
polish, regression

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

16 years ago
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

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

Comment 2

16 years ago
Created attachment 96294 [details] [diff] [review]
Proposed patch
(Reporter)

Comment 3

16 years ago
The regression was "caused" by caillon's tab title fix.
Keywords: patch, polish, regression, review, ui
Created attachment 96399 [details] [diff] [review]
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?
Attachment #96294 - Attachment is obsolete: true
(Reporter)

Comment 5

16 years ago
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!
Attachment #96399 - Flags: needs-work+
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

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

Comment 8

16 years ago
Created attachment 96899 [details] [diff] [review]
factor the url clean-up code out
Attachment #96399 - Attachment is obsolete: true
Created attachment 96989 [details] [diff] [review]
Factored into c++

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

Comment 10

16 years ago
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
Created attachment 97736 [details] [diff] [review]
Addresses rpotts' comments
Attachment #96899 - Attachment is obsolete: true
Attachment #96989 - Attachment is obsolete: true
+        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?

OS: Windows 95 → All
Hardware: PC → All
Created attachment 97895 [details] [diff] [review]
Address bz's comments
Attachment #97736 - Attachment is obsolete: true
+    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?
Created attachment 97906 [details] [diff] [review]
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.
Attachment #97895 - Attachment is obsolete: true
Comment on attachment 97906 [details] [diff] [review]
Better patch :-)

sr=bzbarsky
Attachment #97906 - Flags: superreview+

Comment 17

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

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

Other than that, r=jag
Attachment #97906 - Flags: review+
Fixed
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Reporter)

Comment 19

16 years ago
Spun off bug 168509.
Status: RESOLVED → VERIFIED
note: bug to port this to toolkit is bug 169826
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.