Tom S [:evilpie] Assignee
	Description • 8 years ago

The amount of time this behavior is triggered is basically irrelevant. 1.48B vs 24 (http://mzl.la/1LW2fdo).

So with this change when invoking __define{GS}etter__ with null/undefined will throw. We have a few addons that trigger this by using __defineGetter__("a", function(){}) at the global scope.

	Till Schneidereit [:till]
	Comment 3 • 8 years ago

Comment on attachment 8729148 [details] [diff] [review]
Remove legacy __defineGetter__/__defineSetter__ this behavior

Review of attachment 8729148 [details] [diff] [review]:
-----------------------------------------------------------------

Do we still need the telemetry? Do you intend to check later on that we didn't make a mistake in removing this? If you don't, then please also remove the reporting, perhaps in a new patch.

::: js/src/jit-test/tests/ion/bug1076026.js
@@ +1,1 @@
> +var global = this;

This doesn't seem to be required.

	Till Schneidereit [:till]
	Comment 4 • 8 years ago

Comment on attachment 8729150 [details] [diff] [review]
Add steps to legacy functions and a new test

Review of attachment 8729150 [details] [diff] [review]:
-----------------------------------------------------------------

Can you change the patch description to include the actual code changes? Without that, this sort of hides the fact that there's more going on than a simple addition of comments. r=me with that, with or without feedback addressed.

::: js/src/builtin/Object.js
@@ +59,5 @@
>      // Step 2.
>      return callContentFunction(O.toString, O);
>  }
>  
> +// ES7 draft (2016 March 10) B.2.2.3

The draft is actually from March 8. It won't matter in this case, but theoretically there could be situations where relevant changes are committed between release of a draft and whenever the code comment is made.

@@ +89,4 @@
>      std_Object_defineProperty(object, key, desc);
> +
> +    // Step 6.
> +    return undefined;

I usually do `// Step 6 (implicit).` and omit the return, but up to you.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 8 years ago

Comment on attachment 8729703 [details] [diff] [review]
Fix a DOM test that uses legacy define behavior

r=me

	Tom S [:evilpie] Assignee
	Comment 7 • 8 years ago

(In reply to Till Schneidereit [:till] from comment #3)
> Do we still need the telemetry? Do you intend to check later on that we
> didn't make a mistake in removing this? If you don't, then please also
> remove the reporting, perhaps in a new patch.

I want to keep this one more version, so we still get Telemetry and can revert this change if something bad happens.

	Pulsebot
	Comment 8 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd22586cac82
https://hg.mozilla.org/integration/mozilla-inbound/rev/16b05a0fb83a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b19afd04625

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/bd22586cac82
https://hg.mozilla.org/mozilla-central/rev/16b05a0fb83a
https://hg.mozilla.org/mozilla-central/rev/7b19afd04625

	Kohei Yoshino [:kohei]
	Comment 10 • 8 years ago

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/definegetter-and-definesetter-can-no-longer-be-called-at-the-global-scope/

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 8 years ago

Thanks for posting that!  One nit:

> since the legacy behaviour, where the associated object becomes null or undefined, is not allowed by ECMAScript 7

The legacy behavior is that undefined or null this means this gets set to the global.  That's backwards from what you have in the compat note...

	Kohei Yoshino [:kohei]
	Comment 12 • 8 years ago

Thanks Boris! Yes, the draft was misleading. Corrected as follows:
https://github.com/fxsitecompat/www.fxsitecompat.com/commit/a981f7c

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 8 years ago

Ah, excellent.  I hadn't realized there was a github repo for fxsitecompat; I would have just filed an issue there.

	Kohei Yoshino [:kohei]
	Comment 14 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #13)
> Ah, excellent.  I hadn't realized there was a github repo for fxsitecompat;
> I would have just filed an issue there.

There is also a link saying "Improve this page" at the bottom of each page so feel free to send pull requests :)

	Florian Scholz (Open Web Docs)
	Comment 15 • 8 years ago

https://developer.mozilla.org/en-US/Firefox/Releases/48#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/__defineGetter__#Compatibility_notes
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/__defineSetter__#Compatibility_notes