[mapguide-internals] Additional RFC60 review

[Walt Welton-Lair]

Hi UV,

I've spent most of today reviewing your RFC60 changes.  I agree with Trevor's
review comments, and I saw that you have already addressed most of them.

A note regarding tab characters, I plan to eventually rerun my utility
which cleans these up.  I could wait to run it until after you have
merged and then it will fix your changes as well.  That way you don't
have to do it this time.  For future submissions though, please make
sure your code editor is configured to insert spaces and not tabs.


Here are my comments.  There are a lot of them, but they're all fairly
straightforward to address.  These assume your latest RFC 60 code.

Oem
- I assume nothing in this folder will be merged.

BuildTools\cc.net\ccnet.config
- Should not be merged.

Common\MapGuideCommon\MapLayer\Map.h
- You mentioned m_colorPalette is a lazy instantiated container.  But both
  MgMap constructors currently allocate a container rather than initialize
  the pointer to NULL, so it's not lazy at all.

Common\MapGuideCommon\MapLayer\Map.cpp
- There are some memory leaks:
  * MgMap::Create(MgResourceService* ...) has a leak.  If the color
    palette exists but is empty, a new one is allocated and the pointer is
    overwritten without deleting the old object (if it exists).
  * MgMap::Create(CREFSTRING ...) creates a memory leak.  The constructor
    allocates m_colorPalette, and this method clears the pointer without
    first deleting the existing object.
  * Inadvertent errors like this is a good reason to make m_colorPalette
    be a ColorStringList, like Trevor originally suggested.  I recommend
    changing it as well.  It simplifies the code and removes any chance of
    a leak.  The lazy instantiation performance improvement is not worth it
    IMO.
- Me just nitpicking: don't add extra unnecessary newlines (line 411, 451)
- No need to clear the palette in MgMap::Dispose, since destructor will do it.
- In AddColorsToPalette you filter empty strings before calling std::transform,
  but still add the empty strings to the color palette.  Should they not be
  added?
- You have calls to std::sort and std::unique in GetColorPalette.  You
  should therefore remove the sort/unique calls in ParseColorStrings in the
  rendering service, since it calls GetColorPalette.
- Me just nitpicking: line 1144 - put the comment on a separate line.

Common\MapGuideCommon\Services\RenderingOptions.cpp
- Looks good.

Common\Renderers\AGGImageIO.cpp
- Includes should never be added before the include of "stdafx.h".  When
  precompiled header usage is enabled, any includes before stdafx.h are
  ignored.
- Is the include of ace/OS.h needed when _DEBUG_PNG8 is turned on?  If so
  then wrap the include in an ifdef.
- Add back include of AGGRenderer.h (see notes for AGGImageIO.h).
- Remove commented include of smartalloc.h.
- Add a brief comment explaining the purpose of the UseColorMap static.
- In CreateGdImageWithPalette, declare gdPPaletteFromImg24 in the block
  where it's actually used.
- In CreateGdImageWithPalette, get rid of commented calls to delete.  They
  distracting, as well as incorrect.
- The use of your myassert macro can lead to memory leaks.
  * In CreateGdImageWithPalette, the third call to myassert can cause a leak.
    Since the myassert macro throws an exception, the previously allocated
    gdPPalette pointer ends up getting leaked.  Same problem with the fourth
    call to myassert in this method.
  * There are two calls to myassert in one of the AGGImageIO::Save overloads.
    Both result in leaks if the assert goes off.  For the first, the gdImage24
    pointer gets leaked, while for the second, the gdImage24 and gdImgPalette
    pointers get leaked.
  * Do you really want myassert to raise an exception?
- Lines 1011-1014: get rid of commented "former GIF code"
- Me just nitpicking: lines 962, 1015 - put comments on separate lines.
- Always keep at least one blank line between methods (preferably two).

Common\Renderers\AGGImageIO.h
- Remove the include of AGGRenderer.h (it's no longer needed now that you've
  moved the RS_ColorVector definition to RenderStyles.h).
