Bug 149069

Summary:

Should use CARenderServerRenderLayerWithTransform for snapshots on iOS

Product:

WebKit

Reporter:

Beth Dakin <bdakin>

Component:

WebKit2

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bdakin, cdumez, thorton

Priority:

Version:

Other

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	thorton: review+

Description Beth Dakin 2015-09-11 11:49:07 PDT

We should use CARenderServerRenderLayerWithTransform for snapshots on iOS. This will let us own the backing IOSurface, which will let us make them purgeable and share more code with Mac.

Comment 1 Beth Dakin 2015-09-11 12:02:21 PDT

Created attachment 261010 [details]
Patch

Comment 2 Tim Horton 2015-09-11 12:04:57 PDT

Comment on attachment 261010 [details]
Patch

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

> Source/WebKit2/ChangeLog:11
> +        This will let us own the backing IOSurface, which will let us make them 
> +        purgeable and share more code with Mac.

Except we're not going to make them purgeable for now and it's not clear that that is actually the plan so maybe we shouldn't mention that part :D

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1110
> +    auto surface = WebCore::IOSurface::create(WebCore::expandedIntSize(snapshotSize), WebCore::ColorSpaceDeviceRGB);

We should do more perf testing here.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1111
> +    CARenderServerRenderLayerWithTransform(MACH_PORT_NULL, self.layer.context.contextId, reinterpret_cast<uint64_t>(self.layer), surface->surface(), 0, 0, &transform);

I wonder if we have to use some other function on older OSes, like _snapshotWhatever does? Probably.

> Source/WebKit2/UIProcess/mac/ViewSnapshotStore.h:-44
> -#if PLATFORM(MAC)

YAYYYYY

Comment 3 Tim Horton 2015-09-11 12:05:17 PDT

Also not sure I can really review this :D

Comment 4 Beth Dakin 2015-09-11 14:25:19 PDT

(In reply to comment #2)
> Comment on attachment 261010 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261010&action=review
> 
> > Source/WebKit2/ChangeLog:11
> > +        This will let us own the backing IOSurface, which will let us make them 
> > +        purgeable and share more code with Mac.
> 
> Except we're not going to make them purgeable for now and it's not clear
> that that is actually the plan so maybe we shouldn't mention that part :D
> 

Fixed this!

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1110
> > +    auto surface = WebCore::IOSurface::create(WebCore::expandedIntSize(snapshotSize), WebCore::ColorSpaceDeviceRGB);
> 
> We should do more perf testing here.
> 

Perf bot watcher has been alerted! 

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1111
> > +    CARenderServerRenderLayerWithTransform(MACH_PORT_NULL, self.layer.context.contextId, reinterpret_cast<uint64_t>(self.layer), surface->surface(), 0, 0, &transform);
> 
> I wonder if we have to use some other function on older OSes, like
> _snapshotWhatever does? Probably.
> 

Tim and I considered this, and he changed his mind.

> > Source/WebKit2/UIProcess/mac/ViewSnapshotStore.h:-44
> > -#if PLATFORM(MAC)
> 
> YAYYYYY


YAY. Thanks Tim!

http://trac.webkit.org/changeset/189628

Comment 5 Beth Dakin 2015-09-11 14:29:09 PDT

Build fix: http://trac.webkit.org/changeset/189630

Comment 6 Beth Dakin 2015-09-11 15:34:15 PDT

Another build fix: http://trac.webkit.org/changeset/189635

Comment 7 Beth Dakin 2015-09-11 16:06:52 PDT

More build fix: http://trac.webkit.org/changeset/189637