[webkit-reviews] review denied: [Bug 21918] A JavaScript Exception abstraction : [Attachment 24849] Platform independence with magic headers

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 21918: A JavaScript Exception abstraction
https://bugs.webkit.org/show_bug.cgi?id=21918

Attachment 24849: Platform independence with magic headers
https://bugs.webkit.org/attachment.cgi?id=24849&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
> --- /dev/null
> +++ b/WebCore/dom/ExceptionContext.cpp
> @@ -0,0 +1,39 @@
> +/*
> + * Copyright (C) 2008 Google Inc.
> + * Copyright (C) 2008 Apple Inc. All rights reserved.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License

> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + */
> +
> +#include "config.h"
> +#include "ExceptionContext.h"
> +
> +#include "JSDOMBinding.h"
> +#include "Node.h"
> +
> +namespace WebCore {
> +
> +ExceptionContext::ExceptionContext(Node* node)
> +    : m_data(execStateFromNode(node))
> +{
> +}
> +
> +bool ExceptionContext::hadException()
> +{
> +    return m_data->hadException();
> +}
> +
> +}  // namespace WebCore

I don't understand this source file. It's in the "dom" directory, but it has
JavaScriptCore-specific JavaScript code in it, calling execStateFromNode. How
does that work?

I think the name ExceptionContext is a little misleading. To be clear,
ExecState* is not an "exception" context for JavaScriptCore. It's more than
that. It's both the context needed to execute more JavaScript code, and also,
almost incidentally, a way to return the JavaScript exception to the caller. If
we only wanted to give a way to return the JavaScript exception, we could just
use an out parameter with a place to put the exception, but that wouldn't
suffice because JavaScript execution requires more context than that. The
comments in this new ExceptionContext class seem to be vague on this point, and
the class name too.

> +	   ExceptionContext(Node* node);

It seems pretty mysterious what this constructor would do. I know, but only
because I designed the old code, that this is going to figure out the correct
JavaScript execution context given a DOM node. But if the class really was just
an "exception context", then you wouldn't need this argument. I also don't
think that providing this as a constructor for an exception context is good
encapsulation; it's more about how the DOM uses JavaScript than it is about the
JavaScript machinery itself.

> +	   ExceptionContext(ExceptionContextData data) : m_data(data) {}

If ExecutionContext is going to be more than just a single
ExceptionContextData, then I would think that it's a hidden implementation
detail what ExceptionContextData is, and there shouldn't be a public
constructor that takes one of them. If we're really going to make it public
that ExceptionContextData is all that's inside an ExceptionContext, then I
don't understand why we need both the ExceptionContext and ExceptionContextData
types. Also if ExceptionContextData is any non-trivial object, I think we'd
want to pass const ExceptionContextData&.

> +	   ~ExceptionContext() {}

This definition of the constructor should be omitted. The compiler will do the
same thing if you don't declare a constructor at all.

> +	   ExceptionContextData data() const { return m_data; }
> +	   void setData(ExceptionContextData data) { m_data = data; }

Same comments apply here as in the constructor. I see no value added by the
ExceptionContext class if it's just a holder for a single ExceptionContextData.
Seems over-engineered. A typedef for ExecState* would accomplish the same
thing.

Unless there's future plan for putting more into ExceptionContext I suggest
just using typedef ExecState ExceptionContext; adding another class here isn't
adding additional value.

I'm going to say review- on this. I understand the need to make this code
JavaScript-engine-independent, but I think it can be done either more simply
than this, or in a more complete way.

A great patch would also make some progress on addressing the issue of
propagating exceptions from languages other than JavaScript, such as
Objective-C; but I guess I'm not asking for that since I don't really see how
to solve that problem.