Bug 78196 - Enable MUTATION_OBSERVERS by default on all platforms
Summary: Enable MUTATION_OBSERVERS by default on all platforms
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
Depends on:
Blocks: 68729
  Show dependency treegraph
 
Reported: 2012-02-08 19:09 PST by Adam Klein
Modified: 2012-02-10 16:07 PST (History)
9 users (show)

See Also:


Attachments
Patch (10.81 KB, patch)
2012-02-08 19:09 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (12.40 KB, patch)
2012-02-09 11:10 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (14.35 KB, patch)
2012-02-09 13:49 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (14.36 KB, patch)
2012-02-09 14:15 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (21.78 KB, patch)
2012-02-09 16:48 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (23.00 KB, patch)
2012-02-10 12:37 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (22.89 KB, patch)
2012-02-10 14:54 PST, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2012-02-08 19:09:08 PST
Enable MUTATION_OBSERVERS by default on all platforms
Comment 1 Adam Klein 2012-02-08 19:09:30 PST
Created attachment 126222 [details]
Patch
Comment 2 Adam Klein 2012-02-09 11:10:52 PST
Created attachment 126332 [details]
Patch
Comment 3 Adam Klein 2012-02-09 13:49:07 PST
Created attachment 126366 [details]
Patch
Comment 4 Adam Klein 2012-02-09 14:15:03 PST
Created attachment 126369 [details]
Patch
Comment 5 Adam Klein 2012-02-09 16:48:04 PST
Created attachment 126404 [details]
Patch
Comment 6 Adam Klein 2012-02-10 12:37:03 PST
Created attachment 126558 [details]
Patch
Comment 7 Adam Klein 2012-02-10 14:39:16 PST
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.
Comment 8 Ojan Vafai 2012-02-10 14:48:07 PST
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...
Comment 9 Adam Klein 2012-02-10 14:54:30 PST
Created attachment 126588 [details]
Patch for landing
Comment 10 Ryosuke Niwa 2012-02-10 15:06:41 PST
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.
Comment 11 Ojan Vafai 2012-02-10 15:11:21 PST
(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?
Comment 12 Ryosuke Niwa 2012-02-10 15:18:33 PST
(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.
Comment 13 Ojan Vafai 2012-02-10 15:30:38 PST
(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.
Comment 14 Adam Klein 2012-02-10 15:34:40 PST
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?
Comment 15 Ryosuke Niwa 2012-02-10 15:41:17 PST
(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.
Comment 16 Ryosuke Niwa 2012-02-10 15:43:45 PST
(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.
Comment 17 Ojan Vafai 2012-02-10 15:46:13 PST
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.
Comment 18 Ryosuke Niwa 2012-02-10 15:49:39 PST
(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.
Comment 19 Ryosuke Niwa 2012-02-10 15:52:55 PST
On my second thought, enabling this on trunk is fine as long as each port can disable it.
Comment 20 Adam Klein 2012-02-10 15:54:53 PST
(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.
Comment 21 WebKit Review Bot 2012-02-10 16:07:11 PST
Comment on attachment 126588 [details]
Patch for landing

Clearing flags on attachment: 126588

Committed r107454: <http://trac.webkit.org/changeset/107454>
Comment 22 WebKit Review Bot 2012-02-10 16:07:21 PST
All reviewed patches have been landed.  Closing bug.