Last Comment Bug 749920 - Unprefix MozMutationObserver and add a warning about use of mutation events
: Unprefix MozMutationObserver and add a warning about use of mutation events
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla15
Assigned To: Olli Pettay [:smaug]
:
Mentors:
Depends on:
Blocks: 759150
  Show dependency treegraph
 
Reported: 2012-04-28 01:27 PDT by Olli Pettay [:smaug]
Modified: 2013-07-23 12:54 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed


Attachments
patch (20.07 KB, patch)
2012-04-28 02:59 PDT, Olli Pettay [:smaug]
jonas: review+
Details | Diff | Splinter Review
patch for aurora (17.19 KB, patch)
2012-05-26 05:51 PDT, Masatoshi Kimura [:emk]
bugs: review+
akeybl: approval‑mozilla‑aurora-
Details | Diff | Splinter Review
patch for aurora (17.03 KB, patch)
2012-05-30 07:54 PDT, Masatoshi Kimura [:emk]
bugs: review+
lukasblakk+bugs: approval‑mozilla‑aurora-
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2012-04-28 01:27:30 PDT
I think we should try to get rid of mutation events asap, which means we should also have
unprefixed replacement asap.
Not sure if FF15 would be too early, or whether to wait for FF16.
Comment 1 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-28 01:48:49 PDT
Agreed. I think the spec is mature enough to get unprefixed. Also the fact that we now have two independent implementations means that it's unlikely to change.
Comment 3 Ryosuke Niwa 2012-04-29 11:46:39 PDT
Do our implementation (WebKit & Gecko) match exactly?
Comment 4 Olli Pettay [:smaug] 2012-04-29 11:48:25 PDT
I think WebKit misses still some recent spec changes.
I'll test some more next week.
Comment 5 Ryosuke Niwa 2012-04-29 11:52:48 PDT
Okay, let's make sure our implementations match the spec before shipping them unprefixed :)
Comment 6 Ryosuke Niwa 2012-04-29 11:57:01 PDT
I've filed https://bugs.webkit.org/show_bug.cgi?id=85161 to track the WebKit change.
Comment 7 Olli Pettay [:smaug] 2012-05-23 03:35:19 PDT
The version of Chromium (some old v19) I have doesn't seem to handle mutations on DocumentFragment correctly.
Comment 8 Olli Pettay [:smaug] 2012-05-23 03:52:23 PDT
I'll land the patch. Easy to back out if needed, and there is still time to fix possible
minor problems before FF15.
Comment 9 Olli Pettay [:smaug] 2012-05-23 04:59:16 PDT
https://hg.mozilla.org/mozilla-central/rev/e54a85233701
Comment 10 Masatoshi Kimura [:emk] 2012-05-23 07:39:20 PDT
It will be nice if Web authors don't have to care about the Moz prefix from the start.
Comment 11 Masatoshi Kimura [:emk] 2012-05-26 05:51:38 PDT
Created attachment 627474 [details] [diff] [review]
patch for aurora

Removed deprecation warning to avoid string change.
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2012-05-26 09:30:22 PDT
I think it would be super great if we could add a non-localized warning on branch. Getting warnings for mutation event usage is something we should do asap to avoid getting them deployed more now that IE supports them.
Comment 13 Masatoshi Kimura [:emk] 2012-05-26 23:03:56 PDT
Is there an utility function to display non-localized warnings?
Comment 14 Olli Pettay [:smaug] 2012-05-27 02:22:06 PDT
(In reply to Masatoshi Kimura [:emk] from comment #10)
> It will be nice if Web authors don't have to care about the Moz prefix from
> the start.

well, the idea is that there is at least one release where the API is marked somehow experimental, 
or perhaps "comments-are-very-welcome--minor-changes-are-still-possible" would be more accurate.
Comment 15 Jonas Sicking (:sicking) PTO Until July 5th 2012-05-27 12:09:22 PDT
I don't think it'll be meaningfully easier to modify the API just because it's prefixed. Especially given that it'll only be prefixed for 6 weeks and especially given that chrome will have been shipping for a while. But it'll likely mean that developers end up supporting the prefixed version for a long time (we're still not great at migrating people to the latest version of firefox).

