Closed Bug 365381 Opened 18 years ago Closed 18 years ago

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

Categories

(Core :: XBL, defect)

Product:
Core
Component:
XBL
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: asaf, Assigned: Gavin)

Details

Attachments

(1 file, 1 obsolete file)

for checkin
1.91 KB, patch
Details | Diff | Splinter Review 
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
Attached patch patch (obsolete) — — Splinter Review 
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+
Attached patch for checkin — — Splinter Review 

        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
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9beta
Target Milestone: mozilla1.9beta → mozilla1.9alpha

    
  
Flags: in-testsuite?
Flags: blocking1.9?

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: