Closed Bug 749920 Opened 12 years ago Closed 12 years ago

Unprefix MozMutationObserver and add a warning about use of mutation events

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox14 + fixed
firefox15 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 1 obsolete file)

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.
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.
Do our implementation (WebKit & Gecko) match exactly?
I think WebKit misses still some recent spec changes.
I'll test some more next week.
Okay, let's make sure our implementations match the spec before shipping them unprefixed :)
I've filed https://bugs.webkit.org/show_bug.cgi?id=85161 to track the WebKit change.
The version of Chromium (some old v19) I have doesn't seem to handle mutations on DocumentFragment correctly.
I'll land the patch. Easy to back out if needed, and there is still time to fix possible
minor problems before FF15.
https://hg.mozilla.org/mozilla-central/rev/e54a85233701
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
It will be nice if Web authors don't have to care about the Moz prefix from the start.
Target Milestone: --- → mozilla15
Attached patch patch for aurora (obsolete) — Splinter Review
Removed deprecation warning to avoid string change.
Attachment #627474 - Flags: review?(bugs)
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.
Is there an utility function to display non-localized warnings?
(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.
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.
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.
(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.
takeRecords is a backwards compatible addition though, no?
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.)
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 on attachment 627474 [details] [diff] [review]
patch for aurora

Well, ok, if you two insist that we should do this...
Attachment #627474 - Flags: review?(bugs) → review+
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
Attachment #627474 - Flags: approval-mozilla-aurora?
Blocks: 759150
(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.
Attachment #627474 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Ugh, I've overlooked an IID change.
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.
Attached patch patch for auroraSplinter Review
We can unprefix the interface without iid bump because it is binary-compatible.
Attachment #627474 - Attachment is obsolete: true
Attachment #628337 - Flags: review?(bugs)
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.
Attachment #628337 - Flags: approval-mozilla-aurora?
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)?
Yes. binary addons don't refer the name.
A precedent: WebSocket didn't bump the iid when unprefixed.
https://hg.mozilla.org/mozilla-central/rev/2afd7ae68e8b#l3.21
Attachment #628337 - Flags: review?(bugs) → review+
Looping in Jorge to get his final opinion on this bug. Just want to double verify comment 29.
(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 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.
Attachment #628337 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: checkin-needed
Whiteboard: [land 628337 to aurora]
Comment on attachment 628337 [details] [diff] [review]
patch for aurora

Migration is complete, please nominate for mozilla-beta
Attachment #628337 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora-
Comment on attachment 628337 [details] [diff] [review]
patch for aurora

Requesting beta approval.
Attachment #628337 - Flags: approval-mozilla-beta?
Keywords: checkin-needed
Whiteboard: [land 628337 to aurora]
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.
Attachment #628337 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Keywords: checkin-needed
Whiteboard: [land 628337 to beta]
I messed up the review flag on this patch, but oh well.

https://hg.mozilla.org/releases/mozilla-beta/rev/4698044001a2
Keywords: checkin-needed
Whiteboard: [land 628337 to beta]
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?
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.
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.