[BinAST] Expose the APIs necessary for skipping skippable nodes

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

(Blocks 1 bug)

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

No description provided.
Attachment #8970834 - Flags: review?(efaustbmo)
Attachment #8970834 - Flags: review?(arai.unmht)
Assignee: nobody → dteller
Priority: -- → P2
Comment on attachment 8970834 [details]
Bug 1456609 - Expose a method readSkippableSubTree for BinTokenReader{Tester, Multipart}

https://reviewboard.mozilla.org/r/239612/#review245560

I have some questions.

::: js/src/frontend/BinSource-auto.cpp:6174
(Diff revision 1)
> +#endif // defined(DEBUG)
> +
> +
> +
> +
> +    BINJS_MOZ_TRY_DECL(offset, tokenizer_->readSkippableSubTree());

why is this assigning the tree to a variable named `offset`?
(instead of `skipped` or something?)
(maybe related to the change in main.rs)

also, where does the `BinField::Offset` come from?
there's only `skipped` field in SkippableSOMETHING interfaces.

::: js/src/frontend/binsource/src/main.rs:1002
(Diff revision 1)
> -                        (Some(format!("uint32_t {var_name};", var_name = var_name)),
> -                        Some(format!("MOZ_TRY_VAR({var_name}, tokenizer_->readOffset());", var_name = var_name)))
> +                        (Some(format!("BinTokenReaderBase::SkippableSubTree {var_name};", var_name = var_name)),
> +                        Some(format!("MOZ_TRY_VAR({var_name}, tokenizer_->readSkippableSubTree());", var_name = var_name)))
>                      } else {
>                          (None,
> -                        Some(format!("BINJS_MOZ_TRY_DECL({var_name}, tokenizer_->readOffset());", var_name = var_name)))
> +                        Some(format!("BINJS_MOZ_TRY_DECL({var_name}, tokenizer_->readSkippableSubTree());", var_name = var_name)))

this sounds strange.
this block is handling `offset`, right?
why are you modifying this block to handle `SkippableSubTree` ?
and how this generates the file in the patch?
Comment on attachment 8970834 [details]
Bug 1456609 - Expose a method readSkippableSubTree for BinTokenReader{Tester, Multipart}

https://reviewboard.mozilla.org/r/239612/#review245560

> why is this assigning the tree to a variable named `offset`?
> (instead of `skipped` or something?)
> (maybe related to the change in main.rs)
> 
> also, where does the `BinField::Offset` come from?
> there's only `skipped` field in SkippableSOMETHING interfaces.

The `_offset` field (which is normalized to `offset` in C++) and the `Offset` data structure are magically added during grammar preprocessing to every node labelled `[Skippable]`. This is done by the crate `binjs_meta`, which is vendored. I agree that name `_offset` is ill-chosen, so there is a patch to `binjs_meta` that renames it `_skip`.

As this is not observable, I just didn't want to vendor the new version just yet, as I already expect efaust's merging to be pretty complicated, and I didn't want to make it even worse.
Comment on attachment 8970834 [details]
Bug 1456609 - Expose a method readSkippableSubTree for BinTokenReader{Tester, Multipart}

https://reviewboard.mozilla.org/r/239612/#review245624

Thank you for explanation :)
Attachment #8970834 - Flags: review?(arai.unmht) → review+
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b917e7e80a51
Expose a method readSkippableSubTree for BinTokenReader{Tester, Multipart} r=arai
https://hg.mozilla.org/mozilla-central/rev/b917e7e80a51
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Attachment #8970834 - Flags: review?(efaustbmo)
You need to log in before you can comment on or make changes to this bug.