| Summary: | [Streams API] Separate compile flag for ReadableStream and WritableStream | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||||||
| Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | achristensen, calvaris, commit-queue | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
youenn fablet
2016-08-22 06:12:35 PDT
Created attachment 286595 [details]
Patch
Created attachment 286596 [details]
Fixing option
Created attachment 286600 [details]
Trying to fix EFL/GTK builds
Created attachment 288116 [details]
Patch
Comment on attachment 288116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288116&action=review I made some comments that IMHO need to be changed. Besides them, I think somebody else should review the Apple build system code. > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:152 > +ENABLE_READABLESTREAM_API = ENABLE_READABLESTREAM_API; I think we should use READABLE_STREAM_API throughout the whole code. Same for WRITABLE_STREAM_API > Source/WebCore/CMakeLists.txt:3727 > + list(APPEND WebCore_DERIVED_BUILTIN_HEADERS ${DERIVED_SOURCES_WEBCORE_DIR}/${_objectName}Builtins.h) > endforeach () > > add_custom_command( > - OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltins.cpp > - ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltinInternals.cpp > + OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltinInternals.cpp > ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltins.h > ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltinInternals.h > MAIN_DEPENDENCY ${WebCore_BUILTINS_SOURCES} > - DEPENDS ${BUILTINS_GENERATOR_SCRIPTS} > + DEPENDS ${BUILTINS_GENERATOR_SCRIPTS} ${WebCore_DERIVED_BUILTIN_HEADERS} What does this have to do with the current objective of the patch? > Source/WebCore/Modules/streams/ByteLengthQueuingStrategy.idl:33 > + Conditional=READABLESTREAM_API, Apparently https://streams.spec.whatwg.org/#blqs-class this is also used in Writable Streams > Source/WebCore/Modules/streams/ByteLengthQueuingStrategy.js:27 > +// @conditional=ENABLE(READABLESTREAM_API) Ditto. > Source/WebCore/Modules/streams/CountQueuingStrategy.js:26 > +// @conditional=ENABLE(READABLESTREAM_API) Ditto > Source/WebCore/Modules/streams/ReadableStream.js:27 > +// @conditional=ENABLE(READABLESTREAM_API) Ditto > Tools/Scripts/webkitperl/FeatureList.pm:127 > + $readablestreamAPISupport, Let's use $readableStreamAPISupport everywhere. > Tools/Scripts/webkitperl/FeatureList.pm:159 > + $writablestreamAPISupport, Let's use $writableStreamAPISupport everywhere. > > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:152 > > +ENABLE_READABLESTREAM_API = ENABLE_READABLESTREAM_API; > > I think we should use READABLE_STREAM_API throughout the whole code. Same > for WRITABLE_STREAM_API OK > > Source/WebCore/CMakeLists.txt:3727 > > + list(APPEND WebCore_DERIVED_BUILTIN_HEADERS ${DERIVED_SOURCES_WEBCORE_DIR}/${_objectName}Builtins.h) > > endforeach () > > > > add_custom_command( > > - OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltins.cpp WebCoreJSBuiltins.cpp is only used for non cmake build systems. > > - ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltinInternals.cpp > > + OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltinInternals.cpp > > ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltins.h > > ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltinInternals.h > > MAIN_DEPENDENCY ${WebCore_BUILTINS_SOURCES} > > - DEPENDS ${BUILTINS_GENERATOR_SCRIPTS} > > + DEPENDS ${BUILTINS_GENERATOR_SCRIPTS} ${WebCore_DERIVED_BUILTIN_HEADERS} > > What does this have to do with the current objective of the patch? We ensure that if the header files are modified, we call the script. Otherwise, as in this case, the script is not called. That is the reason of the previous patch failures. > > Source/WebCore/Modules/streams/ByteLengthQueuingStrategy.idl:33 > > + Conditional=READABLESTREAM_API, > > Apparently https://streams.spec.whatwg.org/#blqs-class this is also used in > Writable Streams Yes, but we do not test them with writable streams... Well, let's be optimistic, writable stream tests are on their WPT way. > > Tools/Scripts/webkitperl/FeatureList.pm:127 > > + $readablestreamAPISupport, > > Let's use $readableStreamAPISupport everywhere. OK > > Tools/Scripts/webkitperl/FeatureList.pm:159 > > + $writablestreamAPISupport, > > Let's use $writableStreamAPISupport everywhere. OK Created attachment 288130 [details]
Renaming macros
(In reply to comment #6) > > > Source/WebCore/CMakeLists.txt:3727 > > > + list(APPEND WebCore_DERIVED_BUILTIN_HEADERS ${DERIVED_SOURCES_WEBCORE_DIR}/${_objectName}Builtins.h) > > > endforeach () > > > > > > add_custom_command( > > > - OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltins.cpp > > WebCoreJSBuiltins.cpp is only used for non cmake build systems. > > > > - ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltinInternals.cpp > > > + OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltinInternals.cpp > > > ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltins.h > > > ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltinInternals.h > > > MAIN_DEPENDENCY ${WebCore_BUILTINS_SOURCES} > > > - DEPENDS ${BUILTINS_GENERATOR_SCRIPTS} > > > + DEPENDS ${BUILTINS_GENERATOR_SCRIPTS} ${WebCore_DERIVED_BUILTIN_HEADERS} > > > > What does this have to do with the current objective of the patch? > > We ensure that if the header files are modified, we call the script. > Otherwise, as in this case, the script is not called. > That is the reason of the previous patch failures. Since this is a different problem, shouldn't this be done in a different patch? > Since this is a different problem, shouldn't this be done in a different
> patch?
But the proof it is working is the other changes.
We could land this patch without the build system changes, but this would require clean builds.
Given the small size of these specific changes, I prefer land it in one go if that is possible.
Comment on attachment 288130 [details] Renaming macros View in context: https://bugs.webkit.org/attachment.cgi?id=288130&action=review Besides this nuances that need to be fixed, I'd appreciate if somebody else could give the r+ after reviewing the Apple build specifics. > Source/WebCore/CMakeLists.txt:3727 > + list(APPEND WebCore_DERIVED_BUILTIN_HEADERS ${DERIVED_SOURCES_WEBCORE_DIR}/${_objectName}Builtins.h) > endforeach () > > add_custom_command( > - OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltins.cpp > - ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltinInternals.cpp > + OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltinInternals.cpp > ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltins.h > ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltinInternals.h > MAIN_DEPENDENCY ${WebCore_BUILTINS_SOURCES} > - DEPENDS ${BUILTINS_GENERATOR_SCRIPTS} > + DEPENDS ${BUILTINS_GENERATOR_SCRIPTS} ${WebCore_DERIVED_BUILTIN_HEADERS} Ok, then write it in the corresponding changelog, cause I can't even see the corresponding empty entry for this one, which leads me to think that they need update. > Ok, then write it in the corresponding changelog, cause I can't even see the
> corresponding empty entry for this one, which leads me to think that they
> need update.
Right, I added it for mac but forgot cmake...
Created attachment 288133 [details]
Updating change log
Comment on attachment 288133 [details] Updating change log View in context: https://bugs.webkit.org/attachment.cgi?id=288133&action=review > LayoutTests/ChangeLog:10 > +2016-09-07 Youenn Fablet <youenn@apple.com> This ChangeLog entry should be removed. Created attachment 288140 [details]
Patch for landing
Comment on attachment 288140 [details] Patch for landing Clearing flags on attachment: 288140 Committed r205549: <http://trac.webkit.org/changeset/205549> All reviewed patches have been landed. Closing bug. |