Bug 156795

Summary:

Move parsing of mspace attributes to a MathMLSpaceElement class

Product:

WebKit

Reporter:

Frédéric Wang (:fredw) <fred.wang>

Component:

MathML

Assignee:

Frédéric Wang (:fredw) <fred.wang>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

alex, darin

Priority:

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Bug Depends on:

156792

Bug Blocks:

156536

Attachments:

Description	Flags
WIP Patch	none
WIP Patch	none
Patch for EWS testing	none
Patch for EWS	none
Patch	none
Patch	bfulgham: review-
Patch	bfulgham: review+
Patch bug 156792 + 156795 (for EWS testing)	none
Patch	none

Frédéric Wang (:fredw)

Reported 2016-04-20 09:39:04 PDT

Created attachment 276830 [details] WIP Patch This is bug 156536 for the <mspace/> element.

Attachments
WIP Patch (11.00 KB, patch) 2016-04-20 09:39 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
WIP Patch (16.59 KB, patch) 2016-05-17 03:16 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch for EWS testing (21.44 KB, patch) 2016-05-17 05:30 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch for EWS (34.53 KB, patch) 2016-05-17 05:32 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (21.44 KB, patch) 2016-05-17 06:06 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (21.59 KB, patch) 2016-05-18 06:33 PDT, Frédéric Wang (:fredw)	bfulgham: review-	Details Formatted Diff Diff
Patch (21.74 KB, patch) 2016-07-08 07:04 PDT, Frédéric Wang (:fredw)	bfulgham: review+	Details Formatted Diff Diff
Patch bug 156792 + 156795 (for EWS testing) (35.72 KB, patch) 2016-07-11 02:25 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (21.86 KB, patch) 2016-07-11 21:57 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Frédéric Wang (:fredw)

Comment 1 2016-04-23 02:14:58 PDT

Comment on attachment 276830 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276830&action=review > Source/WebCore/rendering/mathml/RenderMathMLSpace.cpp:66 > + spaceElement.depthLength().toUserUnits(style(), m_depth); Note: my plan (not tested yet) was to directly introduce helper function mspaceWidth() mspaceHeight() and mspaceDepth() that would directly call toUserUnits. That way we can just remove m_width, m_height and m_depth members.

Frédéric Wang (:fredw)

Comment 2 2016-05-17 03:16:00 PDT

Created attachment 279108 [details] WIP Patch

Frédéric Wang (:fredw)

Comment 3 2016-05-17 05:30:35 PDT

Created attachment 279113 [details] Patch for EWS testing

Frédéric Wang (:fredw)

Comment 4 2016-05-17 05:32:07 PDT

Created attachment 279114 [details] Patch for EWS

Frédéric Wang (:fredw)

Comment 5 2016-05-17 06:06:02 PDT

Created attachment 279115 [details] Patch

Frédéric Wang (:fredw)

Comment 6 2016-05-18 06:33:13 PDT

Created attachment 279241 [details] Patch

Brent Fulgham

Comment 7 2016-07-07 12:20:57 PDT

Comment on attachment 279241 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279241&action=review This looks good except: (1) I think we could reduce some #include churn if you moved those implementations into the CPP file, and (2) this doesn't apply to the tree at present. Could you please revise and I'll approve it? > Source/WebCore/mathml/MathMLElement.h:66 > + bool dirty = true; These should be in our standard C++11 style: LengthType type { LengthType::ParsingFailed; } float value { 0 }; bool dirty { true }; > Source/WebCore/mathml/MathMLSpaceElement.h:40 > + const Length& depth() { return cachedMathMLLength(MathMLNames::depthAttr, m_depth); } If you moved these into the implementation file, you could probably avoid #including MathMLNames.h in the header file. This would be beneficial to build time. > Source/WebCore/rendering/mathml/RenderMathMLSpace.cpp:-78 > - setNeedsLayoutAndPrefWidthsRecalc(); Yay!

Frédéric Wang (:fredw)

Comment 8 2016-07-08 07:04:21 PDT

Created attachment 283153 [details] Patch Review comments addressed, but this is still blocked by bug 156792.

Frédéric Wang (:fredw)

Comment 9 2016-07-11 02:25:23 PDT

Created attachment 283308 [details] Patch bug 156792 + 156795 (for EWS testing)

Brent Fulgham

Comment 10 2016-07-11 13:48:13 PDT

Comment on attachment 283153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283153&action=review Looks good. r=me after you land the patch this depends on. > Source/WebCore/mathml/MathMLSpaceElement.cpp:39 > + : MathMLElement(tagName, document) It's a little unusual for us to inline constructors like this. Did you measure this as a bottleneck of some kind?

Frédéric Wang (:fredw)

Comment 11 2016-07-11 21:55:14 PDT

(In reply to comment #10) > Comment on attachment 283153 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=283153&action=review > > Looks good. r=me after you land the patch this depends on. > > > Source/WebCore/mathml/MathMLSpaceElement.cpp:39 > > + : MathMLElement(tagName, document) > > It's a little unusual for us to inline constructors like this. Did you > measure this as a bottleneck of some kind? I don't think I did that on purpose, I'll remove the inline keyword (it seems that I copied it from MathMLMathElement which has had it for a long time).

Frédéric Wang (:fredw)

Comment 12 2016-07-11 21:57:01 PDT

Created attachment 283387 [details] Patch

Frédéric Wang (:fredw)

Comment 13 2016-07-11 23:00:46 PDT

Committed r203108: <http://trac.webkit.org/changeset/203108>

Note You need to log in before you can comment on or make changes to this bug.