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

[Boroondas Gupte]

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



indra/newview/llvoicevivox.h
<http://codereview.secondlife.com/r/333/#comment818>

    Very good idea to explain the semantics of this field in detail, as the name alone doesn't give it away fully. :-)



indra/newview/llvoicevivox.cpp
<http://codereview.secondlife.com/r/333/#comment817>

    These are two sentences. Use a period instead of a comma.



indra/newview/llvoicevivox.cpp
<http://codereview.secondlife.com/r/333/#comment816>

    requestParcelInfo() might be a more natural name than using 'do'.



indra/newview/llvoicevivox.h
<http://codereview.secondlife.com/r/333/#comment821>

    Not a native speaker myself, but I don't think 'keep s.th. staying in <a state>' is proper English. Maybe either write
    	// Setting mRegionHasVoiceCaps to false makes the voice client stay in stateDisabled.
    or simply
    	// Setting mRegionHasVoiceCaps to false keeps the voice client in stateDisabled.



indra/newview/llvoicevivox.h
<http://codereview.secondlife.com/r/333/#comment820>

    This sentence confused me for some reason. I guess 'allows to leave' would be clearer than 'allows escaping'.



indra/newview/llvoicevivox.cpp
<http://codereview.secondlife.com/r/333/#comment822>

    I think this should be "changed region before the region cap response for the previous region arrived" to be clear.



indra/newview/llvoicevivox.cpp
<http://codereview.secondlife.com/r/333/#comment823>

    https://wiki.secondlife.com/wiki/Coding_standard#Naming_Convention says local variables should be lower_case, not camelCase so make this parcel_local_ID.



indra/newview/llvoicevivox.cpp
<http://codereview.secondlife.com/r/333/#comment824>

    What does the first 'it' refer to? "ID with" what?



indra/newview/llvoicevivox.cpp
<http://codereview.secondlife.com/r/333/#comment825>

    Why not directly
    		mCurrentRegion = gAgent.getRegion();
    and then use mCurrentRegion in the rest of this method?



indra/newview/llvoicevivox.cpp
<http://codereview.secondlife.com/r/333/#comment826>

    Here, too, either just 'keeps' without 'staying' or 'makes [...] stay', I think.


- Boroondas


On June 24, 2011, 4:45 a.m., Jonathan Yap wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/333/
> -----------------------------------------------------------
> 
> (Updated June 24, 2011, 4:45 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 04e2a3ddca51 
>   indra/newview/llviewerregion.h 04e2a3ddca51 
>   indra/newview/llviewerregion.cpp 04e2a3ddca51 
>   indra/newview/llvoicevivox.h 04e2a3ddca51 
>   indra/newview/llvoicevivox.cpp 04e2a3ddca51 
> 
> Diff: http://codereview.secondlife.com/r/333/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jonathan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110624/51ab7d49/attachment-0001.htm