Bug 74752 – Consolidate before-advice regarding attribute modification into a single method

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Klein]

Consolidate before-advice regarding attribute modification into a single method

[Adam Klein]

Created attachment 119680 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 119680 [details]
Patch

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

> Source/WebCore/dom/Attr.cpp:182
> +            valueBuilder.append(static_cast<Text *>(n)->data());

There should be no space between Text and *.

> Source/WebCore/dom/Attr.cpp:185
> +    AtomicString value = valueBuilder.toString();

We should probably call this variable newValue for clarity.

> Source/WebCore/dom/NamedNodeMap.cpp:146
> +    if (!attribute || !m_element) {

Can we have a test for this change?

[Adam Klein]

Comment on attachment 119680 [details]
Patch

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

>> Source/WebCore/dom/Attr.cpp:182
>> +            valueBuilder.append(static_cast<Text *>(n)->data());
> 
> There should be no space between Text and *.

Done.

>> Source/WebCore/dom/Attr.cpp:185
>> +    AtomicString value = valueBuilder.toString();
> 
> We should probably call this variable newValue for clarity.

Done.

>> Source/WebCore/dom/NamedNodeMap.cpp:146
>> +    if (!attribute || !m_element) {
> 
> Can we have a test for this change?

It's not possible to get a reference to a NamedNodeMap whose element is null from JavaScript, since the NamedNodeMap wrapper holds a (hidden) reference back to the element.  See NamedNodeMap::detachFromElement() for more discussion.

Adding this null check makes it clear why it's safe to call a method on m_element below, and is symmetrical with the check at the top of setNamedItem().  Would you rather I replace this with an ASSERT(m_element) instead?

[Adam Klein]

Created attachment 119698 [details]
Resolved conflicts with trunk

[Ryosuke Niwa]

Comment on attachment 119680 [details]
Patch

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

>>> Source/WebCore/dom/NamedNodeMap.cpp:146
>>> +    if (!attribute || !m_element) {
>> 
>> Can we have a test for this change?
> 
> It's not possible to get a reference to a NamedNodeMap whose element is null from JavaScript, since the NamedNodeMap wrapper holds a (hidden) reference back to the element.  See NamedNodeMap::detachFromElement() for more discussion.
> 
> Adding this null check makes it clear why it's safe to call a method on m_element below, and is symmetrical with the check at the top of setNamedItem().  Would you rather I replace this with an ASSERT(m_element) instead?

I'd rather see ASSERT(m_element).

[Ryosuke Niwa]

Comment on attachment 119698 [details]
Resolved conflicts with trunk

r=me provided the change in removeNamedItem is replaced by an assertion.

[Adam Klein]

Created attachment 119709 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 119709 [details]
Patch for landing

Clearing flags on attachment: 119709

Committed r103140: <http://trac.webkit.org/changeset/103140>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.