Bug 105459 – Remove ENABLE_MUTATION_OBSERVERS #define

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

[Adam Klein]

Remove ENABLE_MUTATION_OBSERVERS #define

[Adam Klein]

Created attachment 180223 [details]
Patch

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

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.

[Adam Klein]

(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.

[Adam Klein]

(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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

s/some port that was/some port was/

[Adam Klein]

Created attachment 180226 [details]
Patch

[Adam Klein]

(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).

[Ryosuke Niwa]

Comment on attachment 180226 [details]
Patch

r=me assuming it builds on all ports & nobody objects to it.

[Adam Klein]

(empty, so far) webkit-dev thread: http://lists.webkit.org/pipermail/webkit-dev/2012-December/023181.html

[Adam Klein]

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.

[Martin Robinson]

(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!

[Adam Klein]

(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).

[Gyuyoung Kim]

(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.

[Adam Klein]

Created attachment 181250 [details]
Patch for landing

[Rob Buis]

The BlackBerry change looks fine, thanks for putting it into your patch!

[Rob Buis]

The BlackBerry change looks fine, thanks for putting it into your patch!

[WebKit Review Bot]

Comment on attachment 181250 [details]
Patch for landing

Clearing flags on attachment: 181250

Committed r138811: <http://trac.webkit.org/changeset/138811>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.