Bug 524275 (nsStretchDirection)

Make nsStretchDirection available for any operators

RESOLVED FIXED in mozilla2.0b5

Status

()

Core
MathML
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: fredw, Assigned: fredw)

Tracking

Trunk
mozilla2.0b5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

8 years ago
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
(Assignee)

Comment 1

8 years ago
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.
(Assignee)

Comment 2

8 years ago
Created attachment 408199 [details]
testcase for LookupOperators

testcase to verify bug indicated in comment 2
(Assignee)

Comment 3

8 years ago
Created attachment 413908 [details] [diff] [review]
Make nsStretchDirection available for any operators (comment 0)
(Assignee)

Comment 4

8 years ago
Created attachment 413909 [details] [diff] [review]
Improve MathML operator forms and fix LookupOperator (comment 1)
(Assignee)

Comment 5

8 years ago
(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.
(Assignee)

Comment 6

8 years ago
Created attachment 415004 [details] [diff] [review]
Make nsStretchDirection available for any operators (V2)
Attachment #413908 - Attachment is obsolete: true
(Assignee)

Comment 7

8 years ago
Created attachment 415005 [details] [diff] [review]
Fix LookupOperator
Attachment #413909 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #415004 - Flags: review?(karlt)
(Assignee)

Updated

8 years ago
Attachment #415005 - Flags: review?(karlt)
(Assignee)

Comment 8

8 years ago
Comment on attachment 415005 [details] [diff] [review]
Fix LookupOperator

Move this separate issue to bug 563365.
Attachment #415005 - Attachment is obsolete: true
Attachment #415005 - Flags: review?(karlt)
(Assignee)

Updated

8 years ago
Alias: nsStretchDirection
Summary: MathML: Make nsStretchDirection available for any operators, and other improvements → Make nsStretchDirection available for any operators
(Assignee)

Updated

8 years ago
Attachment #408199 - Attachment is obsolete: true
(Assignee)

Comment 9

8 years ago
Created attachment 445173 [details] [diff] [review]
Patch v.3

Just updating the patch to be compatible with the one of bug 563365.
Attachment #415004 - Attachment is obsolete: true
Attachment #415004 - Flags: review?(karlt)
(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.
(Assignee)

Comment 12

8 years ago
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.
(Assignee)

Comment 14

8 years ago
Created attachment 446793 [details] [diff] [review]
Patch v. 4

N.B. For the moment, this new patch is not compatible with the "scale stretchy" patch.
Attachment #445173 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #446793 - Flags: review?(karlt)
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.
Attachment #446793 - Flags: review?(karlt) → review+
(Assignee)

Comment 16

8 years ago
Created attachment 447034 [details] [diff] [review]
Patch v. 5
Attachment #446793 - Attachment is obsolete: true
Attachment #447034 - Flags: review+
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.
Attachment #447034 - Flags: approval2.0?
Attachment #447034 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/16fd52fea1aa
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
You need to log in before you can comment on or make changes to this bug.