Add [TypeChecking=Interface] to XPath interfaces

Add [TypeChecking=Interface] to XPath interfaces

Also make all arguments non-optional, make some nullable, change the type
of XPathResult input arguments to "object?" and drop the custom bindings
for DocumentXPathEvaluator.evaluate() (which did nothing special at all).

All this matches Firefox's behavior, and the informal "specification" at
https://wiki.whatwg.org/wiki/DOM_XPath as well as, to some degree, the
older specification at http://www.w3.org/TR/DOM-Level-3-XPath.

The changed type of XPathResult input arguments means that any object can
be used for those arguments, but the value is not used at all in our
implementation anyway, so strictly checking that an XPathResult is passed
serves little purpose in practice.

BUG=462561
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191546

[Jens Widell, 2015-03-06 12:43:23]

jl@opera.com changed reviewers:
+ haraken@chromium.org, philipj@opera.com

[Jens Widell, 2015-03-06 12:43:24]

PTAL

This might be risky, I suppose.

Document.evalate() is used on 0.1 % of page loads, which isn't insignificant
usage, and omitting at least some of the previously optional arguments (all but
the first two, really) would still leave a perfectly meaningful call.

OTOH, such calls already throw exceptions in Firefox.

[haraken, 2015-03-06 13:03:13]

Can you probably get IE results? If IE also follows the behavior, we can be more
confident about this change.

[Jens Widell, 2015-03-06 13:04:22]

On 2015/03/06 13:03:13, haraken wrote:
> Can you probably get IE results? If IE also follows the behavior, we can be
more
> confident about this change.

IE doesn't support these interfaces at all, I think.

[haraken, 2015-03-06 13:08:04]

On 2015/03/06 13:04:22, Jens Widell wrote:
> On 2015/03/06 13:03:13, haraken wrote:
> > Can you probably get IE results? If IE also follows the behavior, we can be
> more
> > confident about this change.
> 
> IE doesn't support these interfaces at all, I think.

Then LGTM, but API owners should make a judgement.

[philipj_slow, 2015-03-06 15:53:13]

It's unfortunate that there's no proper spec to consult here, but if I think
that if we wrote a spec today some of these arguments would actually be
optional, in particular any trailing nullable arguments.

That would be a slightly more conservative change.

https://codereview.chromium.org/983123003/diff/1/Source/core/xml/XPathEvaluat...
File Source/core/xml/XPathEvaluator.idl (right):

https://codereview.chromium.org/983123003/diff/1/Source/core/xml/XPathEvaluat...
Source/core/xml/XPathEvaluator.idl:25: [RaisesException] XPathExpression
createExpression(DOMString expression, XPathNSResolver? resolver);
I think "optional XPathNSResolver? resolver = null"

https://codereview.chromium.org/983123003/diff/1/Source/core/xml/XPathEvaluat...
Source/core/xml/XPathEvaluator.idl:29: [RaisesException] XPathResult
evaluate(DOMString expression, Node contextNode, XPathNSResolver? resolver,
unsigned short type, object? inResult);
resolver and the following can be optional with default values I think. Not
contextNode, because that would currently throw in the C++ code.

https://codereview.chromium.org/983123003/diff/1/Source/core/xml/XPathResult.idl
File Source/core/xml/XPathResult.idl (right):

https://codereview.chromium.org/983123003/diff/1/Source/core/xml/XPathResult....
Source/core/xml/XPathResult.idl:46: [RaisesException] Node? iterateNext();
Both of these changes make good sense.

[Jens Widell, 2015-03-09 07:21:31]

On 2015/03/06 15:53:13, philipj_UTC7 wrote:
> It's unfortunate that there's no proper spec to consult here, but if I think
> that if we wrote a spec today some of these arguments would actually be
> optional, in particular any trailing nullable arguments.
> 
> That would be a slightly more conservative change.

I've changed the arguments that can meaningfully be omitted to be optional with
default values instead. PTAL.

[philipj_slow, 2015-03-09 08:17:31]

Thanks, optional arguments looks good and conservative. Some doubt about that
unused argument, but LGTM however you want to deal with that.

https://codereview.chromium.org/983123003/diff/40001/LayoutTests/fast/xpath/x...
File LayoutTests/fast/xpath/xpath-empty-string.html (right):

https://codereview.chromium.org/983123003/diff/40001/LayoutTests/fast/xpath/x...
LayoutTests/fast/xpath/xpath-empty-string.html:8:
document.evaluate("//a[@id='']", document, null, XPathResult.ANY_TYPE, null);
This test change shouldn't be needed any longer.

https://codereview.chromium.org/983123003/diff/40001/Source/core/xml/XPathExp...
File Source/core/xml/XPathExpression.idl (right):

https://codereview.chromium.org/983123003/diff/40001/Source/core/xml/XPathExp...
Source/core/xml/XPathExpression.idl:25: [RaisesException] XPathResult
evaluate(Node contextNode, optional unsigned short type = 0, optional object?
inResult = null);
I now see that inResult is not actually used, the previous code was odd indeed,
un-obviously ending up unused in XPathExpression::evaluate. If the only effect
it can have it to throw if type checking fails, and that was previously a silent
error, how about just removing the argument everywhere?

[Jens Widell, 2015-03-09 08:30:22]

https://codereview.chromium.org/983123003/diff/40001/Source/core/xml/XPathExp...
File Source/core/xml/XPathExpression.idl (right):

https://codereview.chromium.org/983123003/diff/40001/Source/core/xml/XPathExp...
Source/core/xml/XPathExpression.idl:25: [RaisesException] XPathResult
evaluate(Node contextNode, optional unsigned short type = 0, optional object?
inResult = null);
On 2015/03/09 08:17:30, philipj_UTC7 wrote:
> I now see that inResult is not actually used, the previous code was odd
indeed,
> un-obviously ending up unused in XPathExpression::evaluate. If the only effect
> it can have it to throw if type checking fails, and that was previously a
silent
> error, how about just removing the argument everywhere?

