Last Comment Bug 729048 - Move nsParserService::CheckQName to nsContentUtils
: Move nsParserService::CheckQName to nsContentUtils
Status: RESOLVED FIXED
[mentor=hsivonen][lang=C++]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla14
Assigned To: Matias Juntunen (:dwarfcrank)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 852551
Blocks: 729050
  Show dependency treegraph
 
Reported: 2012-02-21 02:33 PST by Henri Sivonen (:hsivonen)
Modified: 2013-03-19 08:17 PDT (History)
3 users (show)
hsivonen: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Move nsParserService::CheckQName to nsContentUtils and change calls within nsContentUtils to use nsContentUtils::CheckQName instead. (5.19 KB, patch)
2012-03-23 07:09 PDT, Matias Juntunen (:dwarfcrank)
no flags Details | Diff | Splinter Review
Part 2: Replace calls to nsParserService::CheckQName with nsContentUtils::CheckQName in nsDocument.cpp (1.40 KB, patch)
2012-03-23 07:10 PDT, Matias Juntunen (:dwarfcrank)
hsivonen: review+
Details | Diff | Splinter Review
Part 3: Replace call to nsParserService::CheckQName with nsContentUtils::CheckQName in nsXULDocument.cpp (1.42 KB, patch)
2012-03-23 07:10 PDT, Matias Juntunen (:dwarfcrank)
hsivonen: review+
Details | Diff | Splinter Review
Part 4: Replace calls to nsParserService::CheckQName with nsContentUtils::CheckQName in txXMLUtils.h (1.16 KB, patch)
2012-03-23 07:12 PDT, Matias Juntunen (:dwarfcrank)
hsivonen: review+
Details | Diff | Splinter Review
Remove nsParserService::CheckQName (3.58 KB, patch)
2012-03-23 07:12 PDT, Matias Juntunen (:dwarfcrank)
no flags Details | Diff | Splinter Review
Part 1: Move nsParserService::CheckQName to nsContentUtils and change calls within nsContentUtils to use nsContentUtils::CheckQName instead. (5.24 KB, patch)
2012-03-23 07:29 PDT, Matias Juntunen (:dwarfcrank)
hsivonen: review+
Details | Diff | Splinter Review
Part 5: Remove nsParserService::CheckQName (3.53 KB, patch)
2012-03-23 07:30 PDT, Matias Juntunen (:dwarfcrank)
hsivonen: review+
Details | Diff | Splinter Review
Part 1: Move nsParserService::CheckQName to nsContentUtils and change calls within nsContentUtils to use nsContentUtils::CheckQName instead. (5.24 KB, patch)
2012-03-26 11:59 PDT, Matias Juntunen (:dwarfcrank)
no flags Details | Diff | Splinter Review
Part 1: Move nsParserService::CheckQName to nsContentUtils and change calls within nsContentUtils to use nsContentUtils::CheckQName instead. (5.24 KB, patch)
2012-03-26 12:08 PDT, Matias Juntunen (:dwarfcrank)
hsivonen: review+
Details | Diff | Splinter Review

Description User image Henri Sivonen (:hsivonen) 2012-02-21 02:33:07 PST
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 User image Matias Juntunen (:dwarfcrank) 2012-03-23 07:09:09 PDT
Created attachment 608689 [details] [diff] [review]
Part 1: Move nsParserService::CheckQName to nsContentUtils and change calls within nsContentUtils to use nsContentUtils::CheckQName instead.
Comment 2 User image Matias Juntunen (:dwarfcrank) 2012-03-23 07:10:14 PDT
Created attachment 608690 [details] [diff] [review]
Part 2: Replace calls to nsParserService::CheckQName with nsContentUtils::CheckQName in nsDocument.cpp
Comment 3 User image Matias Juntunen (:dwarfcrank) 2012-03-23 07:10:59 PDT
Created attachment 608691 [details] [diff] [review]
Part 3: Replace call to nsParserService::CheckQName with nsContentUtils::CheckQName in nsXULDocument.cpp
Comment 4 User image Matias Juntunen (:dwarfcrank) 2012-03-23 07:12:02 PDT
Created attachment 608692 [details] [diff] [review]
Part 4: Replace calls to nsParserService::CheckQName with nsContentUtils::CheckQName in txXMLUtils.h
Comment 5 User image Matias Juntunen (:dwarfcrank) 2012-03-23 07:12:34 PDT
Created attachment 608693 [details] [diff] [review]
Remove nsParserService::CheckQName
Comment 6 User image Matias Juntunen (:dwarfcrank) 2012-03-23 07:29:49 PDT
Created attachment 608704 [details] [diff] [review]
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 User image Matias Juntunen (:dwarfcrank) 2012-03-23 07:30:41 PDT
Created attachment 608706 [details] [diff] [review]
Part 5: Remove nsParserService::CheckQName

Forgot to remove commented-out parts.
Comment 8 User image Henri Sivonen (:hsivonen) 2012-03-26 06:56:57 PDT
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 <matias.juntunen@gmail.com>
>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 User image Matias Juntunen (:dwarfcrank) 2012-03-26 11:59:53 PDT
Created attachment 609404 [details] [diff] [review]
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 User image :Ms2ger (⌚ UTC+1/+2) 2012-03-26 12:02:31 PDT
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 User image Matias Juntunen (:dwarfcrank) 2012-03-26 12:08:35 PDT
Created attachment 609409 [details] [diff] [review]
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 User image Henri Sivonen (:hsivonen) 2012-03-27 02:54:15 PDT
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 User image Matias Juntunen (:dwarfcrank) 2012-03-28 08:46:42 PDT
(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 User image Henri Sivonen (:hsivonen) 2012-03-29 00:02:49 PDT
(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 User image Matias Juntunen (:dwarfcrank) 2012-03-29 06:33:24 PDT
(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 17 User image Henri Sivonen (:hsivonen) 2012-03-30 00:13:14 PDT
(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!

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