Chromium: Scrollbar with tickmarks doesn't respond to clicks

[Sailesh Agrawal]

Chromium: Scrollbar with tickmarks doesn't respond to clicks

[Sailesh Agrawal]

Created attachment 162641 [details]
Patch

[Build Bot]

Comment on attachment 162641 [details]
Patch

Attachment 162641 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13775613

[Nico Weber]

Does the apple port have this problem too? Does this patch fix things there as well?

Is it possible to test this?

[Sailesh Agrawal]

> Does the apple port have this problem too? Does this patch fix things there as well?
The apple port doesn't draw tickmarks on the scrollbar.

> Is it possible to test this?
I'm not sure if there's a way to add tickmarks from a test. Looking into it right now.

[Nico Weber]

There is, see the test I added here: https://bugs.webkit.org/show_bug.cgi?id=91949

[Sailesh Agrawal]

Just talked to Nico offline. Current there's no way to force the scrollbar code to draw in overlay mode. This means that this fix isn't testable right now.

I'll check with dpranke to see if we have 10.7/10.8 bots running chromium layout tests. If we do I'll send out a separate patch to add the testing infrastructure and add a test for this.

[Sailesh Agrawal]

Created attachment 162821 [details]
Patch

[Sailesh Agrawal]

Created attachment 162917 [details]
Patch

[Sailesh Agrawal]

Actually, adding the test infrastructure turned out the be fairly simple. I've included it with this patch.
On my 10.8 box, the test fails without my fix and passes with my fix.

I've kicked

Let me know if this looks ok and I'll add Beth and other folks to the review.

Thanks

