Bug 154729

Created attachment 272331 [details] Patch

Attachment 272331 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestClassWithJSBuiltinConstructorAndCallWith.mm:30: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestClassWithJSBuiltinConstructorAndCallWith.cpp:181: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 272498 [details] Patch

Attachment 272498 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestClassWithJSBuiltinConstructorAndCallWith.mm:30: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestClassWithJSBuiltinConstructorAndCallWith.cpp:181: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 272498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272498&action=review > Source/WebCore/Modules/fetch/FetchResponse.cpp:165 > + return true; Does the comment in XMLHttpRequest::canSuspendForDocumentSuspension apply here? > Source/WebCore/bindings/js/JSDOMConstructor.h:224 > + if (!object) > + return throwConstructorDocumentUnavailableError(*state, info()->className); Why is this needed now for the fetch api and it wasn't needed before? > Source/WebCore/bindings/scripts/test/TestClassWithJSBuiltinConstructorAndCallWith.idl:2 > + * Copyright (C) 2015 Canon Inc. 2016

(In reply to comment #5) > Comment on attachment 272498 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=272498&action=review > > > Source/WebCore/Modules/fetch/FetchResponse.cpp:165 > > + return true; > > Does the comment in XMLHttpRequest::canSuspendForDocumentSuspension apply > here? It seems to make sense when doing network loads from fetch. For blob (i.e. converting a blob stored in a body as a string or array buffer), I am less sure about it. Plan is to implement Blob conversion first, using the simple approach (if loading, do not suspend). This should be revisited when doing network loads. > > > Source/WebCore/bindings/js/JSDOMConstructor.h:224 > > + if (!object) > > + return throwConstructorDocumentUnavailableError(*state, info()->className); > > Why is this needed now for the fetch api and it wasn't needed before? This mimicks the checks for regular constructors called with ScriptExecutionContext. This check was not needed as createJSObject could not return a null pointer in any valid case before that patch. > > Source/WebCore/bindings/scripts/test/TestClassWithJSBuiltinConstructorAndCallWith.idl:2 > > + * Copyright (C) 2015 Canon Inc. > > 2016 OK

> It seems to make sense when doing network loads from fetch. > For blob (i.e. converting a blob stored in a body as a string or array > buffer), I am less sure about it. > Plan is to implement Blob conversion first, using the simple approach (if > loading, do not suspend). > This should be revisited when doing network loads. This probably only applies to FetchResponse, FetchRequest having only to handle Blob.

Created attachment 272740 [details] Rebasing

Attachment 272740 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestClassWithJSBuiltinConstructorAndCallWith.mm:30: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestClassWithJSBuiltinConstructorAndCallWith.cpp:181: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.

Thanks for the feedback. I rebased the patch and made some updates according your comments/questions. (In reply to comment #5) > Comment on attachment 272498 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=272498&action=review > > > Source/WebCore/Modules/fetch/FetchResponse.cpp:165 > > + return true; > > Does the comment in XMLHttpRequest::canSuspendForDocumentSuspension apply > here? Added some info in Changelog about suspending request and response. > > > Source/WebCore/bindings/js/JSDOMConstructor.h:224 > > + if (!object) > > + return throwConstructorDocumentUnavailableError(*state, info()->className); > > Why is this needed now for the fetch api and it wasn't needed before? Added some info in the Changelog also. > > > Source/WebCore/bindings/scripts/test/TestClassWithJSBuiltinConstructorAndCallWith.idl:2 > > + * Copyright (C) 2015 Canon Inc. > > 2016 Fixed.

Comment on attachment 272740 [details] Rebasing View in context: https://bugs.webkit.org/attachment.cgi?id=272740&action=review > Source/WebCore/bindings/js/JSDOMConstructor.h:224 > + if (!object) > + return throwConstructorDocumentUnavailableError(*state, info()->className); This patch looks good to me except this. This adds a branch to some pretty hot code, and I imagine it would significantly affect performance. I'd like someone more familiar with the bindings to look at this. If this is indeed newly needed, then I think there should be a new inline function that has this branch so we don't add this branch to all existing bindings.

Comment on attachment 272740 [details] Rebasing View in context: https://bugs.webkit.org/attachment.cgi?id=272740&action=review >> Source/WebCore/bindings/js/JSDOMConstructor.h:224 >> + return throwConstructorDocumentUnavailableError(*state, info()->className); > > This patch looks good to me except this. This adds a branch to some pretty hot code, and I imagine it would significantly affect performance. I'd like someone more familiar with the bindings to look at this. If this is indeed newly needed, then I think there should be a new inline function that has this branch so we don't add this branch to all existing bindings. It indeed adds a check for all DOM objects having a JS built-in constructor, while there were none previously. It has no effect for other DOM objects though. The number of JS built-in constructed DOM classes is fairly low at the moment but it may increase in the future. The ususal way of fixing this would be to update the binding generator to generate that check when needed but that might require the binding generator to generate the whole JSBuiltinConstructor::construct. An alternative would be to update JSBuiltinConstructor by adding a template/compile-time boolean parameter to activate or not this check. If that is found worthwhile, I can tackle this as a follow-up patch.

Comment on attachment 272740 [details] Rebasing View in context: https://bugs.webkit.org/attachment.cgi?id=272740&action=review > Source/WebCore/Modules/fetch/FetchRequest.h:87 > + FetchRequest(FetchBody&&, Ref<FetchHeaders>&&, InternalRequest&&, ScriptExecutionContext&); ScriptExecutionContext& seems usually to be the first constructor parameter. I plan to update this in a next patch or when landing this one.

Comment on attachment 272740 [details] Rebasing View in context: https://bugs.webkit.org/attachment.cgi?id=272740&action=review >> Source/WebCore/Modules/fetch/FetchRequest.h:87 >> + FetchRequest(FetchBody&&, Ref<FetchHeaders>&&, InternalRequest&&, ScriptExecutionContext&); > > ScriptExecutionContext& seems usually to be the first constructor parameter. > I plan to update this in a next patch or when landing this one. Contexts like this one, GraphicsContext, ExecState, Document, Frame, Page, etc. are conventionally the first parameter to any function. That’s the pattern you are seeing and considering following. > Source/WebCore/Modules/fetch/FetchResponse.cpp:54 > -Ref<FetchResponse> FetchResponse::error() > +Ref<FetchResponse> FetchResponse::error(ScriptExecutionContext* context) > { > - return adoptRef(*new FetchResponse(Type::Error, { }, FetchHeaders::create(FetchHeaders::Guard::Immutable), ResourceResponse())); > + return adoptRef(*new FetchResponse(Type::Error, { }, FetchHeaders::create(FetchHeaders::Guard::Immutable), ResourceResponse(), *context)); > } This code assumes the context is non-null, so the argument should be ScriptExecutionContext&. If it can’t be a reference, then instead we at least have to figure out where the guarantee that the argument is non-null comes from. > Source/WebCore/Modules/fetch/FetchResponse.cpp:68 > + RefPtr<FetchResponse> redirectResponse = adoptRef(*new FetchResponse(Type::Default, { }, FetchHeaders::create(FetchHeaders::Guard::Immutable), ResourceResponse(), *context)); Same issue here. > Source/WebCore/Modules/fetch/FetchResponse.cpp:127 > + RefPtr<FetchResponse> cloned = adoptRef(*new FetchResponse(m_type, FetchBody(m_body), FetchHeaders::create(headers()), ResourceResponse(m_response), *scriptExecutionContext())); Variation on the same issue here. What guarantees scriptExecutionContext() is non-null, and if nothing does, then why does it return a pointer rather than a reference? >>> Source/WebCore/bindings/js/JSDOMConstructor.h:224 >>> + return throwConstructorDocumentUnavailableError(*state, info()->className); >> >> This patch looks good to me except this. This adds a branch to some pretty hot code, and I imagine it would significantly affect performance. I'd like someone more familiar with the bindings to look at this. If this is indeed newly needed, then I think there should be a new inline function that has this branch so we don't add this branch to all existing bindings. > > It indeed adds a check for all DOM objects having a JS built-in constructor, while there were none previously. > It has no effect for other DOM objects though. > The number of JS built-in constructed DOM classes is fairly low at the moment but it may increase in the future. > > The ususal way of fixing this would be to update the binding generator to generate that check when needed but that might require the binding generator to generate the whole JSBuiltinConstructor::construct. > An alternative would be to update JSBuiltinConstructor by adding a template/compile-time boolean parameter to activate or not this check. > If that is found worthwhile, I can tackle this as a follow-up patch. Another way to do it is to change the createJSObject function so that in some classes it returns a pointer and in others it returns a reference. Then we can use a private member function that is overloaded: return callConstructor(*state, castedThis->createJSObject(), *castedThis->initializeFunction()); There are two inline callConstructor functions, one that takes a JSObject& and another that takes a JSObject*. The JSObject* one does this: if (!object) return throwConstructorDocumentUnavailableError(state, info()->className); return callConstructor(state, object, function); And the JSObject& one does this: callFunctionWithCurrentArguments(state, object,function); return JSC::JSValue::encode(&object); All we have to do is make the createJSObject function return a reference when known to be non-null, and a pointer when it might be null.

> > Source/WebCore/Modules/fetch/FetchResponse.cpp:54 > > -Ref<FetchResponse> FetchResponse::error() > > +Ref<FetchResponse> FetchResponse::error(ScriptExecutionContext* context) > > { > > - return adoptRef(*new FetchResponse(Type::Error, { }, FetchHeaders::create(FetchHeaders::Guard::Immutable), ResourceResponse())); > > + return adoptRef(*new FetchResponse(Type::Error, { }, FetchHeaders::create(FetchHeaders::Guard::Immutable), ResourceResponse(), *context)); > > } > > This code assumes the context is non-null, so the argument should be > ScriptExecutionContext&. If it can’t be a reference, then instead we at > least have to figure out where the guarantee that the argument is non-null > comes from. The binding generated code does the check and should pass a reference. This is done for constructors but not yet for methods. > > Source/WebCore/Modules/fetch/FetchResponse.cpp:68 > > + RefPtr<FetchResponse> redirectResponse = adoptRef(*new FetchResponse(Type::Default, { }, FetchHeaders::create(FetchHeaders::Guard::Immutable), ResourceResponse(), *context)); > > Same issue here. Ditto. > > Source/WebCore/Modules/fetch/FetchResponse.cpp:127 > > + RefPtr<FetchResponse> cloned = adoptRef(*new FetchResponse(m_type, FetchBody(m_body), FetchHeaders::create(headers()), ResourceResponse(m_response), *scriptExecutionContext())); > > Variation on the same issue here. What guarantees scriptExecutionContext() > is non-null, and if nothing does, then why does it return a pointer rather > than a reference? Right, for consistency, clone should be set as CallWith=ScriptExecutionContext I will fix that as a follow-up patch. > >>> Source/WebCore/bindings/js/JSDOMConstructor.h:224 > >>> + return throwConstructorDocumentUnavailableError(*state, info()->className); > >> > >> This patch looks good to me except this. This adds a branch to some pretty hot code, and I imagine it would significantly affect performance. I'd like someone more familiar with the bindings to look at this. If this is indeed newly needed, then I think there should be a new inline function that has this branch so we don't add this branch to all existing bindings. > > > > It indeed adds a check for all DOM objects having a JS built-in constructor, while there were none previously. > > It has no effect for other DOM objects though. > > The number of JS built-in constructed DOM classes is fairly low at the moment but it may increase in the future. > > > > The ususal way of fixing this would be to update the binding generator to generate that check when needed but that might require the binding generator to generate the whole JSBuiltinConstructor::construct. > > An alternative would be to update JSBuiltinConstructor by adding a template/compile-time boolean parameter to activate or not this check. > > If that is found worthwhile, I can tackle this as a follow-up patch. > > Another way to do it is to change the createJSObject function so that in > some classes it returns a pointer and in others it returns a reference. Then > we can use a private member function that is overloaded: > > return callConstructor(*state, castedThis->createJSObject(), > *castedThis->initializeFunction()); > > There are two inline callConstructor functions, one that takes a JSObject& > and another that takes a JSObject*. The JSObject* one does this: > > if (!object) > return throwConstructorDocumentUnavailableError(state, > info()->className); > return callConstructor(state, object, function); > > And the JSObject& one does this: > > callFunctionWithCurrentArguments(state, object,function); > return JSC::JSValue::encode(&object); > > All we have to do is make the createJSObject function return a reference > when known to be non-null, and a pointer when it might be null. I will handle this as a separated bug.

Created attachment 273007 [details] Patch based on fix 155022

Created attachment 273162 [details] Updating style

Comment on attachment 273162 [details] Updating style View in context: https://bugs.webkit.org/attachment.cgi?id=273162&action=review > Source/WebCore/Modules/fetch/FetchRequest.h:91 > + const char* activeDOMObjectName() const override; > + bool canSuspendForDocumentSuspension() const override; Should say final instead of override. > Source/WebCore/Modules/fetch/FetchResponse.h:84 > + const char* activeDOMObjectName() const override; > + bool canSuspendForDocumentSuspension() const override; Should be final, not override.

Created attachment 273279 [details] Patch for landing

Comment on attachment 273279 [details] Patch for landing Clearing flags on attachment: 273279 Committed r197744: <http://trac.webkit.org/changeset/197744>

All reviewed patches have been landed. Closing bug.