compositor_unittests target is unimplmented on Mac

compositor_unittests target is unimplmented on Mac

Adds necessary pieces to get the WebKit compositor working with
compositor_unittests target on Mac.  Required gyp build flags are:

    'use_aura': 1,
    'use_webkit_compositor': 1,
    'use_skia': 1,

BUG=104390, 104555
TEST=compositor_unittests --gtest_filter=LayerWithRealCompositorTest.* passes and shows correct visual results.

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

[dhollowa, 2011-11-16 18:55:25]

Hi Folks, this CL puts the basic NSView-backed WebKit compositor in place on
Mac.  The various #ifdefs and .gyp manipulations are in service of getting the
use_aura=1 configuration compiling and linking on the Mac.

Thoughts on review emphasis:

kbr -> ui/gfx/gl/...
piman -> ui/gfx/compositor/...
ben -> use_aura=1 related gyp and #ifdef changes
thakis -> objc / mac stuff

Thanks in advance.

[Nico, 2011-11-16 19:11:13]

mac bits basically lgtm

http://codereview.chromium.org/8486020/diff/1/printing/printing.gyp
File printing/printing.gyp (right):

http://codereview.chromium.org/8486020/diff/1/printing/printing.gyp#newcode114
printing/printing.gyp:114: ['OS=="mac" and use_aura==1',{
Will printing ever be supported in this configuration? If so, file bug and add a
link to it in a comment. If not, add a comment saying so.

http://codereview.chromium.org/8486020/diff/1/ui/gfx/compositor/test_composit...
File ui/gfx/compositor/test_compositor_host_mac.mm (right):

http://codereview.chromium.org/8486020/diff/1/ui/gfx/compositor/test_composit...
ui/gfx/compositor/test_compositor_host_mac.mm:41: NSOpenGLContext* context =
[NSOpenGLContext currentContext];
Won't that just return nil? Where are you setting up a GL context?

http://codereview.chromium.org/8486020/diff/1/ui/gfx/compositor/test_composit...
ui/gfx/compositor/test_compositor_host_mac.mm:52: class FoundationHost {
Can you use testing/platform_test.h instead?

http://codereview.chromium.org/8486020/diff/1/ui/gfx/compositor/test_composit...
ui/gfx/compositor/test_compositor_host_mac.mm:72: [NSApplication
sharedApplication];
base relies on NSApp implementing some custom protocol – should you be using
CrApplication here?

Also, most of our test suites create NSApp during test suite main(), not in the
individual tests – if gfx_tests doesn't do that, maybe add it there instead?

http://codereview.chromium.org/8486020/diff/1/ui/gfx/gl/gl_surface_nsview.mm
File ui/gfx/gl/gl_surface_nsview.mm (right):

http://codereview.chromium.org/8486020/diff/1/ui/gfx/gl/gl_surface_nsview.mm#...
ui/gfx/gl/gl_surface_nsview.mm:32: CGLContextObj cgl_context =
CGLGetCurrentContext();
Is there some way to DCHECK that the right context is current here?

http://codereview.chromium.org/8486020/diff/1/ui/gfx/gl/gl_surface_nsview.mm#...
ui/gfx/gl/gl_surface_nsview.mm:39: return gfx::Size(bounds.size.width,
bounds.size.height);
Doesn't gfx::Size() have a constructor that takes an NSSize (or at least a
CGSize)?

http://codereview.chromium.org/8486020/diff/1/ui/gfx/native_widget_types.h
File ui/gfx/native_widget_types.h (right):

http://codereview.chromium.org/8486020/diff/1/ui/gfx/native_widget_types.h#ne...
ui/gfx/native_widget_types.h:230: #elif defined(USE_WEBKIT_COMPOSITOR) &&
defined(OS_MACOSX)
I don't know what the longer-term plan for this is, but these defines sound
generic enough that people might think USE_WEBKIT_COMPOSITOR could work in
desktop chrome. Can you add a comment that this only makes sense with the aura
prototype, and that getting this into "normal" chrome is fairly involved?

[dhollowa, 2011-11-16 19:59:18]

http://codereview.chromium.org/8486020/diff/1/printing/printing.gyp
File printing/printing.gyp (right):

http://codereview.chromium.org/8486020/diff/1/printing/printing.gyp#newcode114
printing/printing.gyp:114: ['OS=="mac" and use_aura==1',{
On 2011/11/16 19:11:13, Nico wrote:
> Will printing ever be supported in this configuration? If so, file bug and add
a
> link to it in a comment. If not, add a comment saying so.

I can't say it never will, but it is not in the plan of record at the moment.

Done.

http://codereview.chromium.org/8486020/diff/1/ui/gfx/compositor/test_composit...
File ui/gfx/compositor/test_compositor_host_mac.mm (right):

http://codereview.chromium.org/8486020/diff/1/ui/gfx/compositor/test_composit...
ui/gfx/compositor/test_compositor_host_mac.mm:41: NSOpenGLContext* context =
[NSOpenGLContext currentContext];
The current context gets set by the compositing machinery.  The test "host"
coordinates the drawing code, which draws using the compositor, then when the
compositor has done its thing it alerts the "host" that drawing should happen. 
This code gets triggered and the gl context is properly set.

On 2011/11/16 19:11:13, Nico wrote:
> Won't that just return nil? Where are you setting up a GL context?

http://codereview.chromium.org/8486020/diff/1/ui/gfx/compositor/test_composit...
ui/gfx/compositor/test_compositor_host_mac.mm:52: class FoundationHost {
Not in this case, no.  The tests need to run cross-platform, so a parallel
"host" hierarchy was needed.

On 2011/11/16 19:11:13, Nico wrote:
> Can you use testing/platform_test.h instead?

http://codereview.chromium.org/8486020/diff/1/ui/gfx/compositor/test_composit...
ui/gfx/compositor/test_compositor_host_mac.mm:72: [NSApplication
sharedApplication];
On 2011/11/16 19:11:13, Nico wrote:
> base relies on NSApp implementing some custom protocol – should you be using
> CrApplication here?
> 

No, CrApplication's main benefit is in diverting events.  There is no need for
that functionality in these tests which are largely visual, and non-interactive.


> Also, most of our test suites create NSApp during test suite main(), not in
the
> individual tests – if gfx_tests doesn't do that, maybe add it there instead?

I followed the principle of narrowest scope with this.  Since these are
currently the only tests that need AppKit stuff I've kept them contained here. 
I prefer this.  I'll keep an eye on this though and promote up to the gfx_tests
level if we need them elsewhere.  That is, if you agree.

http://codereview.chromium.org/8486020/diff/1/ui/gfx/gl/gl_surface_nsview.mm
File ui/gfx/gl/gl_surface_nsview.mm (right):

http://codereview.chromium.org/8486020/diff/1/ui/gfx/gl/gl_surface_nsview.mm#...
ui/gfx/gl/gl_surface_nsview.mm:32: CGLContextObj cgl_context =
CGLGetCurrentContext();
Not really, no.  The view doesn't expose accessors for its context, it is
implied by call order.  That is the view's drawing traversal sets the context up
but the only way to get at it is via the notion of "current" context.  Lots of
pushing/popping, but no direct access.

On 2011/11/16 19:11:13, Nico wrote:
> Is there some way to DCHECK that the right context is current here?

http://codereview.chromium.org/8486020/diff/1/ui/gfx/gl/gl_surface_nsview.mm#...
ui/gfx/gl/gl_surface_nsview.mm:39: return gfx::Size(bounds.size.width,
bounds.size.height);
On 2011/11/16 19:11:13, Nico wrote:
> Doesn't gfx::Size() have a constructor that takes an NSSize (or at least a
> CGSize)?

Done.

http://codereview.chromium.org/8486020/diff/1/ui/gfx/native_widget_types.h
File ui/gfx/native_widget_types.h (right):

http://codereview.chromium.org/8486020/diff/1/ui/gfx/native_widget_types.h#ne...
ui/gfx/native_widget_types.h:230: #elif defined(USE_WEBKIT_COMPOSITOR) &&
defined(OS_MACOSX)
Done.  Yes.

I also wonder about the possible merging of |AcceleratedWidget| and
|PluginWindowHandle|.  They're almost identical, and the gl/gl_surface*
interfaces consume |PluginWindowHandle| but |AcceleratedWidget| seems more
appropriate somehow.


On 2011/11/16 19:11:13, Nico wrote:
> I don't know what the longer-term plan for this is, but these defines sound
> generic enough that people might think USE_WEBKIT_COMPOSITOR could work in
> desktop chrome. Can you add a comment that this only makes sense with the aura
> prototype, and that getting this into "normal" chrome is fairly involved?

[Nico, 2011-11-16 20:04:22]

http://codereview.chromium.org/8486020/diff/1/ui/gfx/compositor/test_composit...
File ui/gfx/compositor/test_compositor_host_mac.mm (right):

http://codereview.chromium.org/8486020/diff/1/ui/gfx/compositor/test_composit...
ui/gfx/compositor/test_compositor_host_mac.mm:72: [NSApplication
sharedApplication];
On 2011/11/16 19:59:18, dhollowa wrote:
> On 2011/11/16 19:11:13, Nico wrote:
> > base relies on NSApp implementing some custom protocol – should you be using
> > CrApplication here?
> > 
> 
> No, CrApplication's main benefit is in diverting events.  There is no need for
> that functionality in these tests which are largely visual, and
non-interactive.

message_loop_mac still DCHECKs that NSApp
conformsToProtocol:@protocol(CrAppProtocol). Maybe the tests don't use message
loops?

[dhollowa, 2011-11-16 20:25:24]

http://codereview.chromium.org/8486020/diff/1/ui/gfx/compositor/test_composit...
File ui/gfx/compositor/test_compositor_host_mac.mm (right):

http://codereview.chromium.org/8486020/diff/1/ui/gfx/compositor/test_composit...
ui/gfx/compositor/test_compositor_host_mac.mm:72: [NSApplication
sharedApplication];
On 2011/11/16 20:04:22, Nico wrote:
> On 2011/11/16 19:59:18, dhollowa wrote:
> > On 2011/11/16 19:11:13, Nico wrote:
> > > base relies on NSApp implementing some custom protocol – should you be
using
> > > CrApplication here?
> > > 
> > 
> > No, CrApplication's main benefit is in diverting events.  There is no need
for
> > that functionality in these tests which are largely visual, and
> non-interactive.
> 
> message_loop_mac still DCHECKs that NSApp
> conformsToProtocol:@protocol(CrAppProtocol). Maybe the tests don't use message
> loops?

Correct.  No message loops here, though that may change.  If/when it does I'll
bring in CrApplication.  Thanks for pointing that out.

[piman, 2011-11-16 20:55:59]

http://codereview.chromium.org/8486020/diff/5002/ui/gfx/compositor/layer_anim...
File ui/gfx/compositor/layer_animation_element.cc (right):

http://codereview.chromium.org/8486020/diff/5002/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animation_element.cc:49: #if !defined(OS_MACOSX)
Why? I don't see what's mac-adverse here.

http://codereview.chromium.org/8486020/diff/5002/ui/gfx/gl/gl_context_mac.mm
File ui/gfx/gl/gl_context_mac.mm (right):

http://codereview.chromium.org/8486020/diff/5002/ui/gfx/gl/gl_context_mac.mm#...
ui/gfx/gl/gl_context_mac.mm:50: #if defined(USE_WEBKIT_COMPOSITOR)
I don't think USE_WEBKIT_COMPOSITOR ought to have any impact here.
USE_WEBKIT_COMPOSITOR is whether ui::Compositor is implemented using WebKit or
the "old" compositor. But It shouldn't have any impact on the context
implementation, which is lower in the stack.
In particular this code is used in the GPU process as well, where
USE_WEBKIT_COMPOSITOR has absolutely no meaning.

What would happen if this code was enabled in all cases?

http://codereview.chromium.org/8486020/diff/5002/ui/gfx/native_widget_types.h
File ui/gfx/native_widget_types.h (right):

http://codereview.chromium.org/8486020/diff/5002/ui/gfx/native_widget_types.h...
ui/gfx/native_widget_types.h:230: #elif defined(USE_WEBKIT_COMPOSITOR) &&
defined(OS_MACOSX)
Same here, I don't think USE_WEBKIT_COMPOSITOR is what you want.
If anything, VIEWS_COMPOSITOR. But even then, would things go wrong if it was
always defined this way? I'd rather avoid UI-level concepts leaking into the
infrastructure.

[dhollowa, 2011-11-16 23:28:58]

http://codereview.chromium.org/8486020/diff/5002/ui/gfx/compositor/layer_anim...
File ui/gfx/compositor/layer_animation_element.cc (right):

http://codereview.chromium.org/8486020/diff/5002/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animation_element.cc:49: #if !defined(OS_MACOSX)
ui/base/animation/tween.h:41 is #ifdef'd out on Mac.  My plan is to investigate
further and get this working in a subsequent CL.

On 2011/11/16 20:55:59, piman wrote:
> Why? I don't see what's mac-adverse here.

http://codereview.chromium.org/8486020/diff/5002/ui/gfx/gl/gl_context_mac.mm
File ui/gfx/gl/gl_context_mac.mm (right):

http://codereview.chromium.org/8486020/diff/5002/ui/gfx/gl/gl_context_mac.mm#...
ui/gfx/gl/gl_context_mac.mm:50: #if defined(USE_WEBKIT_COMPOSITOR)
Due to the different definition of the PluginWindowHandle type, compilation of
GLSurfaceNSView fails in the use_aura=0 case (see my comment in
native_widget_types.h).  Since GLSurfaceNSView and GLContextNSView only work
together, I feel it is best to only compile and link them in to a given
configuration as a pair.

Note, I've change this and elsewhere to "USE_AURA" instead of
"USE_WEBKIT_COMPOSITOR" to better reflect that this is a platform difference,
not just a configuration difference.

On 2011/11/16 20:55:59, piman wrote:
> I don't think USE_WEBKIT_COMPOSITOR ought to have any impact here.
> USE_WEBKIT_COMPOSITOR is whether ui::Compositor is implemented using WebKit or
> the "old" compositor. But It shouldn't have any impact on the context
> implementation, which is lower in the stack.
> In particular this code is used in the GPU process as well, where
> USE_WEBKIT_COMPOSITOR has absolutely no meaning.
> 
> What would happen if this code was enabled in all cases?

http://codereview.chromium.org/8486020/diff/5002/ui/gfx/native_widget_types.h
File ui/gfx/native_widget_types.h (right):

http://codereview.chromium.org/8486020/diff/5002/ui/gfx/native_widget_types.h...
ui/gfx/native_widget_types.h:230: #elif defined(USE_WEBKIT_COMPOSITOR) &&
defined(OS_MACOSX)
I think the correct designation is "Aura-Mac" i.e. USE_AURA && OS_MACOSX.

Mac-Aura is like a different "platform" than regular non-Aura Mac.  The regular
non-Aura Mac treats PluginWindowHandle as an id having nothing to do with a
platform window or view.  But with Mac-Aura the UI is drawn into a given NSView.

I would propose we keep the #ifdef'd separation but use USE_AURA && OS_MACOSX
(and not USE_WEBKIT_COMPOSITOR).

On 2011/11/16 20:55:59, piman wrote:
> Same here, I don't think USE_WEBKIT_COMPOSITOR is what you want.
> If anything, VIEWS_COMPOSITOR. But even then, would things go wrong if it was
> always defined this way? I'd rather avoid UI-level concepts leaking into the
> infrastructure.

[piman, 2011-11-16 23:42:22]

I could probably live with USE_AURA for now. But I'd prefer it to be cleaned up
in the long term, AURA is still a UI concept that shouldn't have an impact in
the GPU process.

[dhollowa, 2011-11-16 23:59:51]

On 2011/11/16 23:42:22, piman wrote:
> I could probably live with USE_AURA for now. But I'd prefer it to be cleaned
up
> in the long term, AURA is still a UI concept that shouldn't have an impact in
> the GPU process.

I will follow-up on this.  Tracked with crbug.com/104551.

[dhollowa, 2011-11-17 00:04:01]

On 2011/11/16 23:59:51, dhollowa wrote:
> On 2011/11/16 23:42:22, piman wrote:
> > I could probably live with USE_AURA for now. But I'd prefer it to be cleaned
> up
> > in the long term, AURA is still a UI concept that shouldn't have an impact
in
> > the GPU process.
> 
> I will follow-up on this.  Tracked with crbug.com/104551.

And for tracking the tweening work: crbug.com/104555.

[piman, 2011-11-17 00:09:47]

On 2011/11/16 23:59:51, dhollowa wrote:
> On 2011/11/16 23:42:22, piman wrote:
> > I could probably live with USE_AURA for now. But I'd prefer it to be cleaned
> up
> > in the long term, AURA is still a UI concept that shouldn't have an impact
in
> > the GPU process.
> 
> I will follow-up on this.  Tracked with crbug.com/104551.

The current patch still uses USE_WEBKIT_COMPOSITOR.

[piman, 2011-11-17 00:10:15]

On 2011/11/17 00:04:01, dhollowa wrote:
> On 2011/11/16 23:59:51, dhollowa wrote:
> > On 2011/11/16 23:42:22, piman wrote:
> > > I could probably live with USE_AURA for now. But I'd prefer it to be
cleaned
> > up
> > > in the long term, AURA is still a UI concept that shouldn't have an impact
> in
> > > the GPU process.
> > 
> > I will follow-up on this.  Tracked with crbug.com/104551.
> 
> And for tracking the tweening work: crbug.com/104555.

See my note there. I think you can just remove the #ifdefs.

[dhollowa, 2011-11-17 00:10:52]

On 2011/11/17 00:09:47, piman wrote:
> On 2011/11/16 23:59:51, dhollowa wrote:
> > On 2011/11/16 23:42:22, piman wrote:
> > > I could probably live with USE_AURA for now. But I'd prefer it to be
cleaned
> > up
> > > in the long term, AURA is still a UI concept that shouldn't have an impact
> in
> > > the GPU process.
> > 
> > I will follow-up on this.  Tracked with crbug.com/104551.
> 
> The current patch still uses USE_WEBKIT_COMPOSITOR.

Done.

[Ken Russell (switch to Gerrit), 2011-11-17 00:14:40]

The ui/gfx/gl changes mostly look good. A few relatively minor issues and a
couple of questions.

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/gl/gl_context_nsview.h
File ui/gfx/gl/gl_context_nsview.h (right):

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/gl/gl_context_nsview....
ui/gfx/gl/gl_context_nsview.h:21: #endif
Does this header even work if it's included by a non-Objective C file?
scoped_nsobject.h assumes it's included from Objective-C code. I note that all
files which include this one are now .mm.

I would just import NSOpenGL.h here. Either way, the forward declaration of
NSOpenGLPixelFormat definitely isn't needed.

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/gl/gl_context_nsview.mm
File ui/gfx/gl/gl_context_nsview.mm (right):

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/gl/gl_context_nsview....
ui/gfx/gl/gl_context_nsview.mm:55: [context_ makeCurrentContext];
Where do you set the context's view to that in the GLSurface?

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/gl/gl_surface_nsview.h
File ui/gfx/gl/gl_surface_nsview.h (right):

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/gl/gl_surface_nsview....
ui/gfx/gl/gl_surface_nsview.h:16: #endif
It would be nicer to avoid this forward declaration. Suggestions: use
PluginWindowHandle as the type for view_, or use void* and cast to NSView* in
the implementation.

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/gl/gl_surface_nsview.mm
File ui/gfx/gl/gl_surface_nsview.mm (right):

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/gl/gl_surface_nsview....
ui/gfx/gl/gl_surface_nsview.mm:33: CGLFlushDrawable(cgl_context);
I don't think it's valid to assume that flushing the CGL context will take care
of all of the necessary bookkeeping at the AppKit level. I'd suggest either
keeping a weak pointer to the current GLContextNSView in the surface, calling a
new FlushBuffer method on that instance, and implementing it properly there; or
minimally calling [[NSOpenGLContext currentContext] flushBuffer].

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/native_widget_types.h
File ui/gfx/native_widget_types.h (right):

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/native_widget_types.h...
ui/gfx/native_widget_types.h:230: #elif defined(USE_WEBKIT_COMPOSITOR) &&
defined(OS_MACOSX)
I think that this should be defined as defined(USE_AURA) && defined(OS_MACOSX)
because USE_WEBKIT_COMPOSITOR sounds like it might eventually be supported on
desktop Mac. Looking back at earlier comments, piman had the same feedback. Here
and below.

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/path.h
File ui/gfx/path.h (right):

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/path.h#newcode31
ui/gfx/path.h:31: #if defined(USE_AURA) || defined(OS_WIN) || defined(USE_X11)
Should this be instead (defined(USE_AURA) && defined(OS_MACOSX))?

[piman, 2011-11-17 00:31:32]

LGTM for my part

[dhollowa, 2011-11-17 00:35:27]

On 2011/11/17 00:10:15, piman wrote:
> On 2011/11/17 00:04:01, dhollowa wrote:
> > On 2011/11/16 23:59:51, dhollowa wrote:
> > > On 2011/11/16 23:42:22, piman wrote:
> > > > I could probably live with USE_AURA for now. But I'd prefer it to be
> cleaned
> > > up
> > > > in the long term, AURA is still a UI concept that shouldn't have an
impact
> > in
> > > > the GPU process.
> > > 
> > > I will follow-up on this.  Tracked with crbug.com/104551.
> > 
> > And for tracking the tweening work: crbug.com/104555.
> 
> See my note there. I think you can just remove the #ifdefs.

Ah, yes.  I thought you'd not seen the tween.h #ifdefs.  But I've removed them
and they work nicely!  Thanks.

[dhollowa, 2011-11-17 01:41:51]

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/gl/gl_context_nsview.h
File ui/gfx/gl/gl_context_nsview.h (right):

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/gl/gl_context_nsview....
ui/gfx/gl/gl_context_nsview.h:21: #endif
On 2011/11/17 00:14:40, kbr wrote:
> Does this header even work if it's included by a non-Objective C file?
> scoped_nsobject.h assumes it's included from Objective-C code. I note that all
> files which include this one are now .mm.
> 
> I would just import NSOpenGL.h here. Either way, the forward declaration of
> NSOpenGLPixelFormat definitely isn't needed.

Done.  That code was added prior to the .cc -> .mm conversion, but as you point
out is no longer needed.

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/gl/gl_context_nsview.mm
File ui/gfx/gl/gl_context_nsview.mm (right):

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/gl/gl_context_nsview....
ui/gfx/gl/gl_context_nsview.mm:55: [context_ makeCurrentContext];
On 2011/11/17 00:14:40, kbr wrote:
> Where do you set the context's view to that in the GLSurface?

test_compositor_host_mac.mm:41 in the host window's |-drawRect:|.

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/gl/gl_surface_nsview.h
File ui/gfx/gl/gl_surface_nsview.h (right):

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/gl/gl_surface_nsview....
ui/gfx/gl/gl_surface_nsview.h:16: #endif
On 2011/11/17 00:14:40, kbr wrote:
> It would be nicer to avoid this forward declaration. Suggestions: use
> PluginWindowHandle as the type for view_, or use void* and cast to NSView* in
> the implementation.

Done.

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/gl/gl_surface_nsview.mm
File ui/gfx/gl/gl_surface_nsview.mm (right):

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/gl/gl_surface_nsview....
ui/gfx/gl/gl_surface_nsview.mm:33: CGLFlushDrawable(cgl_context);
On 2011/11/17 00:14:40, kbr wrote:
> I don't think it's valid to assume that flushing the CGL context will take
care
> of all of the necessary bookkeeping at the AppKit level. I'd suggest either
> keeping a weak pointer to the current GLContextNSView in the surface, calling
a
> new FlushBuffer method on that instance, and implementing it properly there;
or
> minimally calling [[NSOpenGLContext currentContext] flushBuffer].

Done.  I went with the weak pointer approach.  It involved a down-cast but seems
more resistant to bugs where the scoped context is not set properly.

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/native_widget_types.h
File ui/gfx/native_widget_types.h (right):

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/native_widget_types.h...
ui/gfx/native_widget_types.h:230: #elif defined(USE_WEBKIT_COMPOSITOR) &&
defined(OS_MACOSX)
On 2011/11/17 00:14:40, kbr wrote:
> I think that this should be defined as defined(USE_AURA) && defined(OS_MACOSX)
> because USE_WEBKIT_COMPOSITOR sounds like it might eventually be supported on
> desktop Mac. Looking back at earlier comments, piman had the same feedback.
Here
> and below.

Done.  Yes, I'd addressed this in Patch Set 4.  It got missed in PS3.

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/path.h
File ui/gfx/path.h (right):

http://codereview.chromium.org/8486020/diff/8001/ui/gfx/path.h#newcode31
ui/gfx/path.h:31: #if defined(USE_AURA) || defined(OS_WIN) || defined(USE_X11)
On 2011/11/17 00:14:40, kbr wrote:
> Should this be instead (defined(USE_AURA) && defined(OS_MACOSX))?

No, this is correct as it stands.  The definitions should be available for any
Aura build.  The implementation defaults to path_aura.cc, but the platform may
override this in ui.gyp.

[Ben Goodger (Google), 2011-11-17 20:55:29]

General LGTM for ui/

[Ken Russell (switch to Gerrit), 2011-11-17 22:28:18]

LGTM with the following change made.

http://codereview.chromium.org/8486020/diff/15025/ui/gfx/compositor/test_comp...
File ui/gfx/compositor/test_compositor_host_mac.mm (right):

http://codereview.chromium.org/8486020/diff/15025/ui/gfx/compositor/test_comp...
ui/gfx/compositor/test_compositor_host_mac.mm:42: [context setView:self];
The call to setView belongs in GLContextNSView::MakeCurrent, before making the
OpenGL context current. The surface should be fetched from the passed GLSurface
(downcast to a GLSurfaceNSView). This is the contract of MakeCurrent and will
make this work the same as on other platforms. As a bonus, you won't have to
touch the NSOpenGLContext here.

http://codereview.chromium.org/8486020/diff/15025/ui/gfx/gl/gl_surface_nsview.h
File ui/gfx/gl/gl_surface_nsview.h (right):

http://codereview.chromium.org/8486020/diff/15025/ui/gfx/gl/gl_surface_nsview...
ui/gfx/gl/gl_surface_nsview.h:36: gfx::PluginWindowHandle view_;
nit: the gfx scoping is unnecessary here.

[dhollowa, 2011-11-18 00:50:37]

http://codereview.chromium.org/8486020/diff/15025/ui/gfx/compositor/test_comp...
File ui/gfx/compositor/test_compositor_host_mac.mm (right):

http://codereview.chromium.org/8486020/diff/15025/ui/gfx/compositor/test_comp...
ui/gfx/compositor/test_compositor_host_mac.mm:42: [context setView:self];
On 2011/11/17 22:28:18, kbr wrote:
> The call to setView belongs in GLContextNSView::MakeCurrent, before making the
> OpenGL context current. The surface should be fetched from the passed
GLSurface
> (downcast to a GLSurfaceNSView). This is the contract of MakeCurrent and will
> make this work the same as on other platforms. As a bonus, you won't have to
> touch the NSOpenGLContext here.

Done.  Thanks, ya, that makes sense.  The only complication is that
GLContext::MakeCurrent can be called prior to the view being parented, and thus
not a valid "drawable".  Once the view is parented (has a parent window that is)
it becomes a valid drawable, so I test for this.

http://codereview.chromium.org/8486020/diff/15025/ui/gfx/gl/gl_surface_nsview.h
File ui/gfx/gl/gl_surface_nsview.h (right):

http://codereview.chromium.org/8486020/diff/15025/ui/gfx/gl/gl_surface_nsview...
ui/gfx/gl/gl_surface_nsview.h:36: gfx::PluginWindowHandle view_;
On 2011/11/17 22:28:18, kbr wrote:
> nit: the gfx scoping is unnecessary here.

Done.

[Ken Russell (switch to Gerrit), 2011-11-18 01:11:00]

Thanks, looks great. LGTM

[dhollowa, 2011-11-18 16:45:22]

+tony, +jam for webkit/glue, webkit/plugins OWNERS respectively.

[tony, 2011-11-18 18:48:36]

webkit/glue LGTM

[tony, 2011-11-18 18:50:10]

http://codereview.chromium.org/8486020/diff/20001/webkit/glue/webkit_glue.gypi
File webkit/glue/webkit_glue.gypi (right):

http://codereview.chromium.org/8486020/diff/20001/webkit/glue/webkit_glue.gyp...
webkit/glue/webkit_glue.gypi:474: ['exclude',
'^../plugins/npapi/webplugin_delegate_impl_mac.mm'],
Nit: This is a regex so the . will match any char.  Doesn't really matter here,
but if you wanted to be thorough, you could use \\ escape the dots.

[jam, 2011-11-18 19:24:36]

webkit/plugins lgtm

[dhollowa, 2011-11-19 02:12:52]

http://codereview.chromium.org/8486020/diff/20001/webkit/glue/webkit_glue.gypi
File webkit/glue/webkit_glue.gypi (right):

http://codereview.chromium.org/8486020/diff/20001/webkit/glue/webkit_glue.gyp...
webkit/glue/webkit_glue.gypi:474: ['exclude',
'^../plugins/npapi/webplugin_delegate_impl_mac.mm'],
On 2011/11/18 18:50:10, tony wrote:
> Nit: This is a regex so the . will match any char.  Doesn't really matter
here,
> but if you wanted to be thorough, you could use \\ escape the dots.

Done.

[apatrick_chromium, 2011-11-21 23:34:45]

Sorry I missed this one going by. A couple of points.

http://codereview.chromium.org/8486020/diff/27001/ui/gfx/gl/gl_context_nsview.mm
File ui/gfx/gl/gl_context_nsview.mm (right):

http://codereview.chromium.org/8486020/diff/27001/ui/gfx/gl/gl_context_nsview...
ui/gfx/gl/gl_context_nsview.mm:50:
static_cast<GLSurfaceNSView*>(surface)->SetGLContext(this);
GLContext implementations should not make assumptions about the concrete type of
a GLSurface because the object might actually be an adapter around the
underlying GLSurface. We do this on other platforms so it would be nice not to
make the assumption on mac.

http://codereview.chromium.org/8486020/diff/27001/ui/gfx/gl/gl_surface_nsview.h
File ui/gfx/gl/gl_surface_nsview.h (right):

http://codereview.chromium.org/8486020/diff/27001/ui/gfx/gl/gl_surface_nsview...
ui/gfx/gl/gl_surface_nsview.h:29: virtual void* GetHandle() OVERRIDE;
It would be safer to implement OnMakeCurrent here instead and cache off the
context at that point. That would also make this code work in the event that a
surface is made current with a surface that is different to the one that was
passed to GLContextNSView::Initialize.