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

[bugzilla-daemon at webkit.org]

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





--- Comment #23 from John Knottenbelt <jknotten at chromium.org>  2011-03-01 01:50:49 PST ---
(From update of attachment 81085)
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.

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