Bug 42472

Summary: Implement MathML mfenced element
Product: WebKit Reporter: François Sausset <sausset>
Component: MathMLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, commit-queue, dumi, eric, kenneth, sausset
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Macintosh Intel   
OS: OS X 10.6   
Bug Depends on: 43715    
Bug Blocks: 3251, 33703    
Attachments:
Description Flags
Patch
none
Revised patch
none
Regenerated patch
kenneth: review-, kenneth: commit-queue-
Revised patch
none
Revised patch
none
Regenerated patch
none
Patch
none
Patch
commit-queue: commit-queue-
Updated patch
commit-queue: commit-queue-
Regenerated patch
eric: commit-queue-
Regenerated patch none

Description François Sausset 2010-07-16 11:06:30 PDT
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
Comment 1 François Sausset 2010-07-16 11:18:04 PDT
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.
Comment 2 François Sausset 2010-07-16 11:20:26 PDT
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.
Comment 3 François Sausset 2010-08-03 12:54:53 PDT
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.
Comment 4 François Sausset 2010-08-03 13:24:07 PDT
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.
Comment 5 Alex Milowski 2010-08-04 06:13:37 PDT
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.
Comment 6 François Sausset 2010-08-04 06:19:01 PDT
(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.
Comment 7 Kenneth Rohde Christiansen 2010-08-05 06:00:18 PDT
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 (alex@milowski.com). 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
Comment 8 François Sausset 2010-08-05 08:26:17 PDT
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.
Comment 9 Kenneth Rohde Christiansen 2010-08-05 11:28:23 PDT
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
Comment 10 Kenneth Rohde Christiansen 2010-08-05 11:28:52 PDT
Feel free to fix the issues and commit a patch with the reviewer field already set. Then you set cq?
Comment 11 François Sausset 2010-08-05 15:22:06 PDT
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)
Comment 12 WebKit Commit Bot 2010-08-06 07:12:23 PDT
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
Comment 13 Alex Milowski 2010-08-06 07:17:34 PDT
The project file patch is out of date.  We'll need to update this patch for it to succeed.
Comment 14 François Sausset 2010-08-06 07:19:30 PDT
Created attachment 63721 [details]
Regenerated patch

Generated from latest trunk version to avoid failure with .xcodeproj file.
Comment 15 Antonio Gomes 2010-08-06 08:57:39 PDT
(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?".
Comment 16 François Sausset 2010-08-06 09:01:35 PDT
Created attachment 63726 [details]
Patch

Reviewed by Kenneth Christiansen
Comment 17 Antonio Gomes 2010-08-06 09:04:05 PDT
Comment on attachment 63726 [details]
Patch

Fill his name here:

+2010-08-06  François Sausset  <sausset@gmail.com>
+
+        Reviewed by NOBODY (OOPS!).
+

for both ChangeLogs you touched.
Comment 18 François Sausset 2010-08-06 09:09:19 PDT
Created attachment 63728 [details]
Patch

First time I had to do that.

Thanks for explanations.
Comment 19 WebKit Commit Bot 2010-08-06 10:08:56 PDT
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).
Comment 20 François Sausset 2010-08-06 10:13:11 PDT
Created attachment 63736 [details]
Updated patch

Updated reviewer name.
Comment 21 WebKit Commit Bot 2010-08-06 11:17:32 PDT
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
Comment 22 François Sausset 2010-08-06 11:35:53 PDT
Created attachment 63745 [details]
Regenerated patch

Fix svn failure on .xcodeproj file.
Comment 23 Eric Seidel (no email) 2010-08-08 03:16:40 PDT
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.
Comment 24 Eric Seidel (no email) 2010-08-08 03:16:47 PDT
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.
Comment 25 Eric Seidel (no email) 2010-08-09 00:08:25 PDT
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
Comment 26 François Sausset 2010-08-09 00:15:51 PDT
Created attachment 63865 [details]
Regenerated patch

one more time svn apply failed on the Xcode project file.
Patch regenerated from latest trunk version again.
Comment 27 Eric Seidel (no email) 2010-08-09 00:53:45 PDT
Comment on attachment 63865 [details]
Regenerated patch

Clearing flags on attachment: 63865

Committed r64967: <http://trac.webkit.org/changeset/64967>
Comment 28 Eric Seidel (no email) 2010-08-09 00:53:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Dumitru Daniliuc 2010-08-09 04:22:00 PDT
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.
Comment 30 François Sausset 2010-08-09 04:27:08 PDT
(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)
Comment 31 Alex Milowski 2010-08-09 04:29:12 PDT
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.
Comment 32 Dumitru Daniliuc 2010-08-09 05:23:37 PDT
Looks like r64976 fixed the problem.