Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Inline IsXMLLetter, IsXMLNCNameChar, DecodeEntity form nsIParserService

RESOLVED FIXED in mozilla14

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: hsivonen, Assigned: Charles Chan)

Tracking

(Blocks: 1 bug)

Trunk
mozilla14
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=hsivonen][lang=C++])

Attachments

(1 attachment, 5 obsolete attachments)

IsXMLLetter, IsXMLNCNameChar, DecodeEntity in nsIParserService each have one caller. Inline these methods into their callers to make it possible to zap nsIParserService.
OS: Linux → All
Hardware: x86_64 → All
Blocks: 729050
Whiteboard: [mentor=hsivonen][lang=C++]
(Assignee)

Comment 1

5 years ago
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?

Updated

5 years ago
Assignee: nobody → charles.wh.chan
(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.
(Assignee)

Comment 3

5 years ago
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
Attachment #604703 - Attachment is obsolete: true

Comment 4

5 years ago
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.
Attachment #604803 - Flags: review?(hsivonen)
(Assignee)

Comment 5

5 years ago
(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. :)
(Assignee)

Comment 6

5 years ago
Created attachment 605282 [details] [diff] [review]
Bug-729049: Patch-3

Resubmit patch with 8 line context.
Attachment #604803 - Attachment is obsolete: true
Attachment #605282 - Flags: review?(hsivonen)
Attachment #604803 - Flags: review?(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.
Attachment #605282 - Flags: review?(hsivonen) → review-
(Assignee)

Comment 8

5 years ago
Thanks Henri, I will address these and re-submit the fix for review.
(Assignee)

Comment 9

5 years ago
Created attachment 605654 [details] [diff] [review]
Bug-729049: Patch-4

Fix the issues identified in the code review.
Attachment #605282 - Attachment is obsolete: true
Attachment #605654 - Flags: review?(hsivonen)
(Assignee)

Comment 10

5 years ago
Created attachment 605660 [details] [diff] [review]
Bug-729049: Patch-5

Please ignore the last patch, re-attach the correct file.
Attachment #605654 - Attachment is obsolete: true
Attachment #605660 - Flags: review?(hsivonen)
Attachment #605654 - Flags: review?(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.
Attachment #605660 - Flags: review?(hsivonen) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 606113 [details] [diff] [review]
Bug-729049: Patch-6

Fixed the extra spaces.
Attachment #605660 - Attachment is obsolete: true
Attachment #606113 - Flags: review?(hsivonen)
Comment on attachment 606113 [details] [diff] [review]
Bug-729049: Patch-6

Looks good. Thanks.
Attachment #606113 - Flags: review?(hsivonen) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Thanks for the patch and thanks for using mq to make it!
https://hg.mozilla.org/integration/mozilla-inbound/rev/c593e878662a
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/c593e878662a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.