Bug 83218 - [MutationObservers] implement takeRecords()
Summary: [MutationObservers] implement takeRecords()
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rafael Weinstein
Depends on:
Blocks: 68729
  Show dependency treegraph
Reported: 2012-04-04 14:28 PDT by Rafael Weinstein
Modified: 2012-04-05 11:02 PDT (History)
7 users (show)

See Also:

Patch (7.63 KB, patch)
2012-04-04 14:31 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (7.62 KB, patch)
2012-04-04 15:55 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rafael Weinstein 2012-04-04 14:28:00 PDT
Per https://www.w3.org/Bugs/Public/show_bug.cgi?id=16606, MutationObserver should expose a takeRecords() call which synchronously retrieves & clears any pending mutation records. If none are pending, it should return an empty array. It should not do anything else (in particular, it should not clear transient observers).

Anne has committed to updating the spec by end of week.
Comment 1 Rafael Weinstein 2012-04-04 14:31:12 PDT
Created attachment 135685 [details]
Comment 2 Adam Klein 2012-04-04 14:40:22 PDT
Comment on attachment 135685 [details]

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

> Source/WebCore/dom/WebKitMutationObserver.h:36
> +#include "MutationCallback.h"

No need for this include...

> Source/WebCore/dom/WebKitMutationObserver.h:82
> +    MutationRecordArray takeRecords();

...or this use of the MutationRecordArray typedef. That's only necessary to deal with the way Callbacks are handled by WebKit's IDL processing. I'd make this the same return type name as m_records, i.e., Vector<RefPtr<MutationRecord> >.

Alternatively, I suppose you could use the typedef as the type of m_records as well, if you think that type name's too much to be repeating.

> Source/WebCore/dom/WebKitMutationObserver.idl:39
> +        sequence<MutationRecord> takeRecords();

Awesome, no custom code?

> LayoutTests/fast/mutation/observe-subtree.html:59
> +function testTakeRecords() {

I'd rather see this test in its own file for clarity: it's testing the takeRecords method, not subtree observation.
Comment 3 Ojan Vafai 2012-04-04 15:01:42 PDT
Comment on attachment 135685 [details]

r=me once you address Adam's comments.
Comment 4 Build Bot 2012-04-04 15:10:11 PDT
Comment on attachment 135685 [details]

Attachment 135685 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12331064
Comment 5 Adam Klein 2012-04-04 15:12:03 PDT
(In reply to comment #4)
> (From update of attachment 135685 [details])
> Attachment 135685 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/12331064

As a bonus, not using the typedef (and thus the #include of MutationCallback.h) should make this error go away.
Comment 6 Rafael Weinstein 2012-04-04 15:55:02 PDT
Created attachment 135705 [details]
Comment 7 WebKit Review Bot 2012-04-04 19:50:43 PDT
Comment on attachment 135705 [details]

Clearing flags on attachment: 135705

Committed r113279: <http://trac.webkit.org/changeset/113279>
Comment 8 WebKit Review Bot 2012-04-04 19:50:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Simon Fraser (smfr) 2012-04-05 11:02:28 PDT
This broke the window-properties.html test: