[WK2] REGRESSION(125091): pixel results don't show scrollbars anymore

[Balazs Kelemen]

A typo was made in the patch, it calls FrameView::paintContents instead of paint so ScrollView has not got a chance to take care about scrollbars.

[Balazs Kelemen]

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.

[Balazs Kelemen]

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.

[Beth Dakin]

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

[Beth Dakin]

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

[Balazs Kelemen]

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

[Beth Dakin]

Created attachment 157802 [details]
Patch that adds new SnapshotOptions

[WebKit Review Bot]

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.

[Beth Dakin]

Created attachment 157804 [details]
Patch for style bot

[Beth Dakin]

Created attachment 157923 [details]
Patch

[Balazs Kelemen]

(In reply to comment #11)
> Created an attachment (id=157923) [details]
> Patch

That seems fine to me, could somebody review the patch?

[Beth Dakin]

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