This is the next step after bug 519126 and is needed to fix bug 414277. I've studied the code of nsMathMLOperator/nsMathMLChar. Here is the changes I suggest: nsMathMLOperator - remove gStretchyOperatorArray - remove CountStretchyOperator() - remove DisableStretchyOperatorAt(PRInt32 aIndex) -replace static PRInt32 FindStretchyOperator(PRUnichar aOperator) by static OperatorData* FindOperatorData(PRUnichar aOperator) - replace static nsStretchDirection GetStretchyDirectionAt(PRInt32 aIndex) by nsStretchDirection GetStretchDirection() nsMathMLChar - replace PRInt32 mOperator by OperatorData* mOperator - nsMathMLOperators::GetStretchyDirectionAt(mOperator); can be written mOperator->GetStretchDirection(); - CountStretchyOperator and DisableStretchyOperatorAt should be removable

There are two "other improvements" that I thought when reading the code: 1) Often, a table of size 4 is used to store properties for infix, postfix and prefix forms (gOperatorFound for instance and other local ones). However the first element of the table never seems to be necessary. I suggest to enumerate NS_MATHML_OPERATOR_FORM_INFIX, NS_MATHML_OPERATOR_FORM_PREFIX and NS_MATHML_OPERATOR_FORM_POSTFIX from 0 to 2 rather than 1 to 3, so that we could use tables of size 3 instead. 2) Apparently, nsMathMLOperators::LookupOperators is supposed to retrieve all the variants (infix, prefix, postfix) of an operator. However when reading the code, it seems that it only gets the first found. Setting a breakpoint on LookupOperators and loading a page with a "<mo>|</mo>" (| has the 3 variants) shows that only the infix form is returned. I think nsMathMLOperators::LookupOperator (without s) need to be modify to handle this properly when aForm=0.

testcase to verify bug indicated in comment 2

(In reply to comment #3) > Created an attachment (id=413908) [details] > Make nsStretchDirection available for any operators (comment 0) FindStretchyOperator should probably be renamed FindOperator. (In reply to comment #4) > Created an attachment (id=413909) [details] > Improve MathML operator forms and fix LookupOperator (comment 1) This patch breaks stretching. I'm not sure whether it is relevant to apply change 1) from comment 1, it seems there are more parts to modify that I thought if we want to make this change works. I've extracted the fix to LookupOperator and will attach a new patch.

Comment on attachment 415005 [details] [diff] [review] Fix LookupOperator Move this separate issue to bug 563365.

Just updating the patch to be compatible with the one of bug 563365.

(In reply to comment #0) > nsMathMLChar > > - replace PRInt32 mOperator > by OperatorData* mOperator I'm a bit concerned about having a raw pointer here for fear of it requiring strict order of object deletion at shutdown, but I need to look more at this.

Comment on attachment 445173 [details] [diff] [review] Patch v.3 (In reply to comment #10) > (In reply to comment #0) > > nsMathMLChar > > > > - replace PRInt32 mOperator > > by OperatorData* mOperator > > I'm a bit concerned about having a raw pointer here for fear of it requiring > strict order of object deletion at shutdown, but I need to look more at this. That may be OK but I'm not certain. However, I don't think we need to store mOperator in nsMathMLChar; we can just look up the direction when we need it based on mData. nsMathMLChar is actually another situation where it is getting data from the operator dictionary but doesn't know the form. The old code is using the first direction found, which is not necessarily the first entry for that character. The code in this patch is using the first entry for the character. I don't think exposing all of the OperatorData struct to nsMathMLChar is the right thing to do given that much of the data doesn't necessarily apply to this form, so I suggest static nsStretchDirection nsMathMLOperatorData::GetStretchyDirection(const nsString& aOperator); Searching through all the stretchy operators in nsMathMLOperators::FindStretchyOperator was quite inefficient, and this gets more significant in FindOperator. I think this can be avoided by using LookupOperators. Perhaps LookupOperator could be used instead of LookupOperators if we can be sure that all forms have the same direction set. It would be nice to have code run in debug builds to verify this, but I don't know whether it is easy to do that. > // never try to stretch this operator again >- nsMathMLOperators::DisableStretchyOperatorAt(mOperator); The comment also can be removed.

Thanks for your review, Karl. I think it's fine to make all forms with the same direction, since the stretch depends really only on the shape of the operator. Normally, that's what we are doing but there may be operators with both stretchy and non-stretchy forms and we need to add direction for the non-stretchy ones. I'm not sure to understand what you want to do in debug mode. It seems to me that we need an exhaustive checking. I can probably modify the script of bug 534970 to check this constraint.

(In reply to comment #12) > I'm not sure to understand what you want to do in debug mode. > It seems to me that we need an exhaustive checking. I can probably modify > the script of bug 534970 to check this constraint. I was thinking about adding some code within "#ifdef DEBUG" that asserted that all forms have the same direction set, but in many ways having the script do this check is better, so don't worry about doing it in the C++ code.

N.B. For the moment, this new patch is not compatible with the "scale stretchy" patch.

Comment on attachment 446793 [details] [diff] [review] Patch v. 4 > nsStretchDirection direction = NS_STRETCH_DIRECTION_UNSUPPORTED; >+ direction = nsMathMLOperators::GetStretchyDirection(mData); Combine these into a single declaration with initialization.

Comment on attachment 447034 [details] [diff] [review] Patch v. 5 Requesting approval2.0, as this is needed for bug 414277. It should be landed with some of the patches in bug 534970.