[Webkit-unassigned] [Bug 47264] Introduce the device element as an experimental feature

Thu Mar 10 05:57:10 PST 2011

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

--- Comment #24 from Adam Bergkvist <adam.bergkvist at ericsson.com>  2011-03-10 05:57:08 PST ---
(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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.