Bug 53776

Summary: Device element patch 1: adding a basic skeleton of the HTMLDeviceElement and the Stream objects.
Product: WebKit Reporter: Leandro Graciá Gil <leandrogracia>
Component: DOMAssignee: Leandro Graciá Gil <leandrogracia>
Status: RESOLVED WONTFIX    
Severity: Normal CC: abarth, adam.bergkvist, andreip, buildbot, gns, jorlow, leandrogracia, luiz, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on: 53595    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Leandro Graciá Gil 2011-02-04 09:30:28 PST
This patch creates the basic skeleton for the asynchronous client-based implementation of the device element that we proposed.
This is designed to be the second of a set of small patches. Please see https://bugs.webkit.org/show_bug.cgi?id=53595 for the first one.
All functionality and specific details will be included in later patches.
Comment 1 Leandro Graciá Gil 2011-02-04 10:18:50 PST
Created attachment 81242 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2011-02-04 11:07:38 PST
Attachment 81242 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7701071
Comment 3 Build Bot 2011-02-04 11:31:40 PST
Attachment 81242 [details] did not build on win:
Build output: http://queues.webkit.org/results/7698578
Comment 4 Eric Seidel (no email) 2011-02-05 01:55:05 PST
Comment on attachment 81242 [details]
Patch

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

build failures.

> Source/WebCore/dom/DeviceType.cpp:43
> +// Avoid using static Strings. Code should not run on WebKit load.
> +const char* DeviceType::s_names[] =
> +{
> +    "invalid",
> +    "media"
> +};

We've historically done this with AtomicStrings and an init() method.  But in this case for only 2 types, I'm not sure having an array actually buys us anything besides complexity.

> Source/WebCore/dom/Stream.h:49
> +    static PassRefPtr<Stream> create(HTMLDeviceElement*, const String& url);

Should these urls be strings or KURLs?

> Source/WebCore/html/HTMLDeviceElement.cpp:87
> +        HTMLElement::defaultEventHandler(event);
> +        return;

This could just be one line.
Comment 5 Leandro Graciá Gil 2011-03-09 11:22:12 PST
Created attachment 85202 [details]
Patch
Comment 6 Leandro Graciá Gil 2011-03-09 11:26:38 PST
Created attachment 85203 [details]
Patch
Comment 7 Leandro Graciá Gil 2011-03-09 11:27:36 PST
(In reply to comment #4)
> (From update of attachment 81242 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81242&action=review
> 
> build failures.
> 
> > Source/WebCore/dom/DeviceType.cpp:43
> > +// Avoid using static Strings. Code should not run on WebKit load.
> > +const char* DeviceType::s_names[] =
> > +{
> > +    "invalid",
> > +    "media"
> > +};
> 
> We've historically done this with AtomicStrings and an init() method.  But in this case for only 2 types, I'm not sure having an array actually buys us anything besides complexity.
> 
> > Source/WebCore/dom/Stream.h:49
> > +    static PassRefPtr<Stream> create(HTMLDeviceElement*, const String& url);
> 
> Should these urls be strings or KURLs?
> 
> > Source/WebCore/html/HTMLDeviceElement.cpp:87
> > +        HTMLElement::defaultEventHandler(event);
> > +        return;
> 
> This could just be one line.

Fixed.

Adding also the session manager code to make or proposal clear.
Comment 8 Leandro Graciá Gil 2011-03-11 09:17:00 PST
Created attachment 85481 [details]
Patch
Comment 9 Leandro Graciá Gil 2011-03-11 09:52:17 PST
Decoupled the DeviceSession to the API calls to make clear that the object is managed in WebCore, not by the embedder. The object itself has been also considerably simplified by removing specific information that was not required. Also deleted some code not being currently used.
Comment 10 Eric Seidel (no email) 2011-04-18 09:12:37 PDT
Comment on attachment 85481 [details]
Patch

Cleared review? from attachment 85481 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).