Last Comment Bug 628061 - Panorama button should reflect the number of groups before Panorama has launched
: Panorama button should reflect the number of groups before Panorama has launched
Status: VERIFIED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 7
Assigned To: Raymond Lee [:raymondlee]
:
:
Mentors:
Depends on:
Blocks: 660175
  Show dependency treegraph
 
Reported: 2011-01-22 15:49 PST by Michael Yoshitaka Erlewine [:mitcho]
Modified: 2016-04-12 14:00 PDT (History)
7 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (7.96 KB, patch)
2011-05-24 02:53 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Splinter Review
v2 (7.96 KB, patch)
2011-05-24 10:07 PDT, Raymond Lee [:raymondlee]
ian: review+
Details | Diff | Splinter Review
Patch for checkin (8.04 KB, patch)
2011-05-25 18:40 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v3 (1.89 KB, patch)
2011-06-03 10:34 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Splinter Review
v4 (3.12 KB, patch)
2011-06-03 12:46 PDT, Raymond Lee [:raymondlee]
ian: review+
Details | Diff | Splinter Review
Patch for checkin (3.40 KB, patch)
2011-06-06 20:56 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Michael Yoshitaka Erlewine [:mitcho] 2011-01-22 15:49:00 PST
The Panorama button currently only accurately reflects the number of groups after Panorama has been loaded. Fix it.
Comment 1 Raymond Lee [:raymondlee] 2011-05-24 02:53:02 PDT
Created attachment 534700 [details] [diff] [review]
v1
Comment 2 Tim Taubert [:ttaubert] 2011-05-24 04:48:20 PDT
Comment on attachment 534700 [details] [diff] [review]
v1

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

Looks good!

::: browser/base/content/browser-tabview.js
@@ +104,5 @@
> +        data = sessionstore.getWindowValue(window, this.GROUPS_IDENTIFIER);
> +        if (data) {
> +          let parsedData = JSON.parse(data);
> +          this.updateGroupNumberBroadcaster(parsedData.totalNumber);
> +        }

We should put .getWindowValue() and JSON.parse() in a try-catch block.

@@ +391,5 @@
> +  // Updates the group number broadcaster.
> +  updateGroupNumberBroadcaster: function TabView_updateGroupNumberBroadcaster(number) {
> +    let groupsNumber = document.getElementById("tabviewGroupsNumber");
> +    groupsNumber.setAttribute("groups", number);
> +    dump("set " + number + "\n")

Nit: leftover debug statement.
Comment 3 Raymond Lee [:raymondlee] 2011-05-24 10:07:57 PDT
Created attachment 534818 [details] [diff] [review]
v2
Comment 4 Ian Gilman [:iangilman] 2011-05-25 11:01:03 PDT
Comment on attachment 534818 [details] [diff] [review]
v2

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

R+ with those comments addressed

::: browser/base/content/browser-tabview.js
@@ +106,5 @@
> +          if (data)
> +            existingData = JSON.parse(data);
> +        } catch (e) {
> +          // getWindowValue will fail if the property doesn't exist
> +        }

Doesn't look like we need this getWindowValue at all; please remove.

@@ +113,5 @@
> +        if (data) {
> +          let parsedData = JSON.parse(data);
> +          this.updateGroupNumberBroadcaster(parsedData.totalNumber);
> +        }
> +

The first time this happens on a new upgrade, parsedData.totalNumber will be undefined. Rather than passing undefined into updateGroupNumberBroadcaster, pass: 

  parsedData.totalNumber || 0
Comment 5 Raymond Lee [:raymondlee] 2011-05-25 18:40:20 PDT
Created attachment 535251 [details] [diff] [review]
Patch for checkin
Comment 6 Tim Taubert [:ttaubert] 2011-05-26 02:04:37 PDT
http://hg.mozilla.org/mozilla-central/rev/32862966ebaf
Comment 7 Ian Gilman [:iangilman] 2011-05-26 09:49:45 PDT
I just realized something... aren't we already saving the number of groups in the group data? Why save that number again in a second place for this patch?

