Bug 42472

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.

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.

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.

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.

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.

(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 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

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 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

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

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 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

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

Created attachment 63721 [details] Regenerated patch Generated from latest trunk version to avoid failure with .xcodeproj file.

(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?".

Created attachment 63726 [details] Patch Reviewed by Kenneth Christiansen

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.

Created attachment 63728 [details] Patch First time I had to do that. Thanks for explanations.

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).

Created attachment 63736 [details] Updated patch Updated reviewer name.

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

Created attachment 63745 [details] Regenerated patch Fix svn failure on .xcodeproj file.

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 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 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

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 on attachment 63865 [details] Regenerated patch Clearing flags on attachment: 63865 Committed r64967: <http://trac.webkit.org/changeset/64967>

All reviewed patches have been landed. Closing bug.

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.

(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)

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.

Looks like r64976 fixed the problem.