Closed Bug 1168663 Opened 5 years ago Closed 4 years ago

`Object.getOwnPropertyNames(window)` in browser window triggers FUEL deprecation warning

Categories

(Firefox :: General, defect)

defect
Not set
Points:
1

Tracking

()

RESOLVED WONTFIX

People

(Reporter: kmag, Unassigned)

References

Details

(Whiteboard: [good first bug][lang=js])

+++ This bug was initially created as a clone of Bug #989307 +++

Calling `Object.getOwnPropertyNames(window)` results in the following warning:

DEPRECATION WARNING: FUEL is deprecated, you should use the add-on SDK instead.
You may find more details about this deprecation at: https://developer.mozilla.org/Add-ons/SDK/
jar:file:///home/kris/testing/ff41/browser/omni.ja!/components/fuelApplication.js 1458 Application
jar:file:///home/kris/testing/ff41/browser/omni.ja!/components/fuelApplication.js 726 af_ci
...

This warning should be issued when someone actually tries to use FUEL, not when they accidentally make an oblique reference to it.
Yes, this is basically the same as bug 1159693.
The problem is that enumerating windows properties initializes window.Application that initializes FUEL. That's also not nice since code not needing FUEL ends up initializing it, so the warning is not totally wrong.
I'm not sure if there's a way to make Application global property even lazier.
Warning on every use would not be trivial, since FUEL imports extApplication prototype from Toolkit and we don't want to deprecate that.
Using a Proxy would also not be trivial.

Any ideas? Otherwise, we'll have to live with the warning until we can remove FUEL completely :/
No longer blocks: 1094832
What about making `Application` a Proxy which calls the deprecation warning the first time one of its properties is requested?
Proxies are not completely transparent, Application has also methods, not just properties. Then we should basically proxy every method.
also, not taking into account all the subtle bugs we might create...
(In reply to Marco Bonardo [::mak] from comment #3)
> Proxies are not completely transparent, Application has also methods, not
> just properties. Then we should basically proxy every method.

I don't follow you.
A simple
   new Proxy(foo, {get: function(target, name) { /* print warning */; return target[name]; } });
should do the trick, even for methods.
I think a proxy should work without any problems. We already use them for much trickier cases than this. The only caveats are that we'd probably have to move to implementing nsIDOMGlobalPropertyInitializer, and for any functions we return, we might have to use `target[name].bind(target)` to avoid triggering deprecation warnings from internal code.

Ideally we should also avoid issuing warnings more than once per caller, with something along the lines of:

    let seen = new Set;
    return new Proxy(Application, {
	get: function (target, name) {
            let caller = Components.stack.caller || {};
            if (!seen.has(caller.filename)) {
                seen.add(caller.filename)
                Deprecated.warning("FUEL is deprecated, you should use the add-on SDK instead.",
                                   "https://developer.mozilla.org/Add-ons/SDK/");
            }
            if (typeof target[name] === "function")
                return target[name].bind(target);
            return target[name];
        }
    });

(Although I'd prefer that logic to be handled by Deprecated.jsm)
(In reply to Kris Maglione [:kmag] from comment #6)
> for any
> functions we return, we might have to use `target[name].bind(target)` to
> avoid triggering deprecation warnings from internal code.

which internal code?
All of this sounds more complex than I was hoping, we should not spend development time on FUEL for any reason.
to be clear, I would like to issue the deprecation only once in the session, like it is now. So even with a proxy, I'd issue deprecation only the first time we proxy something. There's no point in flooding the console, we have also other ways to reach developers.
(In reply to Marco Bonardo [::mak] from comment #7)
> which internal code?

Functions in extApplication.js which refer to `this`.

(In reply to Marco Bonardo [::mak] from comment #8)
> to be clear, I would like to issue the deprecation only once in the session,
> like it is now.

Well, the main reason I'd prefer to issue it once per caller is that I'd like extension developers to realize the warning is associated with their code so that they fix it. And, again, I'd really like that logic to be in Deprecated.jsm, because I think it's the way most deprecation warnings should behave (so I wouldn't consider it development time spent on FUEL).
(In reply to Marco Bonardo [::mak] from comment #7)
> (In reply to Kris Maglione [:kmag] from comment #6)
> which internal code?

let raw = { x: 1, getX: function() { return this.x; } };
let proxied = new Proxy(raw, { get: function(target, name) { console.error("Hey, please don't use", name); return target[name]; });

proxied.x; // => "Hey, please don't use x";
proxied.getX(); // => "Hey, please don't use getX"; "Hey, please don't use x"

The "internal code" here is the reference to `this.x`.
No longer blocks: 1090880
Depends on: 1090880
FUEL is being removed in bug 1090880, so this is now wontfix.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.