Bug 78196 – Enable MUTATION_OBSERVERS by default on all platforms

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

[Adam Klein]

Enable MUTATION_OBSERVERS by default on all platforms

[Adam Klein]

Created attachment 126222 [details]
Patch

[Adam Klein]

Created attachment 126332 [details]
Patch

[Adam Klein]

Created attachment 126366 [details]
Patch

[Adam Klein]

Created attachment 126369 [details]
Patch

[Adam Klein]

Created attachment 126404 [details]
Patch

[Adam Klein]

Created attachment 126558 [details]
Patch

[Adam Klein]

I think this is now ready for review.

As of http://trac.webkit.org/changeset/107149, all tests under fast/mutation pass on WebKit/Mac except for end-of-task-delivery.html (which fails due to https://bugs.webkit.org/show_bug.cgi?id=78290). I'd like to get the ENABLE flag turned on so that this code won't go stale and we'll be notified of any breakages in it.

[Ojan Vafai]

Comment on attachment 126558 [details]
Patch

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

> LayoutTests/platform/gtk/Skipped:286
> +<<<<<<< HEAD
>  http/tests/workers/terminate-during-sync-operation.html
> +=======
> +fast/mutation/filesystem-callback-delivery.html
> +>>>>>>> Turn flag on by default

um...

[Adam Klein]

Created attachment 126588 [details]
Patch for landing

[Ryosuke Niwa]

But mutation observer doesn't work for editing and drag & drop. Given that RTEs are the major users of the existing mutation events, what good does it do to enable this feature without addressing that use case? As far as I understand it, landing this patch breaks the feature detection on nightly builds.

[Ojan Vafai]

(In reply to comment #10)
> But mutation observer doesn't work for editing and drag & drop. Given that RTEs are the major users of the existing mutation events, what good does it do to enable this feature without addressing that use case? 

You are talking about https://bugs.webkit.org/show_bug.cgi?id=78290, right? While I believe that should be fixed, I don't see any benefit to blocking enabling the feature on that bug. Just because it doesn't address all use-cases without that bug fixed doesn't mean it's not worth enabling.

The good that enabling it does is that it gets the bots running the tests to make sure they don't regress.

> As far as I understand it, landing this patch breaks the feature detection on nightly builds.

What do you mean?

[Ryosuke Niwa]

(In reply to comment #11)
> (In reply to comment #10)
> > But mutation observer doesn't work for editing and drag & drop. Given that RTEs are the major users of the existing mutation events, what good does it do to enable this feature without addressing that use case? 
> 
> You are talking about https://bugs.webkit.org/show_bug.cgi?id=78290, right? While I believe that should be fixed, I don't see any benefit to blocking enabling the feature on that bug.

But that's a very important feature of this API. Without notifications being delivered at right timing, this API becomes much less useful.

>Just because it doesn't address all use-cases without that bug fixed doesn't mean it's not worth enabling. The good that enabling it does is that it gets the bots running the tests to make sure they don't regress.

That's purely a development issue. We can just add a new bot that enables this flag like we did for flexbox and grid layout if we really wanted to have bots.

We had this build flag for months, I don't understand why we want to enable this feature now on non-Chromium ports when there is an obvious major bug we need to fix. If anything, tests would run on Chromium port and would catch any failures there.

> > As far as I understand it, landing this patch breaks the feature detection on nightly builds.
> 
> What do you mean?

By enabling this, we expose prefixed DOM APIs. Any web apps that try to feature-detect this prefixed webkit DOM API will break on non-Chromium WebKit browsers in unexpected ways due to the bug 78290.

[Ojan Vafai]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> >Just because it doesn't address all use-cases without that bug fixed doesn't mean it's not worth enabling. The good that enabling it does is that it gets the bots running the tests to make sure they don't regress.
> 
> That's purely a development issue. We can just add a new bot that enables this flag like we did for flexbox and grid layout if we really wanted to have bots.

The point of the flag IMO is to get the feature to a usable point, not to a finished point, before turning it on. We turned on flexbox way before it was actually finished (it's not finished now!). We never even put Regions behind a flag in the first place! I don't think it's realistic to have a bot per unfinished, but mostly working feature. This is a significant development overhead that just isn't worth it.

> By enabling this, we expose prefixed DOM APIs. Any web apps that try to feature-detect this prefixed webkit DOM API will break on non-Chromium WebKit browsers in unexpected ways due to the bug 78290.

This doesn't seem like a big deal to me. Hopefully someone on the JSC side will have the time to fix bug 78290 in the near future.

[Adam Klein]

To take a step back(In reply to comment #10)
> As far as I understand it, landing this patch breaks the feature detection on nightly builds.

I want to address this concern. There is zero code in the wild using this API. There are no other implementations. What exactly are you worrying will happen if this API is exposed in WebKit nightlies?

[Ryosuke Niwa]

(In reply to comment #14)
> To take a step back(In reply to comment #10)
> > As far as I understand it, landing this patch breaks the feature detection on nightly builds.
> 
> I want to address this concern. There is zero code in the wild using this API. There are no other implementations. What exactly are you worrying will happen if this API is exposed in WebKit nightlies?

People would write code based on either non-Chromium or Chromium behavior using the prefixed version and find out that the feature doesn't work as expected or releases an app/etc... that only works on one or the other.

[Ryosuke Niwa]

(In reply to comment #13)
> (In reply to comment #12)
> > 
> > That's purely a development issue. We can just add a new bot that enables this flag like we did for flexbox and grid layout if we really wanted to have bots.
> 
> The point of the flag IMO is to get the feature to a usable point, not to a finished point, before turning it on. We turned on flexbox way before it was actually finished (it's not finished now!). We never even put Regions behind a flag in the first place! I don't think it's realistic to have a bot per unfinished, but mostly working feature. This is a significant development overhead that just isn't worth it.

I don't think we should be doing that. There was A HUGE complaints from other browser vendors about WebKit shipping half-baked implementations and breaking feature-detection. In fact, some people have claimed that WebKit is the worst offender in this area.

As an example, when we enabled new types for form controls, we didn't initially have useful UIs for most of types. This resulted in completely wrecking feature-detection for the entire Web because it wasn't even prefixed. Now all websites that want to use new types and fallback to JS-based UI have to do a browser-version sniffing.

[Ojan Vafai]

Turning this feature on before it's 100% finished seems clearly inline with how other WebKit development of new features is done. If you have a problem with WebKit's culture around turning on unfinished features, perhaps that generally should be discussed on webkit-dev.

[Ryosuke Niwa]

(In reply to comment #17)
> Turning this feature on before it's 100% finished seems clearly inline with how other WebKit development of new features is done. If you have a problem with WebKit's culture around turning on unfinished features, perhaps that generally should be discussed on webkit-dev.

I disagree but starting a webkit-dev thread seems like a good idea here anyways.

[Ryosuke Niwa]

On my second thought, enabling this on trunk is fine as long as each port can disable it.

[Adam Klein]

(In reply to comment #19)
> On my second thought, enabling this on trunk is fine as long as each port can disable it.

That makes sense to me: it's why I didn't just rip out all the #if ENABLE(MUTATION_OBSERVERS) lines in this change.  I still want to make it easy to turn the feature off (say, for a release branch), just want to flip the default to on.

[WebKit Review Bot]

Comment on attachment 126588 [details]
Patch for landing

Clearing flags on attachment: 126588

Committed r107454: <http://trac.webkit.org/changeset/107454>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.