|
Replies:
26
-
Last Post:
May 3, 2007 10:28 AM
by: Vladimir Sizikov
|
Threads:
[
Previous
|
Next
]
|
|
|
|
|
|
PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
Apr 26, 2007 2:44 AM
|
|
|
Guys,
please review my new update for better test export in CLDC mode:
http://fisheye4.cenqua.com/changelog/cqme/branches/users/dtrounine/testExport?cs=746
Brief description: ExportBundler and ExportBuilder are refactored: CLDC
and MIDP functionalities resides now in separate classes.
Added export of JAM files in CLDC mode.
Thanks
Dmitri.
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
Apr 27, 2007 8:50 AM
in response to: Dmitri Trounine
|
|
|
Hi Dmitri,
I've spent some time playing with this branch, haven't looked at the
sources yet.
Here are some found problems/notes:
0. (Good news) Test Export now works in CLDC mode, but...
1. When tcktests.html is created, only first link [+Sources] works and
expands. Clicking on similar links for second, third, etc., bundles
does nothing
2. In both CLDC and MIDP mode, "Description" link doesn't work,
broken.
3. If I specify empty string in "JAR URL Prefix" in order to be able
to click in Browser on various links in tcktests.html (without the
actual server), I get the NullPointerException:
java.lang.NullPointerException
at
com.sun.tck.j2me.javatest.ExportTestBuilder.init(ExportTestBuilder.java:99)
at
com.sun.tck.midp.javatest.MidExportBuilder.init(MidExportBuilder.java:97)
at
com.sun.tck.midp.javatest.MidExportBundler.createTestBuilder(MidExportBundler.j
ava:107)
at
com.sun.tck.cldc.javatest.TestBundler.reset(TestBundler.java:203)
at
com.sun.tck.midp.javatest.ExportBundler.reset(ExportBundler.java:578)
at
com.sun.tck.j2me.services.testBundlingService.TestBundlingService.start(TestBun
dlingService.java:447)
at
com.sun.tck.j2me.services.LifeCycleAwareManager.start(LifeCycleAwareManager.jav
a:61)
at
com.sun.tck.j2me.javatest.ServiceBasedTestSuite.startRun(ServiceBasedTestSuite.
java:133)
at
com.sun.tck.cldc.javatest.CldcTCKBaseTestSuite.startRun(CldcTCKBaseTestSuite.ja
va:160)
at
com.sun.tck.j2me.javatest.ExportTestSuite.startRun(ExportTestSuite.java:123)
at
com.sun.tck.j2me.javatest.ServiceBasedTestSuite.starting(ServiceBasedTestSuite.
java:214)
at com.sun.javatest.Harness.runTests(Harness.java:656)
at com.sun.javatest.Harness.access$000(Harness.java:45)
at com.sun.javatest.Harness$1.run(Harness.java:529)
4. Should we really put the URL Prefix even in links for JAD and JAR
files? Maybe, relative links would be enough? Otherwise, the links
don't work when tcktests.html is viewed in browser off the hard
drive (no server).
5. In MIDP, in trusted mode, export doesn't work, I get the exception:
java.lang.RuntimeException: Cannot create SuiteSigner instance
at
com.sun.tck.midp.javatest.MidExportBuilder.instantiateSuiteSigner(MidExportBuil
der.java:168)
at
com.sun.tck.midp.javatest.MidExportBuilder.init(MidExportBuilder.java:100)
at
com.sun.tck.midp.javatest.MidExportBundler.createTestBuilder(MidExportBundler.j
ava:107)
at
com.sun.tck.cldc.javatest.TestBundler.reset(TestBundler.java:203)
at
com.sun.tck.midp.javatest.ExportBundler.reset(ExportBundler.java:578)
at
com.sun.tck.j2me.services.testBundlingService.TestBundlingService.start(TestBun
dlingService.java:447)
at
com.sun.tck.j2me.services.LifeCycleAwareManager.start(LifeCycleAwareManager.jav
a:61)
at
com.sun.tck.j2me.javatest.ServiceBasedTestSuite.startRun(ServiceBasedTestSuite.
java:133)
at
com.sun.tck.cldc.javatest.CldcTCKBaseTestSuite.startRun(CldcTCKBaseTestSuite.ja
va:160)
at
com.sun.tck.j2me.javatest.ExportTestSuite.startRun(ExportTestSuite.java:123)
at
com.sun.tck.j2me.javatest.ServiceBasedTestSuite.starting(ServiceBasedTestSuite.
java:214)
at com.sun.javatest.Harness.runTests(Harness.java:656)
at com.sun.javatest.Harness.access$000(Harness.java:45)
at com.sun.javatest.Harness$1.run(Harness.java:529)
Caused by: java.lang.ClassNotFoundException:
com.sun.tck.midp.signer.FastJKSSigner
at java.net.URLClassLoader$1.run(URLClassLoader.java:200)
at java.security.AccessController.doPrivileged(Native Method)
at java.net.URLClassLoader.findClass(URLClassLoader.java:188)
at java.lang.ClassLoader.loadClass(ClassLoader.java:306)
at java.lang.ClassLoader.loadClass(ClassLoader.java:251)
at
com.sun.tck.midp.javatest.MidExportBuilder.instantiateSuiteSigner(MidExportBuil
der.java:162)
... 13 more
Maybe, some of these issues are known, maybe not. Please file the bugs
as appropriate.
Thanks,
--Vladimir
On Thu, Apr 26, 2007 at 01:44:29PM +0400, Dmitri Trounine wrote:
> Guys,
>
> please review my new update for better test export in CLDC mode:
>
> http://fisheye4.cenqua.com/changelog/cqme/branches/users/dtrounine/testExport?cs=746
>
> Brief description: ExportBundler and ExportBuilder are refactored: CLDC
> and MIDP functionalities resides now in separate classes.
> Added export of JAM files in CLDC mode.
>
> Thanks
> Dmitri.
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
Apr 27, 2007 9:05 AM
in response to: Vladimir Sizikov
|
|
|
Vladimir,
Thank you for having tried this update. See my comments below:
Vladimir Sizikov wrote:
> 1. When tcktests.html is created, only first link [+Sources] works and
> expands. Clicking on similar links for second, third, etc., bundles
> does nothing
>
It's subject of issue #72:
Missed sources come from agent_client.jar. The content of this JAR is
read from file when the first bundle is being created. For next bundles,
the content comes from cache and addJarFileContent is not invoked. Need
to improve caching mechanism.
> 2. In both CLDC and MIDP mode, "Description" link doesn't work,
> broken.
>
Will file an issue.
> 3. If I specify empty string in "JAR URL Prefix" in order to be able
> to click in Browser on various links in tcktests.html (without the
> actual server), I get the NullPointerException:
>
>
I observed this behavior a time ago, but thought it doesn't happen
anymore. Will file an issue.
> 4. Should we really put the URL Prefix even in links for JAD and JAR
> files? Maybe, relative links would be enough? Otherwise, the links
> don't work when tcktests.html is viewed in browser off the hard
> drive (no server).
>
tcktests.html may be used for OTA provisioning, and our RIs require
absolute links to JADs.
Maybe we could have both relative and absolute links...
> 5. In MIDP, in trusted mode, export doesn't work, I get the exception:
> java.lang.RuntimeException: Cannot create SuiteSigner instance
>
Did not observe that. Will investigate.
Thanks,
Dmitri.
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
Apr 28, 2007 2:24 AM
in response to: Dmitri Trounine
|
|
|
Guys,
if you want to review something, but are not sure about the priorities,
I encourage you to review this update. It's critical for me to commit it
ASAP, because in this update a number of classes was created and code
moved. If this update is committed, it would be much easier for me to
integrate another bug fixes in the same code without hard merging.
If you have any concerns about this update which are not directly
related to its subject "test export doesn't work in CLDC mode", I
encourage you to file new issues.
Thanks,
Dmitri.
Dmitri Trounine wrote:
> Vladimir,
>
> Thank you for having tried this update. See my comments below:
>
> Vladimir Sizikov wrote:
>> 1. When tcktests.html is created, only first link [+Sources] works and
>> expands. Clicking on similar links for second, third, etc., bundles
>> does nothing
>>
> It's subject of issue #72:
> Missed sources come from agent_client.jar. The content of this JAR is
> read from file when the first bundle is being created. For next
> bundles, the content comes from cache and addJarFileContent is not
> invoked. Need to improve caching mechanism.
>> 2. In both CLDC and MIDP mode, "Description" link doesn't work,
>> broken.
>>
> Will file an issue.
>> 3. If I specify empty string in "JAR URL Prefix" in order to be able
>> to click in Browser on various links in tcktests.html (without the
>> actual server), I get the NullPointerException:
>>
>>
> I observed this behavior a time ago, but thought it doesn't happen
> anymore. Will file an issue.
>> 4. Should we really put the URL Prefix even in links for JAD and JAR
>> files? Maybe, relative links would be enough? Otherwise, the links
>> don't work when tcktests.html is viewed in browser off the hard
>> drive (no server).
>>
> tcktests.html may be used for OTA provisioning, and our RIs require
> absolute links to JADs.
> Maybe we could have both relative and absolute links...
>> 5. In MIDP, in trusted mode, export doesn't work, I get the exception:
>> java.lang.RuntimeException: Cannot create SuiteSigner instance
>>
> Did not observe that. Will investigate.
>
> Thanks,
> Dmitri.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
Apr 28, 2007 3:04 AM
in response to: Dmitri Trounine
|
|
|
Hi Dmitri,
I've quickly reviewed the changes. Overall looks good. But I think
Vladimir should explicitly give your permission to commit since he has
started to review your update.
But I think you can simple create the branch for your further updates
from this one. If the updates for this changes will be required then you
will just merge them to another branches.
Thanks,
Alexander
Dmitri Trounine wrote:
> Guys,
>
> if you want to review something, but are not sure about the priorities,
> I encourage you to review this update. It's critical for me to commit it
> ASAP, because in this update a number of classes was created and code
> moved. If this update is committed, it would be much easier for me to
> integrate another bug fixes in the same code without hard merging.
> If you have any concerns about this update which are not directly
> related to its subject "test export doesn't work in CLDC mode", I
> encourage you to file new issues.
>
> Thanks,
> Dmitri.
>
> Dmitri Trounine wrote:
>> Vladimir,
>>
>> Thank you for having tried this update. See my comments below:
>>
>> Vladimir Sizikov wrote:
>>> 1. When tcktests.html is created, only first link [+Sources] works and
>>> expands. Clicking on similar links for second, third, etc., bundles
>>> does nothing
>>>
>> It's subject of issue #72:
>> Missed sources come from agent_client.jar. The content of this JAR is
>> read from file when the first bundle is being created. For next
>> bundles, the content comes from cache and addJarFileContent is not
>> invoked. Need to improve caching mechanism.
>>> 2. In both CLDC and MIDP mode, "Description" link doesn't work,
>>> broken.
>>>
>> Will file an issue.
>>> 3. If I specify empty string in "JAR URL Prefix" in order to be able
>>> to click in Browser on various links in tcktests.html (without the
>>> actual server), I get the NullPointerException:
>>>
>>>
>> I observed this behavior a time ago, but thought it doesn't happen
>> anymore. Will file an issue.
>>> 4. Should we really put the URL Prefix even in links for JAD and JAR
>>> files? Maybe, relative links would be enough? Otherwise, the links
>>> don't work when tcktests.html is viewed in browser off the hard
>>> drive (no server).
>>>
>> tcktests.html may be used for OTA provisioning, and our RIs require
>> absolute links to JADs.
>> Maybe we could have both relative and absolute links...
>>> 5. In MIDP, in trusted mode, export doesn't work, I get the exception:
>>> java.lang.RuntimeException: Cannot create SuiteSigner instance
>>>
>> Did not observe that. Will investigate.
>>
>> Thanks,
>> Dmitri.
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
>> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
Apr 28, 2007 3:53 AM
in response to: Alexander Alexeev
|
|
|
Alexander Alexeev wrote:
> Hi Dmitri,
>
> I've quickly reviewed the changes. Overall looks good. But I think
> Vladimir should explicitly give your permission to commit since he has
> started to review your update.
>
> But I think you can simple create the branch for your further updates
> from this one. If the updates for this changes will be required then you
> will just merge them to another branches.
Thank you Alexander! I'll do so.
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 2:46 AM
in response to: Dmitri Trounine
|
|
|
Hi Dmitri,
Sorry for the delay.
I'll be post my comments as soon as I have something to say, so that
you'll be able to get the feedback as fast as possible.
And here's the first question:
Is there a way to improve our Bundler hierarchy which looks rather
strange now:
TestBundler --> MidpBundler --> ExportBundler --> MidExportBundler
So, ExportBundler ideally should not know anything about midp mode at
all, but currently being an extension on top of MidpBundler, allows
the export bundler to know about Midp.
Even more, ExportBundler even has 'midAgentArgs' parameter in its
constructor.
I understand that it's not always possible to update the hierarchy,
but it worth at least to think about.
But it seems like inheritance is not flexible enough for us here.
Thanks,
--Vladimir
On Sat, Apr 28, 2007 at 01:24:12PM +0400, Dmitri Trounine wrote:
> Guys,
>
> if you want to review something, but are not sure about the priorities,
> I encourage you to review this update. It's critical for me to commit it
> ASAP, because in this update a number of classes was created and code
> moved. If this update is committed, it would be much easier for me to
> integrate another bug fixes in the same code without hard merging.
> If you have any concerns about this update which are not directly
> related to its subject "test export doesn't work in CLDC mode", I
> encourage you to file new issues.
>
> Thanks,
> Dmitri.
>
> Dmitri Trounine wrote:
> >Vladimir,
> >
> >Thank you for having tried this update. See my comments below:
> >
> >Vladimir Sizikov wrote:
> >>1. When tcktests.html is created, only first link [+Sources] works and
> >> expands. Clicking on similar links for second, third, etc., bundles
> >> does nothing
> >>
> >It's subject of issue #72:
> >Missed sources come from agent_client.jar. The content of this JAR is
> >read from file when the first bundle is being created. For next
> >bundles, the content comes from cache and addJarFileContent is not
> >invoked. Need to improve caching mechanism.
> >>2. In both CLDC and MIDP mode, "Description" link doesn't work,
> >> broken.
> >>
> >Will file an issue.
> >>3. If I specify empty string in "JAR URL Prefix" in order to be able
> >> to click in Browser on various links in tcktests.html (without the
> >> actual server), I get the NullPointerException:
> >>
> >>
> >I observed this behavior a time ago, but thought it doesn't happen
> >anymore. Will file an issue.
> >>4. Should we really put the URL Prefix even in links for JAD and JAR
> >> files? Maybe, relative links would be enough? Otherwise, the links
> >> don't work when tcktests.html is viewed in browser off the hard
> >> drive (no server).
> >>
> >tcktests.html may be used for OTA provisioning, and our RIs require
> >absolute links to JADs.
> >Maybe we could have both relative and absolute links...
> >>5. In MIDP, in trusted mode, export doesn't work, I get the exception:
> >>java.lang.RuntimeException: Cannot create SuiteSigner instance
> >>
> >Did not observe that. Will investigate.
> >
> >Thanks,
> > Dmitri.
> >
> >---------------------------------------------------------------------
> >To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> >For additional commands, e-mail: meframework-help@cqme.dev.java.net
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 3:04 AM
in response to: Vladimir Sizikov
|
|
|
Some more comments:
J2meTestBundlingService:
1. The following comment doesn't make sense anymore:
// We use MIDP-specific bundler for the test export in CLDC mode,
// since there is no CLDC-specific test export bundler.
2. ExportBundler not only has midAgentArgs, but also "trusted"!
I understand that you need these params so that MidExportBundler
constructor could call super constructor. But from user's pont fo
view, if the user only would like to use the base export bundler in
CLDC mode, these params do not make any sense.
I'd suggest to create a 2-param constructor (without untrusted and
midpAgentArgs), that delegates to 4 param constructor.
Alternatively, and maybe even better, it is possible to add setters
for untrusted and midpAgentArgs to the MidpBundler, then there will be
no need to have 4 param constructor in ExportBundler (and
MidExportBundler could call setters directly)
What do you think?
Thanks,
--Vladimir
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 3:24 AM
in response to: Vladimir Sizikov
|
|
|
3rd part of the comments:
J2meTestBundlingService:
I don't like the hardcoded casts in initTestBundler(), and especially
a hardcoded test suite. By doing this we effectively FORCE users to
use our test suite and make the interdependencies between independent
components, making it harder to understand, and to test in isolation.
The whole idea of services was that they are independent components
that can be reused and not hard-wired to our test suite.
For example, we use hardcoded (J2meBaseTestSuite) getTestSuite().
And if users use different TestSuite they will get an exception here.
Besides, there is need to use this class to get the info (ever more,
the info retrieved from the J2meBaseTestSuite can be not exactly
right).
The service knows about the TestEnvironment, and can get the
necessary security info from there:
SecurityInfo.Factory.fromEnv(getTestEnvironment())
ATTN: Do check against the null. null means no security info found.
We need to decide what to do when no security info is found and we are
in midp mode. Most probably, this is an error and the exception should
be thrown (MidExportBundler cannot really work with null security
policy, we'll get NPEs from it).
What do you think?
Thanks,
--Vladimir
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 3:40 AM
in response to: Vladimir Sizikov
|
|
|
In order to have a logicaly built hierarchy we need double heritage here 
Without it the hierarchy can by only linear, because MidExportBundler
uses the functionalities of all its ancestors.
Sure, TestBundler should be at the top.
So, we have two options:
1) TestBundler --> MidpBundler --> ExportBundler --> MidExportBundler
2) TestBundler --> ExportBundler --> MidpBundler --> MidExportBundler
Both have pros and cons. Common problem is that MidpBundler and
ExportBundler are orthogonal: former is for MIDP, later is for Test Export.
So, I decided to keep the case 1 as it's already functional.
Maybe we could make better hierarchy with interfaces simulating double
heritage... Does it worth?
Vladimir Sizikov wrote:
>Hi Dmitri,
>
>Sorry for the delay.
>
>I'll be post my comments as soon as I have something to say, so that
>you'll be able to get the feedback as fast as possible.
>
>And here's the first question:
>
>Is there a way to improve our Bundler hierarchy which looks rather
>strange now:
>
>TestBundler --> MidpBundler --> ExportBundler --> MidExportBundler
>
>So, ExportBundler ideally should not know anything about midp mode at
>all, but currently being an extension on top of MidpBundler, allows
>the export bundler to know about Midp.
>
>Even more, ExportBundler even has 'midAgentArgs' parameter in its
>constructor.
>
>I understand that it's not always possible to update the hierarchy,
>but it worth at least to think about.
>
>But it seems like inheritance is not flexible enough for us here.
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 4:12 AM
in response to: Dmitri Trounine
|
|
|
Dima,
On Thu, May 03, 2007 at 02:40:20PM +0400, Dmitri Trounine wrote:
> In order to have a logicaly built hierarchy we need double heritage
> here
Right
> Without it the hierarchy can by only linear, because MidExportBundler
> uses the functionalities of all its ancestors.
> Sure, TestBundler should be at the top.
>
> So, we have two options:
>
> 1) TestBundler --> MidpBundler --> ExportBundler --> MidExportBundler
> 2) TestBundler --> ExportBundler --> MidpBundler --> MidExportBundler
>
> Both have pros and cons. Common problem is that MidpBundler and
> ExportBundler are orthogonal: former is for MIDP, later is for Test Export.
>
> So, I decided to keep the case 1 as it's already functional.
Agreed. Playing inheritance tricks is not the solution here for
sure.
But maybe there other ways, e.g., to replace inheritance with
composition? For example, ExportBundler could have an internal
TestBundler, and if made properly then we could use Export bundler
with any other TestBundler. In such approach, there will be no need to
create long inheritance chains.
We'd have TestBundler (CLDC mode), MidpBundler (MidpMode) and
ExportBundler that we can use with ether of "real" bundles, delegating
most of the work to them.
Sounds simple, right? Unfortunately, in practice, it's typically
very tricky to retrofit the old hieararchy based things into this
brave new way. But I do think that it definitely worth at least
investigating and figuring out where the pain points are and what is
missing in our Bundler API.
> Maybe we could make better hierarchy with interfaces simulating double
> heritage... Does it worth?
Probably not.
Thanks,
--Vladimir
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 4:42 AM
in response to: Vladimir Sizikov
|
|
|
>We'd have TestBundler (CLDC mode), MidpBundler (MidpMode) and
>ExportBundler that we can use with ether of "real" bundles, delegating
>most of the work to them.
>
>Sounds simple, right? Unfortunately, in practice, it's typically
>very tricky to retrofit the old hieararchy based things into this
>brave new way. But I do think that it definitely worth at least
>investigating and figuring out where the pain points are and what is
>missing in our Bundler API.
>
>
I've thought about this approach. It sounds very attractive, but in fact
there are no way to implement it without big changes in TestBundler
(which is from sacred cldc package). ExportBundler uses a lot of
TestBundler's protected methods, and invoking protected method on a
worker TestBundler instance is impossible.
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 5:16 AM
in response to: Dmitri Trounine
|
|
|
Dima,
On Thu, May 03, 2007 at 03:42:59PM +0400, Dmitri Trounine wrote:
> >We'd have TestBundler (CLDC mode), MidpBundler (MidpMode) and
> >ExportBundler that we can use with ether of "real" bundles, delegating
> >most of the work to them.
> >
> >Sounds simple, right? Unfortunately, in practice, it's typically
> >very tricky to retrofit the old hieararchy based things into this
> >brave new way. But I do think that it definitely worth at least
> >investigating and figuring out where the pain points are and what is
> >missing in our Bundler API.
> >
> >
> I've thought about this approach. It sounds very attractive, but in fact
> there are no way to implement it without big changes in TestBundler
> (which is from sacred cldc package). ExportBundler uses a lot of
> TestBundler's protected methods, and invoking protected method on a
> worker TestBundler instance is impossible.
Right, there is no free lunch, and this transition from inheritance to
incapsulation typically requires changes in the main base class. So
let's just put it in the back of our brains for a while, we'll not be
able to make a major update to the base class now. But maybe some time
later..
Thanks,
--Vladimir
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 4:53 AM
in response to: Vladimir Sizikov
|
|
|
Dima,
Comments part #4:
ExportTestBuilder:
1. Typo: "Expotr".
2. In class level javadoc: Implement --> Implements.
3. incapsulates --> encapsulates
Actually, I find this comment for testExportInfo field as very ironic:
"This field incapsulates all test export specific parameters".
Actually, it does not encapsulate but rather exposes the field, which
is an opposite to encapsulation.
I'd suggest to eliminate both protected fields (testExportInfo and
appMainClassName) and provide protected getters.
4. Why do we need init() method at all? Both its params,
TestExportInfo and appMainClassName are REQUIRED for the builder
proper functionality, right? And invoking init() multiple times
leads to errrors, right? So, why not just add these both parameters
to the constructor? This way, users will not be able to use the
Builder incorrectly (they will not forget to call init() since
there will be no init(), they will not be able to call init()
twice, etc.).
5. For init():
a) "Initialize this MidExportBuilder" --> Initialize_s_ this
ExportTestBuilder.
b) "invokes instantiateSuiteSigner() method." is not true for the
base builder.
c) "untrusted Whether the tests should be created for untrusted
mode (do not sign JADs)" -- Irrelevant and not needed for the
base builder.
But I do hope that we can eliminate the init altogether
6. ressources --> resources
7. to JAR) --> to the JAR file)
8. clsses --> classes
9. Pathnama --> Pathname
10. ressource --> resource
Thanks,
--Vladimir
On Thu, May 03, 2007 at 04:12:02AM -0700, Vladimir Sizikov wrote:
> Dima,
>
> On Thu, May 03, 2007 at 02:40:20PM +0400, Dmitri Trounine wrote:
> > In order to have a logicaly built hierarchy we need double heritage
> > here
>
> Right
>
> > Without it the hierarchy can by only linear, because MidExportBundler
> > uses the functionalities of all its ancestors.
> > Sure, TestBundler should be at the top.
> >
> > So, we have two options:
> >
> > 1) TestBundler --> MidpBundler --> ExportBundler --> MidExportBundler
> > 2) TestBundler --> ExportBundler --> MidpBundler --> MidExportBundler
> >
> > Both have pros and cons. Common problem is that MidpBundler and
> > ExportBundler are orthogonal: former is for MIDP, later is for Test Export.
> >
> > So, I decided to keep the case 1 as it's already functional.
>
> Agreed. Playing inheritance tricks is not the solution here for
> sure.
>
> But maybe there other ways, e.g., to replace inheritance with
> composition? For example, ExportBundler could have an internal
> TestBundler, and if made properly then we could use Export bundler
> with any other TestBundler. In such approach, there will be no need to
> create long inheritance chains.
>
> We'd have TestBundler (CLDC mode), MidpBundler (MidpMode) and
> ExportBundler that we can use with ether of "real" bundles, delegating
> most of the work to them.
>
> Sounds simple, right? Unfortunately, in practice, it's typically
> very tricky to retrofit the old hieararchy based things into this
> brave new way. But I do think that it definitely worth at least
> investigating and figuring out where the pain points are and what is
> missing in our Bundler API.
>
> > Maybe we could make better hierarchy with interfaces simulating double
> > heritage... Does it worth?
>
> Probably not.
>
> Thanks,
> --Vladimir
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 5:13 AM
in response to: Vladimir Sizikov
|
|
|
Dima,
Comments part #5.
MidExportBuilder:
1. init() method should be eliminated as with the base Builder:
> Why do we need init() method at all? Both its params,
> TestExportInfo and appMainClassName are REQUIRED for the builder
> proper functionality, right? And invoking init() multiple times
> leads to errrors, right? So, why not just add these both parameters
> to the constructor? This way, users will not be able to use the
> Builder incorrectly (they will not forget to call init() since
> there will be no init(), they will not be able to call init()
> twice, etc.).
In this case, the third parameter (untrusted) can be enabled via
setTrusted() method, it's much more readable then "true" in 6th
constructor parameter..
2. hardcoded "agent_client.jar". We should at least put a TODO here,
and fix some time later. Hardcoded things like this are very
fragile. And if the name of the jar is changed, we'll get problems
here.
3. getExporedJads() - where is it used? I don't see any refernces to
it. Why do we need String[] here? List<String> seems better and
easier to implement.
4. protected "untrusted" and "suiteSigner" most probably should be
private.
5. shoud --> should
6. "extends TestBuilder" ---> "extends ExportTestBuilder".
Thanks,
--Vladimir
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 5:31 AM
in response to: Vladimir Sizikov
|
|
|
Dima,
Here's the final part #6 of my comments:
Exportbundler and MidExportBundler:
1. The major comment: the two exportHtmlIndex() methods are almost
identical copy-paste methods. The methods are very big and
complex, thus this copy-paste is really, really needs to be
addressed, once and for all.
2. Many methods were made protected (they were private). Do we really
need this? We always can use package-private, if we need, and by
doing so we'll not expose lots of methods to the public API.
OK, I'm done.
Thanks,
--Vladimir
On Thu, May 03, 2007 at 05:13:21AM -0700, Vladimir Sizikov wrote:
> Dima,
>
> Comments part #5.
>
> MidExportBuilder:
>
> 1. init() method should be eliminated as with the base Builder:
>
> > Why do we need init() method at all? Both its params,
> > TestExportInfo and appMainClassName are REQUIRED for the builder
> > proper functionality, right? And invoking init() multiple times
> > leads to errrors, right? So, why not just add these both parameters
> > to the constructor? This way, users will not be able to use the
> > Builder incorrectly (they will not forget to call init() since
> > there will be no init(), they will not be able to call init()
> > twice, etc.).
>
> In this case, the third parameter (untrusted) can be enabled via
> setTrusted() method, it's much more readable then "true" in 6th
> constructor parameter..
>
> 2. hardcoded "agent_client.jar". We should at least put a TODO here,
> and fix some time later. Hardcoded things like this are very
> fragile. And if the name of the jar is changed, we'll get problems
> here.
>
> 3. getExporedJads() - where is it used? I don't see any refernces to
> it. Why do we need String[] here? List<String> seems better and
> easier to implement.
>
> 4. protected "untrusted" and "suiteSigner" most probably should be
> private.
>
> 5. shoud --> should
>
> 6. "extends TestBuilder" ---> "extends ExportTestBuilder".
>
> Thanks,
> --Vladimir
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 7:17 AM
in response to: Dmitri Trounine
|
|
|
Vladimir,
Thank you for very detailed review! See my comments below:
Part 1 (about hierarchy)
------------------------
Discussing in separate thread.
Part 2
------
> J2meTestBundlingService:
> 1. The following comment doesn't make sense anymore:
> // We use MIDP-specific bundler for the test export in CLDC mode,
> // since there is no CLDC-specific test export bundler.
Fixed.
> 2. ExportBundler not only has midAgentArgs, but also "trusted"!
> I understand that you need these params so that MidExportBundler
> constructor could call super constructor. But from user's pont fo
> view, if the user only would like to use the base export bundler in
> CLDC mode, these params do not make any sense.
> I'd suggest to create a 2-param constructor (without untrusted and
> midpAgentArgs), that delegates to 4 param constructor.
> Alternatively, and maybe even better, it is possible to add setters
> for untrusted and midpAgentArgs to the MidpBundler, then there will be
> no need to have 4 param constructor in ExportBundler (and
> MidExportBundler could call setters directly)
I've implemented the last proposal: added setters for untrusted and midpAgentArgs to MidBundler.
Part 3
------
> J2meTestBundlingService:
> I don't like the hardcoded casts in initTestBundler(), and especially
> a hardcoded test suite. By doing this we effectively FORCE users to
> use our test suite and make the interdependencies between independent
> components, making it harder to understand, and to test in isolation.
...
> The service knows about the TestEnvironment, and can get the
> necessary security info from there:
> SecurityInfo.Factory.fromEnv(getTestEnvironment())
Fixed. I've removed cast to J2meBaseTestSuite.
> ATTN: Do check against the null. null means no security info found.
> We need to decide what to do when no security info is found and we are
> in midp mode. Most probably, this is an error and the exception should
> be thrown (MidExportBundler cannot really work with null security
> policy, we'll get NPEs from it).
I dont think missing security policy in Test Export mode is critical. java.policy file with information about untrusted security policy is exported just for information and does'n affect the functionality. So, we can just do nothing if security info is missing.
Fixed MidExportBundler.exportSecurityPolicy to do so.
Part 4
------
> ExportTestBuilder:
> 1. Typo: "Expotr".
Fixed.
> 2. In class level javadoc: Implement --> Implements.
Fixed.
> 3. incapsulates --> encapsulates
> Actually, I find this comment for testExportInfo field as very ironic:
> "This field incapsulates all test export specific parameters".
> Actually, it does not encapsulate but rather exposes the field, which
> is an opposite to encapsulation.
> I'd suggest to eliminate both protected fields (testExportInfo and
> appMainClassName) and provide protected getters.
Added getter, removed comments.
> 4. Why do we need init() method at all? Both its params,
> TestExportInfo and appMainClassName are REQUIRED for the builder
> proper functionality, right? And invoking init() multiple times
> leads to errrors, right? So, why not just add these both parameters
> to the constructor? This way, users will not be able to use the
> Builder incorrectly (they will not forget to call init() since
> there will be no init(), they will not be able to call init()
> twice, etc.).
...
> But I do hope that we can eliminate the init altogether
I've removed init() method from both builders and updated the constructors.
> 6. ressources --> resources
Fixed.
> 7. to JAR) --> to the JAR file)
Fixed.
> 8. clsses --> classes
Fixed.
> 9. Pathnama --> Pathname
Fixed.
> 10. ressource --> resource
Fixed.
Part 5
------
> MidExportBuilder:
> 1. init() method should be eliminated as with the base Builder:
Done.
> In this case, the third parameter (untrusted) can be enabled via
> setTrusted() method, it's much more readable then "true" in 6th
> constructor parameter..
I've added setUntrusted(boolean untrasted) method to switch between trusted/untrusted modes. Do you really preffer the name 'setTrusted()' for this method? As I see, we use the term 'untrusted' everywere in ME Framework.
> 2. hardcoded "agent_client.jar". We should at least put a TODO here,
> and fix some time later. Hardcoded things like this are very
> fragile. And if the name of the jar is changed, we'll get problems
> here.
Agree. This code looks ugly. Added @TODO tag to the comments.
> 3. getExporedJads() - where is it used? I don't see any refernces to
> it. Why do we need String[] here? List<String> seems better and
> easier to implement.
It was used in very early versions of Test Export. Just forgotten to remove it. Also removed exportedJads field which is not used no more.
> 4. protected "untrusted" and "suiteSigner" most probably should be
> private.
Made private and added protected getters.
> 5. shoud --> should
Fixed.
> 6. "extends TestBuilder" ---> "extends ExportTestBuilder".
Fixed.
Once more, thanks for your comments. Please, take look at new update:
http://fisheye4.cenqua.com/changelog/cqme/branches/users/dtrounine/testExport?cs=811
While we continue to discuss about latest changes, I'll add javadoc comments for new methods (getters, setters).
Thanks,
Dmitri.
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 7:38 AM
in response to: Dmitri Trounine
|
|
|
Hi Dmitri,
On Thu, May 03, 2007 at 06:17:23PM +0400, Dmitri Trounine wrote:
> Once more, thanks for your comments. Please, take look at new update:
>
> http://fisheye4.cenqua.com/changelog/cqme/branches/users/dtrounine/testExport?cs=811
>
> While we continue to discuss about latest changes, I'll add javadoc
> comments for new methods (getters, setters).
The proposed changes look good, thanks!
But please do add the javadocs
Also, while browsing the sources I noticed that in MidExportBuilder
you use:
signerJarUrl = signerJar.toURL();
This is not going to work with paths that have URL-unsafe chars like
space, etc.
Here's what the javadoc for toURL says:
"Usage note: This method does not automatically escape characters that
are illegal in URLs. It is recommended that new code convert an
abstract pathname into a URL by first converting it into a URI, via
the toURI method, and then converting the URI into a URL via the
URI.toURL method. "
We might postpone this after the main bugfix is done.
Feel free to file an ISSUE in the issue tracker.
So, the only big thing with the current fix that remains unsolved is
the big code duplication in index.html generating methods, right?
Thanks,
--Vladimir
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 7:57 AM
in response to: Vladimir Sizikov
|
|
|
Vladimir Sizikov wrote:
> Hi Dmitri,
>
> On Thu, May 03, 2007 at 06:17:23PM +0400, Dmitri Trounine wrote:
>
>> Once more, thanks for your comments. Please, take look at new update:
>>
>> http://fisheye4.cenqua.com/changelog/cqme/branches/users/dtrounine/testExport?cs=811
>>
>> While we continue to discuss about latest changes, I'll add javadoc
>> comments for new methods (getters, setters).
>>
>
> The proposed changes look good, thanks!
> But please do add the javadocs
>
>
Done. I've also fixed ExportBundler class which had the same problem
with unused getExportedJams method as MidExportBundler.
http://fisheye4.cenqua.com/changelog/cqme/branches/users/dtrounine/testExport?cs=812
> Also, while browsing the sources I noticed that in MidExportBuilder
> you use:
>
> signerJarUrl = signerJar.toURL();
>
> This is not going to work with paths that have URL-unsafe chars like
> space, etc.
>
> Here's what the javadoc for toURL says:
> "Usage note: This method does not automatically escape characters that
> are illegal in URLs. It is recommended that new code convert an
> abstract pathname into a URL by first converting it into a URI, via
> the toURI method, and then converting the URI into a URL via the
> URI.toURL method. "
>
> We might postpone this after the main bugfix is done.
> Feel free to file an ISSUE in the issue tracker.
>
Thank you for having noticed this issue! Will file it.
> So, the only big thing with the current fix that remains unsolved is
> the big code duplication in index.html generating methods, right?
>
>
Right. Indeed, there is a code duplication in exportHtmlIndex() methods.
It does a lot of different tasks: generates list of tests, generates
list of sources, adds javascript, adds links to JAD and JAR files etc.
It's possible to break this method into smaller parts, and override only
those of them which need to be changed. But this will expose many
internal implemetation details via protected methods.
So, we need to decide: code duplication or many new protected methods.
Which is worse?
Thanks,
Dmitri.
> Thanks,
> --Vladimir
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 8:16 AM
in response to: Dmitri Trounine
|
|
|
Dima,
On Thu, May 03, 2007 at 06:57:17PM +0400, Dmitri Trounine wrote:
> Done. I've also fixed ExportBundler class which had the same problem
> with unused getExportedJams method as MidExportBundler.
>
> http://fisheye4.cenqua.com/changelog/cqme/branches/users/dtrounine/testExport?cs=812
Some more comments for you:
1. MidExportBuilder.java:
trusted mode by calling to {link: #setUntrusted(boolean untrusted)}
There should be {@link #blahblah}?
2. MidBundler.java:
"Sets arguments for MIDletAgent. These arguments will be stored in
<code>mid_agent.jar</code> file in the test JAR bundle."
Typo: mid_agent.dat file, not jar.
3. MidBundler.java:
323 + * @return <code>SuiteSigner</code> instance, or <code>null</code> if
324 + * <code>setUntrusted(false)</code> was not invoked on this
325 + * <code>MidExportBuilder</code>.
326 + */
Hmmm, I'm not happy with this comment. You see, more often then not,
writing javadocs give us nice hints that something is not right.
Why there is such dependency between getSuiteSigner and
setUntrusted(false)? Maybe because we use getSuiteSigner()
incorrectly? Maybe we should use isUntrusted instead, in places where
we detect should we sign or not?
Why can't we instantiate signer in the constructor, once and for all?
And why setUntrusted couldn't be just a simple setter, without current
convoluted logic?
I might be missing something here...
> >So, the only big thing with the current fix that remains unsolved is
> >the big code duplication in index.html generating methods, right?
> >
> Right. Indeed, there is a code duplication in exportHtmlIndex() methods.
> It does a lot of different tasks: generates list of tests, generates
> list of sources, adds javascript, adds links to JAD and JAR files etc.
> It's possible to break this method into smaller parts, and override only
> those of them which need to be changed. But this will expose many
> internal implemetation details via protected methods.
> So, we need to decide: code duplication or many new protected methods.
> Which is worse?
There might be a better choice! Both classes are in the same
package and hence we can use package-private access, which gives us
all benefits of "protected" without exposing the public API.
Thanks,
--Vladimir
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 8:28 AM
in response to: Vladimir Sizikov
|
|
|
One more comment/question for you, Dmitri.
I wonder why do you use to-do comment in such never-seen-by-me form:
// @TODO
The canonical form recognizable by major IDEs is:
// TODO:
During the code scrubbing process, we agreed that we will use TODO
comment in form I described above.
Thanks,
--Vladimir
On Thu, May 03, 2007 at 08:16:43AM -0700, Vladimir Sizikov wrote:
> Dima,
>
> On Thu, May 03, 2007 at 06:57:17PM +0400, Dmitri Trounine wrote:
> > Done. I've also fixed ExportBundler class which had the same problem
> > with unused getExportedJams method as MidExportBundler.
> >
> > http://fisheye4.cenqua.com/changelog/cqme/branches/users/dtrounine/testExport?cs=812
>
> Some more comments for you:
>
> 1. MidExportBuilder.java:
>
> trusted mode by calling to {link: #setUntrusted(boolean untrusted)}
>
> There should be {@link #blahblah}?
>
> 2. MidBundler.java:
>
> "Sets arguments for MIDletAgent. These arguments will be stored in
> <code>mid_agent.jar</code> file in the test JAR bundle."
>
> Typo: mid_agent.dat file, not jar.
>
> 3. MidBundler.java:
> 323 + * @return <code>SuiteSigner</code> instance, or <code>null</code> if
> 324 + * <code>setUntrusted(false)</code> was not invoked on this
> 325 + * <code>MidExportBuilder</code>.
> 326 + */
>
> Hmmm, I'm not happy with this comment. You see, more often then not,
> writing javadocs give us nice hints that something is not right.
>
> Why there is such dependency between getSuiteSigner and
> setUntrusted(false)? Maybe because we use getSuiteSigner()
> incorrectly? Maybe we should use isUntrusted instead, in places where
> we detect should we sign or not?
>
> Why can't we instantiate signer in the constructor, once and for all?
> And why setUntrusted couldn't be just a simple setter, without current
> convoluted logic?
>
> I might be missing something here...
>
> > >So, the only big thing with the current fix that remains unsolved is
> > >the big code duplication in index.html generating methods, right?
> > >
> > Right. Indeed, there is a code duplication in exportHtmlIndex() methods.
> > It does a lot of different tasks: generates list of tests, generates
> > list of sources, adds javascript, adds links to JAD and JAR files etc.
> > It's possible to break this method into smaller parts, and override only
> > those of them which need to be changed. But this will expose many
> > internal implemetation details via protected methods.
> > So, we need to decide: code duplication or many new protected methods.
> > Which is worse?
>
> There might be a better choice! Both classes are in the same
> package and hence we can use package-private access, which gives us
> all benefits of "protected" without exposing the public API.
>
> Thanks,
> --Vladimir
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 9:13 AM
in response to: Vladimir Sizikov
|
|
|
Vladimir,
I've fixed the typos...
> dat file, not jar.
>
> 3. MidBundler.java:
> 323 + * @return <code>SuiteSigner</code> instance, or <code>null</code> if
> 324 + * <code>setUntrusted(false)</code> was not invoked on this
> 325 + * <code>MidExportBuilder</code>.
> 326 + */
>
> Hmmm, I'm not happy with this comment. You see, more often then not,
> writing javadocs give us nice hints that something is not right.
>
> Why there is such dependency between getSuiteSigner and
> setUntrusted(false)? Maybe because we use getSuiteSigner()
> incorrectly? Maybe we should use isUntrusted instead, in places where
> we detect should we sign or not?
>
> Why can't we instantiate signer in the constructor, once and for all?
> And why setUntrusted couldn't be just a simple setter, without current
> convoluted logic?
>
> I might be missing something here...
>
>
Let me explain...
First, SuiteSigner can be instantiated only in trusted mode. If we try
to instantiate SuiteSigner in untrusted mode, it will fail with an NPE.
We must assure that SuiteSigner is instantiated only in trusted mode.
We cannot instantiate it from within the constructor since it has no
'untrusted' argument.
So, the only remaining place to instantiate SuiteSigner is in
setUntrusted(boolean untrusted) and only if it's invoked with
untrusted=false.
I propose to add 'untrusted' argument to constructor. It will allow us
to instantiate SuiteSigner from constructor, and the javadoc comments
for getSuiteSigner() will state "returns null if untrusted is the
current security mode."
Ok?
>>> So, the only big thing with the current fix that remains unsolved is
>>> the big code duplication in index.html generating methods, right?
>>>
>>>
>> Right. Indeed, there is a code duplication in exportHtmlIndex() methods.
>> It does a lot of different tasks: generates list of tests, generates
>> list of sources, adds javascript, adds links to JAD and JAR files etc.
>> It's possible to break this method into smaller parts, and override only
>> those of them which need to be changed. But this will expose many
>> internal implemetation details via protected methods.
>> So, we need to decide: code duplication or many new protected methods.
>> Which is worse?
>>
>
> There might be a better choice! Both classes are in the same
> package and hence we can use package-private access, which gives us
> all benefits of "protected" without exposing the public API.
>
> Thanks,
> --Vladimir
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 9:40 AM
in response to: Dmitri Trounine
|
|
|
Dmitri,
On Thu, May 03, 2007 at 08:13:50PM +0400, Dmitri Trounine wrote:
> >3. MidBundler.java:
> >323 + * @return <code>SuiteSigner</code> instance, or <code>null</code> if
> >324 + * <code>setUntrusted(false)</code> was not invoked on this
> >325 + * <code>MidExportBuilder</code>.
> >326 + */
> >
> >Hmmm, I'm not happy with this comment. You see, more often then not,
> >writing javadocs give us nice hints that something is not right.
> >
> >Why there is such dependency between getSuiteSigner and
> >setUntrusted(false)? Maybe because we use getSuiteSigner()
> >incorrectly? Maybe we should use isUntrusted instead, in places where
> >we detect should we sign or not?
> >
> >Why can't we instantiate signer in the constructor, once and for all?
> >And why setUntrusted couldn't be just a simple setter, without current
> >convoluted logic?
> >
> >I might be missing something here...
> >
> >
> Let me explain...
>
> First, SuiteSigner can be instantiated only in trusted mode. If we try
> to instantiate SuiteSigner in untrusted mode, it will fail with an NPE.
>
> We must assure that SuiteSigner is instantiated only in trusted mode.
> We cannot instantiate it from within the constructor since it has no
> 'untrusted' argument.
> So, the only remaining place to instantiate SuiteSigner is in
> setUntrusted(boolean untrusted) and only if it's invoked with
> untrusted=false.
>
> I propose to add 'untrusted' argument to constructor. It will allow us
> to instantiate SuiteSigner from constructor, and the javadoc comments
> for getSuiteSigner() will state "returns null if untrusted is the
> current security mode."
>
> Ok?
Hmmm. How about to leave everything as is, but to modify
getSuiteSigner() docs and implementation a bit:
@return ... instance, or <code>null</null> in untrusted mode.
and in the method:
if (untrusted) {
return null;
} else {
return suiteSigner;
}
Thanks,
--Vladimir
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 10:26 AM
in response to: Dmitri Trounine
|
|
|
Dmitri,
On Thu, May 03, 2007 at 09:22:08PM +0400, Dmitri Trounine wrote:
> Vladimir Sizikov wrote:
> >Hmmm. How about to leave everything as is, but to modify
> >getSuiteSigner() docs and implementation a bit:
> >
> >@return ... instance, or <code>null</null> in untrusted mode.
> >
> >and in the method:
> >
> >if (untrusted) {
> > return null;
> >} else {
> > return suiteSigner;
> >}
> >
> Done. Here is new update fixing the typos and changing getSuiteSigner():
>
> http://fisheye4.cenqua.com/browse/cqme/branches/users/dtrounine/testExport/issue52/src/share/classes/com/sun/tck/midp/javatest/MidExportBuilder.java?r1=812&r2=815
> http://fisheye4.cenqua.com/browse/cqme/branches/users/dtrounine/testExport/issue52/src/share/classes/com/sun/tck/midp/javatest/MidBundler.java?r1=812&r2=814
Yep, looks good, reviewed the changes as soons as I saw the
notifications.
You've missed about 3 @TODO's, but I don't think it's really critical
at the moment.
One more typo in MidBundler.java:
Switchs --> Switches
So, the only big issue remaining is that big code duplication in html
generation.
Thanks,
--Vladimir
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 10:25 AM
in response to: Vladimir Sizikov
|
|
|
Vladimir Sizikov wrote:
>> Right. Indeed, there is a code duplication in exportHtmlIndex() methods.
>> It does a lot of different tasks: generates list of tests, generates
>> list of sources, adds javascript, adds links to JAD and JAR files etc.
>> It's possible to break this method into smaller parts, and override only
>> those of them which need to be changed. But this will expose many
>> internal implemetation details via protected methods.
>> So, we need to decide: code duplication or many new protected methods.
>> Which is worse?
>>
>
> There might be a better choice! Both classes are in the same
> package and hence we can use package-private access, which gives us
> all benefits of "protected" without exposing the public API.
>
Yes, this should work! Maybe I file an rfe about code duplication and
resolve it in separate update?
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
|
|
|
|
Re: PLease review: fix for issue #52 (test export in CLDC mode)
Posted:
May 3, 2007 10:28 AM
in response to: Dmitri Trounine
|
|
|
Dmitri,
On Thu, May 03, 2007 at 09:25:17PM +0400, Dmitri Trounine wrote:
> >There might be a better choice! Both classes are in the same
> >package and hence we can use package-private access, which gives us
> >all benefits of "protected" without exposing the public API.
> >
> Yes, this should work! Maybe I file an rfe about code duplication and
> resolve it in separate update?
Sounds like a good idea. We've done a lot today, so let's not spoil it
by adding more and more code changes. Please make the RFE P2 so that
we definitly address it in FW 1.2 time frame.
Thanks,
--Vladimir
P.S. It was fun!
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net
|
|
|
|
|
java.net RSS
