Bug 145947 – Enable MEDIA_STREAM flag to be turned on

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Matthew Daiter]

Enabling the MEDIA_STREAM flag to be turned on

[Matthew Daiter]

Created attachment 254834 [details]
Patch

[Matthew Daiter]

Created attachment 254836 [details]
Patch

[Jon Lee]

Needs rebasing?

[Matthew Daiter]

Created attachment 255237 [details]
Updated patch.

[WebKit Commit Bot]

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.

[Matthew Daiter]

Created attachment 255238 [details]
More style-conforming

[Matthew Daiter]

Created attachment 255314 [details]
Patch

[Jon Lee]

Comment on attachment 255314 [details]
Patch

Build issues in the mock.

[Matthew Daiter]

Created attachment 255359 [details]
Patch

[Matthew Daiter]

Created attachment 255363 [details]
Patch

[Matthew Daiter]

Created attachment 255365 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Matthew Daiter]

rdar://problem/21365829

[Matthew Daiter]

Was modifying files, compiled correctly but was told to use Refs instead of RefPtrs to avoid possibilities of null references.

[Matthew Daiter]

Created attachment 255417 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Matthew Daiter]

Created attachment 255424 [details]
Patch

[Jon Lee]

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?

[Matthew Daiter]

Created attachment 255427 [details]
Patch

[Jon Lee]

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.

[Brent Fulgham]

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).

[Matthew Daiter]

Created attachment 255450 [details]
Patch

[Brent Fulgham]

Comment on attachment 255450 [details]
Patch

Great! Let's get this integrated.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Brent Fulgham]

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

[Matthew Daiter]

Created attachment 255490 [details]
Patch

[Jon Lee]

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.

[Jon Lee]

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  <[e-mail]>

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?

[Brent Fulgham]

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)."

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Matthew Daiter]

Created attachment 255510 [details]
Patch

[Matthew Daiter]

Created attachment 255514 [details]
Patch

[Brent Fulgham]

Comment on attachment 255514 [details]
Patch

r=me.

[WebKit Commit Bot]

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

[Matthew Daiter]

Created attachment 255524 [details]
Patch

[Brent Fulgham]

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.

[Matthew Daiter]

Created attachment 255525 [details]
Patch

[Brent Fulgham]

Comment on attachment 255525 [details]
Patch

r=me

[WebKit Commit Bot]

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

[Brent Fulgham]

Committed r185956: <http://trac.webkit.org/changeset/185956>

[Csaba Osztrogonác]

(In reply to comment #53)
> Committed r185956: <http://trac.webkit.org/changeset/185956>

It made all tests crash on the Apple Mac bots. :-/

[WebKit Commit Bot]

Re-opened since this is blocked by bug 146321

[Brent Fulgham]

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.

[Brent Fulgham]

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?

[Brent Fulgham]

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?

[Matthew Daiter]

Created attachment 255794 [details]
Patch

[Matthew Daiter]

Created attachment 255800 [details]
Patch

[Matthew Daiter]

New patch today changed quite a bit. Needed to recompile, redo patch from scratch. Uploading new one.

[Matthew Daiter]

Created attachment 255827 [details]
Patch

[Matthew Daiter]

Headers couldn't be found. Changed to private headers and re-uploaded.

[Matthew Daiter]

Created attachment 255837 [details]
Patch

[Matthew Daiter]

Created attachment 255838 [details]
Patch

[Matthew Daiter]

Created attachment 255842 [details]
Patch

[Matthew Daiter]

Created attachment 255862 [details]
Patch

[Matthew Daiter]

Created attachment 255880 [details]
Patch

[Eric Carlson]

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.

[Matthew Daiter]

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

[Matthew Daiter]

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

[Eric Carlson]

(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!

[Matthew Daiter]

Created attachment 255927 [details]
Patch

[Brent Fulgham]

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

[Matthew Daiter]

Created attachment 255930 [details]
Patch

[Eric Carlson]

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+

[Brent Fulgham]

Comment on attachment 255930 [details]
Patch

gtk is happy as well. r+ for commit-queue.

[WebKit Commit Bot]

Comment on attachment 255930 [details]
Patch

Clearing flags on attachment: 255930

Committed r186182: <http://trac.webkit.org/changeset/186182>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

Build fix for Mavericks with Xcode 5.0.2 in http://trac.webkit.org/changeset/186233