Re: [opensource-dev] Review Request: Changes to fix CHOP-662.

2011-06-21 Thread Boroondas Gupte 

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



doc/contributions.txt


Please indent issue keys in this file with a single tab, rather than four 
spaces.



doc/contributions.txt


Current and past practice is that only volunteers are listed in 
doc/contributions.txt, not LL employees or contractors (like PE employees).

I'm not against changing that practice (or getting rid of the file 
altogether, now that changeset committers better reflect the change authors), 
but this should probably be discussed first.



indra/llvfs/lldiriterator.cpp


The glob_to_regex funcion looks like it might be useful outside the context 
of LLDirIterator or even file systems, so maybe it should reside in a source 
file of its own, or together with other string manipulation stuff.



indra/llvfs/lldiriterator.cpp


+1 for documentation.

Bonus points if you turn this into doxygen. Further, start the first 
sentence with a capital letter and end the last with a period.



indra/llvfs/lldiriterator.cpp


Not related to your change, but I'd find '* 2' more readable than a 
left-shift. A decent compiler should produce the same code from both, so for 
performance, it should not matter.



indra/llvfs/lldiriterator.cpp


The second comment line here doesn't start a new sentence, thus should 
start with a lower case letter. Both lines together are a sentence, so end the 
second line with a period.



indra/llvfs/lldiriterator.cpp


Indent the regex+= line with 4 tabs and the default line with 3 tabs, to be 
consistent with surrounding lines.



indra/llvfs/lldiriterator.cpp


Consider surrounding the += operator by a space on each side. (Also in 
occurrences above.)



indra/newview/lllogchat.cpp


Not introduced by your change, but getting more apparent as more characters 
get converted to '_':
Is it intended that this can lead to filename 'collisions'? I just tried on 
Aditi and founded groups *Boroondas* and >Boroondas<, and indeed, chat in both 
gets logged to the same file.


- Boroondas


On June 20, 2011, 8:21 p.m., Squire Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/357/
> ---
> 
> (Updated June 20, 2011, 8:21 p.m.)
> 
> 
> 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

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

2011-06-21 Thread 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

2011-06-21 Thread 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: OPEN-67: make LLDirIterator implementation compatible to boost::filesystem v3 (as found in Boost 1.44 and newer)

2011-06-21 Thread Boroondas Gupte 


> On May 25, 2011, 2:55 p.m., Altair Memo wrote:
> > work fine with both 1.45 from 3p-libs both custom 1.46 prebuilt libs 
> > (non-standalone)

Have you actually reviewed the code change or just tested the result?


- Boroondas


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


