920780 - Sidebars attach event emits when using show() even if the sidebar is already shown

Status: RESOLVED FIXED

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

Description

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

Example add-on:

'use strict';

var self = require("sdk/self");

// data URI encoding of "some content"
var dataUrl = "data:text/plain;charset=utf-8;base64,c29tZSBjb250ZW50";

var sidebar = require("sdk/ui/sidebar").Sidebar({
  id: 'my-sidebar',
  title: 'My sidebar',
  url: dataUrl,
  onAttach: function (worker) {
    // this gets logged every time you click the widget
    console.log("worker is attached");
  }
});

require("sdk/widget").Widget({
  id: "clickme",
  label: "Click me",
  content: "Click me!",
  width: 50,
  onClick: function() {
    console.log("you clicked me")
    sidebar.show();
  }
});

Comment 1

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

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

Pointer to Github pull-request

Comment 2

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

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

The unit test doesn't fail when expected.

Comment 3

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

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

[Approval Request Comment]
User impact if declined: A new Jetpack api emit events when they should not be.  Early adopters will have to write work arounds.

Testing completed (on m-c, etc.): yes

Risk to taking this patch (and alternatives if risky): Low/No risk

Comment 4

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

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

looks good! The nit / comments are up to you, are not blockers. r+

Comment 5

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/bcdc976eb5a5d22d8e2f3771a6aaf2d0d838130e
Bug 920780 - Sidebars attach event emits when using show() even if the sidebar is already shown

https://github.com/mozilla/addon-sdk/commit/2bc0d47ff3f9fde423f36ba862d62ef65901635e
Bug 920780 working around the race condition and making the test more reliable.

https://github.com/mozilla/addon-sdk/commit/a44176661b1b61dffb46ce2ff5a4156bda38cf49
Merge pull request #1248 from erikvold/920780

Bug 920780 - Sidebars attach event emits when using show() even if the sidebar is already shown r=@zer0

Comment 6

[bhavana bajaj [:bajaj]]

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

Given where we are still early in the cycle, this is ok to take. Please make sure to test/verify.

Comment 7

[bhavana bajaj [:bajaj]]

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

Flipping back the a+ till we have more information per https://bugzilla.mozilla.org/show_bug.cgi?id=923348#c9