Bug 66504

Summary:

Chromium Mac: Need tests for switching between overlay and opaque scrollbars

Product:

WebKit

Reporter:

Sailesh Agrawal <sail>

Component:

Platform

Assignee:

Sailesh Agrawal <sail>

Status:

RESOLVED WONTFIX

Severity:

Normal

CC:

abarth, asvitkine, bdakin, dglazkov, gustavo.noronha, gustavo, jamesr, marshall, schenney, simon.fraser, thakis, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.7

Bug Depends on:

68134

Bug Blocks:

Attachments:

Description	Flags
Patch	thakis: review-, webkit.review.bot: commit-queue-

Sailesh Agrawal

Reported 2011-08-18 15:40:22 PDT

This bug is to track the work need to add new tests to test the scrollbar drawing code when switching between opaque and overlay scrollbars.

Attachments
Patch (52.07 KB, patch) 2011-08-25 14:35 PDT, Sailesh Agrawal	thakis: review- webkit.review.bot: commit-queue-	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Sailesh Agrawal

Comment 1 2011-08-25 14:35:41 PDT

Created attachment 105245 [details] Patch

Sailesh Agrawal

Comment 2 2011-08-25 14:39:53 PDT

This is not ready to be checked in but I wanted to send it out to get some early feedback. dglazkov - is adding the new window.internals API ok? bdakin - does this approach seem ok? if so do you want me to make similar changes to ScrollbarThemeMac and ScrollAnimatorMac? jamesr / thakis: I'm not sure where the test files are supposed to go. Does this seem correct? The tests in this patch don't actually change the scrollbar type at run time. Once this is checked in I'll send out a separate patch with that test.

Dimitri Glazkov (Google)

Comment 3 2011-08-25 14:53:13 PDT

Comment on attachment 105245 [details] Patch Is window.internals the right spot to expose this API? The code looks pretty chromium-specific, so perhaps this should be a layoutTestController API?

Sailesh Agrawal

Comment 4 2011-08-25 14:54:44 PDT

(In reply to comment #3) > (From update of attachment 105245 [details]) > Is window.internals the right spot to expose this API? The code looks pretty chromium-specific, so perhaps this should be a layoutTestController API? This is actually mac specific. It should work Safari's mac port as well.

WebKit Review Bot

Comment 5 2011-08-25 17:29:51 PDT

Comment on attachment 105245 [details] Patch Attachment 105245 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9507678 New failing tests: platform/mac-lion/scrollbars/opaque-scrollbar.html platform/mac-lion/scrollbars/overlay-scrollbar.html

Collabora GTK+ EWS bot

Comment 6 2011-08-25 22:35:02 PDT

Comment on attachment 105245 [details] Patch Attachment 105245 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9513919

Beth Dakin

Comment 7 2011-08-26 15:51:16 PDT

(In reply to comment #2) > bdakin - does this approach seem ok? if so do you want me to make similar changes to ScrollbarThemeMac and ScrollAnimatorMac? Hi Sailesh, this looks like a good approach! It would be awesome if you would add similar code to ScrollbarThemeMac and ScrollAnimatorMac. My one comment about the naming in the ScrollbarStyle enum, which currently looks like this in your patch: enum ScrollbarStyle { ScrollbarStyleSystemDefault, ScrollbarStyleLegacy, ScrollbarStyleOverlay }; The phrase "legacy scrollbar" is meaningful on the Mac platform, but it's not meaningful on other platforms. Since this enum lives and will be used in cross-platform code, I recommend a name that makes sense on all platforms. What about "physical scrollbars," or specifically ScrollbarStylePhysical for the enum?

Nico Weber

Comment 8 2011-09-16 16:08:28 PDT

Comment on attachment 105245 [details] Patch r- based on bdakin's comments

Note You need to log in before you can comment on or make changes to this bug.