Closed Bug 330373 Opened 19 years ago Closed 19 years ago

[regression] Duplicated text in certain bidi case with image

Categories

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

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: uriber, Assigned: uriber)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

This is a regression from bug 329878 (why do I only find these after checking in?). I'll attach a testcase immediately. When loading the testcase for the first time, or shift-reloading it later, some the text from the second line is duplicated on the first line (which therefore overflows to the right of the green box). Doing a regular reload, or changing text size, fixes the problem. This is apparently triggered by an incremental reflow, but not other kinds of reflows, which is why I haven't found it before. The in-flows which are "left alone" by EnsureBidiContinuation aren't being reused properly in this case, but just stay there. I don't have a fix for this yet. If I won't find one, we might need to consider backing out bug 329878, and perhaps bug 319930 as well (since it basically does the same thing).
Attached file testcase
Description is in comment #0.
> The in-flows which are "left alone" by EnsureBidiContinuation aren't being > reused properly in this case, but just stay there. Should the relevant lines be getting marked dirty or something?
(In reply to comment #2) > > The in-flows which are "left alone" by EnsureBidiContinuation aren't being > > reused properly in this case, but just stay there. > > Should the relevant lines be getting marked dirty or something? > So, what happens here is that only the second (and third, presumably) lines are marked as dirty (ironically, because the code mentioned in bug 330350 comment 1 is not reached in this case). The reflow then starts from the "reused" frames which used to be on the second line, and the first frame (which contains all of the RTL text which is supposed to be split accross two lines) is not reflowed, and therefore not split.
What's marking the frames dirty?
(In reply to comment #4) > What's marking the frames dirty? > I'm afraid I haven't figured out how to answer this. I'm also not absolutely sure that what I wrote above (about the second line being marked dirty) is accurate. All I know is that the first frames actually being reflowed are the in-flows of the second line. The first (primary) frame is not being reflowed. I'm afraid I'll have to get back to this tomorrow. If you can give me a clue on how to find out what's being marked dirty and by whom, that will help me.
You can set a breakpoint in nsBidiPresUtils::Resolve, then when that's hit set one in nsLineBox::MarkDirty and continue. See what callers under Resolve() hit MarkDirty.
Well, during the second (problematic) reflow (which, BTW, is apparently a resize reflow, not an incremental reflow), there are no calls to MarkDirty() from under nsBidiPresUtils::Resolve(). So Someone else must be doing this.
In case it helps here's the relevant portion of the reflow log: VP 0x239609c r=1 a=21375,10125 c=21375,10125 cnt=1300 scroll 0x239627c r=1 a=21375,10125 c=21375,10125 cnt=1301 canvas 0x2396130 r=1 a=21375,UC c=21375,UC cnt=1302 area 0x2396e00 r=1 a=21375,UC c=21375,UC cnt=1303 block 0x2396f84 r=1 incr. (Dirty) a=21375,UC c=21135,UC cnt=1304 block 0x2397600 r=0 a=21135,UC c=3975,UC cnt=1305 text 0x23976e8 r=0 a=3975,UC c=UC,UC cnt=1306 text 0x23976e8 d=3345,270 status=0x1 text 0x2397a08 r=0 a=3975,UC c=UC,UC pif=0x23976e8 cnt=1307 text 0x2397a08 d=2385,270 text 0x2396424 r=0 a=1590,UC c=UC,UC cnt=1308 text 0x2396424 d=240,270 text 0x2397728 r=0 a=1350,UC c=UC,UC cnt=1309 text 0x2397728 d=1305,270 status=0x4301 text 0x2397a7c r=0 a=3975,UC c=UC,UC pif=0x2397728 cnt=1310 text 0x2397a7c d=2400,270 text 0x239776c r=0 a=1575,UC c=UC,UC cnt=1311 text 0x239776c d=75,270 img 0x23978b4 r=0 a=1500,UC c=210,150 cnt=1312 img 0x23978b4 d=210,150 block 0x2397600 d=3975,810 block 0x2396f84 d=21135,810 area 0x2396e00 d=21375,1050 canvas 0x2396130 d=21375,1050 scroll 0x239627c d=21375,10125 VP 0x239609c d=21375,10125 VP 0x21724f0 r=1 a=21405,11985 c=21405,11985 cnt=1313 root 0x2172584 r=1 incr. (Dirty) a=21405,11985 c=21405,11985 cnt=1314 root 0x2172584 d=21405,11985 VP 0x21724f0 d=21405,11985 Document file:///Users/urib/Documents/HTML/ynet.html loaded successfully VP 0x21724f0 r=1 a=21405,11985 c=21405,11985 cnt=1315 root 0x2172584 r=1 incr. (Dirty) a=21405,11985 c=21405,11985 cnt=1316 root 0x2172584 d=21405,11985 VP 0x21724f0 d=21405,11985 VP 0x239609c r=1 a=21375,10125 c=21375,10125 cnt=1317 scroll 0x239627c r=1 a=21375,10125 c=21375,10125 cnt=1318 canvas 0x2396130 r=1 a=21375,UC c=21375,UC cnt=1319 area 0x2396e00 r=1 a=21375,UC c=21375,UC cnt=1320 block 0x2396f84 r=1 a=21375,UC c=21135,UC cnt=1321 block 0x2397600 r=1 incr. (Dirty) a=21135,UC c=3975,UC cnt=1322 text 0x2397a08 r=2 a=3975,UC c=UC,UC pif=0x23976e8 cnt=1323 text 0x2397a08 d=2385,270 text 0x2396424 r=2 a=1590,UC c=UC,UC cnt=1324 text 0x2396424 d=240,270 text 0x2397728 r=2 a=1350,UC c=UC,UC nif=0x2397a7c cnt=1325 text 0x2397728 d=1305,270 status=0x4301 text 0x2397a7c r=2 a=3975,UC c=UC,UC pif=0x2397728 cnt=1326 text 0x2397a7c d=2400,270 text 0x239776c r=2 a=1575,UC c=UC,UC cnt=1327 text 0x239776c d=75,270 img 0x23978b4 r=0 a=1500,UC c=210,150 cnt=1328 img 0x23978b4 d=210,150 block 0x2397600 d=3975,810 block 0x2396f84 d=21135,810 area 0x2396e00 d=21375,1050 canvas 0x2396130 d=21375,1050 scroll 0x239627c d=21375,10125 VP 0x239609c d=21375,10125 Notice that the second reflow starts from the second text frame. Now I'm really off to sleep.
I guess we set the MarkDirty breakpoint and see where it's called from in general, then....
(In reply to comment #4) > What's marking the frames dirty? It's this code: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsBlockFrame.cpp&rev=3.758&mark=5707-5713#5700 (the removed frame is the image frame). Not that I'm sure how knowing this helps us. Perhaps the solution is just to mark the lines dirty in EnsureBidiContinuation and RemoveBidiContionuation?
Right. That's what I was saying in comment 2 -- we should probably twiddle the continuation chain as we do now, then somehow mark those lines dirty... Problem is how.
Attached patch like this? (obsolete) — Splinter Review
Pretty ugly, and possibly involving a performance hit, but this is what I managed to come up with.
Attachment #215042 - Flags: review?(bzbarsky)
I'm not sure whether that's better or whether it's better to hack ReflowDirtyChild to not post a reflow command if called under Resolve() (that is, use a bit in blockframe to control that). roc, dbaron, what do you think?
Can't you pass the blockframe into EnsureBidiContinuation and avoid the loop/QI? Make Resolve() take an actual nsBlockFrame*. This is pretty much what I was expecting when Boris and I talked on IRC. It won't perform though, it could easily make Resolve be O(N^2). How about keeping a line iterator in Resolve that means "the line containing the current frame"?
(In reply to comment #14) > Can't you pass the blockframe into EnsureBidiContinuation and avoid the > loop/QI? Make Resolve() take an actual nsBlockFrame*. I can see how passing the blockframe would avoid the QI, but I think I'll still need the loop, because I need to pass a direct child of the block to FindLineFor().(In reply to comment #14) > It won't perform though, it could easily make Resolve be O(N^2). I don't understand why. > How about keeping > a line iterator in Resolve that means "the line containing the current frame"? Not sure how to do this, but I can try.
(In reply to comment #15) > (In reply to comment #14) > > It won't perform though, it could easily make Resolve be O(N^2). > > I don't understand why. Oh, now I do (I think. Because FindLineFor() is linear, right?)
Yeah, exactly.
(In reply to comment #15) > I can see how passing the blockframe would avoid the QI, but I think I'll > still need the loop, because I need to pass a direct child of the block to > FindLineFor(). Ah, of course.
Attached patch patch (obsolete) — Splinter Review
With a line iterator in Resolve().
Attachment #215042 - Attachment is obsolete: true
Attachment #215064 - Flags: superreview?(bzbarsky)
Attachment #215064 - Flags: review?(roc)
Attachment #215042 - Flags: review?(bzbarsky)
But maybe now I can get rid of the entire aForceReflow thing?
(In reply to comment #20) > But maybe now I can get rid of the entire aForceReflow thing? Yes, that's the point of the exercise :-) Other than that, this looks great.
Also, I'd prefer it if we cached end_lines() in a local here.
Blocks: 330350
Also cached end_lines() in a local.
Attachment #215064 - Attachment is obsolete: true
Attachment #215104 - Flags: superreview?(bzbarsky)
Attachment #215104 - Flags: review?(roc)
Attachment #215064 - Flags: superreview?(bzbarsky)
Attachment #215064 - Flags: review?(roc)
Comment on attachment 215104 [details] [diff] [review] patch, no aForceReflow >Index: layout/base/nsBidiPresUtils.cpp >+ if (nsLayoutAtoms::directionalFrame != frameType) { >+ // Advance the line iterator to the line containing frame So if frameType is directionalFrame we won't advance line... but will still mark it dirty? Generally this is sorta ok, except if it's still at begin_lines(). It seems to me that we should be doing the advancing at the places where we call MarkDirty(); perhaps with a function that takes the frame, nsBlockFrame::line_iterator& line, const nsBlockFrame::line_iterator& endLines? That would also address the second concern I had, which is that the code as it stands always iterates the lines, even if it doesn't actually change any continuations. Other than that, this looks most excellent.
Attachment #215104 - Flags: superreview?(bzbarsky) → superreview+
(In reply to comment #24) > (From update of attachment 215104 [details] [diff] [review] [edit]) > >Index: layout/base/nsBidiPresUtils.cpp > >+ if (nsLayoutAtoms::directionalFrame != frameType) { > >+ // Advance the line iterator to the line containing > frame > > So if frameType is directionalFrame we won't advance line... but will still > mark it dirty? No, because both calls to markDirty are inside the "else" block of |if (nsLayoutAtoms::directionalFrame == frameType)|. > > It seems to me that we should be doing the advancing at the places where we > call MarkDirty(); perhaps with a function that takes the frame, > nsBlockFrame::line_iterator& line, const nsBlockFrame::line_iterator& endLines? But then we will be calling this function with continuation frames that we just created (in the previous iteration). I'm not sure these frames even know what line they're on. (notice that we are not taking |frame| from mLogicalFrames with each iteration of the loop).
> No, because both calls to markDirty are inside the "else" block Ah, ok. Worth asserting that at the callsite so it doesn't change. > notice that we are not taking |frame| from mLogicalFrames with each iteration > of the loop We could use another var that's set whenever we get frame from mLogicalFrames... But either way is ok, I guess. Let's do this and see what the profiles look like.
(In reply to comment #26) > > No, because both calls to markDirty are inside the "else" block > > Ah, ok. Worth asserting that at the callsite so it doesn't change. > I know you usually win these arguments, but I'll try anyhow: I don't see the point of asserting that the frame is not a directionalFrame at that point. If it is a directionalFrame (and, in fact, if it is anything other than a textFrame, which should be assured by the |if (isTextFrame)| we're inside) than nothing we're doing makes any sense (creating continuations for directional frames??), and marking the wrong line (or an invalid line) dirty is the least of our concerns. But then again, I think it's clear enough, just from looking at the code, that this cannot happen. > We could use another var that's set whenever we get frame from > mLogicalFrames... > > But either way is ok, I guess. Let's do this and see what the profiles look > like. > So, with your permission, I'll leave it this way (which I find to be more elegant) for now, and as you suggest, if profiles show this is significant I'll do what you suggest.
> than nothing we're doing makes any sense OK, that I buy. ;) No need for the assert; let's land this as-is if roc likes it. > think it's clear enough, just from looking at the code I don't buy that, though, for future reference. I've seen too many cases when code gets changed without looking at how that change affects other code 20 lines away, so I like to assert assumptions right where they're made...
Comment on attachment 215104 [details] [diff] [review] patch, no aForceReflow I think the optimization bz suggested is worthwhile, but I'm happy to have it happen in a separate patch.
Attachment #215104 - Flags: review?(roc) → review+
Checked in: Checking in layout/base/nsBidiPresUtils.h; /cvsroot/mozilla/layout/base/nsBidiPresUtils.h,v <-- nsBidiPresUtils.h new revision: 1.21; previous revision: 1.20 done Checking in layout/base/nsBidiPresUtils.cpp; /cvsroot/mozilla/layout/base/nsBidiPresUtils.cpp,v <-- nsBidiPresUtils.cpp new revision: 1.73; previous revision: 1.72 done Checking in layout/generic/nsBlockFrame.cpp; /cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v <-- nsBlockFrame.cpp new revision: 3.762; previous revision: 3.761 done
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
For me, with the testcase in bug 319930, this only improved performance marginally, if at all. I suspect that for that case, all lines end up reflowed anyway.
Depends on: 339935
Attachment #214938 - Attachment mime type: text/html → text/html; charset=windows-1255
Depends on: 423676
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: