Last Comment Bug 847790 - Make nsTreeContentView::mRows an nsTArray<nsAutoPtr<Row> >
: Make nsTreeContentView::mRows an nsTArray<nsAutoPtr<Row> >
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla23
Assigned To: Catalin Iacob
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-04 21:15 PST by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2013-04-18 17:29 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First attempt, guidance needed (7.79 KB, patch)
2013-04-14 04:25 PDT, Catalin Iacob
bzbarsky: review+
Details | Diff | Review
New version, addressing Boris' review (8.05 KB, patch)
2013-04-15 13:33 PDT, Catalin Iacob
no flags Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-04 21:15:25 PST
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 2013-04-14 04:25:00 PDT
Created attachment 737240 [details] [diff] [review]
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 [:njn] (on vacation until July 11) 2013-04-14 15:59:19 PDT
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 [:bz] (Out June 25-July 6) 2013-04-15 11:33:09 PDT
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 2013-04-15 13:33:46 PDT
Created attachment 737651 [details] [diff] [review]
New version, addressing Boris' review
Comment 5 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-04-15 16:20:26 PDT
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 [:njn] (on vacation until July 11) 2013-04-15 16:41:23 PDT
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 [:njn] (on vacation until July 11) 2013-04-15 22:38:20 PDT
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 [:njn] (on vacation until July 11) 2013-04-16 15:58:46 PDT
> 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 [:njn] (on vacation until July 11) 2013-04-16 22:10:16 PDT
> 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] 2013-04-17 09:59:39 PDT
https://hg.mozilla.org/mozilla-central/rev/4b63c7bfced0
Comment 11 Catalin Iacob 2013-04-18 13:39:02 PDT
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 [:njn] (on vacation until July 11) 2013-04-18 17:29:33 PDT
> 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 :)

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