847790 - Make nsTreeContentView::mRows an nsTArray<nsAutoPtr<Row> >

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Nicholas Nethercote [inactive]]

In bug 847248 comment 7, bz said:

> (part 3) - Remove the nsFixedSizeAllocator from nsTreeContentView.
> 
> r=me, though worth filing a followup to make mRows into an
> nsTArray<nsAutoPtr<Row> >

|mRows| is currently a nsTArray<Row*>.

Comment 1

[Catalin Iacob]

Attachment: First attempt, guidance needed

Here's my first attempt at this (and my first Mozilla patch). The patch compiles and runs but I don't consider it finished, see below.

I'm not happy with the new loops that call InsertElementAt to copy rows into mRows. However, I don't see how to do it differently. I couldn't leave the single call to InsertElementsAt that was there before because InsertElementsAt takes a const source array but copying the nsAutoPtr from the source destroys them; I therefore get a compilation error:

In file included from ../../../dist/include/nsCOMArray.h:12:0,
                 from /home/catalin/hacking/moz/mozilla-central/layout/xul/tree/nsTreeStyleCache.h:11,
                 from /home/catalin/hacking/moz/mozilla-central/layout/xul/tree/nsTreeUtils.h:11,
                 from /home/catalin/hacking/moz/mozilla-central/layout/xul/tree/nsTreeContentView.cpp:9:
../../../dist/include/nsTArray.h: In instantiation of ‘static void nsTArrayElementTraits<E>::Construct(E*, const A&) [with A = nsAutoPtr<Row>; E = nsAutoPtr<Row>]’:
../../../dist/include/nsTArray.h:543:7:   required from ‘static void AssignRangeAlgorithm<IsPod, IsSameType>::implementation(ElemType*, IndexType, SizeType, const Item*) [with Item = nsAutoPtr<Row>; ElemType = nsAutoPtr<Row>; IndexType = unsigned int; SizeType = unsigned int; bool IsPod = false; bool IsSameType = true]’
../../../dist/include/nsTArray.h:1394:5:   required from ‘void nsTArray_Impl<E, Alloc>::AssignRange(nsTArray_Impl<E, Alloc>::index_type, nsTArray_Impl<E, Alloc>::size_type, const Item*) [with Item = nsAutoPtr<Row>; E = nsAutoPtr<Row>; Alloc = nsTArrayInfallibleAllocator; nsTArray_Impl<E, Alloc>::index_type = unsigned int; nsTArray_Impl<E, Alloc>::size_type = unsigned int]’
../../../dist/include/nsTArray.h:912:5:   required from ‘nsTArray_Impl<E, Alloc>::elem_type* nsTArray_Impl<E, Alloc>::ReplaceElementsAt(nsTArray_Impl<E, Alloc>::index_type, nsTArray_Impl<E, Alloc>::size_type, const Item*, nsTArray_Impl<E, Alloc>::size_type) [with Item = nsAutoPtr<Row>; E = nsAutoPtr<Row>; Alloc = nsTArrayInfallibleAllocator; nsTArray_Impl<E, Alloc>::elem_type = nsAutoPtr<Row>; nsTArray_Impl<E, Alloc>::index_type = unsigned int; nsTArray_Impl<E, Alloc>::size_type = unsigned int]’
../../../dist/include/nsTArray.h:946:72:   required from ‘nsTArray_Impl<E, Alloc>::elem_type* nsTArray_Impl<E, Alloc>::InsertElementsAt(nsTArray_Impl<E, Alloc>::index_type, const nsTArray_Impl<Item, Allocator>&) [with Item = nsAutoPtr<Row>; Allocator = nsTArrayInfallibleAllocator; E = nsAutoPtr<Row>; Alloc = nsTArrayInfallibleAllocator; nsTArray_Impl<E, Alloc>::elem_type = nsAutoPtr<Row>; nsTArray_Impl<E, Alloc>::index_type = unsigned int]’
/home/catalin/hacking/moz/mozilla-central/layout/xul/tree/nsTreeContentView.cpp:1244:57:   required from here
../../../dist/include/nsTArray.h:512:5: error: no matching function for call to ‘nsAutoPtr<Row>::nsAutoPtr(const nsAutoPtr<Row>&)’
../../../dist/include/nsTArray.h:512:5: note: candidates are:
In file included from ../../../dist/include/nsHashKeys.h:12:0,
                 from /home/catalin/hacking/moz/mozilla-central/layout/xul/tree/../../base/nsIPresShell.h:24,
                 from /home/catalin/hacking/moz/mozilla-central/layout/xul/tree/../../base/nsPresContext.h:16,
                 from /home/catalin/hacking/moz/mozilla-central/layout/xul/tree/../../style/nsRuleNode.h:14,
                 from /home/catalin/hacking/moz/mozilla-central/layout/xul/tree/../../style/nsStyleContext.h:11,
                 from /home/catalin/hacking/moz/mozilla-central/layout/xul/tree/nsTreeStyleCache.h:13,
                 from /home/catalin/hacking/moz/mozilla-central/layout/xul/tree/nsTreeUtils.h:11,
                 from /home/catalin/hacking/moz/mozilla-central/layout/xul/tree/nsTreeContentView.cpp:9:
