1055571 - New API: ToolSidebar.removeTab

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

It is already possible to utilize ToolSidebar object and create side bars in new or existing Toolbox panels. It's also possible to create new side-panels, but there is now API to remove them.

The implementation should happen in:
browser/devtools/framework/sidebar.js

Honza

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1055571-1.patch

Suggested patch, couple of comments:

1) New removeTab method implemented
2) Use event.target to get win+doc from the loaded iframe. iframe.contentDocument isn't set if the iframe is being loaded in hidden <tabbox> (could be related to bug 1056033)

Joe, I saw you did the original implementation of the code, so asking for review.

Honza

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8475878 [details] [diff] [review]
bug1055571-1.patch

Ah, forgot to set the flag...
Honza

Comment 3

[Joe Walker [:jwalker]]

Comment on attachment 8475878 [details] [diff] [review]
bug1055571-1.patch

Review of attachment 8475878 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry Honza - I'm going to have to dodge this, I'm afraid

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #3)
> Sorry Honza - I'm going to have to dodge this, I'm afraid
Ok, not a problem.

Btw. any chance you have more info about the hack (a time out) here:
https://github.com/mozilla/gecko-dev/blob/master/browser/devtools/framework/sidebar.js#L104

Honza

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8475878 [details] [diff] [review]
bug1055571-1.patch

Review of attachment 8475878 [details] [diff] [review]:
-----------------------------------------------------------------

Changes look good overally - can you add a case in devtools/framework/test/browser_toolbox_sidebar.js to cover this new API?

::: browser/devtools/framework/sidebar.js
@@ +114,5 @@
>    },
>  
> +  removeTab: function(id) {
> +    let tab = this._tabbox.tabs.querySelector("tab#sidebar-tab-" + id);
> +    if (!tab)

Please use brackets around if statements

@@ +119,5 @@
> +      return;
> +
> +    tab.remove();
> +
> +    let panel = this._tabbox.tabpanels.querySelector(

You could call `let panel = this.getTab(id)`

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1055571-2.patch

Thanks for the review Brian. Here is updated patch.
Honza

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8484907 [details] [diff] [review]
bug1055571-2.patch

Review of attachment 8484907 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good - pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3725d452ce18

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/2600340abfbe

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2600340abfbe