Media controls refer to less magic state.

Repaint media controls when network state changes.

Cause any change in network state in HTMLMediaElement to repaint the media
controls, since they rely on it silently during painting.

BUG=545704
Committed: https://crrev.com/cf0b46870c0ea22aa558903496685e4fe5b3840c
Cr-Commit-Position: refs/heads/master@{#357153}

[liberato (no reviews please), 2015-10-21 19:02:28]

Patchset #1 (id:1) has been deleted

[liberato (no reviews please), 2015-10-22 21:30:03]

Description was changed from

==========
Media controls refer to less magic state.

MediaControlsPainter looked at magic bits of state to change how
they draw.  The overlay play button, for example, might choose not
to paint anything if it believed that the media element couldn't
play currently.

The problem with this is that changes in state weren't being
reflected with new paints.

Now, MediaControls takes more responsility for managing this state,
and reflecting changes in ways that actually cause a repaint.  For
example, the overlay play button is now marked as isWanted() only
when it's actually supposed to be on the screen.

Similar changes were made for closed caption, full screen, mute,
and regular play.

BUG=545704
==========

to

==========
Media controls refer to less magic state.

MediaControlsPainter looked at magic bits of state to change how they draw.  The
overlay play button, for example, might choose not to paint anything if it
believed that the media element couldn't play currently.

The problem with this is that changes in state weren't being reflected with new
paints.

Now, MediaControls takes more responsility for managing this state, and
reflecting changes in ways that actually cause a repaint.  For example, the
overlay play button is now marked as isWanted() only when it's actually supposed
to be on the screen.

Similar changes were made for closed caption, full screen, mute, and regular
play.

BUG=545704
==========

[liberato (no reviews please), 2015-10-22 21:30:03]

liberato@chromium.org changed reviewers:
+ philipj@opera.com

[liberato (no reviews please), 2015-10-22 21:33:49]

all of the test rebaselines were cases in which the test memorized non-disabled
buttons incorrectly.

i'd like to add an additional test for the case of "player becomes ready but
doesn't paint", but it's pretty hard to trigger without a race.  the best i
could come up with was an Internals method to lie about the network state on a
player that has no source.

does that seem useful?  do you have a better idea.

[philipj_slow, 2015-10-23 07:54:43]

Description was changed from

==========
Media controls refer to less magic state.

MediaControlsPainter looked at magic bits of state to change how they draw.  The
overlay play button, for example, might choose not to paint anything if it
believed that the media element couldn't play currently.

The problem with this is that changes in state weren't being reflected with new
paints.

Now, MediaControls takes more responsility for managing this state, and
reflecting changes in ways that actually cause a repaint.  For example, the
overlay play button is now marked as isWanted() only when it's actually supposed
to be on the screen.

Similar changes were made for closed caption, full screen, mute, and regular
play.

BUG=545704
==========

to

==========
Media controls refer to less magic state.

MediaControlsPainter looked at magic bits of state to change how they
draw. The overlay play button, for example, might choose not to paint
anything if it believed that the media element couldn't play currently.

The problem with this is that changes in state weren't being reflected
with new paints.

Now, MediaControls takes more responsility for managing this state, and
reflecting changes in ways that actually cause a repaint. For example,
the overlay play button is now marked as isWanted() only when it's
actually supposed to be on the screen.

Similar changes were made for closed caption, fullscreen, mute, and
regular play.

BUG=545704
==========

[philipj_slow, 2015-10-23 08:31:25]

This CL is doing a bunch of different things, I'd like to discuss the core of
issue 545704 and get that fixed first, and if possible save refactoring for a
separate CL.

So, if I understand correctly, the precise problem here is that
MediaControlsPainter paints different things depending on networkState, but a
change in networkState does not always result in a repaint.

There's a similar situation in MediaControlsPainter::paintMediaSliderInternal
where currentTime and duration are taken from the media element instead of via
some style or attribute set by MediaControls. Here the problem is solved by
calling setShouldDoFullPaintInvalidation() in
MediaControlTimelineElement::setDuration() and ::setPosition().

The overlay play button isn't the only thing that depends on networkState, so
it's possible that there are more subtle problems like this.

A solution, I think, would be to introduce a HTMLMediaElement::setNetworkState()
that is consistently used instead of assigning to m_networkState. Then, call a
new MediaControls::networkStateChanged() or possibly just MediaControls::reset()
to repaint what needs repainting.

That ought to be a very small CL, and I'd be interested to see if it fixes the
problem or not.

(More generally it would be great if MediaControlsPainter.cpp didn't know about
HTMLMediaElement at all so that all state had to be propagated in the same way,
but that's a significant refactoring.)

[liberato (no reviews please), 2015-10-23 15:04:08]

On 2015/10/23 08:31:25, philipj wrote:
> This CL is doing a bunch of different things, I'd like to discuss the core of
> issue 545704 and get that fixed first, and if possible save refactoring for a
> separate CL.
> 
> So, if I understand correctly, the precise problem here is that
> MediaControlsPainter paints different things depending on networkState, but a
> change in networkState does not always result in a repaint.
> 
> There's a similar situation in MediaControlsPainter::paintMediaSliderInternal
> where currentTime and duration are taken from the media element instead of via
> some style or attribute set by MediaControls. Here the problem is solved by
> calling setShouldDoFullPaintInvalidation() in
> MediaControlTimelineElement::setDuration() and ::setPosition().
> 
> The overlay play button isn't the only thing that depends on networkState, so
> it's possible that there are more subtle problems like this.
> 
> A solution, I think, would be to introduce a
HTMLMediaElement::setNetworkState()
> that is consistently used instead of assigning to m_networkState. Then, call a
> new MediaControls::networkStateChanged() or possibly just
MediaControls::reset()
> to repaint what needs repainting.
> 
> That ought to be a very small CL, and I'd be interested to see if it fixes the
> problem or not.
> 
> (More generally it would be great if MediaControlsPainter.cpp didn't know
about
> HTMLMediaElement at all so that all state had to be propagated in the same
way,
> but that's a significant refactoring.)

i did get a little overzealous trying to move state out of the painter, yes.
however, the same repro also breaks the control bar play button similarly
between opaque and partially transparent, where the next paint would make it
switch to the correct state.  all of these need to be fixed, but i'll do
them in separate CLs.

HTMLMediaElement::setNetworkState: good idea!

core problem: not sure that we see the same core problem.  i entirely agree
that, in the long run, having the painters start poking around for internal
state is bad, and is just asking for weird display bugs like this one.

however, for our overlay play button, i think it's especially extra broken and
needs to be fixed sooner.  i don't see it as simply missing a repaint when the
internal state changes.

in particular, it's a very weird that the play button has two different 'not
shown' states. MediaController can decide to display:none it, and if not, then
the painter can choose simply to make it invisible if it doesn't agree. 
further, these decisions are entirely federated between them; there's no sort of
central decision.  i believe that's just broken.

for this CL, i'll try the repaint-only solution that you suggest -- it seems
safer if we want to merge back to M46.  i'll also make a separate CL that merges
the logic into one place for the overlay play button.

[philipj_slow, 2015-10-23 17:36:17]

I do agree that it's extra strange when the very *same* kind of state
(visibility) is handled both via CSS and the painter. If that confusion is
really the cause of issue 545704 then of course it should be fixed, but from the
issue comments it sounds like it's really the lack of invalidation on
networkState changes.

[philipj_slow, 2015-10-23 17:41:21]

Note sure if you wanted review yet, but this looks great. A test, somehow, would
be nice of course. If layout tests can't be made reliable (or fast) then a unit
test might be possible.

https://codereview.chromium.org/1417683004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1417683004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3589: if
(MediaControls* controls = mediaControls())
Yep, looks good. Later it may be possible to bake
changeNetworkStateFromLoadingToIdle() into this by checking old and new state,
but not this CL.

[liberato (no reviews please), 2015-10-23 21:56:33]

Patchset #9 (id:180001) has been deleted

[liberato (no reviews please), 2015-10-23 21:59:17]

Description was changed from

==========
Media controls refer to less magic state.

MediaControlsPainter looked at magic bits of state to change how they
draw. The overlay play button, for example, might choose not to paint
anything if it believed that the media element couldn't play currently.

The problem with this is that changes in state weren't being reflected
with new paints.

Now, MediaControls takes more responsility for managing this state, and
reflecting changes in ways that actually cause a repaint. For example,
the overlay play button is now marked as isWanted() only when it's
actually supposed to be on the screen.

Similar changes were made for closed caption, fullscreen, mute, and
regular play.

BUG=545704
==========

to

==========
Repaint media controls when network state changes.
    
Cause any change in network state in HTMLMediaElement to repaint the media
controls, since they rely on it silently during painting.
    
BUG=545704
==========

[philipj_slow, 2015-10-26 10:33:00]

Did you forget to upload media/controls-repaint-for-network-change.html?

[liberato (no reviews please), 2015-10-26 14:07:25]

On 2015/10/26 10:33:00, philipj wrote:
> Did you forget to upload media/controls-repaint-for-network-change.html?

yes, sorry about that.  fixed.

-fl

[philipj_slow, 2015-10-28 15:14:02]

Actual solution looks good, just some thoughts about the things around it.

https://codereview.chromium.org/1417683004/diff/260001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1417683004/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1005: crbug.com/545704
media/controls-repaint-for-network-change.html [ NeedsRebaseline ]
If a layout test is indeed the best way to test this, then a reftest should
still be possible. In other words, a
media/controls-repaint-for-network-change-expected.html that gets into the same
state but by loading an audio file or perhaps using MSE.

https://codereview.chromium.org/1417683004/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html
(right):

https://codereview.chromium.org/1417683004/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html:1:
<html>
https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-...

https://codereview.chromium.org/1417683004/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html:10:
var video1 = document.getElementById("video1");
You might also use document.querySelectorAll("video") and then video[0] to save
some IDs and lines of code.

https://codereview.chromium.org/1417683004/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html:37:
<p>Test that network state transitions paint video controls properly.</p>
Is it not possible to write a reliable test using the original repro steps that
does not need to use a new internal API?

https://codereview.chromium.org/1417683004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1417683004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3589: if
(MediaControls* controls = mediaControls())
I haven't investigated, but I wouldn't be too surprised if some of the
setNetworkState(NETWORK_LOADING) calls happen over and over again, which would
invalidate every time. An if (m_networkState == networkState) return would fix
this.

https://codereview.chromium.org/1417683004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1417683004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:445: void
setNetworkState(NetworkState);
Make this private if it's possible to avoid the Internals API.

https://codereview.chromium.org/1417683004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/1417683004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:417:
repaint(m_muteButton);
Maybe call it invalidate() to match the comment? I doesn't synchronously
repaint, after all. I guess it doesn't even synchronously invalidate, but that's
close enough for my taste :)

https://codereview.chromium.org/1417683004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/1417683004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.cpp:2567:
mediaElement->setNetworkState((WebMediaPlayer::NetworkState)((int)state));
Use static_cast and first assert that state is in the correct range since
nothing else will.

https://codereview.chromium.org/1417683004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/1417683004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.idl:361:
[TypeChecking=Interface] void setMediaElementNetworkState(HTMLMediaElement
element, double state);
If this stays, s/double/long/ or some other integer type.

[liberato (no reviews please), 2015-10-29 16:10:26]

https://chromiumcodereview.appspot.com/1417683004/diff/260001/third_party/Web...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://chromiumcodereview.appspot.com/1417683004/diff/260001/third_party/Web...
third_party/WebKit/LayoutTests/TestExpectations:1005: crbug.com/545704
media/controls-repaint-for-network-change.html [ NeedsRebaseline ]
On 2015/10/28 15:14:01, philipj wrote:
> If a layout test is indeed the best way to test this, then a reftest should
> still be possible. In other words, a
> media/controls-repaint-for-network-change-expected.html that gets into the
same
> state but by loading an audio file or perhaps using MSE.

i've switched to reftest.  had to switch to audio, and restrict the width of the
element so that the time slider doesn't show up.  otherwise, the ref has a
slider thumb and while the test doesn't.

https://chromiumcodereview.appspot.com/1417683004/diff/260001/third_party/Web...
File
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html
(right):

https://chromiumcodereview.appspot.com/1417683004/diff/260001/third_party/Web...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html:1:
<html>
On 2015/10/28 15:14:01, philipj wrote:
>
https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-...

Done.

https://chromiumcodereview.appspot.com/1417683004/diff/260001/third_party/Web...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html:10:
var video1 = document.getElementById("video1");
On 2015/10/28 15:14:02, philipj wrote:
> You might also use document.querySelectorAll("video") and then video[0] to
save
> some IDs and lines of code.

Done.

https://chromiumcodereview.appspot.com/1417683004/diff/260001/third_party/Web...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html:37:
<p>Test that network state transitions paint video controls properly.</p>
On 2015/10/28 15:14:01, philipj wrote:
> Is it not possible to write a reliable test using the original repro steps
that
> does not need to use a new internal API?

not that i could find.  it seems to be an actual race on when the network state
changes, and when the most recent paint was.

https://chromiumcodereview.appspot.com/1417683004/diff/260001/third_party/Web...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://chromiumcodereview.appspot.com/1417683004/diff/260001/third_party/Web...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3589: if
(MediaControls* controls = mediaControls())
On 2015/10/28 15:14:02, philipj wrote:
> I haven't investigated, but I wouldn't be too surprised if some of the
> setNetworkState(NETWORK_LOADING) calls happen over and over again, which would
> invalidate every time. An if (m_networkState == networkState) return would fix
> this.

Done.

https://chromiumcodereview.appspot.com/1417683004/diff/260001/third_party/Web...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://chromiumcodereview.appspot.com/1417683004/diff/260001/third_party/Web...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:445: void
setNetworkState(NetworkState);
On 2015/10/28 15:14:02, philipj wrote:
> Make this private if it's possible to avoid the Internals API.

Acknowledged.

https://chromiumcodereview.appspot.com/1417683004/diff/260001/third_party/Web...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://chromiumcodereview.appspot.com/1417683004/diff/260001/third_party/Web...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:417:
repaint(m_muteButton);
On 2015/10/28 15:14:02, philipj wrote:
> Maybe call it invalidate() to match the comment? I doesn't synchronously
> repaint, after all. I guess it doesn't even synchronously invalidate, but
that's
> close enough for my taste :)

Done.  ... but it really does toggleVariousBits().  :)

