Comment on attachment 453292 [details] [diff] [review] patch r=me with nits: Please use "Create" no underscore instead of "New_". Create is a more popular convention I think (in factory pattern). Please get a clean try run. I'm fine with the removal of the out of memory checks, but let's be consistent and not require these checks generally. +1 for removing nsINode from more internal API. -1 for making me look up "pellucid" :)

(In reply to comment #2) > (From update of attachment 453292 [details] [diff] [review]) > r=me with nits: > > Please use "Create" no underscore instead of "New_". Create is a more popular > convention I think (in factory pattern). I'm not sure I'm agree. I want to return them not addrefed but just an object created dynamic memory. Create and any other word makes callers to think it's addrefed I guess. I like New word since it's close to new operator. > Please get a clean try run. sorry, try server build or do you mean something else?

(In reply to comment #2) > I'm fine with the removal of the out of memory checks, but let's be consistent > and not require these checks generally. Technically memory checks shouldn't be removed and they aren't removed by this patch. Do I miss something?

(In reply to comment #3) > > Please get a clean try run. > > sorry, try server build or do you mean something else? Push to try, make sure we build ok and pass tests -- but not important as we don't expect a problem and I compiled on Mac just in case ;)

(In reply to comment #3) > > Please use "Create" no underscore instead of "New_". Create is a more popular > > convention I think (in factory pattern). > > I'm not sure I'm agree. I want to return them not addrefed but just an object > created dynamic memory. Create and any other word makes callers to think it's > addrefed I guess. I like New word since it's close to new operator. I'm not that picky, let's see what Roc says. (Note for example 'createDocument' http://en.wikipedia.org/wiki/Factory_method_pattern#C.2B.2B) r way is fine but I'd prefer no underscore.

Sure, I did this. - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/surkov.alexander@gmail.com-3149abd5f17e/ it's not finished yet but at least all has been done is good shape.

(In reply to comment #6) > I'm not that picky, let's see what Roc says. > (Note for example 'createDocument' > http://en.wikipedia.org/wiki/Factory_method_pattern#C.2B.2B) > r way is fine but I'd prefer no underscore. Ok, if it's common practice then I'm fine with this. Let's wait for Roc's opinion though.

Comment on attachment 453292 [details] [diff] [review] patch (In reply to comment #5) > (In reply to comment #3) > > > Please get a clean try run. > > > > sorry, try server build or do you mean something else? > > Push to try, make sure we build ok and pass tests -- but not important as we > don't expect a problem and I compiled on Mac just in case ;) I was looking at these removals: >- if (!*aAccessible) >- return NS_ERROR_OUT_OF_MEMORY; OK.

(In reply to comment #9) > I was looking at these removals: > > >- if (!*aAccessible) > >- return NS_ERROR_OUT_OF_MEMORY; They were hidden by nsAccessibilityService::GetAccessible() in any way, so now we return nsnull and have the same result.

Robert, what do you think New_ vs Create method prefixes?

I prefer Create. Why are you returning non-Addrefed objects?

(In reply to comment #12) > I prefer Create. ok > Why are you returning non-Addrefed objects? to avoid constructions of several nsRefPtr and already_AddRefed, the object returned from nsIFrame pass through few methods where it happens before we put it into cache and addref. So I would prefer to get raw pointer from nsIFrame so that we can put it into cache and addref in the end. In general I'm trying to avoid to deal with addref/release where it's safe and not necessary.

Passing around zero-refcount objects is not good because normally a single addref/release pair does nothing, but on a zero-refcount object it deletes the object. Since you're going to have to addref it eventually, I suggest you addref it early and using already_AddRefed and nsRefPtr::forget() to avoid unnecessary addrefs.

(In reply to comment #15) > Passing around zero-refcount objects is not good because normally a single > addref/release pair does nothing, but on a zero-refcount object it deletes the > object. Yes, I'm aware. > Since you're going to have to addref it eventually, I suggest you > addref it early and using already_AddRefed and nsRefPtr::forget() to avoid > unnecessary addrefs. That's we do now. Ok, I think we can continue to do this since it makes us potentially safer.

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/c39ab74a7da1

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; de; rv:2.0b2pre) Gecko/20100630 Minefield/4.0b2pre (.NET CLR 3.5.30729) by testing performance of first two testcases from bug 570500.