Last Comment Bug 219767 - move most of nsStyleUtil into nsRuleNode.cpp
: move most of nsStyleUtil into nsRuleNode.cpp
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Shriram (irc: Mavericks) Away
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-09-19 22:17 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2012-05-31 05:51 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch that moves functions in nsStyleUtil into nsRuleNode.cpp (29.72 KB, patch)
2012-05-22 08:27 PDT, Shriram (irc: Mavericks) Away
dbaron: feedback+
Details | Diff | Review
Patch that moves functions in nsStyleUtil into nsRuleNode.cpp (30.46 KB, patch)
2012-05-26 19:54 PDT, Shriram (irc: Mavericks) Away
no flags Details | Diff | Review
Patch that moves functions in nsStyleUtil into nsRuleNode.cpp (30.41 KB, patch)
2012-05-26 20:03 PDT, Shriram (irc: Mavericks) Away
dbaron: review+
Details | Diff | Review
Patch that moves functions in nsStyleUtil into nsRuleNode.cpp (30.10 KB, patch)
2012-05-30 21:50 PDT, Shriram (irc: Mavericks) Away
dbaron: review+
Details | Diff | Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-09-19 22:17:34 PDT
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-7 (review requests must explain patch) 2012-05-20 09:03:32 PDT
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) Away 2012-05-20 18:37:53 PDT
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-7 (review requests must explain patch) 2012-05-21 10:22:47 PDT
They don't *need* to be moved, but yes, this bug is about moving them.
Comment 4 Shriram (irc: Mavericks) Away 2012-05-22 08:27:41 PDT
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?
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-05-24 12:18:03 PDT
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-7 (review requests must explain patch) 2012-05-24 12:18:45 PDT
And did you test that the code compiles and runs?
Comment 7 Shriram (irc: Mavericks) Away 2012-05-24 18:15:58 PDT
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) Away 2012-05-26 19:54:11 PDT
Created attachment 627527 [details] [diff] [review]
Patch that moves functions in nsStyleUtil into nsRuleNode.cpp
Comment 9 Shriram (irc: Mavericks) Away 2012-05-26 20:03:58 PDT
Created attachment 627528 [details] [diff] [review]
Patch that moves functions in nsStyleUtil into nsRuleNode.cpp
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-05-30 21:18:49 PDT
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) Away 2012-05-30 21:50:49 PDT
Created attachment 628596 [details] [diff] [review]
Patch that moves functions in nsStyleUtil into nsRuleNode.cpp
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-05-30 21:52:06 PDT
Comment on attachment 628596 [details] [diff] [review]
Patch that moves functions in nsStyleUtil into nsRuleNode.cpp

r=dbaron
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-05-30 22:22:23 PDT
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] 2012-05-31 05:51:22 PDT
https://hg.mozilla.org/mozilla-central/rev/63ed999a6fe9

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