Bug 74991

Summary: Automate elements' registration as document namedItem/extraNamedItem.
Product: WebKit Reporter: Andreas Kling <kling>
Component: DOMAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, koivisto, rakuco, tkent, webkit.review.bot, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch
none
Proposed patch
koivisto: review-
Proposed patch v2
none
Proposed patch v3 koivisto: review+, webkit.review.bot: commit-queue-

Description Andreas Kling 2011-12-20 21:25:02 PST
Some of our HTML elements (applet, embed, form, iframe, image and object) track their "name" and "id" attributes in order to keep the "named item" and "extra named item" maps in HTMLDocument in sync.

We could do this automatically by hooking the relevant attribute changes and get rid of some duplicated code (plus those elements will shrink by two AtomicStrings each.)

Rough patch idea coming..
Comment 1 Andreas Kling 2011-12-20 21:38:09 PST
Created attachment 120139 [details]
Proposed patch

A first stab at this.
Comment 2 Andreas Kling 2011-12-20 21:42:16 PST
Created attachment 120140 [details]
Proposed patch
Comment 3 Antti Koivisto 2011-12-21 11:12:54 PST
Comment on attachment 120140 [details]
Proposed patch

Looks good.
Comment 4 Antti Koivisto 2011-12-21 11:21:23 PST
Comment on attachment 120140 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120140&action=review

r- for now for perf concern.

> Source/WebCore/html/HTMLElement.cpp:1005
> +void HTMLElement::insertedIntoDocument()
> +{
> +    StyledElement::insertedIntoDocument();
> +
> +    const AtomicString& name = fastGetAttribute(HTMLNames::nameAttr);
> +    if (!name.isNull())
> +        updateName(nullAtom, name);
> +}

Hmm, adding attribute lookup here might be slow. Is there a way to avoid this?
Comment 5 Andreas Kling 2011-12-21 12:42:57 PST
Created attachment 120214 [details]
Proposed patch v2

Perf concern addressed by using a Node bit to cache whether we have a name attribute or not (just like we already did for id attributes.)
Comment 6 WebKit Review Bot 2011-12-21 12:54:04 PST
Attachment 120214 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'CMakeLists.txt', u'ChangeLog', u'LayoutTes..." exit_code: 1

ERROR: FAILURES FOR <lucid, x86_64, release, cpu> in LayoutTests/platform/chromium/test_expectations.txt
ERROR: Line:192 Path does not exist. http/tests/workers/interrupt-database-sync-open-crash.html
LayoutTests/platform/chromium/test_expectations.txt:192:  Path does not exist. http/tests/workers/interrupt-database-sync-open-crash.html  [test/expectations] [2]
Total errors found: 1 in 62 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Antti Koivisto 2011-12-21 14:04:48 PST
Comment on attachment 120214 [details]
Proposed patch v2

r=me
Comment 8 Andreas Kling 2011-12-21 16:25:31 PST
Comment on attachment 120214 [details]
Proposed patch v2

Clearing flags on attachment: 120214

Committed r103473: <http://trac.webkit.org/changeset/103473>
Comment 9 Andreas Kling 2011-12-21 16:25:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Andreas Kling 2011-12-21 17:36:58 PST
Reverted r103473 for reason:

Overestimated my superpowers.

Committed r103478: <http://trac.webkit.org/changeset/103478>
Comment 11 Andreas Kling 2011-12-21 21:11:59 PST
Created attachment 120265 [details]
Proposed patch v3

Same patch with the setHasName() moved to StyledElement::attributeChanged() to ensure that the hasID flag always gets set.
Comment 12 WebKit Review Bot 2011-12-21 21:14:02 PST
Attachment 120265 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:30:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 WebKit Review Bot 2011-12-23 09:36:01 PST
Comment on attachment 120265 [details]
Proposed patch v3

Rejecting attachment 120265 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
file Source/WebCore/html/HTMLFormElement.cpp
patching file Source/WebCore/html/HTMLFormElement.h
patching file Source/WebCore/html/HTMLImageElement.cpp
patching file Source/WebCore/html/HTMLImageElement.h
patching file Source/WebCore/html/HTMLObjectElement.cpp
patching file Source/WebCore/html/HTMLObjectElement.h
patching file Source/WebCore/html/HTMLPlugInElement.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Antti Koivisto', u'--f..." exit_code: 1

Full output: http://queues.webkit.org/results/10955394
Comment 14 Andreas Kling 2011-12-23 13:37:14 PST
Committed r103638: <http://trac.webkit.org/changeset/103638>