1466727 - Remove nsITreeColumn

Status: RESOLVED FIXED

(Core :: XUL, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

Jorg, this one will likely need Thunderbird bits too.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Mark nsITreeColumn builtinclass

The only JS implementation seems to be in a test, and we can stop doing that.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove use of nsITreeColumn in xpidl interfaces

There is one actual behavior change here, in the webidl version of
TreeBoxObject::GetCellAt.  I believe this change fixes a leak of the
nsTreeColumn, but could use careful review.

I tried to avoid changes not needed to get this compiling.  There will be a lot
more cleanup in the next few changesets.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Stop pretending like the mContent of an nsTreeColumn can be null

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Stop using nsITreeColumn in accessibility code

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Stop using nsITreeColumn in toolkit

I had to do window.TreeColumn instead of just TreeColumn to work around eslint
being buggy.  That bug is tracked in bug 1466842.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Stop using nsITreeColumn in layout

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Remove nsITreeColumn

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 8983410 [details] [diff] [review]
part 3.  Stop pretending like the mContent of an nsTreeColumn can be null

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

More context in the diffs would be appreciated in general.

::: layout/xul/tree/nsTreeColumns.h
@@ +64,5 @@
>    nsIContent* GetParentObject() const;
>    virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
>  
> +  // Never returns null, but is named GetElement to avoid
> +  // name-colliding with the Element type.

I'm confused by this comment, it doesn't seem to be named GetElement now?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Mark nsITreeColumn builtinclass

The only JS implementation seems to be in a test, and we can stop doing that.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove use of nsITreeColumn in xpidl interfaces

There is one actual behavior change here, in the webidl version of
TreeBoxObject::GetCellAt.  I believe this change fixes a leak of the
nsTreeColumn, but could use careful review.

I tried to avoid changes not needed to get this compiling.  There will be a lot
more cleanup in the next few changesets.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Stop pretending like the mContent of an nsTreeColumn can be null

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Stop using nsITreeColumn in accessibility code

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Stop using nsITreeColumn in toolkit

I had to do window.TreeColumn instead of just TreeColumn to work around eslint
being buggy.  That bug is tracked in bug 1466842.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I'm confused by this comment, it doesn't seem to be named GetElement now?

Good catch; the comment predates some other changes I made.  I'm just removing it.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Stop pretending like the mContent of an nsTreeColumn can be null

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 diff -w for review convenience

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 8983480 [details] [diff] [review]
part 4.  Stop using nsITreeColumn in accessibility code

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

::: accessible/base/nsCoreUtils.cpp
@@ +574,4 @@
>  }
>  
>  already_AddRefed<nsTreeColumn>
> +nsCoreUtils::GetNextSensibleColumn(nsTreeColumn *aColumn)

nit: type* name and below

::: accessible/xul/XULTreeGridAccessible.cpp
@@ +431,5 @@
>  
>    NS_ASSERTION(mTreeView, "mTreeView is null");
>  
> +  int16_t type = mColumn->Type();
> +  if (type == dom::TreeColumnBinding::TYPE_CHECKBOX)

it'd be nice to get rid of local variables and do if (mColumn->Type() ==

@@ +540,1 @@
>    if (isCycler)

here too and below

@@ +626,4 @@
>  void
>  XULTreeGridCellAccessible::ColHeaderCells(nsTArray<Accessible*>* aHeaderCells)
>  {
> +  RefPtr<dom::Element> columnElm = mColumn->Element();

dom::Element*

@@ +816,4 @@
>    if (NS_FAILED(rv) || !isEditable)
>      return false;
>  
> +  RefPtr<dom::Element> columnElm = mColumn->Element();

dom::Element*

Comment 18

[Boris Zbarsky [:bzbarsky]]

> nit: type* name and below

Done.

>it'd be nice to get rid of local variables and do if (mColumn->Type() ==
...
>here too and below

Done.

>dom::Element*

Done, both places.

Thank you for the review!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/824b74c6269d
part 1.  Mark nsITreeColumn builtinclass.  r=mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/96c6c5a075a4
part 2.  Remove use of nsITreeColumn in xpidl interfaces.  r=mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/55feb670e070
part 3.  Stop pretending like the mContent of an nsTreeColumn can be null.  r=mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/a819042f1909
part 4.  Stop using nsITreeColumn in accessibility code.  r=surkov
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cc696a399ed
part 5.  Stop using nsITreeColumn in toolkit.  r=mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f9c0f55aa88
part 6.  Stop using nsITreeColumn in layout.  r=mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/830e780325e4
part 7.  Remove nsITreeColumn.  r=mossop

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/824b74c6269d
https://hg.mozilla.org/mozilla-central/rev/96c6c5a075a4
https://hg.mozilla.org/mozilla-central/rev/55feb670e070
https://hg.mozilla.org/mozilla-central/rev/a819042f1909
https://hg.mozilla.org/mozilla-central/rev/3cc696a399ed
https://hg.mozilla.org/mozilla-central/rev/4f9c0f55aa88
https://hg.mozilla.org/mozilla-central/rev/830e780325e4