RFR: 7167: Agent Plugin [v6]

[Joshua Matsuoka]

On Tue, 1 Jun 2021 20:07:59 GMT, Jie Kang <jkang at openjdk.org> wrote:

>> Joshua Matsuoka has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix rcp feature to use new agent plugin name
>
> application/org.openjdk.jmc.console.agent/plugin.xml line 46:
> 
>> 44:             icon="icons/agent-16.png"
>> 45:             id="org.openjdk.jmc.console.ext.agent"
>> 46:             description="The JMC Agent Plugin can be used to add JFR instrumentation declaratively to a running program"
> 
> Super minor suggestion:
> 
> s/add... declaratively/declaratively add .../

Fixed, thanks

> application/org.openjdk.jmc.console.agent/pom.xml line 68:
> 
>> 66: 							<groupId>sun.jdk</groupId>
>> 67: 							<artifactId>tools</artifactId>
>> 68: 							<version>1.8.0</version>
> 
> There's a `java.version` property defined above in this pom that doesn't seem to be used. Is it meant to be here instead of this magic version number for `sun.jdk tools`?

Yes, it should be used there, fixed.

> application/org.openjdk.jmc.console.agent/src/main/java/org/openjdk/jmc/console/agent/manager/model/CapturedValue.java line 49:
> 
>> 47: 	private static final String DEFAULT_STRING_FIELD = ""; // $NON-NLS-1$
>> 48: 	private static final Object DEFAULT_OBJECT_TYPE = null;
>> 49: 	private static final String CONVERTER_REGEX = "([a-zA-Z_$][a-zA-Z0-9_$]*\\.)*([a-zA-Z_$][a-zA-Z0-9_$]*)"; // $NON-NLS-1$
> 
> If possible I'd appreciate a comment on this Regex or class to share intention.

Added comments to both regexes, thanks

> application/org.openjdk.jmc.console.agent/src/main/java/org/openjdk/jmc/console/agent/manager/wizards/CapturedValueEditingPage.java line 222:
> 
>> 220: 		IMethodParameter parameter = (IMethodParameter) capturedValue;
>> 221: 
>> 222: 		// TODO: do this is in model instead
> 
> I don't think these todos are too good for non-authors to really know what should needs to be done instead. I'd rephrase them or remove them

These TODOs are no longer needed and have been removed now

> application/org.openjdk.jmc.console.agent/src/main/java/org/openjdk/jmc/console/agent/raweditor/internal/XmlEditor.java line 69:
> 
>> 67: 	@Override
>> 68: 	protected void createActions() {
>> 69: //		super.createActions();
> 
> Is this intentional? If so replace with comment that it's intentionally empty

Replaced it with a comment that it's intentionally empty, thanks

> application/org.openjdk.jmc.feature.console/feature.xml line 59:
> 
>> 57:          install-size="0"
>> 58:          version="0.0.0"
>> 59:          unpack="false"/> 
> 
> I think this change can be pruned

Fixed

-------------

PR: https://git.openjdk.java.net/jmc/pull/226