1083793 - Make nsITreeBoxObject and nsIBoxObject scriptable again

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

There are some nsITreeBoxObject implementation in comm-central.

For examples:
http://mxr.mozilla.org/comm-central/source/mailnews/base/test/unit/test_junkingWhenDisabled.js#42
http://mxr.mozilla.org/comm-central/source/mail/base/content/folderDisplay.js#2560

Unfortunately bug 979835 made the interface unscriptable. So lots of failures have been appeared in both xpcshell tests  and mozmill tests

Comment 1

[Boris Zbarsky [:bzbarsky]]

Does just adding the "scriptable" back fix things?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Does just adding the "scriptable" back fix things?

nsIBoxObject is also be scriptable, but yes as far as I confirmed.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

There are multiple static_casts of nsITreeBoxObject to the concrete type, but I guess those were there before too ...

Comment 4

[Boris Zbarsky [:bzbarsky]]

The other option is to fix the things comm-central is hacking around there, of course so it can stop the insanity.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I would take a patch for this, I guess.

Comment 6

[ISHIKAWA, Chiaki]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> (In reply to Boris Zbarsky [:bz] from comment #1)
> > Does just adding the "scriptable" back fix things?
> 
> nsIBoxObject is also be scriptable, but yes as far as I confirmed.

Making them (BOTH nsIBOxObject and nsITreeBoxObject) scriptable will let TB to run, then?



(In reply to Boris Zbarsky [:bz] from comment #4)
> The other option is to fix the things comm-central is hacking around there,
> of course so it can stop the insanity.

Are your referring to the following code by "around there"?

http://mxr.mozilla.org/comm-central/source/mailnews/base/test/unit/test_junkingWhenDisabled.js#42
http://mxr.mozilla.org/comm-central/source/mail/base/content/folderDisplay.js#2560

The former seems to be created by
Bug 487610 - Messages marked manually as Junk are never moved to Junk folder (if user doesn't check "Enable adaptive junk mail controls for this account"), r/sr=bienvenu 
(in 2009)

The latter seems to be created by
Bug 474701 - gloda global search on toolbar, folder display refactoring mega-bug
(in 2010)

I looked at the code 
I am afraid people's recollection or the amount of excess man-power to
remedy the above code (especially the latter) is not large.

I would ask someone to create the simple "restore scriptable" patch.
Quick and Dirty patch would be immensely helpful for this issue.

TIA

Comment 7

[ISHIKAWA, Chiaki]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> (In reply to Boris Zbarsky [:bz] from comment #1)
> > Does just adding the "scriptable" back fix things?
> 
> nsIBoxObject is also be scriptable, but yes as far as I confirmed.

Making them (BOTH nsIBOxObject and nsITreeBoxObject) scriptable will let TB to run, then?

(In reply to Boris Zbarsky [:bz] from comment #4)
> The other option is to fix the things comm-central is hacking around there,
> of course so it can stop the insanity.

Are your referring to the following code by "around there"?

http://mxr.mozilla.org/comm-central/source/mailnews/base/test/unit/test_junkingWhenDisabled.js#42

http://mxr.mozilla.org/comm-central/source/mail/base/content/folderDisplay.js#2560

The former seems to be created by
Bug 487610 - Messages marked manually as Junk are never moved to Junk folder (if user doesn't check "Enable adaptive junk mail controls for this account"), r/sr=bienvenu 
(in 2009)

The latter seems to be created by
Bug 474701 - gloda global search on toolbar, folder display refactoring mega-bug
(in 2010)

I looked at the code, but not understand much of it.
 
I am afraid people's recollection or the amount of excess man-power to
remedy the above code (especially the latter) is not large.

I would ask someone to create the simple "restore scriptable" patch.
Quick and Dirty patch would be immensely helpful for this issue.

TIA

Comment 8

[ISHIKAWA, Chiaki]

Oops, sorry for double posts.
I am checking this "restore scriptable" patch locally now.
TIA

Comment 9

[ISHIKAWA, Chiaki]

I meant "try creating" instead of "checking".
Looks much harder than I initially thought. Like placing "scriptable" here and there. NOT :-(

Comment 10

[ISHIKAWA, Chiaki]

I managed to suppress
>NS_ERROR_FAILURE: Failure arg 0 [nsITreeView.setTree]
type of bugs at least by changing some .idl files

But still 
about 1/4 of |make mozmill| test (1000+) fails locally.
Something is amiss.
Does the change here make changes that cause previously working code behave in
a subtly different manner (instead of breaking it completely)?

There could be such a bug since I see

TEST-START | /REF-COMM-CENTRAL/comm-central/mail/test/mozmill/folder-display/test-selection.js | test_selection_last_message_deleted
Step Pass: {"function": "Controller.keypress()"}
Assertion failure: mTopRowIndex == mozilla::clamped(mTopRowIndex, 0, maxTopRowIndex), at /REF-COMM-CENTRAL/comm-central/mozilla/layout/xul/tree/nsTreeBodyFrame.cpp:4088

which suggests something went wrong.
This assertion failure, which resulted in crash, may block the subsequent 200 or so
tests (but I am not entirely sure.) |make mozmill| test log is full of spurious warnings and error messages as usual.

TIA

Comment 11

[ISHIKAWA, Chiaki]

Attachment: restore scriptable to some interfaces (W-I-P)

For those who might want to check if just restoring scriptable 
eliminates the problems, here is the local patch I have created.
I am afraid that I have made one extra interface scriptable by mistake (there are so many simiarly looking identfiers.)

Anyway, this compiles C-C TB just fine, and eliminates the
XPCOM interface error (Failure discussed here).

But as far as |make mozmill| is concerned I still have many errors.
But now I am not sure if that is due to local patches, or not.

YMMV.

TIA

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to ISHIKAWA, Chiaki from comment #10)
> I managed to suppress
> >NS_ERROR_FAILURE: Failure arg 0 [nsITreeView.setTree]
> type of bugs at least by changing some .idl files
> 
> But still 
> about 1/4 of |make mozmill| test (1000+) fails locally.
> Something is amiss.
> Does the change here make changes that cause previously working code behave
> in
> a subtly different manner (instead of breaking it completely)?
> 
> There could be such a bug since I see
> 
> TEST-START |
> /REF-COMM-CENTRAL/comm-central/mail/test/mozmill/folder-display/test-
> selection.js | test_selection_last_message_deleted
> Step Pass: {"function": "Controller.keypress()"}
> Assertion failure: mTopRowIndex == mozilla::clamped(mTopRowIndex, 0,
> maxTopRowIndex), at
> /REF-COMM-CENTRAL/comm-central/mozilla/layout/xul/tree/nsTreeBodyFrame.cpp:
> 4088

Ah, I am sorry. I did not run the test on debug build. I just confirmed on opt build.

I am guessing that rowCount in our tree view something different from what nsTreeBodyFrame expects.
But I don't understand that the assertion is related to scriptable.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

The assertion was introduced by other bug, bug 1066459.

Comment 14

[ISHIKAWA, Chiaki]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> The assertion was introduced by other bug, bug 1066459.

Thanks for the pointer.
Though, I am not sure if that bug fix in bug 1066459 has introduced the condition 
that triggered the assertion condition or something.
(I will try to see if backing out the patch there can make |make mozmill| with
fewer unexpected failures.)
I will clean up the patch in the meantime.

TIA

Comment 15

[Joshua Cranmer [:jcranmer]]

Attachment: Add scriptable to affected interfaces

In the short term, this gets TB working again.

I agree that the pseudo-implementation of nsITreeBoxObject really bad, but a deep understanding of what's going on is needed to be able to remove it. The shallow uses in the tests were removed in bug 1085562, and I have a little-tested patch that removes the .QI(nsITreeBoxObject) in a few places. But folderDisplay.js's FakeTreeBoxObject is a different story, and the little archæology I did appears to indicate that it does do something useful and important.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8508372 [details] [diff] [review]
Add scriptable to affected interfaces

r=me

Comment 17

[Joshua Cranmer [:jcranmer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04142473004f

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/04142473004f