Bug 145827 - Update media controls JS and CSS to use pip
Summary: Update media controls JS and CSS to use pip
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on:
Blocks: 145825
  Show dependency treegraph
 
Reported: 2015-06-09 16:39 PDT by Jon Lee
Modified: 2015-06-11 14:07 PDT (History)
1 user (show)

See Also:


Attachments
Simplify event handling logic for search cancel button (26.42 KB, patch)
2015-06-11 12:07 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Need a short description (OOPS!). (7.66 KB, patch)
2015-06-11 12:38 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Replace gSimulateOptimizedFullscreenAvailable (2.49 KB, patch)
2015-06-11 12:38 PDT, Jon Lee
dino: review+
Details | Formatted Diff | Diff
Replace optimizedFullscreenButton (8.24 KB, patch)
2015-06-11 12:38 PDT, Jon Lee
dino: review+
Details | Formatted Diff | Diff
Replace handleOptimizedFullscreenButtonClicked (2.43 KB, patch)
2015-06-11 12:38 PDT, Jon Lee
dino: review+
Details | Formatted Diff | Diff
Replace pseudo -webkit-media-controls-optimized-fullscreen-button (6.62 KB, patch)
2015-06-11 12:38 PDT, Jon Lee
dino: review+
Details | Formatted Diff | Diff
Move images and styles into js and css (12.36 KB, patch)
2015-06-11 12:38 PDT, Jon Lee
dino: review+
Details | Formatted Diff | Diff
Update handle events (2.44 KB, patch)
2015-06-11 12:38 PDT, Jon Lee
dino: review+
Details | Formatted Diff | Diff
Clean up MediaControlsHost and HTMLMediaElement (3.13 KB, patch)
2015-06-11 12:38 PDT, Jon Lee
dino: review+
Details | Formatted Diff | Diff
Need a short description (OOPS!). (7.66 KB, patch)
2015-06-11 12:40 PDT, Jon Lee
dino: review+
Details | Formatted Diff | Diff
Patch for submission (34.67 KB, patch)
2015-06-11 13:42 PDT, Jon Lee
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2015-06-09 16:39:08 PDT
Update media controls JS and CSS to use pip
Comment 1 Radar WebKit Bug Importer 2015-06-09 16:39:22 PDT
<rdar://problem/21311576>
Comment 2 Jon Lee 2015-06-11 12:07:05 PDT
Created attachment 254737 [details]
Simplify event handling logic for search cancel button
Comment 3 Jon Lee 2015-06-11 12:07:47 PDT
Comment on attachment 254737 [details]
Simplify event handling logic for search cancel button

ignore that attachment. bad passing of arguments in webkit-patch
Comment 4 Jon Lee 2015-06-11 12:38:16 PDT
Created attachment 254741 [details]
Need a short description (OOPS!).
Comment 5 Jon Lee 2015-06-11 12:38:47 PDT
Created attachment 254742 [details]
Replace gSimulateOptimizedFullscreenAvailable
Comment 6 Jon Lee 2015-06-11 12:38:48 PDT
Created attachment 254743 [details]
Replace optimizedFullscreenButton
Comment 7 Jon Lee 2015-06-11 12:38:50 PDT
Created attachment 254744 [details]
Replace handleOptimizedFullscreenButtonClicked
Comment 8 Jon Lee 2015-06-11 12:38:52 PDT
Created attachment 254745 [details]
Replace pseudo -webkit-media-controls-optimized-fullscreen-button
Comment 9 Jon Lee 2015-06-11 12:38:54 PDT
Created attachment 254746 [details]
Move images and styles into js and css
Comment 10 Jon Lee 2015-06-11 12:38:56 PDT
Created attachment 254747 [details]
Update handle events
Comment 11 Jon Lee 2015-06-11 12:38:59 PDT
Created attachment 254748 [details]
Clean up MediaControlsHost and HTMLMediaElement
Comment 12 Jon Lee 2015-06-11 12:40:19 PDT
Created attachment 254749 [details]
Need a short description (OOPS!).
Comment 13 Jon Lee 2015-06-11 13:21:37 PDT
I've upload 8 small patches for review. I will also post the full patch that EWS can chew on and will represent what I will submit.
Comment 14 Jon Lee 2015-06-11 13:42:22 PDT
Created attachment 254755 [details]
Patch for submission
Comment 15 Dean Jackson 2015-06-11 13:46:02 PDT
Comment on attachment 254743 [details]
Replace optimizedFullscreenButton

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

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:468
> +        pictureInPictureButton.setAttribute('pseudo', '-webkit-media-controls-optimized-fullscreen-button');

This pseudo attribute should change too.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:469
> +        pictureInPictureButton.setAttribute('aria-label', this.UIString('Display Optimized Full Screen'));

And maybe this label.

> Source/WebCore/Modules/mediacontrols/mediaControlsiOS.js:130
> +        this.listenFor(this.controls.pictureInPictureButton, 'touchstart', this.handleOptimizedFullscreenTouchStart);
> +        this.listenFor(this.controls.pictureInPictureButton, 'touchend', this.handleOptimizedFullscreenTouchEnd);
> +        this.listenFor(this.controls.pictureInPictureButton, 'touchcancel', this.handleOptimizedFullscreenTouchCancel);

Are we changing the function names in a later patch?
Comment 16 Dean Jackson 2015-06-11 13:50:23 PDT
Comment on attachment 254755 [details]
Patch for submission

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

> Source/WebCore/ChangeLog:3
> +        Update media controls JS and CSS to use pip

I think we should expand "pip" in the bug title.
Comment 17 Jon Lee 2015-06-11 13:54:23 PDT
Committed r185472: <http://trac.webkit.org/changeset/185472>
Comment 18 Jon Lee 2015-06-11 13:55:20 PDT
(In reply to comment #16)
> Comment on attachment 254755 [details]
> Patch for submission
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254755&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Update media controls JS and CSS to use pip
> 
> I think we should expand "pip" in the bug title.

Updated in the ChangeLogs.
Comment 19 Eric Carlson 2015-06-11 14:07:09 PDT
Comment on attachment 254755 [details]
Patch for submission

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

>>> Source/WebCore/ChangeLog:3
>>> +        Update media controls JS and CSS to use pip
>> 
>> I think we should expand "pip" in the bug title.
> 
> Updated in the ChangeLogs.

NitL "PiP" would be more correct.