Bug 146257 – Building MediaDeviceInfo for enumerateDevices

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

[Matthew Daiter]

Needed to create MediaDeviceInfo files in Javascript to ensure that methods like enumarateDevices can work.

[Matthew Daiter]

(In reply to comment #0)
> Needed to create MediaDeviceInfo files in Javascript to ensure that methods
> like enumarateDevices can work.

Should be "MediaDeviceInfo class in Javascript"

[Radar WebKit Bug Importer]

<rdar://problem/21513462>

[Matthew Daiter]

Created attachment 255475 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 255475 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaDeviceInfo.cpp:14
> +#include "ContextDescructionObserver.h"

typo: ContextDes_c_ructionObserver : c -> t

How come the build worked on Mac?

> Source/WebCore/Modules/mediastream/MediaDeviceInfo.h:14
> +#include "ContextDescructionObserver.h"

ditto

> Source/WebCore/Modules/mediastream/MediaDeviceInfo.h:21
> +class MediaDeviceInfo : public RefCounted<MediaDeviceInfo>, public ScriptWrappable, public ContextDescructionObserver {

ditto

[Matthew Daiter]

Created attachment 255481 [details]
Patch

[Matthew Daiter]

Sorry, was typo. Fixed now.

[Eric Carlson]

Comment on attachment 255481 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaDeviceInfo.h:39
> +    const String& m_label;
> +    const String& m_deviceId;
> +    const String& m_groupId;
> +    const String& m_kind;

Do you really want these to be const string *references*?

[Csaba Osztrogonác]

The EFL/GTK build is still broken:
Source/WebCore/Modules/mediastream/MediaDeviceInfo.h:40:1: error: expected ';' after class definition

Please fix them before landing. How come Mac build isn't broken?
Have you tested with ENABLE(MEDIA_STREAM) == true configuration?

[Matthew Daiter]

Created attachment 255484 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 255484 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Adding MediaDeviceInfo.

It should be exactly the title of the bug report: "Building MediaDeviceInfo for enumerateDevices"

> Source/WebCore/Modules/mediastream/MediaDeviceInfo.cpp:7
> +//
> +//  MediaDeviceInfo.cpp
> +//  WebCore
> +//
> +//  Created by Matt Daiter on 6/23/15.
> +//
> +//

Please remove these lines and add proper copyright and license here.

> Source/WebCore/Modules/mediastream/MediaDeviceInfo.h:7
> +//
> +//  MediaDeviceInfo.h
> +//  WebCore
> +//
> +//  Created by Matt Daiter on 6/23/15.
> +//
> +//

ditto

[Matthew Daiter]

Created attachment 255488 [details]
Patch

[Eric Carlson]

Comment on attachment 255488 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaDeviceInfo.h:38
> +class MediaDeviceInfo : public RefCounted<MediaDeviceInfo>, public ScriptWrappable, public ContextDestructionObserver {

Why does this class inherit from ContextDestructionObserver, will it need to override contextDestroyed() later?

> Source/WebCore/Modules/mediastream/MediaDeviceInfo.h:42
> +    

Nit: this extra blank line isn't necessary.

> Source/WebCore/Modules/mediastream/MediaDeviceInfo.idl:2
> +[
> +NoInterfaceObject,

.idl files need a copyright header too.

> Source/WebCore/Modules/mediastream/MediaDeviceInfo.idl:10
> +    readonly    attribute DOMString deviceId;
> +    readonly    attribute DOMString groupId;
> +    readonly    attribute DOMString kind;
> +    readonly    attribute DOMString label;

Nit: I don't think the extra spaces between "readonly" and "attribute" are necessary.

[Matthew Daiter]

(In reply to comment #12)
> Comment on attachment 255488 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255488&action=review
> 
> > Source/WebCore/Modules/mediastream/MediaDeviceInfo.h:38
> > +class MediaDeviceInfo : public RefCounted<MediaDeviceInfo>, public ScriptWrappable, public ContextDestructionObserver {
> 
> Why does this class inherit from ContextDestructionObserver, will it need to
> override contextDestroyed() later?
> 
Good point! Won't include in the next update. I believe that while debugging I had been experimenting with using it for destroying pointers to strings to free space. Next time, I won't inherit from it unless necessary.
> > Source/WebCore/Modules/mediastream/MediaDeviceInfo.h:42
> > +    
> 
Sure. I can remove this.
> Nit: this extra blank line isn't necessary.
> 
> > Source/WebCore/Modules/mediastream/MediaDeviceInfo.idl:2
> > +[
> > +NoInterfaceObject,
> 
> .idl files need a copyright header too.
> 
Will do.
> > Source/WebCore/Modules/mediastream/MediaDeviceInfo.idl:10
> > +    readonly    attribute DOMString deviceId;
> > +    readonly    attribute DOMString groupId;
> > +    readonly    attribute DOMString kind;
> > +    readonly    attribute DOMString label;
> 
> Nit: I don't think the extra spaces between "readonly" and "attribute" are
> necessary.
Sorry, will remove on next patch submit

[WebKit Commit Bot]

Comment on attachment 255488 [details]
Patch

Rejecting attachment 255488 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 255488 [details]
Patch

Clearing flags on attachment: 255488

Committed r185940: <http://trac.webkit.org/changeset/185940>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

This broke builds on several bots. I landed several build fixes (more may be needed):
<https://trac.webkit.org/r185944>
<https://trac.webkit.org/r185945>

I am not sure why EWS did not complain on GTK/EFL.

[Chris Dumez]

(In reply to comment #17)
> This broke builds on several bots. I landed several build fixes (more may be
> needed):
> <https://trac.webkit.org/r185944>
> <https://trac.webkit.org/r185945>
> 
> I am not sure why EWS did not complain on GTK/EFL.

and <http://trac.webkit.org/changeset/185946>...

[Csaba Osztrogonác]

(In reply to comment #18)
> (In reply to comment #17)
> > This broke builds on several bots. I landed several build fixes (more may be
> > needed):
> > <https://trac.webkit.org/r185944>
> > <https://trac.webkit.org/r185945>
> > 
> > I am not sure why EWS did not complain on GTK/EFL.
> 
> and <http://trac.webkit.org/changeset/185946>...

It seems the build was broken only on the GTK debug bot. How is it 
possible? Is this feature used somewhere or is it simple dead code?

[Csaba Osztrogonác]

(In reply to comment #19)
> It seems the build was broken only on the GTK debug bot. How is it 
> possible? Is this feature used somewhere or is it simple dead code?

I built r185940 (EFL) and build works fine. It seems JSMediaDeviceInfo.cpp
(generated from the idl file) doesn't use MediaDeviceInfo::MediaDeviceInfo
and MediaDeviceInfo::create at all. Is it normal behaviour?

[Csaba Osztrogonác]

Just out of curiosity: Was this patch tested/built on Mac with enabled MEDIA_STREAM ? Can we add a layout test to check if it works or not?

[Matthew Daiter]

Hey,
Planning on tackling larger problems, which uses MediaDeviceInfo. Dead code now, not dead code later.