Summary: | Provide preference to enable additional AVFoundation options | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||||||
Component: | Media | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | bfulgham, calvaris, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, philipj, sergio | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
Brent Fulgham
2014-03-14 17:37:09 PDT
Created attachment 226802 [details]
Patch
Comment on attachment 226802 [details]
Patch
Initial WIP. Checking Windows build.
Created attachment 226810 [details]
Patch
Comment on attachment 226810 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226810&action=review Where's WebKit2? > Source/WebCore/html/HTMLMediaElement.cpp:5986 > + return true; This logic, while correct, seems a bit odd to me. Why check for the attribute first before the setting, rather than the other way around? > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:2 > + * Copyright (C) 2011-2014 Apple Inc. All rights reserved. I think proper style was to list individual years separately than a range…? > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:589 > + Please remove. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:617 > + [options.get() setObject: [NSNumber numberWithBool: TRUE] forKey: AVURLAssetInheritURIQueryComponentFromReferencingURIKey]; no space after ":" Created attachment 226824 [details]
Updated to handle Windows.
Comment on attachment 226810 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226810&action=review >> Source/WebCore/html/HTMLMediaElement.cpp:5986 >> + return true; > > This logic, while correct, seems a bit odd to me. Why check for the attribute first before the setting, rather than the other way around? After talking with Jer, we decided to make this a generic method that could check for arbitrary attributes, not just this specific case we are dealing with today. If the attribute doesn't exist, it doesn't really matter if the setting is turned off since the result is the same. But if the setting does exist, we only need to check the setting for this specific URI case. I'm not strongly attached to this; I could switch the order if you prefer. >> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:2 >> + * Copyright (C) 2011-2014 Apple Inc. All rights reserved. > > I think proper style was to list individual years separately than a range…? Darin told me in a recent review that we should use ranges. > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:255 > +static CFStringRef AVCFURLAssetInheritURIQueryComponentFromReferencingURIKey() Note: This isn't needed and is removed in an updated patch. >> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:589 >> + > > Please remove. Will do. >> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:617 >> + [options.get() setObject: [NSNumber numberWithBool: TRUE] forKey: AVURLAssetInheritURIQueryComponentFromReferencingURIKey]; > > no space after ":" Will do. (In reply to comment #4) > (From update of attachment 226810 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=226810&action=review > > Where's WebKit2? I'll review this on Mac after the servers are back up. I believe the preference logic is auto generated based on the Settings.in file. Created attachment 226875 [details]
Patch
Created attachment 226878 [details]
Patch
Comment on attachment 226878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226878&action=review > Source/WebCore/platform/graphics/MediaPlayer.h:251 > + virtual bool doesHaveAttribute(const QualifiedName&) const { return false; } This is a layering violation, QualifiedName is in WebCore/dom. > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:39 > +#include "HTMLNames.h" This can't be included from platform/ > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1339 > + bool value = true; Nit: why not reuse inheritURI? > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:43 > +#import "HTMLNames.h" Layering. Created attachment 226935 [details]
Patch
Comment on attachment 226935 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226935&action=review WIP patch to move changes from Mac to Windows. > Source/WebCore/platform/graphics/MediaPlayer.h:142 > +class QualifiedName; Whoops -- need to remove this. > Source/WebKit/win/WebView.cpp:5054 > + hr = prefsPrivate->isInheritURIQueryComponentEnabled(&enabled); // OOPS! Don't checkin Oops 2. Comment on attachment 226935 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226935&action=review > Source/WebCore/html/HTMLMediaElement.cpp:5989 > + Nit: unnecessary white space. > Source/WebCore/platform/graphics/MediaPlayer.cpp:1414 > + Ditto. > Source/WebCore/platform/graphics/MediaPlayer.h:303 > + Ditto. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:615 > + Ditto. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:618 > + Ditto. > Source/WebKit2/Shared/WebPreferencesStore.h:218 > + Ditto. > Source/WebKit2/UIProcess/API/C/WKPreferencesPrivate.h:324 > + Ditto. > Source/WebKit2/UIProcess/API/C/WKPreferencesPrivate.h:328 > + Ditto. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2628 > + Ditto. > Source/WebKit/mac/WebView/WebView.mm:2394 > + Ditto. Created attachment 226945 [details]
Finalized after testing.
Comment on attachment 226935 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226935&action=review >> Source/WebCore/html/HTMLMediaElement.cpp:5989 >> + > > Nit: unnecessary white space. Looks like I have a bad Xcode editor preference set. I'll fix these when landing. Committed r165753: <http://trac.webkit.org/changeset/165753> |