219767 - move most of nsStyleUtil into nsRuleNode.cpp

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Most of the functions in nsStyleUtil are used only by nsRuleNode.cpp, and should
live there, rather than in a separate class.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 2

[Shriram (irc: Mavericks)]

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?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

They don't *need* to be moved, but yes, this bug is about moving them.

Comment 4

[Shriram (irc: Mavericks)]

Attachment: 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?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And did you test that the code compiles and runs?

Comment 7

[Shriram (irc: Mavericks)]

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?

Comment 8

[Shriram (irc: Mavericks)]

Attachment: Patch that moves functions in nsStyleUtil into nsRuleNode.cpp

Comment 9

[Shriram (irc: Mavericks)]

Attachment: Patch that moves functions in nsStyleUtil into nsRuleNode.cpp

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 11

[Shriram (irc: Mavericks)]

Attachment: Patch that moves functions in nsStyleUtil into nsRuleNode.cpp

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

r=dbaron

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/63ed999a6fe9