Service Worker: Annotate IDLs with [TypeChecking] to turn it on

Service Worker: Annotate IDLs with [TypeChecking] to turn it on

We should be doing type checking everywhere, but for back-compat reasons
Blink currently doesn't do "interface" or "unrestricted" enforcement.
Turn it on for all the new SW interfaces (since there's no compat
issue) and to fix a cache where Cache.put()'s second argument went
unchecked.

R=haraken@chromium.org
BUG=426153
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184295

[jsbell, 2014-10-22 23:52:59]

haraken@ - PTAL?

The test case exercises the failure caught by asanka's tests in
https://codereview.chromium.org/425413002/ so may be redundant.

I turned [TypeChecking] on for all the Service Worker IDLs since it would
prevent future failures if we add methods that need checking. I also compared
the old/new generated binding code. These methods now have added checks in the
generated code:

Cache.match()
Cache.matchAll()
Cache.add()
Cache.put()
Cache.delete()
Cache.keys()
Header() (constructor)
Request() (constructor)
Response() (constructor)
ServiceWorkerGlobalScope.fetch()

... but all of these (except Cache.put()):

(1) just need to type check the first argument - others are simple types or
dictionaries

(2) are overloaded and are dispatching based on type checking the first argument

So the new type checks are redundant (except Cache.put()) but better to be safe
than crash if new methods are added.

[haraken, 2014-10-23 00:43:51]

I understand you need "TypeChecking=Interface", but why do you need
"TypeChecking=Unrestricted"?

LGTM to avoid overnight review.

[jsbell, 2014-10-23 16:11:38]

On 2014/10/23 00:43:51, haraken wrote:
> I understand you need "TypeChecking=Interface", but why do you need
> "TypeChecking=Unrestricted"?
> 
> LGTM to avoid overnight review.

It's not necessary, but if we added a method e.g. foo(double d) and forgot to
add [TypeChecking=Unrestricted] we'd be generating code that wasn't spec
compliant.

ISTM we should require (via code review) using it on all new IDL interfaces
added to Blink.

[haraken, 2014-10-23 16:15:19]

On 2014/10/23 16:11:38, jsbell wrote:
> On 2014/10/23 00:43:51, haraken wrote:
> > I understand you need "TypeChecking=Interface", but why do you need
> > "TypeChecking=Unrestricted"?
> > 
> > LGTM to avoid overnight review.
> 
> It's not necessary, but if we added a method e.g. foo(double d) and forgot to
> add [TypeChecking=Unrestricted] we'd be generating code that wasn't spec
> compliant.
> 
> ISTM we should require (via code review) using it on all new IDL interfaces
> added to Blink.

Is restricted double a legacy behavior?

[jsbell, 2014-10-23 16:55:46]

On 2014/10/23 16:15:19, haraken wrote:
> Is restricted double a legacy behavior?

The opposite. Without [TypeChecking=Unrestricted] blink does not throw if
Infinity or NaN is passed to foo(double d);. With [TypeChecking=Unrestricted],
blink throws. If you want to not throw, an IDL should specify foo(unrestricted
double d);

Since there's uncertainty - and since we probably won't add methods taking
numbers here in the near future - I'll remove the 'Unrestricted'.

[jsbell, 2014-10-23 16:57:58]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-10-23 16:58:51]

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

[commit-bot: I haz the power, 2014-10-23 18:47:42]

Committed patchset #3 (id:40001) as 184295

[haraken, 2014-10-23 23:33:14]

On 2014/10/23 16:55:46, jsbell wrote:
> On 2014/10/23 16:15:19, haraken wrote:
> > Is restricted double a legacy behavior?
> 
> The opposite. Without [TypeChecking=Unrestricted] blink does not throw if
> Infinity or NaN is passed to foo(double d);. With [TypeChecking=Unrestricted],
> blink throws. If you want to not throw, an IDL should specify foo(unrestricted
> double d);
> 
> Since there's uncertainty - and since we probably won't add methods taking
> numbers here in the near future - I'll remove the 'Unrestricted'.

Makes sense, thanks for the clarification!