Move parsing of the form attribute attribute to MathMLOperatorElement

• Component: MathML
• Status: RESOLVED FIXED

[Frédéric Wang (:fredw)]

Created attachment 284686 [details]
Patch WIP

Another step for bug 156537. Attached is a WIP patch, but we need to parse the text content in the MathMLOperatorElement class first to handle the fallback when no explicit attributes are provided.

[Frédéric Wang (:fredw)]

Created attachment 284762 [details]
Patch (applies after bugs 160241 and 160245)

[Frédéric Wang (:fredw)]

Created attachment 284782 [details]
Patch (applies after bugs 160241 and 160245)

Just some minor renaming as the leading/trailing functions are really for the default spacing not the one specified in the attributes.

[Javier Fernandez]

Comment on attachment 284782 [details]
Patch (applies after bugs 160241 and 160245)

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

> Source/WebCore/ChangeLog:3
> +        Move parsing of the form attribute attribute to MathMLOperatorElement

nit: duplicated "attribute" term.

> Source/WebCore/mathml/MathMLOperatorDictionary.cpp:1117
> +    if (const Entry* entry = getEntry(character, static_cast<Form>(form)))

Use 'auto' here since the type¡ is well known.
Also, why "const" ?
I don't get why we need the static_cast<Form>, since the form argument is already of such type.

> Source/WebCore/mathml/MathMLOperatorDictionary.cpp:1124
> +    if (const Entry* entry = getEntry(character))

Use 'auto' here.

I think the code will be simpler if getEntry return an Optional<Entry>

> Source/WebCore/mathml/MathMLOperatorElement.cpp:76
> +    const AtomicString& value = attributeWithoutSynchronization(formAttr);

We can use just "auto& value" here.

> Source/WebCore/mathml/MathMLOperatorElement.cpp:125
> +    return space;

we could do it simpler

  return { LengthType::MathUnit, (float)dictionaryProperty().leadingSpaceInMathUnit };

> Source/WebCore/mathml/MathMLOperatorElement.cpp:133
> +    return space;

Ditto.

> Source/WebCore/mathml/MathMLOperatorElement.cpp:146
> +        m_dictionaryProperty.dirty = true;

I'm not sure if it may happen, but I assume m_dictionaryProperty.dirty could be already dirty, so this line does nothing useful. 
Do you think it'd be better to do this instead ?

  m_dictionaryProperty.dirty |= name == formAttr;

> Source/WebCore/rendering/mathml/RenderMathMLFencedOperator.cpp:-29
> -

Do we need to remove this line ?

> Source/WebCore/rendering/mathml/RenderMathMLFencedOperator.cpp:59
> +    m_leadingSpace = 5 * style().fontCascade().size() / 18; // This sets leading space to "thickmathspace".

Perhaps it'd be a good idea to use named constants for these values.

> Source/WebCore/rendering/mathml/RenderMathMLFencedOperator.cpp:60
> +    m_trailingSpace = 5 * style().fontCascade().size() / 18; // This sets trailing space to "thickmathspace".

ditto.

> Source/WebCore/rendering/mathml/RenderMathMLFencedOperator.cpp:62
> +    m_maxSize = intMaxForLayoutUnit; // This sets maxsize to "infinity".

It seems m_maxSize is always intMaxForLayoutUnit. Perhaps we could just define it as a static constant and initialize it in the class.

> Source/WebCore/rendering/mathml/RenderMathMLFencedOperator.cpp:66
> +        m_leadingSpace = entry.value().lspace * style().fontCascade().size() / 18;

It looks like style().fontCascade().size() / 18 is a value we use several times in this class. Perhaps we can cached it somewhere and avoid repeating the same arithmetic operation.

> Source/WebCore/rendering/mathml/RenderMathMLFencedOperator.cpp:67
> +        m_trailingSpace = entry.value().rspace * style().fontCascade().size() / 18;

