make BeginFrameSourceMixIn's SetNeedsBeginFrame final

make BeginFrameSourceMixIn's SetNeedsBeginFrame final

BUG=429391
Committed: https://crrev.com/73b8251aab80465102178d00f6674d4316a188fc
Cr-Commit-Position: refs/heads/master@{#314516}

[Jimmy Jo, 2015-02-03 11:16:27]

jincheol.jo@navercorp.com changed reviewers:
+ brianderson@chromium.org, simonhong@chromium.org

[Jimmy Jo, 2015-02-03 11:34:15]

Patchset #1 (id:1) has been deleted

[Jimmy Jo, 2015-02-03 11:35:03]

PTAL!!

[danakj, 2015-02-03 17:32:24]

On Tue, Feb 3, 2015 at 3:35 AM, <jincheol.jo@navercorp.com> wrote:

> Reviewers: brianderson, simonhong,
>
> Message:
> PTAL!!
>
> Description:
> make BeginFrameSourceMixIn's SetNeedsBeginFrame final
>

do you measure some perf win from doing this? or what is the motivation?

>
> BUG=429391
>
> Please review this at https://codereview.chromium.org/891953003/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+5, -3 lines):
>   M cc/scheduler/begin_frame_source.h
>   M cc/scheduler/begin_frame_source.cc
>
>
> Index: cc/scheduler/begin_frame_source.cc
> diff --git a/cc/scheduler/begin_frame_source.cc
> b/cc/scheduler/begin_frame_source.cc
> index 8ef3b6798bbbb7cd04d76b48ce4da3c2635bb4c4..
> d2723df357968d4e90baf99aa1eeafdc2614eae4 100644
> --- a/cc/scheduler/begin_frame_source.cc
> +++ b/cc/scheduler/begin_frame_source.cc
> @@ -414,7 +414,8 @@ bool BeginFrameSourceMultiplexer::NeedsBeginFrames()
> const {
>    }
>  }
>
> -void BeginFrameSourceMultiplexer::SetNeedsBeginFrames(bool
> needs_begin_frames) {
> +void BeginFrameSourceMultiplexer::OnNeedsBeginFramesChange(
> +    bool needs_begin_frames) {
>    DEBUG_FRAMES("BeginFrameSourceMultiplexer::SetNeedsBeginFrames",
>                 "active_source",
>                 active_source_,
> Index: cc/scheduler/begin_frame_source.h
> diff --git a/cc/scheduler/begin_frame_source.h b/cc/scheduler/begin_frame_
> source.h
> index d3dd9de2659f0d8db53c2a42563372a251f5313a..
> fbf4dbf96948126ef15c1b3d2fa1afe7cd6462b8 100644
> --- a/cc/scheduler/begin_frame_source.h
> +++ b/cc/scheduler/begin_frame_source.h
> @@ -138,7 +138,7 @@ class CC_EXPORT BeginFrameSourceMixIn : public
> BeginFrameSource {
>
>    // BeginFrameSource
>    bool NeedsBeginFrames() const override;
> -  void SetNeedsBeginFrames(bool needs_begin_frames) override;
> +  void SetNeedsBeginFrames(bool needs_begin_frames) final;
>    void DidFinishFrame(size_t remaining_frames) override {}
>    void AddObserver(BeginFrameObserver* obs) final;
>    void RemoveObserver(BeginFrameObserver* obs) final;
> @@ -261,7 +261,8 @@ class CC_EXPORT BeginFrameSourceMultiplexer : public
> BeginFrameSourceMixIn,
>
>    // BeginFrameSource
>    bool NeedsBeginFrames() const override;
> -  void SetNeedsBeginFrames(bool needs_begin_frames) override;
> +  // BeginFrameSourceMixIn
> +  void OnNeedsBeginFramesChange(bool needs_begin_frames) override;
>    void DidFinishFrame(size_t remaining_frames) override;
>
>    // Tracing
>
>
>

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

[simonhong, 2015-02-03 19:37:55]

@danakj, BeginFrameSourceMixIn provides OnNeedsBeginFramesChange() interface for
sub-class's additional behaviour.This confuses which one should be used when
sub-class overrides. So, this cl would clarify that OnNeedsBeginFramesChange()
should be used.

https://codereview.chromium.org/891953003/diff/20001/cc/scheduler/begin_frame...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/891953003/diff/20001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_source.cc:419:
DEBUG_FRAMES("BeginFrameSourceMultiplexer::SetNeedsBeginFrames",
SetNeedsBeginFrames -> OnNeedsBeginFramesChange.

https://codereview.chromium.org/891953003/diff/20001/cc/scheduler/begin_frame...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/891953003/diff/20001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_source.h:265: void OnNeedsBeginFramesChange(bool
needs_begin_frames) override;
Below DidFinishFrame(...) overrides BeginFrameSource::DidFinishFrame(...).
So line 264, 265 should be moved to other place.

[Jimmy Jo, 2015-02-04 01:46:47]

All Done.
Thank Simon for detailed review.

https://codereview.chromium.org/891953003/diff/20001/cc/scheduler/begin_frame...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/891953003/diff/20001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_source.cc:419:
DEBUG_FRAMES("BeginFrameSourceMultiplexer::SetNeedsBeginFrames",
On 2015/02/03 19:37:55, simonhong wrote:
> SetNeedsBeginFrames -> OnNeedsBeginFramesChange.

Done.

https://codereview.chromium.org/891953003/diff/20001/cc/scheduler/begin_frame...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/891953003/diff/20001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_source.h:265: void OnNeedsBeginFramesChange(bool
needs_begin_frames) override;
On 2015/02/03 19:37:55, simonhong wrote:
> Below DidFinishFrame(...) overrides BeginFrameSource::DidFinishFrame(...).
> So line 264, 265 should be moved to other place.

Done.

[brianderson, 2015-02-04 05:29:03]

Thanks for the cleanup. lgtm.

+mithro as fyi

[simonhong, 2015-02-04 06:14:20]

On 2015/02/04 01:46:47, Jincheol Jo wrote:
> All Done.
> Thank Simon for detailed review.
> 
>
https://codereview.chromium.org/891953003/diff/20001/cc/scheduler/begin_frame...
> File cc/scheduler/begin_frame_source.cc (right):
> 
>
https://codereview.chromium.org/891953003/diff/20001/cc/scheduler/begin_frame...
> cc/scheduler/begin_frame_source.cc:419:
> DEBUG_FRAMES("BeginFrameSourceMultiplexer::SetNeedsBeginFrames",
> On 2015/02/03 19:37:55, simonhong wrote:
> > SetNeedsBeginFrames -> OnNeedsBeginFramesChange.
> 
> Done.
> 
>
https://codereview.chromium.org/891953003/diff/20001/cc/scheduler/begin_frame...
> File cc/scheduler/begin_frame_source.h (right):
> 
>
https://codereview.chromium.org/891953003/diff/20001/cc/scheduler/begin_frame...
> cc/scheduler/begin_frame_source.h:265: void OnNeedsBeginFramesChange(bool
> needs_begin_frames) override;
> On 2015/02/03 19:37:55, simonhong wrote:
> > Below DidFinishFrame(...) overrides BeginFrameSource::DidFinishFrame(...).
> > So line 264, 265 should be moved to other place.
> 
> Done.

Thanks jincheol.jo! lgtm :)

[Jimmy Jo, 2015-02-04 06:17:01]

The CQ bit was checked by jincheol.jo@navercorp.com

[commit-bot: I haz the power, 2015-02-04 06:17:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/891953003/40001

[commit-bot: I haz the power, 2015-02-04 07:24:12]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-02-04 07:25:02]

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