Refer to relevant realm instead of relevant global object #347

twiss · 2023-06-20T17:28:40Z

For created objects, this's relevant realm is of interest, rather than its relevant global object.

Also, link out to WebIDL's this definition instead of referencing it explicitly.

Also, note that objects are created in the relevant realm of this by default, rather than mentioning it everywhere.

And finally, refer to the "create an ArrayBuffer" algorithm of WebIDL, rather than constructing ArrayBuffer objects ourselves.

Resolves #346.

Preview | Diff

annevk

"this" is defined in Web IDL.

annevk · 2023-06-20T17:46:58Z

spec/Overview.html

@@ -2062,9 +2061,6 @@ <h4>The deriveBits method</h4>
              <li>
                <p>
                  Let |result| be a new {{ArrayBuffer}}


@bathos suggested referencing create here and elsewhere, right?

Yeah, I opted against it as it made the resulting text a bit clunky. And, the fetch and XHR specs do essentially the same thing as this text, for example.

Yes, afaict “a new ArrayBuffer” isn’t actually defined at this level, right? Something needs to hit AllocateArrayBuffer somewhere to actually create a new ArrayBuffer object in the sense that new ArrayBuffer() in ES code would, and Web IDL’s create an ArrayBuffer alg takes care of that.

OK, fair enough. I've updated the text 👍

I didn’t see the first reply about fetch & XHR here till now. The XHR one is definitely wrong because it links “new” to create a new object implementing the interface, and ArrayBuffer is not an interface. The fetch one doesn’t link it to anything and seems like it’s also a spec hiccup (though again, I’m not an editor/expert so YMMV w/ my analysis).

I think for both of those specifications the text predates the IDL algorithm. Please file issues/PRs?

@annevk issues opened for Fetch and XHR.

Also, link out to WebIDL's `this` definition instead of referencing it explicitly. And finally, note that objects are created in the relevant realm of `this` by default, rather than mentioning it everywhere.

twiss · 2023-06-20T19:43:42Z

"this" is defined in Web IDL.

Er, right. I got misled by all of the instances of "this [[HTML]]" in the text, even after removing all of them :) But I've updated the commit message, thanks for catching that!

bathos · 2023-06-21T02:03:34Z

spec/Overview.html

@@ -1328,8 +1328,7 @@ <h3>Data Types</h3>
          <h3>Methods and Parameters</h3>
          <p>
            Unless otherwise stated, objects created by the methods defined in this section shall be associated with the
-            [= relevant global object =]
-            of `this` [[HTML]].
+            [= relevant realm =] of [= this =].


This is interesting — is an “ambient” realm argument like this typical @annevk?

Not really, though there is an issue against IDL to make realm available as ambient information in a way that matches this as folks get it wrong all the time.

bathos · 2023-06-21T02:12:45Z

spec/Overview.html

-                  associated with the
-                  [= relevant global object =]
-                  of `this` [[HTML]], and
+                  Let |result| be the result of [= ArrayBuffer/create | creating =] an {{ArrayBuffer}}
                  containing the result of performing the derive bits operation


I would have imagined the “containing” part to be written as “with” as the byte sequence is an argument for the create algorithm (along with the realm argument, though iiuc that’s being treated as an implicit here). However I’m not an expert on this and couldn’t say for sure whether this phrasing already fits expectations.

This specific algorithm is worded using "from", and that's the phrasing WebIDL itself seems to use in https://webidl.spec.whatwg.org/#arraybufferview-create, for example. I thought the existing "containing" sounded clearer, but I can change it if needed.

bathos · 2023-06-21T08:02:04Z

spec/Overview.html

-                            Let |result| be a new empty {{ArrayBuffer}} associated with the
-                            [= relevant global object =]
-                            of `this` [[HTML]].
+                            Let |result| be a new empty {{ArrayBuffer}}.


