Bug 42472 – Implement MathML mfenced element

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

[François Sausset]

Implement the MathML mfenced element by using code from the "Monster" patch (which should then be closed):
https://bugs.webkit.org/show_bug.cgi?id=33703

[François Sausset]

Created attachment 61830 [details]
Patch

Patch implementing mfenced with addition to new stretchy brackets compared to the base code in https://bugs.webkit.org/show_bug.cgi?id=33703.

[François Sausset]

This patch depends on https://bugs.webkit.org/show_bug.cgi?id=41961 as new layout tests have been generated with STIX fonts.

The patch should be committed only when this bug will be solved.

[François Sausset]

Created attachment 63368 [details]
Revised patch

This patch updates the layout test results to not use STIX fonts to avoid depending on bug 41961.
The previous patch assumed that bug 41961 would have been solved rapidly.
It will finally take more time than expected to solve bug 41961.

[François Sausset]

Created attachment 63372 [details]
Regenerated patch

The previous patch failed to be applied on WebCore.xcodeproj because it was not generated from the latest trunk revision.

[Alex Milowski]

Why does this depend on the STIX fonts?  We had this working with the Apple Symbol font on the mac.  STIX fonts will only make this better.

In fact, we should use STIX when available and fall back to a platform specific font that gives reasonable coverage for the glyphs that are stacked.

[François Sausset]

(In reply to comment #5)
> Why does this depend on the STIX fonts?  We had this working with the Apple Symbol font on the mac.  STIX fonts will only make this better.
> 
> In fact, we should use STIX when available and fall back to a platform specific font that gives reasonable coverage for the glyphs that are stacked.

You are right.

To make things clearer:
- in the first patch I used a build with STIX fonts enabled to generate layout tests results.
- it was a mistake!
- the last patch corrects that and uses Apple Symbol font on the mac.

All layout tests will later be updated to use STIX fonts when bug 41961 will be solved.

[Kenneth Rohde Christiansen]

Comment on attachment 63372 [details]
Regenerated patch

WebCore/mathml/MathMLInlineContainerElement.cpp:48
 +  : MathMLElement(tagName, document)
This seems wrong in accordance with your coding style guide

WebCore/mathml/RenderMathMLFenced.cpp:2
 +   * Copyright (C) 2009 Alex Milowski ([e-mail]). All rights reserved.
Why doesn't this have your copyright?

WebCore/mathml/RenderMathMLFenced.cpp:43
 +      : RenderMathMLRow(fenced), 
, on the start of next line.

WebCore/mathml/RenderMathMLFenced.cpp:44
 +      m_open(0x28), 
0x28? Could we use an enum for these instead?

WebCore/mathml/RenderMathMLFenced.cpp:106
 +          for (Node* position = child->node(); position; position = position->previousSibling()) 
This part would need braces as the contents spans more than one physical line

WebCore/mathml/RenderMathMLFenced.cpp:113
 +              // use the last separator if we've run out of specified separators
Start comments with capital and end with dot.

WebCore/mathml/RenderMathMLFenced.cpp:125
 +      // if we have a block, we'll wrap it in an inline-block
Same here

WebCore/mathml/RenderMathMLFenced.cpp:128
 +          // block objects wrapper
here

WebCore/mathml/RenderMathMLFenced.cpp:145
 +  
why new line here?

WebCore/mathml/RenderMathMLFenced.cpp:151
 +  
and here?

WebCore/mathml/RenderMathMLFenced.cpp:162
 +      
and here?

WebCore/mathml/RenderMathMLFenced.h:40
 +  protected:
PErsonally I like a newline before protected: private: etc

WebCore/mathml/RenderMathMLOperator.cpp:43
 +  m_operator(0)
wrong change

[François Sausset]

Created attachment 63595 [details]
Revised patch

I took into account all style comments.

I added an enum to improve the readability of the code.

About the copyright, the two files implementing mfenced come from an Alex patch (bug 33703) that was divided into smaller ones. This patch is the last part and bug 33703 should be closed after closing this bug.
Appart from styling, I did nothing on that files, so that I did not put my copyright.

[Kenneth Rohde Christiansen]

Comment on attachment 63595 [details]
Revised patch

WebCore/mathml/RenderMathMLFenced.cpp:42
 +  enum Braces {OpeningBraceChar = 0x28, ClosingBraceChar = 0x29};
I would add a space before Opening or put in on a new line.

WebCore/mathml/RenderMathMLFenced.cpp:46
 +      , m_open(OpeningBraceChar)
What does 'open' mean? We normally name things like "isOpen" instead.

WebCore/mathml/RenderMathMLFenced.cpp:55
 +      // FIXME: Handle open/close values with more than one character (they should be treated like text)   
Misses dot at the end :-)

