729048 - Move nsParserService::CheckQName to nsContentUtils

Status: RESOLVED FIXED

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

Description

[Henri Sivonen (:hsivonen)]

To make it possible to get rid of nsIParserService and to make code less COMtaminated, move nsParserService::CheckQName to nsContentUtils as a static.

Comment 1

[Matias Juntunen (:dwarfcrank)]

Attachment: Part 1: Move nsParserService::CheckQName to nsContentUtils and change calls within nsContentUtils to use nsContentUtils::CheckQName instead.

Comment 2

[Matias Juntunen (:dwarfcrank)]

Attachment: Part 2: Replace calls to nsParserService::CheckQName with nsContentUtils::CheckQName in nsDocument.cpp

Comment 3

[Matias Juntunen (:dwarfcrank)]

Attachment: Part 3: Replace call to nsParserService::CheckQName with nsContentUtils::CheckQName in nsXULDocument.cpp

Comment 4

[Matias Juntunen (:dwarfcrank)]

Attachment: Part 4: Replace calls to nsParserService::CheckQName with nsContentUtils::CheckQName in txXMLUtils.h

Comment 5

[Matias Juntunen (:dwarfcrank)]

Attachment: Remove nsParserService::CheckQName

Comment 6

[Matias Juntunen (:dwarfcrank)]

Attachment: Part 1: Move nsParserService::CheckQName to nsContentUtils and change calls within nsContentUtils to use nsContentUtils::CheckQName instead.

Whoops, accidentally inserted tabs instead of spaces, apologies!

Comment 7

[Matias Juntunen (:dwarfcrank)]

Attachment: Part 5: Remove nsParserService::CheckQName

Forgot to remove commented-out parts.

Comment 8

[Henri Sivonen (:hsivonen)]

Comment on attachment 608704 [details] [diff] [review]
Part 1: Move nsParserService::CheckQName to nsContentUtils and change calls within nsContentUtils to use nsContentUtils::CheckQName instead.

># HG changeset patch
># Parent c20ec27eb0e8ec666a987a6f71a3902257f8128d
># User Matias Juntunen <[e-mail]>
>Bug 729048 - Move nsParserService::CheckQName to nsContentUtils (part 1): Move nsParserService::CheckQName to nsContentUtils and change calls within nsContentUtils to use nsContentUtils::CheckQName instead.

>+                             const PRUnichar** aColon = NULL);

Please use nsnull instead of NULL.

>+  const char* colon = NULL;

Also here.

>+  if (result == 0) {

if (!result) {

r=hsivonen if those nits are fixed.

Comment 9

[Matias Juntunen (:dwarfcrank)]

Attachment: Part 1: Move nsParserService::CheckQName to nsContentUtils and change calls within nsContentUtils to use nsContentUtils::CheckQName instead.

Fixed the issues that were pointed out.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 609404 [details] [diff] [review]
Part 1: Move nsParserService::CheckQName to nsContentUtils and change calls within nsContentUtils to use nsContentUtils::CheckQName instead.

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

::: content/base/src/nsContentUtils.cpp
@@ +2399,5 @@
> +                                 reinterpret_cast<const char*>(end),
> +                                 aNamespaceAware, &colon);
> +
> +  if (!result) {
> +    if(aColon) {

One more nit: 'if ('

Comment 11

[Matias Juntunen (:dwarfcrank)]

Attachment: Part 1: Move nsParserService::CheckQName to nsContentUtils and change calls within nsContentUtils to use nsContentUtils::CheckQName instead.

There, now it should be fine. Sorry for wasting your time with these formatting mistakes!

Comment 12

[Henri Sivonen (:hsivonen)]

Comment on attachment 609409 [details] [diff] [review]
Part 1: Move nsParserService::CheckQName to nsContentUtils and change calls within nsContentUtils to use nsContentUtils::CheckQName instead.

Are you familiar with the landing procedure?

Comment 13

[Matias Juntunen (:dwarfcrank)]

(In reply to Henri Sivonen (:hsivonen) from comment #12)
> Comment on attachment 609409 [details] [diff] [review]
> Part 1: Move nsParserService::CheckQName to nsContentUtils and change calls
> within nsContentUtils to use nsContentUtils::CheckQName instead.
> 
> Are you familiar with the landing procedure?

Somewhat, I have submitted one patch before.

Basically, after the patches have been reviewed and passed the review, one has to ask for someone with the right privileges to add the checkin-needed keyword (after making sure the patches don't break the current tree). After that one waits for it to be eventually pushed into mozilla-incoming. Right?

Comment 14

[Henri Sivonen (:hsivonen)]

(In reply to Matias Juntunen from comment #13)
> Basically, after the patches have been reviewed and passed the review, one
> has to ask for someone with the right privileges to add the checkin-needed
> keyword (after making sure the patches don't break the current tree). After
> that one waits for it to be eventually pushed into mozilla-incoming. Right?

Not quite. You don't need to ask someone else to add "checkin-needed". The patch author typically adds the keyword.

Comment 15

[Matias Juntunen (:dwarfcrank)]

(In reply to Henri Sivonen (:hsivonen) from comment #14)
> Not quite. You don't need to ask someone else to add "checkin-needed". The
> patch author typically adds the keyword.

Ah, okay. It says here: https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch#Committing_the_patch that I should ask someone to add the keyword.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11ef97686043
https://hg.mozilla.org/integration/mozilla-inbound/rev/7af2fcef270f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d8ac6bb09c
https://hg.mozilla.org/integration/mozilla-inbound/rev/db2978f38798
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bbfb954c0dd

Comment 17

[Henri Sivonen (:hsivonen)]

(In reply to Matias Juntunen from comment #15)
> It says here:
> https://developer.mozilla.org/En/Developer_Guide/
> How_to_Submit_a_Patch#Committing_the_patch that I should ask someone to add
> the keyword.

Oh OK. I fixed the wiki. :-)

Thank you for fixing this!

Comment 18

[Ed Morley [:emorley]]

Thank you for the patch - hopefully see you on IRC soon! :-)

https://hg.mozilla.org/mozilla-central/rev/11ef97686043
https://hg.mozilla.org/mozilla-central/rev/7af2fcef270f
https://hg.mozilla.org/mozilla-central/rev/c2d8ac6bb09c
https://hg.mozilla.org/mozilla-central/rev/db2978f38798
https://hg.mozilla.org/mozilla-central/rev/4bbfb954c0dd