Bug 40327

Summary: Leftover calls to RenderStyle color accessors, which are no longer public methods.
Product: WebKit Reporter: Clemmitt Sigler <cmsigler>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, darin, eric, hyatt, red47514f7
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 3251    
Attachments:
Description Flags
Patch to get RenderMathMLRoot and RenderMathMLSquareRoot to build after rev. 59956.
levin: review-
Revised patch to get RenderMathMLRoot and RenderMathMLSquareRoot to build after rev. 59956.
levin: review-
Fix to remove the use of color() and fixes a crash in the tests
levin: review+, levin: commit-queue-
Updated patch in response to comments.
darin: review-, darin: commit-queue-
Update patch yet again. none

Clemmitt Sigler
Reported 2010-06-08 13:19:51 PDT
Hi, I just filed Bug 40326, which causes WebCore/mathml/RenderMathMLRoot.cpp and WebCore/mathml/RenderMathMLSquareRoot.cpp to be re-included in the WebKitGtk build process for GtkLauncher. This change has uncovered some stale code that was made invalid by Rev. 59956. That rev. made RenderStyle color accessors private, but there are still a goodly number of calls, e.g. 'style()->color()', that lurk in a number of modules in the tree, including RenderMathMLRoot.cpp and RenderMathMLSquareRoot.cpp. All this stale code needs to be searched out and updated. I don't believe I can make this contribution myself; I don't know the correct method to update the code :^( Rev. 59956 is documented thusly: "Make the RenderStyle color accessors private. This forces callers to use visitedDependentColor instead (or to make the decision to become friends of the RenderStyle class in order to get access to the real style information)." Do all modules having 'style()->color()' calls simply need to declare themselves friends of RenderStyle? Is that simple solution really the correct solution??? Sorry I can't be of more help. Clemmitt
Attachments
Patch to get RenderMathMLRoot and RenderMathMLSquareRoot to build after rev. 59956. (1.78 KB, patch)
2010-06-09 06:12 PDT, Clemmitt Sigler 		levin: review-
DetailsFormatted DiffDiff
Revised patch to get RenderMathMLRoot and RenderMathMLSquareRoot to build after rev. 59956. (1.84 KB, patch)
2010-06-09 10:45 PDT, Clemmitt Sigler 		levin: review-
DetailsFormatted DiffDiff
Fix to remove the use of color() and fixes a crash in the tests (2.78 KB, patch)
2010-06-15 08:15 PDT, Alex Milowski 		levin: review+
levin: commit-queue-
DetailsFormatted DiffDiff
Updated patch in response to comments. (4.16 KB, patch)
2010-06-16 05:43 PDT, Alex Milowski 		darin: review-
darin: commit-queue-
DetailsFormatted DiffDiff
Update patch yet again. (3.17 KB, patch)
2010-06-16 11:19 PDT, Alex Milowski 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Clemmitt Sigler
Comment 1 2010-06-09 05:28:50 PDT
(In reply to comment #0) <snip> > made invalid by Rev. 59956. That rev. made RenderStyle color > accessors private, but there are still a goodly number of calls, > e.g. 'style()->color()', that lurk in a number of modules in the > tree, including RenderMathMLRoot.cpp and RenderMathMLSquareRoot.cpp. <snip> > Do all modules having 'style()->color()' calls simply need to declare > themselves friends of RenderStyle? Is that simple solution really > the correct solution??? OK, I have more of my wits about me this morning. If all existing 'style()->color()' calls are the only instances of offending out-of-date code, then this problem exists only in these three modules in the tree: WebKit/WebCore/rendering/RenderTreeAsText.cpp WebKit/WebCore/mathml/RenderMathMLRoot.cpp WebKit/WebCore/mathml/RenderMathMLSquareRoot.cpp I note that RenderTreeAsText.cpp isn't generating errors because WebCore/rendering/style/RenderStyle.h declares 'friend class RenderTreeAsText', so I'll try exactly the same solution: friend class RenderMathMLRoot friend class RenderMathMLSquareRoot in RenderStyle.h with corresponding FIXME comments like Dave Hyatt did in rev. 59956. Patch coming RSN. HTH. Clemmitt
Clemmitt Sigler
Comment 2 2010-06-09 06:12:38 PDT
Created attachment 58239 [details] Patch to get RenderMathMLRoot and RenderMathMLSquareRoot to build after rev. 59956. Rev. 59956 made color accessors private, which keeps RenderMathMLRoot and RenderMathMLSquareRoot from building. This patch is a fix for that.
David Levin
Comment 3 2010-06-09 08:58:11 PDT
Comment on attachment 58239 [details] Patch to get RenderMathMLRoot and RenderMathMLSquareRoot to build after rev. 59956. > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 60887) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,16 @@ > +2010-06-09 Clemmitt Sigler <cmsigler@gmail.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Add classes RenderMathMLRoot and RenderMathMLSquareRoot as friends > + of class RenderStyle to allow these two to build until rebaselining > + or a "correct" fix is implemented. Please add a bug link here and re-upload.
Clemmitt Sigler
Comment 4 2010-06-09 10:43:53 PDT
(In reply to comment #3) > Please add a bug link here and re-upload. Hi David, and thanks. Working.... Done, patch is against latest rev. 60899. Attaching corrected patch next. HTH. Clemmitt
Clemmitt Sigler
Comment 5 2010-06-09 10:45:45 PDT
Created attachment 58257 [details] Revised patch to get RenderMathMLRoot and RenderMathMLSquareRoot to build after rev. 59956. Revised patch, obsoletes previous one. TIA.
Eric Seidel (no email)
Comment 6 2010-06-12 19:03:20 PDT
Why not just rebaseline the MathML results? do we even ahve any mathml layout test results?
Alex Milowski
Comment 7 2010-06-13 06:20:13 PDT
This doesn't seem like the correct way to update the code. Instead, we should just use: style()->visitedDependentColor(CSSPropertyColor) to replace the use of style()->color()
David Levin
Comment 8 2010-06-14 11:27:27 PDT
Comment on attachment 58257 [details] Revised patch to get RenderMathMLRoot and RenderMathMLSquareRoot to build after rev. 59956. Please consider Alex's suggestion. I guess the better fix is to get rid of the dependency that is causing these to be "friend class". PS I suspect this part of the comment is incorrect "Rebaselining should be performed to remove this."
Alex Milowski
Comment 9 2010-06-15 08:15:41 PDT
Created attachment 58778 [details] Fix to remove the use of color() and fixes a crash in the tests
David Levin
Comment 10 2010-06-15 13:32:14 PDT
Comment on attachment 58778 [details] Fix to remove the use of color() and fixes a crash in the tests > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 61186) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,19 @@ > +2010-06-15 Alex Milowski <alex@milowski.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Fixed a compile error in the paint() methods by making them use the Extra space after them. > + new visitedDependentColor() method. Please add bug link. > + > + Fixed a crash in RenderMathMLRoot with the tests where there were > + bad assumptions about the children in the layout. This would be better as a function comment (for RenderMathMLRoot::layout).
Alex Milowski
Comment 11 2010-06-15 23:13:21 PDT
(In reply to comment #10) > (From update of attachment 58778 [details]) > > Index: WebCore/ChangeLog > > =================================================================== > > --- WebCore/ChangeLog (revision 61186) > > +++ WebCore/ChangeLog (working copy) > > @@ -1,3 +1,19 @@ > > +2010-06-15 Alex Milowski <alex@milowski.com> > > + > > + Reviewed by NOBODY (OOPS!). > > + > > + Fixed a compile error in the paint() methods by making them use the > > Extra space after them. *sigh* Really? > > > + new visitedDependentColor() method. > > Please add bug link. Bug link to what bug? > > > + > > + Fixed a crash in RenderMathMLRoot with the tests where there were > > + bad assumptions about the children in the layout. > > This would be better as a function comment (for RenderMathMLRoot::layout). No, not really. There would be no useful comment to add for either of the two changes that would enhance the understanding of the code relating to the current status and the fix for the above code. Of course, you can always argue for more comments in general in the layout() method, but not relating to the fix for this crash. What mark it review+ when you don't like the ChangeLog entry?
Alex Milowski
Comment 12 2010-06-16 05:43:10 PDT
Created attachment 58889 [details] Updated patch in response to comments.
Alex Milowski
Comment 13 2010-06-16 05:45:23 PDT
(In reply to comment #6) > Why not just rebaseline the MathML results? do we even ahve any mathml layout test results? Yes, there are layout tests for MathML. They all now need to be re-baselined. Recent changes have made the layout change slightly. This patch does not address that. I will submit a different patch with some tweaks and a new baseline.
Darin Adler
Comment 14 2010-06-16 08:13:00 PDT
Comment on attachment 58889 [details] Updated patch in response to comments. > - RenderBlock* container = new (renderArena()) RenderBlock(node()); > + RenderBlock* container = new (renderArena()) RenderMathMLBlock(node()); What does this change have to do with the bug you're fixing? This seems OK to change, but unrelated to the change log or other changes. > > - RenderBlock* container = new (renderArena()) RenderBlock(node()); > + RenderBlock* container = new (renderArena()) RenderMathMLBlock(node()); Ditto. > - toRenderMathMLBlock(current)->style()->setVerticalAlign(BASELINE); > + // FIXME: should we really be setting the vertical align here? > + current->style()->setVerticalAlign(BASELINE); Comment sentence should be capitalized. Also, would be better if the comment said why it's questionable. As it is, the comment just says "should this next line of code exist", which doesn't give context for what your doubt is. > + // Adjust the bottom position of the index. > indexBox->style()->setBottom(Length(radicalHeight + style()->paddingBottom().value(), Fixed)); This comment says the same thing the line of code does. We normally leave those out, and make sure comments say something the code does not say. Patch otherwise looks fine. review- because of the seemingly unrelated changes
Alex Milowski
Comment 15 2010-06-16 08:45:00 PDT
> > Patch otherwise looks fine. review- because of the seemingly unrelated changes Did you look at the comments in this bug? There is a crash caused by this code as well that I am fixing. The previous reviewer denied the review because I documented that in the change log. Now you're denying the patch because I'm including it. Quite untenable.
Darin Adler
Comment 16 2010-06-16 09:26:26 PDT
(In reply to comment #15) > The previous reviewer denied the review because I documented that in the change log. I just read David’s comments; he suggested that the comment from the change log go into a comment in the the source file instead. You disagreed and instead deleted the explanation entirely. That was a mistake. If you strongly felt a comment should not go in the source file, then you should have left it in the change log. Checking in a change with no explanation at all is wrong. You can check with him, but I’m sure David and I both agree on that.
Alex Milowski
Comment 17 2010-06-16 09:47:44 PDT
(In reply to comment #16) > (In reply to comment #15) > > The previous reviewer denied the review because I documented that in the change log. > > I just read David’s comments; he suggested that the comment from the change log go into a comment in the the source file instead. You disagreed and instead deleted the explanation entirely. That was a mistake. > > If you strongly felt a comment should not go in the source file, then you should have left it in the change log. > > Checking in a change with no explanation at all is wrong. You can check with him, but I’m sure David and I both agree on that. I'm just trying to push this all along. I deleted the comment from the change log against my better judgement trying to see if I can get my code changes in so this will compile again. My general experience is that reviewer are very inconsistent in terms of what they expect. So, next I'll submit a patch that you like and someone else will review it and they'll deny it on the basis of the changes you expect. ...and then the cycle starts again.
Darin Adler
Comment 18 2010-06-16 09:56:29 PDT
I’m sorry you’re frustrated. You have a review+ from David Levin, so you could be getting that patch committed. He asked for a three minor, optional tweaks and gave you a review+. I think you’re twisting yourself up in knots for no good reason here. Removing all explanation of the change from the patch was wrong, and that’s why I did review-. Nothing deeper than that. No infinite sequence of reviews required.
Alex Milowski
Comment 19 2010-06-16 11:19:33 PDT
Created attachment 58909 [details] Update patch yet again.
red47514f7
Comment 20 2010-06-17 07:59:11 PDT
*** Bug 40773 has been marked as a duplicate of this bug. ***
Darin Adler
Comment 21 2010-06-17 08:12:08 PDT
Comment on attachment 58909 [details] Update patch yet again. Clearing flags on attachment: 58909 Committed r61326: <http://trac.webkit.org/changeset/61326>
Darin Adler
Comment 22 2010-06-17 08:12:14 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.