../../../dist/include/nsAutoPtr.h:94:7: note: nsAutoPtr<T>::nsAutoPtr(nsAutoPtr<T>&) [with T = Row]
../../../dist/include/nsAutoPtr.h:94:7: note:   no known conversion for argument 1 from ‘const nsAutoPtr<Row>’ to ‘nsAutoPtr<Row>&’
../../../dist/include/nsAutoPtr.h:88:7: note: nsAutoPtr<T>::nsAutoPtr(nsAutoPtr<T>::Ptr) [with T = Row]
../../../dist/include/nsAutoPtr.h:88:7: note:   no known conversion for argument 1 from ‘const nsAutoPtr<Row>’ to ‘nsAutoPtr<Row>::Ptr’
../../../dist/include/nsAutoPtr.h:82:7: note: nsAutoPtr<T>::nsAutoPtr() [with T = Row]
../../../dist/include/nsAutoPtr.h:82:7: note:   candidate expects 0 arguments, 1 provided
make[1]: *** [nsTreeContentView.o] Error 1

Ideas?

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 737240 [details] [diff] [review]
First attempt, guidance needed

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

First of all, congratulations on submitting your first patch!  You have to overcome several hurdles to get this far, so well done.

I'm not an expert on nsTArray, but InsertElementsAt() has loops inside it, so your explicit looping seems ok to me.  bz knows more about this stuff;  I've asked him to review.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 737240 [details] [diff] [review]
First attempt, guidance needed

>+  for(int32_t i = 0; i < count; i++) {

Probably better to make i an nsTArray::index_type and compare it to rows.Length() instead of using the signed-int version of "count".

And you want a space between "for" and "(".

Same thing for the other loop.

If you add a comment about how InsertElementsAt doesn't work because it can't steal the ownership from its const argument, r=me

Comment 4

[Catalin Iacob]

Attachment: New version, addressing Boris' review

Comment 5

[Nicholas Nethercote [inactive]]

Thanks, Catalin!  I'm happy to land this.

Before doing that, I've triggered a try server run, viewable at https://tbpl.mozilla.org/?tree=Try&rev=3a8fe7adebc3.  It's a "T" push, which means it'll test the build on all platforms, and do all the tests just on one platform (Linux, which I chose because the Linux machines had the lightest load).  It's possibly overkill for a small patch like this, but I figure better safe than sorry for your first patch :)

A minor nitpick:  if you use |hg export| or |hg bzexport| (see https://bitbucket.org/sfink/bzexport/) instead of |hg diff|, the resulting patch file will include the commit message and user name.  Then other people can |hg import| it and not have to fill the commit message and user name on your behalf.  Thanks!

Comment 6

[Nicholas Nethercote [inactive]]

Ugh, that try server run was broken due to a bad patch that landed in inbound just before I pushed.  I cancelled it, and here's another attempt: https://tbpl.mozilla.org/?tree=Try&rev=5cd8f0594376

Comment 7

[Nicholas Nethercote [inactive]]

There are some mochitest-5 failures I'm not sure about -- they look like they're intermittents that have been seen before, but I re-triggered twice and they failed every time.  So I've triggered another run, which runs mochitest-5 on all platforms.  I also built on all platforms, because I just got stung in another bug where I did debug-only builds.  https://tbpl.mozilla.org/?tree=Try&rev=db6eeb6e7c1a

Comment 8

[Nicholas Nethercote [inactive]]

> I've triggered another run...
> https://tbpl.mozilla.org/?tree=Try&rev=db6eeb6e7c1a

Yikes, that was on top of yet more bustage.  Once more with feeling:
https://tbpl.mozilla.org/?tree=Try&rev=c56048cdec81

Comment 9

[Nicholas Nethercote [inactive]]

> Once more with feeling:
> https://tbpl.mozilla.org/?tree=Try&rev=c56048cdec81

That was all green.  Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b63c7bfced0

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4b63c7bfced0

Comment 11

[Catalin Iacob]

Nicholas thanks for the guidance, will apply the advice for future paches.

Now on to the next bug. I have a reproducible failure locally for one test in mochitest-5, I'll try to look at that as it would be nice to have a base where all tests are passing.

Comment 12

[Nicholas Nethercote [inactive]]

> Now on to the next bug. I have a reproducible failure locally for one test
> in mochitest-5, I'll try to look at that as it would be nice to have a base
> where all tests are passing.

Excellent!  You could ask on #developers if the failure you see is one that shows up intermittently on TBPL.  Any of the following people are likely to have a good idea:  philor, RyanVM, edmorley.

Also, bug 798914 is a fairly easy code clean-up if you're looking for another bug :)