Mozilla's implementation of MutationObservers queues MutationRecords for DOM operations executed by the parser, not just those initiated through DOM APIs and editing. Per http://lists.w3.org/Archives/Public/public-webapps/2012AprJun/1198.html, it seems likely that we'll want to change WebKit to have the same behavior to match Firefox.
I think this is probably the right API decision. It's not clear to me when we deliver the records though. Do we deliver them after each time the parser yields? If so, is that exposing too much implementation detail? Do we need to define when mutations are delivered during parsing?
Please be sure to test the performance impact of your change using the html-parser.html PerformanceTest.
I think it would make the most sense to deliver them when the parser yields, yes. Presumably just before any other script runs? The parser may yield at any arbitrary time, so it seems it would be difficult for someone to end up depending on WebKit's delivery behavior (but given how large the interblägs are, I would not be surprised when some page does...).
Whatever we decide to do here, it should be resolved before unprefixing since it amounts to a fairly noticeable difference with Mozilla's unprefixed version.
Note that this has been added to the HTML spec in http://html5.org/r/7484
I was looking at this recently and my plan is: Add a ChildListMutationScope to ContainerNode::parserAppendChild, parserInsertBefore and parserRemoveChild. Add the logic from Element::willModifyAttribute for enqueuing mutation records to Element::parserSetAttributes. Call MutationObserver::deliverAllMutations() somewhere in HTMLScriptRunner. HTMLScriptRunner is really confusing to me and it doesn't seem to have a single entry point for all scripts so that'll require more research.
(In reply to comment #6) > I was looking at this recently and my plan is: Excellent, if we can get something landed here I think we can (finally) safely drop the prefix and likely drop the ENABLE flag as well. > Add a ChildListMutationScope to ContainerNode::parserAppendChild, parserInsertBefore and parserRemoveChild. This sounds like a good first step, but may result in very noisy notifications. There's another approach that more directly involves the parser in keeping something like a ChildListMutationScope (I've got a hacked up beginnings of this in a git client somewhere, and some whiteboard-drawing can probably better explain what I mean). But I think it's probably worth starting with your suggested approach and improving over time (so at least we're spec compliant before we start optimizing). > Add the logic from Element::willModifyAttribute for enqueuing mutation records to Element::parserSetAttributes. I'm not sure this is necessary: do we ever call parserSetAttributes after an Element is in the tree? > Call MutationObserver::deliverAllMutations() somewhere in HTMLScriptRunner. > > HTMLScriptRunner is really confusing to me and it doesn't seem to have a single entry point for all scripts so that'll require more research.
(In reply to comment #7) > (In reply to comment #6) > > I was looking at this recently and my plan is: > > Excellent, if we can get something landed here I think we can (finally) safely drop the prefix and likely drop the ENABLE flag as well. > > > Add a ChildListMutationScope to ContainerNode::parserAppendChild, parserInsertBefore and parserRemoveChild. > > This sounds like a good first step, but may result in very noisy notifications. There's another approach that more directly involves the parser in keeping something like a ChildListMutationScope (I've got a hacked up beginnings of this in a git client somewhere, and some whiteboard-drawing can probably better explain what I mean). But I think it's probably worth starting with your suggested approach and improving over time (so at least we're spec compliant before we start optimizing). Okay. > > Add the logic from Element::willModifyAttribute for enqueuing mutation records to Element::parserSetAttributes. > > I'm not sure this is necessary: do we ever call parserSetAttributes after an Element is in the tree? Nope, you're right this isn't needed. In parserSetAttributes we do assert about it: ASSERT(!inDocument()); ASSERT(!parentNode());
That sounds like a good approach. One design issue to look out for here is that we'll likely want to move the HTML parser to a background thread at some point (maybe even soon). Watching for these mutations in the parser* functions should work well in that future.
Created attachment 180581 [details] WIP
(In reply to comment #10) > Created an attachment (id=180581) [details] > Patch This patch needs more tests for insertBefore and removeChild inside the parser, and also we need to figure out when the mutations are really supposed to be delivered. I can't find reference about all this in the spec.
Comment on attachment 180581 [details] WIP I assuem you tested the various parser benchmarks (or plan to?) :)
(In reply to comment #12) > (From update of attachment 180581 [details]) > I assuem you tested the various parser benchmarks (or plan to?) :) I plan to run the html5-full-parse test.
Comment on attachment 180581 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=180581&action=review The change looks OK. I just worry about perf, and would like to see the perf numbers before approving. :) > Source/WebCore/html/parser/HTMLScriptRunner.cpp:297 > + // FIXME: This may be too agressive as we always deliver mutations at > + // every script element, even if it's not ready to execute yet. There's > + // unfortuantely no obvious way to tell if prepareScript is going to > + // execute the script from out here. You certainly can tell if it *wont* execute the script. :) It's harder to tell if it will, since you could technically be racing with the loader. But detecting synchronously-loaded URLs, or cached-scripts which are ready to execute is doable, and I believe part of that is already done in this code, as we detect if a script is ready to execute somewhere...
(In reply to comment #14) > (From update of attachment 180581 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180581&action=review > > ... > > You certainly can tell if it *wont* execute the script. :) It's harder to tell if it will, since you could technically be racing with the loader. But detecting synchronously-loaded URLs, or cached-scripts which are ready to execute is doable, and I believe part of that is already done in this code, as we detect if a script is ready to execute somewhere... Hmm, I couldn't find a trivial way without duplicating the big logic tree in prepareScript to know if it won't execute script. How would you detect this?
I don't think we ought to block this patch on the script execution question. It still seems a strict improvement to just queue the records during parse; whether we then deliver them too often or not often enough can be dealt with as a followup.
(In reply to comment #16) > I don't think we ought to block this patch on the script execution question. It still seems a strict improvement to just queue the records during parse; whether we then deliver them too often or not often enough can be dealt with as a followup. I ran the perf tests and I don't see any regressions.
Comment on attachment 180581 [details] WIP I'm surprised this has no perf impact. But I guess ojan's hawk-like survey of the perf bots will confirm your testing. If you update the patch you shoudl consider metnioning your perf testing in teh ChangeLog.
(In reply to comment #18) > (From update of attachment 180581 [details]) > I'm surprised this has no perf impact. But I guess ojan's hawk-like survey of the perf bots will confirm your testing. If you update the patch you shoudl consider metnioning your perf testing in teh ChangeLog. My understanding is that adamk already perf tuned the ChildListMutationScope so that's it's super fast if there's no mutation observers present so they didn't regress Node::appendChild, insertBefore and removeChild (re: Dromaeo). :) I'll update the ChangeLog before submitting.
Just to be clear, Elliot: you are saying there is no perf impact when the are no observers registered, right?
(In reply to comment #20) > Just to be clear, Elliot: you are saying there is no perf impact when the are no observers registered, right? Correct, we don't have a PerformanceTests/Parser test with MutationObserver's to my knowledge.
> Correct, we don't have a PerformanceTests/Parser test with MutationObserver's to my knowledge. I'd be curious to know what the perf impact is in the case when mutation observers do exist, but that doesn't need to hold up this patch.
We have tests in PerformanceTests/Mutation. You can manually run them by passing --force to run-pert-tests.
Comment on attachment 180581 [details] WIP Clearing flags on attachment: 180581 Committed r139790: <http://trac.webkit.org/changeset/139790>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 106948
(In reply to comment #26) > Re-opened since this is blocked by bug 106948 Even tough this went through the commit queue it seems the test consistently fails on all the bots and also locally for leviw. :( Looking into what happened...
(In reply to comment #27) > (In reply to comment #26) > > Re-opened since this is blocked by bug 106948 > > Even tough this went through the commit queue it seems the test consistently fails on all the bots and also locally for leviw. :( Looking into what happened... Any idea what went wrong with this? Test looks super-simple...
(In reply to comment #28) > (In reply to comment #27) > > (In reply to comment #26) > > > Re-opened since this is blocked by bug 106948 > > > > Even tough this went through the commit queue it seems the test consistently fails on all the bots and also locally for leviw. :( Looking into what happened... > > Any idea what went wrong with this? Test looks super-simple... Discussed this with adamk and we're going to update this patch to use takeRecords() and reland it, then solve delivery on script execution in another patch as that seems to be the problem.
So funny thing here, the reason this broke was because the MUTATION_OBSERVERS flag was removed and my code was ifdef'ed out so the patch was fine, all the code I added was just turned off. Unfortunately it took me a whole day to realize that none of my code actually runs :(
Created attachment 186147 [details] Patch
Created attachment 186148 [details] Patch
Comment on attachment 186148 [details] Patch Functionally looks fine. Do I need to ask perf questions? :)
(In reply to comment #33) > (From update of attachment 186148 [details]) > Functionally looks fine. Do I need to ask perf questions? :) Nah, perf is the same as before when I checked and it was fine. I do want to add more tests before we land this though!
Comment on attachment 186148 [details] Patch OK.
Created attachment 186956 [details] Patch
Created attachment 187183 [details] Patch
Comment on attachment 187183 [details] Patch Rejecting attachment 187183 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'validate-changelog', '--non-interactive', 187183, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/16438370
Created attachment 187192 [details] Patch for landing
Comment on attachment 187192 [details] Patch for landing Clearing flags on attachment: 187192 Committed r142204: <http://trac.webkit.org/changeset/142204>