move most of nsStyleUtil into nsRuleNode.cpp

RESOLVED FIXED in mozilla15

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
14 years ago
5 years ago

People

(Reporter: dbaron, Assigned: Shriram (irc: Mavericks))

Tracking

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

14 years ago
Most of the functions in nsStyleUtil are used only by nsRuleNode.cpp, and should
live there, rather than in a separate class.
(Reporter)

Updated

10 years ago
Assignee: dbaron → nobody
QA Contact: ian → style-system
(Assignee)

Updated

5 years ago
Assignee: nobody → kshriram18
(Reporter)

Comment 1

5 years ago
At the time this bug was filed, nsStyleUtil contained:
 1. a bunch of functions (CalcFontPointSize, FindNextSmallerFontSize, FindNextLargerFontSize) for relative font sizes
 2. two functions (IsHTMLLink, IsSimpleXLink) for link testing

The code for (2) has already been refactored.

This bug is really about moving the functions in (1).

(Also, ConstrainFontWeight looks like it can be removed.)
(Assignee)

Comment 2

5 years ago
http://dxr.lanedo.com/search.cgi?tree=mozilla-central&request_time=1337563515808&noredirect=1&string=nsStyleUtil
shows some other files with functions in nsStyleUtil.cpp

Did you mean to say those specific nsStyleUtil functions(mentioned in #1 in last comment) need to be moved to nsRuleNode.cpp as they are only used by nsRuleNode.cpp?
(Reporter)

Comment 3

5 years ago
They don't *need* to be moved, but yes, this bug is about moving them.
(Assignee)

Comment 4

5 years ago
Created attachment 626023 [details] [diff] [review]
Patch that moves functions in nsStyleUtil into nsRuleNode.cpp

I have removed one function, and moved the rest as per 1. in comment #1 
I tried to check for dependencies in the process, and made a few more changes like removing a forward class declaration from nsStyleUtil , moving enum values etc

Are there any other things to check for while moving code in general?
Attachment #626023 - Flags: feedback?(dbaron)
(Reporter)

Comment 5

5 years ago
Comment on attachment 626023 [details] [diff] [review]
Patch that moves functions in nsStyleUtil into nsRuleNode.cpp

Instead of putting the code at the end of the file, could you put it between ComputeScriptLevelSize and SetFontSizeCalcOps?

Also, two other things here:

>+nscoord
>+nsRuleNode::CalcFontPointSize(PRInt32 aHTMLSize, PRInt32 aBasePointSize,
>+                               nsPresContext* aPresContext,
>+                               nsFontSizeType aFontSizeType)

1) could you change "nscoord" on the first line to "/* static */ nscoord" (and the same for the other functions

2) fix the indentation on the third and fourth line (one space less)

With those changes I think this looks good (assuming that the moved code is just moved).  Could you make those changes and then request review from me?
Attachment #626023 - Flags: feedback?(dbaron) → feedback+
(Reporter)

Comment 6

5 years ago
And did you test that the code compiles and runs?
(Assignee)

Comment 7

5 years ago
Thanks for the feedback David.
I sent it to try, and got the message inserted in the pushlog db successfully but I didn't get an email with a link to it on tbpl. 
I will make the changes you mentioned and send it to try. Would that help?
(Assignee)

Comment 8

5 years ago
Created attachment 627527 [details] [diff] [review]
Patch that moves functions in nsStyleUtil into nsRuleNode.cpp
Attachment #626023 - Attachment is obsolete: true
Attachment #627527 - Flags: review?(dbaron)
(Assignee)

Comment 9

5 years ago
Created attachment 627528 [details] [diff] [review]
Patch that moves functions in nsStyleUtil into nsRuleNode.cpp
Attachment #627527 - Attachment is obsolete: true
Attachment #627527 - Flags: review?(dbaron)
Attachment #627528 - Flags: review?(dbaron)
(Reporter)

Comment 10

5 years ago
Comment on attachment 627528 [details] [diff] [review]
Patch that moves functions in nsStyleUtil into nsRuleNode.cpp

In nsRuleNode.cpp:

>+/* static */ nscoord
>+nsRuleNode::FindNextSmallerFontSize(nscoord aFontSize, PRInt32 aBasePointSize, 
>+                                             nsPresContext* aPresContext,
>+                                             nsFontSizeType aFontSizeType)

Fix the indent of the third and fourth lines above.

>+/* static */ nscoord
>+nsRuleNode::FindNextLargerFontSize(nscoord aFontSize, PRInt32 aBasePointSize, 
>+                                            nsPresContext* aPresContext,
>+                                            nsFontSizeType aFontSizeType)

Same here

>-}
>+}
>\ No newline at end of file

And don't remove the newline at the end of the file.

r=dbaron with that
Attachment #627528 - Flags: review?(dbaron) → review+
(Assignee)

Comment 11

5 years ago
Created attachment 628596 [details] [diff] [review]
Patch that moves functions in nsStyleUtil into nsRuleNode.cpp
Attachment #627528 - Attachment is obsolete: true
Attachment #628596 - Flags: review?(dbaron)
(Reporter)

Comment 12

5 years ago
Comment on attachment 628596 [details] [diff] [review]
Patch that moves functions in nsStyleUtil into nsRuleNode.cpp

r=dbaron
Attachment #628596 - Flags: review?(dbaron) → review+
(Reporter)

Updated

5 years ago
Keywords: checkin-needed
(Reporter)

Comment 13

5 years ago
Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63ed999a6fe9

I tweaked the commit message a little bit.

Thanks for the patch.
Keywords: checkin-needed
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/63ed999a6fe9
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.