This one seems wrong because this creates a non-growable ArrayBuffer whose length is 0, but the steps which follow “append bytes” to it. Presumably the intent is to append to result.[[ArrayBufferData]], but even so, that isn’t possible — that Data Block’s size is fixed.

I’m not 100% sure this would be the right way to go but my best guess is that this step should really be creating a new empty byte sequence for the accumulation steps. The ArrayBuffer would be created afterwards, the byte sequence now being the data argument for “create”.

(This algorithm seems like an outlier fortunately — I didn’t spot others written with an ArrayBuffer initialization happening before its intended length is actually known.)

Right, I also forgot this one in the search-and-replace, thanks for catching that!

The second step was actually already there (it was creating an ArrayBuffer from this ArrayBuffer below this text), so I've just changed this to creating a byte sequence, as you suggested.

annevk

This seems okay, but I think it's better to be explicit when creating objects. However, given that it's not completely settled in IDL that seems like a reasonable concern to overlook.

SHA: f42610e Reason: push, by twiss Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

twiss force-pushed the update-relevant-realm-handling branch from 79eca43 to 3aeca4e Compare June 20, 2023 17:32

annevk reviewed Jun 20, 2023

View reviewed changes

Refer to relevant realm instead of relevant global object

e0c3afc

Also, link out to WebIDL's `this` definition instead of referencing it explicitly. And finally, note that objects are created in the relevant realm of `this` by default, rather than mentioning it everywhere.

twiss force-pushed the update-relevant-realm-handling branch from 3aeca4e to e0c3afc Compare June 20, 2023 19:43

twiss mentioned this pull request Jun 20, 2023

Out-of-date(?) ArrayBuffer usage in algorithmic text #346

Closed

twiss requested review from bathos and annevk June 20, 2023 19:48

bathos reviewed Jun 21, 2023

View reviewed changes

This was referenced Jun 21, 2023

Wrong algorithm + missing realm argument when creating ArrayBuffer whatwg/xhr#376

Open

“Realmless” ArrayBuffer creation in Body’s arrayBuffer() steps whatwg/fetch#1675

Closed

Refer to the "create an ArrayBuffer" algorithm of WebIDL

46aefca

twiss force-pushed the update-relevant-realm-handling branch from c734af0 to 46aefca Compare June 21, 2023 09:16

annevk mentioned this pull request Jun 30, 2023

JSON conversion #349

Open

annevk approved these changes Jun 30, 2023

View reviewed changes

twiss merged commit f42610e into main Aug 18, 2023
2 checks passed

twiss deleted the update-relevant-realm-handling branch August 18, 2023 07:28

github-actions bot added a commit that referenced this pull request Aug 18, 2023

Merge pull request #347 from w3c/update-relevant-realm-handling

824aa10

SHA: f42610e Reason: push, by twiss Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refer to relevant realm instead of relevant global object #347

Refer to relevant realm instead of relevant global object #347

twiss commented Jun 20, 2023 •

edited by pr-preview bot

Loading

annevk left a comment

annevk Jun 20, 2023

twiss Jun 20, 2023 •

edited

Loading

bathos Jun 20, 2023

twiss Jun 20, 2023

bathos Jun 21, 2023

annevk Jun 21, 2023

bathos Jun 21, 2023

twiss commented Jun 20, 2023

bathos Jun 21, 2023

annevk Jun 21, 2023

bathos Jun 21, 2023

twiss Jun 21, 2023

bathos Jun 21, 2023 •

edited

Loading

twiss Jun 21, 2023

annevk left a comment

Refer to relevant realm instead of relevant global object #347

Refer to relevant realm instead of relevant global object #347

Conversation

twiss commented Jun 20, 2023 • edited by pr-preview bot Loading

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twiss Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twiss commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bathos Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

twiss commented Jun 20, 2023 •

edited by pr-preview bot

Loading

twiss Jun 20, 2023 •

edited

Loading

bathos Jun 21, 2023 •

edited

Loading