[opensource-dev] Review Request: VWR-26066: request LLFloaterWorldMap child "zoom slider" with correct type to get rid of warning when opening map flaoter

[Boroondas Gupte]

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

Review request for Viewer.


Summary
---

To reproduce

   1. Start the viewer from a terminal, or make the debug console visible after 
logging in
   2. Open the map e.g. by pressing Ctrl-m or by clicking the Map button (can 
be made visible from the bottom bar context menu)

  Expected

* Map opens
* Debug console or terminal displays

  INFO: openFloater: Opening floater worldmap

  (and no warning)

  Observed

* Map opens
* Debug console or terminal displays

  INFO: openFloater: Opening floater worldmap
  WARNING: getChild: Found child named "zoom slider" but of wrong type 
12LLSliderCtrl, expecting P8LLSlider


This is due to requesting the child control with the wrong type, which this 
change fixes.


This addresses bug VWR-26066.
http://jira.secondlife.com/browse/VWR-26066


Diffs
-

  doc/contributions.txt 848ad0e546f8 
  indra/newview/llfloaterworldmap.cpp 848ad0e546f8 

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


Testing
---

Merged this fix with e67da2c6e312 and built (linux 64 standalone)
* Warning gone
* Map still works, no perceptible change in behavior noticed.

Not tested: Merging with v-d tip, as I can't build that. (But I know from 
downloaded test builds that it is affected by this bug.)


Thanks,

Boroondas

___
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: STORM-1352 Crash in LLNearbyChatScreenChannel::showToastsBottom()

[Vadim ProductEngine]

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

(Updated June 20, 2011, 6:55 a.m.)


Review request for Viewer.


Changes
---

Changed sort functor return type to bool.

Although it was not strictly necessary, because according to 
http://www.sgi.com/tech/stl/BinaryPredicate.html "The result type must be 
convertible to bool", and int is.


Summary
---

Apparently, a nearby chat toast got somehow destroyed while still remaining in 
the list of active toasts.
Attempt to sort active toasts in showToastsBottom() then triggered the crash.

I don't know how to reproduce the crash, i.e. force destroying a toast in a way 
that its onClose() method (which would remove references to the toast) isn't 
called.
So we'll just remove references to the toast whenever it's destroyed.


This addresses bug STORM-1352.
http://jira.secondlife.com/browse/STORM-1352


Diffs (updated)
-

  indra/newview/llnearbychathandler.cpp UNKNOWN 

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


Testing
---


Thanks,

Vadim

___
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: VWR-26066: request LLFloaterWorldMap child "zoom slider" with correct type to get rid of warning when opening map flaoter

[Vadim ProductEngine]

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

Ship it!


- Vadim


On June 20, 2011, 6:35 a.m., Boroondas Gupte wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/350/
> ---
> 
> (Updated June 20, 2011, 6:35 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> To reproduce
> 
>1. Start the viewer from a terminal, or make the debug console visible 
> after logging in
>2. Open the map e.g. by pressing Ctrl-m or by clicking the Map button (can 
> be made visible from the bottom bar context menu)
> 
>   Expected
> 
> * Map opens
> * Debug console or terminal displays
> 
>   INFO: openFloater: Opening floater worldmap
> 
>   (and no warning)
> 
>   Observed
> 
> * Map opens
> * Debug console or terminal displays
> 
>   INFO: openFloater: Opening floater worldmap
>   WARNING: getChild: Found child named "zoom slider" but of wrong type 
> 12LLSliderCtrl, expecting P8LLSlider
> 
> 
> This is due to requesting the child control with the wrong type, which this 
> change fixes.
> 
> 
> This addresses bug VWR-26066.
> http://jira.secondlife.com/browse/VWR-26066
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 848ad0e546f8 
>   indra/newview/llfloaterworldmap.cpp 848ad0e546f8 
> 
> Diff: http://codereview.secondlife.com/r/350/diff
> 
> 
> Testing
> ---
> 
> Merged this fix with e67da2c6e312 and built (linux 64 standalone)
> * Warning gone
> * Map still works, no perceptible change in behavior noticed.
> 
> Not tested: Merging with v-d tip, as I can't build that. (But I know from 
> downloaded test builds that it is affected by this bug.)
> 
> 
> Thanks,
> 
> Boroondas
> 
>

