Always call the class methods to save/restore contexts.

Always call the class methods to save/restore contexts.

If the current context was ever nil, sending a "restore" message would be a no-op and would likely leave an out-of-scope context as current.

BUG=90140
TEST=repeatedly select items in an open file dialog in column mode; no crash

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95611

Reverted: http://src.chromium.org/viewvc/chrome?view=rev&revision=95631

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95686

[Avi (use Gerrit), 2011-08-04 22:10:58]

You heard me right! Two reviewers!

[Robert Sesek, 2011-08-04 22:15:44]

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
File ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h (left):

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h:23:
scoped_nsobject<NSGraphicsContext> context_;
So if we were to be paranoid, we'd keep a weak ptr here and DCHECK_EQ in the
dtor that +currentContext == the pointer. Paranoia is maybe a good thing (tm)?

[Avi (use Gerrit), 2011-08-04 22:21:03]

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
File ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h (left):

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h:23:
scoped_nsobject<NSGraphicsContext> context_;
You mean before the save context and after the restore? Not sure if it's
paranoia or not trusting the OS to do its job...

[Robert Sesek, 2011-08-04 22:22:13]

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
File ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h (left):

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h:23:
scoped_nsobject<NSGraphicsContext> context_;
On 2011/08/04 22:21:03, Avi wrote:
> You mean before the save context and after the restore? Not sure if it's
> paranoia or not trusting the OS to do its job...

No, it'd prevent changing the current context within the scope of the scoper.

[Avi (use Gerrit), 2011-08-04 22:24:22]

But that seems to be the point of the save/restore; see hover_close_button where
the scoper is used to restore the context. Doesn't matter if it changes as long
as it gets put back properly. (Not that I'm saying that the code in
hover_close_button is correct...)

[Mark Mentovai, 2011-08-04 23:20:17]

I want more.

http://codereview.chromium.org/7572031/diff/2001/chrome/browser/ui/cocoa/book...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_toolbar_view.mm (right):

http://codereview.chromium.org/7572031/diff/2001/chrome/browser/ui/cocoa/book...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_toolbar_view.mm:108: CGContextRef
cgContext = (CGContextRef)[context graphicsPort];
Does [NSGraphicContext currentContext] belong right here?

http://codereview.chromium.org/7572031/diff/2001/chrome/browser/ui/cocoa/tabs...
File chrome/browser/ui/cocoa/tabs/tab_view.mm (right):

http://codereview.chromium.org/7572031/diff/2001/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_view.mm:293: static_cast<CGContextRef>([context
graphicsPort]);
This is probably fine because the port should have stayed the same, but it might
be more correct to make another call to [NSGraphicsContext currentContext] here.

http://codereview.chromium.org/7572031/diff/2001/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_view.mm:311:
gfx::ScopedNSGraphicsContextSaveGState contextSave;
I’m not going to do a line-by-line comparison here, it looks like you’ve done
the right thing with the move-and-reindent job.

http://codereview.chromium.org/7572031/diff/2001/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_view.mm:322: static_cast<CGContextRef>([context
graphicsPort]);
Ditto.

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
File ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h (right):

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
See also
third_party/WebKit/Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm.
We do build this file. Weird.

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h:7: 
Add #pragma once.

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h:8: #include "ui/ui_api.h"
Sort headers: base < ui.

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h:11: @class
NSGraphicsContext;
I would do:

#if defined(__OBJC__)
@class NSGraphicsContext;
#else
class NSGraphicsContext;
#endif

because there’s nothing inherently Objective about this interface. I doubt
anyone would want to use it from non-Objective-C code, but hey, it’s possible.

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h:21:
DISALLOW_COPY_AND_ASSIGN(ScopedNSGraphicsContextSaveGState);
This needs to be in a private: section.

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h:26: #endif  //
UI_GFX_SCOPED_NS_GRAPHICS_CONTEXT_SAVE_GSTATE_MAC_H_
Additional potential interface abuse occurs in:

Operations on +[NSGraphicsContext currentContext] without surrounding
save/restore—these are “draw” routines and it may be OK if some caller is
responsible for doing the save/restore and it doesn’t matter if we dirty the
state:

chrome/browser/ui/cocoa/gradient_button_cell.mm -[GradientButtonCell
drawBorderAndFillForTheme:…]

chrome/browser/ui/cocoa/styled_text_field_cell.mm -[StyledTextFieldCell
drawWithFrame:inView:]

chrome/browser/ui/cocoa/background_tile_view.mm  -[BackgroundTileView drawRect:]

chrome/browser/ui/cocoa/browser_frame_view.mm +[BrowserFrameView
drawWindowThemeInDirtyRect:…]

chrome/browser/ui/cocoa/toolbar/toolbar_view.mm -[ToolbarView drawRect:]

+[NSGraphicsContext restoreGraphicsState] without matching +[NSGraphicsContext
saveGraphicsState]:—candidate for conversion to use this class:

ui/base/clipboard/clipboard_mac.mm
ui::Clipboard::ReadImage(ui::Clipboard::Buffer)

Use of +[NSGraphicsContext currentContext] and +[NSGraphicsContext
setCurrentContext:] instead of +[NSGraphicsContext saveGraphicsState] and
+[NSGraphicsContext restoreGraphicsState]—possible candidate for conversion to
this interface or to be rewritten to use
+saveGraphicsState/+restoreGraphicsState:

webkit/tools/test_shell/mac/webwidget_host.mm WebWidgetHost::Paint()

third_party/WebKit/Tools/DumpRenderTree/chromium/WebThemeEngineDRTMac.mm
WebThemeEngineDRTMac::paintNSScrollerScrollbarThumb(…)

Uses that appear safe but are candidates for conversion to use this class:

chrome/default_plugin/plugin_impl_mac.mm
PluginInstallerImpl::OnDrawRect(CGContextRef, CGRect)

Uses of -saveGraphicsState/-restoreGraphicsState instead of
+saveGraphicsState/+restoreGraphicsState:

third_party/GTM/AppKit/GTMFadeTruncatingTextFieldCell.m
-[GTMFadeTruncatingTextFieldCell drawTextGradientPart:…]

third_party/GTM/AppKit/GTMFadeTruncatingTextFieldCell.m
-[GTMFadeTruncatingTextFieldCell drawInteriorWithFrame:inView:]

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h:26: #endif  //
UI_GFX_SCOPED_NS_GRAPHICS_CONTEXT_SAVE_GSTATE_MAC_H_
Also, note http://code.google.com/p/chromium/issues/detail?id=90140#c31

I think the use in chrome/browser/ui/cocoa/hover_close_button.mm
+[HoverCloseButton imageForBounds:…] is correct, though. Maybe it’s just
evilness with the old broken scoper that we’re replacing anyway.

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
File ui/gfx/scoped_ns_graphics_context_save_gstate_mac.mm (right):

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.mm:7: #include
<AppKit/AppKit.h>
#import

[Mark Mentovai, 2011-08-04 23:26:49]

Fix CL description: Always → Allays.

[Mark Mentovai, 2011-08-04 23:30:35]

rsesek@chromium.org wrote:
> No, it'd prevent changing the current context within the scope of the
> scoper.

We want to allow this, though.

(That’s my current understanding. Keep in mind that my understanding
is still evolving. I think that’s true of all of us.)

[Avi (use Gerrit), 2011-08-05 00:18:50]

http://codereview.chromium.org/7572031/diff/2001/chrome/browser/ui/cocoa/book...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_toolbar_view.mm (right):

http://codereview.chromium.org/7572031/diff/2001/chrome/browser/ui/cocoa/book...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_toolbar_view.mm:108: CGContextRef
cgContext = (CGContextRef)[context graphicsPort];
On 2011/08/04 23:20:17, Mark Mentovai wrote:
> Does [NSGraphicContext currentContext] belong right here?

Done.

http://codereview.chromium.org/7572031/diff/2001/chrome/browser/ui/cocoa/tabs...
File chrome/browser/ui/cocoa/tabs/tab_view.mm (right):