A better solution I think would be to add comments in the documentation saying that the API is somewhat experimental and that there's a risk of changes and that we are very interested in getting feedback.
Comment 16 Jonas Sicking (:sicking) PTO Until July 5th 2012-05-27 12:11:34 PDT
There aren't any utility functions for doing non-localized warnings. Actually, it would be great to add that ability to the current WarnOnce infrastructure that we have since this keeps coming up on a pretty regular basis (it recently happened in the unprefix-file.slize bug as well).

If we had the ability to do non-localized warnings through WarnOnce we could do non-localized on branches, and localized on trunk.
Comment 17 Olli Pettay [:smaug] 2012-05-27 12:44:41 PDT
(In reply to Jonas Sicking (:sicking) from comment #15)
> I don't think it'll be meaningfully easier to modify the API just because
> it's prefixed. Especially given that it'll only be prefixed for 6 weeks and
> especially given that chrome will have been shipping for a while.
Chrome hasn't shipped exactly the same API. There have been number of small changes.
takeRecords() is quite big one from API usage point of view.
Comment 18 Jonas Sicking (:sicking) PTO Until July 5th 2012-05-27 13:03:14 PDT
takeRecords is a backwards compatible addition though, no?
Comment 19 Olli Pettay [:smaug] 2012-05-27 13:13:49 PDT
well, at least it doesn't affect rest of the API.

I still think we should have one release with prefix. That is what prefixes are for, when
used the right way. Indicate some new, possibly a bit unstable API, and unprefix asap.
(I hope Opera will have implementation during this summer, and they may give still some feedback.)
Comment 20 Jonas Sicking (:sicking) PTO Until July 5th 2012-05-27 13:48:37 PDT
I don't see the benefit with releasing for 6 weeks with a prefix. The point of prefixing is to give us an opportunity to modify the API if we discover that there are bad things in it, right?

However, in this case that only helps if all of the following holds true:

* Someone discovers a bug in the API within the first 6 weeks of us releasing it.
* We and Google are *more* willing to make backwards incompatible changes due to
  the prefix even though we've both been shipping prefixed versions.
* We are willing to either take those changes on the beta branch, or prefix on the
  beta branch and then make those changes after an even longer time of shipping.


The first bullet point seems unlikely given how slow pickup tends to be. I think Google has been doing much more testing of the API than we're likely to see within the first 6 weeks of deployment.

The second bullet seems unlikely given that most authors write code like:

MutationObserver = MutationObserver || MozMutationObserver ||
                   WebkitMutationObserver;
<use MutationObserver here>.

So modifying prefixed APIs isn't all that different from modifying unprefix.

The last bullet seems unlikely given that we are very conservative about taking things on beta branch, and if we do get feedback, it's more likely to happen later in the 6 weeks rather than sooner.
Comment 21 Olli Pettay [:smaug] 2012-05-28 08:38:35 PDT
Comment on attachment 627474 [details] [diff] [review]
patch for aurora

Well, ok, if you two insist that we should do this...
Comment 22 Masatoshi Kimura [:emk] 2012-05-28 10:27:06 PDT
Comment on attachment 627474 [details] [diff] [review]
patch for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): not a regression.
User impact if declined: Web developers will have to support prefixed version for a long time.
Testing completed (on m-c, etc.): baked on m-c
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None. string changes are removed
Comment 23 Alex Keybl [:akeybl] 2012-05-29 10:36:34 PDT
(In reply to Masatoshi Kimura [:emk] from comment #22)
> [Approval Request Comment]
> User impact if declined: Web developers will have to support prefixed
> version for a long time.

An extra 6 weeks, to be exact.

> String or UUID changes made by this patch: None. string changes are removed

This patch does appear to have an interface change, which we don't typically take on Aurora/Beta.

Please renominate if I'm missing context as to why MozMutationObserver shouldn't just ride the trains.
Comment 24 Masatoshi Kimura [:emk] 2012-05-29 15:43:41 PDT
Ugh, I've overlooked an IID change.
Comment 25 Jonas Sicking (:sicking) PTO Until July 5th 2012-05-30 01:32:03 PDT
Why do we need to change the iid?

Alex: Developers will have to support the prefixed version a lot longer than 6 weeks given how poor we are at getting users to the latest version still. We still need to fix the iid change though I agree.
Comment 26 Masatoshi Kimura [:emk] 2012-05-30 07:54:06 PDT
Created attachment 628337 [details] [diff] [review]
patch for aurora

We can unprefix the interface without iid bump because it is binary-compatible.
Comment 27 Masatoshi Kimura [:emk] 2012-05-30 07:57:17 PDT
Comment on attachment 628337 [details] [diff] [review]
patch for aurora

[Approval Request Comment]
See comment #25.
Also, MozMutationObserver is a new API since Firefox 14 (current aurora). So this "change" will have no negative effect.
Comment 28 Olli Pettay [:smaug] 2012-05-31 13:43:00 PDT
Comment on attachment 628337 [details] [diff] [review]
patch for aurora

Is it guaranteed that binary addon which has been using MozMutationObserver
works with this patch (without need to recompile)?
Comment 29 Masatoshi Kimura [:emk] 2012-05-31 16:54:36 PDT
Yes. binary addons don't refer the name.
Comment 30 Masatoshi Kimura [:emk] 2012-05-31 20:47:19 PDT
A precedent: WebSocket didn't bump the iid when unprefixed.
https://hg.mozilla.org/mozilla-central/rev/2afd7ae68e8b#l3.21
Comment 31 Alex Keybl [:akeybl] 2012-06-01 14:56:00 PDT
Looping in Jorge to get his final opinion on this bug. Just want to double verify comment 29.
Comment 32 Jorge Villalobos [:jorgev] 2012-06-01 16:12:34 PDT
(In reply to Alex Keybl [:akeybl] from comment #31)
> Looping in Jorge to get his final opinion on this bug. Just want to double
> verify comment 29.

This is correct as far as I know, and we don't mind breaking binary compatibility on Aurora. Getting the feature unprefixed from the start definitely gets my vote.
Comment 33 Alex Keybl [:akeybl] 2012-06-04 08:58:01 PDT
Comment on attachment 628337 [details] [diff] [review]
patch for aurora

[Triage Comment]
Approved for Aurora 14. If this doesn't make it onto mozilla-aurora before this afternoon's merge, please re-nominate for approval-mozilla-beta.
Comment 34 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-04 14:12:10 PDT
Comment on attachment 628337 [details] [diff] [review]
patch for aurora

Migration is complete, please nominate for mozilla-beta
Comment 35 Masatoshi Kimura [:emk] 2012-06-04 14:15:18 PDT
Comment on attachment 628337 [details] [diff] [review]
patch for aurora

Requesting beta approval.
Comment 36 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-04 15:28:02 PDT
Comment on attachment 628337 [details] [diff] [review]
patch for aurora

Carrying forward approval to land on beta, now that we have migrated. Please land as soon as possible so we can get this in the first beta which will be going to build tomorrow.
Comment 37 Andrew McCreight [:mccr8] 2012-06-04 16:34:01 PDT
I messed up the review flag on this patch, but oh well.

https://hg.mozilla.org/releases/mozilla-beta/rev/4698044001a2
Comment 38 John Nagle 2012-06-20 21:34:19 PDT
Please hold off on removing mutation events until mutation observers definitely work. 

I've filed one bug report already: 

https://bugzilla.mozilla.org/show_bug.cgi?id=765837

I suspect further problems in using mutation observers in content scripts, but I'm not yet ready to file a bug report on that. Where's the test case for mutation observers in content scripts?
Comment 39 Olli Pettay [:smaug] 2012-06-21 01:24:57 PDT
The error which
https://bugzilla.mozilla.org/show_bug.cgi?id=765837 is about doesn't seem to have anything to
do with MutationObserver.

But anyhow, removing mutation events won't happen anytime soon. Too many websites rely on them.
Comment 40 Yuhong Bao 2013-07-23 12:54:29 PDT
Sadly IE10 also do not support mutation observers. IE11 finally supports them, but I expect the effects of IE9/10 not supporting them to persist long after they become obsolete.

Note You need to log in before you can comment on or make changes to this bug.