Hi Jahid,
On 15/06/2010 02:33, Md. Jahid Shohel wrote:
>
> 2) The same way DeployUtils class is package local, and I made a true copy.
I assume you needed this for #3? See my next comment.
> 3) The org.apache.Control interface was made to be both "a generic
> control" and "a deploy aware control". Which I felt more of mixing of
> concern. Shouldn't these be two separate interfaces. Like -
>
> public interface DeployAwareControl{
> public void onDeploy(ServletContext context);
> }
>
> And for backward compatibility we can make Control interface extending
> DeployAwareControl.
Click provides two ways of deploying resources. Control#onDeploy and the
Servlet3.0 approach of
packaging resources under /META-INF/resources. I think Control#onDeploy should
be deprecated as it
has some major drawbacks:
- Control has to be instantiated during startup in order to invoke onDeploy.
At startup time there
is no request, and thus no Context object. The net effect of this is that the
Control cannot rely on
Context in it's constructor. Accessing Context in the Control constructor is
useful because one can
pull data from the Session to retrieve state.
- changes to the resources have to be updated in two places, once for
getHeadElements and again for
onDeploy
> 4) The "isTemplate" method is having a bug. In the method we are not
> checking if the file is really a file or folder, Because on my Linux
> (Ubuntu) I was able to create a folder with .htm or .jsp extension. I
> know this might not happen so often, but still I think it might create
> problem.
isTemplate is used at runtime and would create overhead if we were to touch the
filesystem. I don't
see a use case for having .htm and .jsp named folders inside the webapp. That
said what is the
behavior of having such a folder defined?
> 6) When the application is loading, we load all page classes, and their
> bindable page fields. Could be a memory issue when benchmarking Click
> against other frameworks. We can load bindable page fields on demand,
> and cache them. That way we will not end up loading all page fields even
> if its not needed. For an application with 100 pages this could be a
> performance issue. As an example: say on http://click.apache.org there
> are lots of pages which is not browsed by user (example license content
> page, how to release, and pages like that), those pages will still be
> loaded. I know an application running for long time might be loading all
> pages eventually. But still, we can load them on demand, and we can
> cache loaded classes. And we can also keep the house keeping thread to
> eventually clear up rarely loaded pages.
I'd like to see lazy loading as well so feel free to open a JIRA for this. Not
only does it affect
startup but it will allow Click to run in production mode under GAE[1]. GAE
does not support the
method servletContext.getResourcePaths which XmlConfigService uses internally.
Lazy loading does
present its own problems such as synchronizing the data structures used to
store page classes and
templates.
> 7) On "getBindablePageFields" method of XmlConfigService, this piece of
> code -
>
> Field[] publicFields = pageClass.getFields();
> for (Field field : publicFields) {
> fieldMap.put(field.getName(), field);
> }
>
> Though @Bindable fields are still processed, but here we again we are
> processing all public fields (even though they have @Bindable and
> already processed). Also there are some repetition of code (on if-else)
> block. Possible to improve. Will help to get a better picture when
> benchmarking.
What optimization do you have in mind?
Kind regards
Bob
[1]: https://issues.apache.org/jira/browse/CLK-639
