The default bug view has changed. See this FAQ.

Get rid of nsCocoaFeatures

RESOLVED FIXED in mozilla15

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: zpao, Assigned: heycam)

Tracking

Trunk
mozilla15
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Everything in it is in nsToolkit (with minor differences).

It looks like it's only used by WebGLContextGL.cpp but I can't tell if it's actually needed there. It looks like we can just us nsToolkit instead.
(Assignee)

Comment 1

5 years ago
We can't include nsToolkit.h in the files that currently use nsCocoaFeatures since they're not Objective-C files.  What we could do is have nsToolkit derive from nsCocoaFeatures and eliminate the duplicate functions.
Assignee: nobody → cam
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 598638 [details] [diff] [review]
Remove duplicate functions on nsCocoaFeatures and nsToolkit.
Attachment #598638 - Flags: review?(bgirard)
(Assignee)

Comment 3

5 years ago
(This might be bug 676907, too.)
Sorry it took me so long to respond. I'm not a fan of having nsToolkit extend nsCocoaFeatures. Wouldn't it be simpler to refactor the code to use nsCocoaFeatures instead?
(Assignee)

Comment 5

5 years ago
Created attachment 603917 [details] [diff] [review]
Remove duplicate functions on nsCocoaFeatures and nsToolkit. (v1.1)

OK, that works too.
Attachment #603917 - Flags: review?(bgirard)
(Assignee)

Updated

5 years ago
Attachment #598638 - Attachment is obsolete: true
Attachment #598638 - Flags: review?(bgirard)
Comment on attachment 603917 [details] [diff] [review]
Remove duplicate functions on nsCocoaFeatures and nsToolkit. (v1.1)

We don't need to expose SupportCoreAnimationPlugins in nsToolkit, so r+ with that changes. It would of been nice to just remove the function from nsToolkit and replace any usage of nsToolkit OSVersion to use nsCocoaFeatures but this is already an improve so i'd be happy to take it as-is.

If you're not aware you can use mxr.mozilla.org to where find the functions are used.

Also feel free to ping and/or reassign review if it takes too long in the future.
Attachment #603917 - Flags: review?(bgirard) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 603967 [details] [diff] [review]
Remove duplicate functions on nsCocoaFeatures and nsToolkit. (v1.2)

You're right, replacing the nsToolkit::Blah() calls to nsCocoaFeatures::Blah()
would be better.

Re review time tbh I just requested review and forgot about the bug since it
wasn't anything urgent. :)
(Assignee)

Updated

5 years ago
Attachment #603917 - Attachment is obsolete: true
Cameron, is this fine to land? (no rush, I was looking at these files again and got reminded about this)
(Assignee)

Comment 9

5 years ago
Yes, I just forgot about this bug.  Will land momentarily.
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f577f905f3e
Target Milestone: --- → mozilla15
Version: unspecified → Trunk
(Assignee)

Comment 11

5 years ago
Backed out because I was a cowboy and didn't re-run my two month old patch through try: https://hg.mozilla.org/integration/mozilla-inbound/rev/521cd92d3c08
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/12f13acb5ea8
http://hg.mozilla.org/mozilla-central/rev/12f13acb5ea8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.