Bug 61639

Summary: [chromium] Things jump around when selecting anything on the page
Product: WebKit Reporter: Alok Priyadarshi <alokp>
Component: Layout and RenderingAssignee: Alok Priyadarshi <alokp>
Status: RESOLVED FIXED    
Severity: Normal CC: enne, jamesr, reed
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
proposed patch
none
proposed patch
jamesr: review-
proposed patch jamesr: review+

Alok Priyadarshi
Reported 2011-05-27 08:59:00 PDT
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.
Attachments
proposed patch (4.27 KB, patch)
2011-05-27 09:34 PDT, Alok Priyadarshi
no flags
proposed patch (4.54 KB, patch)
2011-05-27 11:18 PDT, Alok Priyadarshi
jamesr: review-
proposed patch (5.00 KB, patch)
2011-06-01 14:30 PDT, Alok Priyadarshi
jamesr: review+
Alok Priyadarshi
Comment 1 2011-05-27 09:34:59 PDT
Created attachment 95184 [details] proposed patch
Vangelis Kokkevis
Comment 2 2011-05-27 10:55:33 PDT
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
Comment 3 2011-05-27 11:18:10 PDT
Created attachment 95198 [details] proposed patch Added explanation for removing glClear to changelog.
Alok Priyadarshi
Comment 4 2011-05-27 11:18:56 PDT
(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 5 2011-05-31 19:34:52 PDT
Comment on attachment 95198 [details] proposed patch Need a test!
Alok Priyadarshi
Comment 6 2011-06-01 14:02:07 PDT
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
Comment 7 2011-06-01 14:12:27 PDT
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
Comment 8 2011-06-01 14:30:54 PDT
Created attachment 95665 [details] proposed patch Added test info to changelog
James Robinson
Comment 9 2011-06-02 13:11:05 PDT
Comment on attachment 95665 [details] proposed patch Seems good!
Alok Priyadarshi
Comment 10 2011-06-02 13:22:33 PDT
Adrienne Walker
Comment 11 2011-06-02 13:36:12 PDT
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
Comment 12 2011-06-02 13:39:04 PDT
(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
Comment 13 2011-06-02 13:42:11 PDT
(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
Comment 14 2011-06-02 13:51:22 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.