668512 - add image support to context menu

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/201

Pointer to Github pull-request

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/201#

Pointer to Github pull-request

Comment 3

[Drew Willcoxon :adw]

Sweet, I've been wanting to add icon support for a while now, thanks.  r+ with the inline comments addressed.  The only thing missing is the doc update, and I'd be happy to write that myself as a follow-up.  And since this is adding to the API, I'd like to get Myk's opinion on it.

Myk, this patch adds an `image` property to menu items whose value is a string URL pointing to image data.  Any thoughts before we land this?

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/201/

Pointer to Github pull-request

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 543661 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/201/

thanks for review! fixed comments. also added update/removal support and made a few other minor changes so asking for another pass.

Comment 6

[Drew Willcoxon :adw]

Thanks for adding null support.  I just noticed a problem: the image setter only works for items in the top-level menu.  It's my fault, since you modeled the image setter after the label setter, and the label setter has the same problem.  Let me figure out how to fix that (bug 669475) and then let's come back to this, if that's OK.

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 543661 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/201/

OK, bug 669475 landed.  I think it should be pretty straightforward to implement images now.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #3)
> Myk, this patch adds an `image` property to menu items whose value is a
> string URL pointing to image data.  Any thoughts before we land this?

ui-r=myk

Any chance y'all can update and land the patch before we merge to the stabilization branch on Tuesday, August 2?

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Note for posterity: I checked other addon platforms, and Google Chrome's calls this an "icon", while Safari's calls it an "image", and XUL calls it an "image".

Comment 10

[Drew Willcoxon :adw]

I think I can update Dietrich's patch and have it ready for review by later today.

Comment 11

[Drew Willcoxon :adw]

Attachment: patch

A few small unrelated changes:

* nsIDOMHTMLIsIndexElement was recently removed.  Bug 666665 comment 4
  says to use nsIDOMHTMLInputElement instead.

* PRIVATE_PROPS_KEY used to be an object before I changed it to a string
  in the big rewrite patch.  Changing it was dumb, since values passed
  to item.valueOf() are compared to it using equality, which means that
  anyone who happens to guess the key could get the item's private
  properties.  So I reverted it back to an object.

* Underscores added to the names of traits' init methods to make them
  private, as they should be.

Comment 12

[Drew Willcoxon :adw]

Attachment: patch with doc

Forgot to update the doc.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 549914 [details] [diff] [review]
patch with doc

Looks good, r=myk, thanks for the quick turnaround!

Comment 14

[Drew Willcoxon :adw]

https://github.com/mozilla/addon-sdk/commit/e9587be2a2d4f1a2c5ab46397623c33cdae46896