| Summary: | We may add a ReadOnly property without setting the corresponding bit on Structure | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Saam Barati <saam> | ||||
| Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | benjamin, commit-queue, fpizlo, ggaren, gskachkov, keith_miller, mark.lam, msaboff, oliver, sukolsak, webkit-bug-importer, ysuzuki | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Local Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Saam Barati
2016-07-07 20:11:43 PDT
Created attachment 283228 [details]
patch
Comment on attachment 283228 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=283228&action=review > Source/JavaScriptCore/runtime/JSObject.h:1569 > + if (attributes & ReadOnly) > + structure->setContainsReadOnlyProperties(); I put this here to be consistent with JSObject::putDirectInternal, but I almost feel like putting it inside Structure::addPropertyWithoutTransition is a more natural place for it. Comment on attachment 283228 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=283228&action=review >> Source/JavaScriptCore/runtime/JSObject.h:1569 >> + structure->setContainsReadOnlyProperties(); > > I put this here to be consistent with JSObject::putDirectInternal, > but I almost feel like putting it inside Structure::addPropertyWithoutTransition > is a more natural place for it. I think that makes sense in addPropertyWithoutTransition() for both call sites. What's wrong with that? Comment on attachment 283228 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=283228&action=review >>> Source/JavaScriptCore/runtime/JSObject.h:1569 >>> + structure->setContainsReadOnlyProperties(); >> >> I put this here to be consistent with JSObject::putDirectInternal, >> but I almost feel like putting it inside Structure::addPropertyWithoutTransition >> is a more natural place for it. > > I think that makes sense in addPropertyWithoutTransition() for both call sites. What's wrong with that? I agree. I'm going to open another bug to consider this. I think we should also consider the Structure::addNewPropertyTransition to have it mark the bit on the new transition it's returning. Comment on attachment 283228 [details] patch Clearing flags on attachment: 283228 Committed r203015: <http://trac.webkit.org/changeset/203015> All reviewed patches have been landed. Closing bug. |