Created attachment 266595 [details] [diff] [review] Patch Spun off from bug 365130.
Can we avoid the changes to FrameNeedsReflow by hooking nsBlockFrame::DidSetStyleContext instead?
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-
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-
Created attachment 274806 [details] [diff] [review] Using DidSetStyleContext
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.
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 ;-)
Created attachment 274889 [details] [diff] [review] Using MarkIntrinsicWidthsDirty The right patch this time
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-
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.
Created attachment 278340 [details] [diff] [review] w-i-p
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
@@ -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.
> 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()?
Renominating because this patch fixes crashes I care about.
Flags: blocking1.9- → blocking1.9?
Yes, the second one should be GetPrevInFlow().
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 #278864 - Flags: approval1.9+
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Nothing to test here, but the dependees will get their own testcases.
Flags: in-testsuite? → in-testsuite-
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.