http://codereview.chromium.org/7572031/diff/2001/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_view.mm:293: static_cast<CGContextRef>([context
graphicsPort]);
On 2011/08/04 23:20:17, Mark Mentovai wrote:
> This is probably fine because the port should have stayed the same, but it
might
> be more correct to make another call to [NSGraphicsContext currentContext]
here.

Done.

http://codereview.chromium.org/7572031/diff/2001/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_view.mm:322: static_cast<CGContextRef>([context
graphicsPort]);
On 2011/08/04 23:20:17, Mark Mentovai wrote:
> Ditto.

Mr Paranoid!

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
File ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h (right):

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h:7: 
On 2011/08/04 23:20:17, Mark Mentovai wrote:
> Add #pragma once.

Done.

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h:8: #include "ui/ui_api.h"
On 2011/08/04 23:20:17, Mark Mentovai wrote:
> Sort headers: base < ui.

Done.

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h:11: @class
NSGraphicsContext;
Removed; not needed.

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h:21:
DISALLOW_COPY_AND_ASSIGN(ScopedNSGraphicsContextSaveGState);
On 2011/08/04 23:20:17, Mark Mentovai wrote:
> This needs to be in a private: section.

Done.

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h:26: #endif  //
UI_GFX_SCOPED_NS_GRAPHICS_CONTEXT_SAVE_GSTATE_MAC_H_
On 2011/08/04 23:20:17, Mark Mentovai wrote:
> Operations on +[NSGraphicsContext currentContext] without surrounding
> save/restore—these are “draw” routines and it may be OK if some caller is
> responsible for doing the save/restore

For draw routines, AppKit is responsible for setup and teardown; we should be
fine.

> ui/base/clipboard/clipboard_mac.mm
> ui::Clipboard::ReadImage(ui::Clipboard::Buffer)

Done.

> webkit/tools/test_shell/mac/webwidget_host.mm WebWidgetHost::Paint()

Can I do that one later? Been way too long since I've touched that.

> third_party/WebKit/Tools/...

Upstream is out of scope for this CL.

> chrome/default_plugin/plugin_impl_mac.mm
> PluginInstallerImpl::OnDrawRect(CGContextRef, CGRect)

Done.

> third_party/GTM/AppKit/GTMFadeTruncatingTextFieldCell.m
> -[GTMFadeTruncatingTextFieldCell drawTextGradientPart:…]
> 
> third_party/GTM/AppKit/GTMFadeTruncatingTextFieldCell.m
> -[GTMFadeTruncatingTextFieldCell drawInteriorWithFrame:inView:]

Upstream is out of scope for this CL.

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h:26: #endif  //
UI_GFX_SCOPED_NS_GRAPHICS_CONTEXT_SAVE_GSTATE_MAC_H_
Now it's good.

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
File ui/gfx/scoped_ns_graphics_context_save_gstate_mac.mm (right):

http://codereview.chromium.org/7572031/diff/2001/ui/gfx/scoped_ns_graphics_co...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.mm:7: #include
<AppKit/AppKit.h>
On 2011/08/04 23:20:17, Mark Mentovai wrote:
> #import

Done.

[Avi (use Gerrit), 2011-08-05 00:23:22]

ptal

[Mark Mentovai, 2011-08-05 00:58:41]

LGTM

If you wanted to add a DCHECK, it would be a DCHECK that [NSGraphicsContext
currentContext] before [NSGraphicsConext saveGraphicsState] is the same as
[NSGraphicsContext currentContext] after [NSGraphicsContext
restoreGraphicsState]. This isn’t a bad idea.

Are you going to handle all of the followups? I’ve pointed out all of the uses
in files that it appears we do build somewhere, it’d be good to get all of them
fixed.

[Robert Sesek, 2011-08-05 01:02:33]

On 2011/08/05 00:58:41, Mark Mentovai wrote:
> LGTM
> 
> If you wanted to add a DCHECK, it would be a DCHECK that [NSGraphicsContext
> currentContext] before [NSGraphicsConext saveGraphicsState] is the same as
> [NSGraphicsContext currentContext] after [NSGraphicsContext
> restoreGraphicsState]. This isn’t a bad idea.

I'm in favor of this. While we *may* want to be able to change the context while
using this scoper, I think the subtlety involved means that we should disallow
it. If we don't, this entire class feels like it needs a large comment block and
belongs in something like ui::subtle. In places where you actually want to
change contexts, I think managing contexts yourself is fine.

[Avi (use Gerrit), 2011-08-05 01:09:34]

On 2011/08/05 01:02:33, rsesek wrote:
> I'm in favor of this.

I'm OK with this.

> While we *may* want to be able to change the context while
> using this scoper, I think the subtlety involved means that we should disallow
> it.

Why? It works fine, and the DCHECK ensures that it does if we're feeling
paranoid. Fiddling with the context is useful and often needed, and I'd rather
that people use this class then try to get it right themselves (given that the
obvious solutions are apparently the ones that don't work).

[Mark Mentovai, 2011-08-05 01:12:06]

rsesek wrote:
> I'm in favor of this. While we *may* want to be able to change the context
while
> using this scoper, I think the subtlety involved means that we should disallow
> it. If we don't, this entire class feels like it needs a large comment block
and
> belongs in something like ui::subtle. In places where you actually want to
> change contexts, I think managing contexts yourself is fine.

Changing the context while inside the scoper is fine. The scoper’s job is to
ensure it’s fine.

What’s not fine is having an unbalanced
[+-]saveGraphicsState/[+-]restoreGraphicsState. That’s what the DCHECK is
intended to protect against. I want to make sure that we pop what we pushed.

[Mark Mentovai, 2011-08-05 01:15:44]

As an example, be mindful of these two legitimate uses.

http://codereview.chromium.org/7572031/diff/7001/chrome/default_plugin/plugin...
File chrome/default_plugin/plugin_impl_mac.mm (right):

http://codereview.chromium.org/7572031/diff/7001/chrome/default_plugin/plugin...
chrome/default_plugin/plugin_impl_mac.mm:146: [NSGraphicsContext
setCurrentContext:ns_context];
☞

http://codereview.chromium.org/7572031/diff/7001/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_mac.mm (right):

http://codereview.chromium.org/7572031/diff/7001/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_mac.mm:262: [NSGraphicsContext
setCurrentContext:cocoa_gc];
☞

[Robert Sesek, 2011-08-05 01:16:59]

On 2011/08/05 01:12:06, Mark Mentovai wrote:
> rsesek wrote:
> > I'm in favor of this. While we *may* want to be able to change the context
> while
> > using this scoper, I think the subtlety involved means that we should
disallow
> > it. If we don't, this entire class feels like it needs a large comment block
> and
> > belongs in something like ui::subtle. In places where you actually want to
> > change contexts, I think managing contexts yourself is fine.
> 
> Changing the context while inside the scoper is fine. The scoper’s job is to
> ensure it’s fine.
> 
> What’s not fine is having an unbalanced
> [+-]saveGraphicsState/[+-]restoreGraphicsState. That’s what the DCHECK is
> intended to protect against. I want to make sure that we pop what we pushed.

You're right. I'm confusing contexts and states. I still think sanity checks are
a good thing, especially when they're cheap.

[Avi (use Gerrit), 2011-08-05 01:24:42]

ok

[Robert Sesek, 2011-08-05 01:26:37]

LGTM! Thanks.

[Mark Mentovai, 2011-08-05 01:34:13]

LGTM

You could have used initializer format to set context_ although it doesn't
matter.

With respect to the followups I asked about, were you planning on handling?

Bonus points if you can check in tonight so we can start baking in tomorrow's
canary.

[Avi (use Gerrit), 2011-08-05 22:09:23]

ptal

[Mark Mentovai, 2011-08-05 22:11:32]

LGTM

http://codereview.chromium.org/7572031/diff/12001/ui/gfx/scoped_ns_graphics_c...
File ui/gfx/scoped_ns_graphics_context_save_gstate_mac.mm (right):

http://codereview.chromium.org/7572031/diff/12001/ui/gfx/scoped_ns_graphics_c...
ui/gfx/scoped_ns_graphics_context_save_gstate_mac.mm:26: // that stale context
(which is likely been dealloced) to remain current
(which is likely to be dealloced)