Last Comment Bug 729049 - Inline IsXMLLetter, IsXMLNCNameChar, DecodeEntity form nsIParserService
: Inline IsXMLLetter, IsXMLNCNameChar, DecodeEntity form nsIParserService
Status: RESOLVED FIXED
[mentor=hsivonen][lang=C++]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Charles Chan
:
:
Mentors:
Depends on:
Blocks: 729050
  Show dependency treegraph
 
Reported: 2012-02-21 02:35 PST by Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03)
Modified: 2012-03-16 06:30 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Bug-729049: Patch-1 (3.18 KB, patch)
2012-03-10 17:44 PST, Charles Chan
no flags Details | Diff | Splinter Review
Bug-729049: Patch-2 (5.73 KB, patch)
2012-03-11 16:54 PDT, Charles Chan
no flags Details | Diff | Splinter Review
Bug-729049: Patch-3 (8.04 KB, patch)
2012-03-12 19:46 PDT, Charles Chan
hsivonen: review-
Details | Diff | Splinter Review
Bug-729049: Patch-4 (8.02 KB, patch)
2012-03-13 21:46 PDT, Charles Chan
no flags Details | Diff | Splinter Review
Bug-729049: Patch-5 (8.05 KB, patch)
2012-03-13 22:14 PDT, Charles Chan
hsivonen: review+
Details | Diff | Splinter Review
Bug-729049: Patch-6 (8.01 KB, patch)
2012-03-14 21:58 PDT, Charles Chan
hsivonen: review+
Details | Diff | Splinter Review

Description Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2012-02-21 02:35:57 PST
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 2012-03-10 17:44:06 PST
Created attachment 604703 [details] [diff] [review]
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) (Not reading bugmail or doing reviews until 2016-10-03) 2012-03-11 01:11:57 PST
(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 2012-03-11 16:54:53 PDT
Created attachment 604803 [details] [diff] [review]
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] 2012-03-11 20:17:14 PDT
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 2012-03-11 20:31:34 PDT
(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 2012-03-12 19:46:47 PDT
Created attachment 605282 [details] [diff] [review]
Bug-729049: Patch-3

Resubmit patch with 8 line context.
Comment 7 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2012-03-13 11:28:29 PDT
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 2012-03-13 12:34:03 PDT
Thanks Henri, I will address these and re-submit the fix for review.
Comment 9 Charles Chan 2012-03-13 21:46:20 PDT
Created attachment 605654 [details] [diff] [review]
Bug-729049: Patch-4

Fix the issues identified in the code review.
Comment 10 Charles Chan 2012-03-13 22:14:16 PDT
Created attachment 605660 [details] [diff] [review]
Bug-729049: Patch-5

Please ignore the last patch, re-attach the correct file.
Comment 11 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2012-03-14 04:06:43 PDT
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 2012-03-14 21:58:37 PDT
Created attachment 606113 [details] [diff] [review]
Bug-729049: Patch-6

Fixed the extra spaces.
Comment 13 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2012-03-15 00:05:57 PDT
Comment on attachment 606113 [details] [diff] [review]
Bug-729049: Patch-6

Looks good. Thanks.
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-03-15 17:13:38 PDT
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] 2012-03-16 06:30:20 PDT
https://hg.mozilla.org/mozilla-central/rev/c593e878662a

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