Bug 123646 – Remote Layer Tree: Move RemoteLayerBackingStore to Shared/

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

Move RemoteLayerBackingStore to Shared/, since it is used on both the UI and Web process side of things.

[Tim Horton]

Created attachment 215775 [details]
patch

will move with svn mv

[Darin Adler]

Comment on attachment 215775 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215775&action=review

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:40
> +#ifdef __has_include
> +#if __has_include(<ApplicationServices/ApplicationServicesPriv.h>)

I think we could do this with a single if instead:

    #if defined __has_include && __has_include(<ApplicationServices/ApplicationServicesPriv.h>)

I didn’t think of that earlier because I found the whole idiom so strange.

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:152
> +    unsigned long bytesPerRow = IOSurfaceAlignProperty(kIOSurfaceBytesPerRow, width * bytesPerElement);

This returns size_t, not unsigned long.

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:155
> +    unsigned long allocSize = IOSurfaceAlignProperty(kIOSurfaceAllocSize, height * bytesPerRow);

Ditto.

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:215
> +        backSurface = createIOSurface(expandedScaledSize);

Why not use m_frontSurface directly here and dump the backSurface local?

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:218
> +        CGContextClearRect(cgContext.get(), CGRectMake(0, 0, expandedScaledSize.width(), expandedScaledSize.height()));

Could we use CGRectInfinite instead?

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:222
> +        backBuffer = ShareableBitmap::createShareable(expandedIntSize(scaledSize), ShareableBitmap::SupportsAlpha);

Why not use m_frontBuffer directly here and dump the backBuffer local?

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:226
> +    drawInContext(*context);

It’s kind of funny that we allocate the context on the heap just so we can share this one line of code.

[Tim Horton]

Heh, thanks, Darin! I'll make those changes in another patch since it doesn't seem like a good idea to do them in a file-move patch.

[Tim Horton]

Did the move in http://trac.webkit.org/changeset/158463

[Tim Horton]

(In reply to comment #2)
> (From update of attachment 215775 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215775&action=review
> 
> > Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:40
> > +#ifdef __has_include
> > +#if __has_include(<ApplicationServices/ApplicationServicesPriv.h>)
> 
> I think we could do this with a single if instead:
> 
>     #if defined __has_include && __has_include(<ApplicationServices/ApplicationServicesPriv.h>)
> 
> I didn’t think of that earlier because I found the whole idiom so strange.

We could! That does look a lot nicer. None of the other instances of this idiom do, but I don't see why not.

> > Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:215
> > +        backSurface = createIOSurface(expandedScaledSize);
> 
> Why not use m_frontSurface directly here and dump the backSurface local?
>
> > Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:222
> > +        backBuffer = ShareableBitmap::createShareable(expandedIntSize(scaledSize), ShareableBitmap::SupportsAlpha);
> 
> Why not use m_frontBuffer directly here and dump the backBuffer local?

I'm a bit concerned about releasing the existing m_frontSurface while we have an image referencing it for the same reason I discovered I need to hold onto the context - I'm pretty sure we'll copy the image data off the GPU. But I haven't tested or looked at the source in this specific case.

Doing that in the software case would be fine, because the CGImageRef will hold on to the ShareableBitmap while it's alive.

[Tim Horton]

Created attachment 215787 [details]
address review comments

[WebKit Commit Bot]

Comment on attachment 215787 [details]
address review comments

Clearing flags on attachment: 215787

Committed r158488: <http://trac.webkit.org/changeset/158488>