729049 - Inline IsXMLLetter, IsXMLNCNameChar, DecodeEntity form nsIParserService

Status: RESOLVED FIXED

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

Description

[Henri Sivonen (:hsivonen)]

IsXMLLetter, IsXMLNCNameChar, DecodeEntity in nsIParserService each have one caller. Inline these methods into their callers to make it possible to zap nsIParserService.

Comment 1

[Charles Chan]

Attachment: Bug-729049: Patch-1

Here's the patch file. Should I also remove the functions from nsParserService.h or the header file altogether?

Comment 2

[Henri Sivonen (:hsivonen)]

(In reply to Charles Chan from comment #1)
> Created attachment 604703 [details] [diff] [review]
> Bug-729049: Patch-1
> 
> Here's the patch file.

Thank you.

> Should I also remove the functions from
> nsParserService.h or the header file altogether?

Yes please. The goal is to take stuff out of nsIParserService until there's nothing there and nsIParserService can be removed.

Comment 3

[Charles Chan]

Attachment: Bug-729049: Patch-2

(In reply to Henri Sivonen (:hsivonen) from comment #2)
> > Should I also remove the functions from
> > nsParserService.h or the header file altogether?
> 
> Yes please. The goal is to take stuff out of nsIParserService until there's
> nothing there and nsIParserService can be removed.

Here's an updated patch which also remove the functions declaration from nsiParserService.h and definition from nsParservice.h

Comment 4

[Josh Matthews [:jdm]]

Comment on attachment 604803 [details] [diff] [review]
Bug-729049: Patch-2

For future reference, Charles, you can (and should!) set flags like review? and feedback? by clicking the Details link for patches and putting the reviewer's email or bugzilla nick (like :hsivonen) in the requestee field.

Comment 5

[Charles Chan]

(In reply to Josh Matthews [:jdm] from comment #4)
> Comment on attachment 604803 [details] [diff] [review]
> Bug-729049: Patch-2
> 
> For future reference, Charles, you can (and should!) set flags like review?
> and feedback? by clicking the Details link for patches and putting the
> reviewer's email or bugzilla nick (like :hsivonen) in the requestee field.

Sorry, I will keep that in mind next time. :)

Comment 6

[Charles Chan]

Attachment: Bug-729049: Patch-3

Resubmit patch with 8 line context.

Comment 7

[Henri Sivonen (:hsivonen)]

Comment on attachment 605282 [details] [diff] [review]
Bug-729049: Patch-3

>           const PRUnichar *afterEntity;
>+          const PRUnichar **aNext = &afterEntity;

Naming a local variable aFoo is not OK. The aFoo pattern is reserved for arguments. However, a new local variable isn't needed at all here.

>+          *aNext = nsnull;

Instead of this, you could intialize afterEntity to nsnull directly:
const PRUnichar* afterEntity = nsnull;

>+                                  reinterpret_cast<const char**>(aNext),

And then here instead of aNext, you can use &afterEntity.

r- for this iteration, but it will turn to r+ with the above comments addressed.

Thank you.

Comment 8

[Charles Chan]

Thanks Henri, I will address these and re-submit the fix for review.

Comment 9

[Charles Chan]

Attachment: Bug-729049: Patch-4

Fix the issues identified in the code review.

Comment 10

[Charles Chan]

Attachment: Bug-729049: Patch-5

Please ignore the last patch, re-attach the correct file.

Comment 11

[Henri Sivonen (:hsivonen)]

Comment on attachment 605660 [details] [diff] [review]
Bug-729049: Patch-5

>+          PRUnichar result[2];        

There appears to be trailing whitespace on this line.

r=hsivonen with the trailing whitespace from the above line removed.

Thank you.

Comment 12

[Charles Chan]

Attachment: Bug-729049: Patch-6

Fixed the extra spaces.

Comment 13

[Henri Sivonen (:hsivonen)]

Comment on attachment 606113 [details] [diff] [review]
Bug-729049: Patch-6

Looks good. Thanks.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Thanks for the patch and thanks for using mq to make it!
https://hg.mozilla.org/integration/mozilla-inbound/rev/c593e878662a

Comment 15

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/c593e878662a