Re: [opensource-dev] Review Request: Restructure loops that use breaks without need (reviewboard test)

[Ricky]

Hmm, it appears to have "background-image:
url('http://codereview.secondlife.comrb/image=s/review_request_box_top_bg.png');"
in the style attribute for one of the tables.  (Don't know why tables
were used instead of divs like the site is built...)

Beyond the fact that it isn't necessary, the URL is also wrong.  The
correct URL is:
https://codereview.secondlife.com/media/rb/images/review_request_box_top_bg.png

Ricky
Cron Stardust

On Thu, Dec 2, 2010 at 12:05 PM, Gigs  wrote:
> I'm getting "To protect your privacy, Thunderbird has blocked remote
> content."   Why do the message include external images?  As well because
> the "From" field is a person and not the list, I would have to whitelist
> every contributor to get rid of the message.
>
> On 12/02/2010 12:05 PM, Thickbrick Sleaford wrote:
>>
>> This is an automatically generated e-mail. To reply, visit:
>> http://codereview.secondlife.com/r/2/
>>
>>
>> indra/newview/llappviewer.cpp
>> 
>> (Diff revision 1)
>>
>> bool LLAppViewer::mainLoop()
>>
>> 1184
>>
>>                               const  F64  max_idle_time  =  
>> llmin(.005*10.0*gFrameTimeSeconds,  0.005);  // 5 ms a second
>>
>>       1184
>>
>>                               const  F64  max_idle_time  =  
>> llmin(.005*10.0*gFrameTimeSeconds,  0.005);  // 5 ms a second
>>
>> Somewhat tangential to this patch: this line is wrong. It should use 
>> gFrameIntervalSeconds, not gFarmeTimeSeconds. As it is, max_idle_time will 
>> always be clamped to 0.005.
>>
>> The comment is also wrong/outdated. It should be something like"50 
>> ms/second, up to 5 ms/frame."
>>
>>
>> - Thickbrick
>>
>>
>> On December 1st, 2010, 7:57 p.m., Oz Linden wrote:
>>
>> Review request for Viewer.
>> By Oz Linden.
>>
>> /Updated 2010-12-01 19:57:24/
>>
>>
>>   Description
>>
>> This review is mostly a first test of reviewboard.
>>
>> I do have an esthetic dislike for the'break'  statement anywhere but as the 
>> end of a case, so I chose to change some instances of break usage that were 
>> not justified by any extreme need.
>>
>>
>>   Testing
>>
>> None at all... have not even compiled it yet.
>>
>> *Bugs: * storm-606 
>>
>>
>>   Diffs
>>
>>     * indra/newview/llappviewer.cpp (bf98b026bcb1)
>>
>> View Diff 
>>
>>
>>
>> ___
>> Policies and (un)subscribe information available here:
>> http://wiki.secondlife.com/wiki/OpenSource-Dev
>> Please read the policies before posting to keep unmoderated posting 
>> privileges
>
> ___
> Policies and (un)subscribe information available here:
> http://wiki.secondlife.com/wiki/OpenSource-Dev
> Please read the policies before posting to keep unmoderated posting privileges
>
___
Policies and (un)subscribe information available here:
http://wiki.secondlife.com/wiki/OpenSource-Dev
Please read the policies before posting to keep unmoderated posting privileges

Re: [opensource-dev] Review Request: Restructure loops that use breaks without need (reviewboard test)

[Gigs]

I'm getting "To protect your privacy, Thunderbird has blocked remote 
content."   Why do the message include external images?  As well because 
the "From" field is a person and not the list, I would have to whitelist 
every contributor to get rid of the message.

