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

Description Clemmitt Sigler 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
Comment 1 Clemmitt Sigler 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
Comment 2 Clemmitt Sigler 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.
Comment 3 David Levin 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.
Comment 4 Clemmitt Sigler 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
Comment 5 Clemmitt Sigler 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.
Comment 6 Eric Seidel (no email) 2010-06-12 19:03:20 PDT
Why not just rebaseline the MathML results?  do we even ahve any mathml layout test results?
Comment 7 Alex Milowski 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()
Comment 8 David Levin 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."
Comment 9 Alex Milowski 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
Comment 10 David Levin 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).
Comment 11 Alex Milowski 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?
Comment 12 Alex Milowski 2010-06-16 05:43:10 PDT
Created attachment 58889 [details]
Updated patch in response to comments.
Comment 13 Alex Milowski 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.
Comment 14 Darin Adler 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
Comment 15 Alex Milowski 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.
Comment 16 Darin Adler 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.
Comment 17 Alex Milowski 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.
Comment 18 Darin Adler 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.
Comment 19 Alex Milowski 2010-06-16 11:19:33 PDT
Created attachment 58909 [details]
Update patch yet again.
Comment 20 red47514f7 2010-06-17 07:59:11 PDT
*** Bug 40773 has been marked as a duplicate of this bug. ***
Comment 21 Darin Adler 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>
Comment 22 Darin Adler 2010-06-17 08:12:14 PDT
All reviewed patches have been landed.  Closing bug.