Remove ENABLE_MUTATION_OBSERVERS #define
Created attachment 180223 [details] Patch
Comment on attachment 180223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180223&action=review > Source/WebCore/ChangeLog:12 > + The only place it was disabled was under "unstable" features for the > + gtk port, but the fact that it's shipping in several major WebKit Is it also enabled on Qt, GTK+, & EFL ports?
Also, I’m worried about the end of micro-task thing each browser has to implement. If we enable the feature by default, we might end up letting vendors ship WebKit browsers without fixing that problem. Worse yet, they may not even realize that they have to do something about it. I’ve started to think that we might just want to add a hack for editing as we discussed before since editing is the only place where this matters in practice.
(In reply to comment #2) > (From update of attachment 180223 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180223&action=review > > > Source/WebCore/ChangeLog:12 > > + The only place it was disabled was under "unstable" features for the > > + gtk port, but the fact that it's shipping in several major WebKit > > Is it also enabled on Qt, GTK+, & EFL ports? Yes, it's enabled by default on qt, gtk, and efl.
(In reply to comment #3) > Also, I’m worried about the end of micro-task thing each browser has to implement. If we enable the feature by default, we might end up letting vendors ship WebKit browsers without fixing that problem. Worse yet, they may not even realize that they have to do something about it. Well, it's already shipped in Safari 6 and I believe Mobile Safari on iOS 6. So I think that ship has already sailed. > I’ve started to think that we might just want to add a hack for editing as we discussed before since editing is the only place where this matters in practice. I still think that's a reasonable approach, if it can be hooked properly. Given that it's already shipping, though, I don't think that this work should block removing the enable flag.
(In reply to comment #5) > (In reply to comment #3) > > Also, I’m worried about the end of micro-task thing each browser has to implement. If we enable the feature by default, we might end up letting vendors ship WebKit browsers without fixing that problem. Worse yet, they may not even realize that they have to do something about it. > > Well, it's already shipped in Safari 6 and I believe Mobile Safari on iOS 6. So I think that ship has already sailed. It’s prefixed. I don’t think we want to keep shipping the broken API if we’re trying to unprefix it. > > I’ve started to think that we might just want to add a hack for editing as we discussed before since editing is the only place where this matters in practice. > > I still think that's a reasonable approach, if it can be hooked properly. Given that it's already shipping, though, I don't think that this work should block removing the enable flag. I tend to agree but we should really fix this before unprefixing the API. Also, it’ll be nice of you if you announced it on webkit-dev just in case some port that was planning to disable it for their release branches.
s/some port that was/some port was/
Created attachment 180226 [details] Patch
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #3) > > > Also, I’m worried about the end of micro-task thing each browser has to implement. If we enable the feature by default, we might end up letting vendors ship WebKit browsers without fixing that problem. Worse yet, they may not even realize that they have to do something about it. > > > > Well, it's already shipped in Safari 6 and I believe Mobile Safari on iOS 6. So I think that ship has already sailed. > > It’s prefixed. I don’t think we want to keep shipping the broken API if we’re trying to unprefix it. > > > > I’ve started to think that we might just want to add a hack for editing as we discussed before since editing is the only place where this matters in practice. > > > > I still think that's a reasonable approach, if it can be hooked properly. Given that it's already shipping, though, I don't think that this work should block removing the enable flag. > > I tend to agree but we should really fix this before unprefixing the API. Also, it’ll be nice of you if you announced it on webkit-dev just in case some port that was planning to disable it for their release branches. Sure, prefixing and the feature flag seem like separate issues to me, especially at this point in time. I'm happy to send a note to webkit-dev (and note that I CCed mrobinson on this bug for input from a gtk port maintainer).
Comment on attachment 180226 [details] Patch r=me assuming it builds on all ports & nobody objects to it.
(empty, so far) webkit-dev thread: http://lists.webkit.org/pipermail/webkit-dev/2012-December/023181.html
Apparently MUTATION_OBSERVERS are now disabled by default on GTK, after http://trac.webkit.org/changeset/138252. Can someone responsible for the GTK port (mrobinson?) comment on whether it's OK to enable it as part fo this patch? According to TestExpectations (and the bots) the mutation observer tests pass under GTK.
(In reply to comment #12) > Apparently MUTATION_OBSERVERS are now disabled by default on GTK, after http://trac.webkit.org/changeset/138252. > > Can someone responsible for the GTK port (mrobinson?) comment on whether it's OK to enable it as part fo this patch? According to TestExpectations (and the bots) the mutation observer tests pass under GTK. If it's enabled by default on stable versions of iOS and Chromium browsers, I think it's safe to enable it for GTK+ as well. If there's any missing functionality that we need to implement, feel free to open a bug and I'm sure someone will tackle it in short order. Thanks for considering us!
(In reply to comment #13) > (In reply to comment #12) > > Apparently MUTATION_OBSERVERS are now disabled by default on GTK, after http://trac.webkit.org/changeset/138252. > > > > Can someone responsible for the GTK port (mrobinson?) comment on whether it's OK to enable it as part fo this patch? According to TestExpectations (and the bots) the mutation observer tests pass under GTK. > > If it's enabled by default on stable versions of iOS and Chromium browsers, I think it's safe to enable it for GTK+ as well. If there's any missing functionality that we need to implement, feel free to open a bug and I'm sure someone will tackle it in short order. Thanks for considering us! The only missing functionality is discussed in bug 78290. Basically, mutation observer delivery isn't timed properly when triggered by editing code (this is what rniwa is referring to in the earlier comments on this bug).
(In reply to comment #4) > (In reply to comment #2) > > Is it also enabled on Qt, GTK+, & EFL ports? > > Yes, it's enabled by default on qt, gtk, and efl. Sorry for late reply. CMake ports (EFL, blackberry and WinCE) have enabled this feature. So, I think there is no problem to remove this macro.
Created attachment 181250 [details] Patch for landing
The BlackBerry change looks fine, thanks for putting it into your patch!
Comment on attachment 181250 [details] Patch for landing Clearing flags on attachment: 181250 Committed r138811: <http://trac.webkit.org/changeset/138811>
All reviewed patches have been landed. Closing bug.