On 12/02/2010 12:05 PM, Thickbrick Sleaford wrote:
>
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/2/
>
>
> indra/newview/llappviewer.cpp
> 
> (Diff revision 1)
>
> bool LLAppViewer::mainLoop()
>
> 1184  
>
>   const  F64  max_idle_time  =  
> llmin(.005*10.0*gFrameTimeSeconds,  0.005);  // 5 ms a second
>
>   1184
>
>   const  F64  max_idle_time  =  
> llmin(.005*10.0*gFrameTimeSeconds,  0.005);  // 5 ms a second
>
> Somewhat tangential to this patch: this line is wrong. It should use 
> gFrameIntervalSeconds, not gFarmeTimeSeconds. As it is, max_idle_time will 
> always be clamped to 0.005.
>
> The comment is also wrong/outdated. It should be something like"50 ms/second, 
> up to 5 ms/frame."
>
>
> - Thickbrick
>
>
> On December 1st, 2010, 7:57 p.m., Oz Linden wrote:
>
> Review request for Viewer.
> By Oz Linden.
>
> /Updated 2010-12-01 19:57:24/
>
>
>   Description
>
> This review is mostly a first test of reviewboard.
>
> I do have an esthetic dislike for the'break'  statement anywhere but as the 
> end of a case, so I chose to change some instances of break usage that were 
> not justified by any extreme need.
>
>
>   Testing
>
> None at all... have not even compiled it yet.
>
> *Bugs: * storm-606 
>
>
>   Diffs
>
> * indra/newview/llappviewer.cpp (bf98b026bcb1)
>
> View Diff 
>
>
>
> ___
> Policies and (un)subscribe information available here:
> http://wiki.secondlife.com/wiki/OpenSource-Dev
> Please read the policies before posting to keep unmoderated posting privileges

___
Policies and (un)subscribe information available here:
http://wiki.secondlife.com/wiki/OpenSource-Dev
Please read the policies before posting to keep unmoderated posting privileges

Re: [opensource-dev] Review Request: Restructure loops that use breaks without need (reviewboard test)

[Thickbrick Sleaford]

---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/2/#review13
---



indra/newview/llappviewer.cpp


Somewhat tangential to this patch: this line is wrong. It should use 
gFrameIntervalSeconds, not gFarmeTimeSeconds. As it is, max_idle_time will 
always be clamped to 0.005.

The comment is also wrong/outdated. It should be something like "50 
ms/second, up to 5 ms/frame."


- Thickbrick


On 2010-12-01 19:57:24, Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/2/
> ---
> 
> (Updated 2010-12-01 19:57:24)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This review is mostly a first test of reviewboard.
> 
> I do have an esthetic dislike for the 'break' statement anywhere but as the 
> end of a case, so I chose to change some instances of break usage that were 
> not justified by any extreme need.
> 
> 
> This addresses bug storm-606.
> http://jira.secondlife.com/browse/storm-606
> 
> 
> Diffs
> -
> 
>   indra/newview/llappviewer.cpp bf98b026bcb1 
> 
> Diff: http://codereview.secondlife.com/r/2/diff
> 
> 
> Testing
> ---
> 
> None at all... have not even compiled it yet.
> 
> 
> Thanks,
> 
> Oz
> 
>

___
Policies and (un)subscribe information available here:
http://wiki.secondlife.com/wiki/OpenSource-Dev
Please read the policies before posting to keep unmoderated posting privileges

Re: [opensource-dev] Review Request: Restructure loops that use breaks without need (reviewboard test)

[Boroondas Gupte]

> On 2010-12-02 04:52:10, Boroondas Gupte wrote:
> >
> 
> Boroondas Gupte wrote:
> EDITED TO ADD: (apparently Review Board can't handle comments on comments 
> on lines (sic!) and new comments on lines at the same time)
> "The Review-Board diff view conveniently highlights them in red, so they 
> are hard to miss ;-)"
> Of course, Review Board can't detect all whitespace mistakes:

Ah, I guess I should have read 
http://www.reviewboard.org/docs/manual/1.5/users/reviews/reviewing-diffs/#reading-existing-comments
 , first: "It’s important to note that this is meant to be used as a reference 
to see if other people have already said what you plan to say. The comment box 
is not the place to reply to those comments. Instead, you can click the Reply 
link next to the particular comment, which will take you back to the review 
request page and open a reply box."


- Boroondas


---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/2/#review10
---


On 2010-12-01 19:57:24, Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/2/
> ---
> 
> (Updated 2010-12-01 19:57:24)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This review is mostly a first test of reviewboard.
> 
> I do have an esthetic dislike for the 'break' statement anywhere but as the 
> end of a case, so I chose to change some instances of break usage that were 
> not justified by any extreme need.
> 
> 
> This addresses bug storm-606.
> http://jira.secondlife.com/browse/storm-606
> 
> 
> Diffs
> -
> 
>   indra/newview/llappviewer.cpp bf98b026bcb1 
> 
> Diff: http://codereview.secondlife.com/r/2/diff
> 
> 
> Testing
> ---
> 
> None at all... have not even compiled it yet.
> 
> 
> Thanks,
> 
> Oz
> 
>

