|
Description
B. Green
2009-09-22 13:32:35 PDT
Created attachment 39940 [details]
Proposed implementation of window.Navigator.registerProtocolHandler
Comment on attachment 39940 [details]
Proposed implementation of window.Navigator.registerProtocolHandler
Why does Chrome::runJavaScriptRegisterProtocolHandler return a boolean?
This code won't even compile. Navigator::registerProtocolHandler calls a non-existent method named "runJavascripRegisterProtocolHandler".
NAVIGATOR_ABILITIES is an incredibly vague name for a define and doesn't sound at all related to the function in question. It's not clear to me why this is #if'd at all given that the default behavior is to do nothing.
We also prefer that a full name is used in ChangeLog entries.
Created attachment 39979 [details] Proposed implementation of window.Navigator.registerProtocolHandler In reply to Mike Rowes' review: >Why does Chrome::runJavaScriptRegisterProtocolHandler return a boolean? It does not need to. I had thought this would be a proper place for the UA to return information to webkit, however it is better removed. >This code won't even compile. Fixed. Attempted migration from another enlistment by typing, and then sanity built without the flag defined. My apologies. >NAVIGATOR_ABILITIES is an incredibly vague name for a define and >doesn't sound at all related to the function in question Fixed by renaming the define to ENABLE_REGISTER_PROTOCOL_HANDLER. The original name was from a grouping used in the HTML5 spec for this and related APIs. >It's not clear to me why this is #if'd at all given that the default behavior is to do nothing. The API simply passes information to the UA. This is done by adding the runJavaScriptRegisterProtocolHandler pure virtual function to the ChromeClient interface defined in WebCore/page/ChromeClient.h. If it is not defined in the codebase of the UA using webkit, compilation will break. If it is dynamically linked, then a runtime failure will occur. As with the introduction of Workers, perhaps it is better to leave it behind the ENABLE_ flag for a while? RegisterProtocolHandler capitalization is wrong in the ChangeLog. (In reply to comment #3) > >It's not clear to me why this is #if'd at all given that the default behavior is to do nothing. > The API simply passes information to the UA. This is done by adding the > runJavaScriptRegisterProtocolHandler pure virtual function to the ChromeClient > interface defined in WebCore/page/ChromeClient.h. If it is not defined in the > codebase of the UA using webkit, compilation will break. If it is dynamically > linked, then a runtime failure will occur. As with the introduction of > Workers, perhaps it is better to leave it behind the ENABLE_ flag for a while? In the past I believe minor things like this have been added by simply adding stubs to the ports whose implementations are available in the WebKit tree. I think that would be fine for this, I see no need to add an ENABLE_ flag. OTOH, I would very much like to see registerContentHandler supported here too. Why add one and not the other? Created attachment 40158 [details]
Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler
@Eric Siedel
Capitalization is corrected in the change log.
@Peter Kasting
I have removed the #ifdef guards and provided default no-op implementation in ChromeClient.h.
I have added registerContentHandler as per your request. It is simply a mirror of this API, so it is correct to add it to this patch.
Comment on attachment 40158 [details]
Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler
Why are these results platform specific:
LayoutTests/platform/mac-leopard/fast/dom/navigator-detached-no-crash-expected.txt
how many other platforms do we need to update?
This looks fine to me. The question which is not answered by the ChangeLog is what spec covers this. the bug doesn't explain either. A link to the relevant section of the spec would be helpful.
We're also not testing this to make sure that it works. It seems that we could add this functionality to DumpRenderTree. Although I guess then we'd just be testing a mock.
Could we at least have a manual test?
Is this supposed to be completely left up to the client? Do we allow file:// urls here, should we? What about data urls? I'm not sure there is any reason we should disallow either.
r- due to lack of spec information and the fact that this will cause several platforms to fail due to platform-specific results.
(In reply to comment #7) > This looks fine to me. The question which is not answered by the ChangeLog is > what spec covers this. the bug doesn't explain either. A link to the relevant > section of the spec would be helpful. Doesn't comment 0 cover this? Comment on attachment 40158 [details]
Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler
Odd. I swear the bug page was empty before. My bad.
Either way, the documentation should ideally end up in the ChangLog as well, since that's what folks use during review and when doing an svn blame.
Thanks for the clarification, I will update the change log and tests soon. Created attachment 41753 [details]
Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler.
The following are responses to the previous review and other offline comments, #1. Please include the specification for the APIs in the ChangeLog. Done, links to the HTML5 spec for both APIs have been added to the ChangeLog. #2. Please include the reasoning why the behavior of these APIs is not implemented in webkit beyond passing parameters to the UA. Done, this has been added to the ChangeLog. #3. runJavascriptXXX is used for displaying a modal dialog to the user, please rename. Done, the APIs in Chrome and ChromeClient have been changed to registerXXX from runJavascriptRegsterXXX. #4. By unescaping the URL in the registerProtocolHandler call passed to ChromClient you may be losing information. I have removed the call to unescape the URL. The display text and ascii protocol/content name are still unescaped. #5. Explain why you have platform specific tests. Done. This is completed in in the LayoutTests change log. Created attachment 41755 [details]
Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler.
Created attachment 41756 [details]
Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler.
Previously uploaded wrong file (created prior to check-webkit-style).
Comment on attachment 41756 [details] Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler. This is getting really good, we are close. A few things: > Index: WebCore/ChangeLog > + notifies teh UA that a page has made the call. The UA is then /teh/the/ > Index: WebCore/loader/EmptyClients.h > + virtual void registerProtocolHandler(Frame*, const String&, const String&, const String&) { } > + virtual void runJavaScriptRegisterContentHandler(Frame*, const String&, const String&, const String&) { } the second one still has 'runJavaScript'prefix... > Index: WebCore/page/Chrome.cpp > +void Chrome::registerProtocolHandler(Frame* frame, const String& scheme, const String& url, const String& title) > +{ > + // Defer loads in case the client method runs a new event loop that would > + // otherwise cause the load to continue while we're in the middle of executing JavaScript. > + PageGroupLoadDeferrer deferrer(m_page, true); > + > + ASSERT(frame); > + m_client->registerProtocolHandler(frame, frame->displayStringModifiedByEncoding(scheme), url, frame->displayStringModifiedByEncoding(title)); I don't think we should be decoding scheme. title is fine to be decoded though. Also, lets remove Frame* parameter from corresponding method on ChromeClient unless we have a reason to have it. Same about PageGroupLoadDeferrer - it is needed in case we want to run modal dialog, which is unlikely in response to registerProtocolHandler(). It is going to be something like infobar or some other non-modal UI, in which case there is no modal loop. > +void Chrome::registerContentHandler(Frame* frame, const String& mimeType, const String& url, const String& title) Notes above apply here too. > Index: WebCore/page/Navigator.cpp > +void Navigator::registerProtocolHandler(const String& scheme, const String& url, const String& title) > +{ > + if (!m_frame) > + return; > + > + Page* page = m_frame->page(); > + if (!page) > + return; > + > + page->chrome()->registerProtocolHandler(m_frame, scheme, url, title); Before calling chrome, we need to do checks prescribed by spec and throw JS exceptions (SYNTAX_ERR and SECURITY_ERR). The IDL for these methods should specify 'raises(DOMException)' and then there will be a parameter to return exception code. Also, it's a good place to grab base URL which has to be passed to UA so it could be resolved later when '$s' will be replaced with actual email. > +void Navigator::registerContentHandler(const String& mimeType, const String& url, const String& title) Same here. Also, with checks and exceptions added, there is a good opportunity to write a test that verifies those work. Looking forward to updated patch! Comment on attachment 41756 [details]
Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler.
r- due to checks and test to be added.
Created attachment 41992 [details]
Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler.
Created attachment 41993 [details]
Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler.
Comment on attachment 41993 [details]
Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler.
Updated all issues and added tests.
Comment on attachment 41993 [details] Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler. Ha, looks really really close. A few small things: > Index: WebCore/ChangeLog > + We do however follow the specification in insuring that the reserved > + schemes and mimeTypes are not passed to the UA as custmo handler /custmo/custom/ > + registration tests. We also insure tht the "%s" token is present as /tht/that/ > Index: WebCore/page/Navigator.cpp > + Page* page = m_frame->page(); > + > + if (!page) > + return; > + > + page->chrome()->registerProtocolHandler(scheme, baseURL, url, m_frame->displayStringModifiedByEncoding(title)); usually in WebKit it looks like: if (Page* page = m_frame->page()) page->.... a bit shorter this way. Same for registerContentHandler. > Index: WebCore/page/Navigator.h > - class Navigator : public NavigatorBase, public RefCounted<Navigator> { > - public: > - static PassRefPtr<Navigator> create(Frame* frame) { return adoptRef(new Navigator(frame)); } It is better to avoid style changes (like indentation of the whole class declaration) with actual changes. It makes it harder later to see what exact change was applied. It's better to leave indent as is if there are other changes. > Index: WebCore/page/Navigator.idl > + > + void registerProtocolHandler(in DOMString scheme, in DOMString url, in DOMString title) > + raises(EventException); > + void registerContentHandler(in DOMString mimeType, in DOMString url, in DOMString title) > + raises(EventException); Those should be DOMException. > Index: LayoutTests/ChangeLog > + We add two fast/js tests to insure that the proper exceptions are fast/js has tests for js engine itself. We probably should have these tests in fast/dom > Index: LayoutTests/fast/js/registerContentHandler.html > +<!doctype html> > +<html> > +<head> > +</head> 'doctype' and 'head' are not adding anything here. Could be removed. > +<body> > +<p>This test makes sure that navigator.registerContentHandler throws the proper exceptions and has no-op default implementation.</p> > +<pre id="console"></pre> > +<script> > + if (window.layoutTestController) { The content of <script> tag (whole script) is usually not indented. Also there seems to be tabs in the test files. > + layoutTestController.setUseDashboardCompatibilityMode(true); Why do we need this? > + layoutTestController.dumpAsText(); > + layoutTestController.waitUntilDone(); No need to do waitUntilDone() and notifyDone() in this test, since the inline script will be executed synchronously before snapshot. > + if (succeeded == true) { if(succeeded) > +</html> > \ No newline at end of file Need newline. > Index: LayoutTests/fast/js/registerProtocolHandler.html Same as for the previous test. r- but I feel next iteration will be fine. Should we add ENABLE guards to this? Created attachment 42194 [details]
Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler.
Created attachment 42196 [details]
Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler.
Created attachment 42198 [details]
Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler.
Comment on attachment 42198 [details]
Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler.
Updated all issues.
Created attachment 42246 [details]
Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler.
Created attachment 42270 [details]
Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler.
Comment on attachment 42270 [details] Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler. > Index: WebCore/page/Navigator.cpp > Index: LayoutTests/fast/dom/navigator-detached-no-crash.html > + } catch(err) { > + // navigator.registerXXX will throw on invalid input. > + strings.push("navigator."+p+"() threw err "+err); I'd change comment to explicitly indicate that exception is an expected result here. > Index: LayoutTests/fast/dom/registerContentHandler.html > + var succeeded = false; > + try { > + window.navigator.registerContentHandler(protocol, "invalid protocol %s", "title"); > + } catch(e) { > + if('SECURITY_ERR' == e.name) { > + succeeded = true; > + }; > + }; > + > + if(succeeded == true) { > + debug('Pass: Invalid protocol "' + protocol + '" threw SECURITY_ERR exception.'); > + } else { > + debug('Fail: Invalid protocol "' + protocol + '" allowed.'); > + }; I think by removal of a 'succeeded' the test could be smaller and more readable. > +if(succeeded == true) { > + debug('Pass: Valid call succeeded.'); > +} else { > + debug('Fail: Invalid call did not succeed.'); typo, should be "Fail: Valid call did not succeed." Overall looks good. r=me. I'll make the changes above on landing. |