This tracks chrome bug: http://code.google.com/p/chromium/issues/detail?id=53610
Created attachment 67499 [details] add method to FrameLoaderClient
Comment on attachment 67499 [details] add method to FrameLoaderClient View in context: https://bugs.webkit.org/attachment.cgi?id=67499&action=prettypatch This looks fine to me. We can change how we grab the URL in a followup patch. We'll need fishd to give the R+ since this touches the WebKit API. > WebCore/bindings/v8/V8DOMWindowShell.cpp:343 > + if (m_frame->loader()->client()->allowScriptExtension(extensions[i]->name(), m_frame->loader()->activeDocumentLoader()->url(), extensionGroup)) Ugg... That's not the right way to grab the URL. I see that's what the old code does, but we should get it from the m_frame->document()->url().
+fishd for WebKit API review.
Comment on attachment 67499 [details] add method to FrameLoaderClient View in context: https://bugs.webkit.org/attachment.cgi?id=67499&action=prettypatch > WebKit/chromium/public/WebFrameClient.h:305 > + virtual bool allowScriptExtension(WebFrame*, const WebString& extension, const WebURL& url, int extensionGroup) { return false; } nit: extension -> extensionName? url -> documentURL? actually, is this parameter necessary given that one can just use WebFrame::url() to access the same value? > WebKit/chromium/public/WebKitClient.h:243 > + // given URL and script context group. nit: "script context group" -> "extension group" should there be some caller of this function?
Comment on attachment 67499 [details] add method to FrameLoaderClient View in context: https://bugs.webkit.org/attachment.cgi?id=67499&action=prettypatch > WebCore/bindings/v8/V8DOMWindowShell.cpp:343 > + if (m_frame->loader()->client()->allowScriptExtension(extensions[i]->name(), m_frame->loader()->activeDocumentLoader()->url(), extensionGroup)) See the note above: we used to use the document()->url(), but it didn't work due to window.open navigating to about:blank first.
(In reply to comment #5) > (From update of attachment 67499 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=67499&action=prettypatch > > > WebCore/bindings/v8/V8DOMWindowShell.cpp:343 > > + if (m_frame->loader()->client()->allowScriptExtension(extensions[i]->name(), m_frame->loader()->activeDocumentLoader()->url(), extensionGroup)) > See the note above: we used to use the document()->url(), but it didn't work due to window.open navigating to about:blank first. I suspect using the DocumentLoader isn't the right fix for that bug, but that's an issue for another patch.
Comment on attachment 67499 [details] add method to FrameLoaderClient View in context: https://bugs.webkit.org/attachment.cgi?id=67499&action=prettypatch I'm removing the URL parameter from the FrameLoaderClient interface, and moving the call to get the URL up into the WebKit/chromium layer. That's the highest layer where I still have access to the activeDocumentLoader, and if I can fix the bug in a different way and remove the need for the URL parameter entirely, there will be less interface to change. > WebKit/chromium/public/WebFrameClient.h:305 > + virtual bool allowScriptExtension(WebFrame*, const WebString& extension, const WebURL& url, int extensionGroup) { return false; } Name changes made. See my reply to Adam about why we need the URL parameter. > WebKit/chromium/public/WebKitClient.h:243 > + // given URL and script context group. Good catch - this was left over from an abandoned change. I'll reverted this file.
Created attachment 67608 [details] fixed with fishd's comments
Comment on attachment 67608 [details] fixed with fishd's comments View in context: https://bugs.webkit.org/attachment.cgi?id=67608&action=prettypatch > WebKit/chromium/src/FrameLoaderClientImpl.cpp:156 > + WebURL documentURL = m_webFrame->frame()->loader()->activeDocumentLoader()->url(); isn't this the same exact thing as: WebFrame* frame = ...; WebDataSource* ds = frame->provisionalDataSource(); if (!ds) ds = frame->dataSource(); WebURL url = ds->request().url(); In other words, you shouldn't need to pass documentURL here.
Created attachment 67702 [details] remove URL param Thanks for the tip! I removed the URL param from the interface. (FYI, the chromium side of this change is here: http://codereview.chromium.org/3398001/show )
Comment on attachment 67702 [details] remove URL param r=me
Created attachment 68138 [details] try #2, this time with a multi-stage patch My last patch broke some things and got reverted. I'm going to try to land it again, in multiple stages this time. The differences from the previous patch are: - This is an additive change only. I added a registerExtension2 API that will be unused until a corresponding Chromium change lands. - WebFrameClient defaults to allowing all script extensions. I think this is what broke the previous patch - test_shell and other test clients were probably preventing important v8 extensions from running.
Attachment 68138 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/bindings/v8/V8Proxy.cpp:856: Place brace on its own line for function definitions. [whitespace/braces] [4] WebCore/bindings/v8/V8Proxy.h:131: One space before end of line comments [whitespace/comments] [5] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 68139 [details] style fixes
Comment on attachment 68139 [details] style fixes View in context: https://bugs.webkit.org/attachment.cgi?id=68139&action=review > WebCore/bindings/v8/V8DOMWindowShell.cpp:344 > + // Ensure our extension is always allowed. nit: Ensure our date extension is always allowed. > WebKit/chromium/public/WebFrameClient.h:305 > + virtual bool allowScriptExtension(WebFrame*, const WebString& extensionName, int extensionGroup) { return true; } should the magic extensionGroup with value 0 be defined here? > WebKit/chromium/public/WebScriptController.h:62 > + WEBKIT_API static void registerExtension2(v8::Extension*); it may be easier to just call this registerExtension. overriding based on arguments is fine. that way you do not need to rename registerExtension2 back to registerExtension later.
Created attachment 68287 [details] fixed review comments
Comment on attachment 68139 [details] style fixes View in context: https://bugs.webkit.org/attachment.cgi?id=68139&action=review >> WebCore/bindings/v8/V8DOMWindowShell.cpp:344 >> + // Ensure our extension is always allowed. > > nit: Ensure our date extension is always allowed. Done. >> WebKit/chromium/public/WebFrameClient.h:305 >> + virtual bool allowScriptExtension(WebFrame*, const WebString& extensionName, int extensionGroup) { return true; } > > should the magic extensionGroup with value 0 be defined here? I added some comments. >> WebKit/chromium/public/WebScriptController.h:62 >> + WEBKIT_API static void registerExtension2(v8::Extension*); > > it may be easier to just call this registerExtension. overriding based on arguments is fine. > that way you do not need to rename registerExtension2 back to registerExtension later. Done. I was a bit wary to do this at first, since registerExtension already exists with the same signature. But I think the new version will have the same behavior as long as allowScriptExtension returns true.
Created attachment 68288 [details] fixed ChangeLog
Comment on attachment 68288 [details] fixed ChangeLog r=me
Comment on attachment 68288 [details] fixed ChangeLog Clearing flags on attachment: 68288 Committed r68061: <http://trac.webkit.org/changeset/68061>
All reviewed patches have been landed. Closing bug.