Media Controls: cache the minimum width in order to reduce incorrect width usage.

Media Controls: cache the minimum width in order to reduce incorrect width usage.

The width of media controls buttons depends on the platform (Android vs
desktop) and is known by looking at the layout of the play button or the
overflow menu button. This technique has limitations because it requires
one layout to happen which might take a long time depending on when the
controls have been toggled.

The new solution consist of saving the minimum width so that as soon as
the controls discovered the right value, they should no longer appear
broken.

BUG=663931
R=zqzhang@chromium.org

Review-Url: https://codereview.chromium.org/2704013002
Cr-Commit-Position: refs/heads/master@{#451611}
Committed: https://chromium.googlesource.com/chromium/src/+/e5399d9f871fab1ea1e7b061125106199ee0545a

[mlamouri (slow - plz ping), 2017-02-18 13:31:08]

PTAL

[mlamouri (slow - plz ping), 2017-02-18 13:31:12]

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

[commit-bot: I haz the power, 2017-02-18 13:31:19]

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

[Zhiqiang Zhang (Slow), 2017-02-18 15:04:37]

The idea sounds good but is there a better way to solve the bug?

Does Window.getComputedStyle() work for our case?

https://codereview.chromium.org/2704013002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/controls/buttons-after-reset.html
(right):

https://codereview.chromium.org/2704013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/controls/buttons-after-reset.html:15:
video.addEventListener('loadedmetadata', () => {
Can you explain why hide and show the controls and then check if it matches the
expectation?

[commit-bot: I haz the power, 2017-02-18 15:21:18]

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

[commit-bot: I haz the power, 2017-02-18 15:21:19]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2017-02-19 10:33:57]

https://codereview.chromium.org/2704013002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/controls/buttons-after-reset.html
(right):

https://codereview.chromium.org/2704013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/controls/buttons-after-reset.html:15:
video.addEventListener('loadedmetadata', () => {
On 2017/02/18 at 15:04:37, Zhiqiang Zhang wrote:
> Can you explain why hide and show the controls and then check if it matches
the expectation?

Without the fix, when we do hide/show, we end up without a computed size so 48
will end up being used. With the fix, the 32 value that was computed earlier
will be kept.

My guess with regards to why this is happening is that when we hide the
controls, obviously the style is lost. Then, we show them back but the
`computeWhichControlsFit()` is called before the controls are drawn (which makes
sense). Thus, they have no style and 48px will be used as the default value.
Because we don't do useless layouts on purpose, we wouldn't call
`computeWhichControlsFit()` again for a while.

The real fix would be much complex unless we hard code the values: the issue
here is that `computeWhichControlsFit()` requires a style to be available but
this is called as part of the layout, creating a weird cyclical dependency.
Another dirty work around would be to, on purpose, have a second layout in some
situations so `computeWhichControlsFit()` would be called in a proper state the
second time. Though, it might create flickering.

[Zhiqiang Zhang (Slow), 2017-02-19 23:44:52]

lgtm

[mlamouri (slow - plz ping), 2017-02-20 11:02:36]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2017-02-20 11:02:53]

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-20 13:03:31]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1487588556221060, "parent_rev":
"e91c63172c67544b11170de90e82dcd71a0f5799", "commit_rev":
"e5399d9f871fab1ea1e7b061125106199ee0545a"}

[commit-bot: I haz the power, 2017-02-20 13:04:05]

Description was changed from

==========
Media Controls: cache the minimum width in order to reduce incorrect width
usage.

The width of media controls buttons depends on the platform (Android vs
desktop) and is known by looking at the layout of the play button or the
overflow menu button. This technique has limitations because it requires
one layout to happen which might take a long time depending on when the
controls have been toggled.

The new solution consist of saving the minimum width so that as soon as
the controls discovered the right value, they should no longer appear
broken.

BUG=663931
R=zqzhang@chromium.org
==========

to

==========
Media Controls: cache the minimum width in order to reduce incorrect width
usage.

The width of media controls buttons depends on the platform (Android vs
desktop) and is known by looking at the layout of the play button or the
overflow menu button. This technique has limitations because it requires
one layout to happen which might take a long time depending on when the
controls have been toggled.

The new solution consist of saving the minimum width so that as soon as
the controls discovered the right value, they should no longer appear
broken.

BUG=663931
R=zqzhang@chromium.org

Review-Url: https://codereview.chromium.org/2704013002
Cr-Commit-Position: refs/heads/master@{#451611}
Committed:
https://chromium.googlesource.com/chromium/src/+/e5399d9f871fab1ea1e7b0611251...
==========

[commit-bot: I haz the power, 2017-02-20 13:04:07]

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