Last Comment Bug 691320 - typo in GroupItems.newTab from panorama
: typo in GroupItems.newTab from panorama
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 10
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on:
Blocks: 654721
  Show dependency tree / graph
 
Reported: 2011-10-03 07:10 PDT by tabmix.onemen
Modified: 2016-04-12 14:00 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

Attachments
v1 (1.26 KB, patch)
2011-10-03 20:12 PDT, Raymond Lee [:raymondlee] 		ttaubert: review+
Details | Diff | Splinter Review
Patch for checkin (1.51 KB, patch)
2011-10-04 02:04 PDT, Raymond Lee [:raymondlee] 		no flags Details | Diff | Splinter Review
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Description tabmix.onemen 2011-10-03 07:10:13 PDT 
in the function GroupItems.newTab there is a typo:

  let targetGroupItem;
  // find first visible non-app tab in the tabbar.
  gBrowser.visibleTabs.some(function(tab) {
    if (!tab.pinned && tab != tabItem.tab) {
      if (tab._tabViewTabItem) {
-       if (!tab._tabViewTabItem.parent && !tab._tabViewTabItem.parent.hidden) {
+       if (tab._tabViewTabItem.parent && !tab._tabViewTabItem.parent.hidden) {
          // the first visible tab belongs to a group, add the new tabItem to 
          // that group
          targetGroupItem = tab._tabViewTabItem.parent;
        }
      }
      return true;
    }
    return false;
  });
Comment 1 Raymond Lee [:raymondlee] 2011-10-03 19:49:52 PDT 
Hi reporter, which version did you find the typo?

Looking at the mozilla central, everything looks right?
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/groupitems.js#2387
Comment 2 Raymond Lee [:raymondlee] 2011-10-03 19:52:44 PDT 
(In reply to Raymond Lee [:raymondlee] from comment #1)
> Hi reporter, which version did you find the typo?
> 
> Looking at the mozilla central, everything looks right?
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/
> groupitems.js#2387

I see what you mean now. the if condition should use tab._tabViewTabItem.parent.
Comment 3 Raymond Lee [:raymondlee] 2011-10-03 20:12:57 PDT 
Created attachment 564447 [details] [diff] [review]
v1
Comment 4 Tim Taubert [:ttaubert] 2011-10-03 23:27:14 PDT 
Comment on attachment 564447 [details] [diff] [review]
v1

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

Thanks! Can you please push it to try to make sure we don't rely on that wrong condition?
Comment 5 Raymond Lee [:raymondlee] 2011-10-03 23:39:04 PDT 
Pushed to try and waiting for the results
https://tbpl.mozilla.org/?tree=Try&rev=5663f9b06052
Comment 6 Raymond Lee [:raymondlee] 2011-10-04 02:04:13 PDT 
Created attachment 564490 [details] [diff] [review]
Patch for checkin

Passed Try!
Comment 7 Tim Taubert [:ttaubert] 2011-10-04 02:18:37 PDT 
https://hg.mozilla.org/integration/fx-team/rev/bce52912af92
Comment 8 Rob Campbell [:rc] (:robcee) 2011-10-04 05:40:49 PDT 
https://hg.mozilla.org/mozilla-central/rev/bce52912af92
Note You need to log in before you can comment on or make changes to this bug.