| 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: | P2 | Keywords: | InRadar | ||||||||||||||||
| Version: | Other | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Eric Carlson
2020-04-01 13:35:04 PDT
Created attachment 395197 [details]
Patch
Created attachment 395203 [details]
Patch
Created attachment 395212 [details]
Patch
Created attachment 395216 [details]
Patch
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. rendering/RenderThemeWin.cpp(846,30): error C3861: 'adjustMediaSliderThumbSize': identifier not found Created attachment 395281 [details]
Patch for landing
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. 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. Created attachment 395286 [details]
Patch for landing
Created attachment 395307 [details]
Patch for landing
Comment on attachment 395307 [details]
Patch for landing
The Windows test failures seem to be unrelated.
Committed r259435: <https://trac.webkit.org/changeset/259435> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395307 [details]. |