Bug 111724

Created attachment 192006 [details] Patch

Comment on attachment 192006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192006&action=review ok > Source/WebCore/ChangeLog:30 > + * bindings/v8/custom/V8LocationCustom.cpp: can you add a comment why you only had to change these custom bindings?

Comment on attachment 192006 [details] Patch Attachment 192006 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17086329 New failing tests: inspector/extensions/extensions-eval-content-script.html inspector/extensions/extensions-audits-content-script.html

Comment on attachment 192006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192006&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:792 > +static v8::Handle<v8::Value> ${funcName}AttrGetterCallback(v8::Local<v8::String> name, const v8::AccessorInfo& info) > +{ > + if (worldType(info.GetIsolate()) == MainWorld) > + return ${interfaceName}V8Internal::${funcName}AttrGetterInMainWorld(name, info); > + return ${interfaceName}V8Internal::${funcName}AttrGetterInNonMainWorld(name, info); > +} I don't understand why we need this special case for domain safe functions. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:798 > +sub GenerateDomainSafeFunctionGetterHelper "Helper" is not a very meaningful name for this function. Is there a better name we can use?

Comment on attachment 192006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192006&action=review I might want to see a couple of more iterations:) > Source/WebCore/ChangeLog:13 > + No new tests (no changes in functionality yet). Please update run-bindings-tests. >> Source/WebCore/ChangeLog:30 >> + * bindings/v8/custom/V8LocationCustom.cpp: > > can you add a comment why you only had to change these custom bindings? The change looks reasonable, although I don't understand why these custom getters are needed in the first place. I tried to kill all of them, but couldn't due to test failures... (https://bugs.webkit.org/show_bug.cgi?id=109789) > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:784 > + GenerateDomainSafeFunctionGetterHelper($function, $interfaceName, "InMainWorld", "MainWorld"); > + GenerateDomainSafeFunctionGetterHelper($function, $interfaceName, "InNonMainWorld", "NonMainWorld"); Let's make naming consistent: "ForMainWorld" or "InMainWorld". You mix both in this patch. It looks like we prefer "ForMainWorld" in V8 bindings. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:-819 > -static v8::Handle<v8::Value> ${funcName}AttrGetterCallback(v8::Local<v8::String> name, const v8::AccessorInfo& info) > -{ > - return ${interfaceName}V8Internal::${funcName}AttrGetter(name, info); > -} > - You shouldn't remove this. We're currently implementing wrapper methods for all the entry points to DOM custom/non-custom getters/setters/callbacks/constructors. The wrapper methods are named xxxCallback(). The purpose is to enable us to hook all DOM operations (e.g. logging, profiling etc). Thus, here you can generate: static v8::Handle<v8::Value> ${funcName}AttrGetterForMainWorldCallback(v8::Local<v8::String> name, const v8::AccessorInfo& info) { return ${interfaceName}V8Internal::${funcName}AttrGetterForMainWorld(name, info); } static v8::Handle<v8::Value> ${funcName}AttrGetterCallback(v8::Local<v8::String> name, const v8::AccessorInfo& info) { return ${interfaceName}V8Internal::${funcName}AttrGetter(name, info); }

.. I need to split up this patch, too, there are too many fixes to make this work. Known needed fixes: - "can access frame" check in (V8)InjectedScriptManager - killing the static functiontemplate parameters inside generated functions (and V8LocationCustom) - something has to be done to HasInstance (either check all worlds, or pass a world type) With these fixes, the 2 failing tests, inspector/extensions/extensions-eval-content-script.html and inspector/extensions/extensions-audits-content-script.html will pass.

(In reply to comment #6) > Known needed fixes: > - "can access frame" check in (V8)InjectedScriptManager > - killing the static functiontemplate parameters inside generated functions (and V8LocationCustom) > - something has to be done to HasInstance (either check all worlds, or pass a world type) > > With these fixes, the 2 failing tests, > inspector/extensions/extensions-eval-content-script.html > and > inspector/extensions/extensions-audits-content-script.html > will pass. Sounds good.

Created attachment 192704 [details] Patch

Comment on attachment 192006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192006&action=review >> Source/WebCore/ChangeLog:13 >> + No new tests (no changes in functionality yet). > > Please update run-bindings-tests. Done. >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:784 >> + GenerateDomainSafeFunctionGetterHelper($function, $interfaceName, "InNonMainWorld", "NonMainWorld"); > > Let's make naming consistent: "ForMainWorld" or "InMainWorld". You mix both in this patch. It looks like we prefer "ForMainWorld" in V8 bindings. Fixed. >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:792 >> +} > > I don't understand why we need this special case for domain safe functions. I simplified this, so we don't generate 2 functions (for main and non-main), but only one. That's not really needed in this step, it was just unnecessarily complication. >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:798 >> +sub GenerateDomainSafeFunctionGetterHelper > > "Helper" is not a very meaningful name for this function. Is there a better name we can use? This was removed. >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:-819 >> - > > You shouldn't remove this. We're currently implementing wrapper methods for all the entry points to DOM custom/non-custom getters/setters/callbacks/constructors. The wrapper methods are named xxxCallback(). The purpose is to enable us to hook all DOM operations (e.g. logging, profiling etc). > > Thus, here you can generate: > > static v8::Handle<v8::Value> ${funcName}AttrGetterForMainWorldCallback(v8::Local<v8::String> name, const v8::AccessorInfo& info) > { > return ${interfaceName}V8Internal::${funcName}AttrGetterForMainWorld(name, info); > } > > static v8::Handle<v8::Value> ${funcName}AttrGetterCallback(v8::Local<v8::String> name, const v8::AccessorInfo& info) > { > return ${interfaceName}V8Internal::${funcName}AttrGetter(name, info); > } I wasn't removing it, it was moved above. Anyway, this part of the code is simpler now.

Created attachment 192737 [details] Patch

Comment on attachment 192737 [details] Patch r=me

Created attachment 192747 [details] Patch

Comment on attachment 192747 [details] Patch Clearing flags on attachment: 192747 Committed r145554: <http://trac.webkit.org/changeset/145554>

All reviewed patches have been landed. Closing bug.

I believe this broke several ExtensionBrowserTests: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=@ToT - chromium.org&testType=browser_tests&tests=ExtensionBrowserTest.RSSMultiRelLink,ExecuteScriptApiTest.ExecuteScriptFrameAfterLoad,ExtensionBrowserTest.PageAction,ExecuteScriptApiTest.ExecuteScriptCallback

Re-opened since this is blocked by bug 112182

Created attachment 192905 [details] Patch

(In reply to comment #17) > Created an attachment (id=192905) [details] > Patch What's the difference from the previous patch? I mean, what is the cause of the ExtensionBrowserTests' failures?

The difference is that that in this version hasInstance() checks from all worlds, and doesn't try to do things like - try to determine the world type from the context - only check templates from the given world type This will "fix" the extension tests but only because it won't uncover some problems that the previous patch uncovered. But I don't think we want to do that.

The change looks reasonable, but how about fixing bug 112250 first? Then you don't need to introduce GetFromContext, do you?

Created attachment 193101 [details] Patch

The latest patch - undoes the ugly hasInstance hack and does the check the right way - does not introduce GetFromContext world type; call sites need to pass the world type explicitly - fixes the rest of the failing chromium tests by checking hasInstance for all worlds in WebArrayBuffer(View?) Now that the patch in bug 112262 has landed, all the previously failing chromium browser_tests now pass.

Comment on attachment 193101 [details] Patch rs=me

Comment on attachment 193101 [details] Patch Clearing flags on attachment: 193101 Committed r145802: <http://trac.webkit.org/changeset/145802>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 112444

Created attachment 193310 [details] Patch

Comment on attachment 193310 [details] Patch New try, since the landed version of this broke Chrome. In the new try, we add HasInstanceInAnyWorld, and use that in WebBindings.cpp, and in the places where the previous patch had this multi-world HasInstance checking.

Comment on attachment 193310 [details] Patch rs=me

Comment on attachment 193310 [details] Patch Clearing flags on attachment: 193310 Committed r145906: <http://trac.webkit.org/changeset/145906>

All reviewed patches have been landed. Closing bug.