Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(3)

Issue 63225: Per-browser rpc.js [#2] (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
16 years, 10 months ago by johnfargo
Modified:
16 years, 8 months ago
Reviewers:
shindig.remailer, etnu00
Base URL:
http://svn.apache.org/repos/asf/incubator/shindig/trunk/
Visibility:
Public.

Description

Updated w/ private Cache-Control headers to prevent CDN proxying issues. [original description] Implementation of JsFeatureLoader that loads only the rpc.js transport code needed by the target browser. To use this, a concrete implementation of UserAgent.Parser must be provided and bound. In addition, JsFeatureLoader must be bound to implementation SmartRpcJsFeatureLoader. This code is not widely tested, and should be deemed experimental.

Patch Set 1 #

Total comments: 5

Messages

Total messages: 2
johnfargo
16 years, 10 months ago (2009-06-09 23:41:27 UTC) #1
etnu00
16 years, 10 months ago (2009-06-10 00:36:39 UTC) #2
http://codereview.appspot.com/63225/diff/1/7
File
java/common/src/main/java/org/apache/shindig/common/servlet/HttpServletUserAgentProvider.java
(right):

http://codereview.appspot.com/63225/diff/1/7#newcode32
Line 32: private Provider<HttpServletRequest> reqProvider;
These should both be final.

http://codereview.appspot.com/63225/diff/1/6
File java/common/src/main/java/org/apache/shindig/common/servlet/UserAgent.java
(right):

http://codereview.appspot.com/63225/diff/1/6#newcode54
Line 54: public boolean isOtherHtml5() {
This function doesn't really make a lot of sense. Why not have something that
simply returns isHtml5 to test for HTML5 support? Why not just use OTHER when we
don't know anything about the browser and let the calling code make its own
assumptions? If we actually need to add support for a new browser at some point
in the future, we're going to have to add logic to the code that handles this
anyway.

http://codereview.appspot.com/63225/diff/1/6#newcode70
Line 70: public static class Entry {
Why isn't this class UserAgent? UserAgent here is just being used as a pseudo
namespace for containing 3 other classes. This should either be 3 separate
classes (well, 1 class 1 enum and 1 interface), or everything in Entry should
just be moved to UserAgent.

http://codereview.appspot.com/63225/diff/1/10
File java/gadgets/src/main/java/org/apache/shindig/gadgets/JsLibrary.java
(right):

http://codereview.appspot.com/63225/diff/1/10#newcode74
Line 74: return true;
This is more or less a pointless check since almost every gadget rendered is
going to wind up pulling in rpc (and those that don't will have it forced on
them via the libs parameter anyway).

I think it's better to avoid the complexity and just assume that we always use
private caching for the library code.

http://codereview.appspot.com/63225/diff/1/9
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/SmartRpcJsFeatureLoader.java
(right):

http://codereview.appspot.com/63225/diff/1/9#newcode30
Line 30: public class SmartRpcJsFeatureLoader extends JsFeatureLoader {
Some comment explaining what this class is and what it's used for would be nice.
Is the regular JsFeatureLoader dumb? Why is this one smart? It looks to me like
this is just doing special stuff for the "rpc" feature, not really anything
particularly smart.

Wouldn't it be a lot easier to just extend the regular JsFeatureLoader to
support User-Agent blocks (say an attribute on <gadget> elements, just like we
already have the 'container' attribute)? 

If I need to customize rpc.js, I have to edit this class, whereas for any other
feature I only have to modify javascript. I don't see why we cant just add UA
support to the feature manifest.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b