Bug 92275

<rdar://problem/11956802>

Created attachment 155592 [details] Patch

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.

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

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

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

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

Created attachment 155606 [details] New patch

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.

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

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.

Created attachment 155851 [details] Patch Good call, Dan! That totally works. Thank you!

Sounds useful for chrome://newtab

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

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

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.

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

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

Filed bug 93693 to fix the scrollbar issue.