GadgetSpec / MessageBundle factory cleanup

Clean up of GadgetSpec / MessageBundle factories to use a common base class and get consistent behavior between them. 

This allows for a much simpler message bundle model that eliminates a bug in the current code which potentially causes a high level of churn on caches and inconsistent updates. 

Deleted application manifest code as the spec was formally removed from opensocial before 0.9 was released.

[johnfargo, 2009-07-15 02:00:55]

Test comment (experiencing weird codereview issues).

[johnfargo, 2009-07-15 22:25:53]

Looks good.

http://codereview.appspot.com/91120/diff/1/4
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactory.java
(right):

http://codereview.appspot.com/91120/diff/1/4#newcode57
Line 57: return cacheProvider.createCache(CACHE_NAME);
curious, any special value in pulling out this function?

[awiner, 2009-07-15 22:38:00]

http://codereview.appspot.com/91120/diff/1/5
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java
(right):

http://codereview.appspot.com/91120/diff/1/5#newcode105
Line 105: throw new GadgetException(GadgetException.Code.INTERNAL_SERVER_ERROR,
i'd be just as happy/happier to see a ClassCastException, so I'd write this code
above as:

if (obj instanceof GadgetException) {
  throw (GadgetException) obj;
}

return clazz.cast(obj);

http://codereview.appspot.com/91120/diff/1/5#newcode111
Line 111: * Retrieves a gadget specification from the Internet, processes its
views and
inaccurate comment now

http://codereview.appspot.com/91120/diff/1/5#newcode208
Line 208: logger.log(Level.INFO, "Failed to update {1}. Using cached version.",
query.specUri);
{0} - these are zero-indexed

http://codereview.appspot.com/91120/diff/1/5#newcode211
Line 211: logger.log(Level.INFO, "Failed to update {1}. Applying negative
cache.", query.specUri);
ditto

http://codereview.appspot.com/91120/diff/1/8
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultMessageBundleFactory.java
(right):

http://codereview.appspot.com/91120/diff/1/8#newcode69
Line 69: return new MessageBundle(all, new MessageBundle(parent, self));
Wouldn't MessageBundle.EMPTY common?  If so, consider an optimized factory
method
  MessageBundle.merge(a, b)
... that can check for empty bundles and avoid creating and copying maps
unnecessarily.

[etnu00, 2009-07-15 22:45:45]

http://codereview.appspot.com/91120/diff/1/4
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactory.java
(right):

http://codereview.appspot.com/91120/diff/1/4#newcode57
Line 57: return cacheProvider.createCache(CACHE_NAME);
On 2009/07/15 22:25:53, johnfargo wrote:
> curious, any special value in pulling out this function?

To make the line smaller.

super() must be the first line in a ctor. I would have preferred:

Cache<Uri, Object> cache = cacheProvider.createCache(CACHE_NAME);
super(GadgetSpec.class, executor, pipeline, cache, refresh);

but that's not legal.

[etnu00, 2009-07-16 00:43:38]

updated patch

http://codereview.appspot.com/91120/diff/1/5
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java
(right):

http://codereview.appspot.com/91120/diff/1/5#newcode105
Line 105: throw new GadgetException(GadgetException.Code.INTERNAL_SERVER_ERROR,
On 2009/07/15 22:38:00, awiner wrote:
> i'd be just as happy/happier to see a ClassCastException, so I'd write this
code
> above as:
> 
> if (obj instanceof GadgetException) {
>   throw (GadgetException) obj;
> }
> 
> return clazz.cast(obj);

yeah, that makes sense.

http://codereview.appspot.com/91120/diff/1/5#newcode111
Line 111: * Retrieves a gadget specification from the Internet, processes its
views and
On 2009/07/15 22:38:00, awiner wrote:
> inaccurate comment now

Done.

http://codereview.appspot.com/91120/diff/1/5#newcode208
Line 208: logger.log(Level.INFO, "Failed to update {1}. Using cached version.",
query.specUri);
On 2009/07/15 22:38:00, awiner wrote:
> {0} - these are zero-indexed

Done.

http://codereview.appspot.com/91120/diff/1/5#newcode211
Line 211: logger.log(Level.INFO, "Failed to update {1}. Applying negative
cache.", query.specUri);
On 2009/07/15 22:38:00, awiner wrote:
> ditto

Done.

http://codereview.appspot.com/91120/diff/1/8
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultMessageBundleFactory.java
(right):

http://codereview.appspot.com/91120/diff/1/8#newcode69
Line 69: return new MessageBundle(all, new MessageBundle(parent, self));
On 2009/07/15 22:38:00, awiner wrote:
> Wouldn't MessageBundle.EMPTY common?  If so, consider an optimized factory
> method
>   MessageBundle.merge(a, b)
> ... that can check for empty bundles and avoid creating and copying maps
> unnecessarily.

MessageBundle.EMPTY is only going to be common for the middle bundle. Most
gadgets have both a fallback (all/ALL) and a specific match. There's no overhead
for additional 'empty' bundles beyond one extra loop iteration.