Add and use gfx::ScopedNSGraphicsContextState (ui/gfx/scoped_ns_graphics_context_state_mac.h).

Add and use gfx::ScopedNSGraphicsContextState (ui/gfx/scoped_ns_graphics_context_state_mac.h).

BUG=none
TEST=covered by tests


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

[Robert Sesek, 2011-04-26 17:46:03]

I went through the CL a few times to make sure it looks correct, but please go
through with a fine-toothed comb.

[Avi (use Gerrit), 2011-04-26 17:50:29]

http://codereview.chromium.org/6904026/diff/1/ui/gfx/scoped_ns_graphics_conte...
File ui/gfx/scoped_ns_graphics_context_state_mac.h (right):

http://codereview.chromium.org/6904026/diff/1/ui/gfx/scoped_ns_graphics_conte...
ui/gfx/scoped_ns_graphics_context_state_mac.h:34: }  // namespace gfx
Can't you just do [NSGraphicsContext saveGraphicsState] in the constructor,
[NSGraphicsContext restoreGraphicsState] in the destructor, and drop the member
variable entirely?

[Robert Sesek, 2011-04-26 17:51:25]

http://codereview.chromium.org/6904026/diff/1/ui/gfx/scoped_ns_graphics_conte...
File ui/gfx/scoped_ns_graphics_context_state_mac.h (right):

http://codereview.chromium.org/6904026/diff/1/ui/gfx/scoped_ns_graphics_conte...
ui/gfx/scoped_ns_graphics_context_state_mac.h:34: }  // namespace gfx
On 2011/04/26 17:50:29, Avi wrote:
> Can't you just do [NSGraphicsContext saveGraphicsState] in the constructor,
> [NSGraphicsContext restoreGraphicsState] in the destructor, and drop the
member
> variable entirely?

Sure. Would you ever want to operate on a separate context, though?

[Nico, 2011-04-26 17:56:28]

LGTM with nits

http://codereview.chromium.org/6904026/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6904026/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:420:
gfx::ScopedNSGraphicsContextState scopedGState;
hm, this line in itself isn't very obvious. Maybe

    gfx::ScopedSaveNSGraphicsContextState scopedGState;

(i.e. add "Save" to type name)

http://codereview.chromium.org/6904026/diff/1/ui/gfx/scoped_ns_graphics_conte...
File ui/gfx/scoped_ns_graphics_context_state_mac.h (right):

http://codereview.chromium.org/6904026/diff/1/ui/gfx/scoped_ns_graphics_conte...
ui/gfx/scoped_ns_graphics_context_state_mac.h:29: NSGraphicsContext* context_;
If you keep this in a variable, this should probably be a scoped_nsobject with a
retain in the constructor?

Out-of-line things?

[Robert Sesek, 2011-04-26 18:50:52]

http://codereview.chromium.org/6904026/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6904026/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:420:
gfx::ScopedNSGraphicsContextState scopedGState;
On 2011/04/26 17:56:28, Nico wrote:
> hm, this line in itself isn't very obvious. Maybe
> 
>     gfx::ScopedSaveNSGraphicsContextState scopedGState;
> 
> (i.e. add "Save" to type name)

This matches the CGContext version...

http://codereview.chromium.org/6904026/diff/1/ui/gfx/scoped_ns_graphics_conte...
File ui/gfx/scoped_ns_graphics_context_state_mac.h (right):

http://codereview.chromium.org/6904026/diff/1/ui/gfx/scoped_ns_graphics_conte...
ui/gfx/scoped_ns_graphics_context_state_mac.h:29: NSGraphicsContext* context_;
On 2011/04/26 17:56:28, Nico wrote:
> If you keep this in a variable, this should probably be a scoped_nsobject with
a
> retain in the constructor?

Done.

> Out-of-line things?

Done.

[Nico, 2011-04-26 20:02:18]

SLG

On Tue, Apr 26, 2011 at 11:50 AM,  <rsesek@chromium.org> wrote:
>
>
http://codereview.chromium.org/6904026/diff/1/chrome/browser/renderer_host/re...
> File chrome/browser/renderer_host/render_widget_host_view_mac.mm
> (right):
>
>
http://codereview.chromium.org/6904026/diff/1/chrome/browser/renderer_host/re...
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:420:
> gfx::ScopedNSGraphicsContextState scopedGState;
> On 2011/04/26 17:56:28, Nico wrote:
>>
>> hm, this line in itself isn't very obvious. Maybe
>
>>     gfx::ScopedSaveNSGraphicsContextState scopedGState;
>
>> (i.e. add "Save" to type name)
>
> This matches the CGContext version...

:-D

That's what I get for not reviewing my own code I guess.

>
>
http://codereview.chromium.org/6904026/diff/1/ui/gfx/scoped_ns_graphics_conte...
> File ui/gfx/scoped_ns_graphics_context_state_mac.h (right):
>
>
http://codereview.chromium.org/6904026/diff/1/ui/gfx/scoped_ns_graphics_conte...
> ui/gfx/scoped_ns_graphics_context_state_mac.h:29: NSGraphicsContext*
> context_;
> On 2011/04/26 17:56:28, Nico wrote:
>>
>> If you keep this in a variable, this should probably be a
>
> scoped_nsobject with a
>>
>> retain in the constructor?
>
> Done.
>
>> Out-of-line things?
>
> Done.
>
> http://codereview.chromium.org/6904026/
>

[Robert Sesek, 2011-04-26 20:23:13]

On 2011/04/26 20:02:18, Nico wrote:
> SLG
> 
> On Tue, Apr 26, 2011 at 11:50 AM,  <mailto:rsesek@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/6904026/diff/1/chrome/browser/renderer_host/re...
> > File chrome/browser/renderer_host/render_widget_host_view_mac.mm
> > (right):
> >
> >
>
http://codereview.chromium.org/6904026/diff/1/chrome/browser/renderer_host/re...
> > chrome/browser/renderer_host/render_widget_host_view_mac.mm:420:
> > gfx::ScopedNSGraphicsContextState scopedGState;
> > On 2011/04/26 17:56:28, Nico wrote:
> >>
> >> hm, this line in itself isn't very obvious. Maybe
> >
> >> &nbsp; &nbsp; gfx::ScopedSaveNSGraphicsContextState scopedGState;
> >
> >> (i.e. add "Save" to type name)
> >
> > This matches the CGContext version...
> 
> :-D
> 
> That's what I get for not reviewing my own code I guess.

Actually, I lied. The class name is ScopedCGContextSaveGState but it's in
scoped_cg_context_state_mac.h.

I've renamed the NS version appropriately and will come back in a future CL to
fix the filename of the CG version.

[commit-bot: I haz the power, 2011-04-26 23:18:06]

Try job failure for 6904026-6002 on mac:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2011-04-27 06:27:35]

Change committed as 83125