Bug 40587 – Add ScriptExecutionContext argument to File/Blob constructors

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Jian Li]

Need to add ScriptExecutionContext argument to File/Blob constructor. This is needed in order to support Blob.url.

[Jian Li]

Created attachment 58695 [details]
Proposed Patch

[Jian Li]

Created attachment 58698 [details]
Proposed Patch (remove the code piece that should not be in this patch)

[Sam Weinig]

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]

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]

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

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

Created attachment 58732 [details]
Proposed Patch

Remove DragData::createClipboard and add Clipboard::create for it to be directly in DragController.

[WebKit Review Bot]

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]

Created attachment 62368 [details]
Proposed Patch

[WebKit Review Bot]

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]

Attachment 62368 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3561341

[Jian Li]

Created attachment 62464 [details]
Proposed Patch

Fixed GTK build error.

[WebKit Review Bot]

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]

Attachment 62464 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3617058

[Jian Li]

Created attachment 62471 [details]
Proposed Patch

Fixed gtk build errors.

[WebKit Review Bot]

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 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
> [phone]  Jian Li  <[e-mail]>
> +
> +        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]

Committed as http://trac.webkit.org/changeset/64152.