https://chromiumcodereview.appspot.com/1417683004/diff/260001/third_party/Web...
File third_party/WebKit/Source/core/testing/Internals.cpp (right):

https://chromiumcodereview.appspot.com/1417683004/diff/260001/third_party/Web...
third_party/WebKit/Source/core/testing/Internals.cpp:2567:
mediaElement->setNetworkState((WebMediaPlayer::NetworkState)((int)state));
On 2015/10/28 15:14:02, philipj wrote:
> Use static_cast and first assert that state is in the correct range since
> nothing else will.

Done.

https://chromiumcodereview.appspot.com/1417683004/diff/260001/third_party/Web...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://chromiumcodereview.appspot.com/1417683004/diff/260001/third_party/Web...
third_party/WebKit/Source/core/testing/Internals.idl:361:
[TypeChecking=Interface] void setMediaElementNetworkState(HTMLMediaElement
element, double state);
On 2015/10/28 15:14:02, philipj wrote:
> If this stays, s/double/long/ or some other integer type.

done.  the last time i tried to add a non-double to one of the blink idls, it
refused to compile with errors in (i think) the v8 bindings.  weird.

[philipj_slow, 2015-10-30 10:55:37]

lgtm % test nits

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
File
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change-expected.html
(right):

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change-expected.html:17:
var audios = document.getElementById("audio1").src = src;
The audios variable is unused.

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
File
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html
(right):

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html:21:
for(var i=0; i<3 ;i++)
Needs more space ("for (var i = 0; i < 3; i++)") and indentation on the
following line.

