1292999 - Replace in-tree consumer of non-standard Iterator() with standard way in dom/

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Tooru Fujisawa [:arai]]

separated from bug 1290637.
see bug 1290637 for the details.

Required code changes are following:
  * Check each usage of non-standard Iterator [1] function,
    and replace it with some standard way,
    like Object.entries [2] or Object.values [3],
    iterable/iterator [4], or generator [5][6],
    or something appropriate for the specific usage

Here's the rough list for Iterator() usage (not exhaustive)
https://dxr.mozilla.org/mozilla-central/search?q=%22+Iterator(%22+path%3Adom+-ext%3Ah+-path%3AnsDocument.cpp&redirect=false

for example:
  for (let [k, v] in Iterator(obj)) {
    ...
  }
can be replaced to:
  for (let [k, v] of Object.entries(obj)) {
    ...
  }

ril_worker.js, and tests/test_ril_worker_icc_BerTlvHelper.js needs some refactoring:
the result of Iterator is consumed in erTlvHelper.searchForNextTag
https://dxr.mozilla.org/mozilla-central/rev/6b65dd49d4f045c0a9753ce60bdb4b7b4aaedcf8/dom/system/gonk/ril_worker.js#11057
>   searchForNextTag: function(tag, iter) {
>     for (let [index, tlv] in iter) {
>       if (tlv.tag === tag) {
>         return tlv;
>       }
>     }
>     return null;
>   }

searchForNextTag needs to be modified to work on something else, like ES6 iterable/iterator, or generator,
and callers should also be modified to follow the change in searchForNextTag.