On May 25, 2011, 1:25 p.m., Boroondas Gupte wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/313/
> ---
> 
> (Updated May 25, 2011, 1:25 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Context: We are currently using Boost 1.45, which already comes with the new 
> Boost Filesystem Library API (Version 3) but still defaults to the old one 
> (Version 2). From Boost 1.46 on, V3 will be the default and Boost 1.47 will 
> be the last one to come with V2. The Boost Filesystem Library documentation 
> recommends "Existing code should be moved to Version 3 as soon as convenient. 
> New code should be written for Version 3. Version 2 is deprecated, and will 
> not be included in Boost releases 1.48 and later."
> 
> This change overrides the default, so that the V3 API is used, and makes the 
> necessary code changes. (So we can stick to Boost 1.45 and upgrade whenever 
> we feel like it.)
> 
> Note: I only changed stuff that the compiler complained about. If the new API 
> also changes semantic of still-compiling library usage, more changes might be 
> necessary.
> 
> 
> This addresses bug OPEN-67.
> http://jira.secondlife.com/browse/OPEN-67
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 959f9340da92 
>   indra/llvfs/lldiriterator.cpp 959f9340da92 
> 
> Diff: http://codereview.secondlife.com/r/313/diff
> 
> 
> Testing
> ---
> 
> * Compiled Viewer (standalone) with Boost 1.45
> * Started Viewer
> * Logged in
> 
> * Compiled Viewer (standalone) with Boost 1.46
> * Started Viewer
> * Logged in
> 
> Not tested:
> * non-standalone
> 
> 
> 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: OPEN-67: make LLDirIterator implementation compatible to boost::filesystem v3 (as found in Boost 1.44 and newer)

2011-06-21 Thread Francesco "Sythos" 
Well... I'm sorry but for me is quite hard remember what i've done
yesterday... Got focus about what i've done 1 month ago is impissible...

If i recall correctly i've already used this patch in KV, i can check deeper
asap @ home this evening

-- 
Sent by iPhone

Il giorno 21/giu/2011, alle ore 12:14, "Boroondas Gupte" <
slli...@boroon.dasgupta.ch> ha scritto:

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

On May 25th, 2011, 2:55 p.m., *Altair Memo* wrote:

work fine with both 1.45 from 3p-libs both custom 1.46 prebuilt libs
(non-standalone)

 Have you actually reviewed the code change or just tested the result?


- Boroondas

On May 25th, 2011, 1:25 p.m., Boroondas Gupte wrote:
  Review request for Viewer.
By Boroondas Gupte.

*Updated May 25, 2011, 1:25 p.m.*
Description

Context: We are currently using Boost 1.45, which already comes with
the new Boost Filesystem Library API (Version 3) but still defaults to
the old one (Version 2). From Boost 1.46 on, V3 will be the default
and Boost 1.47 will be the last one to come with V2. The Boost
Filesystem Library documentation recommends "Existing code should be
moved to Version 3 as soon as convenient. New code should be written
for Version 3. Version 2 is deprecated, and will not be included in
Boost releases 1.48 and later."

This change overrides the default, so that the V3 API is used, and
makes the necessary code changes. (So we can stick to Boost 1.45 and
upgrade whenever we feel like it.)

Note: I only changed stuff that the compiler complained about. If the
new API also changes semantic of still-compiling library usage, more
changes might be necessary.

  Testing

* Compiled Viewer (standalone) with Boost 1.45
* Started Viewer
* Logged in

* Compiled Viewer (standalone) with Boost 1.46
* Started Viewer
* Logged in

Not tested:
* non-standalone

  *Bugs: * OPEN-67 
Diffs

   - doc/contributions.txt (959f9340da92)
   - indra/llvfs/lldiriterator.cpp (959f9340da92)

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

Re: [opensource-dev] Review Request: Local Bitmap Browser implementation.

2011-06-21 Thread Vadim ProductEngine 


> On June 20, 2011, 2:19 p.m., Vadim ProductEngine wrote:
> > indra/newview/llfloaterlocalbitmap.h, lines 79-80
> > 
> >
> > Consider using enums instead.

Or rather typed constants.


> On June 20, 2011, 2:19 p.m., Vadim ProductEngine wrote:
> > indra/newview/llfloaterlocalbitmap.cpp, line 90
> > 
> >
> > No need to to use this for accessing members: this is why they have a 
> > distinctive naming style.

I meant no need to use "this".


- Vadim


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


On June 18, 2011, 8:04 a.m., Vaalith Jinn wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/347/
> ---
> 
> (Updated June 18, 2011, 8:04 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Local Bitmap Browser is a mechanism to locally load images into the viewer, 
> track them and optionally (per each image)
> have it check if the image has been overwritten locally and if so - update it 
> in the viewer and inworld.
> 
> This change affects build menu by adding a way to open the floater there.
> This change also affects the texture picker - adding tabs that lets the user 
> choose between the "Server" (regular inventory) and "Local" tabs (list of 
> locally added files).
> 
> 
> This addresses bug STORM-64.
> http://jira.secondlife.com/browse/STORM-64
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 6a3e7e403bd1 
>   indra/llmath/llvolumemgr.h 6a3e7e403bd1 
>   indra/llmath/llvolumemgr.cpp 6a3e7e403bd1 
>   indra/newview/CMakeLists.txt 6a3e7e403bd1 
>   indra/newview/llfloaterlocalbitmap.h PRE-CREATION 
>   indra/newview/llfloaterlocalbitmap.cpp PRE-CREATION 
>   indra/newview/lltexturectrl.h 6a3e7e403bd1 
>   indra/newview/lltexturectrl.cpp 6a3e7e403bd1 
>   indra/newview/llviewerfloaterreg.cpp 6a3e7e403bd1 
>   indra/newview/llviewerobjectlist.h 6a3e7e403bd1 
>   indra/newview/llviewertexturelist.h 6a3e7e403bd1 
>   indra/newview/llvovolume.h 6a3e7e403bd1 
>   indra/newview/skins/default/xui/en/floater_local_bitmap.xml PRE-CREATION 
>   indra/newview/skins/default/xui/en/floater_texture_ctrl.xml 6a3e7e403bd1 
>   indra/newview/skins/default/xui/en/menu_viewer.xml 6a3e7e403bd1 
> 
> Diff: http://codereview.secondlife.com/r/347/diff
> 
> 
> Testing
> ---
> 
> Texture/Sculptmap/Avatar Layer show.
> Texture/Sculptmap/Avatar Layer update.
> Multiple sculpties update.
> Opening multiple browser floaters works correctly. (two possible since one 
> can be opened from menu above, one from texture picker. all of them + texture 
> picker play nice.)
> Tested adding over 200 images at the same time.
> 
> 
> Thanks,
> 
> Vaalith
> 
>

___
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: OPEN-67: make LLDirIterator implementation compatible to boost::filesystem v3 (as found in Boost 1.44 and newer)

2011-06-21 Thread Aleric Inglewood 

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



indra/llvfs/lldiriterator.cpp


What is your reasoning to use native() here and not string()?


- Aleric


On May 25, 2011, 1:25 p.m., Boroondas Gupte wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/313/
> ---
> 
> (Updated May 25, 2011, 1:25 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Context: We are currently using Boost 1.45, which already comes with the new 
> Boost Filesystem Library API (Version 3) but still defaults to the old one 
> (Version 2). From Boost 1.46 on, V3 will be the default and Boost 1.47 will 
> be the last one to come with V2. The Boost Filesystem Library documentation 
> recommends "Existing code should be moved to Version 3 as soon as convenient. 
> New code should be written for Version 3. Version 2 is deprecated, and will 
> not be included in Boost releases 1.48 and later."
> 
> This change overrides the default, so that the V3 API is used, and makes the 
> necessary code changes. (So we can stick to Boost 1.45 and upgrade whenever 
> we feel like it.)
> 
> Note: I only changed stuff that the compiler complained about. If the new API 
> also changes semantic of still-compiling library usage, more changes might be 
> necessary.
> 
> 
> This addresses bug OPEN-67.
> http://jira.secondlife.com/browse/OPEN-67
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 959f9340da92 
>   indra/llvfs/lldiriterator.cpp 959f9340da92 
> 
> Diff: http://codereview.secondlife.com/r/313/diff
> 
> 
> Testing
> ---
> 
> * Compiled Viewer (standalone) with Boost 1.45
> * Started Viewer
> * Logged in
> 
> * Compiled Viewer (standalone) with Boost 1.46
> * Started Viewer
> * Logged in
> 
> Not tested:
> * non-standalone
> 
> 
> 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: OPEN-67: make LLDirIterator implementation compatible to boost::filesystem v3 (as found in Boost 1.44 and newer)

2011-06-21 Thread Boroondas Gupte 


> On June 21, 2011, 6:56 a.m., Aleric Inglewood wrote:
> > indra/llvfs/lldiriterator.cpp, line 123
> > 
> >
> > What is your reasoning to use native() here and not string()?

I tried to stick to a 1:1 replacement. The old 
boost::filesystem::path::filename() has return type string_type, and 
boost::filesystem::path::native(), too, so that seemed like the natural 
replacement to me. As we save the result in a std::string, my reasoning might 
not make too much sense, though.


- Boroondas


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


On May 25, 2011, 1:25 p.m., Boroondas Gupte wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/313/
> ---
> 
> (Updated May 25, 2011, 1:25 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Context: We are currently using Boost 1.45, which already comes with the new 
> Boost Filesystem Library API (Version 3) but still defaults to the old one 
> (Version 2). From Boost 1.46 on, V3 will be the default and Boost 1.47 will 
> be the last one to come with V2. The Boost Filesystem Library documentation 
> recommends "Existing code should be moved to Version 3 as soon as convenient. 
> New code should be written for Version 3. Version 2 is deprecated, and will 
> not be included in Boost releases 1.48 and later."
> 
> This change overrides the default, so that the V3 API is used, and makes the 
> necessary code changes. (So we can stick to Boost 1.45 and upgrade whenever 
> we feel like it.)
> 
> Note: I only changed stuff that the compiler complained about. If the new API 
> also changes semantic of still-compiling library usage, more changes might be 
> necessary.
> 
> 
> This addresses bug OPEN-67.
> http://jira.secondlife.com/browse/OPEN-67
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 959f9340da92 
>   indra/llvfs/lldiriterator.cpp 959f9340da92 
> 
> Diff: http://codereview.secondlife.com/r/313/diff
> 
> 
> Testing
> ---
> 
> * Compiled Viewer (standalone) with Boost 1.45
> * Started Viewer
> * Logged in
> 
> * Compiled Viewer (standalone) with Boost 1.46
> * Started Viewer
> * Logged in
> 
> Not tested:
> * non-standalone
> 
> 
> 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: Changes to fix CHOP-662.

2011-06-21 Thread Alain Linden 

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

Ship it!



indra/llvfs/tests/lldiriterator_test.cpp


Personally, I dislike referencing jira items in code comments.  Its 
referencing something ephemeral in something that will long out live it.


- Alain


On June 20, 2011, 8:21 p.m., Squire Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/357/
> ---
> 
> (Updated June 20, 2011, 8:21 p.m.)
> 
> 
> 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

Re: [opensource-dev] Review Request: Changes to fix CHOP-662.

2011-06-21 Thread Boroondas Gupte 


> On June 21, 2011, 7:57 a.m., Alain Linden wrote:
> > indra/llvfs/tests/lldiriterator_test.cpp, line 46
> > 
> >
> > Personally, I dislike referencing jira items in code comments.  Its 
> > referencing something ephemeral in something that will long out live it.

In general I'd agree, but here my interpretation was that this test was 
inspired by a specific issue. Should in that case the whole issue description 
be copied into the comment?


- Boroondas


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


On June 20, 2011, 8:21 p.m., Squire Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/357/
> ---
> 
> (Updated June 20, 2011, 8:21 p.m.)
> 
> 
> 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

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

2011-06-21 Thread Vadim ProductEngine 


> 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.
> 
> 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.

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.


- Vadim


---
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-25965 LLDirIterator fixes in the viewer cache migration and VFS fallback.

2011-06-21 Thread Vadim ProductEngine 

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

(Updated June 21, 2011, 8:33 a.m.)


Review request for Viewer.


Changes
---

Fixed the ticker number.


Summary
---

This removes additional leading delimiters in llappviewer needed due to the 
STORM-477 changes. This change should fix the fallback used when the viewer 
cannot find a VFS with the salt it expects to find and instead looks for any 
VFS data file in the cache directory to use.  It also fixes cache directory 
migration code that runs on Mac and Windows every time the viewer is updated. 


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


Diffs
-

  indra/newview/llappviewer.cpp 1d4d568986e3 

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


Testing
---

Test Plan:
https://jira.secondlife.com/browse/VWR-25965?focusedCommentId=264386

I ran Case 1 on Linux and Case 2 on Mac and Windows. Everything worked as 
expected.


Thanks,

Log

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

2011-06-21 Thread Alain Linden 


> On June 21, 2011, 7:57 a.m., Alain Linden wrote:
> > indra/llvfs/tests/lldiriterator_test.cpp, line 46
> > 
> >
> > Personally, I dislike referencing jira items in code comments.  Its 
> > referencing something ephemeral in something that will long out live it.
> 
> Boroondas Gupte wrote:
> In general I'd agree, but here my interpretation was that this test was 
> inspired by a specific issue. Should in that case the whole issue description 
> be copied into the comment?

The way I see it, the code remains long after the jira is dead and forgotten.  
As much as possible, code commentary should stand alone.  This test makes sense 
regardless of what jira inspired it, so why bother mentioning it?  Years from 
now a developer reading this code won't care about the jira.  That's my opinion.


- Alain


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


On June 20, 2011, 8:21 p.m., Squire Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/357/
> ---
> 
> (Updated June 20, 2011, 8:21 p.m.)
> 
> 
> 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

[opensource-dev] Review Request: Check for null ptr to hopefully prevent crash in LLToolPie

2011-06-21 Thread Alain Linden 

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

Review request for Viewer, Oz Linden and Vadim ProductEngine.


Summary
---

checking for null gAgentAvatarp should hopefully prevent the crash.  I can't 
reproduce it so I don't know for sure.


This addresses bug EXP-825.
http://jira.secondlife.com/browse/EXP-825


Diffs
-

  indra/newview/lltoolpie.cpp e8f2a53c3d6e 

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


Testing
---


Thanks,

Alain

___
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

2011-06-21 Thread 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.
> 
> 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
> > 
> >
> > 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: Check for null ptr to hopefully prevent crash in LLToolPie

2011-06-21 Thread Vadim ProductEngine 

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

Ship it!


The check certainly will not hurt.

- Vadim


On June 21, 2011, 8:53 a.m., Alain Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/359/
> ---
> 
> (Updated June 21, 2011, 8:53 a.m.)
> 
> 
> Review request for Viewer, Oz Linden and Vadim ProductEngine.
> 
> 
> Summary
> ---
> 
> checking for null gAgentAvatarp should hopefully prevent the crash.  I can't 
> reproduce it so I don't know for sure.
> 
> 
> This addresses bug EXP-825.
> http://jira.secondlife.com/browse/EXP-825
> 
> 
> Diffs
> -
> 
>   indra/newview/lltoolpie.cpp e8f2a53c3d6e 
> 
> Diff: http://codereview.secondlife.com/r/359/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alain
> 
>

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

2011-06-21 Thread Seth ProductEngine 


> On June 21, 2011, 7:57 a.m., Alain Linden wrote:
> > indra/llvfs/tests/lldiriterator_test.cpp, line 46
> > 
> >
> > Personally, I dislike referencing jira items in code comments.  Its 
> > referencing something ephemeral in something that will long out live it.
> 
> Boroondas Gupte wrote:
> In general I'd agree, but here my interpretation was that this test was 
> inspired by a specific issue. Should in that case the whole issue description 
> be copied into the comment?
> 
> Alain Linden wrote:
> The way I see it, the code remains long after the jira is dead and 
> forgotten.  As much as possible, code commentary should stand alone.  This 
> test makes sense regardless of what jira inspired it, so why bother 
> mentioning it?  Years from now a developer reading this code won't care about 
> the jira.  That's my opinion.

There is a test file where LLDirIterator is used: llvfs\tests\lldir_test.cpp. 
May be this unit test could be placed there?


- Seth


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


On June 20, 2011, 8:21 p.m., Squire Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/357/
> ---
> 
> (Updated June 20, 2011, 8:21 p.m.)
> 
> 
> 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

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

2011-06-21 Thread Vadim ProductEngine 


> On June 20, 2011, 9:49 a.m., Vadim ProductEngine wrote:
> > indra/newview/llvoicevivox.cpp, line 557
> > 
> >
> > Inconsistent spacing around "if" conditions.
> > 
> > Please use "if (cond)" with no extra or missing spaces.
> > 
> > This also applies to other changes in the patch.
> 
> ArminWeatherHax Resident wrote:
> "if[space](cond)" - is that right?

Yes.


> 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.
> 
> 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.
> 
> ArminWeatherHax Resident wrote:
> 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 :)

Sent.


- Vadim


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

2011-06-21 Thread Seth ProductEngine 

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

Ship it!


The glob to regex conversion was not initially supposed to accept user input as 
its argument that's why it lacked the checks for valid glob expressions. But 
now it looks like these checks are necessary for this case.

- Seth


On June 20, 2011, 8:21 p.m., Squire Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/357/
> ---
> 
> (Updated June 20, 2011, 8:21 p.m.)
> 
> 
> 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

Re: [opensource-dev] Review Request: OPEN-67: make LLDirIterator implementation compatible to boost::filesystem v3 (as found in Boost 1.44 and newer)

2011-06-21 Thread Aleric Inglewood 


> On June 21, 2011, 6:56 a.m., Aleric Inglewood wrote:
> > indra/llvfs/lldiriterator.cpp, line 123
> > 
> >
> > What is your reasoning to use native() here and not string()?
> 
> Boroondas Gupte wrote:
> I tried to stick to a 1:1 replacement. The old 
> boost::filesystem::path::filename() has return type string_type, and 
> boost::filesystem::path::native(), too, so that seemed like the natural 
> replacement to me. As we save the result in a std::string, my reasoning might 
> not make too much sense, though.

No that makes no sense at all, since boost::filesystem::path::native::string() 
also returns a std::string.
So, still the same question: why mIter->path().filename().native() and not 
mIter->path().filename().string()?


- Aleric


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


On May 25, 2011, 1:25 p.m., Boroondas Gupte wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/313/
> ---
> 
> (Updated May 25, 2011, 1:25 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Context: We are currently using Boost 1.45, which already comes with the new 
> Boost Filesystem Library API (Version 3) but still defaults to the old one 
> (Version 2). From Boost 1.46 on, V3 will be the default and Boost 1.47 will 
> be the last one to come with V2. The Boost Filesystem Library documentation 
> recommends "Existing code should be moved to Version 3 as soon as convenient. 
> New code should be written for Version 3. Version 2 is deprecated, and will 
> not be included in Boost releases 1.48 and later."
> 
> This change overrides the default, so that the V3 API is used, and makes the 
> necessary code changes. (So we can stick to Boost 1.45 and upgrade whenever 
> we feel like it.)
> 
> Note: I only changed stuff that the compiler complained about. If the new API 
> also changes semantic of still-compiling library usage, more changes might be 
> necessary.
> 
> 
> This addresses bug OPEN-67.
> http://jira.secondlife.com/browse/OPEN-67
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 959f9340da92 
>   indra/llvfs/lldiriterator.cpp 959f9340da92 
> 
> Diff: http://codereview.secondlife.com/r/313/diff
> 
> 
> Testing
> ---
> 
> * Compiled Viewer (standalone) with Boost 1.45
> * Started Viewer
> * Logged in
> 
> * Compiled Viewer (standalone) with Boost 1.46
> * Started Viewer
> * Logged in
> 
> Not tested:
> * non-standalone
> 
> 
> 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: OPEN-67: make LLDirIterator implementation compatible to boost::filesystem v3 (as found in Boost 1.44 and newer)

2011-06-21 Thread Aleric Inglewood 


> On June 21, 2011, 6:56 a.m., Aleric Inglewood wrote:
> > indra/llvfs/lldiriterator.cpp, line 123
> > 
> >
> > What is your reasoning to use native() here and not string()?
> 
> Boroondas Gupte wrote:
> I tried to stick to a 1:1 replacement. The old 
> boost::filesystem::path::filename() has return type string_type, and 
> boost::filesystem::path::native(), too, so that seemed like the natural 
> replacement to me. As we save the result in a std::string, my reasoning might 
> not make too much sense, though.
> 
> Aleric Inglewood wrote:
> No that makes no sense at all, since 
> boost::filesystem::path::native::string() also returns a std::string.
> So, still the same question: why mIter->path().filename().native() and 
> not mIter->path().filename().string()?
>

I meant boost::filesystem::path::string()


- Aleric


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


On May 25, 2011, 1:25 p.m., Boroondas Gupte wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/313/
> ---
> 
> (Updated May 25, 2011, 1:25 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Context: We are currently using Boost 1.45, which already comes with the new 
> Boost Filesystem Library API (Version 3) but still defaults to the old one 
> (Version 2). From Boost 1.46 on, V3 will be the default and Boost 1.47 will 
> be the last one to come with V2. The Boost Filesystem Library documentation 
> recommends "Existing code should be moved to Version 3 as soon as convenient. 
> New code should be written for Version 3. Version 2 is deprecated, and will 
> not be included in Boost releases 1.48 and later."
> 
> This change overrides the default, so that the V3 API is used, and makes the 
> necessary code changes. (So we can stick to Boost 1.45 and upgrade whenever 
> we feel like it.)
> 
> Note: I only changed stuff that the compiler complained about. If the new API 
> also changes semantic of still-compiling library usage, more changes might be 
> necessary.
> 
> 
> This addresses bug OPEN-67.
> http://jira.secondlife.com/browse/OPEN-67
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 959f9340da92 
>   indra/llvfs/lldiriterator.cpp 959f9340da92 
> 
> Diff: http://codereview.secondlife.com/r/313/diff
> 
> 
> Testing
> ---
> 
> * Compiled Viewer (standalone) with Boost 1.45
> * Started Viewer
> * Logged in
> 
> * Compiled Viewer (standalone) with Boost 1.46
> * Started Viewer
> * Logged in
> 
> Not tested:
> * non-standalone
> 
> 
> 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: OPEN-67: make LLDirIterator implementation compatible to boost::filesystem v3 (as found in Boost 1.44 and newer)

2011-06-21 Thread Boroondas Gupte 


> On June 21, 2011, 6:56 a.m., Aleric Inglewood wrote:
> > indra/llvfs/lldiriterator.cpp, line 123
> > 
> >
> > What is your reasoning to use native() here and not string()?
> 
> Boroondas Gupte wrote:
> I tried to stick to a 1:1 replacement. The old 
> boost::filesystem::path::filename() has return type string_type, and 
> boost::filesystem::path::native(), too, so that seemed like the natural 
> replacement to me. As we save the result in a std::string, my reasoning might 
> not make too much sense, though.
> 
> Aleric Inglewood wrote:
> No that makes no sense at all, since 
> boost::filesystem::path::native::string() also returns a std::string.
> So, still the same question: why mIter->path().filename().native() and 
> not mIter->path().filename().string()?
>
> 
> Aleric Inglewood wrote:
> I meant boost::filesystem::path::string()

The reason I've given above is the only one I had. If that one doesn't make 
sense, no reason is left for preferring mIter->path().filename().native() over 
mIter->path().filename().string(), AFAIK. However, I also see no reason to 
prefer mIter->path().filename().string() over mIter->path().filename().native().

>From the documentation, it seems they should give the exactly same data: the 
>path's (here: the filename's) content as std::string with OS-native 
>directory-separator characters (which shouldn't occur in a path that's just a 
>filename). What could make sense is to use 
>mIter->path().filename().generic_string(), so that the (effectless for pure 
>filenames) '/' --> '\' conversion doesn't have to be executed on Windows.


- Boroondas


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


On May 25, 2011, 1:25 p.m., Boroondas Gupte wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/313/
> ---
> 
> (Updated May 25, 2011, 1:25 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Context: We are currently using Boost 1.45, which already comes with the new 
> Boost Filesystem Library API (Version 3) but still defaults to the old one 
> (Version 2). From Boost 1.46 on, V3 will be the default and Boost 1.47 will 
> be the last one to come with V2. The Boost Filesystem Library documentation 
> recommends "Existing code should be moved to Version 3 as soon as convenient. 
> New code should be written for Version 3. Version 2 is deprecated, and will 
> not be included in Boost releases 1.48 and later."
> 
> This change overrides the default, so that the V3 API is used, and makes the 
> necessary code changes. (So we can stick to Boost 1.45 and upgrade whenever 
> we feel like it.)
> 
> Note: I only changed stuff that the compiler complained about. If the new API 
> also changes semantic of still-compiling library usage, more changes might be 
> necessary.
> 
> 
> This addresses bug OPEN-67.
> http://jira.secondlife.com/browse/OPEN-67
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 959f9340da92 
>   indra/llvfs/lldiriterator.cpp 959f9340da92 
> 
> Diff: http://codereview.secondlife.com/r/313/diff
> 
> 
> Testing
> ---
> 
> * Compiled Viewer (standalone) with Boost 1.45
> * Started Viewer
> * Logged in
> 
> * Compiled Viewer (standalone) with Boost 1.46
> * Started Viewer
> * Logged in
> 
> Not tested:
> * non-standalone
> 
> 
> 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