461410 - basic deCOM of nsILineIterator

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Benjamin Smedberg]

Attachment: nsILineIterator deCOM, rev. 1

This patch does some basic deCOM of nsILineIterator. It also changes the way the class is accessed and destroyed. Instead of reaching it through QueryInterface, this adds a method nsIFrame::GetLineIterator. When you are done with the iterator, call DisposeLineIterator to delete it. We can't use a virtual destructor, because nsTableRowGroupFrame implements nsILineIterator directly, while nsLineIterator must be explicitly deleted.

To avoid merge conflicts, I'd kinda like to land this in 1.9.1... it seems relatively safe, and might even improve performance very slightly... not sure whether there are any hotspots that actually use the line-iterator interface.

I'd like to note that the logic in nsIFrame::PeekOffset is tortuous in the extreme. I in fact have no clue when "result" is supposed to be a failure code or a success code, so I replicated the old logic very precisely. I've included a lot of context in the patch to make the review of just that function possible.

Comment 1

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/d4c9a0776667

Comment 2

[Benjamin Smedberg]

Backed out: either this or bug 461212 was causing mochitest regressions:

*** 38712 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_character_movement.html | Right movement broken in <b>H</b> K - got [object Text], expected [object Text]
*** 38714 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_character_movement.html | Right movement broken in <b>H</b> K - got [object Text], expected [object Text]
*** 38715 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_character_movement.html | Right movement broken in <b>H</b> K - got 1, expected 2
*** 38716 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_character_movement.html | Left movement broken in <b>H</b> K - got [object Text], expected [object Text]
*** 38717 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_character_movement.html | Left movement broken in <b>H</b> K - got 0, expected 1
*** 38718 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_character_movement.html | Left movement broken in <b>H</b> K - got [object Text], expected [object Text]
*** 38764 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_word_movement.html | Right movement broken with eatSpace=false in "<b>Hello</b> Kitty"; sel.anchorNode.parentNode=[object HTMLSpanElement] - got [object Text], expected [object Text]
*** 38765 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_word_movement.html | Right movement broken with eatSpace=false in "<b>Hello</b> Kitty"; sel.anchorNode.parentNode=[object HTMLSpanElement] - got 5, expected 0

roc, can you guide me which of these patches is more likely to cause regressions such as this?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think this one.

Comment 4

[Mats Palmgren (inactive)]

The errors are caused by the change to nsFrame::GetLineNumber:

-      result = blockFrame->QueryInterface(NS_GET_IID(nsILineIteratorNavigator),getter_AddRefs(it));
+      it = blockFrame->GetLineIterator();

The above is inside a 'while (NS_FAILED(result) ...' so you need to add:

      if (!it)
        result = NS_ERROR_FAILURE;

after the assignment above.  (The logic in this method is horrendous but
lets clean that up as a separate step.)

In nsILineIterator* operator=(nsILineIterator* i) we should probably
add "if (i == mRawPtr) return i;" as the first statement just in case...

There's an extra space in the doc comment for nsILineIterator::FindLineAt():
+   *  is above the first line. Returns N (where N is the number of

Comment 5

[Benjamin Smedberg]

Relanded with nits and correctness fixes: http://hg.mozilla.org/mozilla-central/rev/e54c086f0507