[Webkit-unassigned] [Bug 29651] [HTML5] Add a way for an application to register as a protocol handler.

Wed Oct 28 15:11:08 PDT 2009

https://bugs.webkit.org/show_bug.cgi?id=29651

Dmitry Titov <dimich at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #41993|review?                     |review-
               Flag|                            |

--- Comment #20 from Dmitry Titov <dimich at chromium.org>  2009-10-28 15:11:07 PDT ---
(From update of attachment 41993)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.