1010393 - Remove aProtoDoc parameter from nsXULPrototypeScript::Compile

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

It is just unnecessary these days.

Comment 1

[Benny Mei]

I would like to take a stab at this bug. It's my first time doing this... Where do I start?

Comment 2

[Bobby Holley (:bholley)]

(In reply to Benny Mei from comment #1)
> I would like to take a stab at this bug. It's my first time doing this...
> Where do I start?

Hi Benny! Welcome to Mozilla. :-)

The file to edit is in content/xul/content/src/nsXULElement.cpp. If you're looking for a basic guide to getting started, see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide . Also check out #introduction on irc.mozilla.org.

Comment 3

[Benny Mei]

Attachment: aProtoDoc parameter removed from nsXULPrototypeScript::Compile

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8429543 [details] [diff] [review]
aProtoDoc parameter removed from nsXULPrototypeScript::Compile

Review of attachment 8429543 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! Please make the fixes below and upload a second patch. I'll then push it to try and flag for it to be checked in. :-)

::: content/xul/content/src/nsXULElement.cpp
@@ +2622,5 @@
>  nsresult
>  nsXULPrototypeScript::Compile(JS::SourceBufferHolder& aSrcBuf,
>                                nsIURI* aURI,
>                                uint32_t aLineNo,
> +                              nsIDocument* aDocument, 

Nit - please remove the trailing whitespace here.

::: content/xul/document/src/XULDocument.cpp
@@ +3549,5 @@
>              mOffThreadCompileStringBuf = nullptr;
>              mOffThreadCompileStringLength = 0;
>  
>              rv = mCurrentScriptProto->Compile(srcBuf,
> +                                              uri, 1, this, 

and here. You can probably also just move all these arguments onto a single line.

Comment 5

[Benny Mei]

(In reply to Bobby Holley (:bholley) from comment #4)
> Comment on attachment 8429543 [details] [diff] [review]
> aProtoDoc parameter removed from nsXULPrototypeScript::Compile
> 
> Review of attachment 8429543 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great! Please make the fixes below and upload a second patch.
> I'll then push it to try and flag for it to be checked in. :-)
> 
> ::: content/xul/content/src/nsXULElement.cpp
> @@ +2622,5 @@
> >  nsresult
> >  nsXULPrototypeScript::Compile(JS::SourceBufferHolder& aSrcBuf,
> >                                nsIURI* aURI,
> >                                uint32_t aLineNo,
> > +                              nsIDocument* aDocument, 
> 
> Nit - please remove the trailing whitespace here.
> 
> ::: content/xul/document/src/XULDocument.cpp
> @@ +3549,5 @@
> >              mOffThreadCompileStringBuf = nullptr;
> >              mOffThreadCompileStringLength = 0;
> >  
> >              rv = mCurrentScriptProto->Compile(srcBuf,
> > +                                              uri, 1, this, 
> 
> and here. You can probably also just move all these arguments onto a single
> line.

I'm not quite sure what you mean by removing trailing whitespace in nsXULElement.cpp. Much of the other code looks similar to what I submitted in the first patch. 

I attached another patch that moves aLineNo to the same line as aURI. Is that what you meant?

Comment 6

[Benny Mei]

Attachment: Resolved some whitespace issues.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8429613 [details] [diff] [review]
Resolved some whitespace issues.

Review of attachment 8429613 [details] [diff] [review]:
-----------------------------------------------------------------

If you click 'review' on your uploaded patch and look at nsXULElement.cpp, you'll see that the tool highlights an extra space after |nsIDocument* aDocument,|. Let me know if you run into problems.

Comment 8

[Benny Mei]

Attachment: Resolved some whitespace issues.

Noted. Thank you for pointing out this issue. This new patch should remove that extra whitespace.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8429655 [details] [diff] [review]
Resolved some whitespace issues.

Review of attachment 8429655 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! I'll push it to the try server and flag it for checkin.

Comment 10

[Bobby Holley (:bholley)]

checkin-needed, pending the try push:

https://tbpl.mozilla.org/?tree=Try&rev=345d99d2bb09

Comment 11

[Benny Mei]

Assignee: [e-mail] -> [e-mail]

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8db21a5bff9d

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8db21a5bff9d