Last Comment Bug 263359 - bidi algorithm implementation inconsistent with msie (newlines in <pre>)
: bidi algorithm implementation inconsistent with msie (newlines in <pre>)
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- minor with 1 vote (vote)
: mozilla5
Assigned To: Simon Montagu :smontagu
:
Mentors:
: 607541 644768 (view as bug list)
Depends on: 332655 post2.0 645119 645193 650189 650475 663295 663662 664087 668941
Blocks: 330350 477495 566066 css2.1-tests html5bidi 229367 557520 582181 613157 638758 650489 695640
  Show dependency treegraph
 
Reported: 2004-10-07 12:12 PDT by Noam Tamim
Modified: 2012-01-30 08:46 PST (History)
15 users (show)
smontagu: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 (html) (164 bytes, text/html)
2004-10-07 12:20 PDT, Noam Tamim
no flags Details
how the file looks in MSIE. (342 bytes, image/png)
2004-10-07 12:22 PDT, Noam Tamim
no flags Details
how the file looks in Mozilla. (344 bytes, image/png)
2004-10-07 12:24 PDT, Noam Tamim
no flags Details
a no-<pre> testcase (169 bytes, text/html)
2004-10-08 02:17 PDT, Noam Tamim
no flags Details
Patch part 1: preliminary refactoring -- combine InitLogicalArray and CreateBlockBuffer (11.52 KB, patch)
2010-11-30 01:39 PST, Simon Montagu :smontagu
no flags Details | Diff | Review
Patch part 2: more refactoring -- spin ResolveParagraph out of Resolve (1.53 KB, patch)
2010-11-30 01:42 PST, Simon Montagu :smontagu
no flags Details | Diff | Review
Patch part 3: Call ResolveParagraph on encountering <br> or embedded blocks (10.43 KB, patch)
2010-11-30 01:53 PST, Simon Montagu :smontagu
no flags Details | Diff | Review
Tests for bug 229367 and bug 613157 (2.84 KB, patch)
2010-11-30 01:57 PST, Simon Montagu :smontagu
no flags Details | Diff | Review
Patch part 1 v2: preliminary refactoring -- combine InitLogicalArray and CreateBlockBuffer (11.53 KB, patch)
2010-12-07 10:25 PST, Simon Montagu :smontagu
roc: review+
Details | Diff | Review
Patch part 2 v2: more refactoring -- spin ResolveParagraph out of Resolve (11.51 KB, patch)
2010-12-07 10:27 PST, Simon Montagu :smontagu
roc: review+
Details | Diff | Review
Patch part 3 v2: Call ResolveParagraph on encountering <br> or embedding blocks (10.13 KB, patch)
2010-12-07 10:30 PST, Simon Montagu :smontagu
no flags Details | Diff | Review
Patch part 4: call ResolveParagraph on encountering newlines in preformatted elements (16.22 KB, patch)
2010-12-07 10:31 PST, Simon Montagu :smontagu
no flags Details | Diff | Review
Patch part 5: optimize by not doing unnecessary resolution (2.84 KB, patch)
2010-12-07 10:35 PST, Simon Montagu :smontagu
roc: review+
roc: approval2.0-
Details | Diff | Review
Tests for this bug (1.75 KB, patch)
2010-12-07 10:36 PST, Simon Montagu :smontagu
no flags Details | Diff | Review
Patch part 1.5 -- get rid of nsDirectionalFrame (20.95 KB, patch)
2010-12-17 10:03 PST, Simon Montagu :smontagu
roc: review+
Details | Diff | Review
Patch part 2 v3: more refactoring -- spin ResolveParagraph out of Resolve; updated to comments (11.35 KB, patch)
2010-12-17 10:05 PST, Simon Montagu :smontagu
smontagu: review+
Details | Diff | Review
Patch part 3 v3: Call ResolveParagraph on encountering <br> or embedding blocks; updated to comments (12.09 KB, patch)
2010-12-17 10:10 PST, Simon Montagu :smontagu
no flags Details | Diff | Review
Patch part 4 v2: call ResolveParagraph on encountering newlines in preformatted elements; updated to comments (16.99 KB, patch)
2010-12-17 10:12 PST, Simon Montagu :smontagu
roc: review+
Details | Diff | Review
Tests for bug 229367 and bug 613157 (10.26 KB, patch)
2010-12-17 10:14 PST, Simon Montagu :smontagu
no flags Details | Diff | Review
Tests for this bug (10.21 KB, patch)
2010-12-17 10:14 PST, Simon Montagu :smontagu
no flags Details | Diff | Review
printfs for debugging (4.15 KB, patch)
2010-12-17 10:33 PST, Simon Montagu :smontagu
roc: review+
Details | Diff | Review
Patch part 3 v4: Call ResolveParagraph on encountering <br> or embedding blocks (12.03 KB, patch)
2010-12-17 16:12 PST, Simon Montagu :smontagu
roc: review+
Details | Diff | Review
Screenshot of the bidi resolution problem on Persian wikipedia (19.68 KB, image/png)
2011-03-25 15:25 PDT, :Ehsan Akhgari (out sick)
no flags Details
Fix for the regression described in comments 46-47 (1.49 KB, patch)
2011-03-29 13:48 PDT, Simon Montagu :smontagu
roc: review+
Details | Diff | Review
Test for the regression (2.39 KB, patch)
2011-03-29 13:49 PDT, Simon Montagu :smontagu
roc: review+
Details | Diff | Review
Patch part 2.5: cache the lines for each frame while building mLogicalArray (11.72 KB, patch)
2011-06-06 02:34 PDT, Simon Montagu :smontagu
roc: review+
Details | Diff | Review