If you assign audios with Array.from(), you could instead use "for (audio of
audios)", if you like that better.

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html:30:
window.internals.setMediaElementNetworkState(audios[2], 0);
Given that audio 0 and 1 end up in the same state, wouldn't just two audio
elements suffice for this test?

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html:38:
<body onload="test()">
If you like you can omit the <body> and just put the script after the audio
elements. That'll safe a function and some indentation.

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html:39:
<audio id="audio1" controls></audio>
The IDs are unused.

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
File third_party/WebKit/Source/core/testing/Internals.h (right):

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
third_party/WebKit/Source/core/testing/Internals.h:399: void
setMediaElementNetworkState(HTMLMediaElement*, long state);
The long type in WebIDL <https://heycam.github.io/webidl/#idl-long> corresponds
to int32_t in C++, and internally int is used if you look at at e.g.
Element.scrollWidth or the generated code.

[liberato (no reviews please), 2015-10-30 15:09:07]

thanks for the feedback.  all addressed, sending to CQ.

-fl

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
File
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change-expected.html
(right):

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change-expected.html:17:
var audios = document.getElementById("audio1").src = src;
On 2015/10/30 10:55:36, philipj wrote:
> The audios variable is unused.

Done.

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
File
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html
(right):

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html:21:
for(var i=0; i<3 ;i++)
On 2015/10/30 10:55:37, philipj wrote:
> Needs more space ("for (var i = 0; i < 3; i++)") and indentation on the
> following line.
> 
> If you assign audios with Array.from(), you could instead use "for (audio of
> audios)", if you like that better.

