Closed Bug 1083793 Opened 7 years ago Closed 7 years ago

Make nsITreeBoxObject and nsIBoxObject scriptable again

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: hiro, Assigned: jcranmer)

References

Details

Attachments

(1 file, 1 obsolete file)

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
Does just adding the "scriptable" back fix things?
Blocks: 979835
Flags: needinfo?(hiikezoe)
(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.
Flags: needinfo?(hiikezoe)
Summary: Make nsITreeBoxObject scriptable again → Make nsITreeBoxObject and nsIBoxObject scriptable again
There are multiple static_casts of nsITreeBoxObject to the concrete type, but I guess those were there before too ...
The other option is to fix the things comm-central is hacking around there, of course so it can stop the insanity.
I would take a patch for this, I guess.
(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
(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
Oops, sorry for double posts.
I am checking this "restore scriptable" patch locally now.
TIA
I meant "try creating" instead of "checking".
Looks much harder than I initially thought. Like placing "scriptable" here and there. NOT :-(
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
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
(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.
The assertion was introduced by other bug, bug 1066459.
(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
Assignee: nobody → ishikawa
See Also: → 1085562
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.
Assignee: ishikawa → Pidgeot18
Attachment #8507322 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8508372 - Flags: review?(bzbarsky)
Comment on attachment 8508372 [details] [diff] [review]
Add scriptable to affected interfaces

r=me
Attachment #8508372 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/04142473004f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Duplicate of this bug: 1083810
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.