WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
178280
DOMTokenList shouldn't add empty attributes
https://bugs.webkit.org/show_bug.cgi?id=178280
Summary
DOMTokenList shouldn't add empty attributes
Chris Dumez
Reported
2017-10-13 13:01:01 PDT
DOMTokenList shouldn't add empty attributes after: -
https://github.com/whatwg/dom/pull/488
Firefox and Chrome follow the latest spec.
Attachments
Patch
(10.43 KB, patch)
2017-10-13 13:05 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(1.47 KB, patch)
2017-10-15 13:01 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Chris Dumez
Comment 1
2017-10-13 13:05:19 PDT
Created
attachment 323729
[details]
Patch
Ryosuke Niwa
Comment 2
2017-10-13 13:15:01 PDT
Comment on
attachment 323729
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=323729&action=review
> Source/WebCore/html/DOMTokenList.cpp:252 > + if (!m_element.hasAttribute(m_attributeName) && tokens().isEmpty()) > + return;
It's a bit inefficient to check this and then later call setAttribute since that would find the attribute twice but I guess we don't have a mechanism to update the attributes using Attribute easily.
WebKit Commit Bot
Comment 3
2017-10-13 15:36:31 PDT
Comment on
attachment 323729
[details]
Patch Clearing flags on attachment: 323729 Committed
r223306
: <
https://trac.webkit.org/changeset/223306
>
WebKit Commit Bot
Comment 4
2017-10-13 15:36:33 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 5
2017-10-13 15:37:25 PDT
<
rdar://problem/34987431
>
Darin Adler
Comment 6
2017-10-14 21:32:48 PDT
Comment on
attachment 323729
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=323729&action=review
>> Source/WebCore/html/DOMTokenList.cpp:252 >> + return; > > It's a bit inefficient to check this and then later call setAttribute since that would find the attribute twice > but I guess we don't have a mechanism to update the attributes using Attribute easily.
Because hasAttribute is a hash table lookup it is more expensive, so we should check tokens().isEmpty() first.
Darin Adler
Comment 7
2017-10-14 21:37:41 PDT
Comment on
attachment 323729
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=323729&action=review
>>> Source/WebCore/html/DOMTokenList.cpp:252 >>> + return; >> >> It's a bit inefficient to check this and then later call setAttribute since that would find the attribute twice >> but I guess we don't have a mechanism to update the attributes using Attribute easily. > > Because hasAttribute is a hash table lookup it is more expensive, so we should check tokens().isEmpty() first.
I am not so happy with the specification here: It is inconsistent that an empty token list will replace an *existing* attribute's value with empty string rather than deleting the attribute, but if there is no existing attribute, it will make no change. It would be more logical if it always removed the attribute. If that was the design, for our implementation could simply serialize an empty set of tokens as the null string instead of the empty string and otherwise let the normal algorithm run. I’d really prefer that we make a solution that looks up the attribute only once as Ryosuke says. But sadly there is no variation of setAttribute that will set an existing attribute to empty string but not create a new one. We could create that new function in Element I suppose.
Chris Dumez
Comment 8
2017-10-15 11:29:42 PDT
Reopening for follow-up.
Chris Dumez
Comment 9
2017-10-15 13:01:35 PDT
Created
attachment 323848
[details]
Patch
WebKit Commit Bot
Comment 10
2017-10-15 14:58:46 PDT
Comment on
attachment 323848
[details]
Patch Clearing flags on attachment: 323848 Committed
r223330
: <
https://trac.webkit.org/changeset/223330
>
WebKit Commit Bot
Comment 11
2017-10-15 14:58:47 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug