Hmm, I'm not sure if we really need to distinguish null and "not present" here.

After step 28:
- if RequestInit#signal is present, `signal` is set to the init's signal member. This includes that RequestInit#signal is null.
- if RequestInit#signal is not present, `signal` is null (per step 5)

My understanding is that `signal` will be null if init's signal is either unspecified or null.

I should have quoted step 6 to make if-else branch clearer.

> - if RequestInit#signal is not present, `signal` is null (per step 5)
IIUC, it's not correct if the first argument |input| is a Request object.
Step 7 updates `signal` to |input|'s signal in this case.

Even we go step 7, the same can be said after step 28 I guess? Regardless of input's signal step 28 checks only RequestInit's signal.

Probably I'm missing something.

Ah, I got it.  We need to take step 7-1 and 29 into consideration.

5. Let `signal` be null.
7. Otherwise (input is a Request object):
 7-1. Set `request` to input’s request.
 7-2. Set `signal` to input’s signal.
28. If `init`’s signal member is present, then set `signal` to it.
29. Let `r` be a new Request object associated with `request`.
30. If `signal` is not null, then make `r`’s signal follow `signal`.

so at after step 28 in the case `Request(input, init)`,
the temporary variable `signal` can be different, as I wrote in #2.
But whichever it is, for `r`, which is being constructed, its signal will be same with |input|'s signal at step 30.
Because `r` was initialized as `request` which is actually same with `input`.

> Even we go step 7, the same can be said after step 28 I guess? Regardless of input's signal step 28 checks only RequestInit's signal.

It is true, that after step 7 |signal| will always equal the input request's (|input|) signal. Here's what is happening:

1.) Then in step 28, we basically say: If |init|'s |signal| member is either a developer-provided null or a developer-provided AbortSignal object, set |signal| to it. After this, |signal| is either:

   a.) ~the input request's signal~
   b.) null
   c.) the developer-provided signal (RequestInit's signal)

Let's focus on b.) and c.), since that's where we're running into issues.

2.) Step 29 creates a new request whose signal is a completely fresh AbortSignal, in other words, one that _does not_ follow the input request's signal.

3.) Step 30 basically says "Only make our new request's signal follow the older input request's signal if |signal| (which is null or a signal if we assume b.) or c.)) is not null.

This lets developers write:

innerReq = new Request("", {signal: <someAbortedSignal>})
// innerReq's #signal member is an AbortSignal representing an _aborted_ signal.

request = new Request(innerReq, {signal: null});
// request's #signal is not "aborted", and doesn't follow innerReq's #signal because we use null.

So the issue is we cannot detect null vs undefined in these situations.

This issue is beyond the scope of RequestInit as well; for a more concrete example see [1]. Its implementation has a setDoubleOrNullMemberToNull [2] which always ensures that the corresponding `has` getter returns false, which is an issue because we cannot detect null values in any of these cases.

[1]: https://cs.chromium.org/chromium/src/third_party/blink/renderer/bindings/tests/idls/core/test_dictionary.idl?l=12

[2]: https://cs.chromium.org/chromium/src/third_party/blink/renderer/bindings/tests/results/core/test_dictionary.h?type=cs&q=doubleOrNullMember&sq=package:chromium&g=0&l=571

Re: #6

Yeah, peria@ and I chatted offline and discussed { signal: null } case. If we need to override innerReq.signal, we would need to distinguish null and "not present". I'm still not sure there are use cases to reset signal by passing { signal: null }.

Thank you for the detailed example and explanation!
I finally understand why you can't take them together.
|input|'s signal is different from its following one.

FWIW it seems that [1] is testing that a signal is removed by provided {signal:null}. So it seems that:

 - {signal: null} can be used to explicitly not follow the signal of an input request
 - {signal: <differentSignal>} can be used to explicitly follow a different signal than that of the input request.

Oops, sorry - forgot to provide link.

[1]: https://github.com/web-platform-tests/wpt/blob/master/fetch/api/abort/general.any.js#L149

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/98ce8e837f622b611575a5c0e738698856a81535

commit 98ce8e837f622b611575a5c0e738698856a81535
Author: Hitoshi Yoshida <peria@chromium.org>
Date: Thu Jun 28 02:28:31 2018

IDL compiler: Make null and undefined distinguishable partially

This CL makes null and undefined distinguishable for dictionary
members whose type is a nullable interface.

It changes Blink-side expectation about dict->hasFoo(), if
a dictionary member `foo` is in the case. Before this CL,
dict->hasFoo() means dict->foo() does not return null pointers,
but after this CL, dict->foo() can returns nullptr.


Bug:  855968 
Change-Id: I4dfddf0ae79703a69294aa627cff86f5e7c59252
Reviewed-on: https://chromium-review.googlesource.com/1113176
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Hitoshi Yoshida <peria@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571009}
[modify] https://crrev.com/98ce8e837f622b611575a5c0e738698856a81535/third_party/blink/renderer/bindings/scripts/v8_dictionary.py
[modify] https://crrev.com/98ce8e837f622b611575a5c0e738698856a81535/third_party/blink/renderer/bindings/scripts/v8_types.py
[modify] https://crrev.com/98ce8e837f622b611575a5c0e738698856a81535/third_party/blink/renderer/bindings/templates/dictionary_impl.cpp.tmpl
[modify] https://crrev.com/98ce8e837f622b611575a5c0e738698856a81535/third_party/blink/renderer/bindings/templates/dictionary_impl.h.tmpl
[modify] https://crrev.com/98ce8e837f622b611575a5c0e738698856a81535/third_party/blink/renderer/bindings/templates/dictionary_impl_common.cpp.tmpl
[modify] https://crrev.com/98ce8e837f622b611575a5c0e738698856a81535/third_party/blink/renderer/bindings/tests/results/core/test_dictionary.cc
[modify] https://crrev.com/98ce8e837f622b611575a5c0e738698856a81535/third_party/blink/renderer/bindings/tests/results/core/test_dictionary.h
[modify] https://crrev.com/98ce8e837f622b611575a5c0e738698856a81535/third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d.cc

In order to not introduce memory or performance regression, we decided to work only for nullable interfaces in dictionaries, which is the only known case, at the moment.

I'll reopen this issue if other cases are found.

I attempted using the CL resulting in the closing of this issue to fix the problem I was having but I still couldn't get it working. Is it possible the fix in the CL didn't solve the issue? I see that in [1], we're setting the `has_...` member to false. I believe that in this situation we'd need to set the `has_...` to true so that we can tell that we were given a null member, is this correct?

Right now it seems that the CL added another variable but maintains the same behavior as before, thus the member function representing a nullable dictionary interface member can never return nullptr, when I think it should? Let me know if I'm missing something.

[1]: https://cs.chromium.org/chromium/src/third_party/blink/renderer/bindings/tests/results/core/test_dictionary.h?q=core/test_dict&sq=package:chromium&g=0&l=560

Ah, I got it.
I assumed that setFooToNull() was called only from hand-written code, and hence set has_foo_ to 'false' not to make behavior difference minimum.
(If it is true, we can use setFoo(null) to provide a null entry.)

I'll check other callsites' behaviors and update it.

> I assumed that setFooToNull() was called only from hand-written code

Ah, that makes sense; yeah it is generated code that call is unfortunately. Thanks for taking a look!

> (If it is true, we can use setFoo(null) to provide a null entry.)

This probably would work; I think Member<NullableInterface>() will always produce a nullptr member, but I'd verify to be sure it wouldn't break any existing things.

Oops, my explanation was wrong, although your understanding seems correct.

