Closed Bug 960312 Opened 10 years ago Closed 10 years ago

TEXT_IS_IN_SINGLE_CHAR_MI frame state bit should have a different name

Categories

(Core :: MathML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: heycam, Assigned: heycam)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
The TEXT_IS_IN_SINGLE_CHAR_MI frame state bit defined in nsTextFrame.h uses one of the global frame state bits rather than a frame class specific one.  Note the comment in nsIFrame.h that says:

  // Bits 20-31 and 60-63 of the frame state are reserved for implementations.

Luckily there is a single nsTextFrame-specific bit free -- NS_FRAME_STATE_BIT(30).
Attachment #8360706 - Flags: review?(roc)
(In reply to Cameron McCormack (:heycam) from comment #2)
> Backed out for test failures:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8219df0931e3

All of the failures are similar to https://tbpl.mozilla.org/php/getParsedLog.php?id=33074391&tree=Mozilla-Inbound (but are happening in at least m2, m5, C, Cipc, R, Ru... and on at least both linux and osx), the common thing seems to be crashing in [@ nsIFrame::GetWritingMode() const]
This state bit is also set on the block frame that contains the single character operator, so not just on text frames.  Let's rename the constant to make it obvious it's global.

(It's probably possible to avoid it being a global state bit with some effort.  We've only got 4 left.)
Summary: TEXT_IS_IN_SINGLE_CHAR_MI frame state bit should have a different value → TEXT_IS_IN_SINGLE_CHAR_MI frame state bit should have a different name
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8360706 - Attachment is obsolete: true
Attachment #8360861 - Flags: review?(roc)
Comment on attachment 8360861 [details] [diff] [review]
patch v2

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

::: layout/generic/nsTextFrame.cpp
@@ +249,5 @@
>  // nsTextFrame.h has
>  // #define TEXT_IS_IN_TOKEN_MATHML          NS_FRAME_STATE_BIT(32)
>  
>  // nsTextFrame.h has
> +// #define NS_FRAME_IS_IN_SINGLE_CHAR_MI        NS_FRAME_STATE_BIT(59)

Why is this still here?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away January 20-24) from comment #6)
> >  // nsTextFrame.h has
> > +// #define NS_FRAME_IS_IN_SINGLE_CHAR_MI        NS_FRAME_STATE_BIT(59)
> 
> Why is this still here?

It should go.
Comment on attachment 8360861 [details] [diff] [review]
patch v2

You know TEXT_IS_IN_TOKEN_MATHML should be renamed too.  Lemme do that.
Attachment #8360861 - Flags: review?(roc)
(In reply to Cameron McCormack (:heycam) from comment #8)
> You know TEXT_IS_IN_TOKEN_MATHML should be renamed too.  Lemme do that.

Well, looks like there is some deliberate squatting on the global namespace bits there.  That slot used to be taken up by TEXT_FORCE_TRIM_WHITESPACE which is mentioned in the comment about NS_FRAME_IS_PUSHED_FLOAT with which it shares its value.
Attached patch patch v2.1Splinter Review
Attachment #8360863 - Flags: review?(roc)
Attachment #8360861 - Attachment is obsolete: true
Attachment #8360863 - Attachment is patch: true
https://hg.mozilla.org/mozilla-central/rev/ec669f48ec9e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.