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)
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: smaug, Assigned: smaug)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-needed)
Attachments
(2 files, 1 obsolete file)
20.07 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
17.03 KB,
patch
|
smaug
:
review+
lsblakk
:
approval-mozilla-aurora-
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 2•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=e71daa7d1395
Attachment #619271 -
Flags: review?(jonas)
Updated•12 years ago
|
Keywords: dev-doc-needed
Attachment #619271 -
Flags: review?(jonas) → review+
Comment 3•12 years ago
|
||
Do our implementation (WebKit & Gecko) match exactly?
Assignee | ||
Comment 4•12 years ago
|
||
I think WebKit misses still some recent spec changes. I'll test some more next week.
Comment 5•12 years ago
|
||
Okay, let's make sure our implementations match the spec before shipping them unprefixed :)
Comment 6•12 years ago
|
||
I've filed https://bugs.webkit.org/show_bug.cgi?id=85161 to track the WebKit change.
Assignee | ||
Comment 7•12 years ago
|
||
The version of Chromium (some old v19) I have doesn't seem to handle mutations on DocumentFragment correctly.
Assignee | ||
Comment 8•12 years ago
|
||
I'll land the patch. Easy to back out if needed, and there is still time to fix possible minor problems before FF15.
Assignee | ||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e54a85233701
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 10•12 years ago
|
||
It will be nice if Web authors don't have to care about the Moz prefix from the start.
Comment 11•12 years ago
|
||
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.
Comment 13•12 years ago
|
||
Is there an utility function to display non-localized warnings?
Assignee | ||
Comment 14•12 years ago
|
||
(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.
Assignee | ||
Comment 17•12 years ago
|
||
(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?
Assignee | ||
Comment 19•12 years ago
|
||
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.
Assignee | ||
Comment 21•12 years ago
|
||
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 22•12 years ago
|
||
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?
Comment 23•12 years ago
|
||
(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.
Updated•12 years ago
|
Attachment #627474 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Updated•12 years ago
|
Comment 24•12 years ago
|
||
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.
Comment 26•12 years ago
|
||
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 27•12 years ago
|
||
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?
Assignee | ||
Comment 28•12 years ago
|
||
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•12 years ago
|
||
Yes. binary addons don't refer the name.
Comment 30•12 years ago
|
||
A precedent: WebSocket didn't bump the iid when unprefixed. https://hg.mozilla.org/mozilla-central/rev/2afd7ae68e8b#l3.21
Assignee | ||
Updated•12 years ago
|
Attachment #628337 -
Flags: review?(bugs) → review+
Comment 31•12 years ago
|
||
Looping in Jorge to get his final opinion on this bug. Just want to double verify comment 29.
Comment 32•12 years ago
|
||
(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.
Updated•12 years ago
|
Comment 33•12 years ago
|
||
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+
Updated•12 years ago
|
Updated•12 years ago
|
Keywords: checkin-needed
Whiteboard: [land 628337 to aurora]
Comment 34•12 years ago
|
||
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 35•12 years ago
|
||
Comment on attachment 628337 [details] [diff] [review] patch for aurora Requesting beta approval.
Attachment #628337 -
Flags: approval-mozilla-beta?
Updated•12 years ago
|
Keywords: checkin-needed
Whiteboard: [land 628337 to aurora]
Comment 36•12 years ago
|
||
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+
Updated•12 years ago
|
Keywords: checkin-needed
Whiteboard: [land 628337 to beta]
Comment 37•12 years ago
|
||
I messed up the review flag on this patch, but oh well. https://hg.mozilla.org/releases/mozilla-beta/rev/4698044001a2
Updated•12 years ago
|
Comment 38•12 years ago
|
||
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?
Assignee | ||
Comment 39•12 years ago
|
||
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•11 years ago
|
||
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.
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•