Bug 139200 – [Media] Audio content shouldn't have fullscreen buttons, even if in a video element

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

[Dean Jackson]

[Media] Audio content shouldn't have fullscreen buttons, even if in a video element

[Dean Jackson]

Created attachment 242467 [details]
Patch

[Dean Jackson]

        <rdar://problem/18914506>

[Eric Carlson]

Comment on attachment 242467 [details]
Patch

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

> Source/WebCore/ChangeLog:36
> [phone]  Dean Jackson  <[e-mail]>
> +
> +        Missing support for innerHTML on SVGElement
> +        https://bugs.webkit.org/show_bug.cgi?id=136903
> +
> +        Unreviewed followup from r176630. Minor style nits that I missed in my review.
> +
> +        * dom/Element.h: Remove unnecessary parameter name.
> +        * html/parser/HTMLTreeBuilder.cpp: Whitespace cleanup.
> +        (WebCore::HTMLTreeBuilder::adjustedCurrentStackItem):

Double ChangeLog :-O

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:559
> +        if (this.video.videoTracks && this.video.videoTracks.length > 0)
> +            this.hasVisualMedia = true;

Is there any reason to not do this as "this.hasVisualMedia = this.video.videoTracks && this.video.videoTracks.length > 0"?

> Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:559
> +        if (this.video.videoTracks && this.video.videoTracks.length > 0)
> +            this.hasVisualMedia = true;

Ditto.

> Source/WebCore/dom/Element.h:569
> -    void clearTabIndexExplicitlyIfNeeded();    
> +    void clearTabIndexExplicitlyIfNeeded();

You probably don't want these changes in this patch.

> Source/WebCore/dom/Element.h:580
> -    
> -    static void mergeWithNextTextNode(Text& node, ExceptionCode& ec);
> +
> +    static void mergeWithNextTextNode(Text& node, ExceptionCode&);

Ditto.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2842
> -    
> +

Ditto

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2849
> -        
> +

Ditto

[Dean Jackson]

Comment on attachment 242467 [details]
Patch

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

>> Source/WebCore/ChangeLog:36
>> +        (WebCore::HTMLTreeBuilder::adjustedCurrentStackItem):
> 
> Double ChangeLog :-O

Oops. I'm not sure why that was included in the patch. It's already landed, I hope.

>> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:559
>> +            this.hasVisualMedia = true;
> 
> Is there any reason to not do this as "this.hasVisualMedia = this.video.videoTracks && this.video.videoTracks.length > 0"?

Because I'm a moron!! :)

>> Source/WebCore/dom/Element.h:569
>> +    void clearTabIndexExplicitlyIfNeeded();
> 
> You probably don't want these changes in this patch.

Yeah, that's the patch for another bug that should already be landed.

[Dean Jackson]

Ah, I didn't actually commit those other changes. Ignore them, they'll land separately (and are a completely different bug).

[Dean Jackson]

Committed r176714: <http://trac.webkit.org/changeset/176714>

[Xabier Rodríguez Calvar]

Thanks for considering the Base.js also to fix this!