mac: Delete more 10.5-only code

mac: Delete more 10.5-only code

BUG=137676
TBR=tony, sky

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=150878

[Nico, 2012-08-08 04:04:25]

mark: base/, webkit/
rsesek: chrome/browser
shess: chrome/common (mostly objc zombie stuff)
wez: remoting
sail: skia/ext
avi: ui/

[Nico, 2012-08-08 04:04:26]

mark: base/, webkit/
rsesek: chrome/browser
shess: chrome/common (mostly objc zombie stuff)
wez: remoting
sail: skia/ext
avi: ui/

[Nico, 2012-08-08 04:04:56]

On 2012/08/08 04:04:26, Nico wrote:
> mark: base/, webkit/
> rsesek: chrome/browser
> shess: chrome/common (mostly objc zombie stuff)
> wez: remoting
> sail: skia/ext
> avi: ui/

(33 files changed, 18 insertions(+), 520 deletions(-))

[Scott Hess - ex-Googler, 2012-08-08 04:26:14]

LGTM!

https://chromiumcodereview.appspot.com/10837158/diff/10001/chrome/common/mac/...
File chrome/common/mac/objc_zombie.mm (right):

https://chromiumcodereview.appspot.com/10837158/diff/10001/chrome/common/mac/...
chrome/common/mac/objc_zombie.mm:121: objc_destructInstance(self);
Sweet!

[Avi (use Gerrit), 2012-08-08 04:30:47]

https://chromiumcodereview.appspot.com/10837158/diff/10001/skia/ext/skia_util...
File skia/ext/skia_utils_mac.mm (left):

https://chromiumcodereview.appspot.com/10837158/diff/10001/skia/ext/skia_util...
skia/ext/skia_utils_mac.mm:75: // 10.5 is no longer supported we can drop this
and always use NSImageRep.
This comment implies that you can simplify the code. What happened there?

https://chromiumcodereview.appspot.com/10837158/diff/10001/skia/ext/skia_util...
File skia/ext/skia_utils_mac.mm (right):

