Bug 92275 – Need a way to get a snapshot image that does not show the selection

• Component: WebKit2
• Status: RESOLVED FIXED

[Jeff Miller]

We need a way to get a snapshot image that does not show the selection.

For example, navigate to any page and choose Edit->Select All (without a form field selected) to highlight the entire web page. I'd like to be able to generate a snapshot of the page without the selection highlighting.

Simon and Sam suggested that this may require a new paint behavior, and Sam said we could expose this through a new API like WKBundlePageCreateSnapshotInViewCoordinatesWithOptions(). Dan notes that Frame::rangeImage shows how to save and restore the render tree selection without side effects.

[Radar WebKit Bug Importer]

<rdar://problem/11956802>

[Beth Dakin]

Created attachment 155592 [details]
Patch

[Sam Weinig]

Comment on attachment 155592 [details]
Patch

It seems a bit odd to make WKImageOptions know about document coordinates and selection, since it is really just a generic image class.  These options would be better suited as specific to the snapshot function I think.

[Beth Dakin]

(In reply to comment #3)
> (From update of attachment 155592 [details])
> It seems a bit odd to make WKImageOptions know about document coordinates and selection, since it is really just a generic image class.  These options would be better suited as specific to the snapshot function I think.

WKSnapshotOptions?

[Sam Weinig]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 155592 [details] [details])
> > It seems a bit odd to make WKImageOptions know about document coordinates and selection, since it is really just a generic image class.  These options would be better suited as specific to the snapshot function I think.
> 
> WKSnapshotOptions?

Yeah, and just one of the options will make it create a shareable image.  This probably means we need a new function name as well, so how about WKBundlePageCreateSnapshopWithOptions().

[Beth Dakin]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 155592 [details] [details] [details])
> > > It seems a bit odd to make WKImageOptions know about document coordinates and selection, since it is really just a generic image class.  These options would be better suited as specific to the snapshot function I think.
> > 
> > WKSnapshotOptions?
> 
> Yeah, and just one of the options will make it create a shareable image.  This probably means we need a new function name as well, so how about WKBundlePageCreateSnapshopWithOptions().

My patch already adds a function with this very name.

The term “selection” might be ambiguous in the API, because it may be mistaken to mean the selected range, rather than just the selection highlighting (and other effects), especially when combined with “exclude”. I don’t have a concrete alternative name to suggest.

[John Sullivan]

(In reply to comment #7)
> The term “selection” might be ambiguous in the API, because it may be mistaken to mean the selected range, rather than just the selection highlighting (and other effects), especially when combined with “exclude”. I don’t have a concrete alternative name to suggest.

ImageOptionsSnapshotExcludingSelection could be ImageOptionsSnapshotExcludingSelectionHighlight

That might not be perfectly accurate, but it seems clearer.

[Beth Dakin]

Created attachment 155606 [details]
New patch

[Simon Fraser (smfr)]

Comment on attachment 155606 [details]
New patch

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.

[Beth Dakin]

(In reply to comment #10)
> (From update of attachment 155606 [details])
> 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.
> 

Hmm, good point. I originally implemented this by adding 4 new member variables to RenderView to cache this selection information. That approach would avoid this problem, but of course it has the downside of adding 4 member variables to RenderView. Hmm. I'll have to think about this.

> > 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.
> 

I didn't add one because ImageOptions (in the same file) does not have one. Neither does FindOptions…it seems like most of the "options" don't do this. But that doesn't mean it's a good precedent, I suppose.

> > Source/WebKit2/Shared/API/c/WKSharedAPICast.h:761
> > +inline SnapshotOptions wkImageOptionsToSnapshotOptions(WKImageOptions wkImageOptions)
> 
> I think it makes slightly more sense to say snapshotOptionsFromImageOptions
> 

Okay, fixed. 

> > Source/WebKit2/Shared/API/c/WKSharedAPICast.h:768
> > +    return static_cast<SnapshotOptions>(snapshotOptions);
> 
> This is why you need that typedef.
> 

Again, the static cast matches the other "options" conversions in this file.

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

The standard in the file is to use 'to' not 'from.' See toImageOptions() and toFindOptions().

> > 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)?
> 

The plan Sam outlines in the Radar is indeed to deprecate the older methods. As I explain the the Changelog, WKBundlePageCreateSnapshotWithOptions() assumes view coordinated unless you specify document coordinates with that option. 

They are used a few places in Safari, so we should remove them, but I think that should be a separate patch.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1228
> > +    FrameView::SelectionInSnaphot shouldPaintSelection = FrameView::ExcludeSelection;
> 
> Shouldn't this default to IncludeSelection?
> 

Oops! Yes. I just set it to Exclude in order to test and forgot to set it back.

> > 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.

This is probably true.

[Beth Dakin]

Created attachment 155647 [details]
New patch that attempts to address security concern

(In reply to comment #12)
> Created an attachment (id=155647) [details]
> New patch that attempts to address security concern

I think this only partly addresses the concern. Namely, it addresses the fact that a RenderView may go away, but not any of the other two renderer you save for each frame.

It occurs to me that rather than save, clear, and then restore the RenderView selection, you might be able to just clear it, and then later invoke FrameSelection::updateAppearance, which will reconstruct it in a safe manner.

[Beth Dakin]

Created attachment 155851 [details]
Patch

Good call, Dan! That totally works. Thank you!

[Eric Seidel (no email)]

Sounds useful for chrome://newtab

[Eric Seidel (no email)]

This appears to be solving some of the same goals as bug 92043.

[Beth Dakin]

Thanks Anders! Committed change: http://trac.webkit.org/changeset/125091

[Balazs Kelemen]

After this change we don't draw scrollbars anymore, because now it calls FrameView::paintContents instead of ScrollView::paint, so it breaks a huge amount of pixel results (I checked Qt but I guess the same happens with the Mac port.) Was that intentional? If no, I'm happy to prepare a patch to fix this.

[Beth Dakin]

(In reply to comment #18)
> After this change we don't draw scrollbars anymore, because now it calls FrameView::paintContents instead of ScrollView::paint, so it breaks a huge amount of pixel results (I checked Qt but I guess the same happens with the Mac port.) Was that intentional? If no, I'm happy to prepare a patch to fix this.

That was not intentional. Can you tell me some specific tests that are failing? So far I have not seen any failures in the Mac port.

[Balazs Kelemen]

(In reply to comment #19)
> (In reply to comment #18)
> > After this change we don't draw scrollbars anymore, because now it calls FrameView::paintContents instead of ScrollView::paint, so it breaks a huge amount of pixel results (I checked Qt but I guess the same happens with the Mac port.) Was that intentional? If no, I'm happy to prepare a patch to fix this.
> 
> That was not intentional. Can you tell me some specific tests that are failing? So far I have not seen any failures in the Mac port.

Have you run pixel tests? Bots don't do it. Basically every test that normally shows scrollbars are broken. For example there are 67 image fail in css1 with Qt (this can vary between ports according to the number of skipped tests):

run-webkit-tests -p -2 LayoutTests/css1
...
Regressions: Unexpected image-only failures : (67)
  css1/basic/comments.html = IMAGE
  css1/basic/containment.html = IMAGE
  css1/basic/id_as_selector.html = IMAGE
  css1/basic/inheritance.html = IMAGE
  css1/box_properties/border.html = IMAGE
  css1/box_properties/border_bottom.html = IMAGE
  css1/box_properties/border_bottom_width.html = IMAGE
  css1/box_properties/border_left.html = IMAGE
  ...

[Balazs Kelemen]

Filed bug 93693 to fix the scrollbar issue.