Last Comment Bug 536940 - Port |Bug 468053 - gBrowser.addTab not treat null/undefined Uri as blank tab leading to extra work at least when restoring session| to SeaMonkey
: Port |Bug 468053 - gBrowser.addTab not treat null/undefined Uri as blank tab ...
Status: RESOLVED FIXED
: fixed-seamonkey2.0.3
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: seamonkey2.1a1
Assigned To: Serge Gautherie (:sgautherie)
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on: 468053 504344
Blocks: 534647 535907
  Show dependency treegraph
 
Reported: 2009-12-28 02:28 PST by Serge Gautherie (:sgautherie)
Modified: 2010-01-19 06:20 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
(Av1) tabbrowser.xml: check for empty URI too (2.53 KB, patch)
2009-12-28 03:16 PST, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
(Av1a) tabbrowser.xml: check for |!aURI| too [Checkin: Comment 5 & 11] (1.03 KB, patch)
2009-12-28 05:04 PST, Serge Gautherie (:sgautherie)
neil: review+
kairo: approval‑seamonkey2.0.3+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2009-12-28 02:28:46 PST
Bug 535907 comment 12:
{
neil@parkwaycc.co.uk      2009-12-20 13:47:31 PST

>     gBrowser.addTab().linkedBrowser.stop();
Note that our addTab doesn't correctly check for a missing argument...
}
Comment 1 Serge Gautherie (:sgautherie) 2009-12-28 03:16:59 PST
Created attachment 419288 [details] [diff] [review]
(Av1) tabbrowser.xml: check for empty URI too

Please, double-check the 2 first changes which I just assumed needed to be fixed too.

Checking if/which tests need to be updated will have to wait until bug 504344 is fixed.
(I tried to run some test suites locally recently but they have too many other failures...)
Comment 2 neil@parkwaycc.co.uk 2009-12-28 04:46:47 PST
Comment on attachment 419288 [details] [diff] [review]
(Av1) tabbrowser.xml: check for empty URI too

>-                    aLocation.spec != "about:blank")
>+                    (aLocation.spec && aLocation.spec != "about:blank"))

>-            if (aURI == "about:blank")
>+            if (!aURI || aURI == "about:blank")
As far as I know it's not possible for a URI to have an empty spec so we don't need these changes.

>-            var blank = (aURI == "about:blank");
>+            let blank = !aURI || aURI == "about:blank";
Mumble mumble var mumble mumble
Comment 3 Serge Gautherie (:sgautherie) 2009-12-28 05:04:22 PST
Created attachment 419295 [details] [diff] [review]
(Av1a) tabbrowser.xml: check for |!aURI| too
[Checkin: Comment 5 & 11]

Av1, with comment 2 suggestion(s).

(In reply to comment #2)

> As far as I know it's not possible for a URI to have an empty spec so we don't
> need these changes.

(Thanks, I just wanted to be sure I wasn't missing something.)

> Mumble mumble var mumble mumble

Well, what do you think is wrong with 'let'?
Comment 4 neil@parkwaycc.co.uk 2009-12-28 06:32:53 PST
Comment on attachment 419295 [details] [diff] [review]
(Av1a) tabbrowser.xml: check for |!aURI| too
[Checkin: Comment 5 & 11]

Here it's no help at all, but feel free to use let in the places that need it, which are few and far between.
Comment 5 Serge Gautherie (:sgautherie) 2009-12-28 08:34:33 PST
Comment on attachment 419295 [details] [diff] [review]
(Av1a) tabbrowser.xml: check for |!aURI| too
[Checkin: Comment 5 & 11]


http://hg.mozilla.org/comm-central/rev/2ab0fbab5907
Comment 6 Serge Gautherie (:sgautherie) 2009-12-28 08:38:26 PST
(In reply to comment #4)
> (From update of attachment 419295 [details] [diff] [review])
> Here it's no help at all, but feel free to use let in the places that need it,
> which are few and far between.

(Hum, I thought the new goal would be more like 'always use let, unless var is needed'.)
Comment 7 Philip Chee 2009-12-28 21:47:23 PST
> (Hum, I thought the new goal would be more like 'always use let, unless var is
> needed'.)
Only in Firefox. In Suite it's always use var except when using let makes sense.
Comment 8 Tony Mechelynck [:tonymec] 2010-01-13 12:59:12 PST
I see this is FIXED but apparently only on Trunk (Sm 2.1.x). Any idea if this fix is going to be ported to comm-1.9.1 (Sm 2.0.x)?
Comment 9 Serge Gautherie (:sgautherie) 2010-01-16 08:03:29 PST
(In reply to comment #8)
> Any idea if this fix is going to be ported to comm-1.9.1 (Sm 2.0.x)?

It won't (though it's trivial), unless someone writes why it could pass approval...
Comment 10 Serge Gautherie (:sgautherie) 2010-01-18 10:49:25 PST
Comment on attachment 419295 [details] [diff] [review]
(Av1a) tabbrowser.xml: check for |!aURI| too
[Checkin: Comment 5 & 11]

"approval-seamonkey2.0.3=?":
No risk. May help/fix bug 534647 (for example)...
Comment 11 Serge Gautherie (:sgautherie) 2010-01-19 06:19:35 PST
Comment on attachment 419295 [details] [diff] [review]
(Av1a) tabbrowser.xml: check for |!aURI| too
[Checkin: Comment 5 & 11]


http://hg.mozilla.org/releases/comm-1.9.1/rev/392e4ef71e67

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