Tim Taubert [:ttaubert] (inactive) Reporter
	Description • 13 years ago

Provide a TabGroups API and use this as the new storage backend for Panorama.

	Raymond Lee [:raymondlee]
	Comment 2 • 13 years ago

Comment on attachment 570219 [details] [diff] [review]
WIP v1

Tim: this API doesn't provide way to store window related data (i.e. the active group id and number of group items) for Panorama. 

Shall we add TabGoups.storage.getter and setter?

	Raymond Lee [:raymondlee]
	Comment 3 • 13 years ago

* Updated the TabGroups API and Panorama is using it as the new storage
* Fixed most of the tests after switching to the TabGroups API, except browser_tabview_bug654721.js (This is related to changing the data format in session storage, will fix it when writing code to migrate old data format to new one in session storage)

Things to do:
* Migrate old data format to new one in session storage
* Fix the remaining test

	Raymond Lee [:raymondlee]
	Comment 4 • 13 years ago

Fixed all tests and added migration code.

Things to do:
* Update comments for functions and objects in the TabGroups API code.

Tim: Could you have a quick to see whether it's ok please?

	Dão Gottwald [:dao]
	Comment 5 • 13 years ago

Please help me understand this bug. As far as I can see, this isn't just refactoring. You're adding way more code than you're removing. What's the value added?

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 6 • 13 years ago

