Bug 120811 - Get MEDIA_STREAM compiling for other ports (EFL and GTK)
Summary: Get MEDIA_STREAM compiling for other ports (EFL and GTK)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-05 16:44 PDT by Thiago de Barros Lacerda
Modified: 2013-09-06 23:44 PDT (History)
13 users (show)

See Also:


Attachments
Patch (5.73 KB, patch)
2013-09-05 16:53 PDT, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Patch (5.79 KB, patch)
2013-09-05 17:12 PDT, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Patch (5.74 KB, patch)
2013-09-05 20:17 PDT, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Patch (5.59 KB, patch)
2013-09-06 11:02 PDT, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Patch (5.62 KB, patch)
2013-09-06 11:15 PDT, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.