Closed
Bug 1466727
Opened 3 years ago
Closed 3 years ago
Remove nsITreeColumn
Categories
(Core :: XUL, enhancement, P3)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(8 files, 6 obsolete files)
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.
![]() |
Assignee | |
Comment 1•3 years ago
|
||
The only JS implementation seems to be in a test, and we can stop doing that.
Attachment #8983408 -
Flags: review?(dtownsend)
![]() |
Assignee | |
Updated•3 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 2•3 years ago
|
||
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)
![]() |
Assignee | |
Comment 3•3 years ago
|
||
Attachment #8983410 -
Flags: review?(dtownsend)
![]() |
Assignee | |
Comment 4•3 years ago
|
||
Attachment #8983411 -
Flags: review?(surkov.alexander)
![]() |
Assignee | |
Comment 5•3 years ago
|
||
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)
![]() |
Assignee | |
Comment 6•3 years ago
|
||
Attachment #8983413 -
Flags: review?(dtownsend)
![]() |
Assignee | |
Comment 7•3 years ago
|
||
Attachment #8983414 -
Flags: review?(dtownsend)
Updated•3 years ago
|
Attachment #8983408 -
Flags: review?(dtownsend) → review+
Comment 8•3 years ago
|
||
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?
Updated•3 years ago
|
Attachment #8983412 -
Flags: review?(dtownsend) → review+
Updated•3 years ago
|
Attachment #8983413 -
Flags: review?(dtownsend) → review+
Updated•3 years ago
|
Attachment #8983414 -
Flags: review?(dtownsend) → review+
![]() |
Assignee | |
Comment 9•3 years ago
|
||
The only JS implementation seems to be in a test, and we can stop doing that.
Attachment #8983477 -
Flags: review?(dtownsend)
![]() |
Assignee | |
Updated•3 years ago
|
Attachment #8983408 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 10•3 years ago
|
||
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)
![]() |
Assignee | |
Updated•3 years ago
|
Attachment #8983409 -
Attachment is obsolete: true
Attachment #8983409 -
Flags: review?(dtownsend)
![]() |
Assignee | |
Comment 11•3 years ago
|
||
Attachment #8983479 -
Flags: review?(dtownsend)
![]() |
Assignee | |
Updated•3 years ago
|
Attachment #8983410 -
Attachment is obsolete: true
Attachment #8983410 -
Flags: review?(dtownsend)
![]() |
Assignee | |
Comment 12•3 years ago
|
||
Attachment #8983480 -
Flags: review?(surkov.alexander)
![]() |
Assignee | |
Updated•3 years ago
|
Attachment #8983411 -
Attachment is obsolete: true
Attachment #8983411 -
Flags: review?(surkov.alexander)
![]() |
Assignee | |
Comment 13•3 years ago
|
||
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)
![]() |
Assignee | |
Updated•3 years ago
|
Attachment #8983412 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•3 years ago
|
Attachment #8983477 -
Flags: review?(dtownsend)
![]() |
Assignee | |
Updated•3 years ago
|
Attachment #8983481 -
Flags: review?(dtownsend)
![]() |
Assignee | |
Comment 14•3 years ago
|
||
> 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.
![]() |
Assignee | |
Comment 15•3 years ago
|
||
Attachment #8983482 -
Flags: review?(dtownsend)
![]() |
Assignee | |
Updated•3 years ago
|
Attachment #8983479 -
Attachment is obsolete: true
Attachment #8983479 -
Flags: review?(dtownsend)
![]() |
Assignee | |
Comment 16•3 years ago
|
||
Updated•3 years ago
|
Priority: -- → P3
Updated•3 years ago
|
Attachment #8983478 -
Flags: review?(dtownsend) → review+
Updated•3 years ago
|
Attachment #8983482 -
Flags: review?(dtownsend) → review+
Comment 17•3 years ago
|
||
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+
![]() |
Assignee | |
Comment 18•3 years ago
|
||
> 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•3 years ago
|
||
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
Comment 20•3 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•