___
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: VWR-25923 Unnecessary capability request spam

[Vadim ProductEngine]

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


Seems to work for me, although I'd appreciate if you used a more 
straightforward (=less error prone) logic.


indra/newview/llvoicevivox.cpp


Inconsistent spacing around "if" conditions.

Please use "if (cond)" with no extra or missing spaces.

This also applies to other changes in the patch.



indra/newview/llvoicevivox.cpp


It's unclear to me why you set voice to enabled here.
I think it should be set to enabled only when we're sure the parcel 
supports it.



indra/newview/llvoicevivox.cpp


What if region caps never arrive?
The string may grow infinitely.

Also I don't like using region name as a marked that region caps haven't 
arrived yet -- that's a hack.



indra/newview/llvoicevivox.cpp


This could be written shorter:
mRegionHasVoice = enabled;


- Vadim


On June 9, 2011, 9:56 a.m., Jonathan Yap wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/333/
> ---
> 
> (Updated June 9, 2011, 9:56 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This is a patch by ArminWeatherHax.  I am creating the request to help speed 
> this fix along in the system.
> 
> 
> 
> Ways to reproduce: log into a simulator.
> Reproduces: always
> Affects: any version supported, probably all 3rd party viewers but Kokua (and 
> Imprudence, soon).
> 
> What happens:
> In each idle cycle the voice client requests the "ParcelVoiceInfoRequest" 
> capability, thats each time a HTTP GET.
> See LLVivoxVoiceClient::stateMachine() after comment // Check for parcel 
> boundary crossing
> 
> Expected:
> On parcel/region change request the capability once. It's not the region that 
> rezzes in, but the avatar, so do the request for the capability not earlier 
> than the agents region signals capabilitiesReceived() true. After that you 
> are sure if the region returns an empty url you can give up for that region.
> 
> Not sure about the impact on lag - requesting and returning an url is not 
> much data transmitted, though its a pretty big number of people doing it over 
> and over per second (no matter if they have voice on or off).
> 
> 
> 
> 
> going once again through llviewerregion I see its fortunately not each time a 
> HTTP GET, just once when the agent connects to the region. Though the patch 
> still saves all the lookup if the cap is there while it can't be possibly.
> 
> 
> This addresses bug VWR-25923.
> http://jira.secondlife.com/browse/VWR-25923
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt a36a329e77cc 
>   indra/newview/llvoicevivox.h a36a329e77cc 
>   indra/newview/llvoicevivox.cpp a36a329e77cc 
> 
> Diff: http://codereview.secondlife.com/r/333/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jonathan
> 
>

___
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: STORM-1352 Crash in LLNearbyChatScreenChannel::showToastsBottom()

[Boroondas Gupte]

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


You third diff file (STORM-1352_2.diff) seems to be incremental instead of 
total. Please always use total diffs on RB, so that the full change can be seen 
and differences between versions of the change can be viewed to compare.


indra/newview/llnearbychathandler.cpp


On second thought, wouldn't it be good to at least issue a warning here if 
one of the get()s returns NULL? We might be sorting stuff that isn't there, 
which is clearly a sign that something's fishy.


- Boroondas


On June 20, 2011, 6:55 a.m., Vadim ProductEngine wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/341/
> ---
> 
> (Updated June 20, 2011, 6:55 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Apparently, a nearby chat toast got somehow destroyed while still remaining 
> in the list of active toasts.
> Attempt to sort active toasts in showToastsBottom() then triggered the crash.
> 
> I don't know how to reproduce the crash, i.e. force destroying a toast in a 
> way that its onClose() method (which would remove references to the toast) 
> isn't called.
> So we'll just remove references to the toast whenever it's destroyed.
> 
> 
> This addresses bug STORM-1352.
> http://jira.secondlife.com/browse/STORM-1352
> 
> 
> Diffs
> -
> 
>   indra/newview/llnearbychathandler.cpp UNKNOWN 
> 
> Diff: http://codereview.secondlife.com/r/341/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vadim
> 
>

___
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: STORM-1352 Crash in LLNearbyChatScreenChannel::showToastsBottom()

[Oz Linden]

> On June 16, 2011, 4:23 p.m., Boroondas Gupte wrote:
> > indra/newview/llnearbychathandler.cpp, lines 375-382
> > 
> >
> > This function's return type should be changed to bool, if it's intended 
> > for use with std::sort. (And that's how it's being used currently.)

and better yet... there's no excuse in a routine this small for multiple 
returns.
Add a local variable and use the else clause.

bool inOrder;
if (  test ... )
{
   inOrder = false;
}
else
{
   ... calc ...
   inOrder = v1 > v2;
}
return inOrder;

any half way decent optimizer will put that bool in register anyway, so there 
is no performance difference at all and the code is much clearer and easier to 
put breakpoints and logging into.


- Oz


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


On June 20, 2011, 6:55 a.m., Vadim ProductEngine wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/341/
> ---
> 
> (Updated June 20, 2011, 6:55 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Apparently, a nearby chat toast got somehow destroyed while still remaining 
> in the list of active toasts.
> Attempt to sort active toasts in showToastsBottom() then triggered the crash.
> 
> I don't know how to reproduce the crash, i.e. force destroying a toast in a 
> way that its onClose() method (which would remove references to the toast) 
> isn't called.
> So we'll just remove references to the toast whenever it's destroyed.
> 
> 
> This addresses bug STORM-1352.
> http://jira.secondlife.com/browse/STORM-1352
> 
> 
> Diffs
> -
> 
>   indra/newview/llnearbychathandler.cpp UNKNOWN 
> 
> Diff: http://codereview.secondlife.com/r/341/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vadim
> 
>

___
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: STORM-1352 Crash in LLNearbyChatScreenChannel::showToastsBottom()

[Boroondas Gupte]

> On June 16, 2011, 4:23 p.m., Boroondas Gupte wrote:
> > indra/newview/llnearbychathandler.cpp, lines 375-382
> > 
> >
> > This function's return type should be changed to bool, if it's intended 
> > for use with std::sort. (And that's how it's being used currently.)
> 
> Oz Linden wrote:
> and better yet... there's no excuse in a routine this small for multiple 
> returns.
> Add a local variable and use the else clause.
> 
> bool inOrder;
> if (  test ... )
> {
>inOrder = false;
> }
> else
> {
>... calc ...
>inOrder = v1 > v2;
> }
> return inOrder;
> 
> any half way decent optimizer will put that bool in register anyway, so 
> there is no performance difference at all and the code is much clearer and 
> easier to put breakpoints and logging into.
>

I must say I find the version with two returns perfectly readable, maybe 
specifically because the routine is short. One doesn't easily miss that there 
are multiple returns, other than one might when looking at a screen-filling 
routine.

Though, if you do it with if-else, maybe handle the usual case before the 
special case (i.e. test for first.get() && second.get). Also, I'd prefer it the 
variable to store the return value would be initialized upon definition, which 
can even save the else-clause:

bool inOrder = false; // default return value, in case one or both 
toasts don't exist

if (  positive test ... )
{
   ... calc ...
   inOrder = v1 > v2;
}

return inOrder;

And while I'm bike-shedding, we could get rid of the multiple get() calls 
(although those are probably cheap):

LLToast* toast_1 = first.get();
LLToast* toast_2 = second.get();

bool inOrder = false; // default return value, in case one or both 
toasts don't exist

if (toast_1 && toast_2) // Toasts might already be gone, see STORM-1352.
{
   inOrder = toast_1->getTimeLeftToLive() > 
toast_2->getTimeLeftToLive();
}

return inOrder;


- Boroondas


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


On June 20, 2011, 6:55 a.m., Vadim ProductEngine wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/341/
> ---
> 
> (Updated June 20, 2011, 6:55 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Apparently, a nearby chat toast got somehow destroyed while still remaining 
> in the list of active toasts.
> Attempt to sort active toasts in showToastsBottom() then triggered the crash.
> 
> I don't know how to reproduce the crash, i.e. force destroying a toast in a 
> way that its onClose() method (which would remove references to the toast) 
> isn't called.
> So we'll just remove references to the toast whenever it's destroyed.
> 
> 
> This addresses bug STORM-1352.
> http://jira.secondlife.com/browse/STORM-1352
> 
> 
> Diffs
> -
> 
>   indra/newview/llnearbychathandler.cpp UNKNOWN 
> 
> Diff: http://codereview.secondlife.com/r/341/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vadim
> 
>

___
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] Is it just me? Avatar textures downloading last.

[Lee ponzu]

If I can help gather any data let me know.


On Sun, Jun 19, 2011 at 6:55 AM, Oz Linden (Scott Lawrence) <
o...@lindenlab.com> wrote:

> On 2011-06-18 16:43, Lee ponzu wrote:
> > Extra symptom.  So, there I am, and the avatars are all rendered as
> > expected, when  --blink-- one of them turns gray, like the texture got
> > lost.  No, she didn't change to an all gray skin and outfit.  8-)
>
> This is, unfortunately, not new behavior.  I've seen it on and off for
> at least a couple of months but have not been able to pin it down.
>
> If we could organize an effort to 1) improve the instrumentation around
> avatar bakes, and then 2) hunt down the problems and stomp them, it
> would be a major win.
>
> I'd really like it to be true that by the end of 2011 no one in SL needs
> to have the "what do I look like to you" conversation any more.
>
>
> ___
> 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: STORM-1352 Crash in LLNearbyChatScreenChannel::showToastsBottom()

[Vadim ProductEngine]

> On June 20, 2011, 10:01 a.m., Boroondas Gupte wrote:
> > You third diff file (STORM-1352_2.diff) seems to be incremental instead of 
> > total. Please always use total diffs on RB, so that the full change can be 
> > seen and differences between versions of the change can be viewed to 
> > compare.

Ok.


> On June 20, 2011, 10:01 a.m., Boroondas Gupte wrote:
> > indra/newview/llnearbychathandler.cpp, lines 375-382
> > 
> >
> > On second thought, wouldn't it be good to at least issue a warning here 
> > if one of the get()s returns NULL? We might be sorting stuff that isn't 
> > there, which is clearly a sign that something's fishy.

Right, I'll add a warning.


- Vadim


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


On June 20, 2011, 6:55 a.m., Vadim ProductEngine wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/341/
> ---
> 
> (Updated June 20, 2011, 6:55 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Apparently, a nearby chat toast got somehow destroyed while still remaining 
> in the list of active toasts.
> Attempt to sort active toasts in showToastsBottom() then triggered the crash.
> 
> I don't know how to reproduce the crash, i.e. force destroying a toast in a 
> way that its onClose() method (which would remove references to the toast) 
> isn't called.
> So we'll just remove references to the toast whenever it's destroyed.
> 
> 
> This addresses bug STORM-1352.
> http://jira.secondlife.com/browse/STORM-1352
> 
> 
> Diffs
> -
> 
>   indra/newview/llnearbychathandler.cpp UNKNOWN 
> 
> Diff: http://codereview.secondlife.com/r/341/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vadim
> 
>

___
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: STORM-1352 Crash in LLNearbyChatScreenChannel::showToastsBottom()

[Vadim ProductEngine]

> On June 16, 2011, 4:23 p.m., Boroondas Gupte wrote:
> > indra/newview/llnearbychathandler.cpp, lines 375-382
> > 
> >
> > This function's return type should be changed to bool, if it's intended 
> > for use with std::sort. (And that's how it's being used currently.)
> 
> Oz Linden wrote:
> and better yet... there's no excuse in a routine this small for multiple 
> returns.
> Add a local variable and use the else clause.
> 
> bool inOrder;
> if (  test ... )
> {
>inOrder = false;
> }
> else
> {
>... calc ...
>inOrder = v1 > v2;
> }
> return inOrder;
> 
> any half way decent optimizer will put that bool in register anyway, so 
> there is no performance difference at all and the code is much clearer and 
> easier to put breakpoints and logging into.
>
> 
> Boroondas Gupte wrote:
> I must say I find the version with two returns perfectly readable, maybe 
> specifically because the routine is short. One doesn't easily miss that there 
> are multiple returns, other than one might when looking at a screen-filling 
> routine.
> 
> Though, if you do it with if-else, maybe handle the usual case before the 
> special case (i.e. test for first.get() && second.get). Also, I'd prefer it 
> the variable to store the return value would be initialized upon definition, 
> which can even save the else-clause:
> 
>   bool inOrder = false; // default return value, in case one or both 
> toasts don't exist
> 
>   if (  positive test ... )
>   {
>  ... calc ...
>  inOrder = v1 > v2;
>   }
> 
>   return inOrder;
> 
> And while I'm bike-shedding, we could get rid of the multiple get() calls 
> (although those are probably cheap):
> 
>   LLToast* toast_1 = first.get();
>   LLToast* toast_2 = second.get();
> 
>   bool inOrder = false; // default return value, in case one or both 
> toasts don't exist
> 
>   if (toast_1 && toast_2) // Toasts might already be gone, see STORM-1352.
>   {
>  inOrder = toast_1->getTimeLeftToLive() > 
> toast_2->getTimeLeftToLive();
>   }
> 
>   return inOrder;

Oz, I'm not a fan of the endless-indentation-single-return style, especially 
when it comes to a simple case of handling illegal arguments.


- Vadim


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


On June 20, 2011, 6:55 a.m., Vadim ProductEngine wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/341/
> ---
> 
> (Updated June 20, 2011, 6:55 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Apparently, a nearby chat toast got somehow destroyed while still remaining 
> in the list of active toasts.
> Attempt to sort active toasts in showToastsBottom() then triggered the crash.
> 
> I don't know how to reproduce the crash, i.e. force destroying a toast in a 
> way that its onClose() method (which would remove references to the toast) 
> isn't called.
> So we'll just remove references to the toast whenever it's destroyed.
> 
> 
> This addresses bug STORM-1352.
> http://jira.secondlife.com/browse/STORM-1352
> 
> 
> Diffs
> -
> 
>   indra/newview/llnearbychathandler.cpp UNKNOWN 
> 
> Diff: http://codereview.secondlife.com/r/341/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vadim
> 
>

___
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: STORM-1352 Crash in LLNearbyChatScreenChannel::showToastsBottom()

[Vadim ProductEngine]

> On June 16, 2011, 4:23 p.m., Boroondas Gupte wrote:
> > indra/newview/llnearbychathandler.cpp, lines 375-382
> > 
> >
> > This function's return type should be changed to bool, if it's intended 
> > for use with std::sort. (And that's how it's being used currently.)
> 
> Oz Linden wrote:
> and better yet... there's no excuse in a routine this small for multiple 
> returns.
> Add a local variable and use the else clause.
> 
> bool inOrder;
> if (  test ... )
> {
>inOrder = false;
> }
> else
> {
>... calc ...
>inOrder = v1 > v2;
> }
> return inOrder;
> 
> any half way decent optimizer will put that bool in register anyway, so 
> there is no performance difference at all and the code is much clearer and 
> easier to put breakpoints and logging into.
>
> 
> Boroondas Gupte wrote:
> I must say I find the version with two returns perfectly readable, maybe 
> specifically because the routine is short. One doesn't easily miss that there 
> are multiple returns, other than one might when looking at a screen-filling 
> routine.
> 
> Though, if you do it with if-else, maybe handle the usual case before the 
> special case (i.e. test for first.get() && second.get). Also, I'd prefer it 
> the variable to store the return value would be initialized upon definition, 
> which can even save the else-clause:
> 
>   bool inOrder = false; // default return value, in case one or both 
> toasts don't exist
> 
>   if (  positive test ... )
>   {
>  ... calc ...
>  inOrder = v1 > v2;
>   }
> 
>   return inOrder;
> 
> And while I'm bike-shedding, we could get rid of the multiple get() calls 
> (although those are probably cheap):
> 
>   LLToast* toast_1 = first.get();
>   LLToast* toast_2 = second.get();
> 
>   bool inOrder = false; // default return value, in case one or both 
> toasts don't exist
> 
>   if (toast_1 && toast_2) // Toasts might already be gone, see STORM-1352.
>   {
>  inOrder = toast_1->getTimeLeftToLive() > 
> toast_2->getTimeLeftToLive();
>   }
> 
>   return inOrder;
> 
> Vadim ProductEngine wrote:
> Oz, I'm not a fan of the endless-indentation-single-return style, 
> especially when it comes to a simple case of handling illegal arguments.

Boroondas, as far as I remember, there cannot be many active toasts, so no need 
to write more code here to save a microsecond.


- Vadim


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


On June 20, 2011, 6:55 a.m., Vadim ProductEngine wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/341/
> ---
> 
> (Updated June 20, 2011, 6:55 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Apparently, a nearby chat toast got somehow destroyed while still remaining 
> in the list of active toasts.
> Attempt to sort active toasts in showToastsBottom() then triggered the crash.
> 
> I don't know how to reproduce the crash, i.e. force destroying a toast in a 
> way that its onClose() method (which would remove references to the toast) 
> isn't called.
> So we'll just remove references to the toast whenever it's destroyed.
> 
> 
> This addresses bug STORM-1352.
> http://jira.secondlife.com/browse/STORM-1352
> 
> 
> Diffs
> -
> 
>   indra/newview/llnearbychathandler.cpp UNKNOWN 
> 
> Diff: http://codereview.secondlife.com/r/341/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vadim
> 
>

___
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: STORM-1352 Crash in LLNearbyChatScreenChannel::showToastsBottom()

[Vadim ProductEngine]

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

(Updated June 20, 2011, 3:29 p.m.)


Review request for Viewer.


Changes
---

Issue a warning if a NULL chat toast is encountered.

I'm not doing that in the sort functor to make sure we print only one warning 
per invalid item.


Summary
---

Apparently, a nearby chat toast got somehow destroyed while still remaining in 
the list of active toasts.
Attempt to sort active toasts in showToastsBottom() then triggered the crash.

I don't know how to reproduce the crash, i.e. force destroying a toast in a way 
that its onClose() method (which would remove references to the toast) isn't 
called.
So we'll just remove references to the toast whenever it's destroyed.


This addresses bug STORM-1352.
http://jira.secondlife.com/browse/STORM-1352


Diffs (updated)
-

  indra/newview/llnearbychathandler.cpp UNKNOWN 

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


Testing
---


Thanks,

Vadim

___
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: STORM-1352 Crash in LLNearbyChatScreenChannel::showToastsBottom()

[Boroondas Gupte]

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

Ship it!


- Boroondas


On June 20, 2011, 3:29 p.m., Vadim ProductEngine wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/341/
> ---
> 
> (Updated June 20, 2011, 3:29 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Apparently, a nearby chat toast got somehow destroyed while still remaining 
> in the list of active toasts.
> Attempt to sort active toasts in showToastsBottom() then triggered the crash.
> 
> I don't know how to reproduce the crash, i.e. force destroying a toast in a 
> way that its onClose() method (which would remove references to the toast) 
> isn't called.
> So we'll just remove references to the toast whenever it's destroyed.
> 
> 
> This addresses bug STORM-1352.
> http://jira.secondlife.com/browse/STORM-1352
> 
> 
> Diffs
> -
> 
>   indra/newview/llnearbychathandler.cpp UNKNOWN 
> 
> Diff: http://codereview.secondlife.com/r/341/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vadim
> 
>

___
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: Enable watchdog timer for Beta crash hunting.

[Vadim ProductEngine]

> On June 14, 2011, 2:50 p.m., Alain Linden wrote:
> > indra/newview/app_settings/settings.xml, line 12409
> > 
> >
> > I presume this means trigger the watchdog after 20 seconds of 'freeze'.
> 
> Jenn wrote:
> :) yes

Has this patch been submitted? If so, please close the review request.


- Vadim


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


On June 14, 2011, 2:50 p.m., Jenn wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/337/
> ---
> 
> (Updated June 14, 2011, 2:50 p.m.)
> 
> 
> Review request for Viewer, Alain Linden, Richard Nelson, and Nat Goodspeed.
> 
> 
> Summary
> ---
> 
> Enable watchdog timer for Beta release to get tracebacks for classes of 
> crashes for which we currently have little data.
> 
> Setting timer to 20 seconds. Will either disable or make longer for 
> deployment to the Release channel.
> 
> 
> Diffs
> -
> 
>   indra/newview/app_settings/settings.xml a7c507c7e0f8 
> 
> Diff: http://codereview.secondlife.com/r/337/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jenn
> 
>

___
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: OPEN-99: use -march=pentium3 and -march=pentium4 only for 32 bit builds

[Boroondas Gupte]

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

Review request for Viewer.


Summary
---

These flags prevent building for 64-bit (both standalone or non-standalone), so 
only use them for 32-bit builds.


This addresses bug OPEN-99.
http://jira.secondlife.com/browse/OPEN-99


Diffs
-

  doc/contributions.txt e8f2a53c3d6e 
  indra/cmake/00-Common.cmake e8f2a53c3d6e 

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


Testing
---

Tried a non-standalone 64-bit build (without using 64-bit prebuilts, though):
Fails with
[ 11%] Building CXX object llcommon/CMakeFiles/llcommon.dir/u64.o
Linking CXX shared library libllcommon.so

/usr/lib/gcc/x86_64-pc-linux-gnu/4.4.5/../../../../x86_64-pc-linux-gnu/bin/ld: 
skipping incompatible 
${SRC_DIR}/build-linux-i686/packages/lib/release/libbreakpad_client.so when 
searching for -lbreakpad_client

/usr/lib/gcc/x86_64-pc-linux-gnu/4.4.5/../../../../x86_64-pc-linux-gnu/bin/ld: 
skipping incompatible 
${SRC_DIR}/build-linux-i686/packages/lib/release/libaprutil-1.so when searching 
for -laprutil-1

/usr/lib/gcc/x86_64-pc-linux-gnu/4.4.5/../../../../x86_64-pc-linux-gnu/bin/ld: 
skipping incompatible 
${SRC_DIR}/build-linux-i686/packages/lib/release/libaprutil-1.a when searching 
for -laprutil-1

/usr/lib/gcc/x86_64-pc-linux-gnu/4.4.5/../../../../x86_64-pc-linux-gnu/bin/ld: 
skipping incompatible 
${SRC_DIR}/build-linux-i686/packages/lib/release/libdb-5.1.so when searching 
for -ldb-5.1

/usr/lib/gcc/x86_64-pc-linux-gnu/4.4.5/../../../../x86_64-pc-linux-gnu/bin/ld: 
cannot find -ldb-5.1
collect2: ld returned 1 exit status
make[2]: *** [llcommon/libllcommon.so] Error 1
make[1]: *** [llcommon/CMakeFiles/llcommon.dir/all] Error 2
make: *** [all] Error 2
ERROR: building default configuration returned 2
For more information: try re-running your command with --verbose or 
--debug

Tried a standalone 64-bit build (after merging OPEN-38 changes):
Fails with
[ 13%] Building CXX object 
llcharacter/CMakeFiles/llcharacter.dir/llvisualparam.o
Linking CXX static library libllcharacter.a
[ 13%] Built target llcharacter
Scanning dependencies of target INTEGRATION_TEST_bitpack
[ 13%] Building CXX object 
llcommon/CMakeFiles/INTEGRATION_TEST_bitpack.dir/tests/bitpack_test.o
c++: CMakeFiles/INTEGRATION_TEST_bitpack.dir/tests/bitpack_test.o: No 
such file or directory
make[2]: *** 
[llcommon/CMakeFiles/INTEGRATION_TEST_bitpack.dir/tests/bitpack_test.o] Error 1
make[1]: *** [llcommon/CMakeFiles/INTEGRATION_TEST_bitpack.dir/all] 
Error 2
make: *** [all] Error 2
ERROR: building default configuration returned 2
For more information: try re-running your command with --verbose or 
--debug

(Both errors occur much later in the build process than the one that'd occur 
without this change.)

Tried a non-standalone 32-bit build:
Fails on https://jira.secondlife.com/browse/OPEN-23 , just as without this 
change.


Thanks,

Boroondas

___
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] Is it just me? Avatar textures downloading last.

[Monty Brandenberg]

On 6/20/2011 5:20 PM, Lee ponzu wrote:
> If I can help gather any data let me know.

Jira + description of events with timeline + logfile


___
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] Is it just me? Avatar textures downloading last.

[Thickbrick Sleaford]

On Saturday 18 June 2011 20:00:04 Lee ponzu wrote:
> However, and unrelated to shadows, I began to notice last night that while
> my scenes had all downloaded, the avatars around me were still all gray.
>  Their objects were OK, it was just their skin and cloth final bake that is
> missing.  They do show up after several minutes, but long after everything
> else.
> 

I've noticed avatar bakes taking very long to un-gray too, when trying the SL 
v2 viewer recently after not using it for a long-ish while. This is especially 
noticeable in crowded areas.

Looking at the texture console, it looks like any fetch that is using udp 
(i.e. bakes, since they can not be fetched via http) is always at the bottom 
of the list, and usually gets bumped out quickly. I haven't tested this, but I 
get the feeling that the udp fetches are starved out of processing by the 
regular http fetches. If I stay a while in a gray people gathering, and then 
relog, the once-gray people have at least one discard level of their bakes 
quickly decoded, probably from cache. This makes me think that this is a 
prioritization issue, possibly caused by the quick turn-around of http 
textures.

-- 
Thickbrick
___
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: Changes to fix CHOP-662.

[Squire Linden]

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

Review request for Viewer, Oz Linden, Seth ProductEngine, and Alain Linden.


Summary
---

Changes to fix CHOP-662. CHOP-662 was caused by the recent addition of 
lldiriterator.cpp which is used to iterate over directories. It takes a mask in 
the form of a glob string to use as a pattern for which files to iterate over.

Unfortunately, as originally implemented, it converted the glob to a regex 
pattern without escaping any legal glob characters which are illegal regex 
characters. As a result if the passed in mask was an illegal regex it would 
fail and llerrs would cause a crash.

In CHOP-662 this happens whenever a group name has, e.g. a '+' character at the 
beginning. When logging in the viewer would attempt to load the group chat log 
file which would result in a glob starting with a '+' character and in turn to 
an illegal regex in lldiriterator.

Also, the chat code was doing nothing to ensure that illegal glob characters 
were not being passed to the lldiriterator constructor.


This addresses bug CHOP-662.
http://jira.secondlife.com/browse/CHOP-662


Diffs
-

  doc/contributions.txt e8f2a53c3d6e 
  indra/llvfs/CMakeLists.txt e8f2a53c3d6e 
  indra/llvfs/lldiriterator.cpp e8f2a53c3d6e 
  indra/llvfs/tests/lldiriterator_test.cpp PRE-CREATION 
  indra/newview/lllogchat.cpp e8f2a53c3d6e 

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


Testing
---

Added a unit test for lldiriterator which checks for construction failures on 
examples of the most common group names which were causing the crashes.


Thanks,

Squire

___
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