Sorry for not thinking of that sooner...
Comment 8 Raymond Lee [:raymondlee] 2011-05-26 10:47:09 PDT
(In reply to comment #7)
> I just realized something... aren't we already saving the number of groups
> in the group data? Why save that number again in a second place for this
> patch?
> 
> Sorry for not thinking of that sooner...

We can actually use Storage.readGroupItemData(win) to get all group items, and then count the number of groups.  However, user might have many different groups (huge data set) so i don't think it worth to parse all the group items data from string to JSON for just getting the number of groups. Since the user might not even enter into Panorama for the whole browsing section.

What do you think?
Comment 9 Tim Taubert [:ttaubert] 2011-05-27 01:53:01 PDT
(In reply to comment #8)
> We can actually use Storage.readGroupItemData(win) to get all group items,
> and then count the number of groups.  However, user might have many
> different groups (huge data set) so i don't think it worth to parse all the
> group items data from string to JSON for just getting the number of groups.
> Since the user might not even enter into Panorama for the whole browsing
> section.

Yeah, that's what I thought about when reading the patch. IMHO it might be worth the additional property to not have to parse all groupItems on startup.
Comment 10 Tim Taubert [:ttaubert] 2011-05-27 02:08:48 PDT
bugspam
Comment 11 Ian Gilman [:iangilman] 2011-05-27 09:26:29 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > We can actually use Storage.readGroupItemData(win) to get all group items,
> > and then count the number of groups.  However, user might have many
> > different groups (huge data set) so i don't think it worth to parse all the
> > group items data from string to JSON for just getting the number of groups.
> > Since the user might not even enter into Panorama for the whole browsing
> > section.
> 
> Yeah, that's what I thought about when reading the patch. IMHO it might be
> worth the additional property to not have to parse all groupItems on startup.

Cool, good point. Just making sure it was considered.
Comment 12 George Carstoiu 2011-06-02 04:00:01 PDT
Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110601 Firefox/7.0a1

I am still able to see this issue on Mac OS X 10.6 and Win7 - the number of opened groups (one group on a clean profile) is correctly displayed only after Panorama is accessed for the first time.

Also logged Bug 661501 - No panorama icon in List all tabs drop down menu
Comment 13 Raymond Lee [:raymondlee] 2011-06-02 04:53:10 PDT
(In reply to comment #12)
> Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110601 Firefox/7.0a1
> 
> I am still able to see this issue on Mac OS X 10.6 and Win7 - the number of
> opened groups (one group on a clean profile) is correctly displayed only
> after Panorama is accessed for the first time.
>

It seems to make senese that we display zero groups before user enters Panorama for the first time.  When user enters Panorama for the first time, a group is then created.

@tim/ian: any comments?
 
> Also logged Bug 661501 - No panorama icon in List all tabs drop down menu
Comment 14 Ian Gilman [:iangilman] 2011-06-02 09:36:43 PDT
I think there's some confusion between the two different kinds of "entering Panorama for the first time": there's the first time in a session (when Panorama has already been set up for that particular window in a previous session), and there's the first time in a window (where Panorama has never been set up for that window). I believe this bug is about the former "first", while comment 12 is about the latter "first". 

I agree with Raymond that it's reasonable in a window that's never had Panorama set up to indicate that by displaying 0 groups in the Panorama button. On the other hand, I think ultimately we want to get to a state where, at least as far as the UI is concerned, it feels like Panorama is "always just there", not something you have to initialize; in that case, having one group lit when Panorama hasn't been initialized for a window makes sense. 

So I guess it depends on whether we feel like it's still worth indicating to the user that they've never run Panorama on a specific window.
Comment 15 Tim Taubert [:ttaubert] 2011-06-03 02:47:25 PDT
(In reply to comment #14)
> I agree with Raymond that it's reasonable in a window that's never had
> Panorama set up to indicate that by displaying 0 groups in the Panorama
> button. On the other hand, I think ultimately we want to get to a state
> where, at least as far as the UI is concerned, it feels like Panorama is
> "always just there", not something you have to initialize; in that case,
> having one group lit when Panorama hasn't been initialized for a window
> makes sense.

+1 :)

> So I guess it depends on whether we feel like it's still worth indicating to
> the user that they've never run Panorama on a specific window.

I don't think we should make the user think too much about Panorama and espcecially its lazy-loading behavior. So switching the default value to "1" seems totally fine to me.
Comment 16 Raymond Lee [:raymondlee] 2011-06-03 10:34:14 PDT
Created attachment 537177 [details] [diff] [review]
v3

This patch would set the number of group to 1 if user hasn't activated Panorama before.
Comment 17 Tim Taubert [:ttaubert] 2011-06-03 10:59:24 PDT
Comment on attachment 537177 [details] [diff] [review]
v3

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

Looks good! Can you please include this in browser_tabview_bug628061.js?
Comment 18 Raymond Lee [:raymondlee] 2011-06-03 12:46:57 PDT
Created attachment 537210 [details] [diff] [review]
v4
Comment 19 Ian Gilman [:iangilman] 2011-06-06 10:06:30 PDT
Comment on attachment 537210 [details] [diff] [review]
v4

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

Thanks!
Comment 20 Caspy7 2011-06-06 14:11:05 PDT
If the default is 1, then is there any instance in which the "zero" state icon would be displayed? If not then we might as well chuck the default icon.

On that note I would vote that it default to a zero state.  Perhaps there's at least merit in the icon being consistent with marketing.
Comment 21 Raymond Lee [:raymondlee] 2011-06-06 20:49:18 PDT
(In reply to comment #20)
> If the default is 1, then is there any instance in which the "zero" state
> icon would be displayed? If not then we might as well chuck the default icon.
> 

Yes, when there are some orphan tabs. However, bug 654721 should remove the "zero" state.

> On that note I would vote that it default to a zero state.  Perhaps there's
> at least merit in the icon being consistent with marketing.
Comment 22 Raymond Lee [:raymondlee] 2011-06-06 20:56:45 PDT
Created attachment 537730 [details] [diff] [review]
Patch for checkin
Comment 23 Raymond Lee [:raymondlee] 2011-06-07 02:02:34 PDT
Passed Try
http://tbpl.mozilla.org/?tree=Try&rev=f7cff306f927
Comment 24 Tim Taubert [:ttaubert] 2011-06-07 02:32:03 PDT
http://hg.mozilla.org/mozilla-central/rev/716fe9b93db5
Comment 25 George Carstoiu 2011-06-10 02:17:13 PDT
Verified issue on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0a1) Gecko/20110609 Firefox/7.0a1 and also on Win 7 and Win XP. 

Issue is no longer present - setting status to Verified Fixed.

Note You need to log in before you can comment on or make changes to this bug.