585855 - Test that the bookmark all tabs command is disabled when we have one visible and one hidden tab

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P2)

Description

[(no longer active)]

gBookmarkAllTabsHandler._updateCommandState is checking for the visible tabs.  We need to open a new tab, make sure that the bookmark all tabs command is enabled, make it hidden, and make sure that the command becomes disabled.

Comment 1

[Michael Yoshitaka Erlewine [:mitcho]]

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/f2b3832702b9

Committed, though it's not in the Makefile, and we don't currently pass it, as there is no notification to BATH__updateCommandState when we call gBrowser.showOnlyTheseTabs. Mardak and I have briefly discussed adding something like a TabVisibilityChange notification.

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: Patch to fix the test

* Send a notification to invoke BATH__updateCommandState when showOnlyTheseTabs is called.

Comment 3

[Ed Lee :Mardak]

Comment on attachment 464718 [details] [diff] [review]
Patch to fix the test

Do we need TabShow and TabHide events? notifying with tab-visibility-change is very unlike the other tab events.

>+++ b/browser/base/content/browser.js	Wed Aug 11 14:16:02 2010 +0800
>-      gFindBar.getElement("highlight").checked = false;      
>+      gFindBar.getElement("highlight").checked = false;
Best to not add changes not relevant to the bug/patch.

