[chromium] Things jump around when selecting anything on the page

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alok Priyadarshi]

With accelerated drawing (--enable-accelerated-drawing), things jump around when anything is selected on the page.

Test case: http://www.webkit.org/blog-files/3d-transforms/morphing-cubes.html

This is happening because when anything is selected, only a portion of a tile is invalidated and updated. LayerTextureUpdaterSkPicture::updateTextureRect does not update a tile subregion properly.

[Alok Priyadarshi]

Created attachment 95184 [details]
proposed patch

[Vangelis Kokkevis]

Comment on attachment 95184 [details]
proposed patch

The offset changes look good.  Did you mean to include the removal of clear and change in framebufferRenderbuffer with this patch?  If so, please add some explanation in the changelog.

[Alok Priyadarshi]

Created attachment 95198 [details]
proposed patch

Added explanation for removing glClear to changelog.

[Alok Priyadarshi]

(In reply to comment #2)
> (From update of attachment 95184 [details])
> The offset changes look good.  Did you mean to include the removal of clear and change in framebufferRenderbuffer with this patch?  If so, please add some explanation in the changelog.

Yes I meant to remove glClear. Added explanation to changelog.

[James Robinson]

Comment on attachment 95198 [details]
proposed patch

Need a test!

[Alok Priyadarshi]

This case should be covered by most of the layout tests (pixel) targeting hover/selection/editing when chrome is run in compositing mode. Do you want me write a similar test with a composited layer? I heard that we are planning on running all layout tests in compositing mode too. If so, is there any reason to duplicate tests?

[James Robinson]

No, you don't need to add redundant test coverage if this bugfix causes existing tests to progress.  You do need to record which test(s) covers this change in the ChangeLog and add a new test if there aren't any tests currently that cover this.

[Alok Priyadarshi]

Created attachment 95665 [details]
proposed patch

Added test info to changelog

[James Robinson]

Comment on attachment 95665 [details]
proposed patch

Seems good!

[Alok Priyadarshi]

Committed r87949: <http://trac.webkit.org/changeset/87949>

[Adrienne Walker]

Maybe I'm missing something, but the editing tests don't get run automatically with the compositor, so if this breaks in the future, we won't know about it.

Would compositing/repaint/same-size-invalidation.html have caught this? Or, is there some other automatic test that would?

[James Robinson]

(In reply to comment #11)
> Maybe I'm missing something, but the editing tests don't get run automatically with the compositor, so if this breaks in the future, we won't know about it.
> 
> Would compositing/repaint/same-size-invalidation.html have caught this? Or, is there some other automatic test that would?

To be fair, our automated tests don't run with --accelerated-drawing on either.

[Adrienne Walker]

(In reply to comment #12)
> (In reply to comment #11)
> > Maybe I'm missing something, but the editing tests don't get run automatically with the compositor, so if this breaks in the future, we won't know about it.
> > 
> > Would compositing/repaint/same-size-invalidation.html have caught this? Or, is there some other automatic test that would?
> 
> To be fair, our automated tests don't run with --accelerated-drawing on either.

Ah, fair enough.  I guess manual tests are all that can be done then.

[Alok Priyadarshi]

(In reply to comment #11)
> Maybe I'm missing something, but the editing tests don't get run automatically with the compositor, so if this breaks in the future, we won't know about it.
> 

You are right. As I mentioned in the changelog, they will get run if chrome is run in forced compositing mode. I am looking to see if we can run all layout tests with --force-compositing-mode flag on gpu bots. I will also enable accelerated-drawing once it is stable enough.