Created attachment 157649 [details] Patch

Comment on attachment 157649 [details] Patch This is not the right fix. Prior to r125091, WebPage had two functions for taking snapshots, snapshotInViewCoordinates, which captured scroll bars (and only the visible part of the document view) and snapshotInDocumentCoordinates, which didn’t capture scroll bar (and could capture any part of the document view). r125091 neglected to notice this difference and left only one function which works similarly to the old snapshotInDocumentCoordinates, and made WKBundlePageCreateSnapshotInViewCoordinates call into it.

I'm confused. The author, Beth Dakin said the change in pixel test behavior was not intentional. Isn't the unified method supposed to behave like snapshotInViewCoordinates (it is in fact called from WKBundlePageCreateSnapshotInViewCoordinates)?

(In reply to comment #3) > I'm confused. The author, Beth Dakin said the change in pixel test behavior was not intentional. Correct. > Isn't the unified method supposed to behave like snapshotInViewCoordinates (it is in fact called from WKBundlePageCreateSnapshotInViewCoordinates)? No, it isn’t. It is also called from WKBundlePageCreateSnapshotInDocumentCoordinates, which expects it to have the behavior snapshotInDocumentCoordinates had. The fix is to either separate the function back into two, or add an option telling it how to behave, so that it can keep serving both APIs as expected.

(In reply to comment #4) > No, it isn’t. It is also called from WKBundlePageCreateSnapshotInDocumentCoordinates, which expects it to have the behavior snapshotInDocumentCoordinates had. The fix is to either separate the function back into two, or add an option telling it how to behave, so that it can keep serving both APIs as expected. I think we should add a new SnapshotOptions for including or excluding the scrollbar, much like there is an option fro including/excluding selection highlighting. The FrameView::paintContentsForSnapshot could take another parameter of representing whether it should include scrollbars and call paint() or not and call paintContents().

(In reply to comment #5) > (In reply to comment #4) > > No, it isn’t. It is also called from WKBundlePageCreateSnapshotInDocumentCoordinates, which expects it to have the behavior snapshotInDocumentCoordinates had. The fix is to either separate the function back into two, or add an option telling it how to behave, so that it can keep serving both APIs as expected. > > I think we should add a new SnapshotOptions for including or excluding the scrollbar, much like there is an option fro including/excluding selection highlighting. The FrameView::paintContentsForSnapshot could take another parameter of representing whether it should include scrollbars and call paint() or not and call paintContents(). And I am happy to write up this patch if you are busy, Balazs. Just let me know.

> > I think we should add a new SnapshotOptions for including or excluding the scrollbar, much like there is an option fro including/excluding selection highlighting. The FrameView::paintContentsForSnapshot could take another parameter of representing whether it should include scrollbars and call paint() or not and call paintContents(). > > And I am happy to write up this patch if you are busy, Balazs. Just let me know. Well, yes I had other stuff to do, and now it's already late evening here, so I appreciate if you pick this up :)

Created attachment 157802 [details] Patch that adds new SnapshotOptions

Attachment 157802 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/page/FrameView.h:251: The parameter name "coordinateSpace" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 157804 [details] Patch for style bot

Created attachment 157923 [details] Patch

(In reply to comment #11) > Created an attachment (id=157923) [details] > Patch That seems fine to me, could somebody review the patch?

Thanks Sam! Committed http://trac.webkit.org/changeset/125700