Summary: | [Qt] Implement the File API spec | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Helder Correia <helder> | ||||||||||||||||||
Component: | WebKit Misc. | Assignee: | Jarred Nicholls <jarred> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Enhancement | CC: | ariya.hidayat, commit-queue, jarred, ossy, webkit-ews | ||||||||||||||||||
Priority: | P2 | Keywords: | HTML5, Qt | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
URL: | http://www.w3.org/TR/FileAPI/ | ||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||
Bug Blocks: | 50940, 50941 | ||||||||||||||||||||
Attachments: |
|
Description
Helder Correia
2010-11-19 15:32:27 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. Created attachment 76301 [details]
Proposed patch
Fixed indentation
Created attachment 76302 [details]
Proposed patch
Had to rebase from master/trunk, the patch was having merge conflicts.
Attachment 76302 [details] did not build on qt: Build output: http://queues.webkit.org/results/7030014 (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 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. (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. Created attachment 76381 [details]
Final patch rebased w/ trunk
Final patch rebased from trunk
(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. Created attachment 76398 [details]
followup patch for "Final patch rebased w/ trunk"
You will need this followup patch not to break any layout test.
(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. (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. Created attachment 76422 [details]
Proposed patch
Attachment 76422 [details] did not build on qt: Build output: http://queues.webkit.org/results/6918094 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? (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. Created attachment 76624 [details]
Proposed patch
New patch with Ariya's comments.
Attachment 76624 [details] did not build on qt: Build output: http://queues.webkit.org/results/7174012 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. Created attachment 76645 [details]
Proposed patch
Comment on attachment 76645 [details] Proposed patch Clearing flags on attachment: 76645 Committed r74115: <http://trac.webkit.org/changeset/74115> All reviewed patches have been landed. Closing bug. |