Last Comment Bug 586555 - groupItemStorageSanity: if it's a check, let's really check it!
: groupItemStorageSanity: if it's a check, let's really check it!
Status: RESOLVED FIXED
[qa-][testday-20110930]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: P4 normal
: Firefox 8
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-11 20:02 PDT by Michael Yoshitaka Erlewine [:mitcho]
Modified: 2016-04-12 14:00 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (1.11 KB, patch)
2011-07-28 09:53 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Splinter Review
Patch for checkin (1.45 KB, patch)
2011-08-04 22:39 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Michael Yoshitaka Erlewine [:mitcho] 2010-08-11 20:02:05 PDT
In GroupItems.groupItemStorageSanity, we currently only check that the groupItemData has bounds. We should check for everything.
Comment 1 :Ehsan Akhgari (away Aug 1-5) 2010-08-13 13:00:42 PDT
Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".
Comment 2 Aza Raskin [:aza] 2010-10-12 13:27:32 PDT
This seems like something we can punt on? Mitcho, let me know if you disagree.
Comment 3 Michael Yoshitaka Erlewine [:mitcho] 2010-10-12 13:50:14 PDT
(In reply to comment #2)
> This seems like something we can punt on? Mitcho, let me know if you disagree.

Agreed.
Comment 4 Raymond Lee [:raymondlee] 2011-07-28 09:53:02 PDT
Created attachment 549149 [details] [diff] [review]
v1
Comment 5 Tim Taubert [:ttaubert] 2011-08-04 06:02:23 PDT
Comment on attachment 549149 [details] [diff] [review]
v1

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

Looks good.

r=me with the comments addressed.

::: browser/base/content/tabview/groupitems.js
@@ +2237,1 @@
>      if (!Utils.isRect(groupItemData.bounds)) {

Please add a check for .bounds here before we access it.

@@ +2238,5 @@
>        Utils.log('GroupItems.groupItemStorageSanity: bad bounds', groupItemData.bounds);
>        sane = false;
> +    } else if ((groupItemData.userSize &&
> +                !Utils.isPoint(groupItemData.userSize)) ||
> +              !groupItemData.id) {

Nit: please correct the indentation here.
Comment 6 Raymond Lee [:raymondlee] 2011-08-04 22:39:04 PDT
Created attachment 550947 [details] [diff] [review]
Patch for checkin

(In reply to Tim Taubert [:ttaubert] from comment #5)
> r=me with the comments addressed.
> 
> ::: browser/base/content/tabview/groupitems.js
> @@ +2237,1 @@
> >      if (!Utils.isRect(groupItemData.bounds)) {
> 
> Please add a check for .bounds here before we access it.
> 
Added

> @@ +2238,5 @@
> >        Utils.log('GroupItems.groupItemStorageSanity: bad bounds', groupItemData.bounds);
> >        sane = false;
> > +    } else if ((groupItemData.userSize &&
> > +                !Utils.isPoint(groupItemData.userSize)) ||
> > +              !groupItemData.id) {
> 
> Nit: please correct the indentation here.
Fixed

Passed Try http://tbpl.mozilla.org/?tree=Try&rev=ede4c63a8d9c
Comment 7 Tim Taubert [:ttaubert] 2011-08-07 12:21:12 PDT
http://hg.mozilla.org/integration/fx-team/rev/7715c3c492ae
Comment 8 Tim Taubert [:ttaubert] 2011-08-09 09:07:50 PDT
http://hg.mozilla.org/mozilla-central/rev/7715c3c492ae
Comment 9 Matt Evans [:mevans] 2011-09-01 15:57:54 PDT
What can QA do to verify this?
Comment 10 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-30 15:32:31 PDT
(In reply to Matt Evans [:mevans] from comment #9)
> What can QA do to verify this?

This seems pretty code-specific to me; I don't think there is much QA can do to verify this fix. If a testcase or steps can be provided please reset to qa+.

Thanks.

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