[kov's GTK+ EWS bot]

Comment on attachment 162917 [details]
Patch

Attachment 162917 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13774962

[Sailesh Agrawal]

Created attachment 162922 [details]
Patch

[Sailesh Agrawal]

Created attachment 162927 [details]
Patch

[Sailesh Agrawal]

Created attachment 162928 [details]
Patch

[Sailesh Agrawal]

> After reading the ChangeLog entry, I would've expected this to set the locked alpha to oldTrackAlpha, not 1.
> It looks like the lockedalpha is always either 0 or 1 (right?), so maybe it could just be a bool instead?

Done. Changed to a bool.

> one ; should be enough

Done.

> How do async tests work? Do they fail if finishJSTests() isn't called after a timeout? Or after the event loop doesn't have any events left?

It fails after a timeout.

[Build Bot]

Comment on attachment 162928 [details]
Patch

Attachment 162928 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13795027

[Build Bot]

Comment on attachment 162928 [details]
Patch

Attachment 162928 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13786756

[Sailesh Agrawal]

Created attachment 162965 [details]
Patch

[Build Bot]

Comment on attachment 162965 [details]
Patch

Attachment 162965 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13799287

[Sailesh Agrawal]

Created attachment 162967 [details]
Patch

[Build Bot]

Comment on attachment 162967 [details]
Patch

Attachment 162967 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13792442

[Sailesh Agrawal]

Created attachment 162968 [details]
Patch

[Build Bot]

Comment on attachment 162968 [details]
Patch

Attachment 162968 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13797299

[Sailesh Agrawal]

Created attachment 162970 [details]
Patch

[Sailesh Agrawal]

Hi Beth, could you please take a look. Thanks!

[Beth Dakin]

Comment on attachment 162970 [details]
Patch

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

> Source/WebCore/platform/mac/ScrollbarThemeMac.mm:216
> +    NSNumber* number = objc_getAssociatedObject(painter, &gLockedAlphaKey);

This definitely does not seem like the right approach. If you want to call NSScrollerImp API, you should not use objc_getAssociatedObject/objc_setAssociatedObject. You should call it the same way we call all of the other NSScrollerImp APIs, by declaring it in NSScrollerImpDetails.h

> Source/WebCore/platform/mac/ScrollbarThemeMac.mm:225
> +    objc_setAssociatedObject(painter, &gLockedAlphaKey, [NSNumber numberWithBool:isLocked], OBJC_ASSOCIATION_RETAIN);

Same as above.

[Sailesh Agrawal]

(In reply to comment #25)
> (From update of attachment 162970 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162970&action=review
> 
> > Source/WebCore/platform/mac/ScrollbarThemeMac.mm:216
> > +    NSNumber* number = objc_getAssociatedObject(painter, &gLockedAlphaKey);
> 
> This definitely does not seem like the right approach. If you want to call NSScrollerImp API, you should not use objc_getAssociatedObject/objc_setAssociatedObject. You should call it the same way we call all of the other NSScrollerImp APIs, by declaring it in NSScrollerImpDetails.h

Hi Beth, this actually doesn't call NSScrollerImp APIs. Instead it just associates some data with the instance of NSScrollerImp.

Another way to do this would be to keep a std::map<NSScrollerImp, bool> but using objc_getAssociatedObject() is much more straight forward.

[Beth Dakin]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 162970 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=162970&action=review
> > 
> > > Source/WebCore/platform/mac/ScrollbarThemeMac.mm:216
> > > +    NSNumber* number = objc_getAssociatedObject(painter, &gLockedAlphaKey);
> > 
> > This definitely does not seem like the right approach. If you want to call NSScrollerImp API, you should not use objc_getAssociatedObject/objc_setAssociatedObject. You should call it the same way we call all of the other NSScrollerImp APIs, by declaring it in NSScrollerImpDetails.h
> 
> Hi Beth, this actually doesn't call NSScrollerImp APIs. Instead it just associates some data with the instance of NSScrollerImp.
> 
> Another way to do this would be to keep a std::map<NSScrollerImp, bool> but using objc_getAssociatedObject() is much more straight forward.

Why does the data need to be associated with the NSScrollerImp instead of a WebCore object?

[Sailesh Agrawal]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > (From update of attachment 162970 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=162970&action=review
> > > 
> > > > Source/WebCore/platform/mac/ScrollbarThemeMac.mm:216
> > > > +    NSNumber* number = objc_getAssociatedObject(painter, &gLockedAlphaKey);
> > > 
> > > This definitely does not seem like the right approach. If you want to call NSScrollerImp API, you should not use objc_getAssociatedObject/objc_setAssociatedObject. You should call it the same way we call all of the other NSScrollerImp APIs, by declaring it in NSScrollerImpDetails.h
> > 
> > Hi Beth, this actually doesn't call NSScrollerImp APIs. Instead it just associates some data with the instance of NSScrollerImp.
> > 
> > Another way to do this would be to keep a std::map<NSScrollerImp, bool> but using objc_getAssociatedObject() is much more straight forward.
> 
> Why does the data need to be associated with the NSScrollerImp instead of a WebCore object?

Either one will work.

Associating the data with NSScrollerImp is direct.

Associating the data with ScrollbarThemeClient involves piping a lot of call out to Source/WebKit/*.

[Beth Dakin]

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > (In reply to comment #25)
> > > > (From update of attachment 162970 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=162970&action=review
> > > > 
> > > > > Source/WebCore/platform/mac/ScrollbarThemeMac.mm:216
> > > > > +    NSNumber* number = objc_getAssociatedObject(painter, &gLockedAlphaKey);
> > > > 
> > > > This definitely does not seem like the right approach. If you want to call NSScrollerImp API, you should not use objc_getAssociatedObject/objc_setAssociatedObject. You should call it the same way we call all of the other NSScrollerImp APIs, by declaring it in NSScrollerImpDetails.h
> > > 
> > > Hi Beth, this actually doesn't call NSScrollerImp APIs. Instead it just associates some data with the instance of NSScrollerImp.
> > > 
> > > Another way to do this would be to keep a std::map<NSScrollerImp, bool> but using objc_getAssociatedObject() is much more straight forward.
> > 
> > Why does the data need to be associated with the NSScrollerImp instead of a WebCore object?
> 
> Either one will work.
> 
> Associating the data with NSScrollerImp is direct.
> 
> Associating the data with ScrollbarThemeClient involves piping a lot of call out to Source/WebKit/*.

I really don't like the idea of adding data members to an AppKit object. I think we should stick to WebCore objects here. Can you associate it with Scrollbar instead of ScrollbarThemeClient?

[Sailesh Agrawal]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)
> > > (In reply to comment #26)
> > > > (In reply to comment #25)
> > > > > (From update of attachment 162970 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=162970&action=review
> > > > > 
> > > > > > Source/WebCore/platform/mac/ScrollbarThemeMac.mm:216
> > > > > > +    NSNumber* number = objc_getAssociatedObject(painter, &gLockedAlphaKey);
> > > > > 
> > > > > This definitely does not seem like the right approach. If you want to call NSScrollerImp API, you should not use objc_getAssociatedObject/objc_setAssociatedObject. You should call it the same way we call all of the other NSScrollerImp APIs, by declaring it in NSScrollerImpDetails.h
> > > > 
> > > > Hi Beth, this actually doesn't call NSScrollerImp APIs. Instead it just associates some data with the instance of NSScrollerImp.
> > > > 
> > > > Another way to do this would be to keep a std::map<NSScrollerImp, bool> but using objc_getAssociatedObject() is much more straight forward.
> > > 
> > > Why does the data need to be associated with the NSScrollerImp instead of a WebCore object?
> > 
> > Either one will work.
> > 
> > Associating the data with NSScrollerImp is direct.
> > 
> > Associating the data with ScrollbarThemeClient involves piping a lot of call out to Source/WebKit/*.
> 
> I really don't like the idea of adding data members to an AppKit object. I think we should stick to WebCore objects here. Can you associate it with Scrollbar instead of ScrollbarThemeClient?

Currently ScrollbarThemeMac only has access to ScrollbarThemeClient, which is an abstract class.

Would it be ok to store it in a std::map instead?

[Beth Dakin]

(In reply to comment #30)
> 
> Currently ScrollbarThemeMac only has access to ScrollbarThemeClient, which is an abstract class.
> 
> Would it be ok to store it in a std::map instead?

Can you add a data member to ScrollbarThemeClient?

[Beth Dakin]

(In reply to comment #31)
> (In reply to comment #30)
> > 
> > Currently ScrollbarThemeMac only has access to ScrollbarThemeClient, which is an abstract class.
> > 
> > Would it be ok to store it in a std::map instead?
> 
> Can you add a data member to ScrollbarThemeClient?

Well, I guess you would still be adding the data to Scrollbar, but with pure virtual accessors on ScrollbarThemeClient. Would that work?

[Sailesh Agrawal]

(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #30)
> > > 
> > > Currently ScrollbarThemeMac only has access to ScrollbarThemeClient, which is an abstract class.
> > > 
> > > Would it be ok to store it in a std::map instead?
> > 
> > Can you add a data member to ScrollbarThemeClient?
> 
> Well, I guess you would still be adding the data to Scrollbar, but with pure virtual accessors on ScrollbarThemeClient. Would that work?

Yep, sounds good. I'll give it a try. Thanks!

[Sailesh Agrawal]

Created attachment 163747 [details]
Patch

[Sailesh Agrawal]

(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #30)
> > > 
> > > Currently ScrollbarThemeMac only has access to ScrollbarThemeClient, which is an abstract class.
> > > 
> > > Would it be ok to store it in a std::map instead?
> > 
> > Can you add a data member to ScrollbarThemeClient?
> 
> Well, I guess you would still be adding the data to Scrollbar, but with pure virtual accessors on ScrollbarThemeClient. Would that work?

Done! Please take another look. Thanks.

[Beth Dakin]

Comment on attachment 163747 [details]
Patch

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

Looks good!

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm:138
> +        } else {

You should remove the curly braces for the else since it only has only line.

[Sailesh Agrawal]

Created attachment 164038 [details]
Patch for landing

[Sailesh Agrawal]

(In reply to comment #36)
> (From update of attachment 163747 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163747&action=review
> 
> Looks good!
> 
> > Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm:138
> > +        } else {
> 
> You should remove the curly braces for the else since it only has only line.

Updated patch. Could you c+?

Thanks!

[WebKit Review Bot]

Comment on attachment 164038 [details]
Patch for landing

Rejecting attachment 164038 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ilter
Hunk #1 succeeded at 154 (offset 1 line).
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/scrolling/scrollbar-tickmarks-hittest-expected.txt
patching file LayoutTests/fast/scrolling/scrollbar-tickmarks-hittest.html
patching file ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Beth Dakin']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13844755

[Sailesh Agrawal]

Created attachment 164215 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 164215 [details]
Patch for landing

Attachment 164215 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13852554

[Sailesh Agrawal]

Created attachment 164612 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 164612 [details]
Patch for landing

Attachment 164612 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13892308

[Peter Beverloo (cr-android ews)]

Comment on attachment 164612 [details]
Patch for landing

Attachment 164612 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13900298

[Sailesh Agrawal]

Created attachment 164631 [details]
Patch for landing

[Sailesh Agrawal]

Looks like the changes to CCScrollbarLayerImpl.h requires a 3 sided patch.
+jamesr to confirm

[James Robinson]

I don't think you should need to touch CCScrollbarLayerImpl.  We don't do mouse hit testing on that object at any point.

[Sailesh Agrawal]

Hi Beth, could I get another r+/c+? The try bots are all green now. Thanks!

[WebKit Review Bot]

Comment on attachment 164631 [details]
Patch for landing

Clearing flags on attachment: 164631

Committed r128963: <http://trac.webkit.org/changeset/128963>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.