Bug 100818 - [EFL][WK2] Add Ewk_File_Chooser_Request API
Summary: [EFL][WK2] Add Ewk_File_Chooser_Request API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks: 61838 100822
  Show dependency treegraph
 
Reported: 2012-10-30 23:35 PDT by Chris Dumez
Modified: 2012-10-31 12:17 PDT (History)
7 users (show)

See Also:


Attachments
Patch (29.65 KB, patch)
2012-10-31 05:37 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (30.00 KB, patch)
2012-10-31 06:55 PDT, Chris Dumez
kenneth: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (30.02 KB, patch)
2012-10-31 09:50 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.