with two elements, i just unrolled.

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html:30:
window.internals.setMediaElementNetworkState(audios[2], 0);
On 2015/10/30 10:55:37, philipj wrote:
> Given that audio 0 and 1 end up in the same state, wouldn't just two audio
> elements suffice for this test?

Done.

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html:38:
<body onload="test()">
On 2015/10/30 10:55:37, philipj wrote:
> If you like you can omit the <body> and just put the script after the audio
> elements. That'll safe a function and some indentation.

Done.

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html:39:
<audio id="audio1" controls></audio>
On 2015/10/30 10:55:37, philipj wrote:
> The IDs are unused.

Done.

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
File third_party/WebKit/Source/core/testing/Internals.h (right):

https://chromiumcodereview.appspot.com/1417683004/diff/300001/third_party/Web...
third_party/WebKit/Source/core/testing/Internals.h:399: void
setMediaElementNetworkState(HTMLMediaElement*, long state);
On 2015/10/30 10:55:37, philipj wrote:
> The long type in WebIDL <https://heycam.github.io/webidl/#idl-long>
corresponds
> to int32_t in C++, and internally int is used if you look at at e.g.
> Element.scrollWidth or the generated code.

Done.

[liberato (no reviews please), 2015-10-30 15:09:13]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2015-10-30 15:09:13]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com
Link to the patchset:
https://chromiumcodereview.appspot.com/1417683004/#ps320001 (title: "cl
feedback.")

