Bug 152242

Summary: Move MathMLOperatorDictionary features into a separate module
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, jfernandez
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 152244, 159619    
Attachments:
Description Flags
Patch mrobinson: review+, mrobinson: commit-queue-

Description Frédéric Wang (:fredw) 2015-12-14 01:28:17 PST
The RenderMathMLOperator class contains three independent features:
1) The management of the renderer for operator
2) The search in the MathML operator dictionary
3) The selection/measuring/drawing of size variants & glyph assembly for stretchy operators

It is cleaner and more easily readable/manageable for future improvements to do as in Gecko and split these features into separate modules.

Also having 3) in a separate module will allow to draw radical or longdiv symbols without creating an anonymous renderer.

This bug is about the first step move 2) into its own module.
Comment 1 Frédéric Wang (:fredw) 2015-12-14 01:55:23 PST
Created attachment 267287 [details]
Patch
Comment 2 Martin Robinson 2015-12-14 02:03:35 PST
Comment on attachment 267287 [details]
Patch

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

Looks fine to me, though I do have a few nits about the coding style.

> Source/WebCore/rendering/mathml/MathMLOperatorDictionary.cpp:1105
> +        if (entry != dictionary && (entry-1)->character == textContent)
> +            entry--;
> +        if (entry != dictionary && (entry-1)->character == textContent)
> +            entry--;

The binary operators should have spaces around them, thus: "entry - 1"

> Source/WebCore/rendering/mathml/MathMLOperatorDictionary.cpp:1113
> +    return !(tryBinarySearch<const UChar, UChar>(horizontalOperators, WTF_ARRAY_LENGTH(horizontalOperators), textContent, ExtractKeyHorizontal));

Is the outermost pair of parenthesis necessary here? If not, might be better to remove it.
Comment 3 Frédéric Wang (:fredw) 2015-12-14 03:49:19 PST
Committed r194031: <http://trac.webkit.org/changeset/194031>