Ditto.

[Frédéric Wang (:fredw)]

Created attachment 284992 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 284993 [details]
Patch

[Frédéric Wang (:fredw)]

(In reply to comment #3)
> nit: duplicated "attribute" term.

Fixed.

> > Source/WebCore/mathml/MathMLOperatorDictionary.cpp:1117
> > +    if (const Entry* entry = getEntry(character, static_cast<Form>(form)))
> 
> Use 'auto' here since the type¡ is well known.
> Also, why "const" ?
> I don't get why we need the static_cast<Form>, since the form argument is
> already of such type.
> 
> > Source/WebCore/mathml/MathMLOperatorDictionary.cpp:1124
> > +    if (const Entry* entry = getEntry(character))
> 
> Use 'auto' here.
> 
> I think the code will be simpler if getEntry return an Optional<Entry>

There are some constraints due to using the tryBinarySearch on the const dictionary array. I've tried simplifying this code in the new patch.

> We can use just "auto& value" here.

Done.

> we could do it simpler
> 
>   return { LengthType::MathUnit,
> (float)dictionaryProperty().leadingSpaceInMathUnit };

Done

> > Source/WebCore/mathml/MathMLOperatorElement.cpp:146
> > +        m_dictionaryProperty.dirty = true;
> 
> I'm not sure if it may happen, but I assume m_dictionaryProperty.dirty could
> be already dirty, so this line does nothing useful. 
> Do you think it'd be better to do this instead ?
> 
>   m_dictionaryProperty.dirty |= name == formAttr;

Yes, it happens each time one modifies the form attribute two or more times. This is a point of having the dirty boolean to avoid calling parsing each time the attribute changes. I'm not sure such optimization is worth doing, probably the compilers will be clever enough to handle them. Moreover, this code will have several "if else" in follow-up patches so I'm not sure this will be possible / readable.

> 
> > Source/WebCore/rendering/mathml/RenderMathMLFencedOperator.cpp:-29
> > -
> 
> Do we need to remove this line ?

I thought the check-webkit-style complained about it but apparently not.

> 
> > Source/WebCore/rendering/mathml/RenderMathMLFencedOperator.cpp:59
> > +    m_leadingSpace = 5 * style().fontCascade().size() / 18; // This sets leading space to "thickmathspace".
> 
> Perhaps it'd be a good idea to use named constants for these values.

OK, I've use toUserUnits and a gThickMathSpace constant.

> > Source/WebCore/rendering/mathml/RenderMathMLFencedOperator.cpp:62
> > +    m_maxSize = intMaxForLayoutUnit; // This sets maxsize to "infinity".
> 
> It seems m_maxSize is always intMaxForLayoutUnit. Perhaps we could just
> define it as a static constant and initialize it in the class.

I've init it in the constructor. For this and minsize I'm not sure it's worth doing too much thing at this point. It will be much easier to simplify after the refactoring of the length attributes (see bug 160301 and bug 160336).

> 
> > Source/WebCore/rendering/mathml/RenderMathMLFencedOperator.cpp:66
> > +        m_leadingSpace = entry.value().lspace * style().fontCascade().size() / 18;
> 
> It looks like style().fontCascade().size() / 18 is a value we use several
> times in this class. Perhaps we can cached it somewhere and avoid repeating
> the same arithmetic operation.

OK, I've used toUserUnits here too.

[Frédéric Wang (:fredw)]

Created attachment 284994 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 284995 [details]
Patch

[Frédéric Wang (:fredw)]

(In reply to comment #3)
> > Source/WebCore/mathml/MathMLOperatorElement.cpp:125
> > +    return space;
> 
> we could do it simpler
> 
>   return { LengthType::MathUnit,
> (float)dictionaryProperty().leadingSpaceInMathUnit };
> 

For some reason, the GTK bot complained about the missing dirty member so I had to make it explicit.

[Build Bot]

Comment on attachment 284995 [details]
Patch

Attachment 284995 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1790126

New failing tests:
mathml/presentation/fenced.html
mathml/presentation/fenced-mi.html

[Build Bot]

Created attachment 284996 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 284995 [details]
Patch

Attachment 284995 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1790176

New failing tests:
mathml/presentation/fenced.html
mathml/presentation/fenced-mi.html

[Build Bot]

Created attachment 284998 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 284995 [details]
Patch

Attachment 284995 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1790177

New failing tests:
mathml/presentation/fenced.html
mathml/presentation/fenced-mi.html

[Build Bot]

Created attachment 284999 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5

[Build Bot]

Comment on attachment 284995 [details]
Patch

Attachment 284995 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1790359

New failing tests:
mathml/presentation/fenced.html
mathml/presentation/fenced-mi.html

[Build Bot]

Created attachment 285002 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Frédéric Wang (:fredw)]

Created attachment 285003 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 285004 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 285011 [details]
Patch

[Frédéric Wang (:fredw)]

(In reply to comment #9)
> (In reply to comment #3)
> > > Source/WebCore/mathml/MathMLOperatorElement.cpp:125
> > > +    return space;
> > 
> > we could do it simpler
> > 
> >   return { LengthType::MathUnit,
> > (float)dictionaryProperty().leadingSpaceInMathUnit };
> > 
> 
> For some reason, the GTK bot complained about the missing dirty member so I
> had to make it explicit.

It seems that GTK bot has random build errors with that syntax and the Win one is not happy either, so for now I'll keep the more verbose syntax.

[Darin Adler]

Comment on attachment 285011 [details]
Patch

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

> Source/WebCore/mathml/MathMLOperatorDictionary.cpp:1095
> +        return Optional<Entry>();

Should use Nullopt or { }.

> Source/WebCore/mathml/MathMLOperatorDictionary.cpp:1099
> +        return Optional<Entry>(*entry);

Should not need a typecast at all here.

> Source/WebCore/mathml/MathMLOperatorDictionary.cpp:1102
> +        return Optional<Entry>();

Should use Nullopt or { }.

> Source/WebCore/mathml/MathMLOperatorDictionary.cpp:1112
> +        return Optional<Entry>(*entry);

Should not need a typecast here.

> Source/WebCore/mathml/MathMLOperatorDictionary.cpp:1115
> +    return Optional<Entry>();

Should use Nullopt or { }.

> Source/WebCore/mathml/MathMLOperatorElement.cpp:71
> +const MathMLOperatorElement::DictionaryProperty& MathMLOperatorElement::dictionaryProperty()

I think it might be slightly better to factor the part that computes the value from an attribute into a separate function. I’d like this:

    If (!m_dictionaryProperty)
        m_dictionaryProperty = computeDictionaryProperty();
    return m_dictionaryProperty.value();

> Source/WebCore/mathml/MathMLOperatorElement.cpp:79
> +        m_dictionaryProperty.value().form = Prefix;

Does this work if m_dictionaryProperty is null? I guess it constructs a default empty value and then assigns form.

> Source/WebCore/mathml/MathMLOperatorElement.cpp:95
> +    if (Optional<Entry> entry = search(operatorText(), m_dictionaryProperty.value().form, explicitForm)) {

I’d suggest using auto here instead of Optional<Entry>.

> Source/WebCore/mathml/MathMLOperatorElement.cpp:138
>      m_operatorText = Optional<UChar>();
> +    m_dictionaryProperty = Optional<DictionaryProperty>();

More Nullopt would be better, I think.

[Build Bot]

Comment on attachment 285011 [details]
Patch

Attachment 285011 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1792991

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 285021 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Frédéric Wang (:fredw)]

Created attachment 285090 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 285093 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 285095 [details]
Patch

[Frédéric Wang (:fredw)]

Committed r204022: <http://trac.webkit.org/changeset/204022>