Bug 126876

Summary: Avoid unnecessary copies of AccessibilityObject::AccessibilityChildrenVector
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, jcraig, jdiggs, mario, samuel_white
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch kling: review+

Description Zan Dobersek 2014-01-13 01:48:50 PST
Avoid unnecessary copies of AccessibilityObject::AccessibilityChildrenVector
Comment 1 Zan Dobersek 2014-01-13 01:57:56 PST
Avoiding the copies is a huge win on the GTK port. Testing the load of the singe-page HTML Living Standard[1] with WebKitTestRunner, the number of copies is reduced from hundreds of thousands to 75-300.

[1] http://www.whatwg.org/specs/web-apps/current-work/
Comment 2 Zan Dobersek 2014-01-13 01:58:54 PST
Created attachment 221016 [details]
Patch
Comment 3 Zan Dobersek 2014-01-13 03:15:34 PST
Committed r161873: <http://trac.webkit.org/changeset/161873>
Comment 4 chris fleizach 2014-01-13 05:55:45 PST
Comment on attachment 221016 [details]
Patch

I think we probably should use auto in all these cases
Comment 5 Zan Dobersek 2014-01-13 13:02:43 PST
(In reply to comment #4)
> (From update of attachment 221016 [details])
> I think we probably should use auto in all these cases

Do these cases align with any of the guildelines on using auto that were recently discussed on webkit-dev? AccessibilityObject::children() doesn't specifically note that a reference to the vector of object's children is being returned.

I'd very much like to use auto here, as long as it is used in line with the guidelines.
Comment 6 chris fleizach 2014-01-13 13:03:49 PST
(In reply to comment #4)
> (From update of attachment 221016 [details])
> I think we probably should use auto in all these cases

https://bugs.webkit.org/show_bug.cgi?id=126915
Comment 7 chris fleizach 2014-01-13 13:04:19 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 221016 [details] [details])
> > I think we probably should use auto in all these cases
> 
> Do these cases align with any of the guildelines on using auto that were recently discussed on webkit-dev? AccessibilityObject::children() doesn't specifically note that a reference to the vector of object's children is being returned.
> 
> I'd very much like to use auto here, as long as it is used in line with the guidelines.

After reading that discussion, I feel like this would probably fit the criteria. It removes a lot of disgusting code to write
Comment 8 Zan Dobersek 2014-01-13 13:10:08 PST
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 221016 [details] [details] [details])
> > > I think we probably should use auto in all these cases
> > 
> > Do these cases align with any of the guildelines on using auto that were recently discussed on webkit-dev? AccessibilityObject::children() doesn't specifically note that a reference to the vector of object's children is being returned.
> > 
> > I'd very much like to use auto here, as long as it is used in line with the guidelines.
> 
> After reading that discussion, I feel like this would probably fit the criteria. It removes a lot of disgusting code to write

Ah, changing to using the range-based for loops in bug #126876 makes things much better-looking, and auto also makes sense in that context.
Comment 9 chris fleizach 2014-01-13 13:16:40 PST
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (From update of attachment 221016 [details] [details] [details] [details])
> > > > I think we probably should use auto in all these cases
> > > 
> > > Do these cases align with any of the guildelines on using auto that were recently discussed on webkit-dev? AccessibilityObject::children() doesn't specifically note that a reference to the vector of object's children is being returned.
> > > 
> > > I'd very much like to use auto here, as long as it is used in line with the guidelines.
> > 
> > After reading that discussion, I feel like this would probably fit the criteria. It removes a lot of disgusting code to write
> 
> Ah, changing to using the range-based for loops in bug #126876 makes things much better-looking, and auto also makes sense in that context.

The question I had is whether a range based loop will ensure that only a reference is used.