978078 - Create metro tests for opening and closing "Flyout panels" via shortcuts

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Cosmin Malutan, [:cosmin-malutan]]

Create metro test for opening and closing the flyout panels that can be opened with shortcuts.
http://mxr.mozilla.org/mozilla-central/source/browser/metro/locales/en-US/chrome/browser.dtd#114

Comment 1

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.0

Here is the patch that contains the test, the Ui library for Flyout Panels, and the test for library. The library will still have to be enhanced with getElements methods. 
Also I want to say that it would be really easy to have direct-update test for metro once bug 599290 gets landed. We will have to change only the UI lib, so it will use the Flyouts for opening the metro-about flyout.

Report: 
http://mozmill-staging.blargon7.com/#/l10n/report/e248c845ffb052c61b3635551b767642

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'm on latest nightly but I can't find a way how to open those panels. Can you please explain how to do that?

Comment 3

[Cosmin Malutan, [:cosmin-malutan]]

In comment 1 I gaved the link to dtd file, just press ctrl+shift+"o" for options flyout and  ctrl+shift+"a" for about flyout.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The DTD file doesn't help that much given that you do not see the key modifications. So in this case I missed the shift key. Now it works, thanks.

Comment 5

[Andrei Eftimie]

Comment on attachment 8383719 [details] [diff] [review]
patch_v1.0

Review of attachment 8383719 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work, there are still some questions and improvements we can make:

I see all Flyout Panels are listed under a Settings panel (that looks OS controlled).
I haven't found a clean way of opening the Settings panel yet, but this might give another avenue in opening the rest of the Panels.
It would be great if we could also test opening the Flyout Panels from within this Settings Panel, but looks as it might be inaccessible to us.

Also there's a Permissions panel we miss (I can't find a shortcut for it though).
This doesn't apply cleanly (a manifest file).

