Media elements not in a page shouldn't load

• Component: Media
• Status: RESOLVED FIXED

[Brady Eidson]

Media elements not in a page shouldn't load

[Brady Eidson]

Noticed while working on an unrelated patch.

Have no test to reproduce.

[Brady Eidson]

Created attachment 250727 [details]
Patch v1

[Jer Noble]

Comment on attachment 250727 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=250727&action=review

> Source/WebCore/html/HTMLMediaSession.cpp:146
> -    if (m_restrictions & RequirePageConsentToLoadMedia && page && !page->canStartMedia()) {
> +    if (!page || !page->canStartMedia() || m_restrictions & RequirePageConsentToLoadMedia) {

This should be: if (m_restrictions & RequirePageConsentToLoadMedia && (!page || !page->canStartMedia()))

[Brady Eidson]

(In reply to comment #3)
> Comment on attachment 250727 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250727&action=review
> 
> > Source/WebCore/html/HTMLMediaSession.cpp:146
> > -    if (m_restrictions & RequirePageConsentToLoadMedia && page && !page->canStartMedia()) {
> > +    if (!page || !page->canStartMedia() || m_restrictions & RequirePageConsentToLoadMedia) {
> 
> This should be: if (m_restrictions & RequirePageConsentToLoadMedia && (!page
> || !page->canStartMedia()))

Kinda.

The page checks should be first.

But the joining clause after them *should* be an or.

Thanks!

[Brady Eidson]

(In reply to comment #4)

> Kinda.
> 
> The page checks should be first.
> 
> But the joining clause after them *should* be an or.
> 

I meant it should be an and.  *sigh*

[Brady Eidson]

We found another one we discussed on IRC.

[Brady Eidson]

Created attachment 250729 [details]
Patch for landing

[WebKit Commit Bot]

Attachment 250729 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Created attachment 250731 [details]
Real patch for landing.

Disagreement on the media session parts - landing only the loadResource part.

[Darin Adler]

Comment on attachment 250727 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=250727&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:1093
> +    if (!frame->mainFrame().page()) {

Should just be frame->page().

[Brady Eidson]

(In reply to comment #10)
> Comment on attachment 250727 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250727&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:1093
> > +    if (!frame->mainFrame().page()) {
> 
> Should just be frame->page().

Yup, you're definitely correct. Brain dead copy mistake.

[Brady Eidson]

Created attachment 250734 [details]
Land it

[WebKit Commit Bot]

Comment on attachment 250734 [details]
Land it

Clearing flags on attachment: 250734

Committed r182810: <http://trac.webkit.org/changeset/182810>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.