Bug 120811

Summary: Get MEDIA_STREAM compiling for other ports (EFL and GTK)
Product: WebKit Reporter: Thiago de Barros Lacerda <thiago.lacerda>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson, glenn, gyuyoung.kim, hta, hugo.lima, jer.noble, lauro.neto, luciano.wolf, rakuco, sergio, tommyw, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Thiago de Barros Lacerda 2013-09-05 16:44:20 PDT
Now these ports can be compiled with media-stream enabled
Comment 1 Thiago de Barros Lacerda 2013-09-05 16:53:10 PDT
Created attachment 210682 [details]
Patch
Comment 2 Thiago de Barros Lacerda 2013-09-05 17:12:10 PDT
Created attachment 210687 [details]
Patch
Comment 3 Eric Carlson 2013-09-05 17:32:07 PDT
Comment on attachment 210687 [details]
Patch

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

> Source/WebCore/Modules/mediastream/LocalMediaStream.h:39
> +#if PLATFORM(MAC)
>  class LocalMediaStream FINAL : public MediaStream {
> +#else
> +class LocalMediaStream : public MediaStream {
> +#endif

Why doesn't FINAL work for you?

> Source/WebCore/Modules/mediastream/MediaStreamRegistry.cpp:60
> +    RefPtr<MediaStreamDescriptor> descriptor = m_streamDescriptors.get(url);
> +    return descriptor.get();

This is wrong, for HashMap the peek type is the wrapped pointer so you can just remove the ".get()":

  return m_streamDescriptors.get(url)

> Source/WebCore/Modules/mediastream/RTCStatsResponse.h:46
> +#if PLATFORM(MAC)
>  class RTCStatsResponse FINAL : public RTCStatsResponseBase {
> +#else
> +class RTCStatsResponse : public RTCStatsResponseBase {
> +#endif

Why doesn't FINAL work here?
Comment 4 Thiago de Barros Lacerda 2013-09-05 20:08:20 PDT
(In reply to comment #3)
> (From update of attachment 210687 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210687&action=review
> 
> > Source/WebCore/Modules/mediastream/LocalMediaStream.h:39
> > +#if PLATFORM(MAC)
> >  class LocalMediaStream FINAL : public MediaStream {
> > +#else
> > +class LocalMediaStream : public MediaStream {
> > +#endif
> 
> Why doesn't FINAL work for you?

Because both LocalMediaStream and RTCStatsResponseBase in the end will be inherited by struct Base in JSDOMBinding.h (line 540)

> 
> > Source/WebCore/Modules/mediastream/MediaStreamRegistry.cpp:60
> > +    RefPtr<MediaStreamDescriptor> descriptor = m_streamDescriptors.get(url);
> > +    return descriptor.get();
> 
> This is wrong, for HashMap the peek type is the wrapped pointer so you can just remove the ".get()":
> 
>   return m_streamDescriptors.get(url)
> 
My first thought was doing this way you are telling, but I thought it was wrong. OK, I will change.

> > Source/WebCore/Modules/mediastream/RTCStatsResponse.h:46
> > +#if PLATFORM(MAC)
> >  class RTCStatsResponse FINAL : public RTCStatsResponseBase {
> > +#else
> > +class RTCStatsResponse : public RTCStatsResponseBase {
> > +#endif
> 
> Why doesn't FINAL work here?
Comment 5 Thiago de Barros Lacerda 2013-09-05 20:17:24 PDT
Created attachment 210697 [details]
Patch
Comment 6 Zan Dobersek 2013-09-06 02:34:32 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 210687 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=210687&action=review
> > 
> > > Source/WebCore/Modules/mediastream/LocalMediaStream.h:39
> > > +#if PLATFORM(MAC)
> > >  class LocalMediaStream FINAL : public MediaStream {
> > > +#else
> > > +class LocalMediaStream : public MediaStream {
> > > +#endif
> > 
> > Why doesn't FINAL work for you?
> 
> Because both LocalMediaStream and RTCStatsResponseBase in the end will be inherited by struct Base in JSDOMBinding.h (line 540)

I guess you mean the definition of the Base struct in the HasMemoryCostMemberFunction class?
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/JSDOMBinding.h#L613
Comment 7 Thiago de Barros Lacerda 2013-09-06 05:46:29 PDT
(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 210687 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=210687&action=review
> > > 
> > > > Source/WebCore/Modules/mediastream/LocalMediaStream.h:39
> > > > +#if PLATFORM(MAC)
> > > >  class LocalMediaStream FINAL : public MediaStream {
> > > > +#else
> > > > +class LocalMediaStream : public MediaStream {
> > > > +#endif
> > > 
> > > Why doesn't FINAL work for you?
> > 
> > Because both LocalMediaStream and RTCStatsResponseBase in the end will be inherited by struct Base in JSDOMBinding.h (line 540)
> 
> I guess you mean the definition of the Base struct in the HasMemoryCostMemberFunction class?
> http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/JSDOMBinding.h#L613

Yes, sorry about the confusion. It is line 624, inside  HasMemoryCostMemberFunction class
Comment 8 Eric Carlson 2013-09-06 08:58:21 PDT
Comment on attachment 210697 [details]
Patch

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

> Source/WebCore/Modules/mediastream/LocalMediaStream.h:39
> +#if PLATFORM(MAC)
>  class LocalMediaStream FINAL : public MediaStream {
> +#else
> +class LocalMediaStream : public MediaStream {
> +#endif

I think it is better to not have platform specific code here so I am fine with you just removing the FINAL.

> Source/WebCore/Modules/mediastream/RTCStatsResponse.h:46
> +#if PLATFORM(MAC)
>  class RTCStatsResponse FINAL : public RTCStatsResponseBase {
> +#else
> +class RTCStatsResponse : public RTCStatsResponseBase {
> +#endif

Ditto.
Comment 9 Thiago de Barros Lacerda 2013-09-06 11:02:38 PDT
Created attachment 210772 [details]
Patch
Comment 10 WebKit Commit Bot 2013-09-06 11:07:58 PDT
Comment on attachment 210772 [details]
Patch

Rejecting attachment 210772 [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', 210772, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.appspot.com/results/1705448
Comment 11 Thiago de Barros Lacerda 2013-09-06 11:15:25 PDT
Created attachment 210773 [details]
Patch
Comment 12 WebKit Commit Bot 2013-09-06 23:44:04 PDT
Comment on attachment 210773 [details]
Patch

Clearing flags on attachment: 210773

Committed r155248: <http://trac.webkit.org/changeset/155248>
Comment 13 WebKit Commit Bot 2013-09-06 23:44:08 PDT
All reviewed patches have been landed.  Closing bug.