1070952 - Context-menu 2.0

Status: RESOLVED INCOMPLETE

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

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Current context menu API is not optimized for simple us case, it always requires use of content scripts that is quite an overhead. In addition it's really hard to port it to E10s.

I think it's a good time to consider designing context-menu@2.0 with a simpler API that is designed and implemented with e10s in mind. Content script should not be a part of context menu API as it adds a lot of complexity and overhead for a very little benefit.

Comment 1

[Tomislav Jovanovic :zombie]

related bug 1060138, and some discussion (for why old api is bad) in bug 1058698..

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Initial review request

I'm still adding more tests, but I think it's ready for initial review.

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Changes to sdk/core/disposable

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Changes to sdk/util/sequence

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Changes to toolkit/require

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Utils for working with message managers

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Implement react like Component API for UI updates

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Initial patch without changes that moved to other pulls

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8524426 [details] [review]
Changes to toolkit/require

I mentioned a few changes that I'd like to see, and this also needs some tests.

Also I wonder if the modules should be unloaded somehow too, instead of just being deleted.

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8524447 [details] [review]
Utils for working with message managers

Looks good to me, but it needs tests.  Also mark the modules as unstable and add the license header please.

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8522488 [details] [review]
Initial review request

Taking my r? off of this since we agreed to split it up in to separate pull requests.

Comment 12

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8524413 [details] [review]
Changes to sdk/util/sequence

lgtm!

Comment 13

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/62a6ad476137dc0e1acf778cce857ea80b67389d
Merge pull request #1717 from Gozala/context-menu-2-disosables@1070952

Bug 1070952 - Export `setupDisposable` and `disposeDisposable` r=@erikvold

Comment 14

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/29566c0e4096e5beec11d6705831b8aa1731407b
Merge pull request #1718 from Gozala/context-menu-2-sequence@1070952

Bug 1070952 - Update sequence lib to cover ES6 Symbols. r=@erikvold

Comment 15

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Changes to toolkit/require v2

Addressed review comments and have added tests.

Comment 16

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Utils for working with message managers V2

Added licenses and metadata + tests, re-requesting review.

Comment 17

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 8525179 [details]
Changes to toolkit/require v2

>https://github.com/mozilla/addon-sdk/pull/1719

Comment 18

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8525179 [details]
Changes to toolkit/require v2

I made a mentioned of edits for the comments, looks good otherwise.

Comment 19

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8525236 [details] [review]
Utils for working with message managers V2

nice!

Comment 20

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/7009435ffcde9933f2d202b863a29123bcbaac45
Merge pull request #1719 from Gozala/context-menu-2-require@1070952

Bug 1070952 - Implement module reloading useful during development. r=@erikvold

Comment 21

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/fc3b3e0d5109b86f54fce17c7de87922aa79363e
Merge pull request #1720 from Gozala/context-menu-2-message-manager@1070952

Bug 1070952 - Utils for working with message managers. r=@erikvold

Comment 22

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/65cbeef3b22ad43af2108be56c873f91e339e8dd
Merge pull request #1721 from Gozala/context-menu-2-component@1070952

Bug 1070952 - Implement react style component API for UI updates. r=@erikvold

Comment 23

[Erik Vold [:erikvold] (please needinfo? me)]

I had to revert these three commits in order to get the tree green a couple of weeks ago:

https://github.com/mozilla/addon-sdk/commit/aafae3c582ad861172184062ad6184780b2f1e9b
https://github.com/mozilla/addon-sdk/commit/df287ba774f54a25ca70b49df834ecae263d5f77
https://github.com/mozilla/addon-sdk/commit/117ab60d092b364c3f32509eabc67f657a811187

Irakli has already started on a fix in:

https://github.com/mozilla/addon-sdk/pull/1751

Comment 24

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/a5e46bb032b32264a107b2a1b4f1761c9e5a10ae
Merge pull request #1751 from Gozala/re-context-menu@2

Bug 1070952 - ContextMenu@2 r=@erikvold

Comment 31

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562