Last Comment Bug 611328 - Replace Utils.trace() calls with Utils.assert().
: Replace Utils.trace() calls with Utils.assert().
Status: RESOLVED FIXED
[qa-][cleanup]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: P3 minor
: Firefox 8
Assigned To: Marco Túlio Costa
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-11 08:03 PST by Sean Dunn
Modified: 2016-04-12 14:00 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Simple bug. I felt like leaving the asserts inside the if's. (2.72 KB, patch)
2011-08-03 18:34 PDT, Marco Túlio Costa
no flags Details | Diff | Splinter Review
Removed the if's (2.96 KB, patch)
2011-08-04 16:38 PDT, Marco Túlio Costa
ttaubert: review+
Details | Diff | Splinter Review
[checked-in] Better formatted (3.05 KB, patch)
2011-08-09 19:51 PDT, Marco Túlio Costa
no flags Details | Diff | Splinter Review

Description Sean Dunn 2010-11-11 08:03:06 PST
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.
Comment 1 Kevin Hanes 2011-01-12 16:39:57 PST
Punting
Comment 2 Marco Túlio Costa 2011-08-03 18:34:12 PDT
Created attachment 550576 [details] [diff] [review]
Simple bug. I felt like leaving the asserts inside the if's.
Comment 3 Tim Taubert [:ttaubert] 2011-08-04 06:08:05 PDT
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)
Comment 4 Marco Túlio Costa 2011-08-04 16:38:01 PDT
Created attachment 550881 [details] [diff] [review]
Removed the if's

You were right. It's much cleaner now.
Comment 5 Tim Taubert [:ttaubert] 2011-08-05 06:43:52 PDT
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! :)
Comment 6 Marco Túlio Costa 2011-08-09 19:51:06 PDT
Created attachment 551984 [details] [diff] [review]
[checked-in] Better formatted

Thanks to Bonardo's blog post.
Comment 7 Tim Taubert [:ttaubert] 2011-08-12 01:55:44 PDT
Thanks for the patch, Marco!

http://hg.mozilla.org/integration/fx-team/rev/5776ff214937
Comment 8 Rob Campbell [:rc] (:robcee) 2011-08-12 07:03:31 PDT
Comment on attachment 551984 [details] [diff] [review]
[checked-in] Better formatted

http://hg.mozilla.org/mozilla-central/rev/5776ff214937

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