Bug 83167

Summary: MediaStream API: Deleting the chromium bridge class MediaStreamCenterInternal
Product: WebKit Reporter: Tommy Widenflycht <tommyw>
Component: WebCore Misc.Assignee: Tommy Widenflycht <tommyw>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, fischman, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 56459    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ec2-cr-linux-01
none
Archive of layout-test-results from ec2-cr-linux-04
none
Patch none

Tommy Widenflycht
Reported 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.
Attachments
Patch (38.38 KB, patch)
2012-04-04 08:47 PDT, Tommy Widenflycht 		no flags
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Tommy Widenflycht
Comment 1 2012-04-04 08:47:24 PDT
Created attachment 135600 [details] Patch
WebKit Review Bot
Comment 2 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
WebKit Review Bot
Comment 3 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
Adam Barth
Comment 4 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.
Adam Barth
Comment 5 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.
WebKit Review Bot
Comment 6 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
WebKit Review Bot
Comment 7 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
Tommy Widenflycht
Comment 8 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.
Tommy Widenflycht
Comment 9 2012-04-05 04:29:49 PDT
Created attachment 135802 [details] Patch
Adam Barth
Comment 10 2012-04-06 10:15:20 PDT
Comment on attachment 135802 [details] Patch Thanks!
WebKit Review Bot
Comment 11 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>
WebKit Review Bot
Comment 12 2012-04-06 11:24:33 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.