___
Policies and (un)subscribe information available here:
http://wiki.secondlife.com/wiki/OpenSource-Dev
Please read the policies before posting to keep unmoderated posting privileges

Re: [opensource-dev] Review Request: Restructure loops that use breaks without need (reviewboard test)

[Boroondas Gupte]

> On 2010-12-02 04:52:10, Boroondas Gupte wrote:
> >

EDITED TO ADD: (apparently Review Board can't handle comments on comments on 
lines (sic!) and new comments on lines at the same time)
"The Review-Board diff view conveniently highlights them in red, so they are 
hard to miss ;-)"
Of course, Review Board can't detect all whitespace mistakes:


- Boroondas


---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/2/#review10
---


On 2010-12-01 19:57:24, Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/2/
> ---
> 
> (Updated 2010-12-01 19:57:24)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This review is mostly a first test of reviewboard.
> 
> I do have an esthetic dislike for the 'break' statement anywhere but as the 
> end of a case, so I chose to change some instances of break usage that were 
> not justified by any extreme need.
> 
> 
> This addresses bug storm-606.
> http://jira.secondlife.com/browse/storm-606
> 
> 
> Diffs
> -
> 
>   indra/newview/llappviewer.cpp bf98b026bcb1 
> 
> Diff: http://codereview.secondlife.com/r/2/diff
> 
> 
> Testing
> ---
> 
> None at all... have not even compiled it yet.
> 
> 
> Thanks,
> 
> Oz
> 
>

___
Policies and (un)subscribe information available here:
http://wiki.secondlife.com/wiki/OpenSource-Dev
Please read the policies before posting to keep unmoderated posting privileges

Re: [opensource-dev] Review Request: Restructure loops that use breaks without need (reviewboard test)

[Boroondas Gupte]

---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/2/#review10
---



indra/newview/llappviewer.cpp


e.g. here, some lines are indented by spaces and others by tabs


- Boroondas


On 2010-12-01 19:57:24, Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/2/
> ---
> 
> (Updated 2010-12-01 19:57:24)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This review is mostly a first test of reviewboard.
> 
> I do have an esthetic dislike for the 'break' statement anywhere but as the 
> end of a case, so I chose to change some instances of break usage that were 
> not justified by any extreme need.
> 
> 
> This addresses bug storm-606.
> http://jira.secondlife.com/browse/storm-606
> 
> 
> Diffs
> -
> 
>   indra/newview/llappviewer.cpp bf98b026bcb1 
> 
> Diff: http://codereview.secondlife.com/r/2/diff
> 
> 
> Testing
> ---
> 
> None at all... have not even compiled it yet.
> 
> 
> Thanks,
> 
> Oz
> 
>

___
Policies and (un)subscribe information available here:
http://wiki.secondlife.com/wiki/OpenSource-Dev
Please read the policies before posting to keep unmoderated posting privileges

Re: [opensource-dev] Review Request: Restructure loops that use breaks without need (reviewboard test)

[Oz Linden (Scott Lawrence)]

>
> I just want to point out (as a reminder to other list members)
> that the reviewboard is for code contributors only, that is
>
> "In order to post reviews or comment on them in this system,
> you must have executed a Contribution Agreement".
>
> I would assume that we non-contributors still can comment
> here in the mailing list, if appropriate.

Or better yet, become contributors.

___
Policies and (un)subscribe information available here:
http://wiki.secondlife.com/wiki/OpenSource-Dev
Please read the policies before posting to keep unmoderated posting privileges

Re: [opensource-dev] Review Request: Restructure loops that use breaks without need (reviewboard test)

[Opensource Obscure]

 On Thu, 02 Dec 2010 03:57:24 -, "Oz Linden"  
 wrote:
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/2/
> ---
>
> Review request for Viewer.
>
>
> Summary
> ---
>
> This review is mostly a first test of reviewboard.

 I just want to point out (as a reminder to other list members)
 that the reviewboard is for code contributors only, that is

 "In order to post reviews or comment on them in this system,
 you must have executed a Contribution Agreement".

 I would assume that we non-contributors still can comment
 here in the mailing list, if appropriate.


 Opensource Obscure
___
Policies and (un)subscribe information available here:
http://wiki.secondlife.com/wiki/OpenSource-Dev
Please read the policies before posting to keep unmoderated posting privileges

