758102 - Resync' smileApplication.js from fuelApplication.js

Status: VERIFIED FIXED

(SeaMonkey :: General, defect, P3)

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) smileApplication.js: Port bug 506471 and bug 543672

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) smileApplication.js: License, documentation and nits

There is one additional diff which I'm confused about:


http://mxr.mozilla.org/comm-central/source/mozilla/browser/fuel/src/fuelApplication.js
{
97     this._tabbrowser.tabContainer.addEventListener(aType,
123       this._tabbrowser.removeEventListener(type, this._cleanup[type], true);
}

See
http://hg.mozilla.org/mozilla-central/rev/8ca8630b0c88

Is FF missing a '.tabContainer'?


http://mxr.mozilla.org/comm-central/source/suite/smile/src/smileApplication.js
{
142     this._tabbrowser.tabContainer.addEventListener(aType,
172       this._tabbrowser.tabContainer.removeEventListener(type, this._cleanup[type], true);
}

See
http://hg.mozilla.org/comm-central/rev/99b127296682

This looks right,
but we get:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1337670058.1337676375.26828.gz&fulltext=1
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/05/22 00:00:58
{
*** End BrowserChrome Test Results ***
[...]
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "this._tabbrowser.tabContainer is undefined" {file: "resource:///components/smileApplication.js" line: 172}]' when calling method: [nsIObserver::observe]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
}
or is it just a false issue "due" to bug 603110 or the like?

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) smileApplication.js: Port bug 506471 and bug 543672 [Checked in: Comment 16]

Av1, with an additional update.

FF doesn't have this additional change:
am I just wrong or is FF missing this?

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) SMILE tests: Trivial resync' from FUEL, Ports relate part of bug 573246

Comment 5

[[e-mail]]

Comment on attachment 626709 [details] [diff] [review]
(Cv1) SMILE tests: Trivial resync' from FUEL, Ports relate part of bug 573246

>+  // Preference events tests disabled until bug 533290 is fixed
>+  return;
We're not actually failing the test though are we?

Comment 6

[[e-mail]]

(In reply to Serge Gautherie from comment #2)
> Is FF missing a '.tabContainer'?
File a bug and CC :dao

> * Call to xpconnect wrapped JSObject produced this error:  *
> [Exception... "'[JavaScript Error: "this._tabbrowser.tabContainer is
> undefined" {file: "resource:///components/smileApplication.js" line: 172}]'
> when calling method: [nsIObserver::observe]"  nsresult: "0x80570021
> (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame ::
> <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
Ratty was trying to future-proof us against implementing tabs on top. Sadly by the time the shutdown method runs, the binding no longer exists. I think the workaround might be to store the tabContainer in a property too, but :dao might have other ideas, after all he's going to have to fix the same bug ;-)

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #5)
> We're not actually failing the test though are we?

The (whole) test succeeds, afaict.
But I would prefer to be closer to FF (test run), just_in_case/ftb.


(In reply to [e-mail] from comment #6)

> Ratty was trying to future-proof us against implementing tabs on top. Sadly

Yeah, so I read in the bug he fixed.

> by the time the shutdown method runs, the binding no longer exists.

Should we just remove both '.tabContainer' in smile ftb?

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #6)
> File a bug and CC :dao

Bug 758248 Submitted

Comment 9

[[e-mail]]

(In reply to Serge Gautherie from comment #7)
> (In reply to comment #5)
> > We're not actually failing the test though are we?
> The (whole) test succeeds, afaict.
Well, just because FF can't test FUEL properly, doesn't mean that we shouldn't test SMILE properly...

> (In reply to comment #6)
> > by the time the shutdown method runs, the binding no longer exists.
> Should we just remove both '.tabContainer' in smile ftb?
Well, that's the "quick fix", yes.

Comment 10

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1a) smileApplication.js: Remove broken '.tabContainer' support, Update license, documentation and nits [Checked in: See comment 17]

Bv1, with comment 9 suggestion(s).

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1a) SMILE tests: Trivial resync' from FUEL [Checked in: Comment 18]

Cv1, with comment 9 suggestion(s).

Comment 12

[[e-mail]]

(In reply to Serge Gautherie from comment #3)
> FF doesn't have this additional change:
> am I just wrong or is FF missing this?
Please CC :dietrich on a bug for this one!

Comment 13

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #12)
> Please CC :dietrich on a bug for this one!

Bug 758459 Submitted

Comment 14

[[e-mail]]

Comment on attachment 626703 [details] [diff] [review]
(Av1a) smileApplication.js: Port bug 506471 and bug 543672
[Checked in: Comment 16]

Nit: livemarks getter has bitrotted, needs two interfaces.

Comment 15

[[e-mail]]

Comment on attachment 626854 [details] [diff] [review]
(Bv1a) smileApplication.js: Remove broken '.tabContainer' support, Update license, documentation and nits
[Checked in: See comment 17]

>+
> 
Nit: double blank line

>+
>+
Nit: double blank line

>+// fine as not exposed in idl.
Don't bother with this line.

> 
>+
> //=================================================
> // BookmarkRoots implementation
Looks like this one is OK.

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 626703 [details] [diff] [review]
(Av1a) smileApplication.js: Port bug 506471 and bug 543672
[Checked in: Comment 16]

http://hg.mozilla.org/comm-central/rev/c770788d90c1

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 626854 [details] [diff] [review]
(Bv1a) smileApplication.js: Remove broken '.tabContainer' support, Update license, documentation and nits
[Checked in: See comment 17]

http://hg.mozilla.org/comm-central/rev/c28d011cbbe1
Bv1a, with comment 15 suggestion(s).

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 626856 [details] [diff] [review]
(Cv1a) SMILE tests: Trivial resync' from FUEL
[Checked in: Comment 18]

http://hg.mozilla.org/comm-central/rev/54380e539f1a

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1-SM211) smileApplication.js: Remove broken '.tabContainer' support [Too late]

[Approval Request Comment]
Minimal part of patch Bv1b.
Regression caused by (bug #): Bug 647571.
No risk, reverts to previous code.

Comment 20

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1338597699.1338600532.4177.gz&fulltext=1
OS X 10.6 comm-central-trunk debug test mochitest-other on 2012/06/01 17:41:39

V.Fixed.

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 629127 [details] [diff] [review]
(Dv1-SM211) smileApplication.js: Remove broken '.tabContainer' support
[Too late]

Missed SM/2.11 :-/