[Streams API] Create CountQueuingStrategy object as per spec
Created attachment 256109 [details] Patch with [StaticToPrototype] I didn't mark it as r? because I still need to add the building stuff for Mac, but please, review the current code.
Comment on attachment 256109 [details] Patch with [StaticToPrototype] View in context: https://bugs.webkit.org/attachment.cgi?id=256109&action=review > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:53 > + argumentObject = JSFinalObject::create(state->vm(), JSFinalObject::createStructure(state->vm(), state->callee()->globalObject(), jsNull(), 1)); Creating an object here seems overkill. > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:61 > + setPropertyToObject(*state, *jsCountQueuingStrategy.getObject(), "highWaterMark", jsHighWaterMark); I wonder whether we could not simplify this a bit. Here are some suggestions: - make highWaterMark a member of the DOM class - make the constructor not custom and have as parameter a const Dictionary&, so that "highWaterMark" parsing is made easier - check whether having "highWaterMark" in prototype is something that the spec wants to prohibit (If allowed, we could have it as an IDL attribute) - If IDL attribute is not feasible, try fixing getter using JSCustomGetOwnPropertySlotAndDescriptor or similar > Source/WebCore/bindings/js/StreamsJSUtils.h:48 > +} These setter and getter are not specific to streams. If we find them useful, there may be an existing WebCore/bindings/js header where it could be added (JSDOMBinding.h? ScriptState.h?)
Comment on attachment 256109 [details] Patch with [StaticToPrototype] View in context: https://bugs.webkit.org/attachment.cgi?id=256109&action=review > Source/WebCore/Modules/streams/CountQueuingStrategy.idl:33 > + SkipVTableValidation I do not think we need this one. > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:58 > + JSValue jsHighWaterMark = getPropertyFromObject(*state, *argumentObject, "highWaterMark"); I know the spec is not mandating that but shouldn't we check that hwm is a non negative number? Also, jsHighWaterMark may have an undefined value, is it ok?
(In reply to comment #2) > > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:53 > > + argumentObject = JSFinalObject::create(state->vm(), JSFinalObject::createStructure(state->vm(), state->callee()->globalObject(), jsNull(), 1)); > > Creating an object here seems overkill. > > > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:61 > > + setPropertyToObject(*state, *jsCountQueuingStrategy.getObject(), "highWaterMark", jsHighWaterMark); > > I wonder whether we could not simplify this a bit. > Here are some suggestions: > - make highWaterMark a member of the DOM class > - make the constructor not custom and have as parameter a const Dictionary&, > so that "highWaterMark" parsing is made easier > - check whether having "highWaterMark" in prototype is something that the > spec wants to prohibit (If allowed, we could have it as an IDL attribute) > - If IDL attribute is not feasible, try fixing getter using > JSCustomGetOwnPropertySlotAndDescriptor or similar I'll try to make this a bit easier. Anyway, I just copied the structure of ReadableStream. > > Source/WebCore/bindings/js/StreamsJSUtils.h:48 > > +} > > These setter and getter are not specific to streams. > If we find them useful, there may be an existing WebCore/bindings/js header > where it could be added (JSDOMBinding.h? ScriptState.h?) Yes, we might have wanted to do this at the very beginning with getPropertyFromObject already. (In reply to comment #3) > > Source/WebCore/Modules/streams/CountQueuingStrategy.idl:33 > > + SkipVTableValidation > > I do not think we need this one. Ok. > > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:58 > > + JSValue jsHighWaterMark = getPropertyFromObject(*state, *argumentObject, "highWaterMark"); > > I know the spec is not mandating that but shouldn't we check that hwm is a > non negative number? > Also, jsHighWaterMark may have an undefined value, is it ok? No, it is not, the tests specify that you should be able to put garbage as HWM at the beginning as long as it works later and if I am not mistaken, it is checked later.
Created attachment 256298 [details] Patch with [FullCustom] Alternate implementation with [FullCustom]
(In reply to comment #5) > Created attachment 256298 [details] > Patch with [FullCustom] > > Alternate implementation with [FullCustom] I adapted the implementation to the one exposed in bug 146593. I also fixed some other things in the comments but this is not a final patch, though I'd appreciate some review in case anything can be improved. Pending this are trying to remove the CustomConstructor in favor of the Dictionary impl as Youenn suggested.
Comment on attachment 256298 [details] Patch with [FullCustom] View in context: https://bugs.webkit.org/attachment.cgi?id=256298&action=review > Source/WebCore/ChangeLog:17 > + * CMakeLists.txt: Added files to compile. > + * Modules/streams/CountQueuingStrategy.cpp: Added. Can we do without this file, inlining everything in the header file. Counter may not be very useful for this class. > Source/WebCore/ChangeLog:29 > + (WebCore::setPropertyToObject): Added to create a property into an object. I guess these methods will be moved in another file in a future version. > Source/WebCore/Modules/streams/CountQueuingStrategy.h:37 > +#include <wtf/RefPtr.h> Unneeded include > Source/WebCore/Modules/streams/CountQueuingStrategy.h:45 > +class CountQueuingStrategy : public ScriptWrappable, public RefCounted<CountQueuingStrategy> { Is ScriptWrappable needed? > Source/WebCore/Modules/streams/CountQueuingStrategy.h:48 > + virtual ~CountQueuingStrategy(); Is it needed? > Source/WebCore/Modules/streams/CountQueuingStrategy.idl:31 > + CustomConstructor(any properties), To remove this CustomConstructor, ReadableStream may serve as an example > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:44 > + return JSValue::encode(jsNumber(1)); Would it be nice to define a static size() class in CQS and use it here or is it overkill? > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:56 > + return throwVMError(state, createTypeError(state, ASCIILiteral("First argument, if any, should be an object"))); There may be a way to reorder the tests to make the code easier to read. Maybe something like: if (!value.isObject()) { if (!value.isUndefined()) return throw(...) return toJS(...) } JSObject* ... > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:58 > + RefPtr<CountQueuingStrategy> countQueuingStrategy = adoptRef(*new CountQueuingStrategy()); Should be a Ref<>, not a RefPtr<>. Also, since it is only used once, it may be inlined within toJS() below. > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:65 > + setPropertyToObject(*state, *jsCountQueuingStrategy.getObject(), "highWaterMark", jsHighWaterMark); If hwm cannot be strongly typed, I am not sure it is actually worth the effort to remove the custom constructor. Do you know what is the rationale of the hwm design? In particular why it is not a prototype attribute and why it is not typed as a double?
Created attachment 256972 [details] Implementation inside JSC This implements CountQueuingStrategy inside JSC
(In reply to comment #8) > Created attachment 256972 [details] > Implementation inside JSC > > This implements CountQueuingStrategy inside JSC As you can see, this implementation could be too complex compared to the WebCore one.
(In reply to comment #9) > (In reply to comment #8) > > Created attachment 256972 [details] > > Implementation inside JSC > > > > This implements CountQueuingStrategy inside JSC > > As you can see, this implementation could be too complex compared to the > WebCore one. That sounds a lot of code given the few js lines needed by the JS prototype. Would it be worth improving the tooling to remove most of the generic code and implement the functionality within JavaScriptCore builtins?
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > Created attachment 256972 [details] > > > Implementation inside JSC > > > > > > This implements CountQueuingStrategy inside JSC > > > > As you can see, this implementation could be too complex compared to the > > WebCore one. > > That sounds a lot of code given the few js lines needed by the JS prototype. > > Would it be worth improving the tooling to remove most of the generic code > and implement the functionality within JavaScriptCore builtins? Could you please be more specific? What things would you improve in the tooling and what exactly would you implement as JavaScriptCore builtins?
Created attachment 257087 [details] Implementation inside JSC Add build support for Mac
Created attachment 257091 [details] Implementation inside JSC Add build support for Windows
Created attachment 257094 [details] Implementation inside JSC Removed useless header files from cpps
Created attachment 257113 [details] Implementation inside JSC (with builtins) Moved CountQueuingStrategy.protosize.size to builtins. Code ends up being more complicated than without the builtins...
Created attachment 257168 [details] Implementation inside JSC Fixed headers.
Created attachment 257177 [details] Implementation inside JSC (with builtins) Fixed headers
Attachment 257177 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/StreamsCountQueuingStrategyPrototype.cpp:45: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 257192 [details] Implementation inside JSC I am focusing only in the non builtin option as having builtin functions for this easy case could be more complicated. I added a custom test about the hwm and improved the code in that regards. I'd really appreciate some more input here so that I can continue with the implementation in any direction.
Created attachment 257260 [details] Implementation inside JSC Tried the new custom test with the reference implementation and tuned it to match the behavior. I'd really appreciate some more input here so that I can continue with the implementation in any direction.
Created attachment 257348 [details] Implementation inside JSC Updated custom test with comments and added a test for size to ensure it returns 1 in all circumstances (This won't be same case of ByteLength, but I think it is a good idea to keep it like this for consistency). I'd really appreciate some more input here so that I can continue with the implementation in any direction.
Comment on attachment 257348 [details] Implementation inside JSC View in context: https://bugs.webkit.org/attachment.cgi?id=257348&action=review > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:359 > +#if ENABLE(STREAMS_API) > + m_streamsCountQueuingStrategyPrototype.set(vm, this, StreamsCountQueuingStrategyPrototype::create(exec, this, StreamsCountQueuingStrategyPrototype::createStructure(vm, this, m_objectPrototype.get()))); > + m_streamsCountQueuingStrategyStructure.set(vm, this, StreamsCountQueuingStrategy::createStructure(vm, this, m_streamsCountQueuingStrategyPrototype.get())); > +#endif I don't think we want these properties to be available in all JavaScript environments, since they are not specified so by ECMAScript. Rather, we want them available in DOM JavaScript environments. > Source/JavaScriptCore/runtime/StreamsCountQueuingStrategy.cpp:64 > + JSValue argument = vm.topCallFrame->argument(0); This is wrong. The relevant arguments should be type checked and passed into finishCreation by the constructor. It is fragile to assume that all constructions of this object will occur with a constructor live at the top of the stack. > Source/JavaScriptCore/runtime/StreamsCountQueuingStrategyConstructor.cpp:108 > +StreamsCountQueuingStrategy* constructCountQueuingStrategy(ExecState* exec, JSGlobalObject* globalObject, JSFunction* resolver) > +{ > + StreamsCountQueuingStrategyConstructor* countQueuingStrategyConstructor = globalObject->streamsCountQueuingStrategyConstructor(); > + > + ConstructData constructData; > + ConstructType constructType = getConstructData(countQueuingStrategyConstructor, constructData); > + ASSERT(constructType != ConstructTypeNone); > + > + MarkedArgumentBuffer arguments; > + arguments.append(resolver); > + > + return jsCast<StreamsCountQueuingStrategy*>(construct(exec, countQueuingStrategyConstructor, constructType, constructData, arguments)); > +} This is wrong. You shouldn't need this. The function returned by getConstructData should perform the object creation directly.
Created attachment 257647 [details] Patch with [CustomObjectGenerateNoBindings] This refers patch supposes that we have landed patch for bug 146593 with CustomObjectGenerateNoBindings. The constructor sets highWaterMark and size is implemented just by returning 1 without any further binding with the object as the spec requires with much less boilerplater to create the objects. This patch is still WIP and wouldn't land because it has a dependency with bug 146593 and the compilation in Mac and Win is also missing.
Created attachment 257649 [details] Patch with [CustomObjectGenerateNoBindings] I had just uploaded an incomplete patch. This is the complete one
Created attachment 257752 [details] Patch with [CustomObjectGenerateNoBindings] I based the implementation again in the one for bug 146593 with [CustomObjectGenerateNoBindings]. I added support for Mac and Win compilation though in Mac I am getting a linking error I didn't catch yet: Undefined symbols for architecture x86_64: "__ZN7WebCore22JSCountQueuingStrategy14getConstructorERN3JSC2VMEPNS1_14JSGlobalObjectE", referenced from: __ZN7WebCore42jsDOMWindowCountQueuingStrategyConstructorEPN3JSC9ExecStateEPNS0_8JSObjectExNS0_12PropertyNameE in JSDOMWindow.o ld: symbol(s) not found for architecture x86_64 Symbol seems to be there so I don't know if I forgot to add something somewhere.
Created attachment 257949 [details] Patch with [CustomObjectGenerateNoBindings] Fixed the build in Mac. This and bug 146593 are ready for review, but I don't set it as compilation will fail because of the dependency.
Created attachment 257955 [details] Patch This time with ChangeLog improved.
Comment on attachment 257955 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257955&action=review > Source/WebCore/Modules/streams/CountQueuingStrategy.cpp:41 > +CountQueuingStrategy::CountQueuingStrategy() CountQueuingStrategy does nothing really, since we are not storing the hwm value. If it is not extended, we might want to get rid of it, at least the CPP file, if not the Header file. This class could be inlined in the custom binding file for instance. I do not think the leak counter is important enough to justify creating a new CPP file. As is CQS currently, it is sub-optimal to have it be ref countable, since this object is only owned by its JS counter part. It would be nice to remove this counter. The allocation of CQS in the custom binding would also be nice to remove, if it stays like this. > Source/WebCore/Modules/streams/CountQueuingStrategy.h:37 > +#include <wtf/RefPtr.h> Not needed. > Source/WebCore/Modules/streams/CountQueuingStrategy.h:41 > +class ScriptExecutionContext; Ditto. > Source/WebCore/Modules/streams/CountQueuingStrategy.h:45 > +class CountQueuingStrategy : public ScriptWrappable, public RefCounted<CountQueuingStrategy> { Do we need ScriptWrapprable? > Source/WebCore/Modules/streams/CountQueuingStrategy.h:48 > + virtual ~CountQueuingStrategy(); Does it need to be virtual? > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:44 > + return JSValue::encode(jsNumber(1)); If we keep CountQueingStrategy as a header file, would it make sense to have a inline static CountQueingStrategy::size() returning 1? > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:50 > + // improving the IDL generator). Unneeded comment. > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:55 > + RefPtr<CountQueuingStrategy> countQueuingStrategy = adoptRef(*new CountQueuingStrategy()); Should be a Ref<> not a RefPtr<> I guess. > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:56 > + JSValue jsCountQueuingStrategy = toJS(state, jsCast<JSDOMGlobalObject*>(state->callee()->globalObject()), WTF::move(countQueuingStrategy)); I might be wrong, but CREATE_DOM_WRAPPER might be a bit more efficient than toJS, since we create the object. > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:58 > + if (value.isObject()) { Maybe rewrite it as "if (!value.isObject()) return JSValue::encode(jsCountQueuingStrategy);" > Source/WebCore/bindings/js/ReadableJSStream.cpp:43 > +#include "StreamsJSUtils.h" As I said previously, StreamsJSUtils.h does not seem appealing given how generic the declared routines are. If it is found valuable to have these routines on a header, we should find a better location, for instance, JSDOMBinding.h
Created attachment 258061 [details] Patch Honored most of Youenn's suggestions.
(In reply to comment #28) > CountQueuingStrategy does nothing really, since we are not storing the hwm > value. > If it is not extended, we might want to get rid of it, at least the CPP > file, if not the Header file. > This class could be inlined in the custom binding file for instance. The class cannot be inlined because the bindings expect the header file to exist. > I do not think the leak counter is important enough to justify creating a > new CPP file. Removed cpp. > As is CQS currently, it is sub-optimal to have it be ref countable, since > this object is only owned by its JS counter part. > It would be nice to remove this counter. > The allocation of CQS in the custom binding would also be nice to remove, if > it stays like this. Cannot remove refcounting, also used at the bindings. > > Source/WebCore/Modules/streams/CountQueuingStrategy.h:41 > > +class ScriptExecutionContext; > > Ditto. Done. > > Source/WebCore/Modules/streams/CountQueuingStrategy.h:45 > > +class CountQueuingStrategy : public ScriptWrappable, public RefCounted<CountQueuingStrategy> { > > Do we need ScriptWrapprable? No, done. > > Source/WebCore/Modules/streams/CountQueuingStrategy.h:48 > > + virtual ~CountQueuingStrategy(); > > Does it need to be virtual? Besides being a good C++ practice to have destructors virtual, it derives from RefCounted, which needs the destructor virtual also. > > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:44 > > + return JSValue::encode(jsNumber(1)); > > If we keep CountQueingStrategy as a header file, would it make sense to have > a inline static CountQueingStrategy::size() returning 1? I did this though I don't see the point. Specially when in ByteLength we are not going to delegate to the class because it will be all JSC, which we want to keep away from Modules/streams > > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:50 > > + // improving the IDL generator). > > Unneeded comment. Done. > > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:55 > > + RefPtr<CountQueuingStrategy> countQueuingStrategy = adoptRef(*new CountQueuingStrategy()); > > Should be a Ref<> not a RefPtr<> I guess. Done. > > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:56 > > + JSValue jsCountQueuingStrategy = toJS(state, jsCast<JSDOMGlobalObject*>(state->callee()->globalObject()), WTF::move(countQueuingStrategy)); > > I might be wrong, but CREATE_DOM_WRAPPER might be a bit more efficient than > toJS, since we create the object. Done. > > Source/WebCore/bindings/js/JSStreamsCountQueuingStrategyCustom.cpp:58 > > + if (value.isObject()) { > > Maybe rewrite it as "if (!value.isObject()) return > JSValue::encode(jsCountQueuingStrategy);" I did this, though I really prefer to have it the other way as I think we should bail out only for errors, which wasn't the casew. > > Source/WebCore/bindings/js/ReadableJSStream.cpp:43 > > +#include "StreamsJSUtils.h" > > As I said previously, StreamsJSUtils.h does not seem appealing given how > generic the declared routines are. > If it is found valuable to have these routines on a header, we should find a > better location, for instance, JSDOMBinding.h Yes, done.
> > As is CQS currently, it is sub-optimal to have it be ref countable, since > > this object is only owned by its JS counter part. > > It would be nice to remove this counter. > > The allocation of CQS in the custom binding would also be nice to remove, if > > it stays like this. > CQS does not need to be RefCounted<>, it just needs to implement ref() and deref(). I am not sure it is the way to go (probably bad actually), but ref() could be a protected empty method and deref() a protected method freeing the CQS. I wonder whether there is the need to create a new CQS for each JSCQS. If so, this is annoying. I guess it might be a tradeoff to use the binding generator... Since CQS<->JSCQS are stored in a map of JSDOMGlobalObject, that might be the case. If we could instantiate several JSCQS on the same CQS, we could just have one static CQS for all JSCQS and ref/deref being dummy empty protected methods.
(In reply to comment #31) > CQS does not need to be RefCounted<>, it just needs to implement ref() and > deref(). True. > I am not sure it is the way to go (probably bad actually), but ref() could > be a protected empty method and deref() a protected method freeing the CQS. I think it doesn't pay off. > I wonder whether there is the need to create a new CQS for each JSCQS. > If so, this is annoying. I guess it might be a tradeoff to use the binding > generator... > Since CQS<->JSCQS are stored in a map of JSDOMGlobalObject, that might be > the case. > > If we could instantiate several JSCQS on the same CQS, we could just have > one static CQS for all JSCQS and ref/deref being dummy empty protected > methods. It could be an improvement if possible, yes. But I don't think it pays off either since this is not a class like WTF::String that is used everywhere, it's a reasoably small class instantiated from JS when somebody does a "new CountQueuingStrategy()".
Maybe Geoffrey has an opinion here and we can settle and land at last.
(In reply to comment #31) > > > As is CQS currently, it is sub-optimal to have it be ref countable, since > > > this object is only owned by its JS counter part. > > > It would be nice to remove this counter. > > > The allocation of CQS in the custom binding would also be nice to remove, if > > > it stays like this. > > > > CQS does not need to be RefCounted<>, it just needs to implement ref() and > deref(). > I am not sure it is the way to go (probably bad actually), but ref() could > be a protected empty method and deref() a protected method freeing the CQS. > > I wonder whether there is the need to create a new CQS for each JSCQS. > If so, this is annoying. I guess it might be a tradeoff to use the binding > generator... > Since CQS<->JSCQS are stored in a map of JSDOMGlobalObject, that might be > the case. > > If we could instantiate several JSCQS on the same CQS, we could just have > one static CQS for all JSCQS and ref/deref being dummy empty protected > methods. Besides, these improvements could be done in a follow up patch, if possible.
Comment on attachment 258061 [details] Patch A couple of additional cleaning points. For the points we discussed previously (ref/deref), I agree it is safer to wait for some additional advices before making any change. View in context: https://bugs.webkit.org/attachment.cgi?id=258061&action=review > Source/WebCore/ChangeLog:15 > + Covered by current tests. There is indeed good coverage in LayoutTests/streams/reference-implementation/count-queing-strategy.html, in particular for error cases. Might be nice to add the link to the test file to help review. > Source/WebCore/ChangeLog:33 > + * bindings/js/JSStreamsCountQueuingStrategyCustom.cpp: Added. File name seems strange. Should it be JSCountQueuingStrategyCustom.cpp? > Source/WebCore/CMakeLists.txt:1195 > + bindings/js/JSStreamsCountQueuingStrategyCustom.cpp Ditto. > Source/WebCore/Modules/streams/CountQueuingStrategy.h:36 > +#include <wtf/RefPtr.h> include is probably not needed... ;) > Source/WebCore/Modules/streams/CountQueuingStrategy.h:44 > + explicit CountQueuingStrategy() { } Constructor might not be needed. > Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters:4461 > + <ClCompile Include="..\bindings\js\JSStreamsCountQueuingStrategyCustom.cpp"> Filename to be changed? > Source/WebCore/WebCore.xcodeproj/project.pbxproj:619 > + 148B4FFA1B69045400C954E4 /* StreamsJSUtils.h in Headers */ = {isa = PBXBuildFile; fileRef = 148B4FF91B69045400C954E4 /* StreamsJSUtils.h */; }; This should be removed. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:7756 > + 148B4FF91B69045400C954E4 /* StreamsJSUtils.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = StreamsJSUtils.h; path = bindings/js/StreamsJSUtils.h; sourceTree = "<group>"; }; This should be removed. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:15078 > + 148B4FF91B69045400C954E4 /* StreamsJSUtils.h */, This should be removed. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:26020 > + 148B4FFA1B69045400C954E4 /* StreamsJSUtils.h in Headers */, This should be removed.
Created attachment 258169 [details] Patch Honored again most of Youenn's suggestions. I think this might be the definitive version :)
(In reply to comment #35) > > Source/WebCore/ChangeLog:15 > > + Covered by current tests. > > There is indeed good coverage in > LayoutTests/streams/reference-implementation/count-queing-strategy.html, in > particular for error cases. > Might be nice to add the link to the test file to help review. Done. Added brand check tests too, cause they are involved. > > Source/WebCore/ChangeLog:33 > > + * bindings/js/JSStreamsCountQueuingStrategyCustom.cpp: Added. > > File name seems strange. Should it be JSCountQueuingStrategyCustom.cpp? I had man doubts about adding the prefix because it didn't follow the Streams trend of file naming, but I wanted to ensure that people knew without opening the file that it was Streams related. Anyway, if you do open it you'll see quite clearly the "#if ENABLE(STREAMS_API)" switch, so summing up, I changed as I don't have any strong opinion about it. > > Source/WebCore/Modules/streams/CountQueuingStrategy.h:36 > > +#include <wtf/RefPtr.h> > > include is probably not needed... ;) Probably not :) Changed. > > Source/WebCore/Modules/streams/CountQueuingStrategy.h:44 > > + explicit CountQueuingStrategy() { } > > Constructor might not be needed. Might not :) Changed. > > Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters:4461 > > + <ClCompile Include="..\bindings\js\JSStreamsCountQueuingStrategyCustom.cpp"> > > Filename to be changed? Done. > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:619 > > + 148B4FFA1B69045400C954E4 /* StreamsJSUtils.h in Headers */ = {isa = PBXBuildFile; fileRef = 148B4FF91B69045400C954E4 /* StreamsJSUtils.h */; }; > > This should be removed. > > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:7756 > > + 148B4FF91B69045400C954E4 /* StreamsJSUtils.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = StreamsJSUtils.h; path = bindings/js/StreamsJSUtils.h; sourceTree = "<group>"; }; > > This should be removed. > > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:15078 > > + 148B4FF91B69045400C954E4 /* StreamsJSUtils.h */, > > This should be removed. > > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:26020 > > + 148B4FFA1B69045400C954E4 /* StreamsJSUtils.h in Headers */, > > This should be removed. Yep, very good catch. Done all of them.
Comment on attachment 258169 [details] Patch LGTM, modulo the "allocation" discussion where additional points of view would be good to have. View in context: https://bugs.webkit.org/attachment.cgi?id=258169&action=review > Source/WebCore/Modules/streams/CountQueuingStrategy.h:39 > +// CountQueuingStrategy implements a stratagy for the streams that counts the chunks one by one according to the spec. s/stratagy/strategy/
Comment on attachment 258169 [details] Patch (In reply to comment #38) > LGTM, modulo the "allocation" discussion where additional points of view > would be good to have. Yes, I think we could think of it after landing this if Geoffrey or anybody else gives the r+. I am setting the r? though I know build is going to fail because it has a dependency with bug 146593, but we do need the review. > View in context: > https://bugs.webkit.org/attachment.cgi?id=258169&action=review > > > Source/WebCore/Modules/streams/CountQueuingStrategy.h:39 > > +// CountQueuingStrategy implements a stratagy for the streams that counts the chunks one by one according to the spec. > > s/stratagy/strategy/ Yep, will fix when landing.
So I guess only the official r+ (or suggesting changes) is pending.
Comment on attachment 258169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258169&action=review r=me Please fix the items below before landing, and double-check EWS >>> Source/WebCore/Modules/streams/CountQueuingStrategy.h:39 >>> +// CountQueuingStrategy implements a stratagy for the streams that counts the chunks one by one according to the spec. >> >> s/stratagy/strategy/ > > Yep, will fix when landing. s/the streams/streams/, s/the chunks/chunks/ (English idiom) > Source/WebCore/Modules/streams/CountQueuingStrategy.h:43 > + virtual ~CountQueuingStrategy() { } This seems wrong. Since we have no virtual functions and no subclasses, our destructor need not be virtual. Unless you're planning to add subclasses? Please remove the virtual keyword before landing. > Source/WebCore/bindings/js/JSCountQueuingStrategyCustom.cpp:46 > +EncodedJSValue JSC_HOST_CALL constructJSCountQueuingStrategy(ExecState* state) s/state/exec/ (WebKit idiom) > Source/WebCore/bindings/js/JSCountQueuingStrategyCustom.cpp:53 > + Ref<CountQueuingStrategy> countQueuingStrategy = adoptRef(*new CountQueuingStrategy()); > + JSValue jsCountQueuingStrategy = CREATE_DOM_WRAPPER(jsCast<DOMConstructorObject*>(state->callee())->globalObject(), CountQueuingStrategy, countQueuingStrategy.ptr()); It's a little strange to allocate a C++ object that contains no data. But I guess that's the easiest way to match what our DOM implementation expects. > Source/WebCore/bindings/js/JSDOMBinding.h:636 > +inline JSC::JSValue getPropertyFromObject(JSC::ExecState& state, JSC::JSObject& object, const char* identifier) s/state/exec/ > Source/WebCore/bindings/js/JSDOMBinding.h:641 > +inline void setPropertyToObject(JSC::ExecState& state, JSC::JSObject& targetObject, const char* name, JSC::JSValue value) s/targetObject/object/ (To match your naming above) > Source/WebCore/bindings/js/JSDOMBinding.h:644 > + JSC::PutPropertySlot propertySlot(&targetObject); > + targetObject.put(&targetObject, &state, JSC::Identifier::fromString(&state, name), value, propertySlot); Since you're calling a static function, I think it's clearer to write JSObject::put(targetObject, state, ...) s/state/exec/
Created attachment 258480 [details] Patch for landing
(In reply to comment #41) > Please fix the items below before landing, and double-check EWS EWS ongoing right now. > > Source/WebCore/Modules/streams/CountQueuingStrategy.h:43 > > + virtual ~CountQueuingStrategy() { } > > This seems wrong. Since we have no virtual functions and no subclasses, our > destructor need not be virtual. Unless you're planning to add subclasses? > > Please remove the virtual keyword before landing. It is curious, but it I remove the keyword, clang does not compile under the pretext that the class has to be polymorphic. As it does not seem too important. I left it as it is and change it if there is any other solution. The rest of requests were accomplished.
Created attachment 258481 [details] Patch for landing
Comment on attachment 258481 [details] Patch for landing Attachment 258481 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/26966 New failing tests: js/dom/global-constructors-attributes.html
Created attachment 258484 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 258485 [details] Patch for landing Fixed expectations for global constructors test.
Comment on attachment 258485 [details] Patch for landing Attachment 258485 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/27089 New failing tests: js/dom/global-constructors-attributes.html
Created attachment 258489 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 258485 [details] Patch for landing Attachment 258485 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/27091 New failing tests: js/dom/global-constructors-attributes.html
Created attachment 258490 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 258491 [details] Patch for landing Fixed expectations for global constructors test.
Comment on attachment 258491 [details] Patch for landing Attachment 258491 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/27286 New failing tests: js/dom/global-constructors-attributes.html
Created attachment 258493 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 258491 [details] Patch for landing Attachment 258491 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/27292 New failing tests: js/dom/global-constructors-attributes.html
Created attachment 258494 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 258497 [details] Patch for landing Fixed expectations for global constructors test.
Created attachment 258498 [details] Patch for landing Fixed expectations for global constructors test.
Comment on attachment 258498 [details] Patch for landing Clearing flags on attachment: 258498 Committed r188127: <http://trac.webkit.org/changeset/188127>
All reviewed patches have been landed. Closing bug.