Re: [opensource-dev] Review Request: Restructure loops that use breaks without need (reviewboard test)

[Cron Stardust]

> On 2010-12-01 21:26:12, Cron Stardust wrote:
> > Based on the logic of the removed break, line 1192 of the fixed file:
> >  idleTimer.getElapsedTimeF64() >= max_idle_time
> > should be
> >  idleTimer.getElapsedTimeF64() < max_idle_time
> > 
> > The variable "S32 pending;" is redeclared on line 1613 of the fixed file.  
> > The previous edition didn't have this issue due to scoping.
> > 
> > Looks like the break on 4224 can't be easily removed: it's a tail test in a 
> > loop that seems to be required to be a head-test loop.  Maybe the logic and 
> > purpose can be reanalyzed, but then this would be more than a refactor.
> 
> Wolfpup Lowenhar wrote:
> we are just testing here :p

True, but reporting errors (and getting them fixed) is as much part of the test 
(IMHO) as everything else. :D


- Cron


---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/2/#review4
---


On 2010-12-01 19:57:24, Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/2/
> ---
> 
> (Updated 2010-12-01 19:57:24)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This review is mostly a first test of reviewboard.
> 
> I do have an esthetic dislike for the 'break' statement anywhere but as the 
> end of a case, so I chose to change some instances of break usage that were 
> not justified by any extreme need.
> 
> 
> This addresses bug storm-606.
> http://jira.secondlife.com/browse/storm-606
> 
> 
> Diffs
> -
> 
>   indra/newview/llappviewer.cpp bf98b026bcb1 
> 
> Diff: http://codereview.secondlife.com/r/2/diff
> 
> 
> Testing
> ---
> 
> None at all... have not even compiled it yet.
> 
> 
> Thanks,
> 
> Oz
> 
>

___
Policies and (un)subscribe information available here:
http://wiki.secondlife.com/wiki/OpenSource-Dev
Please read the policies before posting to keep unmoderated posting privileges

Re: [opensource-dev] Review Request: Restructure loops that use breaks without need (reviewboard test)

[Wolfpup Lowenhar]

> On 2010-12-01 21:26:12, Cron Stardust wrote:
> > Based on the logic of the removed break, line 1192 of the fixed file:
> >  idleTimer.getElapsedTimeF64() >= max_idle_time
> > should be
> >  idleTimer.getElapsedTimeF64() < max_idle_time
> > 
> > The variable "S32 pending;" is redeclared on line 1613 of the fixed file.  
> > The previous edition didn't have this issue due to scoping.
> > 
> > Looks like the break on 4224 can't be easily removed: it's a tail test in a 
> > loop that seems to be required to be a head-test loop.  Maybe the logic and 
> > purpose can be reanalyzed, but then this would be more than a refactor.

we are just testing here :p


- Wolfpup


---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/2/#review4
---


On 2010-12-01 19:57:24, Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/2/
> ---
> 
> (Updated 2010-12-01 19:57:24)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This review is mostly a first test of reviewboard.
> 
> I do have an esthetic dislike for the 'break' statement anywhere but as the 
> end of a case, so I chose to change some instances of break usage that were 
> not justified by any extreme need.
> 
> 
> This addresses bug storm-606.
> http://jira.secondlife.com/browse/storm-606
> 
> 
> Diffs
> -
> 
>   indra/newview/llappviewer.cpp bf98b026bcb1 
> 
> Diff: http://codereview.secondlife.com/r/2/diff
> 
> 
> Testing
> ---
> 
> None at all... have not even compiled it yet.
> 
> 
> Thanks,
> 
> Oz
> 
>

___
Policies and (un)subscribe information available here:
http://wiki.secondlife.com/wiki/OpenSource-Dev
Please read the policies before posting to keep unmoderated posting privileges

Re: [opensource-dev] Review Request: Restructure loops that use breaks without need (reviewboard test)

[Cron Stardust]

---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/2/#review4
---


Based on the logic of the removed break, line 1192 of the fixed file:
 idleTimer.getElapsedTimeF64() >= max_idle_time
should be
 idleTimer.getElapsedTimeF64() < max_idle_time

The variable "S32 pending;" is redeclared on line 1613 of the fixed file.  The 
previous edition didn't have this issue due to scoping.

