Bug 47264 – Introduce the device element as an experimental feature

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

[Adam Bergkvist]

We have experimented with the device element to enable audio and video capture in the browser. Even though the specification is in an early stage, our intention is to contribute as much as possible.

http://www.whatwg.org/specs/web-apps/current-work/multipage/commands.html#devices

[Adam Bergkvist]

Created attachment 80411 [details]
Proposed patch

Implementation of the device element without any platform specific parts (nothing added to the build systems). The patch contains a few lines of code which has been commented out to show how it ties together with the Stream API (see https://bugs.webkit.org/show_bug.cgi?id=47265). A proposed GTK implementation is available as a patch to https://bugs.webkit.org/show_bug.cgi?id=53264.



It's currently not possible to build without --device-element since the bindings generator, regardless of feature guards, adds includes of JSHTMLDeviceElement.h (in e.g. JSDOMWindow.cpp) which is not generated.

[WebKit Review Bot]

Attachment 80411 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7515405

[Early Warning System Bot]

Attachment 80411 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7549389

[Build Bot]

Attachment 80411 [details] did not build on win:
Build output: http://queues.webkit.org/results/7524335

[WebKit Review Bot]

Attachment 80411 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7581375

[Adam Barth]

Comment on attachment 80411 [details]
Proposed patch

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

r- for build failures and changing the parser without adding a test.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:897
> +#if ENABLE(DEVICE_ELEMENT)
> +        || token.name() == deviceTag
> +#endif

HTML parsing should not be conditional on which features are enabled.  If this line of could should be here, it should be here unconditionally and we should have parser test that demonstrates this change.

> Source/WebCore/platform/device/StreamDeviceManager.cpp:38
> +#if 0

#if 0 ?

[Adam Bergkvist]

(In reply to comment #6)

> HTML parsing should not be conditional on which features are enabled.  If this line of could should be here, it should be here unconditionally and we should have parser test that demonstrates this change.

Is it possible to remove the end tag requirement without touching the parser code, like you could with the old parser? I guess we still want the device element to be feature guarded?

> #if 0 ?

 38 #if 0
 39 #else
 40 #define StreamDeviceManagerPrivateClassName NullStreamDeviceManagerPrivate
 41 #endif 

The "if 0" was intended has a hint to where platform specific code should go. That line is overwritten when you apply the GTK-specific patch (see https://bugs.webkit.org/show_bug.cgi?id=53264). In this case it forces the usage of the fallback class NullStreamDeviceManagerPrivate. Perhaps it would be better to just use a single #define.

[Adam Barth]

(In reply to comment #7)
> (In reply to comment #6)
> 
> > HTML parsing should not be conditional on which features are enabled.  If this line of could should be here, it should be here unconditionally and we should have parser test that demonstrates this change.
> 
> Is it possible to remove the end tag requirement without touching the parser code, like you could with the old parser? I guess we still want the device element to be feature guarded?

Does the HTML5 specification mention the device element when describing the parsing algorithm?  Our parsing algorithm matches the HTML5 spec exactly.  If the HTML5 parsing algorithm says the device element should be in this list, then it should be there unconditionally.  If it doesn't say that the device element should be in this list, then it should not be there unconditionally.  In no case should HTML parsing depend on which features are enabled.

> > #if 0 ?
> 
>  38 #if 0
>  39 #else
>  40 #define StreamDeviceManagerPrivateClassName NullStreamDeviceManagerPrivate
>  41 #endif 
> 
> The "if 0" was intended has a hint to where platform specific code should go. That line is overwritten when you apply the GTK-specific patch (see https://bugs.webkit.org/show_bug.cgi?id=53264). In this case it forces the usage of the fallback class NullStreamDeviceManagerPrivate. Perhaps it would be better to just use a single #define.

That's fine, but we shouldn't have "#if 0" in the code.  IMHO, a FIXME comment is a better way of indicating that some port-specific logics goes here.

[Leandro Graciá Gil]

I have opened a thread to discuss the design and details of this implementation.

Please take a look to https://lists.webkit.org/pipermail/webkit-dev/2011-January/015822.html .

[Andrei Popescu]

A few more comments:

>      for (int i = ((int) m_listEntries.size()) - 1; i >= 0; i--) {

Please do not use C-style casts.

> for (j = ((int) otherSelectedEntries.size()) - 1; j >= 0; j--) 

ditto

> void HTMLDeviceElement::defaultEventHandler(Event* evt)
>  {
>     if (evt->type() == eventNames().DOMActivateEvent) {
>         runDialog();

...

> void HTMLDeviceElement::runDialog()
> {

(....)

>     m_deviceList = deviceManagerProxy().createDeviceList(deviceType());

(...)

>    if (Chrome* chrome = page->chrome())
>         chrome->runDeviceDialog(m_deviceList.get());
 
The above design is a reason for concern: it seems you are invoking synchronously, from inside an event handler, two potentially expensive operations:

1. deviceManagerProxy().createDeviceList(deviceType());

createDeviceList() method above is synchronous and is implemented in platform code and probes for a list of devices (cameras, microphones).

2. runDeviceDialog() is also synchronous and seems to involve running a modal dialog


I am wondering why you made these choices and whether the alternative design proposed by Leandro in https://lists.webkit.org/pipermail/webkit-dev/2011-January/015822.html isn't a much better choice. 

Leandro has actually been working on this and has a patch that he could upload for review. Perhaps the best way to proceed is for him to upload it and see how it differs to the current proposal?

Thanks,
Andrei

[Leandro Graciá Gil]

I have created a patch to introduce compilation guards for the device element. These should be required independently of the design approach we decide to take.

https://bugs.webkit.org/show_bug.cgi?id=53595

[Adam Bergkvist]

Created attachment 81085 [details]
Updated patch

Thank you for your comments.

1. Build failures
Filed bug (https://bugs.webkit.org/show_bug.cgi?id=53672) with patch that modifies make_names.pl to not unconditionally include HTMLDeviceElement.h and JSHTMLDeviceElement.h.

2. Parser modification
Filed bug (http://www.w3.org/Bugs/Public/show_bug.cgi?id=11935) to include device in the list of self-closing elements in the specification.
Added parser test that demonstrates the behavior.

3. "#if 0"
Replaced with FIXME.

4. C-style casts
Fixed.

5. Synchronous dialog
Added callback from dialog.

6. Synchronous device list creation
Moved list creation to platform dialog code.

[Adam Barth]

Comment on attachment 81085 [details]
Updated patch

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

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:898
>      if (token.name() == areaTag
>          || token.name() == brTag
> +        || token.name() == deviceTag
>          || token.name() == embedTag
>          || token.name() == imgTag
>          || token.name() == keygenTag

This list of elements comes from <http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#parsing-main-inbody>.  The spec says:

> A start tag whose tag name is one of: "area", "br", "embed", "img", "keygen", "wbr"

I don't see "device" in that list.  Please do not add non-standard behavior to the parser.

[WebKit Review Bot]

Attachment 81085 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7691437

[Build Bot]

Attachment 81085 [details] did not build on win:
Build output: http://queues.webkit.org/results/7694311

[Early Warning System Bot]

Attachment 81085 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7690582

[Collabora GTK+ EWS bot]

Attachment 81085 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7694408

[Gustavo Noronha (kov)]

Attachment 81085 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7689741

[Adam Barth]

> I don't see "device" in that list.  Please do not add non-standard behavior to the parser.

I discussed this issue with Hixie on IRC.  He thinks its reasonable to make <device> a void element and is considering adding it to the spec.  Personally, I think it's going to cause more pain than it's worth, but that doesn't mean we shouldn't try.

If we decide to modify the parser in this way, I'm unsure whether we should condition the parsing change on whether the feature is enabled.

[Pushparajan]

what would be difference between http://www.w3.org/TR/2010/WD-html-media-capture-20100928/ and http://www.whatwg.org/specs/web-apps/current-work/multipage/commands.html#devices ?

I think W3C is bringing up Device API spec(which is based on JIL/WAC) rather than having a whatwg way of HTML5 device element.

[Adam Barth]

(In reply to comment #20)
> what would be difference between http://www.w3.org/TR/2010/WD-html-media-capture-20100928/ and http://www.whatwg.org/specs/web-apps/current-work/multipage/commands.html#devices ?
> 
> I think W3C is bringing up Device API spec(which is based on JIL/WAC) rather than having a whatwg way of HTML5 device element.

This sounds like a larger discussion that we should have as a project.  Generally speaking, browser vendors are pretty disengaged from the DAP working group.

[Adam Bergkvist]

(In reply to comment #19)
> If we decide to modify the parser in this way, I'm unsure whether we should condition the parsing change on whether the feature is enabled.

Perhaps an experimental feature like this should not affect the parser behavior when not enabled, especially considering that the element will be an HTMLElement in that case. What's your preference?

Could you please have a look at the patch for https://bugs.webkit.org/show_bug.cgi?id=53672 to make this patch build?

[John Knottenbelt]

Comment on attachment 81085 [details]
Updated patch

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

> Source/WebCore/device/DeviceList.h:73
> +    bool m_selected;

I think that it would be simpler to deal only with selected devices in WebCore, so would recommend removing m_selected as a field of DeviceListEntry. See comment below about DeviceList::selectionsMatch.

> Source/WebCore/device/DeviceList.h:85
> +    bool selectionsMatch(DeviceList* other);

I think that this class could be much simplified if DeviceList becomes a simple list of devices that does not deal with selection directly. I expect ChromeClient::runDeviceDialog to manage the details of selection and return only selected devices to DeviceDialogClient::selectionsMade().

Consequently, I would recommend simplifying the following methods so that they do not deal with selected status, and renaming them accordingly:
 DeviceList::selectionsMatch() could be renamed to matches(),
 DeviceList::updateSelections() could be renamed to assign(),
 DeviceList::selectedEntries could be renamed to entries()

> Source/WebCore/device/DeviceManagerProxy.h:47
> +    StreamDeviceManager* streamDeviceManager();

Only DeviceManagerProxy::createDeviceList() calls this method. But DeviceManagerProxy::createDeviceList() is not used, so I think we should remove this and introduce in a later patch if necessary.

> Source/WebCore/device/DeviceManagerProxy.h:49
> +    PassRefPtr<DeviceList> createDeviceList(const DeviceType);

I don't see any calls to DeviceManagerProxy::createDeviceList. Can we remove this method (or introduce in a later patch, if necessary)?

> Source/WebCore/html/HTMLDeviceElement.cpp:173
> +        chrome->runDeviceDialog(m_deviceList.get(), this);

ChromeClient::runDeviceDialog takes a raw pointer to the DeviceDialogClient. This will cause a problem if the HTMLDeviceElement is destroyed while the runDialog is occuring. 

One way of solving this would be for the DeviceDialogClient to be implemented by an object with page lifetime. See, for example, (WebCore/Source/page/SpeechInput.h) SpeechInput::startRecognition / SpeechInput::setRecognitionResult etc.

runDeviceDialog implies that a dialog will be presented to the user, but this should be UA-specific. Perhaps rename to something like selectDevice?

Why do we need to pass in a DeviceList to the client code? Wouldn't it be simpler to pass the type of the device required and leave the probing and device list management to the client?

> Source/WebCore/html/HTMLDeviceElement.cpp:180
> +    bool hasChanged = m_deviceList ? !newDeviceList->selectionsMatch(m_deviceList.get()) : hasSelections;

If the user didn't change the selection, could ChromeClient avoid calling this method at all? 
We could then avoid checking to see if the selection has changed or not.

> Source/WebCore/html/HTMLDeviceElement.cpp:184
> +        m_data = hasSelections ? deviceManagerProxy().createDeviceData(m_deviceList.get()) : 0;

What is the intended behaviour of DeviceManagerProxy::createDeviceData? If it needs to contact the embedder or perform potentially blocking code, it should be made asynchronous. And if so, it would make sense to merge it with ChromeClient::runDeviceDialog which is also asynchronous.

If these two methods can be merged, it would make sense to remove DeviceList altogether and simply receive a new device-specific data object here. In the case of device type=media, this would be a Stream object.

[Adam Bergkvist]

(In reply to comment #23)
> (From update of attachment 81085 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81085&action=review
> 
> > Source/WebCore/device/DeviceList.h:73
> > +    bool m_selected;
> 
> I think that it would be simpler to deal only with selected devices in WebCore, so would recommend removing m_selected as a field of DeviceListEntry. See comment below about DeviceList::selectionsMatch.
>
> > Source/WebCore/device/DeviceList.h:85
> > +    bool selectionsMatch(DeviceList* other);
> 
> I think that this class could be much simplified if DeviceList becomes a simple list of devices that does not deal with selection directly. I expect ChromeClient::runDeviceDialog to manage the details of selection and return only selected devices to DeviceDialogClient::selectionsMade().
> 
> Consequently, I would recommend simplifying the following methods so that they do not deal with selected status, and renaming them accordingly:
>  DeviceList::selectionsMatch() could be renamed to matches(),
>  DeviceList::updateSelections() could be renamed to assign(),
>  DeviceList::selectedEntries could be renamed to entries()
>

The reason for having both selected entries and unselected entries in the list at this point is to be able to update a newly probed list with previous selections before presenting the list in the device selector. To support this functionality without keeping track of selected entries would require two lists; one with all entries and one with previously selected entries. It would then be up to the device selector to combine these and present a single list. A new list would then have to be created only containing the new selections instead of simply marking the selected entries. Also, by handling both selected and unselected entries in the device element, it's possible to determine if a device has been unselected or disconnected from the UA since the last probe. We use this information to determine if a change event should be triggered or not.

The idea with putting quite a lot of functionality in DeviceList, is to minimize the work for platforms to implement the device element. The platform specific device selectors will pretty much be responsible for displaying the device list and update it with the user’s selections; the shared WebCore code will deal with the details from the specification.

> > Source/WebCore/device/DeviceManagerProxy.h:47
> > +    StreamDeviceManager* streamDeviceManager();
> 
> Only DeviceManagerProxy::createDeviceList() calls this method. But DeviceManagerProxy::createDeviceList() is not used, so I think we should remove this and introduce in a later patch if necessary.
>

It's to be used by platforms implementing the device element (e.g. our GTK implementation, https://bugs.webkit.org/show_bug.cgi?id=53264).

> > Source/WebCore/device/DeviceManagerProxy.h:49
> > +    PassRefPtr<DeviceList> createDeviceList(const DeviceType);
> 
> I don't see any calls to DeviceManagerProxy::createDeviceList. Can we remove this method (or introduce in a later patch, if necessary)?
>

See previous comment.
 
> > Source/WebCore/html/HTMLDeviceElement.cpp:173
> > +        chrome->runDeviceDialog(m_deviceList.get(), this);
> 
> ChromeClient::runDeviceDialog takes a raw pointer to the DeviceDialogClient. This will cause a problem if the HTMLDeviceElement is destroyed while the runDialog is occuring. 
> 
> One way of solving this would be for the DeviceDialogClient to be implemented by an object with page lifetime. See, for example, (WebCore/Source/page/SpeechInput.h) SpeechInput::startRecognition / SpeechInput::setRecognitionResult etc.
>

Yes, I think you're right about this. The solution you proposed would do the trick. I've also looked into how it's solved for <input type=file> since it has been the model of our implementation in other regards.

> runDeviceDialog implies that a dialog will be presented to the user, but this should be UA-specific. Perhaps rename to something like selectDevice?
>

That's true, as a result from the discussion with Leandro we have renamed that function to runDeviceSelector.

> Why do we need to pass in a DeviceList to the client code? Wouldn't it be simpler to pass the type of the device required and leave the probing and device list management to the client?
>

The DeviceList is used to transfer the result from the device probing to the device selector. It's quite likely that several platforms will share media engine (e.g. GStreamer), but implement their own device selectors. I.e. different combinations of clients will be involved and need a unified way to talk to each other.

> > Source/WebCore/html/HTMLDeviceElement.cpp:180
> > +    bool hasChanged = m_deviceList ? !newDeviceList->selectionsMatch(m_deviceList.get()) : hasSelections;
> 
> If the user didn't change the selection, could ChromeClient avoid calling this method at all? 
> We could then avoid checking to see if the selection has changed or not.
>

Yes, it would be possible, but we wanted to centralize this functionality to avoid duplicating code and risking inconsistent behavior between ports.

> > Source/WebCore/html/HTMLDeviceElement.cpp:184
> > +        m_data = hasSelections ? deviceManagerProxy().createDeviceData(m_deviceList.get()) : 0;
> 
> What is the intended behaviour of DeviceManagerProxy::createDeviceData? If it needs to contact the embedder or perform potentially blocking code, it should be made asynchronous. And if so, it would make sense to merge it with ChromeClient::runDeviceDialog which is also asynchronous.
>

DeviceManagerProxy::createDeviceData should return a device handler API instance without blocking.

> If these two methods can be merged, it would make sense to remove DeviceList altogether and simply receive a new device-specific data object here. In the case of device type=media, this would be a Stream object.

Merging would prevent us from delegating probing, device selection and device handler API creation to separate components (see comment above).

-----

Thanks for your comments

[Satish Sampath]

Comment on attachment 81085 [details]
Updated patch

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

>>> Source/WebCore/device/DeviceList.h:73
>>> +    bool m_selected;
>> 
>> I think that it would be simpler to deal only with selected devices in WebCore, so would recommend removing m_selected as a field of DeviceListEntry. See comment below about DeviceList::selectionsMatch.
> 
> The reason for having both selected entries and unselected entries in the list at this point is to be able to update a newly probed list with previous selections before presenting the list in the device selector. To support this functionality without keeping track of selected entries would require two lists; one with all entries and one with previously selected entries. It would then be up to the device selector to combine these and present a single list. A new list would then have to be created only containing the new selections instead of simply marking the selected entries. Also, by handling both selected and unselected entries in the device element, it's possible to determine if a device has been unselected or disconnected from the UA since the last probe. We use this information to determine if a change event should be triggered or not.
> 
> The idea with putting quite a lot of functionality in DeviceList, is to minimize the work for platforms to implement the device element. The platform specific device selectors will pretty much be responsible for displaying the device list and update it with the user’s selections; the shared WebCore code will deal with the details from the specification.

Since you mentioned earlier that this device implementation is inspired by the file input element - I see the file input element only receives the list of selected files from the embedder, not the list of all files in all directories. This DeviceList is a list of all devices matching the given type and that seems unnecessarily complicated. Instead it could be done with just a vector of selected device IDs (similar to list of filenames in the input element case) without all this device data.

I also notice that different browsers fire the file input element fires onchange at different times. For example, Safari doesn't fire onchange when user selects the same file again whereas Chrome fires onchange on every invocation of the dialog, even if the same file was selected. I think for <device> onchange should fire even if the same device was selected. One use case is when the device has an issue and needs to be reset/plugged back in/connected to power etc. and the user wants to restart the process without reloading the page or closing the call.

>>> Source/WebCore/device/DeviceManagerProxy.h:47
>>> +    StreamDeviceManager* streamDeviceManager();
>> 
>> Only DeviceManagerProxy::createDeviceList() calls this method. But DeviceManagerProxy::createDeviceList() is not used, so I think we should remove this and introduce in a later patch if necessary.
> 
> It's to be used by platforms implementing the device element (e.g. our GTK implementation, https://bugs.webkit.org/show_bug.cgi?id=53264).

Can you move such code to the patch where it will be used? That would make it easier for reviewing the design.

>>> Source/WebCore/device/DeviceManagerProxy.h:49
>>> +    PassRefPtr<DeviceList> createDeviceList(const DeviceType);
>> 
>> I don't see any calls to DeviceManagerProxy::createDeviceList. Can we remove this method (or introduce in a later patch, if necessary)?
> 
> See previous comment.

Same as above

>>> Source/WebCore/html/HTMLDeviceElement.cpp:180
>>> +    bool hasChanged = m_deviceList ? !newDeviceList->selectionsMatch(m_deviceList.get()) : hasSelections;
>> 
>> If the user didn't change the selection, could ChromeClient avoid calling this method at all? 
>> We could then avoid checking to see if the selection has changed or not.
> 
> Yes, it would be possible, but we wanted to centralize this functionality to avoid duplicating code and risking inconsistent behavior between ports.

As I mentioned above, in the device case there are valid reasons to not do this check because the user may want to select the same device again in case of a failure. I'd remove this check and all code managing the device list.

>>> Source/WebCore/html/HTMLDeviceElement.cpp:184
>>> +        m_data = hasSelections ? deviceManagerProxy().createDeviceData(m_deviceList.get()) : 0;
>> 
>> What is the intended behaviour of DeviceManagerProxy::createDeviceData? If it needs to contact the embedder or perform potentially blocking code, it should be made asynchronous. And if so, it would make sense to merge it with ChromeClient::runDeviceDialog which is also asynchronous.
>> 
>> If these two methods can be merged, it would make sense to remove DeviceList altogether and simply receive a new device-specific data object here. In the case of device type=media, this would be a Stream object.
> 
> DeviceManagerProxy::createDeviceData should return a device handler API instance without blocking.

I see bug 47265 (implement Stream API) doesn't have a patch uploaded yet so it is not clear if this involves a call to the embedder. Can you clarify?

[Alexey Proskuryakov]

Isn't this a duplicate of bug 43572?

[Adam Bergkvist]

Resolved as WONTFIX since the device element has been removed from the spec. The entry point to the Stream API is now a pure JavaScript API (navigator.getUserMedia(...)).