[Webkit-unassigned] [Bug 91957] [EFL] Add File Chooser API

[bugzilla-daemon at webkit.org]

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





--- Comment #25 from Kihong Kwon <kihong.kwon at samsung.com>  2012-07-24 19:51:41 PST ---
(In reply to comment #24)
> (From update of attachment 154220 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154220&action=review
> 
> When you submit modified patch, please take a look this patch further. In addition, please run layout test as well. I don't like too long bug review thread.
> 
> > Source/WebKit/efl/ewk/ewk_file_chooser.cpp:90
> > +    if (capture == "camera")
> 
> If possible, follow enum type definition order.

I think ordering is right.
filesystem is a basic type of capture attribute.
That's why, I let filesystem in the first of enum.
And use that for defalut return value.

http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp#L1723

> 
> > Source/WebKit/efl/ewk/ewk_file_chooser.cpp:99
> > +    return EWK_FILE_CHOOSER_CAPTURE_TYPE_FILESYSTEM;
> 
> I wonder if this patch can guarantee that capture only has camera, camcorder, microphone and file system.
> 
> If not so, you have to modify this as below,

As you can see in the WebCore, http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp#L1723
If there is not a valid value, input element works with "filesystem".

So, EWK_FILE_CHOOSER_CAPTURE_TYPE_INVALID is only used when Ewk_File_Chooser pointer or capture attribute is invalid.
Therefore I think current implementation is right.

> 
> if (capture == "filesystem")
>     return EWK_FILE_CHOOSER_CAPTURE_TYPE_FILESYSTEM;
> 
> return EWK_FILE_CHOOSER_CAPTURE_TYPE_INVALID;
> 
> > Source/WebKit/efl/ewk/ewk_file_chooser.cpp:112
> > +void ewk_file_chooser_free(Ewk_File_Chooser* ewkFileChooser)
> 
> Other functions are using just *chooser*. Keep to be consistent with other functions.

OK.

> 
> > Source/WebKit/efl/ewk/ewk_file_chooser.h:112
> > + *         (use eina_stringshare_del() to free the items)
> 
> I think you don't need to use ( ). See also below comment.
> 
> http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_intent.h#L80
> 
> I will not use (..) in my next patch.

No problem. 
but I wonder which one is right? "Use..." like a ewk_intest.h or "use...".
I thought we don't use capital letter with @return.
But, you didn't mention about that.

Thanks gyuyoung.

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