>+  uninit: function () {
I wonder why the other Tab* listeners don't need to be removed..

>+++ b/browser/base/content/test/browser_visibleTabs_bookmarkAllTabs.js	Wed Aug 11 14:16:02 2010 +0800
>+  let observe = function(subject, topic, data) {
>+    Services.obs.removeObserver(observe, "tab-visibility-change");
>+    setTimeout(function() {
What's this setTimeout for? Doing so makes us need the explicit finish() call.

Comment 4

[Raymond Lee [:raymondlee]]

(In reply to comment #3)
> Comment on attachment 464718 [details] [diff] [review]
> Patch to fix the test
> 
> Do we need TabShow and TabHide events? notifying with tab-visibility-change is
> very unlike the other tab events.

Would it be better to create a event "TabVisibilityChange" event and fire it in showOnlyTheseTabs()?

> 
> >+++ b/browser/base/content/browser.js	Wed Aug 11 14:16:02 2010 +0800
> >-      gFindBar.getElement("highlight").checked = false;      
> >+      gFindBar.getElement("highlight").checked = false;
> Best to not add changes not relevant to the bug/patch.

Oops, that's my editor which removes the trailing spaces.

> 
> >+  uninit: function () {
> I wonder why the other Tab* listeners don't need to be removed..
> 

Yes, I will add that in.  They are missing at the first place in mozilla-central code

> >+++ b/browser/base/content/test/browser_visibleTabs_bookmarkAllTabs.js	Wed Aug 11 14:16:02 2010 +0800
> >+  let observe = function(subject, topic, data) {
> >+    Services.obs.removeObserver(observe, "tab-visibility-change");
> >+    setTimeout(function() {
> What's this setTimeout for? Doing so makes us need the explicit finish() call.

the BATH__updateCommandState is called when it got notified by "tab-visibility-change" topic, hence a timeout is needed in our test to ensure the updateCommandState got called before we check the command element.

Comment 5

[Raymond Lee [:raymondlee]]

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/ed2484b982b9

The above commit uses observer notification to invoke BATH__updateCommandState when showOnlyTheseTabs is called.

The proposed change is to use "TabsVisibilityChange" event instead of the observer notification so it matches the other Tab* events e.g. TabOpen, TabClose, etc

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: Proposed Change

Use "TabsVisibilityChange" event instead of observer notification.

Comment 7

[Dão Gottwald [:dao]]

There are other places touching tab visibility, so these patches won't suffice.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 464718 [details] [diff] [review]
Patch to fix the test

I suggest you never set .hidden directly, introduce hideTab/showTab methods that set .hidden and dispatch TabHide/TabShow events there.

Comment 9

[Ian Gilman [:iangilman]]

Note that this test has been removed from browser/base/content/test/Makefile.in pending resolution. Please re-enable once it's fixed.

Comment 10

[Ian Gilman [:iangilman]]

Let's try to get this sorted out for the beta.

Comment 11

[Ian Gilman [:iangilman]]

Comment on attachment 464866 [details] [diff] [review]
Proposed Change

The change looks good to me.

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: Proposed Change v1

* Created two methods showTab and hideTab which fire TabShow and TabHide respectively

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 465562 [details] [diff] [review]
Proposed Change v1

>         <parameter name="aTabs"/>
>         <body>
>         <![CDATA[
>+          let self = this;
>           Array.forEach(this.tabs, function(tab) {
>-            tab.hidden = aTabs.indexOf(tab) == -1 && !tab.pinned && !tab.selected;
>+            if (aTabs.indexOf(tab) == -1 && !tab.pinned && !tab.selected)
>+              self.hideTab(tab);
>+            else
>+              self.showTab(tab);
>           });

Pass 'this' as the second argument to forEach, remove 'self'.

>+      <method name="showTab">
>+        <parameter name="aTab"/>
>+        <body>
>+        <![CDATA[
>+          if (aTab.hidden) {
>+            aTab.hidden = false;
>+            let event = document.createEvent("Events");
>+            event.initEvent("TabShow", true, false);
>+            this.tabContainer.dispatchEvent(event);
>+          }
>+        ]]>
>+        </body>
>+      </method>
>+
>+      <method name="hideTab">
>+        <parameter name="aTab"/>
>+        <body>
>+        <![CDATA[
>+          if (!aTab.hidden) {
>+            aTab.hidden = true;
>+            let event = document.createEvent("Events");
>+            event.initEvent("TabHide", true, false);
>+            this.tabContainer.dispatchEvent(event);
>+          }
>+        ]]>
>+        </body>
>+      </method>

Dispatch the events with the tab as the target.

TabShown/TabHidden would probably be better names.

Please check tabbrowser.xml for other places setting .hidden and make them use these methods instead.

The test changes seem unnecessary, as the events should be synchronous?!

Comment 14

[Ed Lee :Mardak]

(In reply to comment #13)
> The test changes seem unnecessary, as the events should be synchronous?!
Same question I had ;) But the bookmarkAllTabs callback is also listening on TabHide/TabShow, so us listening to it might happen to get called back before or after.

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #13)
> >         <parameter name="aTabs"/>
> >         <body>
> >         <![CDATA[
> >+          let self = this;
> >           Array.forEach(this.tabs, function(tab) {
> >-            tab.hidden = aTabs.indexOf(tab) == -1 && !tab.pinned && !tab.selected;
> >+            if (aTabs.indexOf(tab) == -1 && !tab.pinned && !tab.selected)
> >+              self.hideTab(tab);
> >+            else
> >+              self.showTab(tab);
> >           });
> 
> Pass 'this' as the second argument to forEach, remove 'self'.

As the third one really...

Comment 16

[Dão Gottwald [:dao]]

(In reply to comment #14)
> (In reply to comment #13)
> > The test changes seem unnecessary, as the events should be synchronous?!
> Same question I had ;) But the bookmarkAllTabs callback is also listening on
> TabHide/TabShow, so us listening to it might happen to get called back before
> or after.

If the test doesn't listen to the event in the first place, bookmark-all-tabs should be guaranteed to run between showOnlyTheseTabs and anything following that.

Comment 17

[Raymond Lee [:raymondlee]]

Attachment: Proposed Change v2

* Added showTab() and hideTab() and they would fire TabShown and TabHidden events.
* Updated tabbrowser.xml for other places setting .hidden

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 465590 [details] [diff] [review]
Proposed Change v2

Move !pinned && !selected checks from showOnlyTheseTabs to hideTab.

r=me with that

I'm still musing about what's really better, TabHidden or TabHide. While the former still seems more logical to me, the latter would be consistent with TabSelect/TabOpen/TabClose at least.

Vlad, care to chime in?

Comment 19

[Raymond Lee [:raymondlee]]

Attachment: Proposed Change v3

V3
* Moved !pinned && !selected checks from showOnlyTheseTabs to hideTab.
* Use TabShow/TabHide to make things consistent with
TabSelect/TabOpen/TabClose

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 465758 [details] [diff] [review]
Proposed Change v3

no strong preference on Hidden/Hide, though consistency is good.

Comment 21

[Ed Lee :Mardak]

Comment on attachment 465758 [details] [diff] [review]
Proposed Change v3

Raymond, dao has given you r+ on the previous patch, so no need to request review again after addressing the review comments.

Unless you feel that you've done a significant amount of change warranting an another review.

Comment 22

[Ed Lee :Mardak]

I've pushed this to maple and tests seem to pass (other than intermittent).

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 465758 [details] [diff] [review]
Proposed Change v3

I think we should get this in b4. Despite the bug summary, this adds a new tabbrowser API (and associated events), so extension authors should really be seeing this sooner rather than later.

Comment 24

[Ed Lee :Mardak]

http://hg.mozilla.org/mozilla-central/rev/62d9ac429278