[Modern Media Controls] Modern media controls should not need to know about specific platforms in shared code
Created attachment 432334 [details] Patch
Created attachment 432337 [details] Patch
Committed r279309 (239185@main): <https://commits.webkit.org/239185@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 432337 [details].
<rdar://problem/79823034>
Comment on attachment 432337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432337&action=review r=me (I realize it's after the fact, but still). Nice work! > Source/WebCore/Modules/modern-media-controls/controls/icon-service.js:106 > + let resourceDirectory = layoutTraits.resourceDirectory(); NIT: you could move this below or inline it since the variable isn't used > Source/WebCore/Modules/modern-media-controls/controls/ios-inline-media-controls.js:31 > - options.layoutTraits = LayoutTraits.iOS; > + options.layoutTraits = new IOSLayoutTraits(LayoutTraits.Mode.Inline); I wonder if this is actually still needed? Or ideally there'd be a way for the `LayoutTraits` object to be provided when this is constructed instead, so that there's only ever one `LayoutTraits` object. > Source/WebCore/Modules/modern-media-controls/controls/layout-traits.js:31 > + this.mode = mode missing `;` > Source/WebCore/Modules/modern-media-controls/controls/layout-traits.js:36 > + return this.mode == LayoutTraits.Mode.Fullscreen; `===` > Source/WebCore/Modules/modern-media-controls/controls/layout-traits.js:46 > + throw "Derived class must implement this function."; Instead of having every subclass override everything, I feel like some of these could have a default implementation and only have some subclasses override. Not a big deal tho. > Source/WebCore/Modules/modern-media-controls/controls/layout-traits.js:97 > + Fullscreen : 1 Style: missing trailing comma > Source/WebCore/Modules/modern-media-controls/controls/macos-fullscreen-media-controls.js:33 > - options.layoutTraits = LayoutTraits.macOS | LayoutTraits.Fullscreen; > + options.layoutTraits = new MacOSLayoutTraits(LayoutTraits.Mode.Fullscreen); ditto (ios-inline-media-controls.js:31) > Source/WebCore/Modules/modern-media-controls/controls/macos-inline-media-controls.js:33 > - options.layoutTraits = LayoutTraits.macOS; > + options.layoutTraits = new MacOSLayoutTraits(LayoutTraits.Mode.Inline); ditto (ios-inline-media-controls.js:31) > Source/WebCore/Modules/modern-media-controls/controls/macos-layout-traits.js:47 > + return this.isFullscreen; I think this should be `return false;`. > Source/WebCore/Modules/modern-media-controls/controls/time-control.js:43 > + this.scrubber = new Slider("scrubber", this.layoutTraits.knobStyleForScrubber()); NIT: I think this should have a more specific name like `knobStyleForTimeControlScrubber` so that it's not mistakenly thought to also be used for the volume scrubber > Source/WebCore/Modules/modern-media-controls/controls/watchos-layout-traits.js:50 > + return false; I think this should be `return this.isFullscreen;`. > Source/WebCore/Modules/modern-media-controls/media/media-controller.js:102 > + let LayoutTraitsClass = eval(this.host.layoutTraitsClassName); Is it absolutely necessary to use `eval`? I don't know if this will cause issues (e.g. having a `<video>` inside an `<iframe>` that has CSP preventing any `eval` (i.e. without `unsafe-eval`). I think the previous approach of returning a string and using a `switch` was fine. > Source/WebCore/Modules/modern-media-controls/media/media-controller.js:212 > + let overridenSupportingObjectClasses = this.layoutTraits.overridenSupportingObjectClasses(); NIT: instead of having `overridenSupportingObjectClasses` you could just have each `LayoutTraits` return their own list of `supportingObjectClasses` > Source/WebCore/Modules/modern-media-controls/media/media-controller.js:333 > + if (this.layoutTraits.controlsNeverAvailable()) NIT: I'd have restructured this as `if (this.isFullscreen && !this.layoutTraits.controlsAllowedInFullscreen)` so that it's more understandable what this means inside the `LayoutTraits` subclasses (and isn't so completely opposite of `controlsAlwaysAvailable`). > Source/WebCore/Modules/modern-media-controls/media/tracks-support.js:49 > + promoteSubMenus: this.mediaController.layoutTraits.promoteSubMenusWhenShowingMediaControlsContextMenu(), NIT: I feel like this should have a name more specific to `TracksSupport` (e.g. `promoteSubMenusWhenShowingTracksContextMenu`) as right now it reads as a very general thing, which is not how it's used.
(In reply to Devin Rousso from comment #5) > Comment on attachment 432337 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=432337&action=review > > r=me (I realize it's after the fact, but still). Nice work! > > > Source/WebCore/Modules/modern-media-controls/controls/icon-service.js:106 > > + let resourceDirectory = layoutTraits.resourceDirectory(); > > NIT: you could move this below or inline it since the variable isn't used Ok. Will fix. > > > Source/WebCore/Modules/modern-media-controls/controls/ios-inline-media-controls.js:31 > > - options.layoutTraits = LayoutTraits.iOS; > > + options.layoutTraits = new IOSLayoutTraits(LayoutTraits.Mode.Inline); > > I wonder if this is actually still needed? Or ideally there'd be a way for > the `LayoutTraits` object to be provided when this is constructed instead, > so that there's only ever one `LayoutTraits` object. That would preclude LayoutTraits from having state, which doesn't seem ideal. > > > Source/WebCore/Modules/modern-media-controls/controls/layout-traits.js:31 > > + this.mode = mode > > missing `;` Ok. Will fix. > > > Source/WebCore/Modules/modern-media-controls/controls/layout-traits.js:36 > > + return this.mode == LayoutTraits.Mode.Fullscreen; > > `===` Ok. Will fix. > > > Source/WebCore/Modules/modern-media-controls/controls/layout-traits.js:46 > > + throw "Derived class must implement this function."; > > Instead of having every subclass override everything, I feel like some of > these could have a default implementation and only have some subclasses > override. Not a big deal tho. I much more prefer making all clients be explicit. > > > Source/WebCore/Modules/modern-media-controls/controls/layout-traits.js:97 > > + Fullscreen : 1 > > Style: missing trailing comma Ok. Will fix. > > > Source/WebCore/Modules/modern-media-controls/controls/macos-layout-traits.js:47 > > + return this.isFullscreen; > > I think this should be `return false;`. Why is that? > > > Source/WebCore/Modules/modern-media-controls/controls/time-control.js:43 > > + this.scrubber = new Slider("scrubber", this.layoutTraits.knobStyleForScrubber()); > > NIT: I think this should have a more specific name like > `knobStyleForTimeControlScrubber` so that it's not mistakenly thought to > also be used for the volume scrubber Ok. Will fix. > > > Source/WebCore/Modules/modern-media-controls/controls/watchos-layout-traits.js:50 > > + return false; > > I think this should be `return this.isFullscreen;`. Why is that? (I don't think this code will ever run actually, so it probably doesn't matter). > > > Source/WebCore/Modules/modern-media-controls/media/media-controller.js:102 > > + let LayoutTraitsClass = eval(this.host.layoutTraitsClassName); > > Is it absolutely necessary to use `eval`? I don't know if this will cause > issues (e.g. having a `<video>` inside an `<iframe>` that has CSP preventing > any `eval` (i.e. without `unsafe-eval`). I think the previous approach of > returning a string and using a `switch` was fine. I can't use a switch statement here as the whole point of this exercise was removing it! I don't want this code to know anything about the platforms that exist. > > > Source/WebCore/Modules/modern-media-controls/media/media-controller.js:212 > > + let overridenSupportingObjectClasses = this.layoutTraits.overridenSupportingObjectClasses(); > > NIT: instead of having `overridenSupportingObjectClasses` you could just > have each `LayoutTraits` return their own list of `supportingObjectClasses`. What would be the goal in that? > > > Source/WebCore/Modules/modern-media-controls/media/media-controller.js:333 > > + if (this.layoutTraits.controlsNeverAvailable()) > > NIT: I'd have restructured this as `if (this.isFullscreen && > !this.layoutTraits.controlsAllowedInFullscreen)` so that it's more > understandable what this means inside the `LayoutTraits` subclasses (and > isn't so completely opposite of `controlsAlwaysAvailable`). Perhaps I don't understand the intent of the code, but it seems like the predicates I added match the existing comments, so seem quite appropriate. // Controls are always available on watchOS. .... // Controls are always available while in fullscreen on macOS, and they are never available when in fullscreen on iOS. Is MediaController's fullscreen bit a better choice than the internal bit in LayoutTraits? > > > Source/WebCore/Modules/modern-media-controls/media/tracks-support.js:49 > > + promoteSubMenus: this.mediaController.layoutTraits.promoteSubMenusWhenShowingMediaControlsContextMenu(), > > NIT: I feel like this should have a name more specific to `TracksSupport` > (e.g. `promoteSubMenusWhenShowingTracksContextMenu`) as right now it reads > as a very general thing, which is not how it's used. Ok. Will fix.
Comment on attachment 432337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432337&action=review >>> Source/WebCore/Modules/modern-media-controls/media/media-controller.js:102 >>> + let LayoutTraitsClass = eval(this.host.layoutTraitsClassName); >> >> Is it absolutely necessary to use `eval`? I don't know if this will cause issues (e.g. having a `<video>` inside an `<iframe>` that has CSP preventing any `eval` (i.e. without `unsafe-eval`). I think the previous approach of returning a string and using a `switch` was fine. > > I can't use a switch statement here as the whole point of this exercise was removing it! I don't want this code to know anything about the platforms that exist. Ah, hmm, good point. Perhaps instead of using `eval` then you could just `window[this.host.layoutTraitsClassName]` (or maybe have each `LayoutTraits` subclass add itself to some `window.layoutTraitsClasses = {}` object that you then use here)? I'm really only concerned with using `eval`. >>> Source/WebCore/Modules/modern-media-controls/media/media-controller.js:212 >>> + let overridenSupportingObjectClasses = this.layoutTraits.overridenSupportingObjectClasses(); >> >> NIT: instead of having `overridenSupportingObjectClasses` you could just have each `LayoutTraits` return their own list of `supportingObjectClasses` > > What would be the goal in that? Instead of having the `LayoutTraits` subclass override the default list, each `LayoutTraits` subclass would just provide its own list entirely. I personally think it would read a little nicer as instead of having a list of overrides, you'd just have THE list. Tho I think as you've mentioned above this may have issues in testing code. >>> Source/WebCore/Modules/modern-media-controls/media/media-controller.js:333 >>> + if (this.layoutTraits.controlsNeverAvailable()) >> >> NIT: I'd have restructured this as `if (this.isFullscreen && !this.layoutTraits.controlsAllowedInFullscreen)` so that it's more understandable what this means inside the `LayoutTraits` subclasses (and isn't so completely opposite of `controlsAlwaysAvailable`). > > Perhaps I don't understand the intent of the code, but it seems like the predicates I added match the existing comments, so seem quite appropriate. > > // Controls are always available on watchOS. > > .... > > > // Controls are always available while in fullscreen on macOS, and they are never available when in fullscreen on iOS. > > > Is MediaController's fullscreen bit a better choice than the internal bit in LayoutTraits? My comment mainly concerned the fact that you have two functions `controlsAlwaysAvailable` and `controlsNeverAvailable` that seem to be the opposite of each other and wouldn't be clear inside the `LayoutTraits` subclass as to what they do and when they would used. When I initially read this I thought `controlsAlwaysAvailable` would always be the opposite of `controlsNeverAvailable` based on the naming. I realize now that your goal was something along the lines of having a function each for "conditions when controls are always available" and "conditions when controls are never available", but it didn't (and still doesn't) read that way to me in code. My suggestion of using `this.isFullscreen` here was only to make the naming of the two functions slightly different. I'm not sure what else to name these that better gets the point across tho, so I suppose it's fine to leave as-is (unless you have a better idea).
(In reply to Devin Rousso from comment #7) > Comment on attachment 432337 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=432337&action=review > > >>> Source/WebCore/Modules/modern-media-controls/media/media-controller.js:102 > >>> + let LayoutTraitsClass = eval(this.host.layoutTraitsClassName); > >> > >> Is it absolutely necessary to use `eval`? I don't know if this will cause issues (e.g. having a `<video>` inside an `<iframe>` that has CSP preventing any `eval` (i.e. without `unsafe-eval`). I think the previous approach of returning a string and using a `switch` was fine. > > > > I can't use a switch statement here as the whole point of this exercise was removing it! I don't want this code to know anything about the platforms that exist. > > Ah, hmm, good point. Perhaps instead of using `eval` then you could just > `window[this.host.layoutTraitsClassName]` (or maybe have each `LayoutTraits` > subclass add itself to some `window.layoutTraitsClasses = {}` object that > you then use here)? I'm really only concerned with using `eval`. The window access was the way I did it first, but I don't believe you can lookup constructors that way. If we want to do this, we can, we just need to add code to the various layout-trait subclasses at the end like, `const MacOSLayoutTraits = MacOSLayoutTraits` or something like that, to shadow the lexical declaration onto the window. I can make this change to remove the eval. > > >>> Source/WebCore/Modules/modern-media-controls/media/media-controller.js:212 > >>> + let overridenSupportingObjectClasses = this.layoutTraits.overridenSupportingObjectClasses(); > >> > >> NIT: instead of having `overridenSupportingObjectClasses` you could just have each `LayoutTraits` return their own list of `supportingObjectClasses` > > > > What would be the goal in that? > > Instead of having the `LayoutTraits` subclass override the default list, > each `LayoutTraits` subclass would just provide its own list entirely. I > personally think it would read a little nicer as instead of having a list of > overrides, you'd just have THE list. > > Tho I think as you've mentioned above this may have issues in testing code. Ok. I'll try it. > > >>> Source/WebCore/Modules/modern-media-controls/media/media-controller.js:333 > >>> + if (this.layoutTraits.controlsNeverAvailable()) > >> > >> NIT: I'd have restructured this as `if (this.isFullscreen && !this.layoutTraits.controlsAllowedInFullscreen)` so that it's more understandable what this means inside the `LayoutTraits` subclasses (and isn't so completely opposite of `controlsAlwaysAvailable`). > > > > Perhaps I don't understand the intent of the code, but it seems like the predicates I added match the existing comments, so seem quite appropriate. > > > > // Controls are always available on watchOS. > > > > .... > > > > > > // Controls are always available while in fullscreen on macOS, and they are never available when in fullscreen on iOS. > > > > > > Is MediaController's fullscreen bit a better choice than the internal bit in LayoutTraits? > > My comment mainly concerned the fact that you have two functions > `controlsAlwaysAvailable` and `controlsNeverAvailable` that seem to be the > opposite of each other and wouldn't be clear inside the `LayoutTraits` > subclass as to what they do and when they would used. When I initially read > this I thought `controlsAlwaysAvailable` would always be the opposite of > `controlsNeverAvailable` based on the naming. I realize now that your goal > was something along the lines of having a function each for "conditions when > controls are always available" and "conditions when controls are never > available", but it didn't (and still doesn't) read that way to me in code. > My suggestion of using `this.isFullscreen` here was only to make the naming > of the two functions slightly different. I'm not sure what else to name > these that better gets the point across tho, so I suppose it's fine to leave > as-is (unless you have a better idea). I think maybe combining them into a single function that returns a tri-state enumeration (show, don't show, fallthrough) would actually be the cleanest here. I'll have to think about the names a bit.