Re: [opensource-dev] Review Request: VWR-25923 Unnecessary capability request spam

[ArminWeatherHax Resident]

> On June 9, 2011, 1:48 p.m., Altair Memo wrote:
> > indra/newview/llvoicevivox.cpp, line 785
> > 
> >
> > not sure is a nice choice remove the warn, a resident lose the only way 
> > to know if all work fine

I'll add a comment and rename mRegionHasVoice to mRegionHasVoiceCap to be more 
clear, and add a warning when the cap response arrived with empty 
ParcelVoiceInfoRequest.


- ArminWeatherHax


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


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

[ArminWeatherHax Resident]

> On June 20, 2011, 9:49 a.m., Vadim ProductEngine wrote:
> > indra/newview/llvoicevivox.cpp, lines 4485-4488
> > 
> >
> > 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.

The state engine resets the string if it was changed, but you are right, it's 
not nice. I'll go for adding LLVivoxVoiceClient::parcelChanged() to 
LLViewerParcelMgr::addAgentParcelChangedCallback which will make the whole 
patch more straightforward.


- ArminWeatherHax


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


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

[ArminWeatherHax Resident]

> On June 20, 2011, 9:49 a.m., Vadim ProductEngine wrote:
> > indra/newview/llvoicevivox.cpp, lines 4485-4488
> > <http://codereview.secondlife.com/r/333/diff/1/?file=2939#file2939line4485>
> >
> > 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.
> 
> ArminWeatherHax Resident wrote:
> The state engine resets the string if it was changed, but you are right, 
> it's not nice. I'll go for adding LLVivoxVoiceClient::parcelChanged() to 
> LLViewerParcelMgr::addAgentParcelChangedCallback which will make the whole 
> patch more straightforward.
> 
> Vadim ProductEngine wrote:
> Sounds good.
> 
> BTW, while working on Windlight Region Settings I added a "capabilities 
> received" signal in LLViewerRegion, which might be of use here as well. That 
> code hasn't been integrated yet, but if you want I can send you the patch.

Currently I have a new state which rechecks capabilitiesReceived until true,  
and was about to add a comment to it that a callback would be much better - so 
yes please about sending that patch :) 


> On June 20, 2011, 9:49 a.m., Vadim ProductEngine wrote:
> > indra/newview/llvoicevivox.cpp, line 557
> > <http://codereview.secondlife.com/r/333/diff/1/?file=2939#file2939line557>
> >
> > Inconsistent spacing around "if" conditions.
> > 
> > Please use "if (cond)" with no extra or missing spaces.
> > 
> > This also applies to other changes in the patch.

"if[space](cond)" - is that right?


- ArminWeatherHax


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


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

[ArminWeatherHax Resident]

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


I pushed a new version of the patch to https://bitbucket.org/ArminW/vwr-25923
Its 2 commits: 
* first Vadims patch
* then my patch addressing the reviews here

- ArminWeatherHax


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-1186 "no mCurrentRMessageTemplate" spam when first opening inventory

[ArminWeatherHax Resident]

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

Ship it!


- ArminWeatherHax


On Aug. 1, 2011, 11:41 a.m., Vadim ProductEngine wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/425/
> ---
> 
> (Updated Aug. 1, 2011, 11:41 a.m.)
> 
> 
> Review request for Viewer, Stone Linden and Richard Nelson.
> 
> 
> Summary
> ---
> 
> Removed the "no mCurrentRMessageTemplate" warning message.
> 
> The mCurrentRMessageTemplate member seems to only be non-NULL for a short 
> while
> after an incoming message was validated and parsed, thus there is no
> guarantee that we can obtain name of the last received message at any given
> time. So if we can't we'll simply return an empty string without spamming the
> log with warnings.
> 
> The issue may have been introduced in changeset 
> https://bitbucket.org/lindenlab/viewer-development/changeset/354916d6f13c.
> 
> 
> This addresses bug STORM-1186.
> http://jira.secondlife.com/browse/STORM-1186
> 
> 
> Diffs
> -
> 
>   indra/llmessage/lltemplatemessagereader.cpp ac0f1a132d35 
> 
> Diff: http://codereview.secondlife.com/r/425/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

[opensource-dev] Review Request: STORM-1532 (was VWR-25923) followup: Unnecessary capability request spam

[ArminWeatherHax Resident]

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

Review request for Viewer.


Summary
---

https://jira.secondlife.com/browse/STORM-1532 (was VWR-25923) this is a 
followup patch since  f7b7b211a33b (Fix for VOICE-3 [crashhunters] 
LLVoiceVoiceClient::stateMachine) already handles parts of the original patch. 
This patch avoids re-checking for the ParcelVoiceInfoRequest capability when 
its clear the region doesn't have it.

Armin


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


Diffs
-

  doc/contributions.txt 478aabd2813b 
  indra/newview/llvoicevivox.cpp 478aabd2813b 

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


Testing
---


Thanks,

ArminWeatherHax

___
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