Closed Bug 1151610 Opened 9 years ago Closed 9 years ago

Manage the case where two extensions fight over the same command

Categories

(DevTools :: Console, defect)

40 Branch
defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: fayolle-florent, Assigned: fayolle-florent)

References

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150330154247

Steps to reproduce:

After bug 1143497 (which introduces the WebConsoleCommands API), the conflicts between two extensions for a same command introduces inconsistent behavior.

See: https://pastebin.mozilla.org/8828974

Conversation with :bgrins about this: https://pastebin.mozilla.org/8828979

Florent
Component: Untriaged → Developer Tools: Console
OS: Linux → All
Hardware: x86_64 → All
Summary: Manage the case where two extensions are fighting over the same command → Manage the case where two extensions fight over the same command
Depends on: 1143497
My thinking right now is that any call to unregister() should restore the default behavior, if any.  So anything that "built in" (i.e. registered by the tools from utils.js) will be restored if that key is unregistered.

This will both prevent the built in commands from being removed with no replacement, and also make it easy for addons to clean up after themselves (just call unregister() on any commands you've added).
Status: UNCONFIRMED → NEW
Ever confirmed: true
I am OK with your idea, Brian. I would also introduce a new API to easily and properly override a command. See https://pastebin.mozilla.org/8829109

Florent
Attached patch 1151610.patch (obsolete) — Splinter Review
Here is the current status of my work.

Florent
Honza, what do you think?

Florent
Flags: needinfo?(odvarko)
(In reply to fayolle-florent from comment #4)
> Honza, what do you think?
> 
> Florent
I also vote for restoring the default (command) behavior if an extension calls unregister (even if there is yet another extension overriding the same command).

Honza
Flags: needinfo?(odvarko)
Comment on attachment 8589848 [details] [diff] [review]
1151610.patch

Alright. So :brian, could you review the patch (which only implements your idea).

Florent
Attachment #8589848 - Flags: review?(bgrinstead)
Comment on attachment 8589848 [details] [diff] [review]
1151610.patch

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

LGTM

::: toolkit/devtools/webconsole/utils.js
@@ +1570,5 @@
>     * @param {string} name The name of the command
>     */
>    unregister: function(name) {
>      this._registeredCommands.delete(name);
> +    if (this._originalCommands.has(name))

Nit: brackets around the if block
Attachment #8589848 - Flags: review?(bgrinstead) → review+
Attached patch 1151610.patchSplinter Review
Done.

Florent
Attachment #8589848 - Attachment is obsolete: true
Attachment #8598188 - Flags: review?(bgrinstead)
Attachment #8598188 - Flags: review?(bgrinstead) → review+
Assignee: nobody → fayolle-florent
Status: NEW → ASSIGNED
Everything looks good after the try push.

Florent
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/21b92a74a01f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.