Use Web IDL Union types in Service Worker's Cache interface

Use Web IDL Union types in Service Worker's Cache interface

No more overloads! The method/methodImpl pattern could be further
collapsed, but this seems like a good start while we work out kinks.

The 'RequestInfo' typedef is from the fetch spec[1], also used in the
ServiceWorker spec[2] to improve readability.

[1] https://fetch.spec.whatwg.org/
[2] https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html

BUG=240176
R=bashi@chromium.org,jkarlin@chromium.org

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

[jkarlin, 2014-11-06 16:59:45]

So much nicer! lgtm with nit

https://codereview.chromium.org/700323003/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/CacheTest.cpp (right):

https://codereview.chromium.org/700323003/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/CacheTest.cpp:270: RequestInfo toRequestInfo(const
String& value)
Rather than override how about stringToRequestInfo?

https://codereview.chromium.org/700323003/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/CacheTest.cpp:277: RequestInfo
toRequestInfo(Request* value)
Rather than override how about requestToRequestInfo?

[jsbell, 2014-11-06 17:00:06]

bashi - failure:

e:\b\build\slave\win_layout\build\src\out\debug\gen\blink\bindings\modules\v8\uniontypesmodules.cpp(67)
:error C2220: warning treated as error - no 'object' file generated
e:\b\build\slave\win_layout\build\src\out\debug\gen\blink\bindings\modules\v8\uniontypesmodules.cpp(67)
: warning C4702: unreachable code

The bindings code generated this in toImpl:

    {
        TONATIVE_VOID_EXCEPTIONSTATE(V8StringResource<>, cppValue,
toUSVString(v8Value, exceptionState), exceptionState);
        impl.setUSVString(cppValue);
        return;
    }

    exceptionState.throwTypeError("Not a valid union member.");

Note: no if test on the block containing the return.

[jsbell, 2014-11-06 17:03:26]

https://codereview.chromium.org/700323003/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/CacheTest.cpp (right):

https://codereview.chromium.org/700323003/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/CacheTest.cpp:270: RequestInfo toRequestInfo(const
String& value)
On 2014/11/06 16:59:44, jkarlin wrote:
> Rather than override how about stringToRequestInfo?

Hrm... Blink code seems to be split about evenly. (The bindings code makes heavy
use of toXXXX overloads.)

But sure.

[jsbell, 2014-11-06 17:39:34]

On 2014/11/06 17:00:06, jsbell wrote:
> Note: no if test on the block containing the return.

Patch set #6 has a fix. Let's see if the try bots agree.

[jsbell, 2014-11-06 22:38:31]

jsbell@chromium.org changed reviewers:
+ haraken@chromium.org

[jsbell, 2014-11-06 22:38:32]

Okay, looks green.

No rush to get this in, but bashi@ and haraken@ - can you take a look?

Do you want me to split out the bindings/ changes into a separate CL?

[bashi, 2014-11-06 23:26:03]

On 2014/11/06 22:38:32, jsbell wrote:
> Okay, looks green.
> 
> No rush to get this in, but bashi@ and haraken@ - can you take a look?
> 
> Do you want me to split out the bindings/ changes into a separate CL?

non-owner bindings/ LGTM. Thank you for fixing a bug in the bindings code!

I think it's ok to include the fix for the bug in this CL.

[haraken, 2014-11-07 00:06:36]

LGTM for bindings/.

https://codereview.chromium.org/700323003/diff/100001/Source/bindings/templat...
File Source/bindings/templates/union.cpp (right):

https://codereview.chromium.org/700323003/diff/100001/Source/bindings/templat...
Source/bindings/templates/union.cpp:136: exceptionState.throwTypeError("Not a
valid union member.");

I guess we unconditionally need exceptionState.throwTypeError, but don't we?

[bashi, 2014-11-07 00:24:07]

https://codereview.chromium.org/700323003/diff/100001/Source/bindings/templat...
File Source/bindings/templates/union.cpp (right):

https://codereview.chromium.org/700323003/diff/100001/Source/bindings/templat...
Source/bindings/templates/union.cpp:136: exceptionState.throwTypeError("Not a
valid union member.");
On 2014/11/07 00:06:36, haraken wrote:
> 
> I guess we unconditionally need exceptionState.throwTypeError, but don't we?

When I implement this, I thought we need to make this unconditional. But as the
compiler complained, we don't need this statement when we generate step16-18.

[haraken, 2014-11-07 01:15:33]

https://codereview.chromium.org/700323003/diff/100001/Source/bindings/templat...
File Source/bindings/templates/union.cpp (right):

https://codereview.chromium.org/700323003/diff/100001/Source/bindings/templat...
Source/bindings/templates/union.cpp:136: exceptionState.throwTypeError("Not a
valid union member.");
On 2014/11/07 00:24:07, bashi1 wrote:
> On 2014/11/07 00:06:36, haraken wrote:
> > 
> > I guess we unconditionally need exceptionState.throwTypeError, but don't we?
> 
> When I implement this, I thought we need to make this unconditional. But as
the
> compiler complained, we don't need this statement when we generate step16-18.

Makes sense, thanks!

[jsbell, 2014-11-07 16:58:03]

Thanks everyone!

FYI, I'm going to wait until after the M40 branch to land this.

[jsbell, 2014-11-10 21:51:11]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-11-10 21:52:18]

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

[commit-bot: I haz the power, 2014-11-10 22:59:13]

Committed patchset #7 (id:120001) as 185073