Bug 100818

Summary: [EFL][WK2] Add Ewk_File_Chooser_Request API
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit EFLAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, kenneth, laszlo.gombos, lucas.de.marchi, rakuco, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61838, 100822    
Attachments:
Description Flags
Patch
none
Patch
kenneth: review+, webkit.review.bot: commit-queue-
Patch for landing none

Description Chris Dumez 2012-10-30 23:35:17 PDT
When the user click on an input button of type "file", we should show a file chooser dialog. However, we don't currently provide an API allowing the browser to do so.
Comment 1 Chris Dumez 2012-10-31 05:37:23 PDT
Created attachment 171625 [details]
Patch
Comment 2 Mikhail Pozdnyakov 2012-10-31 06:15:17 PDT
Comment on attachment 171625 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_file_chooser_request.cpp:72
> +void Ewk_File_Chooser_Request::chooseFiles(Vector< RefPtr<APIObject> >& fileURLs)

does not look like you're modifying fileURLs. Why not const ref?

> Source/WebKit2/UIProcess/API/efl/ewk_file_chooser_request.cpp:143
> +    EINA_LIST_FOREACH(chosenFiles, l, data) {

any check that data is not null, assertion?

> Source/WebKit2/UIProcess/API/efl/ewk_file_chooser_request_private.h:52
> +    inline bool wasHandled() const { return m_wasRequestHandled; }

is inline needed?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_file_chooser_request.cpp:38
> +static void onFileChooserRequest(void* userData, Evas_Object*, void* eventInfo)

shouldn't input parameters been checked?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_file_chooser_request.cpp:43
> +static int compareStrings(const void* string1, const void* string2)

same here
Comment 3 Chris Dumez 2012-10-31 06:30:56 PDT
Comment on attachment 171625 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_file_chooser_request.cpp:72
>> +void Ewk_File_Chooser_Request::chooseFiles(Vector< RefPtr<APIObject> >& fileURLs)
> 
> does not look like you're modifying fileURLs. Why not const ref?

Needed by ImmutableArray::adopt()

>> Source/WebKit2/UIProcess/API/efl/ewk_file_chooser_request.cpp:143
>> +    EINA_LIST_FOREACH(chosenFiles, l, data) {
> 
> any check that data is not null, assertion?

Yeah, I'll add a EINA_SAFETY_ON_NULL_RETURN_VAL().

>> Source/WebKit2/UIProcess/API/efl/ewk_file_chooser_request_private.h:52
>> +    inline bool wasHandled() const { return m_wasRequestHandled; }
> 
> is inline needed?

It does not hurt.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_file_chooser_request.cpp:38
>> +static void onFileChooserRequest(void* userData, Evas_Object*, void* eventInfo)
> 
> shouldn't input parameters been checked?

Ok

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_file_chooser_request.cpp:43
>> +static int compareStrings(const void* string1, const void* string2)
> 
> same here

ok
Comment 4 Chris Dumez 2012-10-31 06:55:33 PDT
Created attachment 171642 [details]
Patch

Take Mikhail's feedback into consideration.
Comment 5 WebKit Review Bot 2012-10-31 09:37:52 PDT
Comment on attachment 171642 [details]
Patch

Rejecting attachment 171642 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 12787 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
50>At revision 12787.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/14561278
Comment 6 Chris Dumez 2012-10-31 09:50:09 PDT
Created attachment 171675 [details]
Patch for landing

Rebased on master and remove tabs.

Could someone please cq+ ?
Comment 7 WebKit Review Bot 2012-10-31 12:17:36 PDT
Comment on attachment 171675 [details]
Patch for landing

Clearing flags on attachment: 171675

Committed r133057: <http://trac.webkit.org/changeset/133057>
Comment 8 WebKit Review Bot 2012-10-31 12:17:41 PDT
All reviewed patches have been landed.  Closing bug.