Closed
Bug 80352
Opened 23 years ago
Closed 19 years ago
Changing Bidi options never triggers reflow
Categories
(Core :: Internationalization, defect)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
People
(Reporter: cathleennscp, Assigned: smontagu)
References
Details
(Keywords: fixed1.8.1)
Attachments
(4 files, 3 obsolete files)
|
579 bytes,
text/html
|
Details | |
|
803 bytes,
patch
|
Details | Diff | Splinter Review | |
|
2.48 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
|
5.44 KB,
patch
|
bzbarsky
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
Marc Attinasi wrote: I'm getting the same results as Curtis, basically. Running the same source built with and without BiDi enabled I get the following reflow data Default Build with IBMBIDI defined: Document http://jrgm.mcom.com/perf/loadtime5/base/www.w3.org_DOML2Core loaded successfully Disabling View Source StyleSheet /perf/loadtime5/base/www.w3.org_DOML2Core/ Init Incrm Resze Style Dirty Total ------------------------------------------------------------------------------ nsTableColFrame 780 0 0 0 0 780 nsTableColGroupFrame 87 0 174 0 0 261 nsBoxToBlockAdaptor 1 28 0 0 0 29 nsGfxScrollFrame 1 26 0 0 0 27 nsTableOuterFrame 87 0 174 0 0 261 nsTextFrame 6226 0 9064 0 0 15290 nsTableRowFrame 110 0 220 0 0 330 nsInlineFrame 2752 0 3997 0 0 6749 ViewportFrame 1 26 0 0 0 27 CanvasFrame 1 26 2 0 0 29 nsTableCellFrame 330 0 435 0 0 765 BRFrame 79 0 66 0 0 145 nsBulletFrame 33 1 36 0 0 70 nsBlockFrame 1962 222 3597 0 0 5781 HRuleFrame 2 0 4 0 0 6 nsTableRowGroupFrame 87 0 174 0 0 261 nsBoxFrame 1 26 0 0 0 27 nsTableFrame 87 0 89 0 0 176 Grand Totals 12627 355 18032 0 0 31014 Build without IBMBIDI defined: Document http://jrgm.mcom.com/perf/loadtime5/base/www.w3.org_DOML2Core loaded successfully Disabling View Source StyleSheet /perf/loadtime5/base/www.w3.org_DOML2Core/ Init Incrm Resze Style Dirty Total ------------------------------------------------------------------------------ nsTableColFrame 549 0 0 0 0 549 nsTableColGroupFrame 87 0 97 0 0 184 nsBoxToBlockAdaptor 1 51 0 0 0 52 nsGfxScrollFrame 1 49 0 0 0 50 nsTableOuterFrame 87 0 21 0 0 108 nsTextFrame 6190 0 3129 0 0 9319 nsTableRowFrame 110 0 126 0 0 236 nsInlineFrame 2746 0 1531 0 0 4277 ViewportFrame 1 49 0 0 0 50 CanvasFrame 1 49 2 0 0 52 nsTableCellFrame 330 0 345 0 0 675 BRFrame 79 0 6 0 0 85 nsBulletFrame 33 0 28 0 0 61 nsBlockFrame 1962 394 1188 0 0 3544 HRuleFrame 2 0 2 0 0 4 nsTableRowGroupFrame 87 0 97 0 0 184 nsBoxFrame 1 49 0 0 0 50 nsTableFrame 87 0 12 0 0 99 Grand Totals 12354 641 6584 0 0 19579 I'd like to start narrowing down the IBMBIDI sections that are causing this, since I cannot see it from a code inspection. nsTextFrame and nsBlockFrame are my initial candidates, and I'll start disabling selectively the IBMBIDI sections from those files an rerunning. - marc
|
||
Comment 1•23 years ago
|
||
I just call marc and he said the number he post is wrong. He somehow build it from his branch and both number are without bidi IT is interesting that these two number are so different in the same build.
|
||
Comment 2•23 years ago
|
||
Yup - my really really bad. In narrowing this down I found out that I actually DID NOT have IBMBIDI defined in either of my builds, so the numbers I posted are actually for the same build! It is interesting that the numbers are so different, and to be honest I ran this build about 5-6 times and the numbers were different each time - I do not know why (timing maybe?) So, please ignore the numbers posted when the bug was opened. I am building again to redo the analysis I tried to do before (sigh).
|
|
||
Comment 3•23 years ago
|
||
Also, I look with Curt with the raw data file (for example ftp://ftp.mozilla.org/pub/data/reflows/010510/reflow.out ) I think the tool he used to add up the number from the raw data file (reflow.out) have some problem-. It basically use "+++ loading" as the seperate between URL. But if you read the reflow.out file yourself, you will find out that there are different urls load between "+++ loading" . So the number we currently got are not accurate for a particulare URL. Regardless of the above issue, the BIDI one is still bigger. In particular, if we read the reflow.out carefully, the non bidi one typically have Style and Dirty one in 0 for every one of it. But the Bidi one have large number in the Style and some non zero number for Dirty. I think that is the reason the reflow count went up. I talk to marc about this and we are looking why the Style one went up now.
|
Assignee | |
Comment 4•23 years ago
|
||
I know some cases where Bidi processing can happen on non-Bidi pages. I've added an assertion in my local tree to detect this and was going to open a bug on all the cases I have found. On Sunday I'll investigate reflow with and without my local changes.
|
|
||
Comment 5•23 years ago
|
||
somehow, the result.out in the ftp.mozilla.org show some number in the Style and Dirty reflow. But in my Window build with Viewer, I always get 0 in Style and Dirty for these pages. Maybe we should also read the Linux warnning to see any stuff we need to think about. simon@softel.co.il probably know this best. It is hard for him because I cannot see this on window and he is on window.
Status: NEW → ASSIGNED
|
|
||
Comment 6•23 years ago
|
||
I run the bidi build on linux with viewer and the dump does not have number for Style and Dirty. I wonder why gtkEmbed see that What is --eable-cpp-rtti do ? Will that make some differences?
|
||
Comment 7•23 years ago
|
||
Bidi resolution may cause additional reflow (nsBlockFrame.cpp, around line 711). Main conditions: 1. Bidi enabled (as a result of presense of hebrew/arabic characters or/and dir=rtl attribute). (BTW this means that "pure Latin" frames wouldn't be involved.) 2. At least 1 frame was removed during bidi resolution.
|
|
||
Comment 8•23 years ago
|
||
Lina- I look at those pages. I don't think those pages fall into your condiction. According to the raw data (see ftp://ftp.mozilla.org/pub/data/reflows/010510/reflow.out ) I see a lot of Style reflow which does not happen before bidi land . Also check Dirty reflow
|
|
||
Comment 9•23 years ago
|
||
hum.... I don't think the issue is window or linux. I have the up to date linux build. And I cannot see the problem in viewer. Howerver, if I run the reflow test with gtkEmbed. I can see the Style reflow count is high. This is strange. For example- let's lookat at the loading of http://jrgm.mcom.com/perf/loadtime5/ base/www.apple.com in ftp://ftp.mozilla.org/pub/data/reflows/010508/reflow.out /perf/loadtime5/base/www.apple.com/ Init Incrm Resze Style Dirty Total ------------------------------------------------------------------------------ nsTableColFrame 49 0 0 0 0 49 nsTableColGroupFrame 11 0 21 0 0 32 nsBoxToBlockAdaptor 2 7 0 0 0 9 nsComboboxControlFrame 1 0 2 0 0 3 nsTableOuterFrame 11 2 20 0 0 33 nsGfxScrollFrame 1 5 0 0 0 6 nsTextFrame 136 0 137 0 0 273 nsListControlFrame 1 0 1 0 0 2 nsTableRowFrame 16 2 29 0 0 47 nsInlineFrame 45 0 21 0 0 66 nsPlaceholderFrame 2 0 2 0 0 4 ViewportFrame 1 5 0 0 0 6 nsGfxButtonControlFrame 8 0 7 0 0 15 CanvasFrame 1 5 2 0 0 8 nsHTMLButtonControlFrame 1 0 0 0 0 1 nsTableCellFrame 22 2 22 0 0 46 BRFrame 13 0 15 0 0 28 nsBlockFrame 79 15 80 0 0 174 nsTableRowGroupFrame 11 2 21 0 0 34 nsGfxTextControlFrame2 1 0 1 0 0 2 nsImageFrame 36 0 11 0 1 48 nsBoxFrame 2 5 1 0 0 8 nsTableFrame 11 2 11 0 0 24 nsScrollFrame 1 0 1 0 0 2 Grand Totals 462 52 405 0 1 920 in ftp://ftp.mozilla.org/pub/data/reflows/010510/reflow.out /perf/loadtime5/base/www.apple.com/ Init Incrm Resze Style Dirty Total ------------------------------------------------------------------------------ nsTableColFrame 66 0 0 0 0 66 nsTableColGroupFrame 11 0 32 0 0 43 nsBoxToBlockAdaptor 2 7 0 2 0 11 nsComboboxControlFrame 1 0 2 1 0 4 nsTableOuterFrame 11 1 20 11 0 43 nsGfxScrollFrame 1 5 0 0 0 6 nsTextFrame 136 0 129 168 0 433 nsListControlFrame 1 0 1 2 0 4 nsTableRowFrame 16 1 45 16 0 78 nsInlineFrame 45 0 21 81 0 147 nsPlaceholderFrame 2 0 2 4 0 8 ViewportFrame 1 5 0 0 0 6 nsGfxButtonControlFrame 8 0 7 15 0 30 nsHTMLButtonControlFrame 1 0 0 1 0 2 CanvasFrame 1 4 2 1 0 8 nsTableCellFrame 22 1 22 22 0 67 BRFrame 13 0 15 18 0 46 nsBlockFrame 79 10 125 81 0 295 nsTableRowGroupFrame 11 1 32 11 0 55 nsGfxTextControlFrame2 1 0 1 2 0 4 nsImageFrame 36 0 11 71 1 119 nsBoxFrame 2 5 1 2 0 10 nsTableFrame 11 1 11 11 0 34 nsScrollFrame 1 0 2 1 0 4 Grand Totals 479 41 481 521 1 1523 and in my local build which run the gtkEmbed- /perf/loadtime5/base/www.apple.com/ Init Incrm Resze Style Dirty Total ------------------------------------------------------------------------------ nsTableColFrame 66 0 0 0 0 66 nsTableColGroupFrame 11 0 32 0 0 43 nsBoxToBlockAdaptor 2 7 0 2 0 11 nsComboboxControlFrame 1 0 2 1 0 4 nsTableOuterFrame 11 1 20 11 0 43 nsGfxScrollFrame 1 5 0 0 0 6 nsTextFrame 136 0 132 168 0 436 nsListControlFrame 1 0 1 2 0 4 nsTableRowFrame 16 1 45 16 0 78 nsInlineFrame 45 0 21 81 0 147 nsPlaceholderFrame 2 0 2 4 0 8 ViewportFrame 1 5 0 0 0 6 nsGfxButtonControlFrame 8 0 7 15 0 30 nsHTMLButtonControlFrame 1 0 0 1 0 2 CanvasFrame 1 4 2 1 0 8 nsTableCellFrame 22 1 22 22 0 67 BRFrame 13 0 15 18 0 46 nsBlockFrame 79 11 125 81 0 296 nsTableRowGroupFrame 11 1 32 11 0 55 nsGfxTextControlFrame2 1 0 1 2 0 4 nsImageFrame 36 0 11 71 1 119 nsBoxFrame 2 5 1 2 0 10 nsTableFrame 11 1 11 11 0 34 nsScrollFrame 1 0 2 1 0 4 Grand Totals 479 42 484 521 1 1527 which you can also see there are non-zero Style reflow. However, in the same linux build, if I run viewer and dump the reflow test by hand, I got the following result /perf/loadtime5/base/www.apple.com/ Init Incrm Resze Style Dirty Total ------------------------------------------------------------------------------ nsTableColFrame 49 0 0 0 0 49 nsTableColGroupFrame 11 0 21 0 0 32 nsBoxToBlockAdaptor 2 5 0 0 0 7 nsComboboxControlFrame 1 0 2 0 0 3 nsGfxScrollFrame 1 5 0 0 0 6 nsTableOuterFrame 11 1 20 0 0 32 nsTextFrame 136 0 129 0 0 265 nsListControlFrame 1 0 1 0 0 2 nsTableRowFrame 16 1 29 0 0 46 nsInlineFrame 45 0 21 0 0 66 nsPlaceholderFrame 2 0 2 0 0 4 ViewportFrame 1 5 0 0 0 6 nsGfxButtonControlFrame 8 0 7 0 0 15 CanvasFrame 1 3 2 0 0 6 nsHTMLButtonControlFrame 1 0 0 0 0 1 nsTableCellFrame 22 1 22 0 0 45 BRFrame 13 0 15 0 0 28 nsBlockFrame 79 8 80 0 1 168 nsTableRowGroupFrame 11 1 21 0 0 33 nsGfxTextControlFrame2 1 0 1 0 0 2 nsBoxFrame 2 5 1 0 0 8 nsImageFrame 36 0 11 0 1 48 nsTableFrame 11 1 11 0 0 23 nsScrollFrame 1 0 1 0 0 2 Grand Totals 462 36 397 0 2 897 Notice that there are no Style reflow and the total total reflow number is smaller than no bidi
|
|
||
Comment 10•23 years ago
|
||
Interesting, I modify test/launch_reflow_test.csh and change
setenv URL_COUNT 64
./watch_reflow.sh -o $OUTFILE -u $URL_COUNT $MOZILLA_FIVE_HOME/gtkEmbed "http://
intrepid/buster.cgi?interval=10&file=lists/reflowX46_ltog.list" >>& $OUTFILE
to
setenv URL_COUNT 1
./watch_reflow.sh -o $OUTFILE -u $URL_COUNT $MOZILLA_FIVE_HOME/gtkEmbed "http://
jrgm.mcom.com/perf/loadtime5/base/www.apple.com" >>& $OUTFILE
and I got the following result
/perf/loadtime5/base/www.apple.com/
Init Incrm Resze Style Dirty Total
------------------------------------------------------------------------------
nsTableColFrame 49 0 0 0 0 49
nsTableColGroupFrame 11 0 21 0 0 32
nsBoxToBlockAdaptor 2 5 0 0 0 7
nsComboboxControlFrame 1 0 2 0 0 3
nsGfxScrollFrame 1 5 0 0 0 6
nsTableOuterFrame 11 1 20 0 0 32
nsTextFrame 136 0 129 0 0 265
nsListControlFrame 1 0 1 0 0 2
nsTableRowFrame 16 1 29 0 0 46
nsInlineFrame 45 0 21 0 0 66
nsPlaceholderFrame 2 0 2 0 0 4
ViewportFrame 1 5 0 0 0 6
nsGfxButtonControlFrame 8 0 7 0 0 15
CanvasFrame 1 3 2 0 0 6
nsHTMLButtonControlFrame 1 0 0 0 0 1
nsTableCellFrame 22 1 22 0 0 45
BRFrame 13 0 15 0 0 28
nsBlockFrame 79 8 80 0 1 168
nsTableRowGroupFrame 11 1 21 0 0 33
nsGfxTextControlFrame2 1 0 1 0 0 2
nsBoxFrame 2 5 1 0 0 8
nsImageFrame 36 0 11 0 1 48
nsTableFrame 11 1 11 0 0 23
nsScrollFrame 1 0 1 0 0 2
Grand Totals 462 36 397 0 2 897
This mean the Style reflow is not increase if we simply load http://
jrgm.mcom.com/perf/loadtime5/base/www.apple.com . It is casued by loading http://
intrepid/buster.cgi?interval=10&file=lists/reflowX46_ltog.list . We should look
at how these test script interact with frame.
|
|
||
Comment 11•23 years ago
|
||
here is the source of http://intrepid/buster.cgi?interval=10&file=lists/ reflowX46_ltog.list <html> <head> <meta http-equiv="Pragma" content="no-cache"> <meta http-equiv="refresh" content="30;url=buster.cgi?file=lists/ reflowX46_ltog.list&page=1&last=-1&refresh=30"> <title>BrowserBuster II: http://jrgm.mcom.com/perf/loadtime5/base/www.google.com< /title> <style type="text/css"> body { overflow: hidden; border: 0; margin: 0; } </style> </head> <script> dump("+++ loading http://jrgm.mcom.com/perf/loadtime5/base/www.google.com\n"); </script> <body> lists/reflowX46_ltog.list: http://jrgm.mcom.com/perf/loadtime5/base/ www.google.com <iframe width="100%" height="100%" src="http://jrgm.mcom.com/perf/loadtime5/base/ www.google.com"> </body> </html> notice body { overflow: hidden; border: 0; margin: 0; } and <iframe width="100%" height="100%" What are these stuff impact to the Style reflow ? I think the test procedure have some impact to the test result
|
|
||
Comment 12•23 years ago
|
||
I should try this url
|
|
||
Comment 13•23 years ago
|
||
try this- http://intrepid/buster.cgi?file=lists/reflowX46_ltog.list&page=4&last=- 1&refresh=30
|
|
||
Comment 14•23 years ago
|
||
that one is http://jrgm.mcom.com/perf/loadtime5/base/www.apple.com
|
|
||
Comment 15•23 years ago
|
||
The test is very strang- it will first load the apple.com and with a reasonable reflow number, and when it try to load the next sites, it will first dump a loading of the new sites and then dump a bigger number of the last sites which it load, the reason the number is bigger is because style reflow. The bidi reflow does increase in the 2nd number in the style reflow. But please notice that this style reflow increase won't be reproduceable if just load the web site by hand, it only could be reproduced if combined with iframe and some style sheet stuff. I wonder what is overflow: hidden; mean
|
|
||
Comment 16•23 years ago
|
||
ok. the body {
overflow: hidden;
border: 0;
margin: 0;
}
have nothing to do with the issue.
Also, the height and width of the iframe have nothing to do with this issue.
|
|
||
Comment 17•23 years ago
|
||
I made a simpiler test cases- 1. open viewer and view that test case 2. type in another url- say an empty html the style change happen when we load the empty html on the test page
|
|
||
Comment 18•23 years ago
|
||
|
|
||
Comment 19•23 years ago
|
||
reproduce procedure 1. make a html file called a.html which only contains "hello" 2. on window (or linux), set your debugging application to viewer 3. set a break point at nsTextFrame::Reflow and make the condiction as aReflowState.reason == eReflowReason_StyleChange 4. run viewer 5. load http://bugzilla.mozilla.org/showattachment.cgi?attach_id=34363 into viewer 6. no break happen yet 7. view a.html which you create at 1 it will break at nsTextFrame::Reflow, but what got Style change is not a.html bu thte test page. I also set break point at all the place which I can find " = eReflowReason_StyleChange" and the earilest break point I hit is the following layout/xul/base/src/nsBoxToBlockAdaptor.cpp#826 819 // if we were marked for style change. 820 // 1) see if we are just supposed to do a resize if so convert to a style change. Kill 2 birds 821 // with 1 stone. 822 // 2) If the command is incremental. See if its style change. If it is everything is ok if not 823 // we need to do a second reflow with the style change. 824 if (mStyleChange) { 825 if (reflowState.reason == eReflowReason_Resize) 826 reflowState.reason = eReflowReason_StyleChange; 827 else if (reason == eReflowReason_Incremental) { and here is the stack trace- nsBoxToBlockAdaptor::Reflow(nsBoxLayoutState & {...}, nsIPresContext * 0x029c65a0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0x00000000, int 0x00000000, int 0x00000000, int 0x000021de, int 0x00001077, int 0x00000001) line 826 nsBoxToBlockAdaptor::DoLayout(nsBoxToBlockAdaptor * const 0x01a00db4, nsBoxLayoutState & {...}) line 523 + 52 bytes nsBox::Layout(nsBox * const 0x01a00db4, nsBoxLayoutState & {...}) line 985 nsScrollBoxFrame::DoLayout(nsScrollBoxFrame * const 0x01a001d4, nsBoxLayoutState & {...}) line 377 nsBox::Layout(nsBox * const 0x01a001d4, nsBoxLayoutState & {...}) line 985 nsContainerBox::LayoutChildAt(nsBoxLayoutState & {...}, nsIBox * 0x01a001d4, const nsRect & {...}) line 591 + 16 bytes nsGfxScrollFrameInner::LayoutBox(nsBoxLayoutState & {...}, nsIBox * 0x01a001d4, const nsRect & {...}) line 1038 + 17 bytes nsGfxScrollFrameInner::Layout(nsBoxLayoutState & {...}) line 1144 nsGfxScrollFrame::DoLayout(nsGfxScrollFrame * const 0x01a0012c, nsBoxLayoutState & {...}) line 1046 + 15 bytes nsBox::Layout(nsBox * const 0x01a0012c, nsBoxLayoutState & {...}) line 985 nsBoxFrame::Reflow(nsBoxFrame * const 0x01a000f4, nsIPresContext * 0x029c65a0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0x00000000) line 781 nsGfxScrollFrame::Reflow(nsGfxScrollFrame * const 0x01a000f4, nsIPresContext * 0x029c65a0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0x00000000) line 735 + 25 bytes nsContainerFrame::ReflowChild(nsIFrame * 0x01a000f4, nsIPresContext * 0x029c65a0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0x00000000, int 0x00000000, unsigned int 0x00000000, unsigned int & 0x00000000) line 722 + 31 bytes ViewportFrame::Reflow(ViewportFrame * const 0x01a00080, nsIPresContext * 0x029c65a0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0x00000000) line 538 nsHTMLReflowCommand::Dispatch(nsHTMLReflowCommand * const 0x037338e0, nsIPresContext * 0x029c65a0, nsHTMLReflowMetrics & {...}, const nsSize & {...}, nsIRenderingContext & {...}) line 145 PresShell::ProcessReflowCommand(nsVoidArray & {...}, int 0x00000001, nsHTMLReflowMetrics & {...}, nsSize & {...}, nsIRenderingContext & {...}) line 5761 PresShell::ProcessReflowCommands(int 0x00000001) line 5816 ReflowEvent::HandleEvent() line 5674 HandlePLEvent(ReflowEvent * 0x03733880) line 5688 PL_HandleEvent(PLEvent * 0x03733880) line 588 + 10 bytes PL_ProcessPendingEvents(PLEventQueue * 0x0106aae0) line 518 + 9 bytes _md_EventReceiverProc(HWND__ * 0x11b0074c, unsigned int 0x0000c14e, unsigned int 0x00000000, long 0x0106aae0) line 1069 + 9 bytes USER32! 77e41186() 0106aae0() It looks like the iframe is wrap inside the nsBox and we try to resize the Box when we load the new page The aDesiredSize is 0 and 0 in PresShell::ProcessReflowCommand
|
|
||
Comment 20•23 years ago
|
||
Ok, here is the reason why we got the reflow in the bidi build
nsHTMLReflowCommand::nsHTMLReflowCommand(nsIFrame * 0x01b6414c,
nsIReflowCommand::ReflowType ReflowDirty, nsIFrame * 0x00000000, nsIAtom *
0x00000000) line 67
NS_NewHTMLReflowCommand(nsIReflowCommand * * 0x0012f814, nsIFrame * 0x01b6414c,
nsIReflowCommand::ReflowType ReflowDirty, nsIFrame * 0x00000000, nsIAtom *
0x00000000) line 47 + 43 bytes
nsFrame::CreateAndPostReflowCommand(nsIPresShell * 0x0331dd30, nsIFrame *
0x01b6414c, nsIReflowCommand::ReflowType ReflowDirty, nsIFrame * 0x00000000,
nsIAtom * 0x00000000, nsIAtom * 0x00000000) line 3923 + 45 bytes
nsContainerFrame::ReflowDirtyChild(nsContainerFrame * const 0x01b640d8,
nsIPresShell * 0x0331dd30, nsIFrame * 0x01b6414c) line 332 + 21 bytes
nsBox::MarkStyleChange(nsBox * const 0x01b64184, nsBoxLayoutState & {...}) line
327 + 28 bytes
nsPresContext::RemapStyleAndReflow(nsPresContext * const 0x03310750) line 441
nsPresContext::SetBidi(nsPresContext * const 0x03310750, unsigned int
0x00000000, int 0x00000001) line 1536
DocumentViewerImpl::SetBidiOptions(DocumentViewerImpl * const 0x032e577c,
unsigned int 0x00000000) line 4926
SetChildBidiOptions(nsIMarkupDocumentViewer * 0x032e577c, void * 0x00000000)
line 4740
DocumentViewerImpl::CallChildren(void (nsIMarkupDocumentViewer *, void *)*
0x016b4e80 SetChildBidiOptions(nsIMarkupDocumentViewer *, void *), void *
0x00000000) line 4560 + 16 bytes
DocumentViewerImpl::SetBidiOptions(DocumentViewerImpl * const 0x03398c3c,
unsigned int 0x00000000) line 4928
nsDocShell::SetupNewViewer(nsDocShell * const 0x012cec80, nsIContentViewer *
0x03398c30) line 3278 + 19 bytes
nsWebShell::SetupNewViewer(nsWebShell * const 0x012cec80, nsIContentViewer *
0x03398c30) line 346 + 13 bytes
nsDocShell::Embed(nsDocShell * const 0x012ceca0, nsIContentViewer * 0x03398c30,
const char * 0x01604610, nsISupports * 0x00000000) line 2764 + 23 bytes
nsWebShell::Embed(nsWebShell * const 0x012ceca0, nsIContentViewer * 0x03398c30,
const char * 0x01604610, nsISupports * 0x00000000) line 375
nsDocShell::CreateContentViewer(nsDocShell * const 0x012cec80, const char *
0x0012fd7c, nsIRequest * 0x02e02d80, nsIStreamListener * * 0x0012fdcc) line 3043
+ 32 bytes
nsDSURIContentListener::DoContent(nsDSURIContentListener * const 0x012ce970,
const char * 0x0012fd7c, int 0x00000000, const char * 0x002e8220
gCommonEmptyBuffer, nsIRequest * 0x02e02d80, nsIStreamListener * * 0x0012fdcc,
int * 0x0012fd60) line 117 + 33 bytes
nsDocumentOpenInfo::DispatchContent(nsIRequest * 0x02e02d80, nsISupports *
0x00000000) line 371 + 109 bytes
nsDocumentOpenInfo::OnStartRequest(nsDocumentOpenInfo * const 0x02e04500,
nsIRequest * 0x02e02d80, nsISupports * 0x00000000) line 240 + 16 bytes
nsResChannel::OnStartRequest(nsResChannel * const 0x02e02d88, nsIRequest *
0x02e041a0, nsISupports * 0x00000000) line 532
nsFileChannel::OnStartRequest(nsFileChannel * const 0x02e041a8, nsIRequest *
0x02e055a4, nsISupports * 0x00000000) line 453 + 37 bytes
nsOnStartRequestEvent::HandleEvent() line 108 + 53 bytes
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x02e05b24) line 64
PL_HandleEvent(PLEvent * 0x02e05b24) line 588 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x012ca0e0) line 518 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x1de10236, unsigned int 0x0000c14e, unsigned int
0x00000000, long 0x012ca0e0) line 1069 + 9 bytes
USER32! 77e41186()
012ca0e0()
|
|
||
Comment 21•23 years ago
|
||
To get the previous stack trace, 1. run viewer, 2. load resource:///res/samples/test9.html 3. set break point at nsHTMLReflowCommand::nsHTMLReflowCommand and 4. load resource:///res/samples/test0.html it will break here Notice that you have to use test9.html because it have frame
|
|
||
Comment 22•23 years ago
|
||
OK, here is the problem code
nsPresContext::SetBidi
....
if (mShell && aForceReflow) {
RemapStyleAndReflow();
}
and
DocumentViewerImpl::SetBidiOptions
if (mPresContext) {
mPresContext->SetBidi(aBidiOptions, PR_TRUE); // force reflow
}
DocumentViewerImpl::SetBidiOptions always pass in PR_TRUE into
nsPresContext::SetBidi which cause it call RemapStyleAndReflow();
|
|
||
Comment 23•23 years ago
|
||
|
|
||
Comment 24•23 years ago
|
||
jst- can you r/sr my temp fix ?
|
||
Comment 25•23 years ago
|
||
sr=vidur for the temporary disabling of on-the-fly bidi change. The fix to figure out why extra reflows are happening (presumably for the new page) should be investigated ASAP.
|
|
||
Comment 27•23 years ago
|
||
temp fix check in. Down grade the bug from moz0.9.1 reassign this bug to simon@softel.co.il
Assignee: ftang → simon
|
|
Assignee | |
Comment 28•23 years ago
|
||
Have a fix for this, but it's hard to separate the diffs from those for bug 80552. When that gets checked in I'll attach a proper patch. Meanwhile, here is the new method. The idea is to remove the aForceReflow argument from nsPresContext::SetBidi and let the method itself decide whether the bidi options require a reflow. NS_IMETHODIMP nsPresContext::SetBidi(PRUint32 aSource) { PRBool isVisual, bidiEnabled; PRBool needReflow = PR_FALSE; // Test for changes to Bidi options that require reflow // 1. Change in page direction: if (GET_BIDI_OPTION_DIRECTION(mBidi) != GET_BIDI_OPTION_DIRECTION(aSource)) needReflow = PR_TRUE; // 2. Change in text type (logical/visual) or bidi enabled status. // Not enough just to test for a change to the options, because these // depend on combinations of different options. So save the previous // state and reflow if there is any change. isVisual = mIsVisual; GetBidiEnabled(&bidiEnabled); mBidi = aSource; if (IBMBIDI_TEXTDIRECTION_RTL == GET_BIDI_OPTION_DIRECTION(mBidi) || IBMBIDI_NUMERAL_HINDI == GET_BIDI_OPTION_NUMERAL(mBidi)) { if (!bidiEnabled) { needReflow = PR_TRUE; } SetBidiEnabled(PR_TRUE); } if (IBMBIDI_TEXTTYPE_VISUAL == GET_BIDI_OPTION_TEXTTYPE(mBidi)) { SetVisualMode(PR_TRUE); } else if (IBMBIDI_TEXTTYPE_LOGICAL == GET_BIDI_OPTION_TEXTTYPE(mBidi)) { SetVisualMode(PR_FALSE); } else { SetVisualMode(IsVisualCharset(mCharset) ); } if (isVisual != mIsVisual) { needReflow = PR_TRUE; } if (needReflow) { // Finally check for bidiEnabled and only reflow if it is set GetBidiEnabled(&bidiEnabled); if (bidiEnabled) { RemapStyleAndReflow(); } } return NS_OK; }
|
|
||
Updated•23 years ago
|
Whiteboard: have fix. need to produce patch
|
|
||
Comment 29•23 years ago
|
||
80552 have been reviewed and checked in . Please update your tree and propose a real fix for this bug :) Thanks
|
|
Assignee | |
Comment 30•23 years ago
|
||
|
||
Comment 31•23 years ago
|
||
for long comments would you consider /* */ or /** */ notations?
|
|
Assignee | |
Comment 32•23 years ago
|
||
OK, I'll consider it. :-) Seriously, commenting notation doesn't seem to be mentioned in any of the style and convention docs on mozilla.org (except for http://www.mozilla.org/hacking/portable-cpp.html#no_cpp_comments_in_c) What many of the docs do emphasize is consistency within a given source file, and nsPresContext.cpp uses // almost without exception.
|
|
||
Comment 33•23 years ago
|
||
simon- what will happen if we don't take your real fix for moz0.9.1 ? what does that mean. Could we wait till moz0.9.2 ?
|
|
||
Comment 34•23 years ago
|
||
I will try it when I have time.
|
|
Assignee | |
Comment 35•23 years ago
|
||
I think the real fix will only make a difference if bidi prefs are enabled, so yes, it can wait until 0.9.2
|
|
||
Comment 36•23 years ago
|
||
should we consider check into trunk now?
|
|
Assignee | |
Comment 37•23 years ago
|
||
Marking as dependent on bug 88100. The patch here will need some revision once that one is checked in.
Depends on: 88100
|
||
Updated•23 years ago
|
QA Contact: andreasb → zach
|
|
Assignee | |
Updated•22 years ago
|
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 38•20 years ago
|
||
Comment 35 is not quite right: we need to force reflow also when changing properties from javascript. I'll bring the patch up to date and check in soon.
|
||
Comment 39•19 years ago
|
||
We should probably actually fix this...
|
|
Assignee | |
Comment 40•19 years ago
|
||
Yes, but changing summary to make it clear what we are fixing. The increased reflow count was fixed by attachment 34391 [details] [diff] [review]. Asaf, do you know any case where dynamically changing Bidi options doesn't cause the expected change in the rendering?
Summary: reflow count increased on 5/10 build, due to BiDi code? → Changing Bidi options never triggers reflow
|
|
Assignee | |
Comment 41•19 years ago
|
||
There are really two parts to this. The first part, as described in comment 28 SetBidi() determines internally whether a reflow is necessary by comparing the old and new options. The second part is intended to prevent the temporary fix from regressing: if (0 == mBidi) { // if mBidi is zero, this is a new PresContext so no need to reflow // (bug 80352) needReflow = PR_FALSE; } I rather suspect that this test is bogus.
Attachment #34931 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 42•19 years ago
|
||
Comment on attachment 202265 [details] [diff] [review] Updated patch Oh, hum. I didn't realize you were touching this code in another bug, Boris. I should read my bugmail more synchronously :(
Attachment #202265 -
Attachment is obsolete: true
|
|
||
Comment 43•19 years ago
|
||
Sorry... I was considering commenting on this bug and pointing to that one, but figured you were cced on it... :(
|
|
Assignee | |
Comment 44•19 years ago
|
||
I decided that the tests in the original patch for whether reflow was required were over-optimization: a script might set one option after the other, and each individual change would not seem to require a reflow, but the accumulation of all of them would. So I am just testing whether any bits change, plus the equivalent of the |if (0 == mBidi)| test in the previous patch.
|
|
Assignee | |
Comment 45•19 years ago
|
||
Some testcases: Go to a logical Bidi page, e.g. http://unicode.org/iuc/iuc10/x-ar.html Open a new tab and use about:config to change bidi.texttype to 3 Return to the previous tab. Expected results: the numbers within the Arabic text should be reversed: 21-01 and 7991 instead of 10-12 and 1997. Actual results: the numbers remain the same unless the page is reloaded. Go to a visual Bidi page, e.g. http://www.cs.bgu.ac.il/~elad/noa_v.html Open a new tab and use about:config to change bidi.texttype to 2 Return to the previous tab. Expected results: all the Hebrew text should be reversed: העונ instead of נועה Actual results: the text remains the same unless the page is reloaded.
|
|
Assignee | |
Comment 46•19 years ago
|
||
Some slight changes here: testing for the current bidi options being zero seems not to be so bogus, since they are set to zero by nsDocument::operator new(). However, given that nsPresContext::GetUserPreferences() is calling SetBidi() without aForceReflow, and that since the fix of bug 88100 DocumentViewerImpl::SetBidiOptions() is no longer called from nsDocShell::SetupNewViewer as in the stack in comment 20, there seems to be no case where the scenario that caused the original bug report will occur, so I have changed that test to an assertion.
Attachment #202389 -
Attachment is obsolete: true
Attachment #202512 -
Flags: superreview?
Attachment #202512 -
Flags: review?
|
|
Assignee | |
Updated•19 years ago
|
Attachment #202512 -
Flags: superreview?(bzbarsky)
Attachment #202512 -
Flags: superreview?
Attachment #202512 -
Flags: review?(bzbarsky)
Attachment #202512 -
Flags: review?
|
|
||
Updated•19 years ago
|
Attachment #202512 -
Flags: superreview?(bzbarsky)
Attachment #202512 -
Flags: superreview+
Attachment #202512 -
Flags: review?(bzbarsky)
Attachment #202512 -
Flags: review+
|
|
Assignee | |
Comment 47•19 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
|
||
Updated•19 years ago
|
Flags: blocking1.8.1?
1.8.1 blocker, but please try to get a 1.8-friendly patch soon.
Flags: blocking1.8.1? → blocking1.8.1+
|
|
Assignee | |
Comment 49•18 years ago
|
||
Attachment #219501 -
Flags: approval-branch-1.8.1?(bzbarsky)
|
|
||
Updated•18 years ago
|
Attachment #219501 -
Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1+
|
|
Assignee | |
Comment 50•18 years ago
|
||
Checked into MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1
Whiteboard: have fix. need to produce patch
|
|
||
Updated•18 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•