Bug 99129

Created attachment 168362 [details] Patch

Haraken, could you take a look?

Comment on attachment 168362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168362&action=review The overall approach looks good. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2066 > +WrapperTypeInfo V8${implClassName}Constructor::info = { V8${implClassName}Constructor::GetTemplate, V8${implClassName}::derefObject, V8${implClassName}::toActiveDOMObject, 0, V8${implClassName}::installPerContextPrototypeProperties, 0, WrapperTypeObjectPrototype }; Can't you pass 0 instead of passing an empty function? Then you can avoid generating empty functions.

Hi Haraken, thanks for reviewing! > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2066 > > +WrapperTypeInfo V8${implClassName}Constructor::info = { V8${implClassName}Constructor::GetTemplate, V8${implClassName}::derefObject, V8${implClassName}::toActiveDOMObject, 0, V8${implClassName}::installPerContextPrototypeProperties, 0, WrapperTypeObjectPrototype }; > > Can't you pass 0 instead of passing an empty function? Then you can avoid generating empty functions. I cannot. In precise, I can do so for this patch. But I have another patch which make it harder, unfortunately.

Comment on attachment 168362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168362&action=review This patch will affect the performance of toV8Slow(). Before landing, please confirm that this patch (+ other patches you're trying to land) won't regress performance of Bindings/create-element.html and Dromaeo DOM tests. > Source/WebCore/ChangeLog:16 > + generated method, which is desined to be called for each time when the prototype Nit: desined => designed. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:845 > + if ($implClassName eq "DOMWindow") { > + push(@implContentDecls, <<END); > + return perContextData->constructorForType(WrapperTypeInfo::unwrap(data), V8DOMWindow::toNative(info.Holder())->document()); > +END > + } elsif ($implClassName eq "WorkerContext") { > + push(@implContentDecls, <<END); > + return perContextData->constructorForType(WrapperTypeInfo::unwrap(data), V8WorkerContext::toNative(info.Holder())); > +END > + } else { > + die "Unknown Context ${implClassName}" > + } > + push(@implContentDecls, <<END); > +} Nit: I would prefer push(@implContentDecls, "...") rather than here documents for one-line code. >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2066 >> +WrapperTypeInfo V8${implClassName}Constructor::info = { V8${implClassName}Constructor::GetTemplate, V8${implClassName}::derefObject, V8${implClassName}::toActiveDOMObject, 0, V8${implClassName}::installPerContextPrototypeProperties, 0, WrapperTypeObjectPrototype }; > > Can't you pass 0 instead of passing an empty function? Then you can avoid generating empty functions. I discussed with morrita-san offline. I understood why you want to pass an empty function.

Created attachment 168376 [details] Patch for landing

Hi Haraken, thanks for quick review! (In reply to comment #5) > (From update of attachment 168362 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=168362&action=review > > This patch will affect the performance of toV8Slow(). Before landing, please confirm that this patch (+ other patches you're trying to land) won't regress performance of Bindings/create-element.html and Dromaeo DOM tests. Done. I don't see any slowdown.

Comment on attachment 168376 [details] Patch for landing Rejecting attachment 168376 [details] from commit-queue. New failing tests: fast/notifications/notifications-with-permission.html Full output: http://queues.webkit.org/results/14266248

Created attachment 168384 [details] Patch for landing

Comment on attachment 168384 [details] Patch for landing Clearing flags on attachment: 168384 Committed r131167: <http://trac.webkit.org/changeset/131167>

All reviewed patches have been landed. Closing bug.

(In reply to comment #10) > (From update of attachment 168384 [details]) > Clearing flags on attachment: 168384 > > Committed r131167: <http://trac.webkit.org/changeset/131167> Bindings-tests failed on multiple bots after the patch: Unknown Context TestObj at WebCore/bindings/scripts/CodeGeneratorV8.pm line 839. gcc: error trying to exec 'cc1obj': execvp: No such file or directory

(In reply to comment #12) > gcc: error trying to exec 'cc1obj': execvp: No such file or directory I would guess you need to install gobjc.

(In reply to comment #13) > (In reply to comment #12) > > gcc: error trying to exec 'cc1obj': execvp: No such file or directory > > I would guess you need to install gobjc. Hm? What changed why we need one more dependency? I haven't seen a heads-up mail on webkit-dev about it.

Otherwise bindings tests fail on _all_ bots: Chromium, Mac, Qt, ... Could you update the result please?

(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #12) > > > gcc: error trying to exec 'cc1obj': execvp: No such file or directory > > > > I would guess you need to install gobjc. > > Hm? What changed why we need one more dependency? > I haven't seen a heads-up mail on webkit-dev about it. Nothing changed. I think the error is not related to this change. I've observed the error when I've not installed gobjc. On what bots is the error happening?

(In reply to comment #15) > Bindings-tests failed on multiple bots after the patch: > > Unknown Context TestObj at WebCore/bindings/scripts/CodeGeneratorV8.pm line 839. On the other hand, this is undoubtedly the error caused by this patch. Let me check it.

The "gcc: error trying to exec 'cc1obj': execvp: No such file or directory" error message isn't related to this patch, it is present long time ago. (at least on the Qt bot) But the "Unknown Context TestObj at WebCore/bindings/scripts/CodeGeneratorV8.pm line 839." error message introduced in r131167 on all platform.

Makes sense. > But the "Unknown Context TestObj at WebCore/bindings/scripts/CodeGeneratorV8.pm line 839." error message introduced in r131167 on all platform. This will be fixed in minutes. > The "gcc: error trying to exec 'cc1obj': execvp: No such file or directory" error message isn't related to this patch, it is present long time ago. (at least on the Qt bot) This will be fixed by installing gobjc to the bots.

(In reply to comment #19) > Makes sense. > > > But the "Unknown Context TestObj at WebCore/bindings/scripts/CodeGeneratorV8.pm line 839." error message introduced in r131167 on all platform. > > This will be fixed in minutes. Fixed in r131180. Sorry for the trouble.

Comment on attachment 168384 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=168384&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:840 > + } else { > + die "Unknown Context ${implClassName}" > + } morrita-san: In r131180, as a quick fix, I commented out this code to hide a run-bindings-tests failure for TestObj.idl (which contains methods with [V8EnabledPerContext]). Please fix the code in a more reasonable way.