Closed
Bug 1493703
Opened 6 years ago
Closed 6 years ago
Tech debt: clean up ENABLE_WASM_GC #ifdefs
Categories
(Core :: JavaScript: WebAssembly, enhancement, P3)
Core
JavaScript: WebAssembly
Tracking
()
RESOLVED
FIXED
People
(Reporter: lth, Assigned: lth)
References
Details
Attachments
(3 files)
1.53 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
10.04 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
5.30 KB,
patch
|
luke
:
feedback+
|
Details | Diff | Splinter Review |
The ENABLE_WASM_GC #ifdefs are applied inconsistently. They fall into four broad classes: just plain unnecessary (nested within others); safe to remove or adjust without creating more attack surface; controlling the text->binary function; and controlling various switches and hacks that will go away once we have proper tracing. I'm going to try to land patches for the first two; upload for discussion a patch for the third; and not try to do anything about the fourth at this point, as it just creates turbulence for other changes that will be coming soon.
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #9011489 -
Flags: review?(luke)
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #9011490 -
Flags: review?(luke)
Assignee | ||
Comment 3•6 years ago
|
||
What's your thinking about this? There's no danger in applying this, and I guess wasmTextToBinary is not in Firefox in any case, it's just in the shell...
Attachment #9011492 -
Flags: feedback?(luke)
Updated•6 years ago
|
Attachment #9011489 -
Flags: review?(luke) → review+
Updated•6 years ago
|
Attachment #9011490 -
Flags: review?(luke) → review+
Comment 4•6 years ago
|
||
Comment on attachment 9011492 [details] [diff] [review] bug1493703-parser-ifdefs.patch Review of attachment 9011492 [details] [diff] [review]: ----------------------------------------------------------------- I was trying to think of whether it's useful to have all the GC features in a bucket so we can audit the final set of allowed syntax... but it's not exposed to content so the stakes are low for dangling syntax so removal makes sense. [Incidentally, WasmTextToBinary.cpp is *built* with Firefox, it's just not installed on any global objects. It is possible to get wasmTextToBinary via getJSTestingFunctions().wasmTextToBinary if you have access to Components.utils (in chrome JS) or via the worker global scope if automation is enabled.]
Attachment #9011492 -
Flags: feedback?(luke) → feedback+
Pushed by lhansen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e57f4697f91f Remove truly redundant #ifdef ENABLE_WASM_GC. r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/f7fd0abf23ce Remove more fairly safe #ifdef ENABLE_WASM_GC. r=luke
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e57f4697f91f https://hg.mozilla.org/mozilla-central/rev/f7fd0abf23ce
Assignee | ||
Comment 7•6 years ago
|
||
Closing this without further work since further work will happen when we pry apart the #ifdefs for wasm reftypes and wasm gc.
You need to log in
before you can comment on or make changes to this bug.
Description
•