Remove unnecessary refreshCastButtonVisibility call

Remove unnecessary refreshCastButtonVisiblity call

computeWhichControlsFit is being called on every mousemove event on a video
(via a call to refreshCastButtonVisiblity). After discussing offline with
mlamouri@ and avayvod@, it was determined that the call to
refreshCastButtonVisiblity is no longer needed, and therefore can be removed.

In addition, this CL refactors some media layout tests that test the cast button visibility, and appends to an existing cast button visibility test (controls-cast-button-narrow.html) to test for correct visibility after expanding and contracting a video.

BUG=639273
Review-Url: https://codereview.chromium.org/2684973004
Cr-Commit-Position: refs/heads/master@{#450524}
Committed: https://chromium.googlesource.com/chromium/src/+/6fb1451de0171f8bdd0915c22016750d79c0b3ac

[steimel, 2017-02-08 23:56:18]

steimel@chromium.org changed reviewers:
+ avayvod@chromium.org, mlamouri@chromium.org

[steimel, 2017-02-08 23:56:18]

PTAL

[steimel, 2017-02-08 23:56:29]

Description was changed from

==========
Remove unnecessary refreshCastButtonVisiblity call

computeWhichControlsFit is being called on every mousemove event on a video
(via a call to refreshCastButtonVisiblity). After discussing offline with
mlamouri@ and avayvod@, it was determined that the call to
refreshCastButtonVisiblity is no longer needed, and therefore can be removed.

BUG=639273
==========

to

==========
Remove unnecessary refreshCastButtonVisiblity call

computeWhichControlsFit is being called on every mousemove event on a video
(via a call to refreshCastButtonVisiblity). After discussing offline with
mlamouri@ and avayvod@, it was determined that the call to
refreshCastButtonVisiblity is no longer needed, and therefore can be removed.

BUG=639273
==========

[steimel, 2017-02-08 23:56:41]

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

[commit-bot: I haz the power, 2017-02-08 23:57:28]

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-09 06:08:39]

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

[commit-bot: I haz the power, 2017-02-09 06:08:39]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2017-02-10 14:12:17]

Could you write a LayoutTest for this to test that the reason why this call was
there is actually not broken. You can find an example in here:
third_party/WebKit/LayoutTests/media/controls-cast-button-narrow.html

(though, better to add the test in media/controls/, there are too many random
various tests in media/.)

Thanks! :)

[whywhat, 2017-02-10 16:02:15]

On 2017/02/10 at 14:12:17, mlamouri wrote:
> Could you write a LayoutTest for this to test that the reason why this call
was there is actually not broken. You can find an example in here:
> third_party/WebKit/LayoutTests/media/controls-cast-button-narrow.html
> 
> (though, better to add the test in media/controls/, there are too many random
various tests in media/.)
> 
> Thanks! :)

Wasn't the example test written for this particular line of code so if it
passes, it means everything still works?

[mlamouri (slow - plz ping), 2017-02-10 17:05:48]

On 2017/02/10 at 16:02:15, avayvod wrote:
> On 2017/02/10 at 14:12:17, mlamouri wrote:
> > Could you write a LayoutTest for this to test that the reason why this call
was there is actually not broken. You can find an example in here:
> > third_party/WebKit/LayoutTests/media/controls-cast-button-narrow.html
> > 
> > (though, better to add the test in media/controls/, there are too many
random various tests in media/.)
> > 
> > Thanks! :)
> 
> Wasn't the example test written for this particular line of code so if it
passes, it means everything still works?

The test I linked to checks that there is no overlay cast button when a change
happens. Would be great to have one that checks that the controls bar gets a
cast button when a change is triggered. Maybe there is such a test though.

[steimel, 2017-02-10 22:01:22]

Description was changed from

==========
Remove unnecessary refreshCastButtonVisiblity call

computeWhichControlsFit is being called on every mousemove event on a video
(via a call to refreshCastButtonVisiblity). After discussing offline with
mlamouri@ and avayvod@, it was determined that the call to
refreshCastButtonVisiblity is no longer needed, and therefore can be removed.

BUG=639273
==========

to

==========
Remove unnecessary refreshCastButtonVisiblity call

computeWhichControlsFit is being called on every mousemove event on a video
(via a call to refreshCastButtonVisiblity). After discussing offline with
mlamouri@ and avayvod@, it was determined that the call to
refreshCastButtonVisiblity is no longer needed, and therefore can be removed.

In addition, this CL refactors some media layout tests that test the cast button
visibility.

BUG=639273
==========

[whywhat, 2017-02-13 16:36:28]

On 2017/02/10 at 17:05:48, mlamouri wrote:
> On 2017/02/10 at 16:02:15, avayvod wrote:
> > On 2017/02/10 at 14:12:17, mlamouri wrote:
> > > Could you write a LayoutTest for this to test that the reason why this
call was there is actually not broken. You can find an example in here:
> > > third_party/WebKit/LayoutTests/media/controls-cast-button-narrow.html
> > > 
> > > (though, better to add the test in media/controls/, there are too many
random various tests in media/.)
> > > 
> > > Thanks! :)
> > 
> > Wasn't the example test written for this particular line of code so if it
passes, it means everything still works?
> 
> The test I linked to checks that there is no overlay cast button when a change
happens. Would be great to have one that checks that the controls bar gets a
cast button when a change is triggered. Maybe there is such a test though.