test_camera_fake_parameters.html also need similar change.
https://dxr.mozilla.org/mozilla-central/rev/6b65dd49d4f045c0a9753ce60bdb4b7b4aaedcf8/dom/camera/test/test_camera_fake_parameters.html#472
>     var sizeGenerator = Iterator(expSizes);
>     return new Promise(function(resolve, reject) {
>       function nextSize() {
>         try {
>           var size = sizeGenerator.next()[1];


[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Iterator
[2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/entries
[3] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/values
[4] https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Iteration_protocols
[5] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Generator
[6] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function*

Comment 1

[Sumit Tiwari]

Attachment: Bug 1292999 - Replace Iterator() in dom/;

Review commit: https://reviewboard.mozilla.org/r/71084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71084/

Comment 2

[Sumit Tiwari]

So I have this patch ready, but for some reason I can't run all the tests. I'll look into it in detail tomorrow, but I just want someone to review my approach with a generator object.

Comment 3

[Tooru Fujisawa [:arai]]

Comment on attachment 8780358 [details]
Bug 1292999 - Replace Iterator() in dom/;

https://reviewboard.mozilla.org/r/71084/#review68588

Thank you for your patch :)

The generator approach would work with some minor fix, but looks like you could use builtin Array methods instead,
that would reduce the code size.

::: dom/camera/test/test_camera_fake_parameters.html:472
(Diff revision 1)
>          }
>        });
>        ok(found == 1, "found size " + size.toSource() + " in pictureSizes");
>      });
>  
> -    var sizeGenerator = Iterator(expSizes);
> +    var sizeGenerator = function*() {

`expSizes` is an array, so you can use `expSizes.values()` to get iterator that iterate over the array elements.
it would be nice to rename `sizeGenerator` variable to `sizeIterator` or something in that case.

::: dom/camera/test/test_camera_fake_parameters.html:473
(Diff revision 1)
>        });
>        ok(found == 1, "found size " + size.toSource() + " in pictureSizes");
>      });
>  
> -    var sizeGenerator = Iterator(expSizes);
> +    var sizeGenerator = function*() {
> +      for (let size in Object.values(expSizes)) {

`in` -> `of`

::: dom/camera/test/test_camera_fake_parameters.html:480
(Diff revision 1)
> +      }
> +    };
>      return new Promise(function(resolve, reject) {
>        function nextSize() {
>          try {
> -          var size = sizeGenerator.next()[1];
> +          var size = sizeGenerator().next();

If you use generator function, you need to call the generator function outside of this function, otherwise it's initialized each time, and yields the 1st element each time, forever.

Then, `Iterator` uses old protocol, that throws `StopIteration` when it finishes, but new iterator protocol (both generator function and array iterator) returns an object with `{value, done}` keys, to tell it's done or not.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols

so you need to assign those properties by
```
  var {value: size, done} = sizeIterator.next();
```
and return if `done` is `true`.
also, remove `if (e instanceof StopIteration) {` branch from `catch` clause below.

::: dom/system/gonk/ril_worker.js:10455
(Diff revision 1)
> -    let iter = Iterator(ctlvs);
> -    for (let [index, ctlv] in iter) {
> +    let iter = function*() {
> +      for (let key of Object.keys(ctlvs)) {
> +        yield([key, ctlvs[key]]);
> +      }
> +    };
> +    for (let [index, ctlv] of iter()) {

`ctlvs` is an Array, and the loop below needs values (`ctlv`) only, so you can iterate over its values by:
```
  for (let ctlv of ctlvs)
```

::: dom/system/gonk/ril_worker.js:11062
(Diff revision 1)
>      return {fileId : (GsmPDUHelper.readHexOctet() << 8) +
>                        GsmPDUHelper.readHexOctet()};
>    },
>  
>    searchForNextTag: function(tag, iter) {
> -    for (let [index, tlv] in iter) {
> +    for (let [index, tlv] of iter()) {

this case is a bit complicated.

`iter` is reused between multiple calls, and its internal state is NOT reset for each call.

for example, if `iter` is iterating over A, B, C, D, E,
and 1st call iterates over A and B, then returns,
the 2nd call starts iterating from C.

so, at least you cannot call `iter` here.
you need to create iterator in caller, and pass the same value across multiple calls.

Then, this loop needs values (`tlv`) only, so you don't need to yield or assign `index`.

::: dom/system/gonk/ril_worker.js:11417
(Diff revision 1)
>      // See TS 102 221 Table 11.4 for the content order of getResponse.
> -    let iter = Iterator(berTlv.value);
> +    let iter = function*() {
> +      for (let key of Object.keys(berTlv.value)) {
> +        yield([key, berTlv.value[key]]);
> +      }
> +    };

As mentioned above,
if you use generator, you need to call it here, not in `searchForNextTag`.

btw, the loop above needs only values (`tlv`), and `BerTlv.value` is an Array (unless I'm missing something),
so you can use `betTlv.value.values()` to get an iterator that iterates over values.
```
  let iter = betTlv.value.values();
```

::: dom/system/gonk/tests/test_ril_worker_icc_BerTlvHelper.js:40
(Diff revision 1)
>    let berTlv = berHelper.decode(tag_test.length);
> -  let iter = Iterator(berTlv.value);
> +  let iter = function*() {
> +      for (let key of Object.keys(berTlv.value)) {
> +        yield([key, berTlv.value[key]]);
> +      }
> +  };

here too, `verTlv.value` is an Array, so you can use `verTlv.value.values()` to get almost same thing.

::: dom/system/gonk/tests/test_ril_worker_icc_BerTlvHelper.js:82
(Diff revision 1)
>    let berTlv = berHelper.decode(tag_test.length);
> -  let iter = Iterator(berTlv.value);
> +  let iter = function*() {
> +      for (let key of Object.keys(berTlv.value)) {
> +        yield([key, berTlv.value[key]]);
> +      }
> +  };

same here.

Comment 4

[Sumit Tiwari]

Comment on attachment 8780358 [details]
Bug 1292999 - Replace Iterator() in dom/;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71084/diff/1-2/

Comment 5

[Tooru Fujisawa [:arai]]

Comment on attachment 8780358 [details]
Bug 1292999 - Replace Iterator() in dom/;

https://reviewboard.mozilla.org/r/71084/#review68940

Thank you again :)
It's almost there.

::: dom/camera/test/test_camera_fake_parameters.html:475
(Diff revision 2)
>      });
>  
> -    var sizeGenerator = Iterator(expSizes);
> +    var sizeIterator = expSizes.values();
>      return new Promise(function(resolve, reject) {
>        function nextSize() {
> -        try {
> +          var {value:size, done} = sizeIterator().next();

`sizeIterator.next()`

Also, please reduce the indent of this block 2 spaces (as you removed `try-catch`), to make this block following 2-spaces indentation.

::: dom/camera/test/test_camera_fake_parameters.html:476
(Diff revision 2)
>  
> -    var sizeGenerator = Iterator(expSizes);
> +    var sizeIterator = expSizes.values();
>      return new Promise(function(resolve, reject) {
>        function nextSize() {
> -        try {
> -          var size = sizeGenerator.next()[1];
> +          var {value:size, done} = sizeIterator().next();
> +          if (!done) {

you could swap the condition to `if (done) {`, and early return here.
```
if (done) {
  resolve();
  return;
}
```
so that remaining block can have fewer indent.

Comment 6

[Sumit Tiwari]

Comment on attachment 8780358 [details]
Bug 1292999 - Replace Iterator() in dom/;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71084/diff/2-3/

Comment 7

[Sumit Tiwari]

Comment on attachment 8780358 [details]
Bug 1292999 - Replace Iterator() in dom/;

https://reviewboard.mozilla.org/r/71084/#review68940

Thank you for your patience and diligence in reviewing the code. :)

Comment 8

[Tooru Fujisawa [:arai]]

Comment on attachment 8780358 [details]
Bug 1292999 - Replace Iterator() in dom/;

https://reviewboard.mozilla.org/r/71084/#review68954

Thank you :)

Only one styling fix needed there.

::: dom/camera/test/test_camera_fake_parameters.html:478
(Diff revision 3)
>      return new Promise(function(resolve, reject) {
>        function nextSize() {
> -        try {
> -          var size = sizeGenerator.next()[1];
> +        var {value:size, done} = sizeIterator.next();
> +        if (done) {
> +          resolve();
> +          return;  

Please remove trailing 2 spaces after "return;"

Comment 9

[Sumit Tiwari]

Comment on attachment 8780358 [details]
Bug 1292999 - Replace Iterator() in dom/;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71084/diff/3-4/

Comment 10

[Tooru Fujisawa [:arai]]

Comment on attachment 8780358 [details]
Bug 1292999 - Replace Iterator() in dom/;

https://reviewboard.mozilla.org/r/71084/#review68956

Thank you again :)

Looks good.
Forwarding the review request to smaug, as I'm not a module owner/peer here.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8780358 [details]
Bug 1292999 - Replace Iterator() in dom/;

Happy to let arai to review this.

Comment 12

[Tooru Fujisawa [:arai]]

Comment on attachment 8780358 [details]
Bug 1292999 - Replace Iterator() in dom/;

https://reviewboard.mozilla.org/r/71084/#review69054

Okay, thanks :)

It passed the try run.
will land it shortly.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af8f5fa2d6ca
Replace Iterator() in dom/; r=arai

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/af8f5fa2d6ca