Split ScrollbarThemeMac into Overlay and NonOverlay subclasses

Split ScrollbarThemeMac into Overlay and NonOverlay subclasses

This is the first of two steps. The second will move the new
subclasses into their own files.

Move the static variables to be scoped to the classes that access
them (Common, Overlay, or NonOverlay). This separation is fairly
simple, with the exception of gButtonPlacement which (less
obviously) is always ScrollbarButtonsNone for Overlay (and as such
can be made NonOverlay-only.

Duplicate scrollbarMap to scrollbarPainterMap -- this served
two functions before (1) enumerating all scrollbar clients so
that their properties could be updated and (2) mapping overlay
scrollbar clients to their ScrollbarPainter. Now use two separate
structures for the two separate functions.

BUG=133093
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155296

[ccameron, 2013-07-31 02:10:25]

This is the first of two scrollbar-technical-debt changes. The goal with this
one is to have minimal diffs so that correctness can easily be checked.

The second change in the sequence moves the functions into separate files (which
is purely cut-and-paste, though the diffs are huge).

Second part is at
https://codereview.chromium.org/21108004/

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
File Source/core/platform/mac/ScrollbarThemeMac.mm (right):

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
Source/core/platform/mac/ScrollbarThemeMac.mm:64: static ScrollbarMap*
scrollbarMap()
scrollbarMap contains all scrollbars (so that they can be updated in
appearancePrefsChanged)

The new scollbarPainterMap contains only the overlay scrollbars and is used to
look up the ScrollbarPainter for a given ScrollbarClient.

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
Source/core/platform/mac/ScrollbarThemeMac.mm:668: return IntRect();
Note here that for Overlay, buttonsPlacement() is always ScrollbarButtonsNone,
so the combined version would always return an empty rect.

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
Source/core/platform/mac/ScrollbarThemeMac.mm:752: return
scrollbar->frameRect();
Likewise, hasButtons in the full verison would always return true for Overlay.

[enne (OOO), 2013-07-31 18:23:58]

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
File Source/core/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
Source/core/platform/mac/ScrollAnimatorMac.mm:69: return
!scrollbarTheme->isMockTheme() ?
static_cast<ScrollbarThemeMacOverlay*>(scrollbarTheme) : 0;
So, I *think* this is only for overlay scrollbars, but am not 100% sure.  Do you
have more confidence that this is the case?

Should there be some ASSERT here that the overlay API is supported?

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
File Source/core/platform/mac/ScrollbarThemeMac.mm (right):

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
Source/core/platform/mac/ScrollbarThemeMac.mm:136: int
ScrollbarThemeMacNonOverlay::cThumbMinLength[ScrollbarControlSizeCount] = { 26,
20 };
Can you keep these as non-class statics? Is there a need for them to be exposed
outside of one compilation unit?

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
Source/core/platform/mac/ScrollbarThemeMac.mm:156: if
(isScrollbarOverlayAPIAvailable())
No need to check this here, since this is a non overlay function?

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
Source/core/platform/mac/ScrollbarThemeMac.mm:583: return
recommendedScrollerStyle() == NSScrollerStyleOverlay;
Here's where I have a question about naming.  My scrollbar knowledge is a little
sketchy, but I think you can have the overlay API available, but not be using
overlay scrollbars (if system preferences are set to always show scrollbars). 
If so, maybe ScrollbarThemeMacOverlay needs a better name, but I'm not sure what
it would be.

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
Source/core/platform/mac/ScrollbarThemeMac.mm:668: return IntRect();
On 2013/07/31 02:10:25, ccameron1 wrote:
> Note here that for Overlay, buttonsPlacement() is always ScrollbarButtonsNone,
> so the combined version would always return an empty rect.

Can this ASSERT that's the case too as documentation of that comment?

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
Source/core/platform/mac/ScrollbarThemeMac.mm:752: return
scrollbar->frameRect();
On 2013/07/31 02:10:25, ccameron1 wrote:
> Likewise, hasButtons in the full verison would always return true for Overlay.

You mean false?

[ccameron, 2013-07-31 20:26:57]

Thanks!!

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
File Source/core/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
Source/core/platform/mac/ScrollAnimatorMac.mm:69: return
!scrollbarTheme->isMockTheme() ?
static_cast<ScrollbarThemeMacOverlay*>(scrollbarTheme) : 0;
On 2013/07/31 18:23:59, enne wrote:
> So, I *think* this is only for overlay scrollbars, but am not 100% sure.  Do
you
> have more confidence that this is the case?
> 
> Should there be some ASSERT here that the overlay API is supported?

Yes, there should be an ASSERT added. Done.

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
File Source/core/platform/mac/ScrollbarThemeMac.mm (right):

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
Source/core/platform/mac/ScrollbarThemeMac.mm:136: int
ScrollbarThemeMacNonOverlay::cThumbMinLength[ScrollbarControlSizeCount] = { 26,
20 };
On 2013/07/31 18:23:59, enne wrote:
> Can you keep these as non-class statics? Is there a need for them to be
exposed
> outside of one compilation unit?

I sort-of liked them being scoped to be private to the only class that can
access them... but it does make for a lot of extra changes in various places, so
I'll leave them as they were.

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
Source/core/platform/mac/ScrollbarThemeMac.mm:156: if
(isScrollbarOverlayAPIAvailable())
On 2013/07/31 18:23:59, enne wrote:
> No need to check this here, since this is a non overlay function?

Oops -- yes, it's dead code -- removed (hmm, I'd removed it locally -- looks
like I forgot to upload the new version).

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
Source/core/platform/mac/ScrollbarThemeMac.mm:583: return
recommendedScrollerStyle() == NSScrollerStyleOverlay;
On 2013/07/31 18:23:59, enne wrote:
> Here's where I have a question about naming.  My scrollbar knowledge is a
little
> sketchy, but I think you can have the overlay API available, but not be using
> overlay scrollbars (if system preferences are set to always show scrollbars). 
> If so, maybe ScrollbarThemeMacOverlay needs a better name, but I'm not sure
what
> it would be.

