Fix compilation of V8DependentRetained and JSDependentRetained.
Created attachment 166337 [details] Patch
Comment on attachment 166337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166337&action=review > Source/WebCore/ChangeLog:8 > + Fix bad usage of putDirect and removeDirect from JSDependentRetained and fix The JSC change looks good. > Source/WebCore/ChangeLog:9 > + incorrect assumptions about how weak handles work in V8. This is refactored The point of the V8 fix is: - Use m_owner.set() instead of m_owern =. - Call value->release() in the weak callback. Am I understanding correctly? > Source/WebCore/bindings/v8/V8DependentRetained.h:37 > +#include <wtf/text/StringBuilder.h> Nit: Why is this needed?
Comment on attachment 166337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166337&action=review >> Source/WebCore/ChangeLog:9 >> + incorrect assumptions about how weak handles work in V8. This is refactored > > The point of the V8 fix is: > > - Use m_owner.set() instead of m_owern =. > - Call value->release() in the weak callback. > > Am I understanding correctly? - Yeah, need to use set() since ScopedPersistent doesn't have overloaded operator=. - I originally had one weakCallback and abarth suggested using two callbacks, one for the owner and one for the value and asserting about the order, but that was a wrong thing to do since the order of the callbacks doesn't seem to work as expected when v8 destroys everything (whole page navigate). No code uses this code yet, but if you did you'd get asserts. >> Source/WebCore/bindings/v8/V8DependentRetained.h:37 >> +#include <wtf/text/StringBuilder.h> > > Nit: Why is this needed? The code didn't compile because someone else refactored it before my other patch landed that actually uses this, and they made it use StringBuilder (see in the create createPropertyName method in the file). No one noticed it broke because nothing uses this yet.
Comment on attachment 166337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166337&action=review >>> Source/WebCore/ChangeLog:9 >>> + incorrect assumptions about how weak handles work in V8. This is refactored >> >> The point of the V8 fix is: >> >> - Use m_owner.set() instead of m_owern =. >> - Call value->release() in the weak callback. >> >> Am I understanding correctly? > > - Yeah, need to use set() since ScopedPersistent doesn't have overloaded operator=. > - I originally had one weakCallback and abarth suggested using two callbacks, one for the owner and one for the value and asserting about the order, but that was a wrong thing to do since the order of the callbacks doesn't seem to work as expected when v8 destroys everything (whole page navigate). No code uses this code yet, but if you did you'd get asserts. Makes sense. Thanks for the clarification!
Comment on attachment 166337 [details] Patch Clearing flags on attachment: 166337 Committed r129968: <http://trac.webkit.org/changeset/129968>
All reviewed patches have been landed. Closing bug.