Bug 25425

Summary:

DOM attribute getter/setter functions should use const AtomicString& type

Product:

WebKit

Reporter:

Darin Adler <darin>

Component:

DOM

Assignee:

Darin Adler <darin>

Status:

RESOLVED WONTFIX

Severity:

Normal

CC:

arv, cdumez, dglazkov

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

27273

Attachments:

Description	Flags
patch with some work on this and other minor style issues	none
patch for part 1	none
patch for part 2	none
patch that does this for eight classes	none
patch that does this for four more classes	none
More of this type of thing	none

Description Darin Adler 2009-04-27 09:17:11 PDT

It’s more efficient for the DOM binding if DOM getter/setter functions use const AtomicString& for their results and arguments. Existing code uses String for results and const String& for arguments. The former results in unnecessary reference counting churn and re-lookup in the atomic string hash table. The latter can result in unnecessary memory allocation when the string in question is already present in the atomic string hash table.

Comment 1 Darin Adler 2009-04-27 09:17:44 PDT

Created attachment 29820 [details]
patch with some work on this and other minor style issues

Comment 2 Darin Adler 2009-06-21 22:30:36 PDT

Created attachment 31632 [details]
patch for part 1

Comment 3 Darin Adler 2009-06-22 01:21:05 PDT

Comment on attachment 31632 [details]
patch for part 1

Clear review flag since this is landed and there is more work to do. (Sorry, bdash.)

Comment 4 Dimitri Glazkov (Google) 2009-06-22 08:07:31 PDT

I'll take care of the V8 bindings.

Comment 5 Darin Adler 2009-06-22 09:33:28 PDT

Created attachment 31652 [details]
patch for part 2

Comment 6 Darin Adler 2009-06-22 18:06:18 PDT

Comment on attachment 31652 [details]
patch for part 2

Clear review flag since this is landed and there is more work to do.

Comment 7 Darin Adler 2009-07-14 11:19:37 PDT

Created attachment 32722 [details]
patch that does this for eight classes

Comment 8 Dimitri Glazkov (Google) 2009-07-14 12:58:23 PDT

Can you hold off landing this for a bit -- let me catch up w/V8 bindings.

Comment 9 Dimitri Glazkov (Google) 2009-07-14 15:09:25 PDT

Comment on attachment 32722 [details]
patch that does this for eight classes

LGTM :)

Comment 10 Dimitri Glazkov (Google) 2009-07-14 15:09:54 PDT

V8 side landed as http://trac.webkit.org/changeset/45874, so we're good to go.

Comment 11 Darin Adler 2009-08-16 22:03:22 PDT

Comment on attachment 32722 [details]
patch that does this for eight classes

Landed as http://trac.webkit.org/changeset/45893

Comment 12 Darin Adler 2009-08-16 22:05:26 PDT

Created attachment 34947 [details]
patch that does this for four more classes

Comment 13 Darin Adler 2009-08-17 00:13:23 PDT

Comment on attachment 34947 [details]
patch that does this for four more classes

Landed as http://trac.webkit.org/changeset/47352

Comment 14 Darin Adler 2010-03-15 09:45:15 PDT

Created attachment 50715 [details]
More of this type of thing

Comment 15 Darin Adler 2018-12-15 15:03:25 PST

I don’t think we need this bug any more. We’ve done lots more of this type of work over the years.