Have Blink be told of scrollbar changes rather than paying attention itself (Blink side).

Have Blink be told of scrollbar changes rather than paying attention itself (Blink side).

BUG=306348
TEST=no visible change; scrollbar changes still work

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

[Avi (use Gerrit), 2013-11-03 23:26:56]

Why is win_blink_rel trying to compile mac/WebScrollbarTheme.cpp?

[tfarina, 2013-11-04 00:08:33]

On Sun, Nov 3, 2013 at 9:26 PM,  <avi@chromium.org> wrote:
> Why is win_blink_rel trying to compile mac/WebScrollbarTheme.cpp?
>
Because it doesn't end with Mac.cpp or .mm I think. And there isn't a
rule to exclude files in mac/ directories I guess. You may want to fix
this in patch though.

> https://codereview.chromium.org/57463002/



-- 
Thiago

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[abarth-chromium, 2013-11-04 00:43:23]

LGTM w/ nit

https://codereview.chromium.org/57463002/diff/70001/Source/core/platform/Scro...
File Source/core/platform/ScrollbarThemeMacCommon.h (right):

https://codereview.chromium.org/57463002/diff/70001/Source/core/platform/Scro...
Source/core/platform/ScrollbarThemeMacCommon.h:40: void
preferencesChanged(float, float, bool, bool);
Would you be willing to name these parameters?  It's not clear what they
represent.

https://codereview.chromium.org/57463002/diff/70001/Source/web/mac/WebScrollb...
File Source/web/mac/WebScrollbarTheme.cpp (right):

https://codereview.chromium.org/57463002/diff/70001/Source/web/mac/WebScrollb...
Source/web/mac/WebScrollbarTheme.cpp:44:
static_cast<ScrollbarThemeMacCommon*>(ScrollbarTheme::theme())->preferencesChanged(
Is there some way to DCHECK that this case is valid?

[Avi (use Gerrit), 2013-11-04 01:40:24]

https://codereview.chromium.org/57463002/diff/70001/Source/core/platform/Scro...
File Source/core/platform/ScrollbarThemeMacCommon.h (right):

https://codereview.chromium.org/57463002/diff/70001/Source/core/platform/Scro...
Source/core/platform/ScrollbarThemeMacCommon.h:40: void
preferencesChanged(float, float, bool, bool);
On 2013/11/04 00:43:24, abarth wrote:
> Would you be willing to name these parameters?  It's not clear what they
> represent.

Of course I'm willing. I'm writing to fit in with the existing style, but if you
ask for it, you get it.

https://codereview.chromium.org/57463002/diff/70001/Source/web/mac/WebScrollb...
File Source/web/mac/WebScrollbarTheme.cpp (right):

https://codereview.chromium.org/57463002/diff/70001/Source/web/mac/WebScrollb...
Source/web/mac/WebScrollbarTheme.cpp:44:
static_cast<ScrollbarThemeMacCommon*>(ScrollbarTheme::theme())->preferencesChanged(
On 2013/11/04 00:43:24, abarth wrote:
> Is there some way to DCHECK that this case is valid?

I suppose that's a good point. I'm not sure what the lifetime of ScrollbarTheme
is...

[abarth-chromium, 2013-11-04 01:41:54]

https://codereview.chromium.org/57463002/diff/70001/Source/core/platform/Scro...
File Source/core/platform/ScrollbarThemeMacCommon.h (right):

https://codereview.chromium.org/57463002/diff/70001/Source/core/platform/Scro...
Source/core/platform/ScrollbarThemeMacCommon.h:40: void
preferencesChanged(float, float, bool, bool);
On 2013/11/04 01:40:24, Avi wrote:
> On 2013/11/04 00:43:24, abarth wrote:
> > Would you be willing to name these parameters?  It's not clear what they
> > represent.
> 
> Of course I'm willing. I'm writing to fit in with the existing style, but if
you
> ask for it, you get it.

The existing style is to omit parameter names when they don't add any additional
information.  For example, naming the GraphicsContext* parameters below
|context| wouldn't convey any additional information.

[Avi (use Gerrit), 2013-11-04 16:25:45]

abarth, please take a look as I addressed your issue though I'm not sure if
you'll be OK with how.

https://codereview.chromium.org/57463002/diff/70001/Source/web/mac/WebScrollb...
File Source/web/mac/WebScrollbarTheme.cpp (right):

https://codereview.chromium.org/57463002/diff/70001/Source/web/mac/WebScrollb...
Source/web/mac/WebScrollbarTheme.cpp:44:
static_cast<ScrollbarThemeMacCommon*>(ScrollbarTheme::theme())->preferencesChanged(
On 2013/11/04 01:40:24, Avi wrote:
> On 2013/11/04 00:43:24, abarth wrote:
> > Is there some way to DCHECK that this case is valid?
> 
> I suppose that's a good point. I'm not sure what the lifetime of
ScrollbarTheme
> is...

Ooooh. Did you mean if this CAST is valid? Does the #if help here?

[abarth-chromium, 2013-11-04 17:19:40]

LGTM w/o the ifdef

https://codereview.chromium.org/57463002/diff/150001/public/web/mac/WebScroll...
File public/web/mac/WebScrollbarTheme.h (right):

https://codereview.chromium.org/57463002/diff/150001/public/web/mac/WebScroll...
public/web/mac/WebScrollbarTheme.h:40: #if OS(MACOSX)
Does this work in this header?  I don't think this will work when you include
this header in a Chromium file.

[Avi (use Gerrit), 2013-11-04 17:20:50]

https://codereview.chromium.org/57463002/diff/150001/public/web/mac/WebScroll...
File public/web/mac/WebScrollbarTheme.h (right):

https://codereview.chromium.org/57463002/diff/150001/public/web/mac/WebScroll...
public/web/mac/WebScrollbarTheme.h:40: #if OS(MACOSX)
On 2013/11/04 17:19:40, abarth wrote:
> Does this work in this header?  I don't think this will work when you include
> this header in a Chromium file.

Mmmm. Good point.

[commit-bot: I haz the power, 2013-11-04 17:23:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/57463002/220001

[commit-bot: I haz the power, 2013-11-04 19:19:06]

Change committed as 161269