::: metrofirefox/lib/tests/testFlyoutPanel.js
@@ +14,5 @@
> +}
> +
> +function testFlyoutPanel() {
> +  // Open and close the about panel
> +  flyoutPanel.openPanel('AboutFlyoutPanel', 'shortcut');

You've been using double quotes for strings throughout the lib.
Could we please preserve consistency as use them in this test as well.

::: metrofirefox/lib/ui/flyoutPanel.js
@@ +64,5 @@
> +   *          <dd>Metro AboutFlyout panel</dd>
> +   *          <dt>PrefsFlyoutPanel</dt>
> +   *          <dd>Metro Options pannel</dd>
> +   *          <dt>Metro Search Engine options panel</dt>
> +   *        </dl>

Instead of this <dl> lets use the real names this parameter can receive (those from this.panels).
Right now we're missing the 3rd one.

And lets use the syntax: aPanel="xxx"|"yyy"|"zzz"

@@ +70,5 @@
> +   *        The event used for closing the panel
> +   *        <dl>
> +   *          <dt>shortcut</dt>
> +   *          <dd>Use the escape key for closing the panel</dd>
> +   *        </dl>

Please remove the <dl>. You've already included the relevant information prior.

@@ +84,5 @@
> +    }
> +    function checkTransitioned() { self.transitioned = true; }
> +    panel.element.getNode().addEventListener("transitionend", checkTransitioned);
> +
> +    switch (type) {

From what I tested we can also close the panel by issuing a touch event (tap?) anywhere outside of the flyout panel.
We might want to try implementing that as well.

@@ +95,5 @@
> +
> +    try {
> +      assert.waitFor(function () {
> +        return self.transitioned;
> +      }, "Pannel '" + aPanel + "' has been opened");

[...] has been closed

@@ +126,5 @@
> +   *          <dd>Metro AboutFlyout panel</dd>
> +   *          <dt>PrefsFlyoutPanel</dt>
> +   *          <dd>Metro Options pannel</dd>
> +   *          <dt>Metro Search Engine options panel</dt>
> +   *        </dl>

Same thing with the options as in the close method. Also for the open method.

@@ +176,5 @@
> +        if (!panel.dtdKey) {
> +          assert.fail("Panel '" + aPanel + "' cannot be opened with a shortcut");
> +        }
> +        let cmdKey = utils.getEntity(this.getDtds(), panel.dtdKey);
> +        this._controller.keypress(null, cmdKey, {accelKey: true, shiftKey: true});

Calling actions against the controller is deprecated. And will probably get phased out in a future mozmill version.
You should use Elem.action, where in this case the element might be the panel element directly.

@@ +200,5 @@
> +
> +  /**
> +   * Bootstrap function for retrieving the flyouts elements and caching them
> +   */
> +  setPanels: function FlyoutPanel_setPanels() {

I'm not sure how much this function helps us.
We're only using the panel.id here, so instead of setting the id, then iterating trough all objects and caching the element,
wouldn't it be easier to set the element directly in the `panels` object?

What I am suggesting is to remove the 'id' completely, remove the `setPanels` function, and set `element` directly on the `panels` object at init time.

Comment 6

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.0

Thanks for review Andrei.
This is the updated patch.
http://mozmill-staging.blargon7.com/#/l10n/report/a438ea29b921b2e8124749eda9630b39

Comment 7

[Andrei Eftimie]

Comment on attachment 8386085 [details] [diff] [review]
patch_v2.0

Review of attachment 8386085 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/lib/ui/flyoutPanel.js
@@ +25,5 @@
> +  // FlyoutPanlesUI elements data
> +  // panelEntry is index of item in the "Settings" menu in the Windows 8 charms
> +  this.panels = {
> +    AboutFlyoutPanel: {
> +      id: "about-flyoutpanel",

As we now have the element directly here, I think we can remove the `id` as I've said int he last review.

@@ +36,5 @@
> +      id: "prefs-flyoutpanel",
> +      element: new findElement.ID(this._controller.window.document,
> +                                  "prefs-flyoutpanel"),
> +      panelEntry: "0",
> +      dtdKey: "optionsFlyout.key"},

Please be consistent. Have the closing bracket either at the end of the last property, or on a newline, but don't combine both styles in a single object.
I find it easier to read if it's on a newline, but I'm really fine either way. Just be consistent.

@@ +68,5 @@
> +   */
> +  closePanel: function FlyoutPanel_closePanel(aPanel, aEventType) {
> +    let type = aEventType || "shortcut";
> +    if (!this.panels[aPanel]) {
> +      assert.fail("Unknown panel type - " + aPanel)

nit: missing semicolon

@@ +70,5 @@
> +    let type = aEventType || "shortcut";
> +    if (!this.panels[aPanel]) {
> +      assert.fail("Unknown panel type - " + aPanel)
> +    }
> +    let panel = this.panels[aPanel];

Hoist this assignment before the previous if check, and use `panel` for the actual check. It will be more DRY.

@@ +91,5 @@
> +      default:
> +        assert.fail("Unknown event type - " + type);
> +    }
> +
> +    try {

Please also include the `switch` in this try. If we fail there we won't catch it.
As in all other places where we are using this construct, everything should be wrapped in a try/finally right after we attach the listener.

@@ +106,5 @@
> +  /**
> +   * Gets all the needed external DTD urls as an array
> +   *
> +   * @returns Array of external DTD urls
> +   * @type [string]

This should be @returns {[string]} [...]

@@ +144,5 @@
> +    let type = aEventType || "shortcut";
> +    if (!this.panels[aPanel]) {
> +      assert.fail("Unknown panel type - " + aPanel)
> +    }
> +    let panel = this.panels[aPanel];

Same as above please.

Comment 8

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.1

I fixed those, thanks for review.

Comment 9

[Andrei Eftimie]

Comment on attachment 8386728 [details] [diff] [review]
patch_v2.1

Review of attachment 8386728 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me & works fine. 
Just 2 small issues mentioned inline.

Please request a review from Henrik with these updated.

::: metrofirefox/lib/tests/testFlyoutPanel.js
@@ +20,5 @@
> +
> +  flyoutPanel.openPanel("AboutFlyoutPanel", "metroSettings");
> +  flyoutPanel.closePanel("AboutFlyoutPanel", "tap");
> +
> +  // Open and close the about panel

pref panel?

@@ +29,5 @@
> +  flyoutPanel.closePanel("PrefsFlyoutPanel", "tap");
> +
> +  // Open and close the search options panel
> +  flyoutPanel.openPanel("SearchFlyoutPanel", "metroSettings");
> +  flyoutPanel.closePanel("SearchFlyoutPanel", "shortcut");

I think we can open this twice with the same method so we can test both close methods.

Comment 10

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.2

I fixed the nits, thanks Andrei!

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8389245 [details] [diff] [review]
patch_v2.2

Review of attachment 8389245 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/lib/tests/testFlyoutPanel.js
@@ +12,5 @@
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.flyoutPanel = new flyoutPanel.FlyoutPanel(aModule.controller);
> +}
> +
> +function testFlyoutPanel() {

We don't need test duplication. Please check what the other library tests are doing. All the stuff below is not necessary.

::: metrofirefox/lib/ui/flyoutPanel.js
@@ +17,5 @@
> + */
> +function FlyoutPanel(aController) {
> +  if (!aController) {
> +    assert.fail("A valid controller must be specified");
> +  }

Not sure why you are using the if condition here. Simply use assert.ok().

@@ +21,5 @@
> +  }
> +
> +  this._controller = aController;
> +
> +  // FlyoutPanlesUI elements data

spelling mistake

@@ +22,5 @@
> +
> +  this._controller = aController;
> +
> +  // FlyoutPanlesUI elements data
> +  // panelEntry is index of item in the "Settings" menu in the Windows 8 charms

Comments should be as close as possible to the code which gets explained.

@@ +23,5 @@
> +  this._controller = aController;
> +
> +  // FlyoutPanlesUI elements data
> +  // panelEntry is index of item in the "Settings" menu in the Windows 8 charms
> +  this.panels = {

I don't see why we would need that, especially in the constructor.

@@ +25,5 @@
> +  // FlyoutPanlesUI elements data
> +  // panelEntry is index of item in the "Settings" menu in the Windows 8 charms
> +  this.panels = {
> +    AboutFlyoutPanel: {
> +      element: new findElement.ID(this._controller.window.document,

You know that we have to use the getElements() method for retrieving elements. Why has this passed all the previous reviews?

@@ +58,5 @@
> +
> +  /**
> +   * Close a panel
> +   *
> +   * @param {String} aPanel="AboutFlyoutPanel"|"PrefsFlyoutPanel"|"SearchFlyoutPanel"

I would define constants we could use and drop the 'FlyoutPanel' so we simply have 'about', 'prefs', and others.

@@ +67,5 @@
> +  closePanel: function FlyoutPanel_closePanel(aPanel, aEventType) {
> +    let type = aEventType || "shortcut";
> +    let panel = this.panels[aPanel];
> +    if (!panel) {
> +      assert.fail("Unknown panel type - " + aPanel);

assert.ok() please.

@@ +69,5 @@
> +    let panel = this.panels[aPanel];
> +    if (!panel) {
> +      assert.fail("Unknown panel type - " + aPanel);
> +    }
> +    let self = {

I would love to see an empty line above. Also this could be a single line.

@@ +84,5 @@
> +          break;
> +        case "tap":
> +          let document = new findElement.Elem(this._controller.window.document.
> +                                              documentElement);
> +          document.tap();

Why we have to tap the document? Can't it also be the window?

@@ +92,5 @@
> +      }
> +
> +      assert.waitFor(function () {
> +        return self.transitioned;
> +      }, "Pannel '" + aPanel + "' has been closed");

Please use an arrow function: assert.waitFor(() => this.transitioned, "message")

@@ +161,5 @@
> +          win.keypress(cmdKey, {accelKey: true, shiftKey: true});
> +          break;
> +        case "metroSettings":
> +          Services.obs.notifyObservers(null, "metro-settings-entry-selected",
> +                                       panel.panelEntry);

What should this be? I don't see that we use any user action here.

::: metrofirefox/tests/l10n/manifest.ini
@@ +1,1 @@
> +[include:testAccessKeys/manifest.ini]

This is not a l10n test, but a usual functional test.

::: metrofirefox/tests/l10n/testAccessKeys/testFlyoutPanelsViaShortcut.js
@@ +12,5 @@
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.flyoutPanel = new flyoutPanel.FlyoutPanel(aModule.controller);
> +}
> +
> +function testFlyoutPanelsViaShortcut() {

Please add a comment to the function with the bug reference.

@@ +19,5 @@
> +  flyoutPanel.closePanel("AboutFlyoutPanel", "shortcut");
> +
> +  // Open and close the options panel
> +  flyoutPanel.openPanel("PrefsFlyoutPanel", "shortcut");
> +  flyoutPanel.closePanel("PrefsFlyoutPanel", "shortcut");

All comments above are superfluous. Also what about the search panel?

Comment 12

[Daniel Gherasim]

I will continue working on this to have it finished quicker.

Comment 13

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #11)
> @@ +84,5 @@
> > +          break;
> > +        case "tap":
> > +          let document = new findElement.Elem(this._controller.window.document.
> > +                                              documentElement);
> > +          document.tap();
> 
> Why we have to tap the document? Can't it also be the window?
> 

From the mozelement.js code:  "throw new Error("A window cannot be a target for mouse events.");"

> 
> @@ +161,5 @@
> > +          win.keypress(cmdKey, {accelKey: true, shiftKey: true});
> > +          break;
> > +        case "metroSettings":
> > +          Services.obs.notifyObservers(null, "metro-settings-entry-selected",
> > +                                       panel.panelEntry);
> 
> What should this be? I don't see that we use any user action here.
> 

We currently don't have an UI element in the metro interface to open the Settings menu or any item from it, just thrugh the backend "Services" module, so I removed this case.

> @@ +19,5 @@
> > +  flyoutPanel.closePanel("AboutFlyoutPanel", "shortcut");
> > +
> > +  // Open and close the options panel
> > +  flyoutPanel.openPanel("PrefsFlyoutPanel", "shortcut");
> > +  flyoutPanel.closePanel("PrefsFlyoutPanel", "shortcut");
> 
> All comments above are superfluous. Also what about the search panel?

Search flyout panel doesn't currently have a shortcut or a Firefox Metro related UI Element assigned, we can open it just using backend code.

There was actually a work in progress to also add a sync flyout with a new shortcut that's disabled for the moment (http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser.xul#282)

Comment 14

[Daniel Gherasim]

Attachment: bug_978078_fix.patch

Updated the patch with the changes requested.

Comment 15

[Andrei Eftimie]

Comment on attachment 8393519 [details] [diff] [review]
bug_978078_fix.patch

Review of attachment 8393519 [details] [diff] [review]:
-----------------------------------------------------------------

We still have to do some changes.

::: metrofirefox/lib/ui/flyoutPanel.js
@@ +8,5 @@
> +
> +var { assert } = require("../../../lib/assertions");
> +var utils = require("../../../firefox/lib/utils");
> +
> +const FLYOUT_PANELS = ["about", "options", "search"];

This doesn't appear to be used anymore.

@@ +99,5 @@
> +   * @param {object} [subtype=""]
> +   *        Attribute of the element to filter
> +   * @param {object} [value=""]
> +   *        Value of the attribute to filter
> +   * @param {object} [parent=document]

There's no value and parent in this particular method.

@@ +119,5 @@
> +   * @param {Object} aSpec.type
> +   *        General type information
> +   * @param {Object} [aSpec.subtype]
> +   *        Specific element or property
> +   * @param {Object} [aSpec.value]

There's no value in this particular method.

@@ +128,5 @@
> +  getElements : function flyoutPanel_getElements(aSpec) {
> +    var spec = aSpec || {};
> +    var elem = null;
> +
> +    switch(aSpec.type) {

This should be `spec`

@@ +129,5 @@
> +    var spec = aSpec || {};
> +    var elem = null;
> +
> +    switch(aSpec.type) {
> +      case "about":

All panels should be of type panel, we'll differentiate on subtype.
The spec object should be similar to this: 
> {type: "panel", subtype: "about"}

@@ +142,5 @@
> +        elem = new findElement.ID(this._controller.window.document,
> +                                  "search-flyoutpanel")
> +        break;
> +      default:
> +       assert.fail("Unknown element type - " + aSpec.type);

spec

@@ +157,5 @@
> +   *
> +   * @returns {MozElement}
> +   *          The Panel element
> +   */
> +  getPanel: function FlyoutPanel_getPanel(aPanel) {

we should remove this method and use getElement directly.

Comment 16

[Daniel Gherasim]

Attachment: bug_978078_fix_v2.patch

Updated patch with requested changes.

Comment 17

[Andrei Eftimie]

Comment on attachment 8394639 [details] [diff] [review]
bug_978078_fix_v2.patch

Review of attachment 8394639 [details] [diff] [review]:
-----------------------------------------------------------------

With the nits fixed this has r+ from me.
Please ask a review from Henrik afterwards.

::: metrofirefox/lib/ui/flyoutPanel.js
@@ +125,5 @@
> +      case "panel":
> +        switch(spec.subtype) {
> +          case "about":
> +            elem = new findElement.ID(this._controller.window.document,
> +                                  "about-flyoutpanel");

nit: indentation

@@ +158,5 @@
> +  openPanel: function FlyoutPanel_openPanel(aPanel, aEventType) {
> +    var type = aEventType || "shortcut";
> +    var panel = this.getElement({type: "panel", subtype: aPanel});
> +
> +    var self = {

You could leave this on a single line as mentioned a few comments back.

Comment 18

[Daniel Gherasim]

Attachment: bug_978078_fix_v3.patch

Patch updated.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8394800 [details] [diff] [review]
bug_978078_fix_v3.patch

Review of attachment 8394800 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/lib/ui/flyoutPanel.js
@@ +8,5 @@
> +
> +var { assert } = require("../../../lib/assertions");
> +var utils = require("../../../firefox/lib/utils");
> +
> +const SHORTCUTS = {

The constant should be named 'PANELS' and is a dict of type panel, which itself contains a shortcut property. For search we should leave the shortcut as null.

@@ +49,5 @@
> +  closePanel: function flyoutPanel_closePanel(aPanel, aEventType) {
> +    var type = aEventType || "shortcut";
> +    var panel = this.getElement({type: "panel", subtype: aPanel});
> +
> +    var self = { transitioned: false }

missing semicolon.

@@ +123,5 @@
> +
> +    switch(spec.type) {
> +      case "panel":
> +        switch(spec.subtype) {
> +          case "about":

I do not see why we need a subtype here. Nothing else than a panel will be returned. So let it get specified via type.

@@ +158,5 @@
> +  openPanel: function FlyoutPanel_openPanel(aPanel, aEventType) {
> +    var type = aEventType || "shortcut";
> +    var panel = this.getElement({type: "panel", subtype: aPanel});
> +
> +    var self = { transitioned: false }

missing semicolon.

::: metrofirefox/tests/functional/testFlyoutPanels/testOpenCloseShortcuts.js
@@ +10,5 @@
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.flyoutPanel = new flyoutPanel.FlyoutPanel(aModule.controller);
> +}
> +

What about a teardownModule method to close a possible still open flyout panel?

Comment 20

[Daniel Gherasim]

Attachment: bug_978078_fix_v4.patch

Updated patch with the small changes asked.
Also added "closeAllPanels()" to the library to close a panel if it's opened and a force parameter to closePanel method.

Comment 21

[Andrei Eftimie]

Comment on attachment 8397667 [details] [diff] [review]
bug_978078_fix_v4.patch

Review of attachment 8397667 [details] [diff] [review]:
-----------------------------------------------------------------

A few nits and a small update to the lib test needed and I think we're good.

::: metrofirefox/lib/tests/testFlyoutPanel.js
@@ +22,5 @@
> +}
> +
> +function testFlyoutPanel() {
> +  ELEMENTS.forEach(function (aElement) {
> +    var element = flyoutPanel.getElement({type: "panel", subtype: aElement.name});

I think this needs to be updated. It currently fails.

::: metrofirefox/lib/ui/flyoutPanel.js
@@ +58,5 @@
> +    var panel = this.getElement({type: aPanel});
> +    if (force) {
> +      panel.getNode().hide();
> +      return;
> +    }

nit: should have a newline before and after this if block.

@@ +92,5 @@
> +   *
> +   * @param {Boolean} [aForce]
> +   *        Force closing the opened panel
> +   */
> +  closeAllPanels: function flyoutPanel_closeAllPanels(aForce) {

nit: missing space before the colon (half of the methods have 1 space, the other half don't)

@@ +93,5 @@
> +   * @param {Boolean} [aForce]
> +   *        Force closing the opened panel
> +   */
> +  closeAllPanels: function flyoutPanel_closeAllPanels(aForce) {
> +    for(var prop in PANELS) {

nit: missing space between for paranthesis

@@ +186,5 @@
> +    try {
> +      switch (type) {
> +        case "shortcut":
> +          assert.ok(PANELS[aPanel].shortcut,
> +                    "Panel '" + aPanel + "' cannot be opened with a shortcut");

[..] can be opened [..]

Comment 22

[Daniel Gherasim]

Attachment: bug_978078_fix_v5.patch

Thanks, 
I fixed the nits.
Hope it's ok now!

Comment 23

[Andrei Eftimie]

Comment on attachment 8397740 [details] [diff] [review]
bug_978078_fix_v5.patch

Review of attachment 8397740 [details] [diff] [review]:
-----------------------------------------------------------------

I've updated the commit message.
This works fine, and is good enough.

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/7bfd960b8502 (default)

::: metrofirefox/lib/ui/flyoutPanel.js
@@ +95,5 @@
> +   * @param {Boolean} [aForce]
> +   *        Force closing the opened panel
> +   */
> +  closeAllPanels : function flyoutPanel_closeAllPanels(aForce) {
> +    for(var prop in PANELS) {

You forgot to add a space here. I've added it in the final patch.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Why has this been landed without my final review? I haven't given a r+ yet.

Comment 25

[Andrei Eftimie]

1) Your review in comment 19 contained nits and one change to force close the panel in the teardownModule (which have been resolved)
2) This is a Metro test and we want to finish them with minimal effort if possible.

The line is still fuzzy for when you want to have another review or not :(

I've had a similar scoped bug a couple of weeks ago where you asked why another review was requested from you instead of being landed directly (bug 924077 comment 37)

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

My review comment in comment not only contained nits. I pointed out an API usage which is not correct and which should be fixed. As agreed on that API changes need my review, this shouldn't have been landed without my final review. Also I haven't said that an updated patch with the comments is fine with me.

Even it is a Metro test you should obey the rules we have setup. There is no reason to dismiss them.

Comment 27

[Andrei Eftimie]

Henrik, why are we still only discussing this?

I've landed this without asking for another round of review.
It was a mistake made because of the reasons mentioned in comment 25.

Let's not dwell on it please and take action. Either please review the patch and ask for follow-up fixes, or let me know if you want me to back it out. I don't see any reason to be mysterious here and just throw blame around.

Just being passive-aggressive doesn't help up progress in any way... :(

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andrei Eftimie from comment #27)
> Henrik, why are we still only discussing this?

Because it has been fixed in the latest patch. Otherwise I would have made my comments for follow-up fixes.

> Just being passive-aggressive doesn't help up progress in any way... :(

This was a clarification of things we have agreed on. Nothing in there was meant to be aggressive in any way.