Invalidate scrollbars when window activity changes if needed

Invalidate scrollbars when window activity changes if needed

Non-overlay Mac scrollbars have different appearances for active vs inactive
windows, so pass activity changes down to Scrollbar, and have Scrollbar
invalidate itself if the ScrollbarTheme calls for it.

BUG=535984
Committed: https://crrev.com/d0a6f1fa930ce84598393a735c50a6cfc4e4e70b
Cr-Commit-Position: refs/heads/master@{#362630}

[Elly Fong-Jones, 2015-10-28 12:44:05]

Description was changed from

==========
Invalidate scrollbars when window activity changes if needed

Non-overlay Mac scrollbars have different appearances for active vs inactive
windows, so pass activity changes down to Scrollbar, and have Scrollbar
invalidate itself if the ScrollbarTheme calls for it.

BUG=535984
==========

to

==========
Invalidate scrollbars when window activity changes if needed

Non-overlay Mac scrollbars have different appearances for active vs inactive
windows, so pass activity changes down to Scrollbar, and have Scrollbar
invalidate itself if the ScrollbarTheme calls for it.

BUG=535984
==========

[Elly Fong-Jones, 2015-10-28 12:44:05]

ellyjones@chromium.org changed reviewers:
+ dcheng@chromium.org, thakis@chromium.org

[Elly Fong-Jones, 2015-10-28 12:44:13]

dcheng, thakis: PTAL? :)

[dcheng, 2015-10-28 17:12:23]

dcheng@chromium.org changed reviewers:
+ esprehn@chromium.org
- dcheng@chromium.org

[dcheng, 2015-10-28 17:12:24]

The changes seem reasonable to me, but this is not an area of expertise for me.

+esprehn to replace me

[dcheng, 2015-10-28 17:12:41]

Also... any chance of adding a layout test?

[Elly Fong-Jones, 2015-10-28 17:59:56]

ellyjones@chromium.org changed reviewers:
+ cbiesinger@chromium.org

[ojan, 2015-10-29 00:25:25]

ojan@chromium.org changed reviewers:
+ ojan@chromium.org

[ojan, 2015-10-29 00:25:25]

https://codereview.chromium.org/1424983002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scroll/ScrollbarThemeMacNonOverlayAPI.h
(right):

https://codereview.chromium.org/1424983002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollbarThemeMacNonOverlayAPI.h:44:
bool invalidateOnWindowActiveChange() const override { return true; }
This is just for mac 10.6 right? If so, we should only invalidate on 10.6.

[Elly Fong-Jones, 2015-10-29 15:12:40]

https://codereview.chromium.org/1424983002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scroll/ScrollbarThemeMacNonOverlayAPI.h
(right):

https://codereview.chromium.org/1424983002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollbarThemeMacNonOverlayAPI.h:44:
bool invalidateOnWindowActiveChange() const override { return true; }
On 2015/10/29 00:25:25, ojan wrote:
> This is just for mac 10.6 right? If so, we should only invalidate on 10.6.

It is, yes. The non-overlay API is the 10.6 API; in 10.7 and later we use the
overlay API instead.

[ojan, 2015-11-30 19:28:58]

Elly and I chatted offline. She clarified that ScrollbarThemeMacNonOverlayAPI
should get deleted entirely when we stop supporting 10.6. So, this patch is
implicitly only changing 10.6 behavior. Given that, I'm OK with shoe-horning in
this temporary solution and believe that it's probably not worth the effort to
figure out how to test this since it's a bit tricky to test.

The thing I would like changed is that the generic function names in generic
code make it clear that this is 10.6 only code and has FIXMEs to delete it when
we stop supporting 10.6.

I'm not attached to the specific names except that they should sound bad and
like something people shouldn't use, e.g. windowActiveChangedForMac10_6Only.
It's OK for them to be verbose as that will further discourage their use.

The reason I'm being so paranoid about this is that invalidations are really
expensive and we've had issues in the past of scrollbars over-invalidating on
all platforms specifically to implement this behavior.

