Bug 59035 – WebKit2: Need a way to get a snapshot of the visible contents of a page from the UI Process

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jessie Berlin]

<rdar://problem/8607239>

[Jessie Berlin]

Created attachment 90443 [details]
Patch

[Brian Weinstein]

Comment on attachment 90443 [details]
Patch

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

I'm going to let Sam review this, just a couple comments I had.

> Source/WebKit2/ChangeLog:22
> +        with a scale factor of 1.

Is this correct if the view is currently scaled when we ask for the snapshot? The visible contents might be at a scale factor of 2, would we expect the snapshot to show that?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:3032
> +    process()->sendSync(Messages::WebPage::VisibleContentsSnapshot(), Messages::WebPage::VisibleContentsSnapshot::Reply(snapshotHandle), m_pageID, 1);

A constant would be nice for the 1 here.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:854
> +    RefPtr<WebImage> snapshotImage = scaledSnapshotInDocumentCoordinates(contentRect, 1, ImageOptionsShareable);

Do we want the 1 here? Or do we want the current scale factor of the frame? (Repeat of question in ChangeLog).

[Jessie Berlin]

(In reply to comment #2)
> (From update of attachment 90443 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90443&action=review
> 
> I'm going to let Sam review this, just a couple comments I had.

Thanks for the feedback!

> 
> > Source/WebKit2/ChangeLog:22
> > +        with a scale factor of 1.
> 
> Is this correct if the view is currently scaled when we ask for the snapshot? The visible contents might be at a scale factor of 2, would we expect the snapshot to show that?

That is already taken into account when the snapshot is made of that rect. The scale factor is any additional scaling that is desired.

> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:3032
> > +    process()->sendSync(Messages::WebPage::VisibleContentsSnapshot(), Messages::WebPage::VisibleContentsSnapshot::Reply(snapshotHandle), m_pageID, 1);
> 
> A constant would be nice for the 1 here.

Changed to be:

   // Do not wait for more than a second (arbitrary) for the WebProcess to get the snapshot so
    // that the UI Process is not permanently stuck waiting on a potentially crashing Web Process.
    static const double visibleContentsSnapshotSyncMessageTimeout = 1.0;
    process()->sendSync(Messages::WebPage::VisibleContentsSnapshot(), Messages::WebPage::VisibleContentsSnapshot::Reply(snapshotHandle), m_pageID, visibleContentsSnapshotSyncMessageTimeout);


> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:854
> > +    RefPtr<WebImage> snapshotImage = scaledSnapshotInDocumentCoordinates(contentRect, 1, ImageOptionsShareable);
> 
> Do we want the 1 here? Or do we want the current scale factor of the frame? (Repeat of question in ChangeLog).

See earlier response. I will update the Changelog comment to say that using a scale factor of one preserves any scaling already done on the web page (e.g. by zooming in or out).

[Sam Weinig]

Comment on attachment 90443 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:69
> +WK_EXPORT WKImageRef WKPageVisibleContentsSnapshot(WKPageRef pageRef);

This function name needs to Create in its name to indicate the ownership model.  I think WKPageCreateSnapshotOfVisibleContent() to match the one in the bundle, would be good.

We usually leave the Ref off the parameter name in the header.

[Jessie Berlin]

(In reply to comment #4)
> (From update of attachment 90443 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90443&action=review
> 
> > Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:69
> > +WK_EXPORT WKImageRef WKPageVisibleContentsSnapshot(WKPageRef pageRef);
> 
> This function name needs to Create in its name to indicate the ownership model.  I think WKPageCreateSnapshotOfVisibleContent() to match the one in the bundle, would be good.

Done (and changed all the other instances of visibleContentsSnapshot to createSnapshotOfVisibleContent).

> 
> We usually leave the Ref off the parameter name in the header.

I was wondering what our official style was for that. Fixed.

[Jessie Berlin]

Comment on attachment 90443 [details]
Patch

Committed in http://trac.webkit.org/changeset/84519

[WebKit Review Bot]

http://trac.webkit.org/changeset/84519 might have broken Qt Linux ARMv7 Release and Qt Windows 32-bit Debug