Bug 83167 - MediaStream API: Deleting the chromium bridge class MediaStreamCenterInternal
Summary: MediaStream API: Deleting the chromium bridge class MediaStreamCenterInternal
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on:
Blocks: 56459
  Show dependency treegraph
 
Reported: 2012-04-04 08:40 PDT by Tommy Widenflycht
Modified: 2012-04-06 11:24 PDT (History)
3 users (show)

See Also:


Attachments
Patch (38.38 KB, patch)
2012-04-04 08:47 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (6.38 MB, application/zip)
2012-04-04 10:43 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-04 (6.37 MB, application/zip)
2012-04-04 11:37 PDT, WebKit Review Bot
no flags Details
Patch (39.38 KB, patch)
2012-04-05 04:29 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tommy Widenflycht 2012-04-04 08:40:56 PDT
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.
Comment 1 Tommy Widenflycht 2012-04-04 08:47:24 PDT
Created attachment 135600 [details]
Patch
Comment 2 WebKit Review Bot 2012-04-04 10:43:54 PDT
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
Comment 3 WebKit Review Bot 2012-04-04 10:43:59 PDT
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
Comment 4 Adam Barth 2012-04-04 10:50:55 PDT
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.
Comment 5 Adam Barth 2012-04-04 10:51:53 PDT
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.
Comment 6 WebKit Review Bot 2012-04-04 11:37:44 PDT
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
Comment 7 WebKit Review Bot 2012-04-04 11:37:50 PDT
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
Comment 8 Tommy Widenflycht 2012-04-05 04:20:39 PDT
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.
Comment 9 Tommy Widenflycht 2012-04-05 04:29:49 PDT
Created attachment 135802 [details]
Patch
Comment 10 Adam Barth 2012-04-06 10:15:20 PDT
Comment on attachment 135802 [details]
Patch

Thanks!
Comment 11 WebKit Review Bot 2012-04-06 11:24:13 PDT
Comment on attachment 135802 [details]
Patch

Clearing flags on attachment: 135802

Committed r113462: <http://trac.webkit.org/changeset/113462>
Comment 12 WebKit Review Bot 2012-04-06 11:24:33 PDT
All reviewed patches have been landed.  Closing bug.