Closed
Bug 611328
Opened 14 years ago
Closed 13 years ago
Replace Utils.trace() calls with Utils.assert().
Categories
(Firefox Graveyard :: Panorama, defect, P3)
Firefox Graveyard
Panorama
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 8
People
(Reporter: seanedunn, Assigned: mmarcottulio)
Details
(Whiteboard: [qa-][cleanup])
Attachments
(1 file, 2 obsolete files)
3.05 KB,
patch
|
Details | Diff | Splinter Review |
In bug 567029, dietrich caught some areas for code cleanup: not related to this patch really, but those trace() calls should be assert()s, no? and if we don't need to throw an error, then we certainly shouldn't ship with those. please file a followup for it, either way.
Updated•14 years ago
|
Whiteboard: [qa-][good first bug][cleanup]
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #550576 -
Flags: review?(tim.taubert)
Comment 3•13 years ago
|
||
Comment on attachment 550576 [details] [diff] [review] Simple bug. I felt like leaving the asserts inside the if's. Review of attachment 550576 [details] [diff] [review]: ----------------------------------------------------------------- Hey Marco, thanks for the patch! If you address my comments below I'll be happy to review your patch again :) ::: browser/base/content/tabview/groupitems.js @@ +527,2 @@ > return; > } Let's just make that: Utils.assert(Utils.isRect(inRect), 'GroupItem.setBounds: rect is not a real rectangle!'); So we can remove the if clause and the return statement because we'll just assert when there's a wrong argument (which is consistent with how we usually handle that). ::: browser/base/content/tabview/tabitems.js @@ +345,5 @@ > // Possible options: > // force - true to always update the DOM even if the bounds haven't changed; default false > setBounds: function TabItem_setBounds(inRect, immediately, options) { > if (!Utils.isRect(inRect)) { > + Utils.assert(false, 'TabItem.setBounds: rect is not a real rectangle!'); (see above) @@ +453,5 @@ > > rect = this.getBounds(); // ensure that it's a <Rect> > > if (!Utils.isRect(this.bounds)) > + Utils.assert(false, 'TabItem.setBounds: this.bounds is not a real rectangle!'); (see above)
Attachment #550576 -
Flags: review?(tim.taubert)
Assignee | ||
Comment 4•13 years ago
|
||
You were right. It's much cleaner now.
Attachment #550576 -
Attachment is obsolete: true
Attachment #550881 -
Flags: review?(tim.taubert)
Comment 5•13 years ago
|
||
Comment on attachment 550881 [details] [diff] [review] Removed the if's Review of attachment 550881 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks! Now we need the patch to be ready for check-in... http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed It would be great if you can make the patch ready, upload it here and mark the bug with the "checkin-needed" keyword. Looking forward to see that fixed! :)
Attachment #550881 -
Flags: review?(tim.taubert) → review+
Assignee | ||
Comment 6•13 years ago
|
||
Thanks to Bonardo's blog post.
Attachment #550881 -
Attachment is obsolete: true
Comment 7•13 years ago
|
||
Thanks for the patch, Marco! http://hg.mozilla.org/integration/fx-team/rev/5776ff214937
Assignee: nobody → mmarcottulio
Status: NEW → ASSIGNED
Whiteboard: [qa-][good first bug][cleanup] → [qa-][cleanup][fixed-in-fx-team]
Version: unspecified → Trunk
Comment 8•13 years ago
|
||
Comment on attachment 551984 [details] [diff] [review] [checked-in] Better formatted http://hg.mozilla.org/mozilla-central/rev/5776ff214937
Attachment #551984 -
Attachment description: Better formatted [for checkin] → [checked-in] Better formatted
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Whiteboard: [qa-][cleanup][fixed-in-fx-team] → [qa-][cleanup]
Target Milestone: Future → Firefox 8
Updated•8 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•