Bug 149518

Created attachment 261864 [details] Patch

Attachment 261864 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSDOMWindowBase.cpp:91: Missing space after , [whitespace/comma] [3] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 261864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261864&action=review > Source/JavaScriptCore/builtins/BuiltinNames.h:115 > + m_privateToPublicMap.add(privateName.impl(), &publicName); Might be good to add an ASSERT above to ensure name collision does not happen.

(In reply to comment #3) > Might be good to add an ASSERT above to ensure name collision does not > happen. Yep, definitely needed. Actually I remember trying to figure out a crash because of a collision so it is very needed. The rest of the shallow review looks good. Only one comment you knew I was going to make, that is why don't you separate into the generation and the actual tee skeleton.

(In reply to comment #4) > (In reply to comment #3) > > Might be good to add an ASSERT above to ensure name collision does not > > happen. > > Yep, definitely needed. Actually I remember trying to figure out a crash > because of a collision so it is very needed. > > The rest of the shallow review looks good. Only one comment you knew I was > going to make, that is why don't you separate into the generation and the > actual tee skeleton. Thanks. As of tee skeleton, you are right, I could have split it. It feels a bit safer to validate everything works fine on all bots at the time the binding generator is updated.

Ping review, this is the last patch messing with JSC before being able to do pure JS Builtins DOM classes.

Comment on attachment 261864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261864&action=review > Source/JavaScriptCore/runtime/CommonIdentifiers.cpp:86 > +} > + > Extra blank line here. > Source/JavaScriptCore/runtime/CommonIdentifiers.h:362 > + JS_EXPORT_PRIVATE void appendExternalName(const Identifier& publicName, const Identifier& privateName); This API is a bit dangerous and difficult to use correctly. These maps hold pointers to Identifier objects. That means the Identifier itself has to survive, not just the reference counted object the Identifier holds. That might not be clear to the caller. Probably even needs a comment. > Source/WebCore/bindings/js/JSDOMWindowBase.h:88 > +#if ENABLE(STREAMS_API) > + ReadableStreamInternalsBuiltinFunctions m_readableStreamFunctions; > +#endif Does this really need to be done like this? It’s not good that we’re going to need to add one of these every time we add another class that uses built-ins. Can we minimize the number of separate places that we have to list ReadableStream explicitly? Instead we could simply have something that is for all built-in functions from all enabled WebCore features and have the #if be done as part of the table generation code instead of at each call site. I count 10 different places that each had to list this explicitly. That’s not good for the future. If JSDOMWindowBase and WebCoreJSClientData are going to need to include all of them, then what’s the value of generating each class’s list of names in a separate file? > Source/WebCore/generate-js-builtins:140 > +class ${Filename}BuiltinFunctions > +{ We’d normally put the brace on the line before.

Thanks for the review! (In reply to comment #7) > Comment on attachment 261864 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=261864&action=review > > > Source/JavaScriptCore/runtime/CommonIdentifiers.cpp:86 > > +} > > + > > > > Extra blank line here. OK > > > Source/JavaScriptCore/runtime/CommonIdentifiers.h:362 > > + JS_EXPORT_PRIVATE void appendExternalName(const Identifier& publicName, const Identifier& privateName); > > This API is a bit dangerous and difficult to use correctly. These maps hold > pointers to Identifier objects. That means the Identifier itself has to > survive, not just the reference counted object the Identifier holds. That > might not be clear to the caller. Probably even needs a comment. Agreed. I will add a comment. > > Source/WebCore/bindings/js/JSDOMWindowBase.h:88 > > +#if ENABLE(STREAMS_API) > > + ReadableStreamInternalsBuiltinFunctions m_readableStreamFunctions; > > +#endif > > Does this really need to be done like this? It’s not good that we’re going > to need to add one of these every time we add another class that uses > built-ins. Can we minimize the number of separate places that we have to > list ReadableStream explicitly? Instead we could simply have something that > is for all built-in functions from all enabled WebCore features and have the > #if be done as part of the table generation code instead of at each call > site. > I count 10 different places that each had to list this explicitly. That’s > not good for the future. > > If JSDOMWindowBase and WebCoreJSClientData are going to need to include all > of them, then what’s the value of generating each class’s list of names in a > separate file? There are two parameters that need to be handled: - whether a given js file should be #if ENABLE(...) - whether a given js file contains functions mapped to WebIDL (owned by JSXXPrototype) or internal functions (owned by the global object). The first one might be handled by the js generator by including this information as a comment in the js file. For the second, the same could be done. Or this could be done as part of the build system. Or it could be done by hand since this might happen less often. Let's do that after ReadableStream is shipped as a refactoring step. > > > Source/WebCore/generate-js-builtins:140 > > +class ${Filename}BuiltinFunctions > > +{ > > We’d normally put the brace on the line before. OK

Created attachment 262239 [details] Patch for landing

Comment on attachment 262239 [details] Patch for landing Rejecting attachment 262239 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 262239, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ore/generate-js-builtins patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/streams/reference-implementation/readable-stream-expected.txt patching file LayoutTests/streams/reference-implementation/readable-stream-tee-expected.txt patching file LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/231568

Created attachment 262240 [details] Rebasing for landing

Attachment 262240 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSDOMWindowBase.cpp:93: Missing space after , [whitespace/comma] [3] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 262246 [details] Patch

Comment on attachment 262246 [details] Patch Clearing flags on attachment: 262246 Committed r190401: <http://trac.webkit.org/changeset/190401>

All reviewed patches have been landed. Closing bug.

> I count 10 different places that each had to list this explicitly. That’s > not good for the future. > > If JSDOMWindowBase and WebCoreJSClientData are going to need to include all > of them, then what’s the value of generating each class’s list of names in a > separate file? Filed bug 149751 to keep track of it.