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

Mon Mar 14 08:09:43 PDT 2011

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

--- Comment #25 from Satish Sampath <satish at chromium.org>  2011-03-14 08:09:41 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.
> 
> 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?

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