WebCore/mathml/RenderMathMLFenced.cpp:72
 +          // The separator defaults to a single comma
Here as well

WebCore/mathml/RenderMathMLFenced.cpp:92
 +      RenderObject* openFence = new (renderArena()) RenderMathMLOperator(node(), m_open);
Why did you need the m_open? Can't you use the enum directtly instead?

WebCore/mathml/RenderMathMLFenced.cpp:100
 +  void RenderMathMLFenced::addChild(RenderObject* child, RenderObject* )
Remove space after *

WebCore/mathml/RenderMathMLFenced.cpp:107
 +          unsigned int count = 0;
count of?

WebCore/mathml/RenderMathMLFenced.cpp:113
 +          if (count>1) {
Needs spaces around >

WebCore/mathml/RenderMathMLFenced.cpp:130
 +  
Remove this newline

WebCore/mathml/RenderMathMLFenced.cpp:141
 +          
And this one

WebCore/mathml/RenderMathMLFenced.cpp:163
 +      
And this one

WebCore/mathml/RenderMathMLOperator.cpp:115
 +  // FIXME: use fractions of style()->fontSize() for proper zooming/resizing
Add dot at end

[Kenneth Rohde Christiansen]

Feel free to fix the issues and commit a patch with the reviewer field already set. Then you set cq?

[François Sausset]

Created attachment 63645 [details]
Revised patch

Style adjustments.

About m_open, it is storing the character for the opening brace and cannot be replaced by enum as it can be changed by attributes of the mo element (see line 58 in RenderMathMLFenced.cpp)

[WebKit Commit Bot]

Comment on attachment 63645 [details]
Revised patch

Rejecting patch 63645 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kenneth Rohde Christiansen', u'--force']" exit_code: 1
Last 500 characters of output:
ksum
patching file LayoutTests/platform/mac/mathml/presentation/over-expected.txt
patching file LayoutTests/platform/mac/mathml/presentation/row-alignment-expected.checksum
patching file LayoutTests/platform/mac/mathml/presentation/row-alignment-expected.txt
patching file LayoutTests/platform/mac/mathml/presentation/row-expected.checksum
patching file LayoutTests/platform/mac/mathml/presentation/row-expected.txt
patching file LayoutTests/platform/mac/mathml/presentation/tables-expected.checksum

Full output: http://queues.webkit.org/results/3661115

[Alex Milowski]

The project file patch is out of date.  We'll need to update this patch for it to succeed.

[François Sausset]

Created attachment 63721 [details]
Regenerated patch

Generated from latest trunk version to avoid failure with .xcodeproj file.

[Antonio Gomes]

(In reply to comment #14)
> Created an attachment (id=63721) [details]
> Regenerated patch
> 
> Generated from latest trunk version to avoid failure with .xcodeproj file.

Your patch does not need to be re-reviewed. Kenneth has already done that. Please re-upload your patch with "Reviewed by Kenneth Christiansen" and just set "cq?".

[François Sausset]

Created attachment 63726 [details]
Patch

Reviewed by Kenneth Christiansen

[Antonio Gomes]

Comment on attachment 63726 [details]
Patch

Fill his name here:

[phone]  François Sausset  <[e-mail]>
+
+        Reviewed by NOBODY (OOPS!).
+

for both ChangeLogs you touched.

[François Sausset]

Created attachment 63728 [details]
Patch

First time I had to do that.

Thanks for explanations.

[WebKit Commit Bot]

Comment on attachment 63728 [details]
Patch

Rejecting patch 63728 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 63728, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
ction=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=42472&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 63728 from bug 42472.
Kenneth Christiansen found in /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[François Sausset]

Created attachment 63736 [details]
Updated patch

Updated reviewer name.

[WebKit Commit Bot]

Comment on attachment 63736 [details]
Updated patch

Rejecting patch 63736 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 1
Last 500 characters of output:
ksum
patching file LayoutTests/platform/mac/mathml/presentation/over-expected.txt
patching file LayoutTests/platform/mac/mathml/presentation/row-alignment-expected.checksum
patching file LayoutTests/platform/mac/mathml/presentation/row-alignment-expected.txt
patching file LayoutTests/platform/mac/mathml/presentation/row-expected.checksum
patching file LayoutTests/platform/mac/mathml/presentation/row-expected.txt
patching file LayoutTests/platform/mac/mathml/presentation/tables-expected.checksum

Full output: http://queues.webkit.org/results/3664039

[François Sausset]

Created attachment 63745 [details]
Regenerated patch

Fix svn failure on .xcodeproj file.

[Eric Seidel (no email)]

Comment on attachment 63595 [details]
Revised patch

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 63595 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 63645 [details]
Revised patch

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 63645 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 63745 [details]
Regenerated patch

Rejecting patch 63745 from commit-queue.

Failed to run "[u'/Projects/WebKit/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 1
Last 500 characters of output:
ksum
patching file LayoutTests/platform/mac/mathml/presentation/over-expected.txt
patching file LayoutTests/platform/mac/mathml/presentation/row-alignment-expected.checksum
patching file LayoutTests/platform/mac/mathml/presentation/row-alignment-expected.txt
patching file LayoutTests/platform/mac/mathml/presentation/row-expected.checksum
patching file LayoutTests/platform/mac/mathml/presentation/row-expected.txt
patching file LayoutTests/platform/mac/mathml/presentation/tables-expected.checksum

Full output: http://queues.webkit.org/results/3696021

[François Sausset]

Created attachment 63865 [details]
Regenerated patch

one more time svn apply failed on the Xcode project file.
Patch regenerated from latest trunk version again.

[Eric Seidel (no email)]

Comment on attachment 63865 [details]
Regenerated patch

Clearing flags on attachment: 63865

Committed r64967: <http://trac.webkit.org/changeset/64967>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Dumitru Daniliuc]

Your change broke the Windows, because you forgot to edit WebCore.vcproj/WebCore.vcproj. I'll try to fix the problem (it's faster than waiting on webkit-patch to revert the patch...), but next time you add/remove files, please make sure you update all build files. To get the list of all build files, look at a recent change that updated WebCore.xcodeproj/project.pbxproj or WebCore.vcproj/WebCore.vcproj.

Also, it's quite helpful to let these kind of patches go through the EWS bots; unfortunately, the EWS bots won't run an r+'ed patch, so you'll have to ask your reviewers not to r+ it until all EWS bots are happy with it.

[François Sausset]

(In reply to comment #29)
> Your change broke the Windows, because you forgot to edit WebCore.vcproj/WebCore.vcproj. I'll try to fix the problem (it's faster than waiting on webkit-patch to revert the patch...), but next time you add/remove files, please make sure you update all build files. To get the list of all build files, look at a recent change that updated WebCore.xcodeproj/project.pbxproj or WebCore.vcproj/WebCore.vcproj.
> 
> Also, it's quite helpful to let these kind of patches go through the EWS bots; unfortunately, the EWS bots won't run an r+'ed patch, so you'll have to ask your reviewers not to r+ it until all EWS bots are happy with it.

Sorry for the inconvenience, I was not aware of that.
Next time I'll be very careful!
One strange thing is that the EWS bots were green... (apart from failures of svn-apply on .xcodeproj file)

[Alex Milowski]

Curious.  Neither I nor Francois are windows developers (although I do have access to a Windows development environment).

I think this due to MathML now being enabled by default.  We'll have to coordinate this better when we add files.  I'm looking at the Gtk build right now.

[Dumitru Daniliuc]

Looks like r64976 fixed the problem.