Firefox throws if the argument value is not an object (or null/undefined). I was
sort of aiming for the same, but we actually (incorrectly) treat 'object' and
'any' the same in the code generator, so there's effectively no difference to
not having the argument at all.

That we don't use it is an implementation detail. Using it was always optional
in the specification. I'm not sure it's an overall win to omit the argument in
the IDL, given that a hypothetical specification would probably have it.

[philipj_slow, 2015-03-09 10:10:47]

https://codereview.chromium.org/983123003/diff/40001/Source/core/xml/XPathExp...
File Source/core/xml/XPathExpression.idl (right):

https://codereview.chromium.org/983123003/diff/40001/Source/core/xml/XPathExp...
Source/core/xml/XPathExpression.idl:25: [RaisesException] XPathResult
evaluate(Node contextNode, optional unsigned short type = 0, optional object?
inResult = null);
On 2015/03/09 08:30:22, Jens Widell wrote:
> On 2015/03/09 08:17:30, philipj_UTC7 wrote:
> > I now see that inResult is not actually used, the previous code was odd
> indeed,
> > un-obviously ending up unused in XPathExpression::evaluate. If the only
effect
> > it can have it to throw if type checking fails, and that was previously a
> silent
> > error, how about just removing the argument everywhere?
> 
> Firefox throws if the argument value is not an object (or null/undefined). I
was
> sort of aiming for the same, but we actually (incorrectly) treat 'object' and
> 'any' the same in the code generator, so there's effectively no difference to
> not having the argument at all.
> 
> That we don't use it is an implementation detail. Using it was always optional
> in the specification. I'm not sure it's an overall win to omit the argument in
> the IDL, given that a hypothetical specification would probably have it.

OK, http://www.w3.org/TR/DOM-Level-3-XPath/xpath.html#XPathEvaluator-evaluate
says:

"The result specifies a specific result object which may be reused and returned
by this method. If this is specified as null or the implementation does not
reuse the specified result, a new result object will be constructed and
returned."

Very strange. If I were writing a new spec for this I'd certainly remove the
argument unless required for compat (unlikely) but OK to leave it alone until
there is a spec.

[Jens Widell, 2015-03-09 11:05:10]

https://codereview.chromium.org/983123003/diff/40001/LayoutTests/fast/xpath/x...
File LayoutTests/fast/xpath/xpath-empty-string.html (right):

https://codereview.chromium.org/983123003/diff/40001/LayoutTests/fast/xpath/x...
LayoutTests/fast/xpath/xpath-empty-string.html:8:
document.evaluate("//a[@id='']", document, null, XPathResult.ANY_TYPE, null);
On 2015/03/09 08:17:30, philipj_UTC7 wrote:
> This test change shouldn't be needed any longer.

True. I've reverted it.

https://codereview.chromium.org/983123003/diff/40001/Source/core/xml/XPathExp...
File Source/core/xml/XPathExpression.idl (right):

https://codereview.chromium.org/983123003/diff/40001/Source/core/xml/XPathExp...
Source/core/xml/XPathExpression.idl:25: [RaisesException] XPathResult
evaluate(Node contextNode, optional unsigned short type = 0, optional object?
inResult = null);
On 2015/03/09 10:10:47, philipj_UTC7 wrote:
> On 2015/03/09 08:30:22, Jens Widell wrote:
> > On 2015/03/09 08:17:30, philipj_UTC7 wrote:
> > > I now see that inResult is not actually used, the previous code was odd
> > indeed,
> > > un-obviously ending up unused in XPathExpression::evaluate. If the only
> effect
> > > it can have it to throw if type checking fails, and that was previously a
> > silent
> > > error, how about just removing the argument everywhere?
> > 
> > Firefox throws if the argument value is not an object (or null/undefined). I
> was
> > sort of aiming for the same, but we actually (incorrectly) treat 'object'
and
> > 'any' the same in the code generator, so there's effectively no difference
to
> > not having the argument at all.
> > 
> > That we don't use it is an implementation detail. Using it was always
optional
> > in the specification. I'm not sure it's an overall win to omit the argument
in
> > the IDL, given that a hypothetical specification would probably have it.
> 
> OK, http://www.w3.org/TR/DOM-Level-3-XPath/xpath.html#XPathEvaluator-evaluate
> says:
> 
> "The result specifies a specific result object which may be reused and
returned
> by this method. If this is specified as null or the implementation does not
> reuse the specified result, a new result object will be constructed and
> returned."
> 
> Very strange. If I were writing a new spec for this I'd certainly remove the
> argument unless required for compat (unlikely) but OK to leave it alone until
> there is a spec.

Presto actually reused the result object, if one was provided.

But sure, it would be reasonable to drop the argument if a modern specification
was written, and most likely safe from a compatibility point of view; it hardly
makes sense to write a script that *assumes* the result object is reused. And
either way, it wouldn't affect Blink's compatibility, since we already don't
reuse it.

[Jens Widell, 2015-03-09 11:05:34]

The CQ bit was checked by jl@opera.com

[Jens Widell, 2015-03-09 11:05:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, philipj@opera.com
Link to the patchset: https://codereview.chromium.org/983123003/#ps60001 (title:
"revert test change; no longer needed")

[commit-bot: I haz the power, 2015-03-09 11:05:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/983123003/60001

[philipj_slow, 2015-03-09 11:22:34]

FWIW, Gecko also seems to use the result object.

[commit-bot: I haz the power, 2015-03-09 13:50:34]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191546