Bug 147181 – [iOS] Add an explicit API to allow media documents to (temporarily) play inline

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[iOS] Add an explicit API to allow media documents to (temporarily) play inline

[Jer Noble]

Created attachment 257240 [details]
Patch

[Darin Adler]

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]

(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]

Created attachment 257307 [details]
Patch

[Jer Noble]

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]

Created attachment 257308 [details]
Patch

[Jer Noble]

Created attachment 257363 [details]
Patch

[Beth Dakin]

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]

Committed r187251: <http://trac.webkit.org/changeset/187251>

[Alex Christensen]

It looks like you had partially renamed flag to allows.  Build fix committed to http://trac.webkit.org/changeset/187263