[commit-bot: I haz the power, 2015-10-30 15:09:29]

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

[commit-bot: I haz the power, 2015-10-30 18:46:10]

Committed patchset #15 (id:320001)

[commit-bot: I haz the power, 2015-10-30 18:46:48]

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

[philipj_slow, 2015-11-03 09:04:06]

Some follow-ups, can you create a new CL?

https://codereview.chromium.org/1417683004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.h (right):

https://codereview.chromium.org/1417683004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.h:399: void
setMediaElementNetworkState(HTMLMediaElement*, long state);
On 2015/10/30 15:09:07, liberato wrote:
> On 2015/10/30 10:55:37, philipj wrote:
> > The long type in WebIDL <https://heycam.github.io/webidl/#idl-long>
> corresponds
> > to int32_t in C++, and internally int is used if you look at at e.g.
> > Element.scrollWidth or the generated code.
> 
> Done.

The int32_t mention was to point out that WebIDL's long has a very specific
range, but nevertheless int should be used internally.

https://codereview.chromium.org/1417683004/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html
(right):

https://codereview.chromium.org/1417683004/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html:21:
var src = findMediaFile("audio", "content/empty");
This ended up unused.

https://codereview.chromium.org/1417683004/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/controls-repaint-for-network-change.html:24:
window.internals.setMediaElementNetworkState(audios[0], 0);
Hmm, wasn't networkState already 0?

https://codereview.chromium.org/1417683004/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.h (right):

https://codereview.chromium.org/1417683004/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.h:399: void
setMediaElementNetworkState(HTMLMediaElement*, int32_t state);
This should have been just int.