I actually assume setFooToNull() is called from generated code, and if in case it is called from hand written code, it means to reset the element to be "not provided". (I'm not sure if it is allowed in spec, though.)

> I think Member<NullableInterface>() will always produce a nullptr member, but I'd verify to be sure it wouldn't break any existing things.
Yes, Member<T>() provides a nullptr.

I don't think setFooToNull should:

1. Ever be called by non-generated code. I don't believe we have a way to "unset" a dictionary member to null. More likely you'll need to copy the dictionary to another, ommitting some members if you want them to behave as if "unset"
2. Make it seem as if the member was "unset". It seems more clear to set it to null, since that is sort of what the function name indicates perhaps?

But yes, regardless, it looks like the `has_` in setFooToNull() should be set to true so we can distinguish NULL from unset etc :)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9ef3909f4552e73bc4931b9b6ceb8cf55c4e0484

commit 9ef3909f4552e73bc4931b9b6ceb8cf55c4e0484
Author: Hitoshi Yoshida <peria@chromium.org>
Date: Tue Jul 03 16:05:30 2018

Web Locks: Make 'signal' member non nullable in LockOptions

This CL makes 'signal' member in LockOption non nullable
to be spec conformant. [1]
It also drops a test that is inconsistent with this change.

[1] https://inexorabletash.github.io/web-locks/#idl-index


Bug:  855968 
Change-Id: I4faf6582451347a05eeca34f02e25a43217d86cd
Reviewed-on: https://chromium-review.googlesource.com/1121953
Commit-Queue: Joshua Bell <jsbell@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572244}
[modify] https://crrev.com/9ef3909f4552e73bc4931b9b6ceb8cf55c4e0484/third_party/WebKit/LayoutTests/http/tests/locks/signal.html
[modify] https://crrev.com/9ef3909f4552e73bc4931b9b6ceb8cf55c4e0484/third_party/blink/renderer/modules/locks/lock_options.idl

As stated in https://bugs.chromium.org/p/chromium/issues/detail?id=860116#c4, it appears this is not quite fixed yet (just re-stating here for clarity!).

I see. I wasn't aware that
  dict.member = null;
in JS calls DictImpl::setMemberToNull() in bindings.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/18d8323d387ae4d87adacb459c5c74e08c728099

commit 18d8323d387ae4d87adacb459c5c74e08c728099
Author: Hitoshi Yoshida <peria@chromium.org>
Date: Mon Jul 09 16:52:20 2018

WebLocks: Update internal order of LockManager::request

This CL changes the order to check flags in LockManager::request
to be conformant with its spec[1], and adds comments to show
which step each part works for.

This is a follow-up CL for https://crrev.com/572244


[1]: https://inexorabletash.github.io/web-locks/#api-lock-manager

Bug:  855968 
Change-Id: Ie47013eb77df91fe6ad1b42858f17cc4777f3638
Reviewed-on: https://chromium-review.googlesource.com/1126745
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Commit-Queue: Hitoshi Yoshida <peria@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573334}
[modify] https://crrev.com/18d8323d387ae4d87adacb459c5c74e08c728099/third_party/blink/renderer/modules/locks/lock_manager.cc

Sorry, "Fixed" the wrong issue in haste. Re-opening!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e2fcd4b0fce6f48b5761c95b9f0a2fb09c45d2e8

commit e2fcd4b0fce6f48b5761c95b9f0a2fb09c45d2e8
Author: Hitoshi Yoshida <peria@chromium.org>
Date: Wed Jul 11 05:14:35 2018

IDL compiler: Make null value provided if |null| is assigned in JS

Before this CL, setting |null| as a member of dictionary had the
same meaning with unsetting the member.
It is consistent with the intention of "present" state, and we don't
require a way to unset members. (We can create a dictionary from
scratch and set necessary members, to achieve such a state.)

After this CL, setting |null| makes the member present and be |null|.