Yes -- that's it exactly. Maybe ScrollbarThemeMac(Non)OverlayAPI would show this
without being overly verbose?

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
Source/core/platform/mac/ScrollbarThemeMac.mm:668: return IntRect();
On 2013/07/31 18:23:59, enne wrote:
> On 2013/07/31 02:10:25, ccameron1 wrote:
> > Note here that for Overlay, buttonsPlacement() is always
ScrollbarButtonsNone,
> > so the combined version would always return an empty rect.
> 
> Can this ASSERT that's the case too as documentation of that comment?

Good call. Done.

https://codereview.chromium.org/21296003/diff/5001/Source/core/platform/mac/S...
Source/core/platform/mac/ScrollbarThemeMac.mm:752: return
scrollbar->frameRect();
On 2013/07/31 18:23:59, enne wrote:
> On 2013/07/31 02:10:25, ccameron1 wrote:
> > Likewise, hasButtons in the full verison would always return true for
Overlay.
> 
> You mean false?

Yes (!hasButtons...) added an assert to be more explicit.

[enne (OOO), 2013-07-31 20:55:34]

The new names seem reasonable to me.  I can't think of any better way to refer
to them.

This patch lgtm in general, with a few nits:

https://codereview.chromium.org/21296003/diff/11001/Source/core/platform/mac/...
File Source/core/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/21296003/diff/11001/Source/core/platform/mac/...
Source/core/platform/mac/ScrollAnimatorMac.mm:68:
ASSERT(isScrollbarOverlayAPIAvailable());
On second thought since this is a cast, how about changing ASSERT to
RELEASE_ASSERT_WITH_SECURITY_IMPLICATION?

https://codereview.chromium.org/21296003/diff/11001/Source/core/platform/mac/...
File Source/core/platform/mac/ScrollbarThemeMac.mm (right):

https://codereview.chromium.org/21296003/diff/11001/Source/core/platform/mac/...
Source/core/platform/mac/ScrollbarThemeMac.mm:132: int cRealButtonLength[] = {
28, 21 };
Sorry, I didn't mean make them not static; I just meant don't make them
class-level static.  I want these variables to be restricted to just one
compilation unit and not have external linkage.

https://codereview.chromium.org/21296003/diff/11001/Source/core/platform/mac/...
Source/core/platform/mac/ScrollbarThemeMac.mm:649: ASSERT(gButtonPlacement !=
ScrollbarButtonsNone);
This could be ASSERT_NE while you're here.

https://codereview.chromium.org/21296003/diff/11001/Source/core/platform/mac/...
Source/core/platform/mac/ScrollbarThemeMac.mm:667: ASSERT(buttonsPlacement() ==
ScrollbarButtonsNone);
ASSERT => ASSERT_EQ

https://codereview.chromium.org/21296003/diff/11001/Source/core/platform/mac/...
Source/core/platform/mac/ScrollbarThemeMac.mm:707: ASSERT(buttonsPlacement() ==
ScrollbarButtonsNone);
ASSERT => ASSERT_EQ

[ccameron, 2013-07-31 21:43:50]

Thanks!!

https://codereview.chromium.org/21296003/diff/11001/Source/core/platform/mac/...
File Source/core/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/21296003/diff/11001/Source/core/platform/mac/...
Source/core/platform/mac/ScrollAnimatorMac.mm:68:
ASSERT(isScrollbarOverlayAPIAvailable());
On 2013/07/31 20:55:34, enne wrote:
> On second thought since this is a cast, how about changing ASSERT to
> RELEASE_ASSERT_WITH_SECURITY_IMPLICATION?

Done.

https://codereview.chromium.org/21296003/diff/11001/Source/core/platform/mac/...
File Source/core/platform/mac/ScrollbarThemeMac.mm (right):

https://codereview.chromium.org/21296003/diff/11001/Source/core/platform/mac/...
Source/core/platform/mac/ScrollbarThemeMac.mm:132: int cRealButtonLength[] = {
28, 21 };
On 2013/07/31 20:55:34, enne wrote:
> Sorry, I didn't mean make them not static; I just meant don't make them
> class-level static.  I want these variables to be restricted to just one
> compilation unit and not have external linkage.

Oh -- didn't mean to remove the static (it got eaten by accident).

https://codereview.chromium.org/21296003/diff/11001/Source/core/platform/mac/...
Source/core/platform/mac/ScrollbarThemeMac.mm:649: ASSERT(gButtonPlacement !=
ScrollbarButtonsNone);
On 2013/07/31 20:55:34, enne wrote:
> This could be ASSERT_NE while you're here.

Looks like those are only available in gtest, not in regular WebKit code.

[commit-bot: I haz the power, 2013-08-01 00:21:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/21296003/22001

[commit-bot: I haz the power, 2013-08-01 03:53:51]

Change committed as 155296