Summary: | [ES6] Enable Symbol in web pages | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | benjamin, commit-queue, fpizlo, ggaren, joepeck, rniwa | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | 141106, 143404 | ||||||
Bug Blocks: | |||||||
Attachments: |
|
Description
Yusuke Suzuki
2015-04-03 08:14:50 PDT
Created attachment 250160 [details]
Patch
After https://bugs.webkit.org/show_bug.cgi?id=143404, I'm planning to land it and enable symbols in web pages. Comment on attachment 250160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250160&action=review > Source/JavaScriptCore/ChangeLog:18 > + and makes enabling symbols by default. Or is it preferable to drop this runtime flag completely? This runtime flag will affect when adding Array.prototype.values. Comment on attachment 250160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250160&action=review >> Source/JavaScriptCore/ChangeLog:18 >> + and makes enabling symbols by default. > > Or is it preferable to drop this runtime flag completely? > This runtime flag will affect when adding Array.prototype.values. Not sure about others, but I think we should dump it completely. Comment on attachment 250160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250160&action=review >>> Source/JavaScriptCore/ChangeLog:18 >>> + and makes enabling symbols by default. >> >> Or is it preferable to drop this runtime flag completely? >> This runtime flag will affect when adding Array.prototype.values. > > Not sure about others, but I think we should dump it completely. I think we should keep it for a while given the previous attempt to enable this feature broke Facebook.com although I agree we should remove it eventually. > I think we should keep it for a while given the previous attempt to enable
> this feature broke Facebook.com
> although I agree we should remove it eventually.
+1
Are you going to land this patch? I was planning to land it after https://bugs.webkit.org/show_bug.cgi?id=143404 is landed. But maybe Symbol.for is not so used. So I'll land it first ;) Comment on attachment 250160 [details] Patch Clearing flags on attachment: 250160 Committed r182653: <http://trac.webkit.org/changeset/182653> All reviewed patches have been landed. Closing bug. JSC tests fail. However, it's very curious. "weak value == nil" is failed, but in this patch, changes in JSC shell is very few. Now in this patch, we only attached implemented well known symbols. So, before this patch, Symbol has the following well known symbols property. macro(hasInstance) \ macro(isConcatSpreadable) \ macro(iterator) \ macro(match) \ macro(replace) \ macro(search) \ macro(species) \ macro(split) \ macro(toPrimitive) \ macro(toStringTag) \ macro(unscopables) After this patch, we only attached macro(iterator) \ macro(unscopables) After investigating the cause of JSC tests failure, I've found that when adding not implemented well known symbols, JSC tests pass. macro(hasInstance) \ macro(isConcatSpreadable) \ macro(iterator) \ macro(match) \ macro(replace) \ macro(search) \ macro(species) \ macro(split) \ macro(toPrimitive) \ macro(toStringTag) \ macro(unscopables) But I think it's storage... After investigation, I've found the test sometimes pass / fail when inserting breakpoint in lldb. And this failure is caused because the previous JSGlobalObject is not collected. I suppose that because previous JSGlobalObject's pointer is still alive in registers or C stacks, it's not released and it causes this failure. OK, so, since it's related to conservative GC (stack/register values) issue, now tests pass. Opened issue for this API test failure. https://bugs.webkit.org/show_bug.cgi?id=143634 Comment on attachment 250160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250160&action=review > Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:124 > m_frontendPage->settings().setJavaScriptRuntimeFlags({ > - JSC::RuntimeFlags::SymbolEnabled > }); Does this function need to be called at all with the empty arguments? > Source/WebKit/mac/WebView/WebPreferencesPrivate.h:56 > - WebKitJavaScriptRuntimeFlagsSymbolEnabled = 1u << 0, > + WebKitJavaScriptRuntimeFlagsSymbolDisabled = 1u << 0, Why do we have an SPI flag for this, does any WebKit client want to disable Symbol? "Private" headers are SPI, meaning that internal Apple clients can rely on them. So, exposing more than we need has a long-term cost, and once they are exposed, they generally cannot be changed. > Source/WebKit2/UIProcess/API/C/WKPreferencesRefPrivate.h:53 > - kWKJavaScriptRuntimeFlagsSymbolEnabled = 1 << 0, > + kWKJavaScriptRuntimeFlagsSymbolDisabled = 1 << 0, Ditto. (In reply to comment #15) > > > Source/WebKit/mac/WebView/WebPreferencesPrivate.h:56 > > - WebKitJavaScriptRuntimeFlagsSymbolEnabled = 1u << 0, > > + WebKitJavaScriptRuntimeFlagsSymbolDisabled = 1u << 0, > > Why do we have an SPI flag for this, does any WebKit client want to disable > Symbol? > > "Private" headers are SPI, meaning that internal Apple clients can rely on > them. So, exposing more than we need has a long-term cost, and once they are > exposed, they generally cannot be changed. > > > Source/WebKit2/UIProcess/API/C/WKPreferencesRefPrivate.h:53 > > - kWKJavaScriptRuntimeFlagsSymbolEnabled = 1 << 0, > > + kWKJavaScriptRuntimeFlagsSymbolDisabled = 1 << 0, > > Ditto. That's a good point. We should probably remove these runtime flags from private headers right now since we don't need them to disable the feature on websites. Comment on attachment 250160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250160&action=review >> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:124 >> }); > > Does this function need to be called at all with the empty arguments? To search runtime flags when changing it, calling it here is nice I think. (In reply to comment #16) > (In reply to comment #15) > > > > > Source/WebKit/mac/WebView/WebPreferencesPrivate.h:56 > > > - WebKitJavaScriptRuntimeFlagsSymbolEnabled = 1u << 0, > > > + WebKitJavaScriptRuntimeFlagsSymbolDisabled = 1u << 0, > > > > Why do we have an SPI flag for this, does any WebKit client want to disable > > Symbol? > > > > "Private" headers are SPI, meaning that internal Apple clients can rely on > > them. So, exposing more than we need has a long-term cost, and once they are > > exposed, they generally cannot be changed. > > > > > Source/WebKit2/UIProcess/API/C/WKPreferencesRefPrivate.h:53 > > > - kWKJavaScriptRuntimeFlagsSymbolEnabled = 1 << 0, > > > + kWKJavaScriptRuntimeFlagsSymbolDisabled = 1 << 0, > > > > Ditto. > > That's a good point. We should probably remove these runtime flags from > private headers right now since we don't need them to disable the feature on > websites. Is it better moving them to public headers? No, we shouldn't expose it as SPI or API. Random app embedding WebKit shouldn't be able to change this runtime flag at all since we want to remove it soon. For binary compatibility, we can't easily remove SPI or API once added. |