On Sat, Feb 27, 2016 at 2:42 PM, sebb <seb...@gmail.com> wrote:
> On 27 February 2016 at 13:06, Philippe Mouawad
> <philippe.moua...@gmail.com> wrote:
> > Hi,
> > I changed code a bit in r1732634 because there was a bug anyway.
> >
> > My answers inline.
> >
> > Regards
> > Philippe
> >
> > On Sat, Feb 27, 2016 at 1:54 PM, sebb <seb...@gmail.com> wrote:
> >
> >> On 27 February 2016 at 12:09, Philippe Mouawad
> >> <philippe.moua...@gmail.com> wrote:
> >> > On Sat, Feb 27, 2016 at 2:01 AM, <s...@apache.org> wrote:
> >> >
> >> >> Author: sebb
> >> >> Date: Sat Feb 27 01:01:07 2016
> >> >> New Revision: 1732590
> >> >>
> >> >> URL: http://svn.apache.org/viewvc?rev=1732590&view=rev
> >> >> Log:
> >> >> Revert change that broke JMX compatibility
> >> >> TODO fix GUI to support change in default
> >> >>
> >> >> Modified:
> >> >>
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
> >> >>
> >> >> Modified:
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
> >> >> URL:
> >> >>
> >>
> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java?rev=1732590&r1=1732589&r2=1732590&view=diff
> >> >>
> >> >>
> >>
> ==============================================================================
> >> >> ---
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
> >> >> (original)
> >> >> +++
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
> >> >> Sat Feb 27 01:01:07 2016
> >> >> @@ -101,8 +101,13 @@ public class CookieManager extends Confi
> >> >> private transient CookieHandler cookieHandler;
> >> >>
> >> >> private transient CollectionProperty initialCookies;
> >> >> +
> >> >> + // MUST NOT BE CHANGED
> >> >>
> >> > A more explicit note should be here to explain why it must not be
> >> changed:
> >> > - Reference the bug or the case
> >>
> >> I can expand the text but the reason has little to do with the bug or
> >> the test case
> >> The reason it cannot be changed is that it relates to how JMX files
> >> are read/written.
> >> Upwards compatibility requires immutable default values
> >>
> >> >> + @SuppressWarnings("deprecation") // cannot be changed
> >> >> + public static final String DEFAULT_POLICY =
> >> >> CookieSpecs.BROWSER_COMPATIBILITY;
> >> >>
> >> > Thinking more about it, keeping "Wrong Default" while the good one
> are in
> >> > GUI does not seem to me great:
> >> > - Code readability, HC4CookieHandler is the real default, developers
> or
> >> > users might think it is still HC3CookieHandler
> >>
> >> That is not the whole story, see below
> >>
> >> > - These Constants will disappear in next HttpClient 5 version , they
> are
> >> > already deprecated
> >>
> >> No, they cannot just disappear.
> >>
> > I am speaking about the HC constants, they are already gone.
> > I understand our constants cannot disappear.
> >
> >>
> >> There are two defaults in operation here.
> >>
> >> * The defaults used in connection with reading/writing JMX files.
> >> These must remain immutable otherwise JMX files are not upwards
> compatible.
> >> For the same reason we cannot change the names of other attributes in
> JMX
> >> files.
> >> The actual values of defaults are largely arbitrary, but are chosen to
> >> be the commonest values.
> >>
> > I am aware of that
> >
> >>
> >> * The defaults used when adding a new test element to a plan.
> >> These can be changed at will.
> >>
> >
> > They can if we stop adding the 3rd parameter to setProperty
>
> If we do that for existing setProperty methods that will cause
> spurious JMX file changes which could confuse users.
>
I agree and this is the case for CookieManager, but at least it is safe.
Let's limit ourselves for now to only CookieManager
It will also affect some unit tests, and may make it harder to detect
> unintended JMX changes.
>
Yes
>
> But yes, we don't need apply defaults for new setProperty methods.
>
Yes
> However if the field has an obvious default that seems very unlikely
> to change it may be better to continue to use a default.
>
Yes, but the problem is how to know that.
>
> >>
> >> Now if the default JMX value ceases to have a meaning, then we have a
> >> different problem.
> >>
> >
> > The old defaults for CookieManager have the following status:
> > - HC3CookieHandler
> > - default for HC4 and HC3 have different behaviour. HC3 default is closer
> > to HC4 standard , but HC4 supports up to date RFC, not HC3.
> >
> >
> >
> >> In the case of class names, this is generally handled by fixing the
> >> JMX file on input.
> >> The code could be extended for field values, or the GUI code could
> >> convert invalid values.
> >>
> >> >
> >> >>
> >> >> - public static final String DEFAULT_IMPLEMENTATION =
> >> >> HC4CookieHandler.class.getName();
> >> >> + // MUST NOT BE CHANGED
> >> >> + public static final String DEFAULT_IMPLEMENTATION =
> >> >> HC3CookieHandler.class.getName();
> >> >>
> >> >> public CookieManager() {
> >> >> clearCookies(); // Ensure that there is always a collection
> >> >> available
> >> >> @@ -119,11 +124,11 @@ public class CookieManager extends Confi
> >> >> }
> >> >>
> >> >> public String getPolicy() {
> >> >> - return getPropertyAsString(POLICY,
> >> >> HC4CookieHandler.DEFAULT_POLICY_NAME);
> >> >> + return getPropertyAsString(POLICY, DEFAULT_POLICY);
> >> >> }
> >> >>
> >> >> public void setCookiePolicy(String policy){
> >> >> - setProperty(POLICY, policy,
> >> HC4CookieHandler.DEFAULT_POLICY_NAME);
> >> >> + setProperty(POLICY, policy, DEFAULT_POLICY);
> >> >> }
> >> >>
> >> >> public CollectionProperty getCookies() {
> >> >>
> >> >>
> >> >>
> >> >
> >> >
> >> > --
> >> > Cordialement.
> >> > Philippe Mouawad.
> >>
> >
> >
> >
> > --
> > Cordialement.
> > Philippe Mouawad.
>
--
Cordialement.
Philippe Mouawad.
