[mapguide-internals] Initial review on RFC90 code

[Trevor Wekel]

Hi UV,

Here are some comments from my initial review of the RFC 90 code.  I will do a follow up review once the RFC has been updated and code changes have been made.

Thank you,
Trevor


General Comments
----------------
----------------

RFC 90 documentation in Trac is incomplete.  All serverconfig.ini parameters and new function signatures should be documented.  A general algorithm overview would also be good.

Based on my initial review, the following items should be documented:
- useMetaTiles and LockMethod serverconfig.ini parameters.
- Behavioural modification to AGGRenderer::Save and AGGImageIO.cpp
- New MgTileCache behaviour (see below, I believe some refactoring would be appropriate)

File specific comments
----------------------
----------------------

RenderingService.h
ProxyRenderingService.h
-----------------------
- new RenderFromMetaTile signature commented out, should it be removed?

TileService.h
-------------
- space change is not necessary.

ConfigProperties.cpp
ConfigProperties.h
------------------
- TileServicePropertyUseMetaTiles, TileServicePropertyLockMethod should be doc'd in RFC.
- New properties should be added to MgConfigProperties::sm_cviTileServiceProperties array


AGGRenderer.cpp
---------------
- "META" image format could be defined in MgImageFormats as an INTERNAL_API
- new ::SaveMethod uses format = "META".  I am not 100% clear on how this all fits together.  Some algorithm documetation in the RFC would be helpful.
- Can assert be replaced by a throw (MgInvalidArgumentException)?

AggImageIO.cpp
--------------
- Please remove commented out code.

Server.sln
----------
- Why were "Static" configurations added?

ServerRenderingService.vcproj
-----------------------------
- Rendering Service should not depend on Tile service headers.

ServerRenderingService.cpp
--------------------------
- GetTileCoords()
What purpose does tileColOffset serve?
Could this method be moved into MgMap as an INTERNAL_API method which takes tileWidth and tileHeight as parameters?

- RenderTileFromMetaTile()
Can asserts be replaced by MgInvalidArgumentException, MgOutOfRangeException?
Should bsimp->GetLength() be tested for 0 < size < 2^31 before casting?
Why is MyByteSource required?  The direct cast is fragile.

ServerTileService.cpp
---------------------
- RFC documentation for sm_creationCutoffTime, sm_pollingInterval, sm_useMetaTiles, sm_lockMethod

- sm_creationCutoffTime and sm_pollingInterval should use same time base.

- MgServerTileService::MgServerTileService() : MgTileService()
METAMAXDIM should be moved to configuration parameters MG_CONFIG_MAX_METATILE_DIM
Error message should use MgConfigurationPropertyValueIsOutOfRange
<sstream> use could be dropped. 

- DetectLockFile()
------------------
This functionality should be moved into MgTileCache and probably renamed to WaitForTileLock.
Is a subsecond sleep really necessary?
ACE_OS::sleep (const ACE_Time_Value &tv) may simplify code

- GetTile()
-----------
Since there is no signature change to the public API, APIVERSION 1.2 is not really required.

- GetSingleTile()
-----------------
MgFoundBlockingLockFile must be added to resource string file(s) in MapGuideCommon/Resources

- GetMetaTile()
---------------
Locking methods should be documented in the RFC.
Algorithm can be simplified by only generating a lock for the left top corner.  Meta tiling will be always on or always off.  For future work, each map will also be meta tiled or not.
I believe there is too much special case locking logic coded here.  The locking logic should be moved into MgTileCache (LockTile(), UnlockTile(), WaitForTileLock()).
All error messages must be defined in MapGuideCommon/Resources
<sstream> use could be dropped.

TileCache.h
-----------
- GetLockMap()
Lock map structure should not be directly exposed using GetLockMap().  LockTile() and UnlockTile() methods should be added.  New methods should hide sm_lockMethod details from the rest of the code.

serverconfig.ini
----------------
- Font names seem to have changed.  Please check editor.