Closed Bug 1698662 Opened 9 months ago Closed 9 months ago

Cleanup in native menu code, round 3

Categories

(Core :: Widget: Cocoa, task, P1)

All
macOS
task

Tracking

()

RESOLVED FIXED
88 Branch
Tracking Status
firefox88 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

(Blocks 1 open bug)

Details

(Whiteboard: [proton-context-menus][mac:mr1])

Attachments

(13 files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
No description provided.

A future patch in this series will move the check all the way out of nsMenuItemIconX, and this is the first step.

This removes another nsIContent* member variable, which is always a good thing.

Depends on D108538

Now the nsMenuItemIconX no longer needs to have a reference to the NSMenuItem,
and the ownership becomes clearer.

Depends on D108539

mSetIcon was used to make it so that, if we have a loaded icon but start a new
load (for example because the "image" attribute changed), we keep the old icon
until the new icon is loaded rather than replacing it with a placeholder icon.

This patch simplifies the logic to the following: Set a placeholder if an icon
load is in progress and there is no other existing icon.

Depends on D108540

It's fine if the icon loader outlives the nsMenuItemIconX object.
In the nsMenuItemIconX destructor we call mIconLoader->Destroy() so the
icon loader will not have a dangling reference to nsMenuItemIconX.
I fixed this in bug 1691861 and forgot to remove this comment.

Depends on D108542

This removes some usage of void*. Now it's always clear what the type of the
native data is.
It also means that some places need to check all subclasses. This even caught a bug.

Depends on D108546

Blocks: 1699252
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/bf0c57e15fbe
Move these checks out of GetIconURI. r=harry
https://hg.mozilla.org/integration/autoland/rev/728767bc102b
Remove unnecessary null check. nsMenuItemIconX is always created with a non-null mContent. r=harry
https://hg.mozilla.org/integration/autoland/rev/3efbae4a7dbc
Clean up nsMenuItemIconX::GetIconURI(). r=harry
https://hg.mozilla.org/integration/autoland/rev/d3c172c04f9d
Move the radio/checkbox menuitem check into nsMenuItemX. r=harry
https://hg.mozilla.org/integration/autoland/rev/3e050ec96d60
Remove nsMenuItemIconX's reference to the nsIContent. r=harry
https://hg.mozilla.org/integration/autoland/rev/dd4dd96d949e
Make nsMenuItemX and nsMenuX responsible for updating the icon on the NSMenuItem. r=harry
https://hg.mozilla.org/integration/autoland/rev/35ab9e0a4c9b
Remove mSetIcon. r=harry
https://hg.mozilla.org/integration/autoland/rev/fd2a85ca1a5d
Move part of SetupIcon into a new method StartIconLoad. r=harry
https://hg.mozilla.org/integration/autoland/rev/dc1c96ca5060
Remove misleading comment. r=harry
https://hg.mozilla.org/integration/autoland/rev/7fb2ac7e64f3
Improve comments on nsChangeObserver. r=harry
https://hg.mozilla.org/integration/autoland/rev/c28f71b21f19
Use container variable here instead of calling GetParent again. container is already the parent. r=harry
https://hg.mozilla.org/integration/autoland/rev/285b71fdcf58
Rename arguments in nsMenuGroupOwnerX. r=harry
https://hg.mozilla.org/integration/autoland/rev/f5c119cb61c4
Remove nsMenuObjectX::GetNativeData and replace it with NativeNSMenu on the various subclasses. r=harry
You need to log in before you can comment on or make changes to this bug.