Summary: | [Modern Media Controls] Basic MediaController | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||||||||
Component: | Media | Assignee: | Antoine Quint <graouts> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue, graouts, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | Safari 10 | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 163500 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Antoine Quint
2016-10-17 03:36:37 PDT
Once https://bugs.webkit.org/show_bug.cgi?id=163500 is reviewed and lands, this patch will be updated to add the two new JS files to the WebCore Xcode project and the list of injected scripts. Created attachment 291807 [details]
Patch
Comment on attachment 291807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291807&action=review > Source/WebCore/Modules/modern-media-controls/main.js:29 > + if (!!host) { What's the point of the !!? What value provided here would not be true as just (host)? > Source/WebCore/Modules/modern-media-controls/media/media-controller.js:36 > + this.layoutTraits = LayoutTraits.macOS; > + > + this.controls = new MacOSInlineMediaControls I think you should put a FIXME: here to make it clear it won't always be macOS. And I just noticed the M in MacOSInlineMediaControls. Are you going to have IOSInlineMediaControls? (In reply to comment #4) > Comment on attachment 291807 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291807&action=review > > > Source/WebCore/Modules/modern-media-controls/main.js:29 > > + if (!!host) { > > What's the point of the !!? What value provided here would not be true as > just (host)? Old habits, will fix. > > Source/WebCore/Modules/modern-media-controls/media/media-controller.js:36 > > + this.layoutTraits = LayoutTraits.macOS; > > + > > + this.controls = new MacOSInlineMediaControls > > I think you should put a FIXME: here to make it clear it won't always be > macOS. > > And I just noticed the M in MacOSInlineMediaControls. Are you going to have > IOSInlineMediaControls? Yes, that's right. Class names always start with capitals. Created attachment 291873 [details]
Patch for landing
Created attachment 291874 [details]
Patch for landing
Created attachment 291876 [details]
Patch for landing
Created attachment 291877 [details]
Patch for landing
Comment on attachment 291877 [details] Patch for landing Clearing flags on attachment: 291877 Committed r207436: <http://trac.webkit.org/changeset/207436> All reviewed patches have been landed. Closing bug. |