(In reply to Dão Gottwald [:dao] from comment #5)
> Please help me understand this bug. As far as I can see, this isn't just
> refactoring. You're adding way more code than you're removing. What's the
> value added?

TL;DR: We want to separate Panorama's storage from its view and think that other code also benefits from this new API.

The purpose of this API should be to provide a simple storage for Panorama and its tab groups. At the same time it should be easy to access this API for add-ons like Vertical Tabs and Tree Style Tabs that have tab groups as well but implement it by their own.

This API is another big step into the direction of Panorama showing all tabs from all windows. Currently the API restricts these tabs to their windows - this gets removed as soon as the UI is ready to handle tabs from all windows. We want to iterate in smaller steps to not run into bigger problems with this transition.

	Raymond Lee [:raymondlee]
	Comment 7 • 13 years ago

* Some method changes to the TabGroups API and Panorama code
* Added some comments to objects.
* Updated the wiki. https://wiki.mozilla.org/Panorama:TabGroupsAPI

To do:
* It passed tests for TabGroups API and Panorama code locally but not on Try so investigating it.

	Raymond Lee [:raymondlee]
	Comment 8 • 13 years ago

Finally fixed all tests and passed try!

https://tbpl.mozilla.org/?tree=Try&rev=d5d176f86214

	Dão Gottwald [:dao]
	Comment 9 • 13 years ago

> TL;DR: We want to separate Panorama's storage from its view and think that
> other code also benefits from this new API.

What other code?

> The purpose of this API should be to provide a simple storage for Panorama
> and its tab groups.

A local tabview module would be sufficient for this alone.

> At the same time it should be easy to access this API
> for add-ons like Vertical Tabs and Tree Style Tabs that have tab groups as
> well but implement it by their own.

Tree Style Tab doesn't have groups but a tab hierarchy. I guess Vertical Tabs could use this; however, what's the benefit beyond saving some code?

> This API is another big step into the direction of Panorama showing all tabs
> from all windows. Currently the API restricts these tabs to their windows -
> this gets removed as soon as the UI is ready to handle tabs from all
> windows.

How does this go with the above? Does Vertical Tabs want to show tabs from all windows? As opposed to panorama, it doesn't hide non-active groups.

> We want to iterate in smaller steps to not run into bigger problems
> with this transition.

Providing an API for add-ons locks you into compatibility hell and may actually make it harder to iterate...

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 10 • 13 years ago

(In reply to Dão Gottwald [:dao] from comment #9)
> > TL;DR: We want to separate Panorama's storage from its view and think that
> > other code also benefits from this new API.
> 
> What other code?

Included in Firefox it's only Panorama. The plan is to make that API available to add-on authors and of course via Jetpack, too.

> > The purpose of this API should be to provide a simple storage for Panorama
> > and its tab groups.
> 
> A local tabview module would be sufficient for this alone.

True, but this way we can't expose the basic storage independent from its view to add-ons and other code areas that might need to know about tabs and their groups. I talked to Limi and there are definitely plans to simplify Panorama and experiment with vertical tabs. We'd of course want a smooth transition for users switching between both of those features.

> > At the same time it should be easy to access this API
> > for add-ons like Vertical Tabs and Tree Style Tabs that have tab groups as
> > well but implement it by their own.
> 
> Tree Style Tab doesn't have groups but a tab hierarchy. I guess Vertical
> Tabs could use this; however, what's the benefit beyond saving some code?

True, but Tree Style Tabs could use the TabGroups API as its backend, too. The hierarchy could be implemented via custom attributes of each group. The benefit is that users could choose to switch between Tree Style Tabs, Vertical Tabs and Panorama whenever they want. We of course want to encourage add-on devs to write other views of tabs and their groups because they might have different/better ideas.

> > This API is another big step into the direction of Panorama showing all tabs
> > from all windows. Currently the API restricts these tabs to their windows -
> > this gets removed as soon as the UI is ready to handle tabs from all
> > windows.
> 
> How does this go with the above? Does Vertical Tabs want to show tabs from
> all windows? As opposed to panorama, it doesn't hide non-active groups.

Yeah it doesn't hide them but that's the point. It's a different view. This is all about an abstract storage of tab groups.

> > We want to iterate in smaller steps to not run into bigger problems
> > with this transition.
> 
> Providing an API for add-ons locks you into compatibility hell and may
> actually make it harder to iterate...

I didn't think of it that way, but it's true. Before landing this API we'd need to make sure that this is all we need to successfully implement the All-Windows UI for Panorama.

	Dão Gottwald [:dao]
	Comment 11 • 13 years ago

(In reply to Tim Taubert [:ttaubert] from comment #10)
> (In reply to Dão Gottwald [:dao] from comment #9)
> > > TL;DR: We want to separate Panorama's storage from its view and think that
> > > other code also benefits from this new API.
> > 
> > What other code?
> 
> Included in Firefox it's only Panorama. The plan is to make that API
> available to add-on authors and of course via Jetpack, too.

It's not obvious to me why we would "of course" expose this via Jetpack. There needs to be some very strong use case. The SDK's stability comes with reduced power, it can't provide APIs for everything. Are tab groups going to exist on mobile Firefox?

> > > The purpose of this API should be to provide a simple storage for Panorama
> > > and its tab groups.
> > 
> > A local tabview module would be sufficient for this alone.
> 
> True, but this way we can't expose the basic storage independent from its
> view to add-ons and other code areas that might need to know about tabs and
> their groups.

"might need to know about" sounds wishy-washy. We need some compelling use cases here, otherwise we shouldn't expose the API, as APIs aren't free.

> I talked to Limi and there are definitely plans to simplify
> Panorama and experiment with vertical tabs. We'd of course want a smooth
> transition for users switching between both of those features.

Sure, panorama can be simplified and we can experiment with stuff. We need no stable API for this.

> > > This API is another big step into the direction of Panorama showing all tabs
> > > from all windows. Currently the API restricts these tabs to their windows -
> > > this gets removed as soon as the UI is ready to handle tabs from all
> > > windows.
> > 
> > How does this go with the above? Does Vertical Tabs want to show tabs from
> > all windows? As opposed to panorama, it doesn't hide non-active groups.
> 
> Yeah it doesn't hide them but that's the point. It's a different view.

My point is that the way in which it's different may be incompatible with the tabs-from-all-windows concept.

> > > We want to iterate in smaller steps to not run into bigger problems
> > > with this transition.
> > 
> > Providing an API for add-ons locks you into compatibility hell and may
> > actually make it harder to iterate...
> 
> I didn't think of it that way, but it's true. Before landing this API we'd
> need to make sure that this is all we need to successfully implement the
> All-Windows UI for Panorama.

Or just land it in tabview/...

	Dietrich Ayala (:dietrich)
	Comment 13 • 13 years ago

I agree with Dao, this should be in tabview, doesn't need a new top-level directory. Also, definitely should confirm that it plays well with hidden vs no-hidden tabs, transferring between those views, etc.

Dao: I specifically requested this from Tim and Raymond for a variety of the reasons above. There's no new API "exposure", outside of making the existing "exposed" API much clearer. There is no "stable api" requirement on this code that's any different than any other internal code that we have in Firefox.

Re Jetpack: *Anyone* can write a Jetpack module for TabGroups, and this internal API makes it much easier to do so. Whether I write that module and distribute it on the Jetpack modules page, or the Jetpack team decides to ship a built-in module is up me or any Jetpack add-on author or the Jetpack team.

This patch also makes XUL add-on interaction with tab groups clearer and easier. Tim consulted with Philikon about his add-on specifically. No doubt there are more use-cases, so thanks for any input you have on where this API does or doesn't address them.

	Raymond Lee [:raymondlee]
	Comment 15 • 12 years ago

* Moved the "tabgroups" directory to "tabview" directory.
* Fixed tests which fail to run.

Push to try
https://tbpl.mozilla.org/?tree=Try&rev=962b60165ee6

	Raymond Lee [:raymondlee]
	Comment 16 • 12 years ago

(In reply to Raymond Lee [:raymondlee] from comment #15)
> Created attachment 593147 [details] [diff] [review]
> v2
> 
> * Moved the "tabgroups" directory to "tabview" directory.
> * Fixed tests which fail to run.
> 
> Push to try
> https://tbpl.mozilla.org/?tree=Try&rev=962b60165ee6

Other commit caused tests to fail in last push.

The same patch and it passed the tests
https://tbpl.mozilla.org/?tree=Try&rev=8d072f077c14

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 17 • 12 years ago

Comment on attachment 593147 [details] [diff] [review]
v2

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

Raymond, thank you for all the work. Some overall comments first:

1) Please assemble everything into TabGroups.jsm instead of #including/having multiple files.

2) Use the new license headers http://www.mozilla.org/MPL/headers/ for all new files (no need to change existing files, they'll be changed by a script).

3) It would be great to split this into multiple patches. Maybe the first implementing TabGroups.jsm, the second implementing the tests, the third changing existing Panorama code and the fourth correcting Panorama tests.

::: browser/base/content/browser-tabview.js
@@ +114,5 @@
> +            window.removeEventListener("SSWindowStateReady", onSSWindowStateReady, false);
> +            // window storage for all windows is set after window state is 
> +            // ready so a delay is needed here
> +            setTimeout(function() {
> +              Cu.import("resource:///modules/TabGroups.jsm");

Please use XPCOMUtils.defineLazyModuleGetter() for TabGroups.

@@ +138,5 @@
>  
> +  // ----------
> +  // Migrates existing data in session store to a new format which works with
> +  // TabGroups API as the new storage.
> +  _migrateSessionData: function TabView__migrateSessionData() {

This whole migration of old data shouldn't be a concern of Panorama. Please move that to TabGroups.jsm.

::: browser/components/tabview/tabgroups/groups.js
@@ +224,5 @@
> + * An object that represents a window.
> + *
> + * @constructor
> + */
> +function Window(aWindow, aData) {

We don't need this, please remove.

@@ +274,5 @@
> +
> +    return group;
> +  },
> +
> +  getWindowStorage: function TGS_getWindowStorage(aWindow) {

We shouldn't expose getWindowStorage() as a public API. Panorama needs to take care of all window-specific values it needs to store itself. The ultimate goal is to handle all windows.

::: browser/components/tabview/tabgroups/storage.js
@@ +101,5 @@
> +
> +    this.setGroupsData(aWindow, groupsData);
> +  },
> +
> +  setAdditionsData: function STO_setAdditionsData(aWindow, aName, aData) {

The 'additions' shouldn't be stored unter the 'tabgroups' key in session store. We should use 'panorama' as a key here because they're specific to Panorama (which is just a view of tab groups). So all this code needs to be in Panorama.

::: browser/components/tabview/tabitems.js
@@ +1097,5 @@
>  
>    // ----------
> +  // Function: _migrateSessionData
> +  // Migrates session data for a xul:tab if needed.
> +  _migrateSessionData: function TabItems__migrateSessionData(tab) {

Same here, please move to the JSM.

	Raymond Lee [:raymondlee]
	Comment 18 • 12 years ago

Tim: you know we have "bounds" and "url" stored under the key "tabview-tab" for tab and "bounds" and "userSize" under "tabview-group" for window in the session at the moment.  Shall we move and store them and store under the key "Panorama" as well?

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 19 • 12 years ago

Yes, exactly. Those properties are view-specific (i.e. Panorama) and should be stored separately.

	Raymond Lee [:raymondlee]
	Comment 20 • 12 years ago

Assembled everything into TabGroups.jsm with tests

	Raymond Lee [:raymondlee]
	Comment 21 • 12 years ago

This patch changes the Panorama code and uses TabGroups.jsm. Some panorama data are saved under the key "panorama" in the session store.  N.B. Pending task is to update all regression tests.

	Raymond Lee [:raymondlee]
	Comment 24 • 12 years ago

Here are all files for the TabGroups API. Please review and apply patches (TabGroups.jsm, Panorama Update and Panorama Test Update) accordingly.

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 25 • 12 years ago

Comment on attachment 602277 [details] [diff] [review]
3. Panorama Tests Update v2

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

::: browser/components/tabview/tabgroups/test/browser_tabgroups_sessions.js
@@ +1,2 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */

This whole file should be included in part 1.

	Raymond Lee [:raymondlee]
	Comment 27 • 12 years ago

(In reply to Tim Taubert [:ttaubert] from comment #25)
> Comment on attachment 602277 [details] [diff] [review]
> 3. Panorama Tests Update v2
> 
> Review of attachment 602277 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/tabview/tabgroups/test/browser_tabgroups_sessions.js
> @@ +1,2 @@
> >  /* Any copyright is dedicated to the Public Domain.
> >     http://creativecommons.org/publicdomain/zero/1.0/ */
> 
> This whole file should be included in part 1.

Moved that to part 1.

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 28 • 12 years ago

Comment on attachment 602275 [details] [diff] [review]
1. TabGroups.jsm v2

Sorry, need to unmark this as obsolete temporarily. My whole review draft got lost and I'll hopefully get it back again.

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 29 • 12 years ago

Comment on attachment 602275 [details] [diff] [review]
1. TabGroups.jsm v2

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

I know there's a lot of code that I wrote a while ago but nevertheless I spotted some corrections on that code as well :)

::: browser/base/content/browser-tabview.js
@@ +39,5 @@
> +XPCOMUtils.defineLazyGetter(this, "TabGroups", function() {
> +  let tmp = {};
> +  Cu.import("resource:///modules/TabGroups.jsm", tmp);
> +  return tmp.TabGroups;
> +});

Better:

XPCOMUtils.defineLazyModuleGetter(this, "TabGroups", "resource:///modules/TabGroups.jsm");

::: browser/components/tabview/tabgroups/TabGroups.jsm
@@ +18,5 @@
> +
> +/**
> + * Methods for debugging.
> + */
> +function debug(aMsg) {

Please remove this function as we don't actively use it.

@@ +23,5 @@
> +  aMsg = ("TabGroups: " + aMsg).replace(/\S{80}/g, "$&\n");
> +  Services.console.logStringMessage(aMsg);
> +}
> +
> +function trace(aMsg) {

Please remove this function as we don't actively use it.

@@ +66,5 @@
> +  return extend(aObject, {
> +    attach: function EH_attach(aElement) {
> +      aEvents.forEach(function EH_attach_addListener(aEvent) {
> +        let callback = aObject["on" + aEvent];
> +        aElement.addEventListener(aEvent, callback, false);

I think createEventHandler() would be better off if it would be using handleEvent() like:

aElement.addEventListener(aEvent, aObject, false);

So the event handler implementations must provide a handleEvent() method that is the single function that handles all events.

@@ +86,5 @@
> +function createSubscribable(aObject) {
> +  return extend(aObject, {
> +    addSubscriber: function SUB_addSubscriber(aType, aCallback) {
> +      assert(aType, "invalid argument <aType>");
> +      assert(typeof aCallback == "function", "invalid argument <aCallback>");

Please remove those asserts.

@@ +102,5 @@
> +    },
> +
> +    removeSubscriber: function SUB_removeSubscriber(aType, aCallback) {
> +      assert(aType, "invalid argument <aType>");
> +      assert(typeof aCallback == "function", "invalid argument <aCallback>");

Please remove those asserts.

@@ +115,5 @@
> +      if (index > -1)
> +        subscribers.splice(index, 1);
> +    },
> +
> +    _sendToSubscribers: function SUB__sendToSubscribers(aType, aData) {

Please change the function names to not be abbreviated and to not contain double underscores.

SUB__sendToSubscribers -> Subscribable_sendToSubscribers

@@ +141,5 @@
> + */
> +let Storage = {
> +  KEY_TABGROUPS: "tabgroups",
> +
> +  _enabled: true,

We could just make this a 'public' property so that we don't need the enable() and disable() method. We'd just set .enabled = true/false directly.

@@ +193,5 @@
> +
> +    if (typeof aData != "undefined")
> +      groupsData[aGroupId] = aData;
> +    else if (aGroupId in groupsData)
> +      delete groupsData[aGroupId];

We should rather add a clearGroupData() method that is more expressive than not specifying the third parameter.

@@ +213,5 @@
> +    if (!this._enabled)
> +      return;
> +
> +    if (typeof aData == "undefined")
> +      aData = {};

We should rather add a clearTabData() method that sets the value to null. This is more expressive than not specifying the second parameter.

@@ +219,5 @@
> +    this._sessionStore.setTabValue(aTab, this.KEY_TABGROUPS,
> +                                   JSON.stringify(aData));
> +  },
> +
> +  getTabValue: function STO_getTabValue(aTab, aName) {

Is this used anywhere, do we need this?

@@ +228,5 @@
> +
> +    return null;
> +  },
> +
> +  setTabValue: function STO_setTabValue(aTab, aName, aValue) {

Is this used anywhere, do we need this?

@@ +418,5 @@
> +    if (index > -1) {
> +      let [aTab] = this._tabs.splice(index, 1);
> +      TabGroupMap.remove(aTab);
> +
> +      Storage.setTabData(aTab);

Storage.clearTabData();

@@ +438,5 @@
> +
> +    this.tabs.forEach(subject.removeTab, subject);
> +    this._sendToSubscribers("close");
> +
> +    Storage.setGroupData(this._window, this.id);

Please use Storage.clearGroupData();

@@ +445,5 @@
> +    this._tabs = null;
> +    this._window = null;
> +  },
> +
> +  _rearrange: function TG__rearrange() {

This should be 'public' as it's called by the TabMove event handler.

@@ +456,5 @@
> +    // if the _window is not set, the tabgroup object is already closed.
> +    if (!this._window)
> +      return;
> +
> +    Storage.setGroupData(this._window, this.id, this.storage);

if (this._window)
  Storage.setGroupData(this._window, this.id, this.storage);

@@ +497,5 @@
> +    }, this);
> +  },
> +
> +  _registerTabGroup: function TGS__registerTabGroup(aGroup) {
> +    let self = this;

Please use .bind(this).

@@ +652,5 @@
> +      callback.call(bind, windows.getNext());
> +  },
> +
> +  observe: function BW_observe(aSubject, aTopic, aData) {
> +    let self = this;

Please use .bind(this).

@@ +774,5 @@
> +
> +/**
> + * A class that handles session migration.
> + */
> +let Migration = {

I'm so sorry. The whole migration should of course be done by Panorama. Shouldn't be too hard to move it back now that it's more modularized. We should have a 'migration.js' that contains all this code. The migrator should be called when Panorama is initialized to detect if there's some legacy data lying around. If that's the case Panorama should parse it and use the TabGroups API to re-create those groups and tabs. Please add a nice tabview test for this to make sure it's working.

@@ +882,5 @@
> +Migration._init();
> +WindowTabGroupsMap._init();
> +TabEventHandler._init();
> +WindowEventHandler._init();
> +TabGroups._init();

Not a big issue but I'd rather like to have these ._init() calls directly below the object definition.

::: browser/components/tabview/tabgroups/test/browser_tabgroups_basics.js
@@ +28,5 @@
> +  // nothing should happen because group doesn't contain this tab anymore,
> +  group.removeTab(tab2);
> +  gBrowser.pinTab(tab2);
> +  group.addTab(tab2);
> +  is(group.tabs.length, 1, "group has one tab");

Please describe here, that the the group "still has one tab" because we don't allow adding pinned tabs to groups.

::: browser/components/tabview/tabgroups/test/browser_tabgroups_privatebrowsing.js
@@ +8,5 @@
> +
> +  registerCleanupFunction(function () {
> +    while (gBrowser.tabs.length > 1)
> +      gBrowser.removeTab(gBrowser.tabs[1]);
> +  });

We don't need that here because head.js already takes care of it.

@@ +23,5 @@
> +  group2.title = "second group";
> +  group2.addTab(gBrowser.addTab("about:mozilla"));
> +
> +  afterAllTabsLoaded(function () {
> +    info("++ setup");

This seems like debug output that I forgot to remove.

@@ +27,5 @@
> +    info("++ setup");
> +    checkSetup();
> +
> +    togglePrivateBrowsing(function () {
> +      info("++ entered pb mode");

Same here.

@@ +32,5 @@
> +      is(gBrowser.tabs.length, 1, "we have one tab");
> +      is(TabGroups.getGroups().length, 0, "there are no groups");
> +
> +      togglePrivateBrowsing(function () {
> +        info("++ left pb mode");

And the same here.

::: browser/components/tabview/tabgroups/test/browser_tabgroups_sessions.js
@@ +26,5 @@
> +function test() {
> +  waitForExplicitFinish();
> +
> +  setBrowserState(state1, function () {
> +    info("++ checking state1");

Debug output I forgot to remove.

@@ +34,5 @@
> +    checkGroup("group2-id", "group2-title", ["http://example.com/#2"]);
> +
> +    // give it a delay to ensure everything is set before setting browser state again
> +    setBrowserState(state2, function () {
> +      info("++ checking state2");

Same here.

@@ +42,5 @@
> +      checkGroup("group5-id", "group5-title", ["http://example.com/#5"]);
> +      checkGroup("group6-id", "group6-title", []);
> +
> +      setBrowserState(stateBackup, function () {
> +        info("++ checking stateBackup");

And here.

::: browser/components/tabview/tabgroups/test/head.js
@@ +13,5 @@
> +// the user to bring them to the foreground. We ensure this by resetting the
> +// related preference (see the "firefox.js" defaults file for details).
> +Services.prefs.setBoolPref("browser.sessionstore.restore_on_demand", false);
> +registerCleanupFunction(function () {
> +  Services.prefs.clearUserPref("browser.sessionstore.restore_on_demand");

Please add this to the registerCleanupFunction() call below.

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 30 • 12 years ago

Comment on attachment 602276 [details] [diff] [review]
2. Panorama Update v2

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

::: browser/components/tabview/tabgroups/TabGroups.jsm
@@ +736,1 @@
>  

This file doesn't belong here (it's part 1).

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 31 • 12 years ago

Comment on attachment 602275 [details] [diff] [review]
1. TabGroups.jsm v2

Marking as obsolete again.

	Raymond Lee [:raymondlee]
	Comment 32 • 12 years ago

(In reply to Tim Taubert [:ttaubert] from comment #29)
> Comment on attachment 602275 [details] [diff] [review]
> 1. TabGroups.jsm v2

Fixed all comments.

	Raymond Lee [:raymondlee]
	Comment 33 • 12 years ago

* Updated Panorama and added migration code

	Raymond Lee [:raymondlee]
	Comment 37 • 12 years ago

Those three patches didn't pass try.  I have fixed them now and should pass try ok.

https://tbpl.mozilla.org/?tree=Try&rev=0ee3ded8eecb

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 38 • 12 years ago

Comment on attachment 605031 [details] [diff] [review]
1. TabGroups.jsm v5

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

The patch looks good so far, I took some time to think about the custom pub/sub approach and described my thoughts below.

Another thing I thought about is that at the moment we allow clients to remove tabs from groups. We don't even enforce tabs to be in a group. I think we shouldn't allow "orphan tabs", we already removed them from Panorama for usability and simplicity and shouldn't re-introduce them. So what we should do is:

1) TabGroups._loadGroups() should create a group if there are none to be loaded. Every tab must be contained in a group. If there are orphan tabs, these should be added to the currently active group. browser_tabgroups_basics.js could take care of ensuring this.
2) The API shouldn't offer TabGroup.removeTab(), only .addTab() which already takes care of removing a tab from its previous group to allow moving tabs between groups. (Though we still need .removeTab() internally)
3) The TabEventHandler must also listen for TabUnpinned and add the unpinned tab to currently active group. This should be covered by browser_tabgroups_events.js.

::: browser/base/content/browser-tabview.js
@@ +35,5 @@
>  # the terms of any one of the MPL, the GPL or the LGPL.
>  #
>  # ***** END LICENSE BLOCK *****
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "TabGroups", "resource:///modules/TabGroups.jsm");

Looks like we don't need this here anymore?

::: browser/components/tabview/tabgroups/TabGroups.jsm
@@ +68,5 @@
> +
> +/**
> + * Extends the given object to become subscribable.
> + */
> +function createSubscribable(aObject) {

After thinking about the use of the pub-sub pattern here I don't think we should introduce our own. If we'd like to be accessible for add-ons and every other code in the future we should use the default observer service and dispatch the following notifications:

* tabgroup-created
* tabgroup-removed
* tabgroup-renamed

The subject is always the tab group itself.

* tabgroup-tab-added
* tabgroup-tab-removed

The subject here should be the tab that is added/removed. We should probably provide a TabGroups.getGroupForTab(aTab) to handle the last two notifications properly.

@@ +459,5 @@
> +
> +    if (eventType == "SSWindowStateReady") {
> +      TabGroups._loadTabGroups(window);
> +    } else if (eventType == "SSWindowClosing") {
> +      TabGroups._sendToSubscribers("window-closing", window);

We shouldn't send this notification as it's only use in a test so far.

@@ +622,5 @@
> +
> +/**
> + * A class that provides access to all tab groups.
> + */
> +let TabGroups = createSubscribable({

All the pseudo-private functions, starting with an underscore, are also visible to API clients outside of this JSM. Can we please create a TabGroupsInternal object that hosts these? So that we have clean internal/external API separation.

::: browser/components/tabview/tabgroups/test/browser_tabgroups_multiwindow.js
@@ +46,5 @@
> +}
> +
> +function testMoveGroup(win) {
> +  TabGroups.addSubscriber("window-closing", function onClosing(aWindow) {
> +    TabGroups.removeSubscriber("window-closing", onClosing);

As window-closing doesn't exist anymore we should rather listen for SSWindowClosing with capturing=true to be notified *before* the TabGroups module.

	Raymond Lee [:raymondlee]
	Comment 39 • 12 years ago

(In reply to Tim Taubert [:ttaubert] from comment #38)
> Another thing I thought about is that at the moment we allow clients to
> remove tabs from groups. We don't even enforce tabs to be in a group. I
> think we shouldn't allow "orphan tabs", we already removed them from
> Panorama for usability and simplicity and shouldn't re-introduce them. So
> what we should do is:
> 
> 1) TabGroups._loadGroups() should create a group if there are none to be
> loaded. Every tab must be contained in a group. If there are orphan tabs,
> these should be added to the currently active group.
> browser_tabgroups_basics.js could take care of ensuring this.

Tim: TabGroups module doesn't have any methods to get/set the currently active group, it seems to be the Panorama's job.  Do you mean we should introduce a way to  allow Panorama/other add-ons to set the active group via TabGroups API?


> browser/components/tabview/tabgroups/test/browser_tabgroups_multiwindow.js
> @@ +46,5 @@
> > +}
> > +
> > +function testMoveGroup(win) {
> > +  TabGroups.addSubscriber("window-closing", function onClosing(aWindow) {
> > +    TabGroups.removeSubscriber("window-closing", onClosing);
> 
> As window-closing doesn't exist anymore we should rather listen for
> SSWindowClosing with capturing=true to be notified *before* the TabGroups
> module.

I have tried to listen for SSWindowClosing with capturing=true in browser_tabgroups_multiwindow.js, however, the code in TabGroups module with capturing=false still get executed and finished before the code in test.

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 40 • 12 years ago

(In reply to Raymond Lee [:raymondlee] from comment #39)
> > 1) TabGroups._loadGroups() should create a group if there are none to be
> > loaded. Every tab must be contained in a group. If there are orphan tabs,
> > these should be added to the currently active group.
> > browser_tabgroups_basics.js could take care of ensuring this.
> 
> Tim: TabGroups module doesn't have any methods to get/set the currently
> active group, it seems to be the Panorama's job.  Do you mean we should
> introduce a way to  allow Panorama/other add-ons to set the active group via
> TabGroups API?

Right, I forgot about that. Active groups are something that only Panorama needs so we shouldn't add this to the TabGroups API. VerticalTabs or TreeStyleTabs shows all its tabs at once so there's no notion of an "active group". After all that's just a security measure to clean up broken state so we could as well just add orphan tabs to the first group.

	Raymond Lee [:raymondlee]
	Comment 41 • 12 years ago

(In reply to Tim Taubert [:ttaubert] from comment #38)
> Comment on attachment 605031 [details] [diff] [review]
> 1. TabGroups.jsm v5
> 
> ::: browser/base/content/browser-tabview.js
> @@ +35,5 @@
> >  # the terms of any one of the MPL, the GPL or the LGPL.
> >  #
> >  # ***** END LICENSE BLOCK *****
> >  
> > +XPCOMUtils.defineLazyModuleGetter(this, "TabGroups", "resource:///modules/TabGroups.jsm");
> 
> Looks like we don't need this here anymore?

If I remove this line and run tests, it would throw "leaked window property: TabGroups" when running the tests.  Therefore, I leave it in.

> 
> ::: browser/components/tabview/tabgroups/TabGroups.jsm
> @@ +68,5 @@
> > +
> > +/**
> > + * Extends the given object to become subscribable.
> > + */
> > +function createSubscribable(aObject) {
> 
> After thinking about the use of the pub-sub pattern here I don't think we
> should introduce our own. If we'd like to be accessible for add-ons and
> every other code in the future we should use the default observer service
> and dispatch the following notifications:
> 
> * tabgroup-created
> * tabgroup-removed
> * tabgroup-renamed
> 
> The subject is always the tab group itself.
> 
> * tabgroup-tab-added
> * tabgroup-tab-removed
> 
> The subject here should be the tab that is added/removed. We should probably
> provide a TabGroups.getGroupForTab(aTab) to handle the last two
> notifications properly.
> 

Done

> @@ +459,5 @@
> > +
> > +    if (eventType == "SSWindowStateReady") {
> > +      TabGroups._loadTabGroups(window);
> > +    } else if (eventType == "SSWindowClosing") {
> > +      TabGroups._sendToSubscribers("window-closing", window);
> 
> We shouldn't send this notification as it's only use in a test so far.

Removed.

> 
> @@ +622,5 @@
> > +
> > +/**
> > + * A class that provides access to all tab groups.
> > + */
> > +let TabGroups = createSubscribable({
> 
> All the pseudo-private functions, starting with an underscore, are also
> visible to API clients outside of this JSM. Can we please create a
> TabGroupsInternal object that hosts these? So that we have clean
> internal/external API separation.
> 

Done.

> :::
> browser/components/tabview/tabgroups/test/browser_tabgroups_multiwindow.js
> @@ +46,5 @@
> > +}
> > +
> > +function testMoveGroup(win) {
> > +  TabGroups.addSubscriber("window-closing", function onClosing(aWindow) {
> > +    TabGroups.removeSubscriber("window-closing", onClosing);
> 
> As window-closing doesn't exist anymore we should rather listen for
> SSWindowClosing with capturing=true to be notified *before* the TabGroups
> module.

If we listen for domwindowclosed, we still can't ensure that we're running code before session store.  We might need to fire a new topic for observers in the closing() in browser.js (http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/browser.js#507), in order to run code to move tabs from the closing window to another one and to retain group information. That is to prepare for panorama to handle tabs from all windows.

	Sonny Piers [:sonny]
	Comment 42 • 12 years ago

Any update?

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 43 • 12 years ago

Comment on attachment 614134 [details] [diff] [review]
1. TabGroups.jsm v6 WIP

Cancelling review as there's a new patch with a new approach "in the works". Need to find time to prepare it so that I can give this to someone with a little more time for this :/

	Justin Dolske [:Dolske]
	Comment 44 • 8 years ago

Panorama has been removed from Firefox 45, currently in Beta and scheduled for release on March 7th. As such, I'm closing all existing Panorama bugs.

If you are still using Panorama, you will see a deprecation message in Firefox 44, and when 45 is released your tab group data will be migrated to bookmarks, with a folder for each group. There are also a few addons offering similar functionality.

See https://support.mozilla.org/en-US/kb/tab-groups-removal for more info.

We're removing Panorama because it has extremely low usage (about 0.01% of users), and has a large number of bugs and usability issues. The cost of fixing all those issues is far too high to justify, and so we'll instead be focusing our time and energy on improving other parts of Firefox.