Merge WebKit r153448 (Optimizes the number of updateTextTrackDisplay calls)

Merge WebKit r153448 (Optimizes the number of updateTextTrackDisplay calls)

Calling updateTextTrackDisplay() will cause the entire DOM subtree containing the active cues
to be torn down and rebuilt. Only call into this method when text tracks have actually changed.

Add an enum parameter to configureTextTrackDisplay() which allows the caller to specify that
method should assume that the list of visible text tracks has changed, forcing a call to
updateTextTrackDisplay().

http://trac.webkit.org/changeset/153448

R=acolwell
TEST=Existing tests pass.

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

[acolwell GONE FROM CHROMIUM, 2013-08-12 20:19:02]

https://codereview.chromium.org/22645014/diff/1/Source/core/html/HTMLMediaEle...
File Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/22645014/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:1040: if (hasMediaControls())
Why is this code dropped? Doesn't this result in a behavior change?

https://codereview.chromium.org/22645014/diff/1/Source/core/html/HTMLMediaEle...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/22645014/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.h:236: DoNotAssumeVisibleTextTracksChanged,
nit: Flags doesn't seem appropriate here since the 2 values are mutually
exclusive. How about something like VisibilityChangeAssumption & change the
values to "AssumeNoVisibleChange" & "AssumeVisibleChange"

https://codereview.chromium.org/22645014/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.h:239: void
configureTextTrackDisplay(ConfigureTextTrackDisplayFlags =
DoNotAssumeVisibleTextTracksChanged);
nit: Do you really need the default argument value here? There appears to be
only two call sites for this method. Any reason not to just explicitly specify
it at the call site that needs it?

[vcarbune.chromium, 2013-08-19 22:20:12]

https://codereview.chromium.org/22645014/diff/1/Source/core/html/HTMLMediaEle...
File Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/22645014/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:1040: if (hasMediaControls())
On 2013/08/12 20:19:02, acolwell wrote:
> Why is this code dropped? Doesn't this result in a behavior change?

No, configureTextTrackDisplay now calls this if AssumeVisibleChange is passed. I
added a while ago this code and it's covered by
track-cue-rendering-mode-changed, which passes using the optimizations.

https://codereview.chromium.org/22645014/diff/1/Source/core/html/HTMLMediaEle...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/22645014/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.h:236: DoNotAssumeVisibleTextTracksChanged,
On 2013/08/12 20:19:02, acolwell wrote:
> nit: Flags doesn't seem appropriate here since the 2 values are mutually
> exclusive. How about something like VisibilityChangeAssumption & change the
> values to "AssumeNoVisibleChange" & "AssumeVisibleChange"

Done. Minimally, this could have been a boolean parameter, but I think this enum
aids readability.

https://codereview.chromium.org/22645014/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.h:239: void
configureTextTrackDisplay(ConfigureTextTrackDisplayFlags =
DoNotAssumeVisibleTextTracksChanged);
On 2013/08/12 20:19:02, acolwell wrote:
> nit: Do you really need the default argument value here? There appears to be
> only two call sites for this method. Any reason not to just explicitly specify
> it at the call site that needs it?

No, explicitly passed it at each function call.

[acolwell GONE FROM CHROMIUM, 2013-08-19 22:37:53]

lgtm

[commit-bot: I haz the power, 2013-08-19 22:48:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vcarbune@chromium.org/22645014/6001

[commit-bot: I haz the power, 2013-08-19 23:02:52]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[jochen (gone - plz use gerrit), 2013-08-20 08:05:12]

lgtm

[commit-bot: I haz the power, 2013-08-20 08:06:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vcarbune@chromium.org/22645014/6001

[commit-bot: I haz the power, 2013-08-20 08:17:14]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-08-20 09:36:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vcarbune@chromium.org/22645014/6001

[commit-bot: I haz the power, 2013-08-20 20:14:37]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[jochen (gone - plz use gerrit), 2013-09-12 09:41:11]

What should we do about this CL?

[vcarbune.chromium, 2013-09-12 10:39:08]

On 2013/09/12 09:41:11, jochen wrote:
> What should we do about this CL?

Oups, I was almost sure I submitted it!
I will rebase it and submit.

[vcarbune.chromium, 2013-09-12 10:40:42]

On 2013/09/12 10:39:08, vcarbune.chromium wrote:
> On 2013/09/12 09:41:11, jochen wrote:
> > What should we do about this CL?
> 
> Oups, I was almost sure I submitted it!
> I will rebase it and submit.

... After re-checking what's happening with these failing tests.

[vcarbune.chromium, 2013-09-27 14:58:08]

On 2013/09/12 10:40:42, vcarbune.chromium wrote:
> On 2013/09/12 10:39:08, vcarbune.chromium wrote:
> > On 2013/09/12 09:41:11, jochen wrote:
> > > What should we do about this CL?
> > 
> > Oups, I was almost sure I submitted it!
> > I will rebase it and submit.
> 
> ... After re-checking what's happening with these failing tests.

So I fixed the code and updated two tests:
*) track-cue-rendering-snap-to-lines was set got an expectation fix (seems that
other parts are currently broken too, tracking under
https://code.google.com/p/chromium/issues/detail?id=299752)
*) different expectation message is now display for video-controls-captions, but
functionality preserved.

[jochen (gone - plz use gerrit), 2013-09-30 09:57:33]

still lgtm

[commit-bot: I haz the power, 2013-09-30 10:05:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vcarbune@chromium.org/22645014/43001

[commit-bot: I haz the power, 2013-09-30 10:56:38]

Change committed as 158530