Ah, got it, you want to resize the element to make it wider and see if the
button has appeared (I'd extend the existing test by expanding and shrinking the
media element and checking that the presence of the button is affected as
intended -- perhaps, it should've been done by the original CL author).

[steimel, 2017-02-14 00:18:23]

PTAL

[steimel, 2017-02-14 00:40:24]

Description was changed from

==========
Remove unnecessary refreshCastButtonVisiblity call

computeWhichControlsFit is being called on every mousemove event on a video
(via a call to refreshCastButtonVisiblity). After discussing offline with
mlamouri@ and avayvod@, it was determined that the call to
refreshCastButtonVisiblity is no longer needed, and therefore can be removed.

In addition, this CL refactors some media layout tests that test the cast button
visibility.

BUG=639273
==========

to

==========
Remove unnecessary refreshCastButtonVisiblity call

computeWhichControlsFit is being called on every mousemove event on a video
(via a call to refreshCastButtonVisiblity). After discussing offline with
mlamouri@ and avayvod@, it was determined that the call to
refreshCastButtonVisiblity is no longer needed, and therefore can be removed.

In addition, this CL refactors some media layout tests that test the cast button
visibility, and appends to an existing cast button visibility test
(controls-cast-button-narrow.html) to test for correct visibility after
expanding and contracting a video.

BUG=639273
==========

[whywhat, 2017-02-14 01:21:42]

lgtm

thanks for updating the tests!

[steimel, 2017-02-14 01:37:20]

The CQ bit was checked by steimel@chromium.org

[commit-bot: I haz the power, 2017-02-14 01:38:08]

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 01:47:02]

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

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

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

[mlamouri (slow - plz ping), 2017-02-14 09:58:06]

sgtm with some tests cleanup :)

https://codereview.chromium.org/2684973004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/controls-cast-button-narrow.html
(right):

https://codereview.chromium.org/2684973004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls-cast-button-narrow.html:24: //
Make video wider
Here and below, most of these comments are simply describing what the JS line
does. We should keep the comments to things that help understand the code IMO.

https://codereview.chromium.org/2684973004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls-cast-button-narrow.html:37:
setTimeout(t.step_func(function() {
You probably want `testRunner.layoutAndPaintAsyncThen(t.step_func(() => {`
instead.

[steimel, 2017-02-14 17:31:01]

PTAL

https://codereview.chromium.org/2684973004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/controls-cast-button-narrow.html
(right):

https://codereview.chromium.org/2684973004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls-cast-button-narrow.html:24: //
Make video wider
On 2017/02/14 09:58:05, mlamouri wrote:
> Here and below, most of these comments are simply describing what the JS line
> does. We should keep the comments to things that help understand the code IMO.

Done.

https://codereview.chromium.org/2684973004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls-cast-button-narrow.html:37:
setTimeout(t.step_func(function() {
On 2017/02/14 09:58:05, mlamouri wrote:
> You probably want `testRunner.layoutAndPaintAsyncThen(t.step_func(() => {`
> instead.

Done.

[mlamouri (slow - plz ping), 2017-02-14 21:00:17]

lgtm with nit-style fixed :)

https://codereview.chromium.org/2684973004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/controls-cast-button-narrow.html
(right):

https://codereview.chromium.org/2684973004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls-cast-button-narrow.html:34: 
nit: one empty line too many.

[steimel, 2017-02-14 22:32:12]

https://codereview.chromium.org/2684973004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/controls-cast-button-narrow.html
(right):

https://codereview.chromium.org/2684973004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls-cast-button-narrow.html:34: 
On 2017/02/14 21:00:17, mlamouri wrote:
> nit: one empty line too many.

Done.

[steimel, 2017-02-14 22:32:16]

The CQ bit was checked by steimel@chromium.org

[steimel, 2017-02-14 22:32:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2684973004/#ps80001
(title: "remove extra blank lines")

[commit-bot: I haz the power, 2017-02-14 22:33:12]

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-15 00:24:23]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487111536403940,
"parent_rev": "4cb673a8d363c7ddb6f05a009a313e7e10d21b11", "commit_rev":
"6fb1451de0171f8bdd0915c22016750d79c0b3ac"}

[commit-bot: I haz the power, 2017-02-15 00:24:58]

Description was changed from

==========
Remove unnecessary refreshCastButtonVisiblity call

computeWhichControlsFit is being called on every mousemove event on a video
(via a call to refreshCastButtonVisiblity). After discussing offline with
mlamouri@ and avayvod@, it was determined that the call to
refreshCastButtonVisiblity is no longer needed, and therefore can be removed.

In addition, this CL refactors some media layout tests that test the cast button
visibility, and appends to an existing cast button visibility test
(controls-cast-button-narrow.html) to test for correct visibility after
expanding and contracting a video.

BUG=639273
==========

to

==========
Remove unnecessary refreshCastButtonVisiblity call

computeWhichControlsFit is being called on every mousemove event on a video
(via a call to refreshCastButtonVisiblity). After discussing offline with
mlamouri@ and avayvod@, it was determined that the call to
refreshCastButtonVisiblity is no longer needed, and therefore can be removed.

In addition, this CL refactors some media layout tests that test the cast button
visibility, and appends to an existing cast button visibility test
(controls-cast-button-narrow.html) to test for correct visibility after
expanding and contracting a video.

BUG=639273

Review-Url: https://codereview.chromium.org/2684973004
Cr-Commit-Position: refs/heads/master@{#450524}
Committed:
https://chromium.googlesource.com/chromium/src/+/6fb1451de0171f8bdd0915c22016...
==========

[commit-bot: I haz the power, 2017-02-15 00:24:59]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/6fb1451de0171f8bdd0915c22016...