https://chromiumcodereview.appspot.com/10837158/diff/10001/skia/ext/skia_util...
skia/ext/skia_utils_mac.mm:209: SkBitmap NSImageRepToSkBitmap(NSImageRep*
image_rep, NSSize size, bool is_opaque) {
80 col

https://chromiumcodereview.appspot.com/10837158/diff/10001/ui/gfx/scoped_ns_g...
File ui/gfx/scoped_ns_graphics_context_save_gstate_mac.mm (right):

https://chromiumcodereview.appspot.com/10837158/diff/10001/ui/gfx/scoped_ns_g...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.mm:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
This file isn't really necessary any more; we can now inline the calls.

[Nico, 2012-08-08 04:38:15]

https://chromiumcodereview.appspot.com/10837158/diff/10001/skia/ext/skia_util...
File skia/ext/skia_utils_mac.mm (left):

https://chromiumcodereview.appspot.com/10837158/diff/10001/skia/ext/skia_util...
skia/ext/skia_utils_mac.mm:75: // 10.5 is no longer supported we can drop this
and always use NSImageRep.
On 2012/08/08 04:30:48, Avi wrote:
> This comment implies that you can simplify the code. What happened there?

It's not clear to me how it could be simplified. I assumed it was a left-over
from an earlier iteration of the code, so I deleted it.

https://chromiumcodereview.appspot.com/10837158/diff/10001/skia/ext/skia_util...
File skia/ext/skia_utils_mac.mm (right):

https://chromiumcodereview.appspot.com/10837158/diff/10001/skia/ext/skia_util...
skia/ext/skia_utils_mac.mm:209: SkBitmap NSImageRepToSkBitmap(NSImageRep*
image_rep, NSSize size, bool is_opaque) {
On 2012/08/08 04:30:48, Avi wrote:
> 80 col

Done.

https://chromiumcodereview.appspot.com/10837158/diff/10001/ui/gfx/scoped_ns_g...
File ui/gfx/scoped_ns_graphics_context_save_gstate_mac.mm (right):

https://chromiumcodereview.appspot.com/10837158/diff/10001/ui/gfx/scoped_ns_g...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.mm:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
On 2012/08/08 04:30:48, Avi wrote:
> This file isn't really necessary any more; we can now inline the calls.

Hm, it doesn't really hurt, and it means file that include
scoped_ns_graphics_context_save_gstate_mac.h don't automatically get logging.h
and appkit.h, which both are pretty big headers. So I'd keep it.

[Jamie, 2012-08-08 06:42:26]

remoting lgtm.

[Avi (use Gerrit), 2012-08-08 13:29:11]

LGTM

[Robert Sesek, 2012-08-08 16:09:53]

LGTM

http://codereview.chromium.org/10837158/diff/8036/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (left):

http://codereview.chromium.org/10837158/diff/8036/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:807:
chrome::ShowObsoleteOSPrompt(browser);
I think you may need to leave this for the GTK one.

[Nico, 2012-08-08 17:21:24]

mark: ping

http://codereview.chromium.org/10837158/diff/8036/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (left):

http://codereview.chromium.org/10837158/diff/8036/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:807:
chrome::ShowObsoleteOSPrompt(browser);
On 2012/08/08 16:09:53, rsesek wrote:
> I think you may need to leave this for the GTK one.

Done, thanks.

[sail, 2012-08-08 20:02:28]

skia/ext and ui/gfx/video_decode_acceleration_support_mac_unittest.mm LGTM

[Mark Mentovai, 2012-08-09 16:03:43]

Sorry, I was out yesterday.

https://chromiumcodereview.appspot.com/10837158/diff/14002/base/mac/mac_util.h
File base/mac/mac_util.h (right):

https://chromiumcodereview.appspot.com/10837158/diff/14002/base/mac/mac_util....
base/mac/mac_util.h:132: BASE_EXPORT bool IsOSSnowLeopardOrLater();
You can get rid of this now too.

https://chromiumcodereview.appspot.com/10837158/diff/14002/base/mac/mac_util....
base/mac/mac_util.h:157: inline bool IsOSSnowLeopardOrLater() { return true; }
And this.

https://chromiumcodereview.appspot.com/10837158/diff/14002/base/mac/mac_util.mm
File base/mac/mac_util.mm (right):

https://chromiumcodereview.appspot.com/10837158/diff/14002/base/mac/mac_util....
base/mac/mac_util.mm:583: bool IsOSSnowLeopardOrLater() {
And this.

https://chromiumcodereview.appspot.com/10837158/diff/14002/base/process_util_...
File base/process_util_mac.mm (right):

https://chromiumcodereview.appspot.com/10837158/diff/14002/base/process_util_...
base/process_util_mac.mm:778: if (base::mac::IsOSSnowLeopardOrEarlier()) {
This was originally written in the obtuse way it was written intentionally (I
think against my suggestion that it be written the way you’re changing it to
be). To follow the original philosophy, make this IsOSSnowLeopard() without the
OrEarlier() bit to assert that we don’t make any guarantees about this code if
you try running it on non-Snow Leopard.

(Because it’ll work on Leopard but not Tiger or earlier.)

https://chromiumcodereview.appspot.com/10837158/diff/14002/chrome/browser/pro...
File chrome/browser/process_info_snapshot_mac.cc (right):

https://chromiumcodereview.appspot.com/10837158/diff/14002/chrome/browser/pro...
chrome/browser/process_info_snapshot_mac.cc:188: static bool
GetProcessMemoryInfoUsingTop(
I hate this file anyway.

https://chromiumcodereview.appspot.com/10837158/diff/14002/chrome/common/mac/...
File chrome/common/mac/objc_zombie.mm (left):

https://chromiumcodereview.appspot.com/10837158/diff/14002/chrome/common/mac/...
chrome/common/mac/objc_zombie.mm:146: if (nlist(objc_runtime_path, nl) < 0 ||
I’m very happy to see this code go.

https://chromiumcodereview.appspot.com/10837158/diff/14002/ui/gfx/scoped_ns_g...
File ui/gfx/scoped_ns_graphics_context_save_gstate_mac.mm (left):

https://chromiumcodereview.appspot.com/10837158/diff/14002/ui/gfx/scoped_ns_g...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.mm:21: if (!context_ &&
base::mac::IsOSLeopardOrEarlier()) {
This was really heinous too.

https://chromiumcodereview.appspot.com/10837158/diff/14002/ui/gfx/video_decod...
File ui/gfx/video_decode_acceleration_support_mac_unittest.mm (right):

https://chromiumcodereview.appspot.com/10837158/diff/14002/ui/gfx/video_decod...
ui/gfx/video_decode_acceleration_support_mac_unittest.mm:40:
base::SysInfo::OperatingSystemVersionNumbers(&major, &minor, &bugfix);
Maybe you should leave behind something like IsOSSnowLeopard in a comment so
that when we eventually deprecate Snow Leopard it’ll be easy to find and kill
this function.

https://chromiumcodereview.appspot.com/10837158/diff/14002/webkit/plugins/npa...
File webkit/plugins/npapi/webplugin_delegate_impl_mac.mm (right):

https://chromiumcodereview.appspot.com/10837158/diff/14002/webkit/plugins/npa...
webkit/plugins/npapi/webplugin_delegate_impl_mac.mm:962: // Currently the plugin
IME implementation only works on 10.6.
Stale comment?

[Nico, 2012-08-09 16:05:52]

Thanks, I'll address your comments in the other files soon.

https://chromiumcodereview.appspot.com/10837158/diff/14002/base/mac/mac_util.h
File base/mac/mac_util.h (right):

https://chromiumcodereview.appspot.com/10837158/diff/14002/base/mac/mac_util....
base/mac/mac_util.h:132: BASE_EXPORT bool IsOSSnowLeopardOrLater();
On 2012/08/09 16:03:43, Mark Mentovai wrote:
> You can get rid of this now too.

Good point. Would you mind if I do that in a different CL?

[Mark Mentovai, 2012-08-09 16:06:30]

Yes, in fact it’s preferable to do that in a separate change so that I don’t
need to do a substantive review of all of these files again.

[Nico, 2012-08-09 17:36:40]

https://chromiumcodereview.appspot.com/10837158/diff/14002/base/process_util_...
File base/process_util_mac.mm (right):

https://chromiumcodereview.appspot.com/10837158/diff/14002/base/process_util_...
base/process_util_mac.mm:778: if (base::mac::IsOSSnowLeopardOrEarlier()) {
On 2012/08/09 16:03:43, Mark Mentovai wrote:
> This was originally written in the obtuse way it was written intentionally (I
> think against my suggestion that it be written the way you’re changing it to
> be). To follow the original philosophy, make this IsOSSnowLeopard() without
the
> OrEarlier() bit to assert that we don’t make any guarantees about this code if
> you try running it on non-Snow Leopard.
> 
> (Because it’ll work on Leopard but not Tiger or earlier.)

Done.

https://chromiumcodereview.appspot.com/10837158/diff/14002/ui/gfx/video_decod...
File ui/gfx/video_decode_acceleration_support_mac_unittest.mm (right):

https://chromiumcodereview.appspot.com/10837158/diff/14002/ui/gfx/video_decod...
ui/gfx/video_decode_acceleration_support_mac_unittest.mm:40:
base::SysInfo::OperatingSystemVersionNumbers(&major, &minor, &bugfix);
On 2012/08/09 16:03:43, Mark Mentovai wrote:
> Maybe you should leave behind something like IsOSSnowLeopard in a comment so
> that when we eventually deprecate Snow Leopard it’ll be easy to find and kill
> this function.

Done.

https://chromiumcodereview.appspot.com/10837158/diff/14002/webkit/plugins/npa...
File webkit/plugins/npapi/webplugin_delegate_impl_mac.mm (right):

https://chromiumcodereview.appspot.com/10837158/diff/14002/webkit/plugins/npa...
webkit/plugins/npapi/webplugin_delegate_impl_mac.mm:962: // Currently the plugin
IME implementation only works on 10.6.
On 2012/08/09 16:03:43, Mark Mentovai wrote:
> Stale comment?

Done.

[Mark Mentovai, 2012-08-09 18:21:00]

LGTM

[commit-bot: I haz the power, 2012-08-09 18:25:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10837158/9038

[commit-bot: I haz the power, 2012-08-09 18:25:51]

Presubmit check for 10837158-9038 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit/plugins/npapi
    ui/gfx

Presubmit checks took 3.0s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Nico, 2012-08-09 18:26:57]

tbr sky and tony for ui/gfx and webkit OWNERS

[commit-bot: I haz the power, 2012-08-09 18:27:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10837158/9038

[commit-bot: I haz the power, 2012-08-09 19:44:57]

Try job failure for 10837158-9038 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...