382422 - Optimize calls to bidi resolution

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Simon Montagu :smontagu]

Attachment: Patch

Spun off from bug 365130.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can we avoid the changes to FrameNeedsReflow by hooking nsBlockFrame::DidSetStyleContext instead?

Comment 2

[Simon Montagu :smontagu]

That would work (I think) in the code path where FrameNeedsReflow is called from nsCSSFrameConstructor::StyleChangeReflow, but it doesn't seem to kick in when FrameNeedsReflow is called from PresShell::StyleChangeReflow. This happens in the quite common scenario (for Bidi users) of changing page direction from the View menu.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's weird. Doesn't the style context change when they change the page direction? Surely it must.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There are a few edge cases where we don't maintain style context immutability (those where we call ClearStyleDataAndReflow, I think), but that was really just intended for the color prefs where we knew it would work.  It's partly still done that way because the elements all still match the same rules, so ReResolveStyleContext would no-op.

Instead, we should probably swap out the rule tree and call ReResolveStyleContext; the swapping out of the rule tree might be a little interesting in that there would be two roots floating around for a short time.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 266595 [details] [diff] [review]
Patch

Now that bug 386640 landed, I think we can do what comment 1 suggests instead.

Comment 6

[Simon Montagu :smontagu]

Attachment: Using DidSetStyleContext

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So -- sorry I didn't think of this earlier -- but what's the advantage of doing this rather than just setting the bit in MarkIntrinsicWidthsDirty (and Init), which should hook into pretty much all the cases this hooks into except at only one point?  Then you wouldn't need the frame constructor changes, the IsFrameOfType bit, or the DidSetStyleContext callback.

Comment 8

[Simon Montagu :smontagu]

Attachment: Using MarkIntrinsicWidthsDirty

I have high hopes of getting this patch down to a one-liner before we're through ;-)

Comment 9

[Simon Montagu :smontagu]

Attachment: Using MarkIntrinsicWidthsDirty

The right patch this time

Comment 10

[Simon Montagu :smontagu]

Comment on attachment 274889 [details] [diff] [review]
Using MarkIntrinsicWidthsDirty

>-// XXX keep the text-run data in the first-in-flow of the block
>-

I had meant to nuke this comment in an earlier patch. The code that it refers to was removed years ago.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I had a longer version of this comment written up but I crashed before submitting, so I'll try to rewrite:

If a block is split among columns or pages, does ResolveBidi handle all the continuations or just the current one?

If the answer is all, then:
 * the MarkIntrinsicWidthsDirty code should add the bit to |dirtyBlock|, not to the implied |this|
 * the |Init| code should only set the bit on the first continuation
 * Reflow should assert that the bit is never set on a non-first continuation
 * the bit should be removed from NS_BLOCK_FLAGS_MASK in nsHTMLParts.h
(Or, alternatively, you could let the bit be set on continuations but just ignore it, but I think I prefer this approach.)

If the answer is just one, then you need some of the above changes, plus some 
loops over continuations.


Also, if ResolveBidi fails, is it really worth trying again?  It seems simpler to just clear the bit at the start?  (The NS_OK for a null BidiUtils also seems a little suspicious.)

Comment 12

[Simon Montagu :smontagu]

ResolveBidi doesn't handle all continuations of a block, but it should.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, that's something I ran into recently.

For this bug I think we should do what dbaron suggested for "all" and make ResolveBidi check the bit on the first-in-flow, and if that's set, loop over all continuations doing ResolveBidi on each.

Comment 14

[Simon Montagu :smontagu]

Attachment: w-i-p

Comment 15

[Simon Montagu :smontagu]

Comment on attachment 278340 [details] [diff] [review]
w-i-p

I think this is good to go. To do bidi properly with columns and pages the loop over block continuations needs to move into nsBidiPresUtils::Resolve, but I'd rather leave that for a second iteration, after bug 393962

Comment 16

[Simon Montagu :smontagu]

This also fixes the crash in bug 392923.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

@@ -6103,6 +6104,8 @@ nsBlockFrame::Init(nsIContent*      aCon
 
   nsresult rv = nsBlockFrameSuper::Init(aContent, aParent, aPrevInFlow);
 
+  GetFirstContinuation()->AddStateBits(NS_BLOCK_NEEDS_BIDI_RESOLUTION);

Shouldn't we just test if this is the first continuation (!aPrevInFlow) and set the bit in that case? We shouldn't need to set the bit just because a continuation was created.

+  if (!(GetStateBits() & NS_BLOCK_NEEDS_BIDI_RESOLUTION))
     return NS_OK;

+  NS_ASSERTION(this == GetFirstContinuation(),
+               "ResolveBidi called on non-first continuation");

Move the assertion above the if. And have it check !aPrevInFlow instead of calling GetFirstContinuation, which could be expensive.

Comment 18

[Simon Montagu :smontagu]

> Move the assertion above the if. And have it check !aPrevInFlow instead of
> calling GetFirstContinuation, which could be expensive.

Huh? ResolveBidi() doesn't have an aPrevInFlow (and neither do any of its callers). Do you mean !GetPrevInFlow()?

Comment 19

[Jesse Ruderman]

Renominating because this patch fixes crashes I care about.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, the second one should be GetPrevInFlow().

Comment 21

[Simon Montagu :smontagu]

Attachment: Addressed roc's comments

Since dbaron is OOTO this week and I think I've well addressed his earlier comments, I'm asking r+sr from roc, but feel free to punt back to him if you don't think that's OK.

Comment 22

[Simon Montagu :smontagu]

Checked in.

Comment 23

[Simon Montagu :smontagu]

Nothing to test here, but the dependees will get their own testcases.