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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Tony Mechelynck [:tonymec]. (NEEDINFO me if you want my attention) 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 User image 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 User image 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 User image 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.