[webkit-reviews] review denied: [Bug 92275] Need a way to get a snapshot image that does not show the selection : [Attachment 155606] New patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Beth Dakin
<bdakin at apple.com>'s request for review:
Bug 92275: Need a way to get a snapshot image that does not show the selection
https://bugs.webkit.org/show_bug.cgi?id=92275

Attachment 155606: New patch
https://bugs.webkit.org/attachment.cgi?id=155606&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=155606&action=review


> Source/WebCore/page/FrameView.cpp:3207
> +    HashMap<RenderView*, RenderViewSelectionInfo> selectionMap;
> +    if (shouldPaintSelection == ExcludeSelection) {
> +	   for (Frame* frame = m_frame.get(); frame; frame =
frame->tree()->traverseNext(m_frame.get())) {
> +	       if (RenderView* root = frame->contentRenderer()) {
> +		   RenderObject* currentStartObject;
> +		   int currentStartPos;
> +		   RenderObject* currentEndObject;
> +		   int currentEndPos;
> +		   root->getSelection(currentStartObject, currentStartPos,
currentEndObject, currentEndPos);
> +		   selectionMap.add(root,
RenderViewSelectionInfo(currentStartObject, currentStartPos, currentEndObject,
currentEndPos));
> +		   root->clearSelection();
> +	       }
> +	   }
> +    }
> +
> +    paintContents(context, imageRect);
> +
> +    // Restore cached selection information.
> +    const HashMap<RenderView*, RenderViewSelectionInfo>::const_iterator end
= selectionMap.end();
> +    for (HashMap<RenderView*, RenderViewSelectionInfo>::const_iterator it =
selectionMap.begin(); it != end; ++it) {
> +	   RenderView* view = it->first;
> +	   RenderViewSelectionInfo selectionInfo = it->second;
> +	   view->setSelection(selectionInfo.startObject,
selectionInfo.startPos, selectionInfo.endObject, selectionInfo.endPos);
> +    }

This makes me a bit nervous. If anything happens during the paint that causes
RenderObjects to get destroyed (e.g. a plugin runs script), then your pointers
can go bad and you have a nasty security issue.

> Source/WebKit2/Shared/ImageOptions.h:39
> +enum SnapshotOptions {
> +    SnapshotOptionsShareable = 1 << 0,
> +    SnapshotOptionsInDocumentCoordinates = 1 << 1,
> +    SnapshotOptionsExcludeSelectionHighlighting = 1 << 2
> +};

I think this needs a typedef for the type that holds the union of the flags,
like WKSnapshotOptions.

> Source/WebKit2/Shared/API/c/WKSharedAPICast.h:761
> +inline SnapshotOptions wkImageOptionsToSnapshotOptions(WKImageOptions
wkImageOptions)

I think it makes slightly more sense to say snapshotOptionsFromImageOptions

> Source/WebKit2/Shared/API/c/WKSharedAPICast.h:768
> +    return static_cast<SnapshotOptions>(snapshotOptions);

This is why you need that typedef.

> Source/WebKit2/Shared/API/c/WKSharedAPICast.h:771
> +inline SnapshotOptions toSnapshotOptions(WKSnapshotOptions
wkSnapshotOptions)

Also use 'from'.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:404
> +WK_EXPORT WKImageRef WKBundlePageCreateSnapshotWithOptions(WKBundlePageRef
page, WKRect rect, WKSnapshotOptions options);
>  WK_EXPORT WKImageRef
WKBundlePageCreateSnapshotInViewCoordinates(WKBundlePageRef page, WKRect rect,
WKImageOptions options);
>  WK_EXPORT WKImageRef
WKBundlePageCreateSnapshotInDocumentCoordinates(WKBundlePageRef page, WKRect
rect, WKImageOptions options);
>  WK_EXPORT WKImageRef
WKBundlePageCreateScaledSnapshotInDocumentCoordinates(WKBundlePageRef page,
WKRect rect, double scaleFactor, WKImageOptions options);

So many snapshot methods!  What coordinate system does
WKBundlePageCreateSnapshotWithOptions() use? How is it different from the 3
others? I think we should try hard not to add a new method here. Maybe the
others should all be one, with flags.

Oh, I see that you're adding one with flags. Can we mark the others as
deprecated? Can we just removed them (since we don't have many API clients
yet)?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1228
> +    FrameView::SelectionInSnaphot shouldPaintSelection =
FrameView::ExcludeSelection;

Shouldn't this default to IncludeSelection?

> Source/WebKit2/WebProcess/WebPage/WebPage.h:352
> +    PassRefPtr<WebImage> snapshotWithOptions(const WebCore::IntRect&,
SnapshotOptions);
> +    PassRefPtr<WebImage> snapshotInViewCoordinates(const WebCore::IntRect&,
SnapshotOptions);
> +    PassRefPtr<WebImage> snapshotInDocumentCoordinates(const
WebCore::IntRect&, SnapshotOptions);
> +    PassRefPtr<WebImage> scaledSnapshotInDocumentCoordinates(const
WebCore::IntRect&, double scaleFactor, SnapshotOptions);

These aren't API, so perhaps we can merge them.