RESOLVED FIXED 147181
[iOS] Add an explicit API to allow media documents to (temporarily) play inline
https://bugs.webkit.org/show_bug.cgi?id=147181
Summary [iOS] Add an explicit API to allow media documents to (temporarily) play inline
Jer Noble
Reported 2015-07-21 19:08:30 PDT
[iOS] Add an explicit API to allow media documents to (temporarily) play inline
Attachments
Patch (15.72 KB, patch)
2015-07-21 19:42 PDT, Jer Noble
no flags
Patch (15.79 KB, patch)
2015-07-22 16:23 PDT, Jer Noble
no flags
Patch (15.71 KB, patch)
2015-07-22 16:52 PDT, Jer Noble
no flags
Patch (15.78 KB, patch)
2015-07-23 12:35 PDT, Jer Noble
bdakin: review+
Jer Noble
Comment 1 2015-07-21 19:42:13 PDT
Darin Adler
Comment 2 2015-07-22 14:13:53 PDT
Comment on attachment 257240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257240&action=review So much plumbing! > Source/WebCore/dom/Document.h:1019 > + void registerForAllowMediaDocumentInlinePlaybackChangedCallbacks(HTMLMediaElement*); > + void unregisterForAllowMediaDocumentInlinePlaybackChangedCallbacks(HTMLMediaElement*); These should take HTMLMediaElement&; > Source/WebCore/page/Page.h:464 > + WEBCORE_EXPORT bool allowMediaDocumentInlinePlayback() const { return m_allowMediaDocumentInlinePlayback; } This function name is a verb phrase, so sounds like it “allows” something. But really it just returns a boolean. That’s why so many Cocoa names of methods like this function use the word “should” or something like that to make it sound like a boolean predicate and not a verb. > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:215 > +@property (nonatomic, getter=_allowMediaDocumentInlinePlayback, setter=_setAllowMediaDocumentInlinePlayback:) BOOL _allowMediaDocumentInlinePlayback; This function name is a verb phrase, so sounds like it will “allow” something. But really it just returns a boolean. That’s why so many Cocoa names of methods like this function use the word “should” or something like that to make it sound like a boolean predicate and not a verb. Or at least “allows” by analogy with “requires” on the line above.
Jer Noble
Comment 3 2015-07-22 15:54:42 PDT
(In reply to comment #2) > Comment on attachment 257240 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=257240&action=review > > So much plumbing! I know!! :-/ > > Source/WebCore/dom/Document.h:1019 > > + void registerForAllowMediaDocumentInlinePlaybackChangedCallbacks(HTMLMediaElement*); > > + void unregisterForAllowMediaDocumentInlinePlaybackChangedCallbacks(HTMLMediaElement*); > > These should take HTMLMediaElement&; Roger. > > Source/WebCore/page/Page.h:464 > > + WEBCORE_EXPORT bool allowMediaDocumentInlinePlayback() const { return m_allowMediaDocumentInlinePlayback; } > > This function name is a verb phrase, so sounds like it “allows” something. > But really it just returns a boolean. That’s why so many Cocoa names of > methods like this function use the word “should” or something like that to > make it sound like a boolean predicate and not a verb. Yeah; allows...? doesAllow...? shouldAllow...? > > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:215 > > +@property (nonatomic, getter=_allowMediaDocumentInlinePlayback, setter=_setAllowMediaDocumentInlinePlayback:) BOOL _allowMediaDocumentInlinePlayback; > > This function name is a verb phrase, so sounds like it will “allow” > something. But really it just returns a boolean. That’s why so many Cocoa > names of methods like this function use the word “should” or something like > that to make it sound like a boolean predicate and not a verb. Or at least > “allows” by analogy with “requires” on the line above. I'll pick a more appropriate name here and post a new patch.
Jer Noble
Comment 4 2015-07-22 16:23:17 PDT
Jer Noble
Comment 5 2015-07-22 16:24:28 PDT
I went with 'allows' rather than 'shouldAllow' or 'doesAllow', because the property is already super long without an extra non-imperative verb on the front.
Jer Noble
Comment 6 2015-07-22 16:52:58 PDT
Jer Noble
Comment 7 2015-07-23 12:35:37 PDT
Beth Dakin
Comment 8 2015-07-23 14:04:56 PDT
Comment on attachment 257363 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257363&action=review > Source/WebCore/html/HTMLMediaElement.cpp:-3885 > - Unnecessary! > Source/WebKit2/UIProcess/WebPageProxy.cpp:3991 > +void WebPageProxy::setAllowsMediaDocumentInlinePlayback(bool flag) I would prefer a more descriptive variable name than 'flag.' > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3011 > +void WebPage::setAllowsMediaDocumentInlinePlayback(bool flag) Boo flag > Source/WebKit2/WebProcess/WebPage/WebPage.messages.in:98 > + SetAllowsMediaDocumentInlinePlayback(bool flag) Boooooooo
Jer Noble
Comment 9 2015-07-23 14:16:52 PDT
Alex Christensen
Comment 10 2015-07-23 15:31:57 PDT
It looks like you had partially renamed flag to allows. Build fix committed to http://trac.webkit.org/changeset/187263
Note You need to log in before you can comment on or make changes to this bug.