Description Noam Tamim 2004-10-07 12:12:42 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10

In mozilla, cross-line bidi text is rendered differently that in MSIE (or other
Microsoft applications, for that matter). 
A testcase is worth a thounsand words, so I will not write too much, and my next
comment will have a testcase.
Anyway, I don't know if this is a mozilla bug or an MSIE bug. I don't have
enough knowledge of the bidi algorithm.
So - if any of you do have that knowledge, please - either confirm this as a
bug, or (more likely) drop it as a WONTFIX. I don't mean mozilla to be
bug-compatible with IE.
More info in the next comment. Can't I attach testcases to bug reports?

Reproducible: Always
Steps to Reproduce:
Comment 1 Noam Tamim 2004-10-07 12:20:09 PDT
Created attachment 161392 [details]
part 1 (html)

This is the html source. Look at the following two images:
msie.png: how the file looks in MSIE.
mozilla.png: how the file looks in Mozilla.
Look at the position of the point on the first line.
Comment 2 Noam Tamim 2004-10-07 12:22:23 PDT
Created attachment 161393 [details]
how the file looks in MSIE.
Comment 3 Noam Tamim 2004-10-07 12:24:05 PDT
Created attachment 161394 [details]
how the file looks in Mozilla.
Comment 4 Simon Montagu :smontagu 2004-10-07 15:18:05 PDT
This problem is alluded to in a code comment at
http://lxr.mozilla.org/seamonkey/source/layout/base/src/nsBidiPresUtils.cpp#486
Comment 5 Noam Tamim 2004-10-08 02:17:17 PDT
Created attachment 161455 [details]
a no-<pre> testcase


The bug (if it is really a bug; again, I don't know the bidi algorithm well
enough) is not special just to <pre> sections. It occurs EVERYWHERE - in normal
html (as in the newly attahced testcase) as well as <textarea>s and
Thunderbird/MailNews messages.
The question is really this: should a \n, or a <br>, break the flow of the
text, or should it be treated like a regular whitespace/punctuation char?
Comment 6 Simon Montagu :smontagu 2004-10-08 02:36:24 PDT
<br> in non-preformatted text is discussed in bug 229367. The two cases are not
necessarily equivalent (so this is not a dupe).
Comment 7 Uri Bernstein (Google) 2005-09-08 02:28:41 PDT
Confirming on OS X => All/All
Comment 8 Jesse Ruderman 2009-02-22 13:30:16 PST
According to comments in bug 477495, this can lead to severe performance issues on some pages.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-10-13 22:40:05 PDT
This is tested in the second test of:
http://test.csswg.org/suites/css2.1/20101001/xhtml1/bidi-breaking-002.xht
(I think tests 3 and 4 are invalid.)
Comment 10 Simon Montagu :smontagu 2010-11-30 01:34:12 PST
*** Bug 607541 has been marked as a duplicate of this bug. ***
Comment 11 Simon Montagu :smontagu 2010-11-30 01:34:59 PST
Copied from bug 607541 comment 0:

