Bug 74448 - Broaden support for mutation observation of attributes
Summary: Broaden support for mutation observation of attributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
Depends on:
Blocks: 68729
  Show dependency treegraph
 
Reported: 2011-12-13 14:18 PST by Adam Klein
Modified: 2011-12-14 13:14 PST (History)
6 users (show)

See Also:


Attachments
Patch (20.24 KB, patch)
2011-12-13 14:26 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (19.59 KB, patch)
2011-12-14 12:23 PST, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2011-12-13 14:18:23 PST
Broaden support for mutation observation of attributes
Comment 1 Adam Klein 2011-12-13 14:26:06 PST
Created attachment 119085 [details]
Patch
Comment 2 Adam Klein 2011-12-13 14:35:55 PST
Note to reviewer(s): the name "enqueueAttributesMutationRecordIfRequested" is rather...unfortunate.  One possible fix is to rename it Element::willMutateAttribute, let me know if you'd refer that more general naming scheme.
Comment 3 Ryosuke Niwa 2011-12-14 01:36:36 PST
Comment on attachment 119085 [details]
Patch

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

> Source/WebCore/ChangeLog:25
> +        (WebCore::Attr::setValue): Enqueue a mutation record when Attr.value is set from JS.
> +        (WebCore::Attr::childrenChanged): Enqueue a mutation record when an Attr's value changes to due additions/removals of Text children.
> +        * dom/Element.cpp:
> +        (WebCore::Element::enqueueAttributesMutationRecordIfRequested): Previously a static, expose as part of Element's interface to allow it to be re-used by NamedNodeMap and Attr.
> +        (WebCore::Element::removeAttribute): Remove enqueue call now handled by NamedNodeMap.
> +        (WebCore::Element::setAttributeInternal): Fixup call of enqueueAttributesMutationRecordIfRequested.
> +        * dom/Element.h:
> +        * dom/NamedNodeMap.cpp:
> +        (WebCore::NamedNodeMap::setNamedItem): Enqueue a mutation record when an attribute is changed via Element.attributes.setNamedItem from JS.
> +        (WebCore::NamedNodeMap::removeNamedItem): Enqueue a mutation record when an attribute is removed, either via Element.attributes.removeNamedItem or Element.removeAttribute.

Please line-break as needed. These lines are extremely long. Doesn't even fit on my high-resolution 15" screen :(

> LayoutTests/fast/mutation/observe-attributes.html:-9
> -<html>
> -<head>
> -<meta charset="utf-8">
>  <script src="../js/resources/js-test-pre.js"></script>
> -</head>
> -<body>
> -<p id=description></p>
> -<div id="console"></div>

Why are we removing these nodes?
Comment 4 Ojan Vafai 2011-12-14 09:18:09 PST
Comment on attachment 119085 [details]
Patch

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

>> LayoutTests/fast/mutation/observe-attributes.html:-9
>> -<div id="console"></div>
> 
> Why are we removing these nodes?

I've asked Rafael to remove these in other reviews. They don't affect the test in anyways, so they just add more noise.
Comment 5 Ryosuke Niwa 2011-12-14 11:14:30 PST
Comment on attachment 119085 [details]
Patch

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

>>> LayoutTests/fast/mutation/observe-attributes.html:-9
>>> -<div id="console"></div>
>> 
>> Why are we removing these nodes?
> 
> I've asked Rafael to remove these in other reviews. They don't affect the test in anyways, so they just add more noise.

But they're auto-generated by the script, right? Also, not having body isn't desirable at times although it works in this test.
Comment 6 Adam Klein 2011-12-14 11:50:36 PST
Comment on attachment 119085 [details]
Patch

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

>> Source/WebCore/ChangeLog:25
>> +        (WebCore::NamedNodeMap::removeNamedItem): Enqueue a mutation record when an attribute is removed, either via Element.attributes.removeNamedItem or Element.removeAttribute.
> 
> Please line-break as needed. These lines are extremely long. Doesn't even fit on my high-resolution 15" screen :(

Will break on submit. I feel like I heard at some point that breaking these lines screwed up trac's formatting, but I must be misremembering.
Comment 7 Adam Klein 2011-12-14 12:23:51 PST
Created attachment 119275 [details]
Patch for landing
Comment 8 WebKit Review Bot 2011-12-14 13:14:33 PST
Comment on attachment 119275 [details]
Patch for landing

Clearing flags on attachment: 119275

Committed r102814: <http://trac.webkit.org/changeset/102814>
Comment 9 WebKit Review Bot 2011-12-14 13:14:38 PST
All reviewed patches have been landed.  Closing bug.