Bug 83167 – MediaStream API: Deleting the chromium bridge class MediaStreamCenterInternal

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tommy Widenflycht]

To continue the Platform/ refactoring I have deleted MediaStreamCenterInternal. To successfully to this I had to make MediaStreamCenter a pure virtual class and let chromium and gstreamer inherit from it. Still a more elegant solution that the extra bridge.

[Tommy Widenflycht]

Created attachment 135600 [details]
Patch

[WebKit Review Bot]

Comment on attachment 135600 [details]
Patch

Attachment 135600 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12320969

New failing tests:
platform/chromium/media/video-capture-preview.html

[WebKit Review Bot]

Created attachment 135624 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adam Barth]

Comment on attachment 135600 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No actual code changes.

I might say "no behavior changes" rather than no "code" changes.  There are code changes.  :)

> Source/WebCore/platform/mediastream/chromium/MediaStreamCenterChromium.h:38
> +#include <public/WebMediaStreamCenterClient.h>

This will be the first time we've put a header like this into a header, but I think it should work as long as no one outside of WebCore/platform ever includes this header directly.  Given the way you've set things up, that seems like it should be the case.

[Adam Barth]

Comment on attachment 135600 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No actual code changes.
> 
> I might say "no behavior changes" rather than no "code" changes.  There are code changes.  :)

I might also add some more information to the ChangeLog about why you're making this change and any trade-offs you debated while writing it.

[WebKit Review Bot]

Comment on attachment 135600 [details]
Patch

Attachment 135600 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12321973

New failing tests:
platform/chromium/media/video-capture-preview.html

[WebKit Review Bot]

Created attachment 135641 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Tommy Widenflycht]

Comment on attachment 135600 [details]
Patch

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

>>> Source/WebCore/ChangeLog:8
>>> +        No actual code changes.
>> 
>> I might say "no behavior changes" rather than no "code" changes.  There are code changes.  :)
> 
> I might also add some more information to the ChangeLog about why you're making this change and any trade-offs you debated while writing it.

Fixed * 2.

>> Source/WebCore/platform/mediastream/chromium/MediaStreamCenterChromium.h:38
>> +#include <public/WebMediaStreamCenterClient.h>
> 
> This will be the first time we've put a header like this into a header, but I think it should work as long as no one outside of WebCore/platform ever includes this header directly.  Given the way you've set things up, that seems like it should be the case.

Yeah, it works and all chromium header usage is hidden away in platform/chromium.

[Tommy Widenflycht]

Created attachment 135802 [details]
Patch

[Adam Barth]

Comment on attachment 135802 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 135802 [details]
Patch

Clearing flags on attachment: 135802

Committed r113462: <http://trac.webkit.org/changeset/113462>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.