628061 - Panorama button should reflect the number of groups before Panorama has launched

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

The Panorama button currently only accurately reflects the number of groups after Panorama has been loaded. Fix it.

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: v2

Comment 4

[Ian Gilman [:iangilman]]

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]]

Attachment: Patch for checkin

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/32862966ebaf

Comment 7

[Ian Gilman [:iangilman]]

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]]

(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] (inactive)]

(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] (inactive)]

bugspam

Comment 11

[Ian Gilman [:iangilman]]

(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]

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]]

(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]]

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] (inactive)]

(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]]

Attachment: v3

This patch would set the number of group to 1 if user hasn't activated Panorama before.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: v4

Comment 19

[Ian Gilman [:iangilman]]

Comment on attachment 537210 [details] [diff] [review]
v4

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

Thanks!

Comment 20

[Caspy7]

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]]

(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]]

Attachment: Patch for checkin

Comment 23

[Raymond Lee [:raymondlee]]

Passed Try
http://tbpl.mozilla.org/?tree=Try&rev=f7cff306f927

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/716fe9b93db5

Comment 25

[George Carstoiu]

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.