- We also want to avoid including gd.h in AGG class headers.  The use of GD
  within AGG is an implementation detail which shouldn't be exposed through
  the headers.  To make your code work without gd.h in the header, you can
  either change the gdImagePtr parameters to void* (see for example GDRenderer's
  m_imgout and m_imsym which also do this), or just change the new static
  methods to be helper methods declared directly in AGGImageIO.cpp (works
  since they're not referenced outside of AGGImageIO.cpp).  I prefer the
  latter.

Common\Renderers\AGGRenderer.cpp
- Separators missing before SupportsTooltips, ApplyLineStyles, ProcessArea,
  DrawScreenPolyline, AddExclusionRegion
- Always keep at least one blank line between methods (preferably two).
- Use slashes for method separators.

Common\Renderers\AGGRenderer.h
- Looks good.

Common\Stylization\DefaultStylizer.cpp
- Me just nitpicking: always keep at least one blank line between methods
  (preferably two).  E.g. StylizeGridLayer doesn't have any in your version
  of the code.

Common\Stylization\DefaultStylizer.h
- For GetSymbolManager move the implementation to the cpp file.  I realize
  it's just a one-line implementation, but for consistency I always put
  implementations in cpp files so people always know where to look.  And use
  newlines in the implementation.  I remember some older versions of Visual
  Studio would not let you hit breakpoints set on methods that were declared /
  implemented on a single line.

Common\Stylization\RendererStyles.h
- Get rid of the gdImage and gdImagePtr typedefs.  These are only used within
  the Renderers project, and in there the typedefs are already available via
  GD.

Server\Src\Common\Manager\Makefile.am
- See comments ServerRenderingService.h.  If you remove the AGGRenderer.h
  include from ServerRenderingService.h then no changes are needed to this
  file.

Server\Src\Common\Manager\ServerManager.vcproj
- If you remove the AGGRenderer.h include from ServerRenderingService.h
  then no changes are needed to this file.
- General note: when making changes to project file configuration settings
  don't forget to update ALL configurations.  You only made changes to the
  Win32 configurations and didn't update the x64 configs.

Server\Src\PostBuild\PostBuild.mak
- This won't be merged, as mentioned in your response to Trevor's review
  comments.

Server\Src\Services\Mapping\MappingUtil.cpp
- Was there a good reason to reorder all the includes?  Linux builds are
  sensitive to the include order (much more than Windows).
- The myassert define is not used in this file, so remove it.
- Comment on line 559: an FdoPtr isn't an autoPtr
- Lines 572-580: move the printf calls inside the if (extractColors) block.
- Lines 843-844: remove commented code
- Line 1335: remove commented code "bool isRef = false;"
- Comment line 1369: "the property setter canonicalizes the palette".  Not
  anymore with your latest changes.
- In ExtractColors, no need to check for an empty ColorStringList since
  AddColorsToPalette already does.
- Calls to Rule::GetLabel - the returned label can be NULL and the code needs
  to check for this.
- Calls to xxRule::GetSymbolization 0 the returned object can be NULL and the
  code should check for this.
- When adding colors to the ColorStringList you're calling substr(), I think
  to create a copy of the string since the MdfStrings returned from the API
  are references.  But isn't a copy automatically made when the string is added
  to the list?  Have you stepped into the std::list::push_back call to verify
  that substr is actually needed?
- FindColorInSymDefHelper methods: is there honestly any benefit in declaring
  these as inline?
- In GraphicElementVisitorImpl, add TODO comments for VisitImage and VisitText.
  These can be implemented by someone at a later time.
- In the CompoundSymbol version of FindColorInSymDefHelper add a TODO comment
  for the case where the symbol is defined via a resourceID reference.  This can be
  implemented at a later time.
- Curly braces go on new lines, per our coding style.

Server\Src\Services\Mapping\MappingUtil.h
- Looks good.

Server\Src\Services\Mapping\ServerMappingService.vcproj
- Does this file actually need to be changed?
- If so, only the Debug|Win32 configuration is currently updated, and all four
  configurations need to be updated.  Also, the Makefile.am file would need
  to be updated.

Server\Src\Services\Rendering\ServerRenderingService.cpp
- Use newlines for implementation of hasColorMap().
- In your latest change you uncommented the new calls to MG_TRY / MG_CATCH
  (I don't think Trevor realize you had originally added them as commented
  code).  The only renderer which actually does cleanup in EndMap is
  DWFRenderer, so it could benefit from this change.  The calls are not needed
  though to skip faulty tiles (as mentioned in your code comment).  If an
  exception is raised in RenderMapInternal while generating a tile it will be
  caught in RenderTile, and your RFC60 code will not be executed.  Two
  additional changes you need to make:
  * You should use the MG_THROW macro and not re-throw the exception yourself.
    Your code also has a reference count bug (look at MG_THROW).
  * In the current code the exception is re-thrown only for debug.  It must
    always be re-thrown, and a server error will be logged further up in
    the call stack.
- Issues with the RS_ColorVector:
  * The tileColorPalette pointer is not initialized to NULL.  In the case
    of an exception occurring during the Save call while using GD, the code
    (as it's currently written) will try to delete an uninitialized
    pointer.  While this code path should not currently happen, it may in
    the future.
  * There's a big memory leak: in the case of AGG the RS_ColorVector object
    is allocated, but it's only getting deleted in the exception handler.
  * Please clean all of this up by allocating the RS_ColorVector object
    on the stack in the case of AGG.  This is faster and it simplifies the
    code.
- Line 955 which checks for the AGG renderer also now includes a check for
  hasColorMap.  This logic is wrong.  If the renderer is AGG but hasColorMap
  returns false then your code will jump to the else block and use GD
  renderer.
- It looks like the RFC60 code in AGGImageIO raises std::exceptions, and
  these are rethrown here.  While the MG_CATCH macro will handle these,
  does this provide sufficient information in the error log?  Have you
  checked this?
- The coding style of the "rfc60 code to correct colormaps" section needs
  work:
  * curly braces go on new lines
  * code alignment is messed up (some lines aren't indented, others are
    indented 3 characters)
  * tab characters need to be replaced with spaces
- You added code to throw a MgNullArgumentException in the case where the
  renderer did not return data.  This type of exception is used when
  "a null argument is passed to a method that expects a non-null value".
  A better one might be MgStylizeLayerFailedException.  In any case, why
  do we need to throw the exception?  Is it to log an error?  Is this a
  requirement of RFC60, or is it to address some other issue?  Also,
  put the throw call on a new line so it's easy to set a breakpoint on
  that specific line.
- Always keep at least one blank line between methods (preferably two).

Server\Src\Services\Rendering\ServerRenderingService.h
- Now that you've moved the RS_ColorVector definition to RenderStyles.h,
  you can get rid of the include of AGGRenderer.h.  This lets you avoid having
  to update the ServerManager project include paths.
- If you also move ParseColorStrings to StylizationUtil as Trevor suggested,
  then no changes are needed to this file.

Server\Src\Services\Tile\ServerTileService.cpp
- You changed a reference to MgUnableToOpenTileFile to MgUnableToOpenLockFile.
  Looking at where MgUnableToOpenTileFile is used, it does seem to always
  refer to an error opening the lock file.  So this change seems ok, but:
  * also update the other two places where it's used
  * you must also update the resource file
    - file is at Common\MapGuideCommon\Resources\mapguide_en.res
    - update the name of the resource
    - update content/value: "Unable to open the tile lock file." sounds
      reasonable
  * get rid of the "MgUnableToOpenTileFile???? sounds wrong" comment
- Get rid of "refactored from below" comment and remove the commented code.
  Your code improvement is good.
- Always keep at least one blank line between methods (preferably two).
- Use slashes for method separators.

Server\Src\Services\Tile\ServerTileService.h
- I agree with Trevor's comments for this file.  The GetResourceServiceForMapDef
  method would definitely be worthwhile if it was called from at least two
  places.  But it's only used in one place and it's relatively small method,
  so IMO there's no tangible benefit.

Server\ServerStructured.sln
- Should not be merged.

 W       E         @__   __o
   A   T   R   @___    _ \<,_
     L           @_   (*)/ (*)