Last Comment Bug 771904 - Update parsing and demo pages for mtable@align
: Update parsing and demo pages for mtable@align
Status: RESOLVED FIXED
[mentor=fredw][lang=c++]
: dev-doc-complete, helpwanted, student-project
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Mohit Sinha
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-08 06:15 PDT by Frédéric Wang (:fredw)
Modified: 2012-09-29 02:38 PDT (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
changed 'baseline-1' to 'baseline -1' (714 bytes, patch)
2012-07-11 09:29 PDT, Mohit Sinha
no flags Details | Diff | Splinter Review
changed 'baseline-1' to 'baseline -1' (714 bytes, patch)
2012-07-11 09:36 PDT, Mohit Sinha
no flags Details | Diff | Splinter Review
changed 'baseline-1' to 'baseline -1' and 'centre-1' to 'centre -1' (1.26 KB, patch)
2012-07-11 10:23 PDT, Mohit Sinha
fred.wang: review+
karlt: checkin+
Details | Diff | Splinter Review
compressingwhitespace (5.39 KB, patch)
2012-07-12 14:54 PDT, Mohit Sinha
fred.wang: feedback+
Details | Diff | Splinter Review
compressingwhitespace (11.97 KB, patch)
2012-07-13 07:08 PDT, Mohit Sinha
fred.wang: review-
Details | Diff | Splinter Review
compressing whitespace , added test cases for 
 xD; x9; x20 (20.48 KB, patch)
2012-07-14 06:46 PDT, Mohit Sinha
no flags Details | Diff | Splinter Review
compressing whitespace (20.50 KB, patch)
2012-07-14 06:56 PDT, Mohit Sinha
fred.wang: review+
Details | Diff | Splinter Review
compressing whitespace (20.57 KB, patch)
2012-07-19 03:12 PDT, Mohit Sinha
fred.wang: review+
ryanvm: checkin+
Details | Diff | Splinter Review
removed rowline attributes in mtable-align-whitespace.html (20.11 KB, patch)
2012-08-08 16:30 PDT, Mohit Sinha
no flags Details | Diff | Splinter Review

Description Frédéric Wang (:fredw) 2012-07-08 06:15:47 PDT
See http://lists.w3.org/Archives/Public/www-math/2012Jun/0015.html

The syntax for mtable@align is

"\s*(top|bottom|center|baseline|axis)(\s*-?[0-9]+)?\s*"

The parsing is implemented here in Gecko:

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmtableFrame.cpp#271

Note that the author does not seem to assume that there is a space and he is even saying that the lack of separator is a shame. The function essentially calls a "Cut" function to remove the alignment name then calls a "ToInteger" function to compute the rownumber. Apparently this "ToInteger" function ignores the space around the number and so Gecko accepts the syntax with optional spaces between the alignment name and the number as well as optional spaces at the end of the attribute value. But spaces at the beginning of the attribute value are not accepted.

We should also update

http://www.mozilla.org/projects/mathml/demo/mtable.html

to use the correct syntax.
Comment 1 Matt Brubeck (:mbrubeck) 2012-07-10 13:35:59 PDT
Mohit Sinha is interested in working on this bug.
Comment 2 Frédéric Wang (:fredw) 2012-07-11 01:14:51 PDT
(In reply to Matt Brubeck (:mbrubeck) from comment #1)
> Mohit Sinha is interested in working on this bug.

Yes, he contacted me yesterday and I gave me some hints to start with.

Actually, my first comment is wrong, the new syntax is

"\s*(top|bottom|center|baseline|axis)(\s+-?[0-9]+)?\s*"

with mandatory space between the alignment name and the row number. But given that some of the pages may use the former syntax e.g.

http://www.mozilla.org/projects/mathml/demo/mtable.html

it may be better to keep the space between the alignment name and the row number. Anyway "ToInteger" seems to allow space around the row number. So I think the only thing to do is to use "CompressWhitespace" to remove possible the leading space.
Comment 3 Mohit Sinha 2012-07-11 09:29:24 PDT
Created attachment 641085 [details] [diff] [review]
changed 'baseline-1' to 'baseline -1'
Comment 4 Mohit Sinha 2012-07-11 09:36:24 PDT
Created attachment 641089 [details] [diff] [review]
changed 'baseline-1' to 'baseline -1'
Comment 5 Frédéric Wang (:fredw) 2012-07-11 10:16:15 PDT
(In reply to Mohit Sinha from comment #4)
> Created attachment 641089 [details] [diff] [review]
> changed 'baseline-1' to 'baseline -1'

@Mohit: the validator also indicates an error with align="center-1"
http://validator.w3.org/nu/?doc=http%3A%2F%2Fwww.mozilla.org%2Fprojects%2Fmathml%2Fdemo%2Fmtable.html
Comment 6 Mohit Sinha 2012-07-11 10:23:49 PDT
Created attachment 641100 [details] [diff] [review]
changed 'baseline-1' to 'baseline -1' and 'centre-1' to 'centre -1'
Comment 7 Frédéric Wang (:fredw) 2012-07-11 14:54:11 PDT
Comment on attachment 641100 [details] [diff] [review]
changed 'baseline-1' to 'baseline -1' and 'centre-1' to 'centre -1'

Thank you, that looks good.

BTW, I think UI-review is for changes of the Firefox User Interface. For this kind of patch, "normal" review is fine.
Comment 8 Karl Tomlinson (:karlt) 2012-07-11 15:39:38 PDT
Comment on attachment 641100 [details] [diff] [review]
changed 'baseline-1' to 'baseline -1' and 'centre-1' to 'centre -1'

r107332
Comment 9 Mohit Sinha 2012-07-12 14:54:34 PDT
Created attachment 641611 [details] [diff] [review]
compressingwhitespace
Comment 10 Mohit Sinha 2012-07-13 00:44:55 PDT
There are some problems in the reftest with 
layout/reftest/mathml/maction-dynamic-1.html & layout/reftest/mathml/maction-dynamic-1-ref.html

These problems are not due to my patch , but were there from before. 

The visible differences are clearly seen.

Can somebody verify it?
Comment 11 Frédéric Wang (:fredw) 2012-07-13 01:20:17 PDT
Comment on attachment 641611 [details] [diff] [review]
compressingwhitespace

Review of attachment 641611 [details] [diff] [review]:
-----------------------------------------------------------------

For the reftests, maybe to make the comparison clearer I would put the "canonical" syntax "center -3" everywhere in mtable-align-whitespace-ref.html and the other syntax "center-3", "center -3  ", "   center -3" etc in mtable-align-whitespace.html. Also, that would be good to test other characters for whitespaces: " ", "	", "
" "&#xA".

::: layout/mathml/nsMathMLmtableFrame.cpp
@@ +275,1 @@
>  

Can you please replace "top5" by "top 5", "axis-1" by "axis -1" and add the comment

"The REC says that the syntax is '\s*(top|bottom|center|baseline|axis)(\s+-?[0-9]+)?\s*'. The parsing could have been simpler with that syntax, but for backward compatibility we make optional the whitespaces between the alignment name and the row number."?

@@ +288,5 @@
>    aRowIndex = 0;
>    aAlign = eAlign_axis;
>    PRInt32 len = 0;
> +
> +  aValue.CompressWhitespace(true,false);  

There should be a space after the comma (actually Trim will work too, although CompressWhitespace is better if we implement the right syntax later).

Can you please add a comment "We only need to remove leading whitespaces because ToInteger ignores the whitespaces around the number."
Comment 12 Mohit Sinha 2012-07-13 07:08:59 PDT
Created attachment 641860 [details] [diff] [review]
compressingwhitespace

I have changed the comments in nsMathMLmtableFrame.cpp file and after the comma put a whitespace.
I have made changes to the html's.
Comment 13 Frédéric Wang (:fredw) 2012-07-13 07:52:53 PDT
Thanks Mohit.

- Please read https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch again: you're supposed to set the review flag to "?" to ask a review to one of the peer here: https://wiki.mozilla.org/Modules/All#MathML. Alternatively, you can set feedback to "?" is you don't think your patch is finished and just want some feedback.
  
- https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide
  Be sure that line length is 80 characters or less in the comments you added.

- Can you add tests for characters " ", "	", "
" "&#xA"? For example "&#xA
	 center&#xA
	 -3&#xA
	 ".
Comment 14 Mohit Sinha 2012-07-14 06:46:13 PDT
Created attachment 642223 [details] [diff] [review]
compressing whitespace , added test cases for 
 xD; x9; x20
Comment 15 Mohit Sinha 2012-07-14 06:56:43 PDT
Created attachment 642224 [details] [diff] [review]
compressing whitespace

Added test cases for 
 xD; x9; x20 and limited the length of comments per line
Comment 16 Frédéric Wang (:fredw) 2012-07-17 01:05:47 PDT
Comment on attachment 642224 [details] [diff] [review]
compressing whitespace

Review of attachment 642224 [details] [diff] [review]:
-----------------------------------------------------------------

That looks good to me, thanks. Can you please give a better message than "bug 771904 :fix"? I thought the link I gave you explained that, but actually this is described: https://developer.mozilla.org/En/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment. r=me with the commit message updated.

Then, you can use autoland to send your patch to the try server (I find this method somewhat slow, so be patient):
https://wiki.mozilla.org/ReleaseEngineering:Autoland
Comment 17 Mohit Sinha 2012-07-19 03:12:54 PDT
Created attachment 643781 [details] [diff] [review]
compressing whitespace
Comment 18 Frédéric Wang (:fredw) 2012-07-23 05:11:17 PDT
It seems that this autoland request from last Thursday failed. Any idea why?
Comment 19 Frédéric Wang (:fredw) 2012-07-25 02:04:06 PDT
(In reply to Frédéric Wang (:fredw) from comment #18)
> It seems that this autoland request from last Thursday failed. Any idea why?

I pushed the patch to try server, without using autoland:
https://tbpl.mozilla.org/?tree=Try&rev=d01de98565f3
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-07-26 18:23:32 PDT
Comment on attachment 643781 [details] [diff] [review]
compressing whitespace

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd32775559f
Comment 22 Mozilla RelEng Bot 2012-08-07 12:01:59 PDT
Autoland Patchset:
	Patches: 643781
	Branch: mozilla-central => try
Patch 643781 could not be applied to mozilla-central.
patching file layout/mathml/nsMathMLmtableFrame.cpp
Hunk #1 FAILED at 263
1 out of 1 hunks FAILED -- saving rejects to file layout/mathml/nsMathMLmtableFrame.cpp.rej
file layout/reftests/mathml/mtable-align-whitespace-ref.html already exists
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/mathml/mtable-align-whitespace-ref.html.rej
file layout/reftests/mathml/mtable-align-whitespace.html already exists
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/mathml/mtable-align-whitespace.html.rej
patching file layout/reftests/mathml/reftest.list
Hunk #1 FAILED at 90
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/mathml/reftest.list.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.
Comment 23 Mohit Sinha 2012-08-08 16:30:07 PDT
Created attachment 650360 [details] [diff] [review]
removed rowline attributes in mtable-align-whitespace.html
Comment 24 Frédéric Wang (:fredw) 2012-08-09 02:10:00 PDT
Comment on attachment 650360 [details] [diff] [review]
removed rowline attributes in mtable-align-whitespace.html

Thanks Mohit, your patch has already been pushed to mozilla-central and this bug is fixed. You should rather create a new patch which modifies only the reftest pages and attach it to bug 781189.

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