Bug 49839 – [Qt] Implement the File API spec

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Helder Correia]

This is a master bug for the introduction of support for the File API in QtWebKit as described in the spec.

[Jarred Nicholls]

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.

[Jarred Nicholls]

Created attachment 76301 [details]
Proposed patch

Fixed indentation

[Jarred Nicholls]

Created attachment 76302 [details]
Proposed patch

Had to rebase from master/trunk, the patch was having merge conflicts.

[Early Warning System Bot]

Attachment 76302 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7030014

[Jarred Nicholls]

(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

[Jarred Nicholls]

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.

[Csaba Osztrogonác]

(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.

[Jarred Nicholls]

Created attachment 76381 [details]
Final patch rebased w/ trunk

Final patch rebased from trunk

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

Created attachment 76398 [details]
followup patch for "Final patch rebased w/ trunk" 

You will need this followup patch not to break any layout test.

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

(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.

[Jarred Nicholls]

Created attachment 76422 [details]
Proposed patch

[Early Warning System Bot]

Attachment 76422 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6918094

[Ariya Hidayat]

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?

[Jarred Nicholls]

(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.

[Jarred Nicholls]

Created attachment 76624 [details]
Proposed patch

New patch with Ariya's comments.

[Early Warning System Bot]

Attachment 76624 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7174012

[Andreas Kling]

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.

[Jarred Nicholls]

Created attachment 76645 [details]
Proposed patch

[WebKit Commit Bot]

Comment on attachment 76645 [details]
Proposed patch

Clearing flags on attachment: 76645

Committed r74115: <http://trac.webkit.org/changeset/74115>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.