Enabling the MEDIA_STREAM flag to be turned on
Created attachment 254834 [details] Patch
Created attachment 254836 [details] Patch
Needs rebasing?
Created attachment 255237 [details] Updated patch.
Attachment 255237 [details] did not pass style-queue: ERROR: ChangeLog:5: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 255238 [details] More style-conforming
Created attachment 255314 [details] Patch
Comment on attachment 255314 [details] Patch Build issues in the mock.
Created attachment 255359 [details] Patch
Created attachment 255363 [details] Patch
Created attachment 255365 [details] Patch
Comment on attachment 255365 [details] Patch Attachment 255365 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5267354325876736 New failing tests: js/dom/global-constructors-attributes.html
Created attachment 255371 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 255365 [details] Patch Attachment 255365 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5345718164258816 New failing tests: js/dom/global-constructors-attributes.html
Created attachment 255373 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
rdar://problem/21365829
Was modifying files, compiled correctly but was told to use Refs instead of RefPtrs to avoid possibilities of null references.
Created attachment 255417 [details] Patch
Attachment 255417 [details] did not pass style-queue: ERROR: Source/WebKit2/ChangeLog:18: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 255417 [details] Patch Attachment 255417 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6586288316612608 New failing tests: js/dom/global-constructors-attributes.html
Created attachment 255421 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 255417 [details] Patch Attachment 255417 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4870400563478528 New failing tests: js/dom/global-constructors-attributes.html
Created attachment 255422 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 255424 [details] Patch
Comment on attachment 255424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255424&action=review > ChangeLog:25 > + Added twice. > Source/WTF/ChangeLog:4 > + Changed ifdef declaration in FeatureDefines Our style is to move these ChangeLog specific comments to after the "Reviewed by" line. The script that puts together the final commit message combines these ChangeLogs together, and the way you have them formatted will look more verbose and confusing in the end. > Source/WTF/wtf/FeatureDefines.h:592 > #endif Can we remove this and instead update the FeatureDefines.xcconfig files? > Source/WebKit2/ChangeLog:-25 > - This shouldn't be here. > Source/WebKit/mac/ChangeLog:10 > + * WebCoreSupport/WebUserMediaClient.mm: Chagned refs, pointers, typo: Changed > Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:184 > + // Stub Please include a FIXME with a link to the bug that fills in the stub. > LayoutTests/platform/mac-mavericks/js/dom/global-constructors-attributes-expected.txt:820 > +PASS Object.getOwnPropertyDescriptor(global, 'MediaStreamTrackEvent').configurable is true Shouldn't all of this be in the mac results, not mac-mavericks?
Created attachment 255427 [details] Patch
Comment on attachment 255427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255427&action=review > ChangeLog:11 > + * WebKitBuild/Debug/DerivedSources/WebCore: Added. This ChangeLog shouldn't exist. > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:144 > +ENABLE_MEDIA_STREAM = ENABLE_MEDIA_STREAM; This needs to be added to the FEATURE_DEFINES below. There's also a missing ChangeLog file. Please make everything up to the Reviewed by line consistent with the other ChangeLogs. > Source/WTF/ChangeLog:7 > + Reviewed by NOBODY (OOPS!). Please make everything up to the Reviewed by line consistent with the other ChangeLogs. > Source/WebCore/ChangeLog:6 > + <rdar://problem/21365829> Please make everything up to the Reviewed by line consistent with the other ChangeLogs. > Source/WebCore/Configurations/FeatureDefines.xcconfig:144 > +ENABLE_MEDIA_STREAM = ENABLE_MEDIA_STREAM; This needs to be added to the FEATURE_DEFINES below. > Source/WebKit2/ChangeLog:3 > + Enable MEDIA_STREAM. Exported necessary functions to WEBCORE Please make everything up to the Reviewed by line consistent with the other ChangeLogs. > Source/WebKit/mac/ChangeLog:8 > + Missing Reviewed by. Please make everything up to the Reviewed by line consistent with the other ChangeLogs. > Source/WebKit/mac/Configurations/FeatureDefines.xcconfig:144 > +ENABLE_MEDIA_STREAM = ENABLE_MEDIA_STREAM; This needs to be added to the FEATURE_DEFINES below. > LayoutTests/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). Please make everything up to the Reviewed by line consistent with the other ChangeLogs.
Comment on attachment 255427 [details] Patch This looks close! Need to correct the FeatureDefines issue I showed you (in person), and the ChangeLog format we discussed. Also, make sure to get rid of the root ChangeLog update (the first item above).
Created attachment 255450 [details] Patch
Comment on attachment 255450 [details] Patch Great! Let's get this integrated.
Comment on attachment 255450 [details] Patch Attachment 255450 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5244379484651520 New failing tests: js/dom/global-constructors-attributes.html
Created attachment 255459 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 255450 [details] Patch Attachment 255450 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5027738113015808 New failing tests: js/dom/global-constructors-attributes.html
Created attachment 255461 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
The tests are still failing because you only updated the platform/mac test expectations. We need the same changes for mac-mavericks and mac-yosemite: bfulgham-macpro:platform bfulgham$ find . -name global-constructors-attributes-expected.txt ./efl/js/dom/global-constructors-attributes-expected.txt ./gtk/js/dom/global-constructors-attributes-expected.txt ./ios-sim-deprecated/js/dom/global-constructors-attributes-expected.txt ./mac/js/dom/global-constructors-attributes-expected.txt ./mac-mavericks/js/dom/global-constructors-attributes-expected.txt ./mac-yosemite/js/dom/global-constructors-attributes-expected.txt ./win/js/dom/global-constructors-attributes-expected.txt
Created attachment 255490 [details] Patch
Comment on attachment 255490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255490&action=review > LayoutTests/platform/mac-mavericks/js/dom/global-constructors-attributes-expected.txt:950 > +PASS Object.getOwnPropertyDescriptor(global, 'PerformanceTiming').configurable is true This shouldn't be here. > LayoutTests/platform/mac-mavericks/js/dom/global-constructors-attributes-expected.txt:2015 > +PASS Object.getOwnPropertyDescriptor(global, 'WebKitPlaybackTargetAvailabilityEvent').configurable is true This shouldn't be here. > LayoutTests/platform/mac-yosemite/js/dom/global-constructors-attributes-expected.txt:2015 > +PASS Object.getOwnPropertyDescriptor(global, 'WebKitPlaybackTargetAvailabilityEvent').configurable is true This shouldn't be here.
Comment on attachment 255490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255490&action=review > Source/WebCore/ChangeLog:259 > +>>>>>>> .r185915 Please remove > Source/WebCore/Modules/mediastream/UserMediaClient.h:47 > + virtual void requestPermission(Ref<WebCore::UserMediaRequest>&&) = 0; Why is the WebCore namespace prefix necessary? > Source/WebKit2/ChangeLog:13 > +015-06-24 Brady Eidson <beidson@apple.com> Looks like a bad merge. > Source/WebKit2/WebProcess/WebPage/WebPage.h:468 > + WEBCORE_EXPORT UserMediaPermissionRequestManager& userMediaPermissionRequestManager() { return m_userMediaPermissionRequestManager; } WebKit2 doesn't use this macro. > Source/WebKit2/WebProcess/WebPage/WebPage.h:1084 > + WEBCORE_EXPORT void didReceiveUserMediaPermissionDecision(uint64_t userMediaID, bool allowed); WebKit2 doesn't use this macro. > Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:172 > +- (void)denyOnlyThisRequest These two methods should be wrapped in PLATFORM(IOS). The whole file is already wrapped in #if ENABLE(MEDIA_STREAM). Why are they needed inside each method? > Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:179 > +#endif Since you're just trying to get everything to build in this patch, I'd just go with ASSERT_NOT_REACHED() until you get to the point where this needs to be implemented. > Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:184 > + // FIXME: https://bugs.webkit.org/show_bug.cgi?id=146245 Ditto. > Tools/TestWebKitAPI/Tests/WebKit2/UserMedia.cpp:21 > +#if WK_HAVE_C_SPI This is unfamiliar to me; where is it defined?
Comment on attachment 255490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255490&action=review >> Source/WebCore/ChangeLog:259 >> +>>>>>>> .r185915 > > Please remove Bad merge! Don't upload a patch with conflicting files! >> Source/WebCore/Modules/mediastream/UserMediaClient.h:47 >> + virtual void requestPermission(Ref<WebCore::UserMediaRequest>&&) = 0; > > Why is the WebCore namespace prefix necessary? I asked about this last night. Matt said he couldn't get it to build without the WebCore prefix. But perhaps he should try again. >> Source/WebKit2/WebProcess/WebPage/WebPage.h:468 >> + WEBCORE_EXPORT UserMediaPermissionRequestManager& userMediaPermissionRequestManager() { return m_userMediaPermissionRequestManager; } > > WebKit2 doesn't use this macro. This should be WK_EXPORT >> Source/WebKit2/WebProcess/WebPage/WebPage.h:1084 >> + WEBCORE_EXPORT void didReceiveUserMediaPermissionDecision(uint64_t userMediaID, bool allowed); > > WebKit2 doesn't use this macro. WK_EXPORT >> Tools/TestWebKitAPI/Tests/WebKit2/UserMedia.cpp:21 >> +#if WK_HAVE_C_SPI > > This is unfamiliar to me; where is it defined? Simon added this to Tools/TestWebKitAPI/config.h about a year ago. "Fix by removing some patterns from EXCLUDED_SOURCE_FILE_NAMES, and #ifdeffing source files instead. config.h defines WK_HAVE_C_SPI when the WebKit C SPI is available (i.e. Mac OS X), and most files use that. Some files with Mac-only functionality (ActionMenus, Downloads) use #if PLATFORM(MAC)."
Comment on attachment 255490 [details] Patch Attachment 255490 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5686711187144704 New failing tests: js/dom/global-constructors-attributes.html
Created attachment 255494 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 255490 [details] Patch Attachment 255490 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5104359255834624 New failing tests: js/dom/global-constructors-attributes.html
Created attachment 255496 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 255510 [details] Patch
Created attachment 255514 [details] Patch
Comment on attachment 255514 [details] Patch r=me.
Comment on attachment 255514 [details] Patch Rejecting attachment 255514 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 255514, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/Source/WebKit/mac/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-queues.appspot.com/results/6610107144077312
Created attachment 255524 [details] Patch
Comment on attachment 255524 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255524&action=review Missing some blank lines! r- > Source/WebCore/ChangeLog:5 > + <rdar://problem/21365829> You need a blank line here. > Source/WebKit2/ChangeLog:5 > + <rdar://problem/21365829> You need a blank line here. > LayoutTests/ChangeLog:5 > + <rdar://problem/21365829> You need a blank line here.
Created attachment 255525 [details] Patch
Comment on attachment 255525 [details] Patch r=me
Comment on attachment 255525 [details] Patch Rejecting attachment 255525 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 255525, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 185936 = c7267a958994970c7d7a5bf9039593f08f32d67b r185939 = 2b93f4d8f70db54fdc664cfc978985a0df5442f2 r185940 = 88c1510b3a4f3993307f404b1893426951ac05de Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: http://webkit-queues.appspot.com/results/6212078029242368
Committed r185956: <http://trac.webkit.org/changeset/185956>
(In reply to comment #53) > Committed r185956: <http://trac.webkit.org/changeset/185956> It made all tests crash on the Apple Mac bots. :-/
Re-opened since this is blocked by bug 146321
The crashes we saw on the main build system are surprising, given the clean EWS run. We might need a clean rebuild for this to work. I'm rolling it out for now to get the bots green again.
I'm seeing crashes with a local build, as well. I'm not sure why EWS claims that these tests are fine. Does EWS only run a subset of tests?
Matt, if you run tests locally in a Debug build, you see many crashes: [2250/36929] compositing/geometry/clipped-video-controller.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=36429]) [2253/36929] accessibility/aria-hidden-hides-all-elements.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=36404]) [2255/36929] compositing/images/direct-svg-image.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=36448]) [2324/36929] contentfiltering/block-after-add-data-then-allow-unblock.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=36411]) [3647/36929] css3/scroll-snap/add-element.html failed unexpectedly (test timed out, text diff) [3695/36929] css3/scroll-snap/dynamic-nested-elements.html failed unexpectedly (text diff) [3715/36929] contentfiltering/block-after-add-data-then-deny-unblock.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=36533]) [3720/36929] css3/scroll-snap/scroll-snap-mandatory-resizeable.html failed unexpectedly (text diff) [3851/36929] compositing/regions/floated-region-with-transformed-child.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=36443]) [3982/36929] css3/shapes/shape-outside/shape-image/shape-image-002.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=36457]) [4505/36929] accessibility/input-auto-fill-button.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=36527]) [4889/36929] css3/shapes/spec-examples/shape-outside-018.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=36406]) [5167/36929] contentfiltering/block-after-add-data.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=36551]) [5182/36929] css3/masking/mask-base64.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=36434]) [5637/36929] css3/flexbox/flexitem.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=36459]) [5637/36929] editing/input/password-echo-passnode2.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=36568]) Can you please replicate this on your system and figure out what's going on?
Created attachment 255794 [details] Patch
Created attachment 255800 [details] Patch
New patch today changed quite a bit. Needed to recompile, redo patch from scratch. Uploading new one.
Created attachment 255827 [details] Patch
Headers couldn't be found. Changed to private headers and re-uploaded.
Created attachment 255837 [details] Patch
Created attachment 255838 [details] Patch
Created attachment 255842 [details] Patch
Created attachment 255862 [details] Patch
Created attachment 255880 [details] Patch
Comment on attachment 255880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255880&action=review > Source/WebCore/ChangeLog:18 > + (WebCore::MediaPlayerPrivateAVFoundation::load): Deleted. > + * platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.h: > + (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::load): Deleted. > + * platform/graphics/mac/MediaPlayerPrivateQTKit.h: > + (WebCore::MediaPlayerPrivateQTKit::load): Deleted. It looks like these comments are incorrect. > Source/WebCore/ChangeLog:24 > + (WebCore::refreshCaptureDeviceList): Deleted. > + (WebCore::AVCaptureDeviceManager::bestSourceForTypeAndConstraints): Deleted. > + (WebCore::AVCaptureDeviceManager::deviceDisconnected): Deleted. Ditto. > Source/WebCore/ChangeLog:28 > + (WebCore::AVMediaCaptureSource::~AVMediaCaptureSource): Deleted. Ditto. > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:157 > + virtual void load(MediaStreamPrivate*) override { } Nit: "virtual" isn't necessary with "override" > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.h:99 > + virtual void load(MediaStreamPrivate*) override { } Ditto. > Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.h:90 > + virtual void load(MediaStreamPrivate*) override { } Ditto. > Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.h:49 > + virtual void validateRequestConstraints(PassRefPtr<MediaStreamCreationClient>, PassRefPtr<MediaConstraints> audioConstraints, PassRefPtr<MediaConstraints> videoConstraints) override; > + virtual void createMediaStream(PassRefPtr<MediaStreamCreationClient>, PassRefPtr<MediaConstraints> audioConstraints, PassRefPtr<MediaConstraints> videoConstraints) override; Ditto. > Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.h:48 > + virtual void requestPermission(Ref<WebCore::UserMediaRequest>&&) override; > + virtual void cancelRequest(WebCore::UserMediaRequest&) override; Ditto.
Comment on attachment 255880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255880&action=review >> Source/WebCore/ChangeLog:18 >> + (WebCore::MediaPlayerPrivateQTKit::load): Deleted. > > It looks like these comments are incorrect. Hm, I used prepare-ChangeLog to make these comments, and those were in the file when I started modifying them. Does this usually happen? >> Source/WebCore/ChangeLog:24 >> + (WebCore::AVCaptureDeviceManager::deviceDisconnected): Deleted. > > Ditto. Hm, I used prepare-ChangeLog to make these comments, and those were in the file when I started modifying them. Does this usually happen? >> Source/WebCore/ChangeLog:28 >> + (WebCore::AVMediaCaptureSource::~AVMediaCaptureSource): Deleted. > > Ditto. Hm, I used prepare-ChangeLog to make these comments, and those were in the file when I started modifying them. Does this usually happen? >> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:157 >> + virtual void load(MediaStreamPrivate*) override { } > > Nit: "virtual" isn't necessary with "override" Okay. Just wanted to make sure the code I was committing synced with the rest of the coding style of the class I was modifying >> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.h:99 >> + virtual void load(MediaStreamPrivate*) override { } > > Ditto. Okay. Just wanted to make sure the code I was committing synced with the rest of the coding style of the class I was modifying >> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.h:90 >> + virtual void load(MediaStreamPrivate*) override { } > > Ditto. Okay. Just wanted to make sure the code I was committing synced with the rest of the coding style of the class I was modifying >> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.h:49 >> + virtual void createMediaStream(PassRefPtr<MediaStreamCreationClient>, PassRefPtr<MediaConstraints> audioConstraints, PassRefPtr<MediaConstraints> videoConstraints) override; > > Ditto. Okay. Just wanted to make sure the code I was committing synced with the rest of the coding style of the class I was modifying >> Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.h:48 >> + virtual void cancelRequest(WebCore::UserMediaRequest&) override; > > Ditto. Okay. Just wanted to make sure the code I was committing synced with the rest of the coding style of the class I was modifying
Comment on attachment 255880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255880&action=review Responses to comments. >>> Source/WebCore/ChangeLog:18 >>> + (WebCore::MediaPlayerPrivateQTKit::load): Deleted. >> >> It looks like these comments are incorrect. > > Hm, I used prepare-ChangeLog to make these comments, and those were in the file when I started modifying them. Does this usually happen? Hm, I used prepare-ChangeLog to make these comments, and those were in the file when I started modifying them. Does this usually happen? >>> Source/WebCore/ChangeLog:24 >>> + (WebCore::AVCaptureDeviceManager::deviceDisconnected): Deleted. >> >> Ditto. > > Hm, I used prepare-ChangeLog to make these comments, and those were in the file when I started modifying them. Does this usually happen? Hm, I used prepare-ChangeLog to make these comments, and those were in the file when I started modifying them. Does this usually happen? >>> Source/WebCore/ChangeLog:28 >>> + (WebCore::AVMediaCaptureSource::~AVMediaCaptureSource): Deleted. >> >> Ditto. > > Hm, I used prepare-ChangeLog to make these comments, and those were in the file when I started modifying them. Does this usually happen? Hm, I used prepare-ChangeLog to make these comments, and those were in the file when I started modifying them. Does this usually happen? >>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:157 >>> + virtual void load(MediaStreamPrivate*) override { } >> >> Nit: "virtual" isn't necessary with "override" > > Okay. Just wanted to make sure the code I was committing synced with the rest of the coding style of the class I was modifying Okay. Just wanted to make sure the code I was committing synced with the rest of the coding style of the class I was modifying >>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.h:99 >>> + virtual void load(MediaStreamPrivate*) override { } >> >> Ditto. > > Okay. Just wanted to make sure the code I was committing synced with the rest of the coding style of the class I was modifying Okay. Just wanted to make sure the code I was committing synced with the rest of the coding style of the class I was modifying >>> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.h:90 >>> + virtual void load(MediaStreamPrivate*) override { } >> >> Ditto. > > Okay. Just wanted to make sure the code I was committing synced with the rest of the coding style of the class I was modifying Okay. Just wanted to make sure the code I was committing synced with the rest of the coding style of the class I was modifying >>> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.h:49 >>> + virtual void createMediaStream(PassRefPtr<MediaStreamCreationClient>, PassRefPtr<MediaConstraints> audioConstraints, PassRefPtr<MediaConstraints> videoConstraints) override; >> >> Ditto. > > Okay. Just wanted to make sure the code I was committing synced with the rest of the coding style of the class I was modifying Okay. Just wanted to make sure the code I was committing synced with the rest of the coding style of the class I was modifying >>> Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.h:48 >>> + virtual void cancelRequest(WebCore::UserMediaRequest&) override; >> >> Ditto. > > Okay. Just wanted to make sure the code I was committing synced with the rest of the coding style of the class I was modifying Okay. Just wanted to make sure the code I was committing synced with the rest of the coding style of the class I was modifying
(In reply to comment #71) > Comment on attachment 255880 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255880&action=review > > Responses to comments. > > >>> Source/WebCore/ChangeLog:18 > >>> + (WebCore::MediaPlayerPrivateQTKit::load): Deleted. > >> > >> It looks like these comments are incorrect. > > > > Hm, I used prepare-ChangeLog to make these comments, and those were in the file when I started modifying them. Does this usually happen? > It sounds like a bug in prepare-ChangeLog!
Created attachment 255927 [details] Patch
Comment on attachment 255880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255880&action=review >>>>> Source/WebCore/ChangeLog:18 >>>>> + (WebCore::MediaPlayerPrivateQTKit::load): Deleted. >>>> >>>> It looks like these comments are incorrect. >>> >>> Hm, I used prepare-ChangeLog to make these comments, and those were in the file when I started modifying them. Does this usually happen? >> >> Hm, I used prepare-ChangeLog to make these comments, and those were in the file when I started modifying them. Does this usually happen? > > It sounds like a bug in prepare-ChangeLog! Prepare-ChangeLog isn't smart enough to remove lines from the ChangeLog once you've edited it. I think these lines were correct when you first ran prepare-ChangeLog, but after you modified your patch a few times they were no longer relevant. >>>> Source/WebCore/ChangeLog:24 >>>> + (WebCore::AVCaptureDeviceManager::deviceDisconnected): Deleted. >>> >>> Ditto. >> >> Hm, I used prepare-ChangeLog to make these comments, and those were in the file when I started modifying them. Does this usually happen? > > Hm, I used prepare-ChangeLog to make these comments, and those were in the file when I started modifying them. Does this usually happen? Ditto. >>>> Source/WebCore/ChangeLog:28 >>>> + (WebCore::AVMediaCaptureSource::~AVMediaCaptureSource): Deleted. >>> >>> Ditto. >> >> Hm, I used prepare-ChangeLog to make these comments, and those were in the file when I started modifying them. Does this usually happen? > > Hm, I used prepare-ChangeLog to make these comments, and those were in the file when I started modifying them. Does this usually happen? Ditto
Created attachment 255930 [details] Patch
Comment on attachment 255930 [details] Patch This looks good, thanks for persevering! Marking r+, but I will wait to make sure the gtk bot is happy with it before marking it cq+
Comment on attachment 255930 [details] Patch gtk is happy as well. r+ for commit-queue.
Comment on attachment 255930 [details] Patch Clearing flags on attachment: 255930 Committed r186182: <http://trac.webkit.org/changeset/186182>
All reviewed patches have been landed. Closing bug.
Build fix for Mavericks with Xcode 5.0.2 in http://trac.webkit.org/changeset/186233