975747 - Convert gOperatorTable to a modern hashtable

Status: RESOLVED FIXED

(Core :: MathML, defect, P4)

Comment 1

[Andrew McCreight [:mccr8]]

Let me know if you have any questions!

Comment 2

[Anuj Agarwal [:anujagarwal464]]

Attachment: removed old hashtable from nsMathMLOperator.cpp

Is this ok?
Do I remove full nsHashtable.h?

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 8391377 [details] [diff] [review]
removed old hashtable from nsMathMLOperator.cpp

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

This looks good to me, aside from the two minor issues I listed.

"Do I remove full nsHashtable.h?"
If you mean the #include, yes, you should remove the include of nsHashtable.h, assuming it compiles.

You should also run the MathML tests locally to make sure everything passes.  I think you want to run these two sets of tests:

./mach mochitest-plain layout/mathml/tests/

./mach crashtest layout/mathml/crashtests/

::: layout/mathml/nsMathMLOperators.cpp
@@ +9,2 @@
>  #include "nsTArray.h"
> +#include "nsHashKeys.h"

this should probably go before nsTArray.h, so the list is alphabetical, aside from nsMathMLOperators.h, which should remain first.

@@ +331,5 @@
>  GetOperatorData(const nsString& aOperator, nsOperatorFlags aForm)
>  {
>    nsAutoString key(aOperator);
>    key.AppendInt(aForm);
> +  return (OperatorData*)gOperatorTable->Get(key);

You don't need the (OperatorData*) cast any more.

Comment 4

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug975747.diff

./mach mochitest-plain layout/mathml/tests/

All test passed.

./mach crashtest layout/mathml/crashtests/

All test passed.

Comment 5

[Frédéric Wang (:fredw)]

(In reply to Anuj Agarwal [:anujagarwal464] from comment #4)
> Created attachment 8391399 [details] [diff] [review]
> bug975747.diff
> 
> ./mach mochitest-plain layout/mathml/tests/
> 
> All test passed.
> 
> ./mach crashtest layout/mathml/crashtests/
> 
> All test passed.

You'll need to run layout/reftests/mathml/ too. I think it would be good to have one test for each operator property as I'm not sure we cover everything in the test suite http://www.w3.org/TR/MathML3/appendixc.html#oper-dict.index.

Comment 6

[Anuj Agarwal [:anujagarwal464]]

(In reply to Frédéric Wang (:fredw) from comment #5)

mach reftest layout/mathml/crashtests/

All test passed.

Comment 7

[Anuj Agarwal [:anujagarwal464]]

(In reply to Frédéric Wang (:fredw) from comment #5)

sorry!

mach reftest layout/reftests/mathml/

All test passed.

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8391399 [details] [diff] [review]
bug975747.diff

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

Thanks for the patch!  This looks good to me, but a MathML peer should review it.

Comment 9

[Frédéric Wang (:fredw)]

(In reply to Anuj Agarwal [:anujagarwal464] from comment #7)
> (In reply to Frédéric Wang (:fredw) from comment #5)
> 
> sorry!
> 
> mach reftest layout/reftests/mathml/
> 
> All test passed.

So the typical ideas for new reftests would be to compare the rendering of operators with and without explicit attribute. The values in the dictionary should be different from the default for
operators that are not in the dictionary (http://www.w3.org/TR/MathML3/chapter3.html#presm.mo.attrs, http://www.w3.org/TR/MathML3/appendixc.html#oper-dict.entries-table). So for example to test lspace/rspace, write a reftest to compare

_<math><mrow><mo form="infix">:</mo></mrow></math>_

against

_<math><mrow><mo form="infix" lspace="veryverythinmathspace" rspace="verythinmathspace">:</mo></mrow></math>_

You can do the same for other operator+infix entries as well as various mo attributes that have a visual effect (stretchy, symmetric, largeop, movablelimits, accent).

Comment 10

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #9)
> operator+infix entries

I meant operator+form

Comment 11

[Andrew McCreight [:mccr8]]

This is just a simple data structure replacement. I don't think we need all new tests here.

Comment 12

[Andrew McCreight [:mccr8]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=90d0ddcfd11b

Comment 13

[Anuj Agarwal [:anujagarwal464]]

Attachment: reftest for operator dictionay

Comment 14

[Frédéric Wang (:fredw)]

Comment on attachment 8391859 [details] [diff] [review]
reftest for operator dictionay

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

I think op-dict-8 ... op-dict-9 can only be one == test that verifies both lspace and rspace at the same time (perhaps for several operators). However "->" is an operator with multiple characters and I don't think our opdict support that at the moment, so please use another operator.

Similarly for op-dict-10, please use another operator that is only made of a single char.

::: layout/reftests/mathml/op-dict-10-ref.html
@@ +5,5 @@
> +</head>
> +<body>
> +	<math>
> +		<mrow>
> +    		<mi>x</mi><mo lspace="veryverythinmathspace"rspace="veryverythickmathspace">-></mo><mi>y</mi>

invalid markup. there should be a space between the attributes.

::: layout/reftests/mathml/op-dict-13.html
@@ +6,5 @@
> +</head>
> +<body>
> +    <math displaystyle="true">
> +    	<munder>
> +        	<mo movablelimits="false">&sum;</mo>

I guess you mean to remove that attribute?

::: layout/reftests/mathml/op-dict-6-ref.html
@@ +3,5 @@
> +<head>
> +    <title>op-dict largeop</title>
> +</head>
> +<body>
> +  	<math>

I think you want to add the displaystyle attribute as in the op-dict-6 and make this a != reftest. The == one for largeop is already op-dict-7

Comment 15

[Anuj Agarwal [:anujagarwal464]]

Attachment: reftest for operator dictionary

Comment 16

[Frédéric Wang (:fredw)]

Comment on attachment 8391922 [details] [diff] [review]
reftest for operator dictionary

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

So if I understand the logic of your tests, each attribute has an != test (to check that the attribute has an effect) and an == test (to test that the value is really picked from the operator dictionary). So

- op-dict-11 should be an == test to test that the op dict value movablelimits="true" is picked from the dictionary.
- there should be an == test for accent to verify the op dict value accent="true" is picked from the dictionary (op-dict-11 is the corresponding != test)
- op-dict-8 should be an == test with the op dict value for lspace/rspace (and op-dict-9 is the corresponding != test). Howevever, ⊢ is again a poor choice, because the lspace/rspace value in the operator dictionary are thickmathspace/thickmathspace which are the same as the default for operators that are not in the dictionary.

Comment 17

[Anuj Agarwal [:anujagarwal464]]

Attachment: reftest for operator dictionary

Comment 18

[Frédéric Wang (:fredw)]

Comment on attachment 8392256 [details] [diff] [review]
reftest for operator dictionary

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

Thanks that looks good with a minor comment below.

::: layout/reftests/mathml/op-dict-8-ref.html
@@ +5,5 @@
> +</head>
> +<body>
> +    <math>
> +        <mrow>
> +            <mo lspace="verythinmathspace">&#x2200;</mo>

The value of &#x2200 in the operator dictionary is lspace="verythinmathspace" rspace="veryverythinmathspace". I think for op-dict-8 and op-dict-9, you should write

<math><mrow><mi>x</mi><mo form="prefix" ...>&#x2200;</mo><mi>y</mi></mrow></math>

so that the spacing around the <mo> will really be visible. At the moment, rspace is not tested since there is nothing on the right of the <mo>.

Comment 19

[Andrew McCreight [:mccr8]]

I'll do a try run with the updated test and then land these patches.

Comment 20

[Anuj Agarwal [:anujagarwal464]]

Attachment: updated tests

@fredw: Thanks for helping in writing reftests.

Comment 21

[Andrew McCreight [:mccr8]]

Anuj, I noticed that your new test files are using DOS line endings.  You should set your editor to use Unix line endings instead.  It looks like there are also some tabs or something in there which are good to avoid.

Also, when you upload a patch, the patch subject or whatever it is called should have a format that looks kind of like
  Bug 975747 - Reftest for MathML operator dictionary. r=fred.wang

That will make it a little easier for other people to land patches for you.  Thanks.

Comment 22

[Frédéric Wang (:fredw)]

Comment on attachment 8392348 [details] [diff] [review]
updated tests

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

> Anuj, I noticed that your new test files are using DOS line endings.  You should set your editor to use Unix line endings instead.  It looks like there are also some tabs or something in there which are good to avoid.

I don't know if I already gave you this link: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style?redirectlocale=en-US&redirectslug=Mozilla_Coding_Style_Guide#General_C.2FC.2B.2B_Practices

::: layout/reftests/mathml/op-dict-8-ref.html
@@ +5,5 @@
> +</head>
> +<body>
> +    <math>
> +        <mrow>
> +        	<mi>x</mi><mo form="prefix" lspace="verythinmathspace">&#x2200;</mo><mi>y</mi>

The rspace attribute  value is still missing. This will only test lspace.

Comment 23

[Anuj Agarwal [:anujagarwal464]]

Attachment: Bug 975747 - Reftest for MathML operator dictionary. r=fred.wang

Comment 24

[Andrew McCreight [:mccr8]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6c34299a99b1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e6bb23cc2294
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9ac4d459a1bd

Thanks for the patches!  This should show up on Nightly in the next day or two.

Comment 25

[Andrew McCreight [:mccr8]]

Oops, that last one is for another bug ( https://hg.mozilla.org/integration/mozilla-inbound/rev/9ac4d459a1bd )

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c34299a99b1
https://hg.mozilla.org/mozilla-central/rev/e6bb23cc2294