Closed
Bug 1083793
Opened 10 years ago
Closed 10 years ago
Make nsITreeBoxObject and nsIBoxObject scriptable again
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: hiro, Assigned: jcranmer)
References
Details
Attachments
(1 file, 1 obsolete file)
1.39 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Does just adding the "scriptable" back fix things?
Blocks: 979835
Flags: needinfo?(hiikezoe)
Reporter | ||
Comment 2•10 years ago
|
||
(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 ...
Comment 4•10 years ago
|
||
The other option is to fix the things comm-central is hacking around there, of course so it can stop the insanity.
No longer blocks: 1084474
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
Reporter | ||
Comment 12•10 years ago
|
||
(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.
Reporter | ||
Comment 13•10 years ago
|
||
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
Updated•10 years ago
|
Assignee: nobody → ishikawa
Assignee | ||
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
Comment on attachment 8508372 [details] [diff] [review]
Add scriptable to affected interfaces
r=me
Attachment #8508372 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 17•10 years ago
|
||
Comment 18•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•