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

RESOLVED FIXED in mozilla23

Status

()

Core
XUL
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: njn, Assigned: Catalin Iacob)

Tracking

unspecified
mozilla23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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*>.
(Reporter)

Updated

5 years ago
Whiteboard: [MemShrink]
(Assignee)

Comment 1

4 years ago
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?
(Reporter)

Comment 2

4 years ago
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.
Attachment #737240 - Flags: review?(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
Attachment #737240 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 4

4 years ago
Created attachment 737651 [details] [diff] [review]
New version, addressing Boris' review
Attachment #737240 - Attachment is obsolete: true
(Reporter)

Comment 5

4 years ago
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!
(Reporter)

Comment 6

4 years ago
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
(Reporter)

Comment 7

4 years ago
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
(Reporter)

Comment 8

4 years ago
> 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
(Reporter)

Comment 9

4 years ago
> 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
https://hg.mozilla.org/mozilla-central/rev/4b63c7bfced0
Assignee: nobody → iacobcatalin
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(Assignee)

Comment 11

4 years ago
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.
(Reporter)

Comment 12

4 years ago
> 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 :)
You need to log in before you can comment on or make changes to this bug.