Updating OnPictureReady() method name per CR feedback

Updating OnPictureReady() method name per CR feedback

BUG=689229
Review-Url: https://codereview.chromium.org/2690943005
Cr-Commit-Position: refs/heads/master@{#450371}
Committed: https://chromium.googlesource.com/chromium/src/+/3aac9a6f4949e33dfe3045326ce6afda6293667d

[joedow, 2017-02-14 00:08:54]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 00:09:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 00:18:19]

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

[commit-bot: I haz the power, 2017-02-14 00:18:19]

Dry run: This issue passed the CQ dry run.

[joedow, 2017-02-14 00:24:47]

joedow@chromium.org changed reviewers:
+ wez@chromium.org

[joedow, 2017-02-14 00:24:48]

PTAL!

https://codereview.chromium.org/2690943005/diff/1/remoting/client/plugin/pepp...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/2690943005/diff/1/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_video_renderer_3d.cc:33: }  // namespace
This felt like it should be in an anonymous namespace so I wrapped it in this
CL.

https://codereview.chromium.org/2690943005/diff/1/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_video_renderer_3d.cc:337: void
PepperVideoRenderer3D::GetNextPictureIfReady() {
Wez's comment about only needing the |decoded_frames_| check in OnPictureReady()
is correct, however it is really easy to call this function and not know about
this insidious and difficult to repro bug.  I would prefer to make all function
calls safe and update the method name.

[Wez, 2017-02-14 00:31:25]

nit: Want to add the BUG number to the description?

[Wez, 2017-02-14 00:33:00]

lgtm

https://codereview.chromium.org/2690943005/diff/1/remoting/client/plugin/pepp...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/2690943005/diff/1/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_video_renderer_3d.cc:337: void
PepperVideoRenderer3D::GetNextPictureIfReady() {
On 2017/02/14 00:24:47, joedow wrote:
> Wez's comment about only needing the |decoded_frames_| check in
OnPictureReady()
> is correct, however it is really easy to call this function and not know about
> this insidious and difficult to repro bug.  I would prefer to make all
function
> calls safe and update the method name.

The alternative would be to DCHECK(!decoded_frames_.empty()) here. :) (Which
raises the question of whether there is any way to mock out slow-decoder
behaviour to create a unit-test for this logic. ;)

[joedow, 2017-02-14 16:01:34]

Thanks!

https://codereview.chromium.org/2690943005/diff/1/remoting/client/plugin/pepp...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/2690943005/diff/1/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_video_renderer_3d.cc:337: void
PepperVideoRenderer3D::GetNextPictureIfReady() {
On 2017/02/14 00:33:00, Wez wrote:
> On 2017/02/14 00:24:47, joedow wrote:
> > Wez's comment about only needing the |decoded_frames_| check in
> OnPictureReady()
> > is correct, however it is really easy to call this function and not know
about
> > this insidious and difficult to repro bug.  I would prefer to make all
> function
> > calls safe and update the method name.
> 
> The alternative would be to DCHECK(!decoded_frames_.empty()) here. :) (Which
> raises the question of whether there is any way to mock out slow-decoder
> behaviour to create a unit-test for this logic. ;)

The unit test is a fair point (as was the suggested of refactoring we had
offline).  I am unsure how much longer the plugin code is going to  live though
so I am hesitant to put more effort into it than fixing critical bugs.

[joedow, 2017-02-14 16:02:19]

Description was changed from

==========
Updating OnPictureReady() method name per CR feedback

BUG=
==========

to

==========
Updating OnPictureReady() method name per CR feedback

BUG=689229
==========

[joedow, 2017-02-14 16:02:41]

The CQ bit was checked by joedow@chromium.org

[commit-bot: I haz the power, 2017-02-14 16:03:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 16:08:28]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1487088161405200, "parent_rev":
"d892f9592860691ae9a782c12260c94ed6bd1a63", "commit_rev":
"3aac9a6f4949e33dfe3045326ce6afda6293667d"}

[commit-bot: I haz the power, 2017-02-14 16:09:03]

Description was changed from

==========
Updating OnPictureReady() method name per CR feedback

BUG=689229
==========

to

==========
Updating OnPictureReady() method name per CR feedback

BUG=689229

Review-Url: https://codereview.chromium.org/2690943005
Cr-Commit-Position: refs/heads/master@{#450371}
Committed:
https://chromium.googlesource.com/chromium/src/+/3aac9a6f4949e33dfe3045326ce6...
==========

[commit-bot: I haz the power, 2017-02-14 16:09:05]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/3aac9a6f4949e33dfe3045326ce6...