563365 - (LookupOperator) nsMathMLOperators::LookupOperators retrieves only the first form found

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

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

Attachment: Fix nsMathMLOperators::LookupOperator

This issue is extracted from bug 524275.

nsMathMLOperators::LookupOperators is used to here:
http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmoFrame.cpp#199

and the function calls nsMathMLOperators::LookupOperator (without s) with parameter Flag=0:

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLOperators.cpp#510

The function expects that "gOperatorFound contains all its variants", but it is not what nsMathMLOperators::LookupOperator does.

Comment 1

[Karl Tomlinson (:karlt)]

IIUC nsMathMLmoFrame::ProcessTextData causes all forms to use the same default for NS_MATHML_OPERATOR_ACCENT and NS_MATHML_OPERATOR_MOVABLELIMITS, and yet it feels to need to search all forms to see if these values are set on any of them.

That seems less than ideal.  Is it possible to get the form so as to get the right default values, or only set these parts of mFlags when the form is known?

What's happening for NS_MATHML_OPERATOR_IS_MUTABLE makes a little more sense because that seems to really mean "might-be-stretchy-or-largeop", but even for that I would expect that the value could be obtained using the correct form?

Comment 2

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

Most of the characteristics of the operator are set according to attributes or according to the operator dictionary. For the former, we need to wait until we process attributes and for the latter we need to determine the form before. The form is itself based on a "form" attribute or on the position in an mrow. But this position is the one of the whole embellished operator while the attribute & dictionary are based on the core embellished operator. So this seems quite complicated and I'm not sure that what we are doing.

I wonder why we need NS_MATHML_OPERATOR_ACCENT and NS_MATHML_OPERATOR_MOVABLELIMITS. It seems that we should wait to have determined the form before setting/using any accent or movablelimits value.

For NS_MATHML_OPERATOR_IS_MUTABLE, I think it's correct to look to all the possibilities.

Comment 3

[Karl Tomlinson (:karlt)]

I get the impression that the only reason for using
NS_MATHML_OPERATOR_IS_MUTABLE during frame initialization is for
the -moz-math-stretchy pseudo-element.

http://hg.mozilla.org/mozilla-central/annotate/d6d331e05fca/layout/mathml/mathml.css#l372

That is no longer useful in author stylesheets because -moz pseudo-elements
are no longer processed in author stylesheets.  We probably could re-enable
that, but I don't think there's any reason to do so.

That means that the only function of the -moz-math-stretchy pseudo-element is
to ignore document font-style and font-family for nsMathMLChars.  I'm guessing
that setting the font-style to normal could be done in nsMathMLChar, and I'm
not sure we want to completely ignore document font-family.

If we don't need to call LookupOperators (with the s) in
nsMathMLmoFrame::ProcessTextData, then we don't need the function at all.

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 443113 [details] [diff] [review]
Fix nsMathMLOperators::LookupOperator

However, if you'd prefer to just fixup LookupOperators for now, then that is
an option.

I'm not fond of using global variables in the way that gOperatorFound is used
because it leads to bugs such as this, so I'd prefer that this was fixed in a
way that doesn't use gOperatorFound.

How about adding an internal function

  static OperatorData*
  GetOperatorData(const nsString& aOperator, nsOperatorFlags aForm)

that just looks up the single operator/form in the table (without the fallback
that LookupOperator does), returning NULL if not found?
Then LookupOperator and LookupOperators can just use GetOperatorData directly,
and gOperatorFound can be removed.

Comment 5

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

> However, if you'd prefer to just fixup LookupOperators for now, then that is
> an option.

Yes, I prefer not to change too many things in the stretching mechanism for now. I just want to solve the bug with operators that have stretchy and non-stretchy forms. To do this, we must determine NS_MATHML_OPERATOR_MUTABLE from "allFlags" not only the first form found.

For NS_MATHML_OPERATOR_ACCENT and NS_MATHML_OPERATOR_MOVABLELIMITS, I still don't know why they are set during frame initialization. And so I don't know if we should set them using "allFlags" or the first form found. Apparently, the intention was to use "allFlags", so maybe we can do this change?

> I'm not fond of using global variables in the way that gOperatorFound is used
> because it leads to bugs such as this, so I'd prefer that this was fixed in a
> way that doesn't use gOperatorFound.

Yes, I think you're right, gOperatorFound should not be used. I'll try to do that.

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to comment #5)
> For NS_MATHML_OPERATOR_ACCENT and NS_MATHML_OPERATOR_MOVABLELIMITS, I still
> don't know why they are set during frame initialization. And so I don't know if
> we should set them using "allFlags" or the first form found. Apparently, the
> intention was to use "allFlags", so maybe we can do this change?

Yes, I think that's fine.  I was really commenting because it looked like we didn't need LookupOperators.

IIUC NS_MATHML_OPERATOR_ACCENT is only going to have an effect when the operator exists on its own anyway.  (That is really infix but currently our code will sometimes choose postfix.)

I don't really know about NS_MATHML_OPERATOR_MOVABLELIMITS but using the first form found is not right.  And, as you say, the intention was that all forms would be checked.

Comment 7

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

Attachment: Patch v.2

nsMathMLOperators::LookupOperator (without s) was used in three places:

* In nsMathMLmfencedFrame.cpp, function GetCharSpacing, which is called in
  *  nsMathMLmfencedFrame::ReflowChar, called in nsMathMLmfencedFrame::Reflow with INFIX, POSTIX and PREFIX forms.
  *  GetMaxCharWidth, called in nsMathMLmfencedFrame::GetIntrinsicWidth with INFIX, POSTIX and PREFIX forms.

* In nsMathMLmoFrame.cpp, function nsMathMLmoFrame::ProcessOperatorData, with INFIX, POSTIX or PREFIX forms.

* In nsMathMLOperators::LookupOperators, but the patch now directly uses GetOperatorData

so I've removed the possibility to pass aForm = 0 to retrieve all the forms (anyway it did not work).

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 445171 [details] [diff] [review]
Patch v.2

Nice, thank you.

Comment 9

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/56a43ab16cf5