556754 - Add-on Builder generates add-on whose context-menu cannot work

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, defect, P3)

Description

[dynamis (Tomoya ASAI)]

Current Add-on Builder
https://addons.mozilla.org/en-US/developers/tools/builder
will generate ff-overlay.xul which includes:

var extname = {
  onLoad: function() {
    // initialization code
    this.initialized = true;
    this.strings = document.getElementById("extname-strings");
  },
  onMenuItemCommand: function(e) {
    var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
                                  .getService(Components.interfaces.nsIPromptService);
    promptService.alert(window, this.strings.getString("helloMessageTitle"),
                                this.strings.getString("helloMessage"));
  },
  ...
}
window.addEventListener("load", extname.onLoad, false);

Context menu cannot work since extname.strings is not defined on window load event but only global strings variable is defined.
# be careful about "this" when you pass method as callback or event handlers.
Please fix last line to:

window.addEventListener("load", function() { extname.onLoad(); }, false);

Comment 1

[Justin Scott [:fligtar]]

Cesar, Jorge, thoughts?

Comment 2

[Jorge Villalobos [:jorgev] (he/him)]

Just by looking at the code in Comment #0, the bug is clearly there, and the fix looks right.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I had the same problem today by using the generated output from the Add-ons Builder and it has been taken more than an hour to figure out this problem. Thankfully Dave helped me here. Can we get this fixed please?

Comment 4

[Justin Scott [:fligtar]]

Jorge, would you be willing to write a patch for fizzypop to fix this? We still have a good amount of add-ons built using the packager.

http://svn.mozilla.org/projects/fizzypop/trunk/

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

Attachment: Patch V1. Add function wrapper for both overlay templates

Here's a patch (trivial, but untested). Who reviews this stuff?

Comment 6

[Justin Scott [:fligtar]]

mfinkle and cesar are responsible for fizzypop I think.

Comment 7

[Jorge Villalobos [:jorgev] (he/him)]

Fixed, r76511.

Comment 9

[Jorge Villalobos [:jorgev] (he/him)]

It appears that this fix (and probably the fix for bug 556752) were never pushed to the server. According to bug 625053, this is still happening.

What's the procedure to get this pushed?

Comment 10

[Jeff Balogh (:jbalogh)]

fizzypop is an svn:external, but it's not locked to a revision.

site/vendors/fizzypop http://svn.mozilla.org/libs/fizzypop.mozdev

I'd expect this to get updated automatically when we svn up remora, but I don't really remember how svn works anymore.

Comment 11

[Jeff Balogh (:jbalogh)]

Ah, I just noticed that jorgev has been editing projects/fizzypop/ while remora has been pointing at libs/fizzypop.mozdev.  Fun stuff!

Comment 12

[Wil Clouser [:clouserw]]

Like Jeff said, AMO is using http://svn.mozilla.org/libs/fizzypop.mozdev .  Can you put any changes into that library?

Comment 13

[Wil Clouser [:clouserw]]

Also, fwiw, I do update it with every remora release, it's just pulling from a spot that wasn't getting new code I guess.

Comment 14

[Jorge Villalobos [:jorgev] (he/him)]

It's fligtar's fault!

I'll move the commits to the new lib in .7. Thanks.

Comment 16

[Jorge Villalobos [:jorgev] (he/him)]

Committing again on r82040.

Comment 17

[Jorge Villalobos [:jorgev] (he/him)]

Verified fixed.