On Tue, May 4, 2010 at 1:00 PM, John Hjelmstad <[email protected]> wrote:
> We've implemented several RequestRewriters that aren't in Shindig, but > they'd be trivial to refactor into ResponseRewriters as described here. > > I'm unaware of anyone who's implemented a custom ImageRewriter (beyond > BasicImageRewriter). > > What more-radical reorganization might you suggest? Any additional ideas > beyond those suggested? > I don't have anything more radical, other than your proposed alternatives. My suggestions are to not worry too much about changing the interfaces if it makes the code simpler/faster. > At one point it seemed like a good idea to consolidate GadgetRewriter and > Request||ResponseRewriter somehow, but the differences still seem notable > enough as to disqualify that idea. > > --j > > On Tue, May 4, 2010 at 12:56 PM, Paul Lindner <[email protected]> wrote: > > > How many people have implemented custom rewriters? I'm guessing it's a > > small number, in which case I'd suggest a more radical reorganization of > > the > > code. > > > > In any case these are all good ideas. > > > > On Fri, Apr 30, 2010 at 5:13 PM, John Hjelmstad <[email protected]> > wrote: > > > > > Hi all, > > > > > > I'm looking into the rewriting pipeline, specifically for HTTP > responses, > > > with a few goals in mind: > > > 1. Consolidate ImageRewriter with RequestRewriter, so it's not just a > > > one-off rewriter injection. > > > - And to simplify the rewriter interfaces overall, to allow for > uniform > > > rewriter profiling code etc. > > > 2. Expand RequestRewriters' capability to modify HTTP response headers. > > > 3. Maintain - and expand - our ability to cache the result of rewriting > > > passes intelligently. Especially important for ImageRewriter, the > output > > of > > > which is presently cached for notable performance gains. > > > > > > To this end, I've created the following CL as a starting point. It > builds > > > upon the CL I recently submitted adding the ability to manipulate > > > MutableContent by byte-array: > http://codereview.appspot.com/1032042/show > > > > > > Downstream of this CL, I'm proposing to change the rewriter interface > > > roughly as follows: > > > > > > OLD > > > // Return true if content modified. > > > boolean RequestRewriter.rewrite(HttpRequest, HttpResponse, > > MutableContent); > > > > > > NEW > > > // Return value represents caching behavior. > > > // Naive strategy: return boolean. True = HttpRequest is a sufficient > key > > > to > > > cache the rewrite. Works for simple cases, but compound rewriter cases > > may > > > be more difficult. > > > // More sophisticated implementation: return a RewriteCacheKey object, > > > containing a normalized set of all inputs to the rewrite behavior > > > (essentially a compound key). > > > // In turn, these objects would be used by a RewriterCache manager > > > responsible for managing these states. > > > ??? ResponseRewriter.rewrite(HttpRequest, HttpResponseBuilder); > > > > > > Note that I suggest a new interface: ResponseRewriter (a more accurate > > name > > > for RequestRewriter anyway), so we can support both Request and > > > ResponseRewriter implementations during transition. > > > > > > Alternate approaches: > > > I also considered simply splitting out the HTTP header-manipulation > > methods > > > from HttpResponseBuilder into an interface: HttpResponseHeaders. > > > RequestRewriter's API would be modified simply as: > > > boolean RequestRewriter.rewrite(HttpRequest, HttpResponseHeaders, > > > MutableContent); > > > > > > While simpler, this had the downsides: > > > * Still involves a signature change. > > > * Doesn't account for better caching behavior. > > > * Involves 2 objects rather than 1 to manipulate for output. > > > > > > > > > At present, I'd like to go the conservative route: > > > 1. HttpResponseBuilder as MutableContent (@see CL) > > > 2. Introduce ResponseRewriter interface, returning RewriteCacheKey with > > > simple flags: HttpRequest-is-sufficient and rewrite-is-idempotent. > > > 3. Transition ImageRewriter to ResponseRewriter. > > > 4. Transition RequestRewriters to ResponseRewriter interface. > > > > > > > > > Thoughts? Input welcome. > > > > > > Cheers, > > > John > > > > > >
