Closed Bug 585855 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Tabbed Browser, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 4.0b4

People

(Reporter: ehsan, Assigned: raymondlee)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Assignee: nobody → raymond
Assignee: raymond → mitcho
Status: NEW → ASSIGNED
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.
Attached patch Patch to fix the test (obsolete) — Splinter Review
* Send a notification to invoke BATH__updateCommandState when showOnlyTheseTabs is called.
Attachment #464718 - Flags: review?(ian)
Attachment #464718 - Flags: feedback?(edilee)
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.
Attachment #464718 - Flags: feedback?(edilee) → feedback?(dao)
(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.
Depends on: 584263
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
OS: Mac OS X → Windows 7
Attached patch Proposed Change (obsolete) — Splinter Review
Use "TabsVisibilityChange" event instead of observer notification.
Attachment #464866 - Flags: review?(ian)
Attachment #464866 - Flags: feedback?(edilee)
Depends on: 582116
There are other places touching tab visibility, so these patches won't suffice.
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.
Attachment #464718 - Flags: review?(ian)
Attachment #464718 - Flags: review-
Attachment #464718 - Flags: feedback?(dao)
Note that this test has been removed from browser/base/content/test/Makefile.in pending resolution. Please re-enable once it's fixed.
Depends on: 574217
Let's try to get this sorted out for the beta.
Assignee: mitcho → raymond
Blocks: 586788
Comment on attachment 464866 [details] [diff] [review]
Proposed Change

The change looks good to me.
Attachment #464866 - Flags: review?(ian) → review+
Whiteboard: b4
Priority: -- → P2
Attached patch Proposed Change v1 (obsolete) — Splinter Review
* Created two methods showTab and hideTab which fire TabShow and TabHide respectively
Attachment #464718 - Attachment is obsolete: true
Attachment #464866 - Attachment is obsolete: true
Attachment #465562 - Flags: review?(dao)
Attachment #465562 - Flags: feedback?(dao)
Attachment #464866 - Flags: feedback?(edilee)
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?!
Attachment #465562 - Flags: review?(dao)
Attachment #465562 - Flags: review-
Attachment #465562 - Flags: feedback?(dao)
Component: TabCandy → Tabbed Browser
Product: Mozilla Labs → Firefox
QA Contact: tabcandy → tabbed.browser
Target Milestone: -- → ---
(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.
(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...
(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.
Attached patch Proposed Change v2 (obsolete) — Splinter Review
* Added showTab() and hideTab() and they would fire TabShown and TabHidden events.
* Updated tabbrowser.xml for other places setting .hidden
Attachment #465562 - Attachment is obsolete: true
Attachment #465590 - Flags: review?(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?
Attachment #465590 - Flags: superreview?(vladimir)
Attachment #465590 - Flags: review?(dao)
Attachment #465590 - Flags: review+
V3
* Moved !pinned && !selected checks from showOnlyTheseTabs to hideTab.
* Use TabShow/TabHide to make things consistent with
TabSelect/TabOpen/TabClose
Attachment #465758 - Flags: superreview?(vladimir)
Attachment #465758 - Flags: review?(dao)
Comment on attachment 465758 [details] [diff] [review]
Proposed Change v3

no strong preference on Hidden/Hide, though consistency is good.
Attachment #465758 - Flags: superreview?(vladimir) → superreview+
Attachment #465590 - Attachment is obsolete: true
Attachment #465590 - Flags: superreview?(vladimir)
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.
Attachment #465758 - Flags: review?(dao)
Blocks: 584263
No longer depends on: 584263
Keywords: checkin-needed
OS: Windows 7 → Windows XP
Component: Tabbed Browser → TabCandy
QA Contact: tabbed.browser → tabcandy
I've pushed this to maple and tests seem to pass (other than intermittent).
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.
Attachment #465758 - Flags: approval2.0?
Attachment #465758 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/62d9ac429278
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
OS: Windows XP → All
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: b4
Target Milestone: --- → Firefox 4.0b4
Component: TabCandy → Tabbed Browser
QA Contact: tabcandy → tabbed.browser
You need to log in before you can comment on or make changes to this bug.