[v8] Minor cleanup: make 2nd argument of removeIfPresnt accept only a value type stored in the DOM map
Created attachment 77567 [details] Patch
Comment on attachment 77567 [details] Patch How do we test this? Please explain what tests this affects in your ChagneLog or explain why testing is impossible.
Eric, that should be covered by the current test. The change is mostly syntactic, all it requires an additional cast which is noop in Release mode. (In reply to comment #2) > (From update of attachment 77567 [details]) > How do we test this? Please explain what tests this affects in your ChagneLog or explain why testing is impossible.
And just in case: this path is exercised by virtually any web page if it only touches a DOM (In reply to comment #3) > Eric, that should be covered by the current test. > > The change is mostly syntactic, all it requires an additional cast which is noop in Release mode. > > (In reply to comment #2) > > (From update of attachment 77567 [details] [details]) > > How do we test this? Please explain what tests this affects in your ChagneLog or explain why testing is impossible.
Created attachment 77602 [details] Patch
Comment on attachment 77602 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77602&action=review > WebCore/bindings/v8/V8DOMMap.h:-63 > - virtual bool removeIfPresent(KeyType* key, v8::Persistent<v8::Data> value) = 0; Why is this virtual if Its never overridden?
(In reply to comment #6) > (From update of attachment 77602 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77602&action=review > > > WebCore/bindings/v8/V8DOMMap.h:-63 > > - virtual bool removeIfPresent(KeyType* key, v8::Persistent<v8::Data> value) = 0; > > Why is this virtual if Its never overridden? It's overridden (see two implementations of this 'interface'), otherwise it won't compile.
Is this patch supposed to be up for review? If so, please flag with r? otherwise it won't get seen in the review queue and will likely be ignored. You also should CC some v8-related reviewers (of which I'm not one).
I am dreadful sorry, I forgot to put the r?. Done. Sorry again. (In reply to comment #8) > Is this patch supposed to be up for review? If so, please flag with r? otherwise it won't get seen in the review queue and will likely be ignored. > > You also should CC some v8-related reviewers (of which I'm not one).
No problem at all. I don't really know what reviewers to suggest for this, but webkit-patch suggest-reviewers can calculate some for you if you find this isn't reviewed in a timely manner.
(In reply to comment #10) > No problem at all. I don't really know what reviewers to suggest for this, but webkit-patch suggest-reviewers can calculate some for you if you find this isn't reviewed in a timely manner. Let me add Mads and Nate.
Comment on attachment 77602 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77602&action=review I don't understand how this is covered by existing tests. > WebCore/bindings/v8/DOMDataStore.cpp:157 > + v8::Persistent<v8::Object> v8Object = v8::Persistent<v8::Object>::Cast(value); So it's not possible that it'd be empty/null/undefined?
(In reply to comment #12) > (From update of attachment 77602 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77602&action=review > > I don't understand how this is covered by existing tests. Every DOM Node is added into this map when it's wrapped into JS object. And eventually, when this wrapper is garbage collected, method removeIfPresent is invoked for each domNodeMap we have. Thus one need to kill renderer process before it does any GC to miss this path. And we have plenty layout tests which exercise GC-related behaviour. > > > WebCore/bindings/v8/DOMDataStore.cpp:157 > > + v8::Persistent<v8::Object> v8Object = v8::Persistent<v8::Object>::Cast(value); > > So it's not possible that it'd be empty/null/undefined? No, it cannot be---it's a wrapper which is now only weakly reachable. And it cannot be empty by design. And null and undefined are never GCed (and never wrapped into persistent handles). Thanks a lot for your comment, Jeremy.
Comment on attachment 77602 [details] Patch Clearing flags on attachment: 77602 Committed r76104: <http://trac.webkit.org/changeset/76104>
All reviewed patches have been landed. Closing bug.