Bug 49839 - [Qt] Implement the File API spec
Summary: [Qt] Implement the File API spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Jarred Nicholls
URL: http://www.w3.org/TR/FileAPI/
Keywords: HTML5, Qt
Depends on:
Blocks: 50940 50941
  Show dependency treegraph
 
Reported: 2010-11-19 15:32 PST by Helder Correia
Modified: 2010-12-15 08:01 PST (History)
5 users (show)

See Also:


Attachments
Proposed patch (12.23 KB, patch)
2010-12-11 04:01 PST, Jarred Nicholls
no flags Details | Formatted Diff | Diff
Proposed patch (13.59 KB, patch)
2010-12-11 04:10 PST, Jarred Nicholls
no flags Details | Formatted Diff | Diff
Proposed patch (12.81 KB, patch)
2010-12-11 04:48 PST, Jarred Nicholls
no flags Details | Formatted Diff | Diff
Final patch rebased w/ trunk (10.10 KB, patch)
2010-12-13 06:41 PST, Jarred Nicholls
no flags Details | Formatted Diff | Diff
followup patch for "Final patch rebased w/ trunk" (13.81 KB, patch)
2010-12-13 09:49 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Proposed patch (14.58 KB, patch)
2010-12-13 12:55 PST, Jarred Nicholls
ariya.hidayat: review-
Details | Formatted Diff | Diff
Proposed patch (14.12 KB, patch)
2010-12-14 20:40 PST, Jarred Nicholls
kling: review+
kling: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (14.37 KB, patch)
2010-12-15 07:41 PST, Jarred Nicholls
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Helder Correia 2010-11-19 15:32:27 PST
This is a master bug for the introduction of support for the File API in QtWebKit as described in the spec.
Comment 1 Jarred Nicholls 2010-12-11 04:01:24 PST
Created attachment 76300 [details]
Proposed patch

Qt's port has most of the implementation of FileSystemQt.cpp complete.  File seeking was the only necessary thing missing.  Mostly, the qmake files were out of wack.

fast/files reader tests work when manually operated.  They do not automatically work due to Qt's inability to handle multi-file inputs (https://bugs.webkit.org/show_bug.cgi?id=22048) as well as Qt's DRT is missing an eventSender.beginDragWithFiles implementation.  A separate bug will need to be filed for adding eventSender.beginDragWithFiles.
Comment 2 Jarred Nicholls 2010-12-11 04:10:00 PST
Created attachment 76301 [details]
Proposed patch

Fixed indentation
Comment 3 Jarred Nicholls 2010-12-11 04:48:08 PST
Created attachment 76302 [details]
Proposed patch

