<method name="foo"><body/></method> doesn't compile

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
XBL
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: mano, Assigned: Gavin)

Tracking

Trunk
mozilla1.9alpha1
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

I've the following method defined in places' tree view (see http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/tree.xml)

      <method name="destroyContextMenu">
        <parameter name="aPopup"/>
        <body/>
      </method>

We get a "destroyContextMenu is not a function" error when it is called unless <body/> is replaced with <body></body>.

I see destroyContextMenu in DOMi's XBL view for the places tree, but not in its JavaScript object view for it.
Flags: blocking1.9?
Summary: <method name="foo><body/></method> doesn't complie → <method name="foo><body/></method> doesn't compile
Summary: <method name="foo><body/></method> doesn't compile → <method name="foo"><body/></method> doesn't compile
OS: Mac OS X 10.3 → All
Hardware: Macintosh → All
Created attachment 250114 [details] [diff] [review]
patch

The reason <body></body> worked for Asaf is because he had whitespace between the two when he tested. "<body></body>" itself shows the same problem. Since nsXBLContentSink::FlushText is not called in those cases, we'd never call AppendBodyText on the nsXBLProtoImplMethod, so when it came time to compile the method in nsXBLProtoImplMethod::CompileMember we'd bail early and not create the function object. This makes us create function objects even if the body had no text content.
Assignee: general → gavin.sharp
Status: NEW → ASSIGNED
Attachment #250114 - Flags: superreview?(bzbarsky)
Attachment #250114 - Flags: review?(bzbarsky)
Comment on attachment 250114 [details] [diff] [review]
patch

>+  // Get the body
>+  nsDependentString body;
>+  PRUnichar *bodyText = mUncompiledMethod->mBodyText.GetText();
>+  if (bodyText)
>+    body.Assign(bodyText);

So the idea is that we want an empty body to be equivalent to 
"function () {}"?

If so, this looks alright, except you want to use Rebind(), not Assign().
Assign() will copy the text, which we don't particularly want here.

With that change, r+sr=bzbarsky
Attachment #250114 - Flags: superreview?(bzbarsky)
Attachment #250114 - Flags: superreview+
Attachment #250114 - Flags: review?(bzbarsky)
Attachment #250114 - Flags: review+
Created attachment 250217 [details] [diff] [review]
for checkin
Attachment #250114 - Attachment is obsolete: true
(In reply to comment #2)
> So the idea is that we want an empty body to be equivalent to 
> "function () {}"?

Yes. Thanks for the review!
Whiteboard: [checkin needed]
mozilla/content/xbl/src/nsXBLProtoImplMethod.cpp 	1.34
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9beta
Target Milestone: mozilla1.9beta → mozilla1.9alpha
Flags: in-testsuite?

Updated

11 years ago
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.