Bug 47264

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.

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

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

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

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

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 ?

(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.

(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.

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 .

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

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

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.

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.

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

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

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

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

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

> 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.

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.

(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.

(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?

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.

(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

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?

Isn't this a duplicate of bug 43572?

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(...)).