Bug 46864 - [chromium] disappearing scrollbar in accelerated compositing mode
Summary: [chromium] disappearing scrollbar in accelerated compositing mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nat Duca
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-29 18:24 PDT by Nat Duca
Modified: 2010-10-05 16:56 PDT (History)
5 users (show)

See Also:


Attachments
Proposed fix. (4.61 KB, patch)
2010-09-29 18:42 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Now, with 2x the foo (4.63 KB, patch)
2010-09-30 10:49 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Fix changelog (4.64 KB, patch)
2010-10-05 14:14 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Use content rect converted to screenspace rather than a hand-created rect. This ensures we factor in both scrollbars during scrolling. (4.63 KB, patch)
2010-10-05 16:02 PDT, Nat Duca
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nat Duca 2010-09-29 18:24:44 PDT
Composited pages with scrollbars will sometimes lose their scrollbar handle when scrolling. This is caused by the damage rect for the scrollbar being incorrectly handled in content space.
Comment 1 Nat Duca 2010-09-29 18:42:36 PDT
Created attachment 69294 [details]
Proposed fix.
Comment 2 WebKit Review Bot 2010-09-29 19:48:17 PDT
Attachment 69294 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4193020
Comment 3 Nat Duca 2010-09-30 10:49:19 PDT
Created attachment 69351 [details]
Now, with 2x the foo
Comment 4 Vangelis Kokkevis 2010-09-30 11:21:07 PDT
Comment on attachment 69351 [details]
Now, with 2x the foo

View in context: https://bugs.webkit.org/attachment.cgi?id=69351&action=review

> WebKit/chromium/src/WebViewImpl.cpp:2227
>      if (!view)

Is this a good opportunity to also take care of the FIXME at the top of this method?

> WebKit/chromium/src/WebViewImpl.cpp:2278
> +        m_rootLayerDirtyRect.unite(innerDamage);

Does this mean you'll now have damage in both the unscrolled location and the scrolled one? Could you just reset the m_rootLayerDirtyRect to be be equal to innerDamage?
Comment 5 Nat Duca 2010-09-30 11:27:38 PDT
> Is this a good opportunity to also take care of the FIXME at the top of this method?
I was going to file a separate bug for that.

> Does this mean you'll now have damage in both the unscrolled location and the 
> scrolled one? Could you just reset the m_rootLayerDirtyRect to be be equal to 
> innerDamage?
Uniting rather than reset is important. Consider when you have an animated image and you scrolled. The damage rect in that case has both the damage from the animation, and damage on the actual scrollbar that moved. Assigning innerDamage to the rootLayerDirtyRect would drop the srollbar damage and we'd see the scrollbar looking funky.
Comment 6 Vangelis Kokkevis 2010-09-30 15:01:34 PDT
(In reply to comment #5)
> > Is this a good opportunity to also take care of the FIXME at the top of this method?
> I was going to file a separate bug for that.
> 
> > Does this mean you'll now have damage in both the unscrolled location and the 
> > scrolled one? Could you just reset the m_rootLayerDirtyRect to be be equal to 
> > innerDamage?
> Uniting rather than reset is important. Consider when you have an animated image and you scrolled. The damage rect in that case has both the damage from the animation, and damage on the actual scrollbar that moved. Assigning innerDamage to the rootLayerDirtyRect would drop the srollbar damage and we'd see the scrollbar looking funky.

I chatted with Nat off line and I see now why this is necessary.  The patch looks good on my end.
Comment 7 James Robinson 2010-10-05 13:34:53 PDT
Comment on attachment 69351 [details]
Now, with 2x the foo

Looks good to me.  We discussed how to test this offline, it depends on a bit more infrastructure on the chromium end but we'll get tests up as soon as we can.
Comment 8 Nat Duca 2010-10-05 14:14:30 PDT
Created attachment 69840 [details]
Fix changelog
Comment 9 Nat Duca 2010-10-05 14:35:32 PDT
Darn, Sheib just pointed out a problem with this patch. Gotta go figure out how to address it.
Comment 10 James Robinson 2010-10-05 14:45:59 PDT
Comment on attachment 69840 [details]
Fix changelog

OK, clearing r+ in that case
Comment 11 Nat Duca 2010-10-05 16:02:44 PDT
Created attachment 69858 [details]
Use content rect converted to screenspace rather than a hand-created rect. This ensures we factor in both scrollbars during scrolling.
Comment 12 WebKit Commit Bot 2010-10-05 16:55:58 PDT
Comment on attachment 69858 [details]
Use content rect converted to screenspace rather than a hand-created rect. This ensures we factor in both scrollbars during scrolling.

Clearing flags on attachment: 69858

Committed r69162: <http://trac.webkit.org/changeset/69162>
Comment 13 WebKit Commit Bot 2010-10-05 16:56:03 PDT
All reviewed patches have been landed.  Closing bug.