Summary: | Add ScriptExecutionContext argument to File/Blob constructors | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jian Li <jianli> | ||||||||||||||
Component: | WebCore JavaScript | Assignee: | 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
Jian Li
2010-06-14 13:52:14 PDT
Created attachment 58695 [details]
Proposed Patch
Created attachment 58698 [details]
Proposed Patch (remove the code piece that should not be in this patch)
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-.
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. (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. (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. Created attachment 58732 [details]
Proposed Patch
Remove DragData::createClipboard and add Clipboard::create for it to be directly in DragController.
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.
Created attachment 62368 [details]
Proposed Patch
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.
Attachment 62368 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3561341 Created attachment 62464 [details]
Proposed Patch
Fixed GTK build error.
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.
Attachment 62464 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3617058 Created attachment 62471 [details]
Proposed Patch
Fixed gtk build errors.
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.
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. Committed as http://trac.webkit.org/changeset/64152. |