Bug 51683 - [v8] Minor cleanup: make 2nd argument of removeIfPresnt accept only a value type stored in the DOM map
Summary: [v8] Minor cleanup: make 2nd argument of removeIfPresnt accept only a value t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: anton muhin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-28 11:35 PST by anton muhin
Modified: 2011-01-18 23:41 PST (History)
7 users (show)

See Also:


Attachments
Patch (3.99 KB, patch)
2010-12-28 11:40 PST, anton muhin
no flags Details | Formatted Diff | Diff
Patch (4.03 KB, patch)
2010-12-28 23:48 PST, anton muhin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description anton muhin 2010-12-28 11:35:15 PST
[v8] Minor cleanup: make 2nd argument of removeIfPresnt accept only a value type stored in the DOM map
Comment 1 anton muhin 2010-12-28 11:40:52 PST
Created attachment 77567 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-12-28 13:47:45 PST
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.
Comment 3 anton muhin 2010-12-28 23:45:16 PST
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.
Comment 4 anton muhin 2010-12-28 23:46:20 PST
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.
Comment 5 anton muhin 2010-12-28 23:48:23 PST
Created attachment 77602 [details]
Patch
Comment 6 Eric Seidel (no email) 2010-12-30 11:46:33 PST
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?
Comment 7 anton muhin 2010-12-30 11:54:03 PST
(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.
Comment 8 Eric Seidel (no email) 2011-01-18 11:19:55 PST
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).
Comment 9 anton muhin 2011-01-18 11:21:54 PST
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).
Comment 10 Eric Seidel (no email) 2011-01-18 11:25:16 PST
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.
Comment 11 anton muhin 2011-01-18 11:26:11 PST
(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 12 Jeremy Orlow 2011-01-18 11:30:46 PST
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?
Comment 13 anton muhin 2011-01-18 11:42:24 PST
(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 14 WebKit Commit Bot 2011-01-18 23:40:57 PST
Comment on attachment 77602 [details]
Patch

Clearing flags on attachment: 77602

Committed r76104: <http://trac.webkit.org/changeset/76104>
Comment 15 WebKit Commit Bot 2011-01-18 23:41:02 PST
All reviewed patches have been landed.  Closing bug.