Move nsParserService::CheckQName to nsContentUtils

RESOLVED FIXED in mozilla14

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: hsivonen, Assigned: dwarfcrank)

Tracking

(Blocks: 1 bug)

Trunk
mozilla14
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(5 attachments, 4 obsolete attachments)

1.40 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
1.42 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
1.16 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
3.53 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
5.24 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
To make it possible to get rid of nsIParserService and to make code less COMtaminated, move nsParserService::CheckQName to nsContentUtils as a static.
(Reporter)

Updated

6 years ago
Blocks: 729050
(Reporter)

Updated

6 years ago
Whiteboard: [mentor=hsivonen][lang=C++]
(Assignee)

Comment 1

6 years ago
Created attachment 608689 [details] [diff] [review]
Part 1: Move nsParserService::CheckQName to nsContentUtils and change calls within nsContentUtils to use nsContentUtils::CheckQName instead.
Attachment #608689 - Flags: review?(hsivonen)
(Assignee)

Comment 2

6 years ago
Created attachment 608690 [details] [diff] [review]
Part 2: Replace calls to nsParserService::CheckQName with nsContentUtils::CheckQName in nsDocument.cpp
Attachment #608690 - Flags: review?(hsivonen)
(Assignee)

Comment 3

6 years ago
Created attachment 608691 [details] [diff] [review]
Part 3: Replace call to nsParserService::CheckQName with nsContentUtils::CheckQName in nsXULDocument.cpp
Attachment #608691 - Flags: review?(hsivonen)
(Assignee)

Updated

6 years ago
Attachment #608691 - Attachment is patch: true
(Assignee)

Comment 4

6 years ago
Created attachment 608692 [details] [diff] [review]
Part 4: Replace calls to nsParserService::CheckQName with nsContentUtils::CheckQName in txXMLUtils.h
Attachment #608692 - Flags: review?(hsivonen)
(Assignee)

Comment 5

6 years ago
Created attachment 608693 [details] [diff] [review]
Remove nsParserService::CheckQName
Attachment #608693 - Flags: review?(hsivonen)
(Assignee)

Comment 6

6 years ago
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!
Attachment #608689 - Attachment is obsolete: true
Attachment #608704 - Flags: review?(hsivonen)
Attachment #608689 - Flags: review?(hsivonen)
(Assignee)

Comment 7

6 years ago
Created attachment 608706 [details] [diff] [review]
Part 5: Remove nsParserService::CheckQName

Forgot to remove commented-out parts.
Attachment #608693 - Attachment is obsolete: true
Attachment #608693 - Flags: review?(hsivonen)
(Assignee)

Updated

6 years ago
Attachment #608706 - Attachment is patch: true
Attachment #608706 - Flags: review?(hsivonen)
(Reporter)

Comment 8

6 years ago
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.
Attachment #608704 - Flags: review?(hsivonen) → review+
(Reporter)

Updated

6 years ago
Attachment #608690 - Flags: review?(hsivonen) → review+
(Reporter)

Updated

6 years ago
Attachment #608691 - Flags: review?(hsivonen) → review+
(Reporter)

Updated

6 years ago
Attachment #608692 - Flags: review?(hsivonen) → review+
(Reporter)

Updated

6 years ago
Attachment #608706 - Flags: review?(hsivonen) → review+
(Reporter)

Updated

6 years ago
Assignee: nobody → matias.juntunen
Status: NEW → ASSIGNED
(Assignee)

Comment 9

6 years ago
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.
Attachment #608704 - Attachment is obsolete: true
Attachment #609404 - Flags: review?(hsivonen)
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 ('
(Assignee)

Comment 11

6 years ago
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!
Attachment #609404 - Attachment is obsolete: true
Attachment #609404 - Flags: review?(hsivonen)
(Reporter)

Comment 12

6 years ago
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?
Attachment #609409 - Flags: review+
(Assignee)

Comment 13

6 years ago
(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?
(Reporter)

Comment 14

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

Comment 15

6 years ago
(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.
Keywords: checkin-needed
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
Flags: in-testsuite?
Keywords: checkin-needed
Target Milestone: --- → mozilla14
(Reporter)

Comment 17

6 years ago
(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!
Flags: in-testsuite? → in-testsuite-
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 852551
You need to log in before you can comment on or make changes to this bug.