Resync' smileApplication.js from fuelApplication.js

VERIFIED FIXED in seamonkey2.12

Status

SeaMonkey
General
P3
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

(Blocks: 1 bug, {regression})

Trunk
seamonkey2.12
regression
Dependency tree / graph
Bug Flags:
in-testsuite +

SeaMonkey Tracking Flags

(seamonkey2.9 wontfix, seamonkey2.10 wontfix, seamonkey2.11 wontfix)

Details

(URL)

Attachments

(3 attachments, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

6 years ago
Blocks: 467530

Updated

6 years ago
See Also: → bug 506471, bug 543672
(Assignee)

Comment 1

6 years ago
Created attachment 626685 [details] [diff] [review]
(Av1) smileApplication.js: Port bug 506471 and bug 543672
Attachment #626685 - Flags: review?(neil)
(Assignee)

Updated

6 years ago
Depends on: 506471, 543672
See Also: bug 506471, bug 543672
(Assignee)

Comment 2

6 years ago
Created attachment 626701 [details] [diff] [review]
(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?
Attachment #626701 - Flags: review?(neil)
(Assignee)

Comment 3

6 years ago
Created attachment 626703 [details] [diff] [review]
(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?
Attachment #626703 - Flags: review?(neil)
(Assignee)

Updated

6 years ago
Depends on: 573246
(Assignee)

Comment 4

6 years ago
Created attachment 626709 [details] [diff] [review]
(Cv1) SMILE tests: Trivial resync' from FUEL, Ports relate part of bug 573246
Attachment #626709 - Flags: review?(neil)

Comment 5

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

6 years ago
(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 ;-)
(Assignee)

Comment 7

6 years ago
(In reply to neil@parkwaycc.co.uk 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 neil@parkwaycc.co.uk 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?
(Assignee)

Updated

6 years ago
Depends on: 758248
(Assignee)

Comment 8

6 years ago
(In reply to neil@parkwaycc.co.uk from comment #6)
> File a bug and CC :dao

Bug 758248 Submitted

Comment 9

6 years ago
(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.
(Assignee)

Comment 10

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

Bv1, with comment 9 suggestion(s).
Attachment #626701 - Attachment is obsolete: true
Attachment #626701 - Flags: review?(neil)
Attachment #626854 - Flags: review?(neil)
(Assignee)

Comment 11

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

Cv1, with comment 9 suggestion(s).
Attachment #626709 - Attachment is obsolete: true
Attachment #626709 - Flags: review?(neil)
Attachment #626856 - Flags: review?(neil)
(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!
(Assignee)

Updated

6 years ago
Depends on: 758459
(Assignee)

Comment 13

6 years ago
(In reply to neil@parkwaycc.co.uk from comment #12)
> Please CC :dietrich on a bug for this one!

Bug 758459 Submitted
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.
Attachment #626703 - Flags: review?(neil) → review+
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.
Attachment #626854 - Flags: review?(neil) → review+

Updated

6 years ago
Attachment #626856 - Flags: review?(neil) → review+
(Assignee)

Updated

6 years ago
Attachment #626685 - Attachment is obsolete: true
Attachment #626685 - Flags: review?(neil)
(Assignee)

Comment 16

6 years ago
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
Attachment #626703 - Attachment description: (Av1a) smileApplication.js: Port bug 506471 and bug 543672 → (Av1a) smileApplication.js: Port bug 506471 and bug 543672 [Checked in: Comment 16]
(Assignee)

Comment 17

6 years ago
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).
Attachment #626854 - Attachment description: (Bv1a) smileApplication.js: Remove broken '.tabContainer' support, Update license, documentation and nits → (Bv1a) smileApplication.js: Remove broken '.tabContainer' support, Update license, documentation and nits [Checked in: See comment 17]
(Assignee)

Comment 18

6 years ago
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
Attachment #626856 - Attachment description: (Cv1a) SMILE tests: Trivial resync' from FUEL → (Cv1a) SMILE tests: Trivial resync' from FUEL [Checked in: Comment 18]
(Assignee)

Updated

6 years ago
Blocks: 647571
No longer blocks: 467530
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-seamonkey2.10: --- → affected
status-seamonkey2.11: --- → affected
status-seamonkey2.9: --- → wontfix
Flags: in-testsuite+
Keywords: regression
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.12
(Assignee)

Comment 19

6 years ago
Created attachment 629127 [details] [diff] [review]
(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.
Attachment #629127 - Flags: approval-comm-beta?
Attachment #629127 - Flags: approval-comm-aurora?
(Assignee)

Updated

6 years ago
Blocks: 760779
(Assignee)

Comment 20

6 years ago
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.
Status: RESOLVED → VERIFIED
(Assignee)

Updated

6 years ago
Attachment #629127 - Flags: approval-comm-aurora?
(Assignee)

Updated

6 years ago
status-seamonkey2.10: affected → wontfix
(Assignee)

Comment 21

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

Missed SM/2.11 :-/
Attachment #629127 - Attachment description: (Dv1-SM211) smileApplication.js: Remove broken '.tabContainer' support → (Dv1-SM211) smileApplication.js: Remove broken '.tabContainer' support [Too late]
Attachment #629127 - Attachment is obsolete: true
Attachment #629127 - Flags: approval-comm-beta?
(Assignee)

Updated

5 years ago
status-seamonkey2.11: affected → wontfix
You need to log in before you can comment on or make changes to this bug.