Optimize calls to bidi resolution

RESOLVED FIXED in mozilla1.9alpha6

Status

()

Core
Layout: Text
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: smontagu, Assigned: smontagu)

Tracking

Trunk
mozilla1.9alpha6
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 ?
wanted1.9 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

11 years ago
Created attachment 266595 [details] [diff] [review]
Patch

Spun off from bug 365130.
Flags: blocking1.9?
Attachment #266595 - Flags: superreview?(dbaron)
Attachment #266595 - Flags: review?(roc)
(Assignee)

Updated

11 years ago
Target Milestone: --- → mozilla1.9alpha6
Can we avoid the changes to FrameNeedsReflow by hooking nsBlockFrame::DidSetStyleContext instead?
(Assignee)

Comment 2

11 years ago
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.
That's weird. Doesn't the style context change when they change the page direction? Surely it must.
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.
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
Comment on attachment 266595 [details] [diff] [review]
Patch

Now that bug 386640 landed, I think we can do what comment 1 suggests instead.
Attachment #266595 - Flags: superreview?(dbaron) → superreview-
(Assignee)

Comment 6

11 years ago
Created attachment 274806 [details] [diff] [review]
Using DidSetStyleContext
Attachment #266595 - Attachment is obsolete: true
Attachment #274806 - Flags: superreview?(dbaron)
Attachment #274806 - Flags: review?(roc)
Attachment #266595 - Flags: review?(roc)
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.
(Assignee)

Comment 8

11 years ago
Created attachment 274888 [details] [diff] [review]
Using MarkIntrinsicWidthsDirty

I have high hopes of getting this patch down to a one-liner before we're through ;-)
Attachment #274806 - Attachment is obsolete: true
Attachment #274888 - Flags: superreview?(dbaron)
Attachment #274888 - Flags: review?(roc)
Attachment #274806 - Flags: superreview?(dbaron)
Attachment #274806 - Flags: review?(roc)
(Assignee)

Comment 9

11 years ago
Created attachment 274889 [details] [diff] [review]
Using MarkIntrinsicWidthsDirty

The right patch this time
Attachment #274888 - Attachment is obsolete: true
Attachment #274889 - Flags: superreview?(dbaron)
Attachment #274889 - Flags: review?(roc)
Attachment #274888 - Flags: superreview?(dbaron)
Attachment #274888 - Flags: review?(roc)
(Assignee)

Comment 10

11 years ago
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.
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.)
Attachment #274889 - Flags: superreview?(dbaron) → superreview-
(Assignee)

Comment 12

11 years ago
ResolveBidi doesn't handle all continuations of a block, but it should.
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.
(Assignee)

Comment 14

11 years ago
Created attachment 278340 [details] [diff] [review]
w-i-p
Attachment #274889 - Attachment is obsolete: true
Attachment #274889 - Flags: review?(roc)
(Assignee)

Comment 15

11 years ago
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
Attachment #278340 - Flags: superreview?(roc)
Attachment #278340 - Flags: review?(dbaron)
(Assignee)

Comment 16

11 years ago
This also fixes the crash in bug 392923.
Blocks: 392923
@@ -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.
(Assignee)

Comment 18

11 years ago
> 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()?

(Assignee)

Updated

11 years ago
Blocks: 394173

Comment 19

11 years ago
Renominating because this patch fixes crashes I care about.
Flags: blocking1.9- → blocking1.9?
Yes, the second one should be GetPrevInFlow().
(Assignee)

Comment 21

11 years ago
Created attachment 278864 [details] [diff] [review]
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.
Attachment #278340 - Attachment is obsolete: true
Attachment #278864 - Flags: superreview?(roc)
Attachment #278864 - Flags: review?(roc)
Attachment #278340 - Flags: superreview?(roc)
Attachment #278340 - Flags: review?(dbaron)
Attachment #278864 - Flags: superreview?(roc)
Attachment #278864 - Flags: superreview+
Attachment #278864 - Flags: review?(roc)
Attachment #278864 - Flags: review+
(Assignee)

Comment 22

11 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Blocks: 389630

Updated

11 years ago
Flags: in-testsuite?

Updated

11 years ago
Depends on: 394805
(Assignee)

Comment 23

11 years ago
Nothing to test here, but the dependees will get their own testcases.
Flags: in-testsuite? → in-testsuite-
Flags: wanted1.9+
Whiteboard: [wanted-1.9]

Updated

10 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: layout.bidi → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.