All users were logged out of Bugzilla on October 13th, 2018

Convert context-menu events to style described in bug 593921

RESOLVED FIXED in 1.0b1

Status

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: adw, Assigned: adw)

Tracking

unspecified
1.0b1

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cherry-pick-1.0b1])

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
Created attachment 495314 [details] [diff] [review]
patch

See bug 593921 comment 8.

Myk, one thing I noticed in doing this conversion is that the descriptiveness provided by onMessage's parameter name is lost.  e.g., if my content script does a postMessage(imageURL), I could write onMessage(imageURL) and it's clear what kind of message it's expecting.  Now I have to look inside onMessage's body to see how event.data (or maybe event.data.imageURL) is used.  Not a big deal, just thought I'd mention it.
Attachment #495314 - Flags: review?(myk)
Comment on attachment 495314 [details] [diff] [review]
patch

> Myk, one thing I noticed in doing this conversion is that the descriptiveness
> provided by onMessage's parameter name is lost.  e.g., if my content script
> does a postMessage(imageURL), I could write onMessage(imageURL) and it's clear
> what kind of message it's expecting.  Now I have to look inside onMessage's
> body to see how event.data (or maybe event.data.imageURL) is used.  Not a big
> deal, just thought I'd mention it.

Yup, it's a good point.

Note that there is a more complicated form of destructuring assignment that does provide this descriptiveness:

  onMessage: function ({ data: imageURL }) {
    // imageURL has been assigned the value of event.data
  }

One can even assign a parameter to a property of a property:

  on("click", function ({ node: { src: nodeSrc }}) {
    // nodeSrc has been assigned the value of event.node.src
  });

I'm not sure the increased descriptiveness is worth the added complexity, however, especially in the "property of a property" case.
Attachment #495314 - Flags: review?(myk) → review+
(Assignee)

Comment 2

8 years ago
patch:
https://github.com/mozilla/addon-sdk/commit/6c7a11af8fb3289c68df1be56b84041edc47b523

merge:
https://github.com/mozilla/addon-sdk/commit/f80182604c663705dae3a6b58ff917a180596533
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [cherry-pick-needed]
Target Milestone: -- → 0.10
(Assignee)

Comment 3

8 years ago
I forgot to update the context-menu example in the Programs page of the tutorial.  It's a small change, so I pushed it as a follow-up:

https://github.com/mozilla/addon-sdk/commit/a520fff1d13149f66605d99c4e05b90d7a5e517a
Target Milestone: 0.10 → 1.0b1
You need to log in before you can comment on or make changes to this bug.