Bug 123646

Summary: Remote Layer Tree: Move RemoteLayerBackingStore to Shared/
Product: WebKit Reporter: Tim Horton <thorton>
Component: WebKit2Assignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, commit-queue, darin, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
darin: review+
address review comments none

Description Tim Horton 2013-11-01 16:03:56 PDT
Move RemoteLayerBackingStore to Shared/, since it is used on both the UI and Web process side of things.
Comment 1 Tim Horton 2013-11-01 16:04:54 PDT
Created attachment 215775 [details]
patch

will move with svn mv
Comment 2 Darin Adler 2013-11-01 16:13:02 PDT
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.
Comment 3 Tim Horton 2013-11-01 16:20:11 PDT
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.
Comment 4 Tim Horton 2013-11-01 16:23:28 PDT
Did the move in http://trac.webkit.org/changeset/158463
Comment 5 Tim Horton 2013-11-01 16:33:52 PDT
(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.
Comment 6 Tim Horton 2013-11-01 17:49:07 PDT
Created attachment 215787 [details]
address review comments
Comment 7 WebKit Commit Bot 2013-11-02 09:02:54 PDT
Comment on attachment 215787 [details]
address review comments

Clearing flags on attachment: 215787

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