Had to rebase from master/trunk, the patch was having merge conflicts.
Comment 4 Early Warning System Bot 2010-12-11 05:20:19 PST
Attachment 76302 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7030014
Comment 5 Jarred Nicholls 2010-12-11 05:43:56 PST
(In reply to comment #4)
> Attachment 76302 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/7030014

Indeed it did fail, but not the library itself.  Something about MiniBrowser fails to build in relation to this page.  I moved some some data structures into a conditional branch for 3D_CANVAS and BLOB support.  Those same files are the ones here that are unable to link.

Like the bot, my library build succeeded.  I, however, am not building WebKit2 nor the MiniBrowser.  My command is identical to the bot's w/ the exception of the number of make jobs.  How come the bot builds WebKit2 but I do not? :-P
Comment 6 Jarred Nicholls 2010-12-11 17:55:11 PST
Ok so this build failure is bogus.  I only briefly looked at the output of the build.  The bot didn't clean the build first, and this is required due to preprocessor changes and .idl changes for the JS bindings.

Awaiting a review, thanks.
Comment 7 Csaba Osztrogonác 2010-12-13 02:19:49 PST
(In reply to comment #6)
> Ok so this build failure is bogus.  I only briefly looked at the output of the build.  The bot didn't clean the build first, and this is required due to preprocessor changes and .idl changes for the JS bindings.
> 
> Awaiting a review, thanks.

Makefiles can't handle if you modify an ENABLE_FEATURE macro and it caused the false positive alarm. Clean build works for me. Please ping me on IRC, when you would like to commit this patch, because I have to do clean build on Qt bots.
Comment 8 Jarred Nicholls 2010-12-13 06:41:53 PST
Created attachment 76381 [details]
Final patch rebased w/ trunk

Final patch rebased from trunk
Comment 9 Csaba Osztrogonác 2010-12-13 09:48:24 PST
(In reply to comment #8)
> Created an attachment (id=76381) [details]
> Final patch rebased w/ trunk
> 
> Final patch rebased from trunk

Qt EWS don't like your patch, but the clean build works for me.
Comment 10 Csaba Osztrogonác 2010-12-13 09:49:45 PST
Created attachment 76398 [details]
followup patch for "Final patch rebased w/ trunk" 

You will need this followup patch not to break any layout test.
Comment 11 Csaba Osztrogonác 2010-12-13 09:50:56 PST
(In reply to comment #10)
> You will need this followup patch not to break any layout test.
Ooops, it contains your patch too. I mean LayoutTests/* changes.
Comment 12 Csaba Osztrogonác 2010-12-13 10:18:28 PST
(In reply to comment #11)
> Ooops, it contains your patch too. I mean LayoutTests/* changes.

Feel free to pick up layout test changes from it and add it to your patch with an additional LayoutTests/ChangeLog entry.
Comment 13 Jarred Nicholls 2010-12-13 12:55:59 PST
Created attachment 76422 [details]
Proposed patch
Comment 14 Early Warning System Bot 2010-12-13 17:31:04 PST
Attachment 76422 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6918094
Comment 15 Ariya Hidayat 2010-12-14 19:56:37 PST
Comment on attachment 76422 [details]
Proposed patch

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

Some minor comments, otherwise LGTM.

> LayoutTests/ChangeLog:5
> +        Allow File API (http://www.w3.org/TR/FileAPI/) to work w/ the Qt port

Please match the single line description with the bug title.
Also, no need to shorten "with" to "w/".

> WebCore/features.pri:211
> +contains(DEFINES, ENABLE_3D_CANVAS=1): FEATURE_DEFINES_JAVASCRIPT += ENABLE_3D_CANVAS=1
> +contains(DEFINES, ENABLE_3D_RENDERING=1): FEATURE_DEFINES_JAVASCRIPT += ENABLE_3D_RENDERING=1

Is the change involving Canvas and Rendering also necessary?

> WebCore/platform/qt/FileSystemQt.cpp:98
> -    return String(QFileInfo(path).absolutePath());
> +    return QFileInfo(path).absolutePath();

Without this change, it won't work?
Comment 16 Jarred Nicholls 2010-12-14 20:04:26 PST
(In reply to comment #15)
> (From update of attachment 76422 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76422&action=review
> 
> Some minor comments, otherwise LGTM.
> 
> > LayoutTests/ChangeLog:5
> > +        Allow File API (http://www.w3.org/TR/FileAPI/) to work w/ the Qt port
> 
> Please match the single line description with the bug title.
> Also, no need to shorten "with" to "w/".

Ok will update.

> 
> > WebCore/features.pri:211
> > +contains(DEFINES, ENABLE_3D_CANVAS=1): FEATURE_DEFINES_JAVASCRIPT += ENABLE_3D_CANVAS=1
> > +contains(DEFINES, ENABLE_3D_RENDERING=1): FEATURE_DEFINES_JAVASCRIPT += ENABLE_3D_RENDERING=1
> 
> Is the change involving Canvas and Rendering also necessary?

No not for this particular bug.  I will remove them (and directory upload & file system) for this bug.  They *need* to be there, but we'll save them for another bug to keep this one clean.  I just "snuck" them in there.

> 
> > WebCore/platform/qt/FileSystemQt.cpp:98
> > -    return String(QFileInfo(path).absolutePath());
> > +    return QFileInfo(path).absolutePath();
> 
> Without this change, it won't work?

It will work, but the rest of the filesystem methods do not explicitly create a platform String, this is the only method that does...the rest rely on compiler copy constructor.  This was done for pure consistency's sake.
Comment 17 Jarred Nicholls 2010-12-14 20:40:52 PST
Created attachment 76624 [details]
Proposed patch

New patch with Ariya's comments.
Comment 18 Early Warning System Bot 2010-12-14 20:52:15 PST
Attachment 76624 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7174012
Comment 19 Andreas Kling 2010-12-15 07:07:22 PST
Comment on attachment 76624 [details]
Proposed patch

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

LGTM, just one thing:

> WebCore/platform/qt/FileSystemQt.cpp:181
> +        return handle->seek(current += offset) ? current : -1;

This is a bit convoluted for my taste, please rewrite it in a more readable way.
Comment 20 Jarred Nicholls 2010-12-15 07:41:44 PST
Created attachment 76645 [details]
Proposed patch
Comment 21 WebKit Commit Bot 2010-12-15 08:01:05 PST
Comment on attachment 76645 [details]
Proposed patch

Clearing flags on attachment: 76645

Committed r74115: <http://trac.webkit.org/changeset/74115>
Comment 22 WebKit Commit Bot 2010-12-15 08:01:12 PST
All reviewed patches have been landed.  Closing bug.