Bug 78180 - MathML internals for bug 52444 fix - type checking, PassRefPtr
Summary: MathML internals for bug 52444 fix - type checking, PassRefPtr
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 52444
  Show dependency treegraph
 
Reported: 2012-02-08 16:44 PST by Dave Barton
Modified: 2012-02-09 11:12 PST (History)
3 users (show)

See Also:


Attachments
Patch (16.32 KB, patch)
2012-02-08 16:54 PST, Dave Barton
no flags Details | Formatted Diff | Diff
Patch (19.34 KB, patch)
2012-02-08 17:06 PST, Dave Barton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Barton 2012-02-08 16:44:24 PST
MathML internals for bug 52444 fix - type checking, PassRefPtr
Comment 1 Dave Barton 2012-02-08 16:54:32 PST
Created attachment 126190 [details]
Patch
Comment 2 Dave Barton 2012-02-08 17:01:22 PST
Wait - I didn't pick up the new ChangeLog entry in my patch. Sorry, I will try again.
Comment 3 Dave Barton 2012-02-08 17:06:15 PST
Created attachment 126195 [details]
Patch
Comment 4 Dave Barton 2012-02-08 17:07:37 PST
Wait - I didn't pick up the new ChangeLog entry in my patch. Sorry, I will try again.
Comment 5 Dave Barton 2012-02-08 17:14:01 PST
The second patch (at 17:06 PST) has the ChangeLog entry and can be reviewed. Thanks for your patience. I am learning (I think). :-)
Comment 6 WebKit Review Bot 2012-02-08 18:39:03 PST
Comment on attachment 126195 [details]
Patch

Attachment 126195 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11473004

New failing tests:
platform/chromium/compositing/layout-width-change.html
Comment 7 Alexey Proskuryakov 2012-02-08 20:19:57 PST
Comment on attachment 126195 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126195&action=review

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.h:37
> -    RenderMathMLUnderOver(Node* expression);
> +    RenderMathMLUnderOver(Element*);

Are you sure that names like this one add no useful information?
Comment 8 Dave Barton 2012-02-08 22:44:45 PST
Comment on attachment 126195 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126195&action=review

>> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.h:37
>> +    RenderMathMLUnderOver(Element*);
> 
> Are you sure that names like this one add no useful information?

In all these RenderMathML classes, the argument to the constructor is a MathML element that corresponds to that RenderMathML class. Sometimes the current (old) code calls it "container" or "expression" or even "fraction" (including when it's not a fraction). In this case, the argument is supposed to be an <munder>, <mover>, or <munderover> element. We could call the argument something like underOver, but that name equally applies to the RenderMathMLUnderOver rendering object that's being constructed. I think in reading the code, the context implies munder/mover/munderover, and the clearest thing is to call the munder/mover/munderover element "element", and the rendering object that corresponds to it has some other name ("this" in this case). The important thing for type safety of later casts is that the argument is an Element*, not just a Node*.

I will change the argument name to "expression" if you insist, but it makes the code less clear to me. The RenderMathMLUnderOver object equally represents a mathematical expression. The difference is that one is a DOM element of type Element*, and the other in a rendering object of type RenderMathMLUnderOver*. So I really find the variable name "element" to be clearer than "expression".
Comment 9 Eric Seidel (no email) 2012-02-09 10:01:01 PST
Comment on attachment 126195 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126195&action=review

LGTM.  Keep them coming.  This code needs lots of love.

> Source/WebCore/rendering/mathml/RenderMathMLFenced.cpp:83
> +PassRefPtr<RenderStyle> RenderMathMLFenced::makeOperatorStyle()

This should probably be renamed createOperatorStyle() (to follow the CreateRule, which the ReftPtr (and Mac OS X style docs) talk about.  That doesn't have to be today, or in this patch.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:48
> -RenderMathMLOperator::RenderMathMLOperator(Node* container, UChar operatorChar)
> -    : RenderMathMLBlock(container)
> +RenderMathMLOperator::RenderMathMLOperator(Node* node, UChar operatorChar)

Odd that these took nodes to begin with, since it would never make sense for a Comment or Document node to be involved in a MathML rendering tree.  (Nodes can't even have style, so they couldn't be styled to look like mathtml..)
Comment 10 WebKit Review Bot 2012-02-09 11:12:12 PST
Comment on attachment 126195 [details]
Patch

Clearing flags on attachment: 126195

Committed r107263: <http://trac.webkit.org/changeset/107263>
Comment 11 WebKit Review Bot 2012-02-09 11:12:16 PST
All reviewed patches have been landed.  Closing bug.