[Elly Fong-Jones, 2015-12-01 13:53:08]

On 2015/11/30 19:28:58, ojan wrote:
> Elly and I chatted offline. She clarified that ScrollbarThemeMacNonOverlayAPI
> should get deleted entirely when we stop supporting 10.6. So, this patch is
> implicitly only changing 10.6 behavior. Given that, I'm OK with shoe-horning
in
> this temporary solution and believe that it's probably not worth the effort to
> figure out how to test this since it's a bit tricky to test.
> 
> The thing I would like changed is that the generic function names in generic
> code make it clear that this is 10.6 only code and has FIXMEs to delete it
when
> we stop supporting 10.6.
> 
> I'm not attached to the specific names except that they should sound bad and
> like something people shouldn't use, e.g. windowActiveChangedForMac10_6Only.
> It's OK for them to be verbose as that will further discourage their use.

This is done; it is now called windowActiveChangedForSnowLeopardOnly().
I also guarded the body with #if OS(MAC), and added an assertion that the
invalidate only happens on Snow Leopard.

> The reason I'm being so paranoid about this is that invalidations are really
> expensive and we've had issues in the past of scrollbars over-invalidating on
> all platforms specifically to implement this behavior.

[ojan, 2015-12-02 01:47:00]

The CQ bit was checked by ojan@chromium.org

[ojan, 2015-12-02 01:47:01]

lgtm

[ojan, 2015-12-02 01:48:10]

Hmmm...I swear I had added paint people to this code review in my first response
on this review. readding in case they have any concerns.

[commit-bot: I haz the power, 2015-12-02 01:48:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1424983002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1424983002/20001

[chrishtr, 2015-12-02 02:01:57]

chrishtr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[chrishtr, 2015-12-02 02:01:58]

Looks cool to me. Thanks for looping me in.

[commit-bot: I haz the power, 2015-12-02 02:49:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-02 02:49:30]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[esprehn, 2015-12-02 03:30:07]

The CQ bit was checked by esprehn@chromium.org

[esprehn, 2015-12-02 03:30:08]

lgtm

[commit-bot: I haz the power, 2015-12-02 03:30:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1424983002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1424983002/20001

[commit-bot: I haz the power, 2015-12-02 04:27:39]

Description was changed from

==========
Invalidate scrollbars when window activity changes if needed

Non-overlay Mac scrollbars have different appearances for active vs inactive
windows, so pass activity changes down to Scrollbar, and have Scrollbar
invalidate itself if the ScrollbarTheme calls for it.

BUG=535984
==========

to

==========
Invalidate scrollbars when window activity changes if needed

Non-overlay Mac scrollbars have different appearances for active vs inactive
windows, so pass activity changes down to Scrollbar, and have Scrollbar
invalidate itself if the ScrollbarTheme calls for it.

BUG=535984
==========

[commit-bot: I haz the power, 2015-12-02 04:27:39]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-02 04:28:17]

Description was changed from

==========
Invalidate scrollbars when window activity changes if needed

Non-overlay Mac scrollbars have different appearances for active vs inactive
windows, so pass activity changes down to Scrollbar, and have Scrollbar
invalidate itself if the ScrollbarTheme calls for it.

BUG=535984
==========

to

==========
Invalidate scrollbars when window activity changes if needed

Non-overlay Mac scrollbars have different appearances for active vs inactive
windows, so pass activity changes down to Scrollbar, and have Scrollbar
invalidate itself if the ScrollbarTheme calls for it.

BUG=535984

Committed: https://crrev.com/d0a6f1fa930ce84598393a735c50a6cfc4e4e70b
Cr-Commit-Position: refs/heads/master@{#362630}
==========

[commit-bot: I haz the power, 2015-12-02 04:28:19]

Patchset 2 (id:??) landed as
https://crrev.com/d0a6f1fa930ce84598393a735c50a6cfc4e4e70b
Cr-Commit-Position: refs/heads/master@{#362630}