Bug 160239 - Move parsing of the form attribute attribute to MathMLOperatorElement
Summary: Move parsing of the form attribute attribute to MathMLOperatorElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on: 160034 160241
Blocks: 156537 160190
  Show dependency treegraph
 
Reported: 2016-07-27 04:47 PDT by Frédéric Wang (:fredw)
Modified: 2016-08-02 04:28 PDT (History)
11 users (show)

See Also:


Attachments
Patch WIP (6.00 KB, patch)
2016-07-27 04:47 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (applies after bugs 160241 and 160245) (21.18 KB, patch)
2016-07-28 02:20 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (applies after bugs 160241 and 160245) (21.05 KB, patch)
2016-07-28 07:50 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (23.96 KB, patch)
2016-08-01 01:45 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (23.94 KB, patch)
2016-08-01 01:47 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (24.06 KB, patch)
2016-08-01 01:58 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (23.94 KB, patch)
2016-08-01 02:06 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.08 MB, application/zip)
2016-08-01 02:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (820.76 KB, application/zip)
2016-08-01 02:56 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (762.11 KB, application/zip)
2016-08-01 03:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (1.48 MB, application/zip)
2016-08-01 03:41 PDT, Build Bot
no flags Details
Patch (24.15 KB, patch)
2016-08-01 03:41 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (24.09 KB, patch)
2016-08-01 04:04 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (24.20 KB, patch)
2016-08-01 06:35 PDT, Frédéric Wang (:fredw)
darin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-yosemite (1.97 MB, application/zip)
2016-08-01 11:09 PDT, Build Bot
no flags Details
Patch (24.75 KB, patch)
2016-08-02 02:56 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (23.78 KB, patch)
2016-08-02 03:32 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (23.80 KB, patch)
2016-08-02 03:57 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2016-07-27 04:47:00 PDT
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.
Comment 1 Frédéric Wang (:fredw) 2016-07-28 02:20:51 PDT
Created attachment 284762 [details]
Patch (applies after bugs 160241 and 160245)
Comment 2 Frédéric Wang (:fredw) 2016-07-28 07:50:54 PDT
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.
Comment 3 Javier Fernandez 2016-07-29 10:46:27 PDT
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.
Comment 4 Frédéric Wang (:fredw) 2016-08-01 01:45:01 PDT
Created attachment 284992 [details]
Patch
Comment 5 Frédéric Wang (:fredw) 2016-08-01 01:47:32 PDT
Created attachment 284993 [details]
Patch
Comment 6 Frédéric Wang (:fredw) 2016-08-01 01:54:48 PDT
(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.
Comment 7 Frédéric Wang (:fredw) 2016-08-01 01:58:30 PDT
Created attachment 284994 [details]
Patch
Comment 8 Frédéric Wang (:fredw) 2016-08-01 02:06:33 PDT
Created attachment 284995 [details]
Patch
Comment 9 Frédéric Wang (:fredw) 2016-08-01 02:18:19 PDT
(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.
Comment 10 Build Bot 2016-08-01 02:41:45 PDT
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
Comment 11 Build Bot 2016-08-01 02:41:49 PDT
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
Comment 12 Build Bot 2016-08-01 02:56:15 PDT
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
Comment 13 Build Bot 2016-08-01 02:56:18 PDT
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
Comment 14 Build Bot 2016-08-01 03:02:55 PDT
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
Comment 15 Build Bot 2016-08-01 03:02:59 PDT
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
Comment 16 Build Bot 2016-08-01 03:40:57 PDT
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
Comment 17 Build Bot 2016-08-01 03:41:02 PDT
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
Comment 18 Frédéric Wang (:fredw) 2016-08-01 03:41:15 PDT
Created attachment 285003 [details]
Patch
Comment 19 Frédéric Wang (:fredw) 2016-08-01 04:04:54 PDT
Created attachment 285004 [details]
Patch
Comment 20 Frédéric Wang (:fredw) 2016-08-01 06:35:14 PDT
Created attachment 285011 [details]
Patch
Comment 21 Frédéric Wang (:fredw) 2016-08-01 06:47:08 PDT
(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.
Comment 22 Darin Adler 2016-08-01 10:47:57 PDT
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.
Comment 23 Build Bot 2016-08-01 11:09:26 PDT
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.
Comment 24 Build Bot 2016-08-01 11:09:32 PDT
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
Comment 25 Frédéric Wang (:fredw) 2016-08-02 02:56:47 PDT
Created attachment 285090 [details]
Patch
Comment 26 Frédéric Wang (:fredw) 2016-08-02 03:32:25 PDT
Created attachment 285093 [details]
Patch
Comment 27 Frédéric Wang (:fredw) 2016-08-02 03:57:10 PDT
Created attachment 285095 [details]
Patch
Comment 28 Frédéric Wang (:fredw) 2016-08-02 04:28:46 PDT
Committed r204022: <http://trac.webkit.org/changeset/204022>