In elements where line breaks are not collapsed, e.g. <textarea> and elements
with white-space:pre|pre-line|pre-wrap, line breaks should constitute UBA
paragraph breaks.

When a line break introduces a UBA paragraph break, the base direction of the
new UBA paragraph will be determined by the computed direction of the nearest
ancestor element whose bidi properties require its contents to be in a separate
UBA paragraph (or sequence of paragraphs), e.g. a block element or an element
directionally isolated by the ubi attribute (which is being proposed in a
separate bug). Furthermore, for every element on the path in between that
results in the creation of an embedding or override level, e.g. a <bdo> element
or any element with a dir attribute or a value other than "normal" for the
unicode-bidi CSS property, the correspondeng embedding or override level is
re-introduced at the start of the new UBA paragraph (to be closed at the end of
the element or the UBA paragraph, whichever comes first).

For more information, see:
 *
<http://www.w3.org/International/docs/html-bidi-requirements/#newline-as-separator>
 * <http://www.w3.org/Bugs/Public/show_bug.cgi?id=10812>
Comment 12 Simon Montagu :smontagu 2010-11-30 01:39:48 PST
Created attachment 493937 [details] [diff] [review]
Patch part 1: preliminary refactoring -- combine InitLogicalArray and CreateBlockBuffer
Comment 13 Simon Montagu :smontagu 2010-11-30 01:42:17 PST
Created attachment 493938 [details] [diff] [review]
Patch part 2: more refactoring -- spin ResolveParagraph out of Resolve
Comment 14 Simon Montagu :smontagu 2010-11-30 01:53:15 PST
Created attachment 493939 [details] [diff] [review]
Patch part 3: Call ResolveParagraph on encountering <br> or embedded blocks

This fixes bug 229367 and bug 613157
Comment 15 Simon Montagu :smontagu 2010-11-30 01:57:08 PST
Created attachment 493940 [details] [diff] [review]
Tests for bug 229367 and bug 613157

This is a simple test based on http://www.w3.org/Bugs/Public/attachment.cgi?id=925.

