Summary: | [Media] Audio content shouldn't have fullscreen buttons, even if in a video element | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dean Jackson <dino> | ||||
Component: | New Bugs | Assignee: | Dean Jackson <dino> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | calvaris, cmarcelo, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, kangil.han, philipj, sergio | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Dean Jackson
2014-12-02 14:41:30 PST
Created attachment 242467 [details]
Patch
Comment on attachment 242467 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242467&action=review > Source/WebCore/ChangeLog:36 > +2014-12-02 Dean Jackson <dino@apple.com> > + > + 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 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. Ah, I didn't actually commit those other changes. Ignore them, they'll land separately (and are a completely different bug). Committed r176714: <http://trac.webkit.org/changeset/176714> Thanks for considering the Base.js also to fix this! |