Bug 209875

Summary:

Remove unused media controls code

Product:

WebKit

Reporter:

Eric Carlson <eric.carlson>

Component:

Media

Assignee:

Eric Carlson <eric.carlson>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aboxhall, apinheiro, calvaris, cdumez, cfleizach, darin, dmazzoni, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jcraig, jdiggs, jer.noble, kangil.han, kondapallykalyan, mifenton, pdr, philipj, samuel_white, sergio, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Other

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none
Patch for landing	none
Patch for landing	none

Eric Carlson

Reported 2020-04-01 13:35:04 PDT

Remove code for the, now unused, C++ based media controls.

Attachments
Patch (208.67 KB, patch) 2020-04-01 13:38 PDT, Eric Carlson	no flags	Details Formatted Diff Diff
Patch (209.56 KB, patch) 2020-04-01 14:42 PDT, Eric Carlson	no flags	Details Formatted Diff Diff
Patch (210.02 KB, patch) 2020-04-01 15:31 PDT, Eric Carlson	no flags	Details Formatted Diff Diff
Patch (210.54 KB, patch) 2020-04-01 15:51 PDT, Eric Carlson	no flags	Details Formatted Diff Diff
Patch for landing (211.05 KB, patch) 2020-04-02 11:28 PDT, Eric Carlson	no flags	Details Formatted Diff Diff
Patch for landing (210.45 KB, patch) 2020-04-02 12:07 PDT, Eric Carlson	no flags	Details Formatted Diff Diff
Patch for landing (210.95 KB, patch) 2020-04-02 15:05 PDT, Eric Carlson	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2020-04-01 13:35:21 PDT

<rdar://problem/61172738>

Eric Carlson

Comment 2 2020-04-01 13:38:21 PDT

Created attachment 395197 [details] Patch

Eric Carlson

Comment 3 2020-04-01 14:42:28 PDT

Created attachment 395203 [details] Patch

Eric Carlson

Comment 4 2020-04-01 15:31:48 PDT

Created attachment 395212 [details] Patch

Eric Carlson

Comment 5 2020-04-01 15:51:22 PDT

Created attachment 395216 [details] Patch

Daniel Bates

Comment 6 2020-04-01 19:41:08 PDT

Comment on attachment 395216 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395216&action=review Patch looks good. Check Windows. > Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:163 > + return m_textTrackContainer.get(); Ok as is. No change needed. Looks like line is indented. > Source/WebCore/html/HTMLMediaElement.cpp:4300 > + if (m_creatingControls) This is ok as-is. No change needed. Is this change concealing a bug revealed by this removal? If so, why is it ok to make this change? If not, proceed. > Source/WebCore/html/HTMLMediaElement.cpp:4303 > m_creatingControls = true; Ok as-is. No change needed. In the future the optimal solution is to use SETforScope() class for this ivar toggling.

Darin Adler

Comment 7 2020-04-01 21:21:30 PDT

rendering/RenderThemeWin.cpp(846,30): error C3861: 'adjustMediaSliderThumbSize': identifier not found

Eric Carlson

Comment 8 2020-04-02 11:28:38 PDT

Created attachment 395281 [details] Patch for landing

Darin Adler

Comment 9 2020-04-02 11:46:56 PDT

Comment on attachment 395281 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=395281&action=review > Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:163 > + return m_textTrackContainer.get(); Indentation mistake here.

Eric Carlson

Comment 10 2020-04-02 11:57:24 PDT

Comment on attachment 395216 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395216&action=review >> Source/WebCore/html/HTMLMediaElement.cpp:4300 >> + if (m_creatingControls) > > This is ok as-is. No change needed. Is this change concealing a bug revealed by this removal? If so, why is it ok to make this change? If not, proceed. ensureUserAgentShadowRoot creates the media controls in the shadow DOM which can trigger scripts in the page and cause this to be called again recursively. In that case we don't need to call ensureUserAgentShadowRoot again. >> Source/WebCore/html/HTMLMediaElement.cpp:4303 >> m_creatingControls = true; > > Ok as-is. No change needed. In the future the optimal solution is to use SETforScope() class for this ivar toggling. I should have thought of that! I'll make that change in another patch, thanks.

Eric Carlson

Comment 11 2020-04-02 12:07:47 PDT

Created attachment 395286 [details] Patch for landing

Eric Carlson

Comment 12 2020-04-02 15:05:55 PDT

Created attachment 395307 [details] Patch for landing

Eric Carlson

Comment 13 2020-04-02 17:10:04 PDT

Comment on attachment 395307 [details] Patch for landing The Windows test failures seem to be unrelated.

EWS

Comment 14 2020-04-02 17:37:07 PDT

Committed r259435: <https://trac.webkit.org/changeset/259435> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395307 [details].

Note You need to log in before you can comment on or make changes to this bug.