I'll add more testcases from bug 229367 and its many dupes.
Comment 16 Aharon (Vladimir) Lanin 2010-11-30 02:58:21 PST
(In reply to comment #9)
> http://test.csswg.org/suites/css2.1/20101001/xhtml1/bidi-breaking-002.xht
> (I think tests 3 and 4 are invalid.)

Tests 3 and 4 are for the behavior of PARAGRAPH SEPARATOR (U+2029) and LINE SEPARATOR (U+2028), respectively, in <pre>. HTML4 used to say something rather ambiguous about them (http://www.w3.org/TR/REC-html40/struct/text.html#whitespace), but HTML5 no longer says anything at all about them, thus leaving them in the purview of CSS, and since that does not say anything about them either, they should be treated according to their Unicode definition, i.e. to start a new line (whether in pre-formatted text or not), with a new bidi paragraph for U+2029 and no new bidi paragraph for U+2028.

Thus, I think that test 3 *is* valid. Unfortunately, test 4 does have a bug. While its comment says "line separator does not break bidi paragraph", it does not actually test this because its second line both starts and ends with an RTL character. Test 4 should thus be something like this:

  <div class="set"> 
    <div class="test"> 
      <div class="pre">
&rlm;&nbsp; + - &times; &divide; &#x05D0;&#x2028;&nbsp; + - &times; &divide; &#x05EA;</div> 
    </div> 
    <div class="control"> 
      <div><bdo dir="ltr">&#x05D0; &divide; &times; - + &nbsp;</bdo></div> 
      <div><bdo dir="ltr">&#x05EA; &divide; &times; - + &nbsp;</bdo></div> 
    </div> 
  </div>
Comment 17 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-11-30 09:08:02 PST
Tests 3 and 4 are invalid for the reasons stated here:
http://lists.w3.org/Archives/Public/public-css-testsuite/2010Oct/0118.html
Comment 18 Simon Montagu :smontagu 2010-11-30 10:54:08 PST
Summary of where we are with this:

The patches so far attached pass on tryserver and fix the related bugs bug 229367 and bug 613157 but don't address newlines in preformatted text, the actual issue that this bug is about.

I made a naïve patch to fix preformatted text like this:

   if (frame->GetStyleContext()->GetStyleText()->NewlineIsSignificant() &&
       frame->HasTerminalNewline()) {
     ResolveParagraph(aBlockFrame, PR_TRUE);

but it doesn't work at all well, I think for two main reasons:

* We do bidi resolution before line breaking, so the test for frame->HasTerminalNewLine() isn't right.
* Even assuming a solution to that, splitting the paragraph inside a content node breaks various assumptions in ResolveParagraph().
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-11-30 12:27:56 PST
Presumably instead of using frame->HasTerminalNewline, you need to test NewlineIsSignificant then actually scan the text looking for all \ns.
Comment 20 Simon Montagu :smontagu 2010-11-30 12:34:50 PST
Will it be OK to actually split the frames after the \ns, or will that confuse the line breaking code in nsTextFrame (or wherever)? If not, bidi resolution will have to pick up again for the next paragraph in mid-frame, which will be a significant complication.
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-11-30 12:44:33 PST
It will be OK to break the frames after the \ns.
Comment 22 Simon Montagu :smontagu 2010-12-07 10:25:21 PST
Created attachment 495867 [details] [diff] [review]
Patch part 1 v2: preliminary refactoring -- combine InitLogicalArray and CreateBlockBuffer
Comment 23 Simon Montagu :smontagu 2010-12-07 10:27:19 PST
Created attachment 495868 [details] [diff] [review]
Patch part 2 v2: more refactoring -- spin ResolveParagraph out of Resolve
Comment 24 Simon Montagu :smontagu 2010-12-07 10:30:28 PST
Created attachment 495869 [details] [diff] [review]
Patch part 3 v2: Call ResolveParagraph on encountering <br> or embedding blocks
Comment 25 Simon Montagu :smontagu 2010-12-07 10:31:47 PST
Created attachment 495870 [details] [diff] [review]
Patch part 4: call ResolveParagraph on encountering newlines in preformatted elements
Comment 26 Simon Montagu :smontagu 2010-12-07 10:35:30 PST
Created attachment 495873 [details] [diff] [review]
Patch part 5: optimize by not doing unnecessary resolution

Also removed some dead code.

This is a very significant optimization for huge text files with little bits of bidi, such as tinderbox logs.
Comment 27 Simon Montagu :smontagu 2010-12-07 10:36:51 PST
Created attachment 495874 [details] [diff] [review]
Tests for this bug

Again, this is a very basic test and I will add more.
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-07 15:18:02 PST
Comment on attachment 495868 [details] [diff] [review]
Patch part 2 v2: more refactoring -- spin ResolveParagraph out of Resolve

+  nsBlockInFlowLineIterator* mLineIter;

nsAutoPtr, so you can remove the manual delete
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-07 16:16:41 PST
+        if (frame->GetStyleContext()->GetStyleDisplay()->mDisplay !=
+            NS_STYLE_DISPLAY_INLINE) {

Is this correct? Should a display:inline-block really end the paragraph? I wouldn't have thought so. Seems to me you probably want IsInlineOutside().

I think mEmbeddingStack would be slightly clearer if it was an nsTArray<PRUnichar>. It shouldn't be interpreted as a string.

The flow of control in part 3 here seems a little weird to me. Before we did InitLogicalArray followed by ResolveParagraph which resolves the whole block. Now, during InitLogicalArray we call ResolveParagraph for all paragraphs except the last one, which has ResolveParagraph called by Resolve.

I think this would make more sense if you renamed InitLogicalArray to TraverseFrames and added a comment to the call to ResolveParagraph from Resolve to say "// Resolve final paragraph".
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-07 16:35:07 PST
+            nsIFrame* lastPreformattedFrame = frame;

This initializer is redundant.

+                /*
+                 * If there is no newline in the frame, just save the text and
+                 * do bidi resolution later
+                 */
+                mBuffer.Append(Substring(text, start));
+                break;

Don't we need to set lastPreformattedFrame here? I guess I'm not really sure what lastPreformattedFrame is supposed to mean. Is it supposed to be firstPreformattedFrame->GetLastContinuation()?

+              PRUint32 lineLength = endLine;

lineLength is a misnomer. Why not just keep using endLine?

+                if (!next) {
+                  // If the frame has no next in flow, create one
+                  CreateContinuation(frame, &next, PR_TRUE);
+                }

Why not move this up inside the previous 'if'?
Comment 31 Simon Montagu :smontagu 2010-12-17 10:03:29 PST
Created attachment 498344 [details] [diff] [review]
Patch part 1.5 -- get rid of nsDirectionalFrame

After part 1 stopped storing the bidi control codes in temporary frames, we can do away with the temporary frames altogether and not waste time creating and destroying them.
Comment 32 Simon Montagu :smontagu 2010-12-17 10:05:08 PST
Created attachment 498345 [details] [diff] [review]
Patch part 2 v3: more refactoring -- spin ResolveParagraph out of Resolve; updated to comments

transferring r=roc
Comment 33 Simon Montagu :smontagu 2010-12-17 10:10:37 PST
Created attachment 498346 [details] [diff] [review]
Patch part 3 v3: Call ResolveParagraph on encountering <br> or embedding blocks; updated to comments

(In reply to comment #29)
> Is this correct? Should a display:inline-block really end the paragraph? I
> wouldn't have thought so. Seems to me you probably want IsInlineOutside().

You're right. Fixed and added a test for that case.

Also handled the logic for getting the right next sibling in TraverseFrames here instead of the lastPreformattedFrame stuff in [the previous version of] the next patch.
Comment 34 Simon Montagu :smontagu 2010-12-17 10:12:52 PST
Created attachment 498347 [details] [diff] [review]
Patch part 4 v2: call ResolveParagraph on encountering newlines in preformatted elements; updated to comments
Comment 35 Simon Montagu :smontagu 2010-12-17 10:14:10 PST
Created attachment 498349 [details] [diff] [review]
Tests for bug 229367 and bug 613157
Comment 36 Simon Montagu :smontagu 2010-12-17 10:14:34 PST
Created attachment 498351 [details] [diff] [review]
Tests for this bug
Comment 37 Simon Montagu :smontagu 2010-12-17 10:33:58 PST
Created attachment 498356 [details] [diff] [review]
printfs for debugging

I've had this in my local tree for years, and I'd like to check it in to save mq churn.
Comment 38 Simon Montagu :smontagu 2010-12-17 15:56:59 PST
Comment on attachment 498346 [details] [diff] [review]
Patch part 3 v3: Call ResolveParagraph on encountering <br> or embedding blocks; updated to comments

Sorry, there's a mistake in this patch
Comment 39 Simon Montagu :smontagu 2010-12-17 16:12:28 PST
Created attachment 498455 [details] [diff] [review]
Patch part 3 v4: Call ResolveParagraph on encountering <br> or embedding blocks
Comment 40 Simon Montagu :smontagu 2010-12-19 22:34:23 PST
Comment on attachment 495873 [details] [diff] [review]
Patch part 5: optimize by not doing unnecessary resolution

Asking approval for this set of patches. It's not risk-free: I've tested extensively and fixed a number of regressions, but there is some chance that there will be other regressions.

I think it's worth taking the patches both for standards conformance and for performance: a tinderbox log with 85000+ lines loads for me in 7 seconds with these patches applied, as opposed to 80 seconds in an unpatched build.
Comment 41 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-20 02:26:15 PST
Comment on attachment 495873 [details] [diff] [review]
Patch part 5: optimize by not doing unnecessary resolution

Tempting as it is, I think this isn't a good time to take it. Let's land it for post-FF4.
Comment 42 :Ehsan Akhgari (out sick) 2011-03-23 20:29:07 PDT
Simon, please land the patches which are meant to be landed here on cedar.
Comment 45 Simon Montagu :smontagu 2011-03-24 14:37:52 PDT
*** Bug 644768 has been marked as a duplicate of this bug. ***
Comment 46 :Ehsan Akhgari (out sick) 2011-03-25 15:19:57 PDT
I backed this out because it caused bug 645119.  Sorry.  :(

http://hg.mozilla.org/mozilla-central/rev/0dc92bb949bf

Another thing to watch for is a bug which a Persian user reported to me.  The bidi resolution breaks when pressing a newline and waiting for a repaint.  I'll attach the screenshot that I received from this user next.
Comment 47 :Ehsan Akhgari (out sick) 2011-03-25 15:25:00 PDT
Created attachment 521980 [details]
Screenshot of the bidi resolution problem on Persian wikipedia
Comment 48 Simon Montagu :smontagu 2011-03-29 13:48:31 PDT
Created attachment 522787 [details] [diff] [review]
Fix for the regression described in comments 46-47
Comment 49 Simon Montagu :smontagu 2011-03-29 13:49:34 PDT
Created attachment 522788 [details] [diff] [review]
Test for the regression
Comment 50 Simon Montagu :smontagu 2011-03-29 13:51:47 PDT
Bug 645119 is caused by attachment 495873 [details] [diff] [review], which was a ride-along optimization and not really part of the fix for this bug. I'll open a new bug for it and fix the regression there.
Comment 53 Simon Montagu :smontagu 2011-04-27 01:53:31 PDT
Backed out again in http://hg.mozilla.org/mozilla-central/rev/fa0295a97f1b because of bug 650189
Comment 55 :Ehsan Akhgari (out sick) 2011-04-30 14:04:27 PDT
OK, I tested bug 650189 with all of these builds, here are the results:

> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-9072c5d3dfcc

No perf regression out of 10 runs.

> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-e794a0c595ef

No perf regression (1 run out of 10 runs performed horribly, but I'm not sure what the cause was).

> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-ceebcc71f7a8

Horrible perf when loading from network, good perf when loading from cache.  The only way that I can explain this is that this build might have the regression, but the regression _might_ be dependent on the way the data is fed into the reflow process.  By the same token, e794a0c595ef _might_ contain the regression as well (but I can't verify this claim).  I tested e794a0c595ef a lot of times after testing this builds, and couldn't reproduce the regression, so it might have been something else.

> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-681666d3bb11

Same as ceebcc71f7a8.

> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-639aa83e113f

Same as ceebcc71f7a8.
Comment 57 :Ehsan Akhgari (out sick) 2011-05-03 15:19:36 PDT
(In reply to comment #56)
> How is
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-4d67e3300692
> ?

At first it seemed to be better, but I managed to reproduce the bug after a couple of tries.
Comment 58 :Ehsan Akhgari (out sick) 2011-05-13 15:04:21 PDT
Does this bug cover the bidi paragraph separation for <br>, as specified in <http://www.whatwg.org/specs/web-apps/current-work/multipage/text-level-semantics.html#the-br-element>?
Comment 59 Simon Montagu :smontagu 2011-05-14 20:28:30 PDT
That depends what you mean by "this bug". It's really bug 229367, but it's fixed by the patches in this bug, specifically attachment 498455 [details] [diff] [review]
Comment 60 Simon Montagu :smontagu 2011-06-06 02:34:15 PDT
Created attachment 537515 [details] [diff] [review]
Patch part 2.5: cache the lines for each frame while building mLogicalArray

This patch kills two birds with one stone: it fixes the performance regression (bug 650189) and also prevents the regression in bug 650489.
Comment 61 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-06 04:56:04 PDT
Comment on attachment 537515 [details] [diff] [review]
Patch part 2.5: cache the lines for each frame while building mLogicalArray

Review of attachment 537515 [details] [diff] [review]:
-----------------------------------------------------------------

Note You need to log in before you can comment on or make changes to this bug.