1694853 - Cleanup in native menu code, round 2

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, task)

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1694853 - Make nsMenuIconX non-refcounted. r=harry

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1694853 - Move nsMenuItemX::Create into the constructor. r=harry

Depends on D106367

Comment 3

[Markus Stange [:mstange]]

Attachment: Bug 1694853 - Move nsMenuX::Create into the constructor. r=harry

Depends on D106368

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1694853 - Use UniquePtr in more places. r=harry

Depends on D106369

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1694853 - Replace getter and setter calls with property gets and sets. r=harry

This does the same but looks more modern.

Depends on D106370

Comment 6

[Markus Stange [:mstange]]

Attachment: Bug 1694853 - Replace outdated 'in' prefix with the more common 'a' prefix. r=harry

Depends on D106371

Comment 7

[Markus Stange [:mstange]]

Attachment: Bug 1694853 - Split mDestroyHandlerCalled into mDidFirePopupHiding and mDidFirePopupHidden, for increased clarity. r=harry

Depends on D106372

Comment 8

[Markus Stange [:mstange]]

Attachment: Bug 1694853 - Use return value instead of outparam for GetMenuPopupContent. r=harry

Depends on D106373

Comment 9

[Markus Stange [:mstange]]

Attachment: Bug 1694853 - Remove some commented out logging code. r=harry

Depends on D106374

Comment 10

[Markus Stange [:mstange]]

Attachment: Bug 1694853 - Remove nsMenuObjectX::Content() and put it on the individual subclasses instead. r=harry

This removes some polymorphism and makes it easier to understand what's actually going on.
The explicit if checks added in this patch will go away once nsMenuX and nsMenuItemX are unified.

Depends on D106375

Comment 11

[Markus Stange [:mstange]]

Attachment: Bug 1694853 - Rename arguments in nsMenuBarX methods to use aArgument style. r=harry

Depends on D106376

Comment 12

[Markus Stange [:mstange]]

Attachment: Bug 1694853 - Use return value instead of outparam for HideItem. r=harry

Depends on D106377

Comment 13

[Markus Stange [:mstange]]

Attachment: Bug 1694853 - Stop giving the nsMenuBarX a reference to the widget. r=harry

Depends on D106378

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c869421dd3b6
Make nsMenuIconX non-refcounted. r=harry
https://hg.mozilla.org/integration/autoland/rev/4276c8091286
Move nsMenuItemX::Create into the constructor. r=harry
https://hg.mozilla.org/integration/autoland/rev/d2eec7a26530
Move nsMenuX::Create into the constructor. r=harry
https://hg.mozilla.org/integration/autoland/rev/6af34c8413f2
Use UniquePtr in more places. r=harry
https://hg.mozilla.org/integration/autoland/rev/2c5bfe90197d
Replace getter and setter calls with property gets and sets. r=harry
https://hg.mozilla.org/integration/autoland/rev/ba0b5e1f127f
Replace outdated 'in' prefix with the more common 'a' prefix. r=harry
https://hg.mozilla.org/integration/autoland/rev/af5900747565
Split mDestroyHandlerCalled into mDidFirePopupHiding and mDidFirePopupHidden, for increased clarity. r=harry
https://hg.mozilla.org/integration/autoland/rev/e6a822a5b9b2
Use return value instead of outparam for GetMenuPopupContent. r=harry
https://hg.mozilla.org/integration/autoland/rev/95a3539d8c75
Remove some commented out logging code. r=harry
https://hg.mozilla.org/integration/autoland/rev/d1bf74c97927
Remove nsMenuObjectX::Content() and put it on the individual subclasses instead. r=harry
https://hg.mozilla.org/integration/autoland/rev/fe34667b3e31
Rename arguments in nsMenuBarX methods to use aArgument style. r=harry
https://hg.mozilla.org/integration/autoland/rev/a0b294c35df5
Use return value instead of outparam for HideItem. r=harry
https://hg.mozilla.org/integration/autoland/rev/5f760bdb82bf
Stop giving the nsMenuBarX a reference to the widget. r=harry

Comment 15

[Natalia Csoregi [:nataliaCs]]

Backed out for causing bustages on nsMenuBarX.mm

backout: https://hg.mozilla.org/integration/autoland/rev/19297968e487e711d38fc1508eba3dc3e8e2a4b2

push: https://treeherder.mozilla.org/jobs?repo=autoland&revision=5f760bdb82bfa02042f3c217c019a8dc50582c99&group_state=expanded&selectedTaskRun=DDOVfOwUQ9ep8MplLK6miw.0

