----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/255/#review580 -----------------------------------------------------------
indra/llimage/llimagedimensionsinfo.h <http://codereview.secondlife.com/r/255/#comment508> You probably mean "whether" not "if". (The check is performed in either case, just the result will be different.) Also, "larger or equal min_len bytes" or "at least min_len bytes large" might be easier to understand than the negation "not shorter than min_len bytes". (Or maybe "long" rather than "large"? Dunno.) The semantic of the method could be further emphasized by naming it e.g. bool isFileLengthAtLeast(S32 min_len); or even bool isFileLengthAtLeastBytes(S32 min_len); Lastly, it might be obvious, but it's probably still worth mentioning that "the file" is mInfile. indra/llimage/llimagedimensionsinfo.cpp <http://codereview.secondlife.com/r/255/#comment514> sizeof(signature)/sizeof(signature[0]) will always be 2 here, due to the line above, won't it? If so, it might be better to do // Read BMP signature. const size_t SIGNATURE_SIZE = 2; U8 signature[SIGNATURE_SIZE]; mInfile.read((void*)signature, SIGNATURE_SIZE); indra/llimage/llimagedimensionsinfo.cpp <http://codereview.secondlife.com/r/255/#comment509> Magic number. (I guess it's BMP header (14) + DIB header - current position (File begin + 2)?) indra/llimage/llimagedimensionsinfo.cpp <http://codereview.secondlife.com/r/255/#comment511> 8 isn't explained ... indra/llimage/llimagedimensionsinfo.cpp <http://codereview.secondlife.com/r/255/#comment512> ... until here. (Assuming it's the same 8.) Might be worth another constant. indra/llimage/llimagedimensionsinfo.cpp <http://codereview.secondlife.com/r/255/#comment513> Wouldn't a seek be a "cheaper" way to determine size than reading into an actual buffer? (According to indra/llcommon/llapr.h, it returns -1 on failure.) There's also a static method LLAPRFile::size, but that seems to operate on not-yet-opened files given by filename. - Boroondas On April 7, 2011, 4:20 p.m., Vadim ProductEngine wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/255/ > ----------------------------------------------------------- > > (Updated April 7, 2011, 4:20 p.m.) > > > Review request for Viewer. > > > Summary > ------- > > * Added checks for image file contents not matching the file extension (e.g. > a bitmap named file.jpg). > * Added checks for abnormally short files to avoid crashes when parsing them. > > > This addresses bug STORM-1118. > http://jira.secondlife.com/browse/STORM-1118 > > > Diffs > ----- > > indra/llimage/llimagedimensionsinfo.h 33ca961b0870 > indra/llimage/llimagedimensionsinfo.cpp 33ca961b0870 > > Diff: http://codereview.secondlife.com/r/255/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