Bug:  855968 
Change-Id: Ia1cbb5cede890cc00834205ca618543c043b9fbb
Reviewed-on: https://chromium-review.googlesource.com/1128696
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Hitoshi Yoshida <peria@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574083}
[modify] https://crrev.com/e2fcd4b0fce6f48b5761c95b9f0a2fb09c45d2e8/third_party/blink/renderer/bindings/templates/dictionary_impl_common.cpp.tmpl
[modify] https://crrev.com/e2fcd4b0fce6f48b5761c95b9f0a2fb09c45d2e8/third_party/blink/renderer/bindings/tests/results/core/test_dictionary.h

Now it should be fixed (for nullable interface).

Hmm, it seems that conversion from V8 object to impl works, but the opposite conversion does not work well.

I’ll look into this as well, but for now could you elaborate? If we accept null as an nullable interface value, when we retrieve it from JS do we get an instance of the object and not null or something?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/53e02676d011468414439a78d0334389eee5309b

commit 53e02676d011468414439a78d0334389eee5309b
Author: Hitoshi Yoshida <peria@chromium.org>
Date: Tue Jul 17 04:14:16 2018

bindings: Update idl-dictionary-unittest

Updates idl-dictionary-unittest to test the change http://crrev.com/574083
Because we can't test IDL dictionary types directly in JS,
we use an interface DictionaryTest as a intermediate path.
This CL also updates the behavior of DictionaryTest to be
consistent with InternalDictionary.


Bug:  855968 
Change-Id: I0ad4530a0143185bbf859a0e0b5371e5e0aefa26
Reviewed-on: https://chromium-review.googlesource.com/1136331
Commit-Queue: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Dominic Farolino <domfarolino@gmail.com>
Cr-Commit-Position: refs/heads/master@{#575548}
[modify] https://crrev.com/53e02676d011468414439a78d0334389eee5309b/third_party/WebKit/LayoutTests/bindings/idl-dictionary-unittest-expected.txt
[modify] https://crrev.com/53e02676d011468414439a78d0334389eee5309b/third_party/WebKit/LayoutTests/bindings/idl-dictionary-unittest.html
[modify] https://crrev.com/53e02676d011468414439a78d0334389eee5309b/third_party/blink/renderer/bindings/scripts/v8_dictionary.py
[modify] https://crrev.com/53e02676d011468414439a78d0334389eee5309b/third_party/blink/renderer/bindings/templates/dictionary_v8.cpp.tmpl
[modify] https://crrev.com/53e02676d011468414439a78d0334389eee5309b/third_party/blink/renderer/bindings/tests/results/core/v8_test_dictionary.cc
[modify] https://crrev.com/53e02676d011468414439a78d0334389eee5309b/third_party/blink/renderer/core/testing/dictionary_test.cc
[modify] https://crrev.com/53e02676d011468414439a78d0334389eee5309b/third_party/blink/renderer/core/testing/dictionary_test.h

Follow-up of #27;
The error came from the implementation of an intermediate tool "TestDictionary" interface, not from Dictionary implementation.

The test was updated in #29.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b19e29b0949511740e3e9359595012e90f4b930c

commit b19e29b0949511740e3e9359595012e90f4b930c
Author: Dominic Farolino <domfarolino@gmail.com>
Date: Wed Jul 18 21:04:03 2018

Use nullable interface for RequestInit AbortSignal

R=kinuko@chromium.org, peria@chromium.org, ricea@chromium.org, yhirano@chromium.org, yoav@yoav.ws

Bug:  855968 
Change-Id: I96be3b769dd75300553f639ca5fa8158771bde2c
Reviewed-on: https://chromium-review.googlesource.com/1137930
Commit-Queue: Dominic Farolino <domfarolino@gmail.com>
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Yoav Weiss <yoav@yoav.ws>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576204}
[modify] https://crrev.com/b19e29b0949511740e3e9359595012e90f4b930c/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js
[modify] https://crrev.com/b19e29b0949511740e3e9359595012e90f4b930c/third_party/blink/renderer/core/fetch/request.cc
[modify] https://crrev.com/b19e29b0949511740e3e9359595012e90f4b930c/third_party/blink/renderer/core/fetch/request_init.idl