Last Comment Bug 715024 - Get rid of nsCocoaFeatures
: Get rid of nsCocoaFeatures
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Cameron McCormack (:heycam) (away Sep 28)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-03 16:40 PST by Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
Modified: 2012-05-17 11:17 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove duplicate functions on nsCocoaFeatures and nsToolkit. (4.58 KB, patch)
2012-02-19 01:51 PST, Cameron McCormack (:heycam) (away Sep 28)
no flags Details | Diff | Splinter Review
Remove duplicate functions on nsCocoaFeatures and nsToolkit. (v1.1) (5.92 KB, patch)
2012-03-07 16:52 PST, Cameron McCormack (:heycam) (away Sep 28)
b56girard: review+
Details | Diff | Splinter Review
Remove duplicate functions on nsCocoaFeatures and nsToolkit. (v1.2) (25.86 KB, patch)
2012-03-07 20:09 PST, Cameron McCormack (:heycam) (away Sep 28)
no flags Details | Diff | Splinter Review

Description Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-03 16:40:55 PST
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.
Comment 1 Cameron McCormack (:heycam) (away Sep 28) 2012-02-19 01:50:51 PST
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.
Comment 2 Cameron McCormack (:heycam) (away Sep 28) 2012-02-19 01:51:21 PST
Created attachment 598638 [details] [diff] [review]
Remove duplicate functions on nsCocoaFeatures and nsToolkit.
Comment 3 Cameron McCormack (:heycam) (away Sep 28) 2012-02-19 01:57:19 PST
(This might be bug 676907, too.)
Comment 4 Benoit Girard (:BenWa) 2012-03-07 14:51:36 PST
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?
Comment 5 Cameron McCormack (:heycam) (away Sep 28) 2012-03-07 16:52:00 PST
Created attachment 603917 [details] [diff] [review]
Remove duplicate functions on nsCocoaFeatures and nsToolkit. (v1.1)

OK, that works too.
Comment 6 Benoit Girard (:BenWa) 2012-03-07 18:18:59 PST
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.
Comment 7 Cameron McCormack (:heycam) (away Sep 28) 2012-03-07 20:09:05 PST
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. :)
Comment 8 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-04-30 18:09:16 PDT
Cameron, is this fine to land? (no rush, I was looking at these files again and got reminded about this)
Comment 9 Cameron McCormack (:heycam) (away Sep 28) 2012-05-16 18:38:54 PDT
Yes, I just forgot about this bug.  Will land momentarily.
Comment 10 Cameron McCormack (:heycam) (away Sep 28) 2012-05-16 18:40:07 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f577f905f3e
Comment 11 Cameron McCormack (:heycam) (away Sep 28) 2012-05-16 19:14:42 PDT
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
Comment 12 Cameron McCormack (:heycam) (away Sep 28) 2012-05-17 00:54:59 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/12f13acb5ea8
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-05-17 11:17:56 PDT
http://hg.mozilla.org/mozilla-central/rev/12f13acb5ea8

Note You need to log in before you can comment on or make changes to this bug.