Bug 32389 - [v8] refactor WeakReferenceMap
Summary: [v8] refactor WeakReferenceMap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-10 12:25 PST by anton muhin
Modified: 2009-12-11 09:21 PST (History)
3 users (show)

See Also:


Attachments
first take (12.97 KB, patch)
2009-12-10 12:30 PST, anton muhin
abarth: review+
Details | Formatted Diff | Diff
just say no to virtuals (13.00 KB, patch)
2009-12-11 06:16 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 2009-12-10 12:25:25 PST
As spotted by Christian, there are some pieces of repeated code.  Bring those refactorings plus some more cleanups.
Comment 1 anton muhin 2009-12-10 12:30:20 PST
Created attachment 44633 [details]
first take
Comment 2 WebKit Review Bot 2009-12-10 12:34:30 PST
style-queue ran check-webkit-style on attachment 44633 [details] without any errors.
Comment 3 Adam Barth 2009-12-10 12:46:30 PST
Comment on attachment 44633 [details]
first take

Wow, this looks great.  I didn't consider using the visitor interface internally, but it makes for some prettier code.

+ virtual bool removeIfPresent
+ virtual void clear

Do these need to be virtual?  It doesn't look like they need to be overridden.
Comment 4 anton muhin 2009-12-11 06:16:55 PST
Created attachment 44681 [details]
just say no to virtuals
Comment 5 WebKit Review Bot 2009-12-11 06:17:58 PST
style-queue ran check-webkit-style on attachment 44681 [details] without any errors.
Comment 6 anton muhin 2009-12-11 06:20:26 PST
(In reply to comment #3)
> (From update of attachment 44633 [details])
> Wow, this looks great.  I didn't consider using the visitor interface
> internally, but it makes for some prettier code.
> 
> + virtual bool removeIfPresent
> + virtual void clear
> 
> Do these need to be virtual?  It doesn't look like they need to be overridden.

If we're lucky, they would be overridden. But conditionals are not for SVN, removing.

Thanks a lot for review.If we're lucky, they would be overridden. But conditionals are not for SVN, removing.

Thanks a lot for review.
Comment 7 WebKit Commit Bot 2009-12-11 09:21:23 PST
Comment on attachment 44681 [details]
just say no to virtuals

Clearing flags on attachment: 44681

Committed r51998: <http://trac.webkit.org/changeset/51998>
Comment 8 WebKit Commit Bot 2009-12-11 09:21:28 PST
All reviewed patches have been landed.  Closing bug.