Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(8 attachments, 6 obsolete attachments)

16.76 KB, patch
mossop
: review+
Details | Diff | Splinter Review
7.17 KB, patch
mossop
: review+
Details | Diff | Splinter Review
1.66 KB, patch
Details | Diff | Splinter Review
46.16 KB, patch
mossop
: review+
Details | Diff | Splinter Review
7.21 KB, patch
surkov
: review+
Details | Diff | Splinter Review
2.02 KB, patch
Details | Diff | Splinter Review
5.92 KB, patch
mossop
: review+
Details | Diff | Splinter Review
4.24 KB, patch
Details | Diff | Splinter Review
Jorg, this one will likely need Thunderbird bits too.
The only JS implementation seems to be in a test, and we can stop doing that.
Attachment #8983408 - Flags: review?(dtownsend)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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.
Attachment #8983409 - Flags: review?(dtownsend)
I had to do window.TreeColumn instead of just TreeColumn to work around eslint
being buggy.  That bug is tracked in bug 1466842.
Attachment #8983412 - Flags: review?(dtownsend)
Attachment #8983408 - Flags: review?(dtownsend) → review+
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?
Attachment #8983412 - Flags: review?(dtownsend) → review+
Attachment #8983413 - Flags: review?(dtownsend) → review+
Attachment #8983414 - Flags: review?(dtownsend) → review+
The only JS implementation seems to be in a test, and we can stop doing that.
Attachment #8983477 - Flags: review?(dtownsend)
Attachment #8983408 - Attachment is obsolete: true
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.
Attachment #8983478 - Flags: review?(dtownsend)
Attachment #8983409 - Attachment is obsolete: true
Attachment #8983409 - Flags: review?(dtownsend)
Attachment #8983410 - Attachment is obsolete: true
Attachment #8983410 - Flags: review?(dtownsend)
Attachment #8983411 - Attachment is obsolete: true
Attachment #8983411 - Flags: review?(surkov.alexander)
I had to do window.TreeColumn instead of just TreeColumn to work around eslint
being buggy.  That bug is tracked in bug 1466842.
Attachment #8983481 - Flags: review?(dtownsend)
Attachment #8983412 - Attachment is obsolete: true
Attachment #8983477 - Flags: review?(dtownsend)
Attachment #8983481 - Flags: review?(dtownsend)
> 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.
Attachment #8983479 - Attachment is obsolete: true
Attachment #8983479 - Flags: review?(dtownsend)
Priority: -- → P3
Attachment #8983478 - Flags: review?(dtownsend) → review+
Attachment #8983482 - Flags: review?(dtownsend) → review+
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*
Attachment #8983480 - Flags: review?(surkov.alexander) → review+
> 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!
Pushed by bzbarsky@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.