We will change classes that do ref counting to start their ref counts at 1 instead of starting at 0 and then getting incremented to 1 by a later call to ref().
Created attachment 19017 [details] patch
Comment on attachment 19017 [details] patch r=me + (KJS::RegExp::create): Added. Calls new RegExp then adopt's the initial ref. Can simply say "adopts" without the apostrophe.
Comment on attachment 19017 [details] patch Clearing the review flag since I landed this.
Created attachment 19212 [details] patch for next step -- actually remove the default
Comment on attachment 19212 [details] patch for next step -- actually remove the default r=me
Comment on attachment 19212 [details] patch for next step -- actually remove the default Landed. Clearing review flag.
Created attachment 19228 [details] work in progress on StyleBase and classes derived from it
Created attachment 19229 [details] work in progress on some other RefCounted classes
Created attachment 19230 [details] a patch with a "sniffer" I was using to see 0 ref counts that were actually hit at runtime
Comment on attachment 19017 [details] patch Marking patch already-landed as obsolete so the list of patches is easier to see.
Comment on attachment 19212 [details] patch for next step -- actually remove the default Marking patch already-landed as obsolete so the list of patches is easier to see.
Anders is working on this now (even more than I am). We're down to only 70 sites that use RefCounted(0).
Created attachment 21561 [details] patch
Comment on attachment 21561 [details] patch These look like they don't belong in this patch: JavaScriptCore/VM/CodeBlock.cpp WebCore/plugins/PluginDebug.h (also included in the ChangeLog) WebKitTools/Scripts/make-js-test-wrappers Other comments: 38 PassRefPtr<StaticNodeList> createSelectorNodeList(PassRefPtr<Node> rootNode, CSSSelector*); Maybe this should be a static StaticNodeList method? Perhaps StaticNodeList::create[By|With|From]Selector(). 101 : RefCounted<HistoryItem>(1) Is the 1 necessary here? 79 static PassRefPtr<HTMLCollection> create(PassRefPtr<Node> base, Type); 38 static PassRefPtr<HTMLFormCollection> create(PassRefPtr<HTMLFormElement>); 38 static PassRefPtr<HTMLOptionsCollection> create(PassRefPtr<HTMLSelectElement>); 41 static PassRefPtr<HTMLTableRowsCollection> create(PassRefPtr<HTMLTableElement>); Why are those not defined in the header? I cannot r+ because I haven't read JSSVGTransformListCustom.cpp, JSSVGPointListCustom.cpp and JSSVGPODTypeWrapper.h. I will try to get to them later or somebody else could review just those (or you can land everything else with r=me).
(In reply to comment #14) > These look like they don't belong in this patch: > > JavaScriptCore/VM/CodeBlock.cpp Oops. Need to land that to fix some warnings, but clearly not the same patch. > WebCore/plugins/PluginDebug.h (also included in the ChangeLog) Right, just another warning fix. > WebKitTools/Scripts/make-js-test-wrappers Yes, same thing. > Other comments: > > 38 PassRefPtr<StaticNodeList> createSelectorNodeList(PassRefPtr<Node> > rootNode, CSSSelector*); > > Maybe this should be a static StaticNodeList method? Perhaps > StaticNodeList::create[By|With|From]Selector(). I don't see why it should be. This is a function that yields a static node list. I wouldn't want to make all functions that yield vectors members of the Vector class. > 101 : RefCounted<HistoryItem>(1) > > Is the 1 necessary here? It is. This is a copy constructor and so we need to initialize base classes explicitly. I tried without it first. > 79 static PassRefPtr<HTMLCollection> create(PassRefPtr<Node> base, Type); > > 38 static PassRefPtr<HTMLFormCollection> > create(PassRefPtr<HTMLFormElement>); > > 38 static PassRefPtr<HTMLOptionsCollection> > create(PassRefPtr<HTMLSelectElement>); > > 41 static PassRefPtr<HTMLTableRowsCollection> > create(PassRefPtr<HTMLTableElement>); > > Why are those not defined in the header? Because I'd have to add additional includes to make those headers compile, and I don't think it's crucial that all these functions be inlined. > I cannot r+ because I haven't read JSSVGTransformListCustom.cpp, > JSSVGPointListCustom.cpp and JSSVGPODTypeWrapper.h. I will try to get to them > later or somebody else could review just those (or you can land everything else > with r=me). I think I'll separate those into a different patch since you reviewed the rest.
It looks like the latest patch breaks the Gtk build. WebKit/gtk/webkit/webkitwebhistoryitem.cpp references an old version of the HistoryItem function that was removed. Specifically, the webkit_web_history_item_new_with_data is using the HistoryItem constructor that takes a KURL* and String as parameters.
Comment on attachment 21561 [details] patch I landed the part of this that Dan reviewed, so clearing the review flag.
Created attachment 21572 [details] patch that does this for more classes
Comment on attachment 21572 [details] patch that does this for more classes r=me. Because I like the one class per file rule, I would would prefer that SimpleEditCommand be in its own file, but, I realize that is a little odd for such a small file.
Comment on attachment 21572 [details] patch that does this for more classes Clearing review flag since I landed this.
Created attachment 21703 [details] patch that deals with almost all remaining classes After this patch is landed, what we have left is: StyleBase NodeFilter TreeShared ThreadSafeShared
Comment on attachment 21703 [details] patch that deals with almost all remaining classes Can ParserRefCounted::hasOneRef be const as well? r=me
(In reply to comment #23) > Can ParserRefCounted::hasOneRef be const as well? I tried making it const, ran into minor problems (casting to work with the dictionary), and so decided not to bother.
Comment on attachment 21703 [details] patch that deals with almost all remaining classes Clearing review flag since this was landed.
Created attachment 21722 [details] patch that converts StyleBase and classes derived from it
Comment on attachment 21722 [details] patch that converts StyleBase and classes derived from it I think it would be better to make the computedStyle function be called CSSComputedStyleDeclaration::create. Is there a reason to change the style for it? Can this be done with a default value for charset of String()? + static PassRefPtr<CSSStyleSheet> create(Node* ownerNode, const String& href) + { + return adoptRef(new CSSStyleSheet(ownerNode, href, String())); + } + static PassRefPtr<CSSStyleSheet> create(Node* ownerNode, const String& href, const String& charset) + { + return adoptRef(new CSSStyleSheet(ownerNode, href, charset)); + } r=me
(In reply to comment #27) > (From update of attachment 21722 [details] [edit]) > I think it would be better to make the computedStyle function be called > CSSComputedStyleDeclaration::create. Is there a reason to change the style for > it? The computedStyle function already existed. I didn't want to have both that function and CSSComputedStyleDeclaration::create. I could be convinced to do it another way. > Can this be done with a default value for charset of String()? > > + static PassRefPtr<CSSStyleSheet> create(Node* ownerNode, const String& > href) > + { > + return adoptRef(new CSSStyleSheet(ownerNode, href, String())); > + } > + static PassRefPtr<CSSStyleSheet> create(Node* ownerNode, const String& > href, const String& charset) > + { > + return adoptRef(new CSSStyleSheet(ownerNode, href, charset)); > + } Sure, it could be done either way. Those are just two different ways of writing the same code.
Comment on attachment 21722 [details] patch that converts StyleBase and classes derived from it Clearing reviewed flag on this one since I landed it.
(In reply to comment #22) > After this patch is landed, what we have left is: > > StyleBase Done. > NodeFilter Done. > TreeShared This one is really a separate task. When we wean classes away from TreeShared, we can make the refcount start at 1 at the same time. > ThreadSafeShared I was mistaken, this was already done a while ago.