Bug 351283

Comment 1 Beyhan Veliev 2011-07-07 06:40:14 EDT

Created attachment 199240 [details]
context patch patch

This patch extends the RWT-Launcher with a context path option. Includes also tests.

Comment 2 Rüdiger Herrmann 2011-07-07 07:16:19 EDT

No idea why, but the patch conflics with HEAD. The copyright headers of the incoming changes contain "Rüdiger" instead of "Rüdiger"

Comment 3 Rüdiger Herrmann 2011-07-07 07:59:22 EDT

Patch looks good, some comments though:
* NON-NLS markers are used inconsistently (see ContextPathValidator, JettyWebXmlProvider, ...)
* ContextPathValidator#isValidContextPath() should be declared static, probably there are more places with the same issue
* the variable contextPath in RWTLaunch#appendContextPath is final, why?
* the code in RWTLaunch#computeBrowserUrl should be more readable. In appendContextPath() the context path is prepended rather than appended and the resuling variable shouldn't be called 'servletName' any more. How about making it more explicit and using two patterns, one for 'no context path' and another for URLs with context path?

Wouldn't it make more sense to extract functionality that is common to both launchers (RAP and RWT) in a bundle of its own instead of duplicating code?

Comment 4 Beyhan Veliev 2011-07-07 09:20:10 EDT

Created attachment 199260 [details]
updated patch


>NON-NLS markers are used inconsistently (see ContextPathValidator, JettyWebXmlProvider, ...)
This is updated
>ContextPathValidator#isValidContextPath() should be declared static, probably there are more places with the same issue
I declared it as static. This is a candidate to be extracted as common functionality btw RWT-Launcher and RAP-Launcher.
>the variable contextPath in RWTLaunch#appendContextPath is final, why?
I had "Save Actions" active and they added it automatically. No need for this final there.
> the code in RWTLaunch#computeBrowserUrl should be more readable. In appendContextPath() the context path is prepended rather than appended and the resuling variable shouldn't >be called 'servletName' any more. How about making it more explicit and using two patterns, one for 'no context path' and another for URLs with context path?
I refactored this part.
>Wouldn't it make more sense to extract functionality that is common to both launchers (RAP and RWT) in a bundle of its own instead of duplicating code?
I want to extend RWT-Launcher with the functionality of RAP-Launcher. Then I'll extract the common functionality from both. I think this should be done as an extra step because there are other parts of the source code which also can be extracted. What do you think?

Comment 5 Rüdiger Herrmann 2011-07-07 12:46:38 EDT

(In reply to comment #4)
> [ ... ]
> >Wouldn't it make more sense to extract functionality that is common to both
> launchers (RAP and RWT) in a bundle of its own instead of duplicating code?
> I want to extend RWT-Launcher with the functionality of RAP-Launcher. Then I'll
> extract the common functionality from both. I think this should be done as an
> extra step because there are other parts of the source code which also can be
> extracted. What do you think?
If the plan is to extract common functionality, then is there any reason why don't you do it right from the start?

Comment 6 Beyhan Veliev 2011-07-08 05:00:32 EDT

(In reply to comment #5)

> If the plan is to extract common functionality, then is there any reason why
> don't you do it right from the start?
Hi Rüdiger,
the real plan was just to extend the RWT-Launcher with some features available in RAP-Launcher. When I start to extend the RWT-Launcher I saw that there are common parts which can be extracted and you mentioned it also. I also want to extract this common functionality in an extra bundle but in an extra step. IMHO an extra step will be more transparent.

Comment 7 Ivan Furnadjiev 2012-12-21 06:39:05 EST

Fixed with commit 2883380c5ae29c637529fbc91bab9289b05424b7.