Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(68)

Issue 219283004: Remove what remains of MediaControllerInterface (Closed)

Created:
6 years, 8 months ago by philipj_slow
Modified:
6 years, 8 months ago
CC:
blink-reviews, nessy, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, adamk+blink_chromium.org, vcarbune.chromium
Visibility:
Public.

Description

Remove what remains of MediaControllerInterface The currentTime() and duration() should simply use the media element, as that is what paintMediaSlider() in RenderMediaControls uses, so the existing code would not work if the media element and its controller had different durations. setCurrentTime() does require taking the controller into account, so add some tests and expand that final instance of mediaControllerInterface(). BUG=341813, 341852 TEST=LayoutTests/media/controls-timeline.html LayoutTests/media/controls-timeline-with-controller.html Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170512

Patch Set 1 #

Total comments: 2

Patch Set 2 : earliest possible position FIXME #

Unified diffs Side-by-side diffs Delta from patch set Stats (+114 lines, -80 lines) Patch
A LayoutTests/media/controls-timeline.html View 1 chunk +33 lines, -0 lines 0 comments Download
A LayoutTests/media/controls-timeline-expected.txt View 1 chunk +8 lines, -0 lines 0 comments Download
A LayoutTests/media/controls-timeline-with-controller.html View 1 chunk +37 lines, -0 lines 0 comments Download
A LayoutTests/media/controls-timeline-with-controller-expected.txt View 1 chunk +11 lines, -0 lines 0 comments Download
M Source/core/html/HTMLMediaElement.h View 3 chunks +4 lines, -5 lines 0 comments Download
M Source/core/html/MediaController.h View 3 chunks +4 lines, -5 lines 0 comments Download
D Source/core/html/MediaControllerInterface.h View 1 chunk +0 lines, -45 lines 0 comments Download
M Source/core/html/shadow/MediaControlElementTypes.h View 2 chunks +0 lines, -2 lines 0 comments Download
M Source/core/html/shadow/MediaControlElementTypes.cpp View 1 chunk +0 lines, -5 lines 0 comments Download
M Source/core/html/shadow/MediaControlElements.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/html/shadow/MediaControlElements.cpp View 2 chunks +7 lines, -2 lines 0 comments Download
M Source/core/html/shadow/MediaControls.h View 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/html/shadow/MediaControls.cpp View 6 chunks +8 lines, -15 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
philipj_slow
PTAL. https://codereview.chromium.org/219523004/ is needed for the new test to pass, so no trybots up front.
6 years, 8 months ago (2014-03-31 17:30:11 UTC) #1
acolwell GONE FROM CHROMIUM
lgtm % nit https://codereview.chromium.org/219283004/diff/1/Source/core/html/shadow/MediaControlElements.cpp File Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/219283004/diff/1/Source/core/html/shadow/MediaControlElements.cpp#newcode378 Source/core/html/shadow/MediaControlElements.cpp:378: mediaElement().controller()->setCurrentTime(time, IGNORE_EXCEPTION); nit: We should probably ...
6 years, 8 months ago (2014-03-31 23:23:13 UTC) #2
philipj_slow
https://codereview.chromium.org/219283004/diff/1/Source/core/html/shadow/MediaControlElements.cpp File Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/219283004/diff/1/Source/core/html/shadow/MediaControlElements.cpp#newcode378 Source/core/html/shadow/MediaControlElements.cpp:378: mediaElement().controller()->setCurrentTime(time, IGNORE_EXCEPTION); On 2014/03/31 23:23:13, acolwell wrote: > nit: ...
6 years, 8 months ago (2014-04-01 03:22:31 UTC) #3
philipj_slow
The CQ bit was checked by philipj@opera.com
6 years, 8 months ago (2014-04-01 03:25:59 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/philipj@opera.com/219283004/20001
6 years, 8 months ago (2014-04-01 03:26:10 UTC) #5
commit-bot: I haz the power
Change committed as 170512
6 years, 8 months ago (2014-04-01 04:38:42 UTC) #6
acolwell GONE FROM CHROMIUM
On 2014/04/01 03:22:31, philipj wrote: > https://codereview.chromium.org/219283004/diff/1/Source/core/html/shadow/MediaControlElements.cpp > File Source/core/html/shadow/MediaControlElements.cpp (right): > > https://codereview.chromium.org/219283004/diff/1/Source/core/html/shadow/MediaControlElements.cpp#newcode378 > ...
6 years, 8 months ago (2014-04-01 15:23:41 UTC) #7
philipj_slow
6 years, 8 months ago (2014-04-02 20:35:57 UTC) #8
Message was sent while issue was closed.
On 2014/04/01 15:23:41, acolwell wrote:
> On 2014/04/01 03:22:31, philipj wrote:
> >
>
https://codereview.chromium.org/219283004/diff/1/Source/core/html/shadow/Medi...
> > File Source/core/html/shadow/MediaControlElements.cpp (right):
> > 
> >
>
https://codereview.chromium.org/219283004/diff/1/Source/core/html/shadow/Medi...
> > Source/core/html/shadow/MediaControlElements.cpp:378:
> > mediaElement().controller()->setCurrentTime(time, IGNORE_EXCEPTION);
> > On 2014/03/31 23:23:13, acolwell wrote:
> > > nit: We should probably have a FIXME: here that indicates we'll need to
> > properly
> > > apply the media element's relative offset, once "timeline offset"(aka
> > > getStartDate()) is implemented. A bug should probably be filed and
> associated
> > > with http://crbug.com/312699
> > 
> > getStartDate() returns the "timeline offset" which is "an explicit date and
> time
> > corresponding to the zero time in the media timeline" so I don't think
that's
> > quite right.
> 
> I was actually referring to the paragraph above the "media resource end
> position" definition
>
(http://www.w3.org/html/wg/drafts/html/master/embedded-content.html#media-reso...).
> IIUC two media elements 0 time don't line up if their "timestamp offsets" are
> different. The code would have to apply its relative offset to the time passed
> to the controller if it doesn't happen to be the HTMLMediaElement with the
> earliest "timestamp offset". This could get somewhat confusing in the UI since
> depending on which HTMLMediaElement's controls you are using, you may not be
> able to seek to the beginning of the presentation.

Oh, I had completely overlooked that bit. Will add a FIXME.

Powered by Google App Engine
This is Rietveld 408576698