963111 - Toolbar should be able to accept buttons as well, other than frames

Status: RESOLVED FIXED

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

Description

[Matteo Ferretti [:zer0] [:matteo]]

Currently the Toolbar is accepting only `Frame` instances in `items` property – as the prototype before.

Developers should be able to add buttons too.

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1361

Comment 2

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

Comment on attachment 8364405 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1361

Matteo, I do like the patch, I'm r- it since I want to get it off my queue until I get a response on the comment in pull, after which I'll be comfortable
to r+ it.

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

I replied to the comments in PR, let me know.

Comment 4

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

Comment on attachment 8364405 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1361

Changeing to r+ given the explanation I got on IRC. Matteo, could you still
please put a comment explaining that id translation is done at the model level
since toolbar depends on identify to get ID's for CUI.

Comment 5

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/0e601a832fb24233a997cf136d06bd063e77291f
Merge pull request #1361 from ZER0/toolbar-button/963111

fix Bug 963111 - Toolbar should be able to accept buttons as well, other than frames

Comment 6

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1425/commits

The patch depends by bug 966727, the only commit strictly related to the test case is the follow: https://github.com/ZER0/addon-sdk/commit/0a23b81e4d7feb0f4ee8af30847b2a8fc962369d

Comment 7

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8388449 [details]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1425/commits

I wasn't sure if it was worthy create a new bug for that. Let me know for the future, just in case, what's the preferable option.

Comment 8

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

Comment on attachment 8388449 [details]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1425/commits

Test looks good, although I'm confused why there's dependency on customizable stuff. R+ as long as what you land will only contain button test.

Comment 9

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/2ef34353773adf45529d9287e7d75b476cf3187e
Bug 963111 - Toolbar should be able to accept buttons as well, other than frames

- Added test case

https://github.com/mozilla/addon-sdk/commit/50251e5b632ea644a4a8618361dd47d280773bf3
Merge pull request #1425 from ZER0/toolbar-button-test/963111

fix Bug 963111 - Toolbar should be able to accept buttons as well, other than frames r=@gozala