Bug 40587

Summary: Add ScriptExecutionContext argument to File/Blob constructors
Product: WebKit Reporter: Jian Li <jianli>
Component: WebCore JavaScriptAssignee: Jian Li <jianli>
Status: RESOLVED FIXED    
Severity: Normal CC: dimich, fishd, gustavo, kinuko, levin, sam, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed Patch
jianli: commit-queue-
Proposed Patch (remove the code piece that should not be in this patch)
sam: review-, jianli: commit-queue-
Proposed Patch
jianli: commit-queue-
Proposed Patch
jianli: commit-queue-
Proposed Patch
jianli: commit-queue-
Proposed Patch levin: review+, jianli: commit-queue-

Jian Li
Reported 2010-06-14 13:52:14 PDT
Need to add ScriptExecutionContext argument to File/Blob constructor. This is needed in order to support Blob.url.
Attachments
Proposed Patch (46.28 KB, patch)
2010-06-14 13:54 PDT, Jian Li
jianli: commit-queue-
Proposed Patch (remove the code piece that should not be in this patch) (44.62 KB, patch)
2010-06-14 13:57 PDT, Jian Li
sam: review-
jianli: commit-queue-
Proposed Patch (50.49 KB, patch)
2010-06-14 17:22 PDT, Jian Li
jianli: commit-queue-
Proposed Patch (54.87 KB, patch)
2010-07-22 17:44 PDT, Jian Li
jianli: commit-queue-
Proposed Patch (54.88 KB, patch)
2010-07-23 14:47 PDT, Jian Li
jianli: commit-queue-
Proposed Patch (54.88 KB, patch)
2010-07-23 16:12 PDT, Jian Li
levin: review+
jianli: commit-queue-
Jian Li
Comment 1 2010-06-14 13:54:03 PDT
Created attachment 58695 [details] Proposed Patch
Jian Li
Comment 2 2010-06-14 13:57:52 PDT
Created attachment 58698 [details] Proposed Patch (remove the code piece that should not be in this patch)
Sam Weinig
Comment 3 2010-06-14 14:29:57 PDT
Comment on attachment 58698 [details] Proposed Patch (remove the code piece that should not be in this patch) It is a layering violation for files in the Platform directory to use the Frame class. r-.
Jian Li
Comment 4 2010-06-14 14:52:27 PDT
Thanks for pointing it out. I am not sure what is the other way to solve this problem: we need to get current ScriptExecutionContext and pass it to File::create in Clipboard*** implementations for Clipboard::files(). I know there is a way in V8 to get current ScriptExecutionContext without relying on Frame, but I do not know whether JSC supports this or not. Sam, do yo know? If we cannot get the ScriptExecutionContext without Frame, we will have to think of some way to pass Frame or similar thing to the Clipboard implementations under platform directory. What are the restrictions for files in the platform directory: are they not allowed to use any class from the non-platform directory? I have seen some dependencies, like DragData referring to Document.
Sam Weinig
Comment 5 2010-06-14 15:25:58 PDT
(In reply to comment #4) > Thanks for pointing it out. I am not sure what is the other way to solve this problem: we need to get current ScriptExecutionContext and pass it to File::create in Clipboard*** implementations for Clipboard::files(). I know there is a way in V8 to get current ScriptExecutionContext without relying on Frame, but I do not know whether JSC supports this or not. Sam, do yo know? The Clipboard object can refer to the Frame, just not the classes in Platform. ClipboardMac, for instance, already has a Frame reference. > > If we cannot get the ScriptExecutionContext without Frame, we will have to think of some way to pass Frame or similar thing to the Clipboard implementations under platform directory. What are the restrictions for files in the platform directory: are they not allowed to use any class from the non-platform directory? I have seen some dependencies, like DragData referring to Document. Classes in the platform directory should only uses classes in the platform directory or in WTF. Places like DragData using Document are bugs and should be fixed.
Sam Weinig
Comment 6 2010-06-14 15:29:27 PDT
(In reply to comment #5) > (In reply to comment #4) > > Thanks for pointing it out. I am not sure what is the other way to solve this problem: we need to get current ScriptExecutionContext and pass it to File::create in Clipboard*** implementations for Clipboard::files(). I know there is a way in V8 to get current ScriptExecutionContext without relying on Frame, but I do not know whether JSC supports this or not. Sam, do yo know? > > The Clipboard object can refer to the Frame, just not the classes in Platform. ClipboardMac, for instance, already has a Frame reference. I see now that the frame is only set when using the Clipboard from the EventHandler class. I am not sure how you should solve this, but adding an additional layering violation is not the way. It seems clear that DragData should not be creating Clipboards in the first place, as that itself is also a layering violation.
Jian Li
Comment 7 2010-06-14 17:22:38 PDT
Created attachment 58732 [details] Proposed Patch Remove DragData::createClipboard and add Clipboard::create for it to be directly in DragController.
WebKit Review Bot
Comment 8 2010-06-14 17:25:26 PDT
Attachment 58732 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/dom/Clipboard.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Jian Li
Comment 9 2010-07-22 17:44:11 PDT
Created attachment 62368 [details] Proposed Patch
WebKit Review Bot
Comment 10 2010-07-22 17:49:08 PDT
Attachment 62368 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/dom/Clipboard.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 11 2010-07-22 20:02:26 PDT
Jian Li
Comment 12 2010-07-23 14:47:59 PDT
Created attachment 62464 [details] Proposed Patch Fixed GTK build error.
WebKit Review Bot
Comment 13 2010-07-23 14:50:17 PDT
Attachment 62464 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/dom/Clipboard.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 14 2010-07-23 16:05:57 PDT
Jian Li
Comment 15 2010-07-23 16:12:53 PDT
Created attachment 62471 [details] Proposed Patch Fixed gtk build errors.
WebKit Review Bot
Comment 16 2010-07-23 16:15:42 PDT
Attachment 62471 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/dom/Clipboard.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.
David Levin
Comment 17 2010-07-26 16:42:33 PDT
Comment on attachment 62471 [details] Proposed Patch Nice clean up of the layering violation that was present. In WebCore/platform/DragData.h, please consider removing the forward declaration of "class Clipboard" as it is no longer needed. > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > +2010-07-22 Jian Li <jianli@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Add ScriptExecutionContext argument to File/Blob constructors. > + https://bugs.webkit.org/show_bug.cgi?id=40587 > + > + Remove DragData::createClipboard and add Clipboard::create for it to be > + called directly in DragController. This is because we need to pass > + Frame pointer to Clipboard class and use it to get ScriptExecutionContext > + in order to construct File objectsin Clipboard::files(). typo: objectsin > > + * bindings/js/SerializedScriptValue.cpp: > + (WebCore::DeserializingTreeWalker::convertIfTerminal): > + * bindings/v8/SerializedScriptValue.cpp: > + (WebCore::ZigZag::Reader::readBlob): > + (WebCore::ZigZag::Reader::readFile): > + (WebCore::ZigZag::Reader::readFileList): > + * dom/Clipboard.h: > + * editing/Editor.cpp: > + (WebCore::Editor::dispatchCPPEvent): > + * editing/Editor.h: > + * editing/android/EditorAndroid.cpp: > + (WebCore::Editor::newGeneralClipboard): > + * editing/brew/EditorBrew.cpp: > + (WebCore::Editor::newGeneralClipboard): > + * editing/chromium/EditorChromium.cpp: > + (WebCore::Editor::newGeneralClipboard): > + * editing/haiku/EditorHaiku.cpp: > + (WebCore::Editor::newGeneralClipboard): > + * editing/mac/EditorMac.mm: > + (WebCore::Editor::newGeneralClipboard): > + * editing/qt/EditorQt.cpp: > + (WebCore::Editor::newGeneralClipboard): > + * editing/wx/EditorWx.cpp: > + (WebCore::Editor::newGeneralClipboard): > + * html/Blob.cpp: > + (WebCore::Blob::Blob): > + (WebCore::Blob::slice): > + * html/Blob.h: > + (WebCore::Blob::create): > + * html/Blob.idl: > + * html/BlobBuilder.cpp: > + (WebCore::BlobBuilder::getBlob): > + * html/BlobBuilder.h: > + * html/BlobBuilder.idl: > + * html/File.cpp: > + (WebCore::File::File): > + * html/File.h: > + (WebCore::File::create): > + * html/HTMLInputElement.cpp: > + (WebCore::HTMLInputElement::appendFormData): > + (WebCore::HTMLInputElement::setFileListFromRenderer): > + * page/DragController.cpp: > + (WebCore::DragController::dragExited): > + (WebCore::DragController::performDrag): > + (WebCore::DragController::tryDHTMLDrag): > + * page/chromium/EventHandlerChromium.cpp: > + (WebCore::EventHandler::createDraggingClipboard): > + * page/gtk/EventHandlerGtk.cpp: > + (WebCore::EventHandler::createDraggingClipboard): > + * page/win/EventHandlerWin.cpp: > + (WebCore::EventHandler::createDraggingClipboard): > + * platform/DragData.h: > + * platform/android/ClipboardAndroid.cpp: > + (WebCore::Clipboard::create): > + * platform/android/DragDataAndroid.cpp: > + * platform/brew/ClipboardBrew.cpp: > + (WebCore::Clipboard::create): > + * platform/brew/DragDataBrew.cpp: > + * platform/chromium/ClipboardChromium.cpp: > + (WebCore::Clipboard::create): > + (WebCore::ClipboardChromium::ClipboardChromium): > + (WebCore::ClipboardChromium::create): > + (WebCore::ClipboardChromium::files): > + * platform/chromium/ClipboardChromium.h: > + * platform/chromium/DragDataChromium.cpp: > + * platform/efl/ClipboardEfl.cpp: > + (WebCore::Editor::newGeneralClipboard): > + (WebCore::Clipboard::create): > + * platform/efl/DragDataEfl.cpp: > + * platform/gtk/ClipboardGtk.cpp: > + (WebCore::Editor::newGeneralClipboard): > + (WebCore::Clipboard::create): > + (WebCore::ClipboardGtk::ClipboardGtk): > + (WebCore::ClipboardGtk::files): > + * platform/gtk/ClipboardGtk.h: > + (WebCore::ClipboardGtk::create): > + * platform/gtk/DragDataGtk.cpp: > + * platform/haiku/ClipboardHaiku.cpp: > + (WebCore::Clipboard::create): > + * platform/haiku/DragDataHaiku.cpp: > + * platform/mac/ClipboardMac.mm: > + (WebCore::Clipboard::create): > + (WebCore::ClipboardMac::files): > + * platform/mac/DragDataMac.mm: > + * platform/qt/ClipboardQt.cpp: > + (WebCore::Clipboard::create): > + * platform/qt/DragDataQt.cpp: > + * platform/win/ClipboardWin.cpp: > + (WebCore::Clipboard::create): > + (WebCore::ClipboardWin::ClipboardWin): > + (WebCore::ClipboardWin::files): > + * platform/win/ClipboardWin.h: > + (WebCore::ClipboardWin::create): > + * platform/win/DragDataWin.cpp: > + * platform/win/EditorWin.cpp: > + (WebCore::Editor::newGeneralClipboard): > + * platform/wince/DragDataWince.cpp: > + * platform/wince/EditorWince.cpp: > + (WebCore::Editor::newGeneralClipboard): > + * platform/wx/ClipboardWx.cpp: > + (WebCore::Clipboard::create): > + * platform/wx/DragDataWx.cpp: In the future please consider, that it's nice to have short comments here explaining what was done in each place (see any of Darin Adler's check-ins for good examples). WebCore/platform/haiku/ClipboardHaiku.cpp:45 + WTF::PassRefPtr<Clipboard> Clipboard::create(ClipboardAccessPolicy policy, DragData*, Frame*) The WTF:: prefix shouldn't be needed here.
Jian Li
Comment 18 2010-07-27 14:17:05 PDT
Note You need to log in before you can comment on or make changes to this bug.