Looks like the break on 4224 can't be easily removed: it's a tail test in a 
loop that seems to be required to be a head-test loop.  Maybe the logic and 
purpose can be reanalyzed, but then this would be more than a refactor.

- Cron


On 2010-12-01 19:57:24, Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/2/
> ---
> 
> (Updated 2010-12-01 19:57:24)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This review is mostly a first test of reviewboard.
> 
> I do have an esthetic dislike for the 'break' statement anywhere but as the 
> end of a case, so I chose to change some instances of break usage that were 
> not justified by any extreme need.
> 
> 
> This addresses bug storm-606.
> http://jira.secondlife.com/browse/storm-606
> 
> 
> Diffs
> -
> 
>   indra/newview/llappviewer.cpp bf98b026bcb1 
> 
> Diff: http://codereview.secondlife.com/r/2/diff
> 
> 
> Testing
> ---
> 
> None at all... have not even compiled it yet.
> 
> 
> Thanks,
> 
> Oz
> 
>

___
Policies and (un)subscribe information available here:
http://wiki.secondlife.com/wiki/OpenSource-Dev
Please read the policies before posting to keep unmoderated posting privileges

Re: [opensource-dev] Review Request: Restructure loops that use breaks without need (reviewboard test)

[zidonuke]

> On 2010-12-01 20:54:32, Wolfpup Lowenhar wrote:
> > testing new review system.
> > 
> > code looks clean to me

Heh, commenting on his comment seems to work.


- Zidonuke


---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/2/#review1
---


On 2010-12-01 19:57:24, Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/2/
> ---
> 
> (Updated 2010-12-01 19:57:24)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This review is mostly a first test of reviewboard.
> 
> I do have an esthetic dislike for the 'break' statement anywhere but as the 
> end of a case, so I chose to change some instances of break usage that were 
> not justified by any extreme need.
> 
> 
> This addresses bug storm-606.
> http://jira.secondlife.com/browse/storm-606
> 
> 
> Diffs
> -
> 
>   indra/newview/llappviewer.cpp bf98b026bcb1 
> 
> Diff: http://codereview.secondlife.com/r/2/diff
> 
> 
> Testing
> ---
> 
> None at all... have not even compiled it yet.
> 
> 
> Thanks,
> 
> Oz
> 
>

___
Policies and (un)subscribe information available here:
http://wiki.secondlife.com/wiki/OpenSource-Dev
Please read the policies before posting to keep unmoderated posting privileges

Re: [opensource-dev] Review Request: Restructure loops that use breaks without need (reviewboard test)

[Wolfpup Lowenhar]

---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/2/#review1
---

Ship it!


testing new review system.

code looks clean to me

- Wolfpup


On 2010-12-01 19:57:24, Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/2/
> ---
> 
> (Updated 2010-12-01 19:57:24)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This review is mostly a first test of reviewboard.
> 
> I do have an esthetic dislike for the 'break' statement anywhere but as the 
> end of a case, so I chose to change some instances of break usage that were 
> not justified by any extreme need.
> 
> 
> This addresses bug storm-606.
> http://jira.secondlife.com/browse/storm-606
> 
> 
> Diffs
> -
> 
>   indra/newview/llappviewer.cpp bf98b026bcb1 
> 
> Diff: http://codereview.secondlife.com/r/2/diff
> 
> 
> Testing
> ---
> 
> None at all... have not even compiled it yet.
> 
> 
> Thanks,
> 
> Oz
> 
>

___
Policies and (un)subscribe information available here:
http://wiki.secondlife.com/wiki/OpenSource-Dev
Please read the policies before posting to keep unmoderated posting privileges

[opensource-dev] Review Request: Restructure loops that use breaks without need (reviewboard test)

[Oz Linden]

---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/2/
---

Review request for Viewer.


Summary
---

This review is mostly a first test of reviewboard.

I do have an esthetic dislike for the 'break' statement anywhere but as the end 
of a case, so I chose to change some instances of break usage that were not 
justified by any extreme need.


This addresses bug storm-606.
http://jira.secondlife.com/browse/storm-606


Diffs
-

  indra/newview/llappviewer.cpp bf98b026bcb1 

Diff: http://codereview.secondlife.com/r/2/diff


Testing
---

None at all... have not even compiled it yet.


Thanks,

Oz

___
Policies and (un)subscribe information available here:
http://wiki.secondlife.com/wiki/OpenSource-Dev
Please read the policies before posting to keep unmoderated posting privileges