failure log: https://treeherder.mozilla.org/logviewer?job_id=331682793&repo=autoland&lineNumber=41307

[task 2021-03-02T02:13:14.467Z] 02:13:14 INFO - In file included from Unified_mm_widget_cocoa1.mm:92:
[task 2021-03-02T02:13:14.467Z] 02:13:14 ERROR - /builds/worker/checkouts/gecko/widget/cocoa/nsMenuBarX.mm:483:5: error: Unused value of must-use type 'already_AddRefed<mozilla::dom::Element>'
[task 2021-03-02T02:13:14.467Z] 02:13:14 INFO - HideItem(domDoc, u"aboutSeparator"_ns);
[task 2021-03-02T02:13:14.467Z] 02:13:14 INFO - ^
[task 2021-03-02T02:13:14.467Z] 02:13:14 ERROR - /builds/worker/checkouts/gecko/widget/cocoa/nsMenuBarX.mm:490:5: error: Unused value of must-use type 'already_AddRefed<mozilla::dom::Element>'
[task 2021-03-02T02:13:14.467Z] 02:13:14 INFO - HideItem(domDoc, u"menu_FileQuitSeparator"_ns);
[task 2021-03-02T02:13:14.467Z] 02:13:14 INFO - ^
[task 2021-03-02T02:13:14.467Z] 02:13:14 ERROR - /builds/worker/checkouts/gecko/widget/cocoa/nsMenuBarX.mm:498:5: error: Unused value of must-use type 'already_AddRefed<mozilla::dom::Element>'
[task 2021-03-02T02:13:14.468Z] 02:13:14 INFO - HideItem(domDoc, u"menu_PrefsSeparator"_ns);
[task 2021-03-02T02:13:14.468Z] 02:13:14 INFO - ^

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26d9473effa7
Make nsMenuIconX non-refcounted. r=harry
https://hg.mozilla.org/integration/autoland/rev/5f04fabbda74
Move nsMenuItemX::Create into the constructor. r=harry
https://hg.mozilla.org/integration/autoland/rev/7483b04409d6
Move nsMenuX::Create into the constructor. r=harry
https://hg.mozilla.org/integration/autoland/rev/8469ccbdd11b
Use UniquePtr in more places. r=harry
https://hg.mozilla.org/integration/autoland/rev/8000f1b854b2
Replace getter and setter calls with property gets and sets. r=harry
https://hg.mozilla.org/integration/autoland/rev/87e88d7cc395
Replace outdated 'in' prefix with the more common 'a' prefix. r=harry
https://hg.mozilla.org/integration/autoland/rev/8a130665c1e8
Split mDestroyHandlerCalled into mDidFirePopupHiding and mDidFirePopupHidden, for increased clarity. r=harry
https://hg.mozilla.org/integration/autoland/rev/0b3de65fd6db
Use return value instead of outparam for GetMenuPopupContent. r=harry
https://hg.mozilla.org/integration/autoland/rev/8d3288672d58
Remove some commented out logging code. r=harry
https://hg.mozilla.org/integration/autoland/rev/b58aa3f4d623
Remove nsMenuObjectX::Content() and put it on the individual subclasses instead. r=harry
https://hg.mozilla.org/integration/autoland/rev/aead89217550
Rename arguments in nsMenuBarX methods to use aArgument style. r=harry
https://hg.mozilla.org/integration/autoland/rev/6a64177bdcb5
Use return value instead of outparam for HideItem. r=harry
https://hg.mozilla.org/integration/autoland/rev/ec9e249d963e
Stop giving the nsMenuBarX a reference to the widget. r=harry

Comment 17

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/26d9473effa7
https://hg.mozilla.org/mozilla-central/rev/5f04fabbda74
https://hg.mozilla.org/mozilla-central/rev/7483b04409d6
https://hg.mozilla.org/mozilla-central/rev/8469ccbdd11b
https://hg.mozilla.org/mozilla-central/rev/8000f1b854b2
https://hg.mozilla.org/mozilla-central/rev/87e88d7cc395
https://hg.mozilla.org/mozilla-central/rev/8a130665c1e8
https://hg.mozilla.org/mozilla-central/rev/0b3de65fd6db
https://hg.mozilla.org/mozilla-central/rev/8d3288672d58
https://hg.mozilla.org/mozilla-central/rev/b58aa3f4d623
https://hg.mozilla.org/mozilla-central/rev/aead89217550
https://hg.mozilla.org/mozilla-central/rev/6a64177bdcb5
https://hg.mozilla.org/mozilla-central/rev/ec9e249d963e