Bug 144973 - [MediaControls] Refactor media controls & bring improvements made to iOS controls to Mac.
Summary: [MediaControls] Refactor media controls & bring improvements made to iOS cont...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
: 144926 (view as bug list)
Depends on: 145035
Blocks:
  Show dependency treegraph
 
Reported: 2015-05-13 16:06 PDT by Jer Noble
Modified: 2016-12-15 14:34 PST (History)
3 users (show)

See Also:


Attachments
Patch (26.66 KB, patch)
2015-05-13 16:27 PDT, Jer Noble
dino: review+
Details | Formatted Diff | Diff
helper patch (6.97 KB, patch)
2015-05-14 14:36 PDT, Roger Fong
no flags Details | Formatted Diff | Diff
Patch for landing. (29.20 KB, patch)
2015-05-15 12:49 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2015-05-13 16:06:48 PDT
[MediaControls] Refactor media controls & bring improvements made to iOS controls to Mac.
Comment 1 Jer Noble 2015-05-13 16:27:17 PDT
Created attachment 253071 [details]
Patch
Comment 2 Jer Noble 2015-05-13 17:34:14 PDT
Comment on attachment 253071 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253071&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:3788
> +    RefPtr<HTMLMediaElement> strongThis = this;
> +    std::function<void()> task = [strongThis] {
> +        if (ShadowRoot* root = strongThis->userAgentShadowRoot())
> +            root->dispatchEvent(Event::create("resize", false, false));
> +    };
> +    document().postTask(task);

This should use a GenericTaskQueue so that it gets cancelled if the HTMLMediaElement is destroyed; i'll change this in a future patch.
Comment 3 Roger Fong 2015-05-14 14:27:32 PDT
*** Bug 144926 has been marked as a duplicate of this bug. ***
Comment 4 Roger Fong 2015-05-14 14:36:39 PDT
Created attachment 253145 [details]
helper patch

This patch makes it so that the button positions are defined using both left and right margins, instead of just one or the other.
This makes it so that each button is essentially defined as the button and the 8pixels to the left and right of it, which allows the control dropoff algorithm to not have to use a spacer element to take the place of the timeline when the video gets too small.

Without this change, the volume and play buttons could potentially appear to be right next to each other at some video widths.
Comment 5 Dean Jackson 2015-05-14 15:12:47 PDT
Comment on attachment 253145 [details]
helper patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253145&action=review

> Source/WebCore/ChangeLog:9
> +        The only visual change here is the swapping of the rewind and play button positions.

Technically we should be able to do that using the flex order. This is ok for now, but I'd like to get the OS X controls to use the order property for the arrangement of the buttons.
Comment 6 Jer Noble 2015-05-14 16:29:32 PDT
Committed r184359: <http://trac.webkit.org/changeset/184359>
Comment 7 Roger Fong 2015-05-14 16:34:05 PDT
reopened until Jer's patch lands.
Comment 8 WebKit Commit Bot 2015-05-14 16:41:58 PDT
Comment on attachment 253145 [details]
helper patch

Clearing flags on attachment: 253145

Committed r184361: <http://trac.webkit.org/changeset/184361>
Comment 9 WebKit Commit Bot 2015-05-14 20:24:45 PDT
Re-opened since this is blocked by bug 145035
Comment 10 Jer Noble 2015-05-15 00:24:51 PDT
I believe the crashes that led to the patch's rollout were due to a pre-existing bug in RenderFlowThread. I filed bug #145042 which should address the underlying cause of the crashes, and if so, I'll roll this patch back in.
Comment 11 Jer Noble 2015-05-15 07:51:21 PDT
(In reply to comment #10)
> I believe the crashes that led to the patch's rollout were due to a
> pre-existing bug in RenderFlowThread. I filed bug #145042 which should
> address the underlying cause of the crashes, and if so, I'll roll this patch
> back in.

And with the patch from that bug in place, I no longer see the crashes in the region test with /this/ patch in place.
Comment 12 Jer Noble 2015-05-15 12:49:29 PDT
Created attachment 253218 [details]
Patch for landing.
Comment 13 Jer Noble 2015-05-15 13:25:47 PDT
Comment on attachment 253218 [details]
Patch for landing.

EWS seems happy, and all tests succeed locally. Marking as cq+.
Comment 14 WebKit Commit Bot 2015-05-15 14:31:48 PDT
Comment on attachment 253218 [details]
Patch for landing.

Clearing flags on attachment: 253218

Committed r184416: <http://trac.webkit.org/changeset/184416>