924074 - Write mozmill metro test for stop navigation and reload a page

Status: RESOLVED FIXED

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

Description

[Andreea Matei [:AndreeaMatei]]

This would be similar to our current test:
http://hg.mozilla.org/qa/mozmill-tests/file/default/tests/remote/testToolbar/testStopReloadButtons.js

Here are the steps for metro:
reload: https://bug831902.bugzilla.mozilla.org/attachment.cgi?id=703470
stop: https://bug831907.bugzilla.mozilla.org/attachment.cgi?id=703476

Comment 2

[mario garbi]

Attachment: newMetro_stopReloadPage.patch

This is dependent on the restructure and metrolib patches and will only apply cleanly after importing those two first.

Comment 3

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 830800 [details] [diff] [review]
newMetro_stopReloadPage.patch

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

::: metrofirefox/tests/functional/testToolbar/testStopReloadButtons.js
@@ +8,5 @@
> +var { assert } = require("../../../../lib/assertions");
> +var toolbars = require("../../../lib/ui/toolbars");
> +var utils = require("../../../../firefox/lib/utils");
> +
> +const TEST_DATA = "www.mozilla.org/en-US/about/contact#map-mountain_view";

This tests should be under remote.

@@ +16,5 @@
> +  aModule.locationBar = new toolbars.LocationBar(aModule.controller);
> +}
> +
> +function teardownModule(aModule) {
> +}

Nothing to clear at all?

@@ +26,5 @@
> +  // Go to the URL and start loading for some milliseconds
> +  controller.open(TEST_DATA);
> +  assert.waitFor( function () {
> +    return locationBar.contains(TEST_DATA) ;
> +  }, "The page started loading");

Not sure how this is relevant. I can type in the location bar but not hit enter, doesn't mean the page is loading.
Also, are you sure it won't finish loading really quickly until you hit stop? It should be an event here.

@@ +30,5 @@
> +  }, "The page started loading");
> +  
> +  var stopButton = locationBar.getElement({type: "stopButton"});
> +  controller.click(stopButton);
> +  

Whitespaces between several blocks.

@@ +36,5 @@
> +  // button extremely fast
> +  var footer = new elementslib.ID(controller.tabs.activeTab, "footer-right");
> +  assert.ok(!footer.exists(), "'Footer' element has not been found");
> +  
> +  // Reload, wait for it to completely loading and test again

nit: completely load

Comment 4

[mario garbi]

Attachment: stopReload.patch

Updated the patch and tested, unfortunately we can't get dashboard reports yet.

Comment 5

[Andrei Eftimie]

You can try sending Metro reports to sandbox right now:
http://mozmill-sandbox.blargon7.com/

Comment 6

[mario garbi]

Attachment: stopReload_v1.patch

Updated the test again and added an event observer, also used mozmill-sandbox for reports:

http://mozmill-sandbox.blargon7.com/#/remote/report/94e33fe3d7ec0be6dbbfa0a702a01f77

Comment 7

[Andrei Eftimie]

Comment on attachment 8362864 [details] [diff] [review]
stopReload_v1.patch

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

::: metrofirefox/tests/remote/testToolbar/testStopReloadButtons.js
@@ +8,5 @@
> +var { assert } = require("../../../../lib/assertions");
> +var toolbars = require("../../../lib/ui/toolbars");
> +var utils = require("../../../../firefox/lib/utils");
> +
> +const TEST_DATA = "http://www.mozilla.org/en-US/contact/spaces/mountain-view/";

Since we are using the unload event this test might work with a local page.
If we manage to make it work, we should move this as part of the functional testrun.

@@ +12,5 @@
> +const TEST_DATA = "http://www.mozilla.org/en-US/contact/spaces/mountain-view/";
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.locationBar = new toolbars.LocationBar(aModule.controller);

This fails right now, you only need to initialize the toolbar, it will contain an instance for all toolbars.

This has changed in the UI lib.

Comment 8

[mario garbi]

Attachment: stopReload_v2.patch

Updated patch as requested

Comment 9

[mario garbi]

Attachment: stopReload_v3.patch

Moved the test to functional folder as we no longer use remote pages.

Comment 10

[mario garbi]

Comment on attachment 8367933 [details] [diff] [review]
stopReload_v3.patch

For some reason bugzilla wouldn't let me set the review request when I uploaded the patch.

Comment 11

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8367933 [details] [diff] [review]
stopReload_v3.patch

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

::: metrofirefox/tests/functional/testToolbar/testStopReloadButtons.js
@@ +26,5 @@
> +  var contentWindow = controller.tabs.activeTab.defaultView;
> +
> +  function onUnload() {
> +   contentWindow.removeEventListener("unload", onUnload, false);
> +   pageUnloaded = true;

I think it's just one space indented here. Please make a try/finally block and move the removal of the event listener in finally.

@@ +39,5 @@
> +    return pageUnloaded;
> +  }, "about:blank page has been unloaded.", 2000, 10);
> +
> +  var stopButton = locationBar.getElement({type: "stopButton"});
> +  controller.click(stopButton);

We should use tap and all the touch events in the metro tests as we try to replicate the user's experience: element.tap().

@@ +42,5 @@
> +  var stopButton = locationBar.getElement({type: "stopButton"});
> +  controller.click(stopButton);
> +
> +  // Even an element at the top of a page shouldn't exist when we hit the stop
> +  // button extremely fast

The comment is a bit wrong as we use footer and it's not at the top of the page.

@@ +48,5 @@
> +  assert.ok(!footer.exists(), "'organization' element has not been found");
> +
> +  // Reload, wait for it to completely load and test again
> +  var reloadButton = locationBar.getElement({type: "reloadButton"});
> +  assert.waitFor( function () {

No need for the space before function ()

Comment 12

[Daniel Gherasim]

Attachment: stopReload_v4.patch

Updated the patch as requested. I also added the teardownModule where we close all tabs and navigate to homepage.

Comment 13

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8369382 [details] [diff] [review]
stopReload_v4.patch

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

Just small updates, otherwise looks good.

::: metrofirefox/tests/functional/testToolbar/testStopReloadButtons.js
@@ +18,5 @@
> +  aModule.tabs = new tabs.TabBrowser(aModule.controller);
> +  aModule.toolbar = new toolbars.ToolBar(aModule.controller);
> +  aModule.locationBar = toolbar.locationBar;
> +
> +  tabs.closeAllTabs(aModule.controller);

No need for this here.

@@ +24,5 @@
> +
> +function teardownModule(aModule) {
> +  tabs.closeAllTabs(controller);
> +
> +  controller.open(utils.getDefaultHomepage());

Neither this one. closeAllTabs takes care of it. You can also remove utils from the imported modules.

@@ +61,5 @@
> +  assert.ok(!footer.exists(), "'organization' element has not been found");
> +
> +  // Reload, wait for it to completely load and test again
> +  var reloadButton = locationBar.getElement({type: "reloadButton"});
> +  assert.waitFor( function () {

Please remove the space before function

Comment 14

[Daniel Gherasim]

Attachment: stopReload_v4.1.patch

Updated patch as requested.

Comment 15

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8369948 [details] [diff] [review]
stopReload_v4.1.patch

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

Looks good to me. Adding Henrik for a final check.

This will still need to wait for the landing of bug 880417 first.

Comment 16

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

Comment on attachment 8369948 [details] [diff] [review]
stopReload_v4.1.patch

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

::: metrofirefox/tests/functional/testToolbar/testStopReloadButtons.js
@@ +15,5 @@
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.tabs = new tabs.TabBrowser(aModule.controller);
> +  aModule.toolbar = new toolbars.ToolBar(aModule.controller);
> +  aModule.locationBar = toolbar.locationBar;

I would drop this line and always use toolBar.locationBar.

@@ +37,5 @@
> +  var contentWindow = controller.tabs.activeTab.defaultView;
> +  contentWindow.addEventListener("unload", onUnload);
> +
> +  // Go to the URL and start loading for some milliseconds
> +  controller.open(TEST_DATA);

This has to go into try. If it fails we would leak the event listener.

@@ +52,5 @@
> +  stopButton.tap();
> +
> +  // An element on page shouldn't exist when we hit the stop button really fast
> +  var footer = new elementslib.ID(controller.tabs.activeTab, "organization");
> +  assert.ok(!footer.exists(), "'organization' element has not been found");

I would say: 'footer element does not exist'. The way as written here could imagine our ID() method is broken.

@@ +58,5 @@
> +  // Reload, wait for it to completely load and test again
> +  var reloadButton = locationBar.getElement({type: "reloadButton"});
> +  assert.waitFor(function () {
> +    return utils.isDisplayed(controller, reloadButton);
> +  }, "Reload button is displayed");

Why do we have to wait for the button to be displayed? I don't see any action which would trigger a visibility change. If you want to wait for the page being loaded, you should call waitForPageLoaded() instead. But should this be necessary?

@@ +61,5 @@
> +    return utils.isDisplayed(controller, reloadButton);
> +  }, "Reload button is displayed");
> +  reloadButton.tap();
> +
> +  footer = new elementslib.ID(controller.tabs.activeTab, "organization");

I don't think it is necessary to retrieve it again.

@@ +64,5 @@
> +
> +  footer = new elementslib.ID(controller.tabs.activeTab, "organization");
> +  assert.waitFor(function () {
> +    return footer.exists();
> +  }, "'organization' element has been found");

So what you want to do here is really to have a waitForPageLoad() call before, and drop the waitFor in favor of assert.ok().

Comment 17

[Daniel Gherasim]

Attachment: stopReload_v4.2.patch

Thanks for the review Henrik, you were right about all. 
Updated the patch as requested and asked Andrei or Andreea for a current check before asking you back.

Comment 18

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8371293 [details] [diff] [review]
stopReload_v4.2.patch

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

Looks good.

Comment 19

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

Comment on attachment 8371293 [details] [diff] [review]
stopReload_v4.2.patch

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

Beside the taps do we have shortcuts we could exercise here? That would be important for localized builds.

::: metrofirefox/tests/functional/testToolbar/testStopReloadButtons.js
@@ +45,5 @@
> +  finally {
> +    contentWindow.removeEventListener("unload", onUnload);
> +  }
> +
> +  var stopButton = toolBar.locationBar.getElement({type: "stopButton"});

I think we should move this getElement call up before the actual page load to safe some milliseconds for fetching the element. As of now it could cause flaky tests.

Comment 20

[Daniel Gherasim]

Attachment: stopReload_v5.patch

I updated the patch and done some changes:
1. Created 2 methods (reload & close) in LocationBar class
2. Test will now open/close tabs through both button & shortcut methods.

Comment 21

[Daniel Gherasim]

Attachment: stopReload_v5.1.patch

Given that we are not checking just for the buttons now, renamed test from testCloseReloadButtons to testCloseReload.

Comment 22

[Andrei Eftimie]

Comment on attachment 8373980 [details] [diff] [review]
stopReload_v5.1.patch

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

Something doesn't work as expected.
Instead of doing the test as it was, its now broken into 2 parts and we're running the first part 2 times followed by the second part 2 times.

Lets regroup a bit.
Since both open and close methods share the same method types, lets have a single forEach() and do the whole test inside.

::: metrofirefox/lib/ui/toolbars.js
@@ +446,5 @@
> +   *
> +   * @param {String} [aEventType="shortcut"]
> +   *        Type of event which triggers the action
> +   */
> +  reload: function locationBar_reload(aEventType) {

Please sort the methods alphabetically. Both of them should come before the `type` method.

::: metrofirefox/tests/functional/testToolbar/testStopReload.js
@@ +38,5 @@
> +    pageUnloaded = true;
> +  }
> +
> +  // Get any element from the web page
> +  var footer = new elementslib.ID(controller.tabs.activeTab, "organization");

We should name it something other than footer. contentElement ?

Comment 23

[Daniel Gherasim]

Attachment: 6.patch

Updated the patch as requested.

Comment 24

[Daniel Gherasim]

Attachment: openCloseTab_v6.patch

Sorry for the wrong patch!
Updated now.

Comment 25

[Daniel Gherasim]

Attachment: stopReload_v6.patch

Sorry again, seems like not a very good day for me with selecting the right patches.

Comment 26

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8374791 [details] [diff] [review]
stopReload_v6.patch

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

Looks good to me.

Comment 27

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

Comment on attachment 8374791 [details] [diff] [review]
stopReload_v6.patch

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

::: metrofirefox/lib/ui/toolbars.js
@@ +438,5 @@
> +   */
> +  reload: function locationBar_reload(aEventType) {
> +    var type = aEventType || "shortcut";
> +
> +    switch(type) {

nit: missing blank before (.

@@ +447,5 @@
> +      case "shortcut":
> +        var win = new findElement.MozMillElement("Elem", this._controller.window);
> +        var cmdKey = utils.getEntity(this.toolbar.dtds, "reload.key");
> +        win.keypress(cmdKey, {accelKey:true});
> +        break;

What about:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser.xul#198

Also we would need a parameter to specify if it is a forced reload, which adds shift to the key.

@@ +462,5 @@
> +   */
> +  stop: function locationBar_stop(aEventType) {
> +    var type = aEventType || "shortcut";
> +
> +    switch(type) {

nit: missing blank before (.

@@ +469,5 @@
> +        elem.tap();
> +        break;
> +      case "shortcut":
> +        var win = new findElement.MozMillElement("Elem", this._controller.window);
> +        win.keypress("VK_ESCAPE", {});

I'm not happy with sending the key to the window. This will fail if a popup or something else opens. We need a better element here, if there is no specific key existent.

@@ +485,5 @@
>     */
>    type : function locationBar_type(aText) {
>      var location = this.getElement({type: "urlbar"});
>      location.sendKeys(aText);
> +  },

please revert this change.

::: metrofirefox/tests/functional/testToolbar/testStopReload.js
@@ +61,5 @@
> +    toolBar.locationBar.reload(aMethod);
> +    controller.waitForPageLoad();
> +    assert.ok(contentElement.exists(), "'contentElement' exists");
> +
> +    pageUnloaded = false;

I would split this into two separate tests. Otherwise you will not run the shortcut tests if the button test fails.

Comment 28

[Daniel Gherasim]

Attachment: stopReload_v6.1.patch

Good review, thanks!

So i updated patch with the following requested changes:
* added the F5 shortcut in the test;
* added the force reload shortcuts;
* split test into two separated tests
** testStopReloadButtons.js
** testStopReloadShortcuts.js

Comment 29

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

Comment on attachment 8378846 [details] [diff] [review]
stopReload_v6.1.patch

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

After a patch update please ask Andreea and Andrei for review first.

Comment 30

[Andrei Eftimie]

Comment on attachment 8378846 [details] [diff] [review]
stopReload_v6.1.patch

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

Looks good.
Please fix the mentioned items, then request review from Henrik.

::: metrofirefox/lib/ui/toolbars.js
@@ +439,5 @@
> +   *        Value if the reload will be forced
> +   */
> +  reload: function locationBar_reload(aEventType, aForce) {
> +    var type = aEventType || "shortcut";
> +    var forceReload = aForce || false;

Lets improve this a bit by casting is as a boolean.
And add a empty newline after.

::: metrofirefox/tests/functional/testToolbar/testStopReloadButtons.js
@@ +36,5 @@
> +  var contentElement = new elementslib.ID(controller.tabs.activeTab, "organization");
> +
> +  // Get current window
> +  var contentWindow = controller.tabs.activeTab.defaultView;
> +

nit: remove this newline

Comment 31

[Daniel Gherasim]

Attachment: stopReload_v6.2.patch

Thanks,
Updated the patch and asked Henrik for review.

Comment 32

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

Comment on attachment 8383587 [details] [diff] [review]
stopReload_v6.2.patch

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

::: metrofirefox/lib/ui/toolbars.js
@@ +437,5 @@
> +   *        Type of event which triggers the action
> +   * @param {Boolean} aForce
> +   *        Value if the reload will be forced
> +   */
> +  reload: function locationBar_reload(aEventType, aForce) {

aForce is also optional. Please mark it as such. As given here you cannot have a required parameter after an optional one.

Sad thing of the current implementation is that you will always have to specify the event type if you want to let it force reload the page. In those cases dicts are more helpful. But lets move this out to the bug you filed earlier.

@@ +444,5 @@
> +
> +    switch (type) {
> +      case "button":
> +        var elem = this.getElement({type: "reloadButton"});
> +        elem.tap();

This misses the force reloading of the page. I'm sure we can synthesize that correctly.

@@ +454,5 @@
> +      case "shortcut2":
> +        var toolbar = this.toolbar.getElement({type: "toolbar"});
> +        var cmdKey = utils.getEntity(this.toolbar.dtds, "reload.key");
> +        toolbar.keypress(cmdKey, {accelKey:true, shiftKey: forceReload});
> +        break;

DTD entities are more important for us. So I would flip this case with shortcut.

@@ +456,5 @@
> +        var cmdKey = utils.getEntity(this.toolbar.dtds, "reload.key");
> +        toolbar.keypress(cmdKey, {accelKey:true, shiftKey: forceReload});
> +        break;
> +      default:
> +        throw new Error("Unknown method - " + type);

You do not specify a 'method' here but an event type.

@@ +479,5 @@
> +        var toolbar = this.toolbar.getElement({type: "toolbar"});
> +        toolbar.keypress("VK_ESCAPE", {});
> +        break;
> +      default:
> +        throw new Error("Unknown method - " + type);

Same here.

::: metrofirefox/tests/functional/testToolbar/testStopReloadButtons.js
@@ +32,5 @@
> +    pageUnloaded = true;
> +  }
> +
> +  // Get any element from the web page
> +  var contentElement = new elementslib.ID(controller.tabs.activeTab, "organization");

How can you retrieve an element from a web page which hasn't been loaded yet?

@@ +34,5 @@
> +
> +  // Get any element from the web page
> +  var contentElement = new elementslib.ID(controller.tabs.activeTab, "organization");
> +
> +  // Get current window

Do we really need this comment?

@@ +42,5 @@
> +  try {
> +    controller.open(TEST_DATA);
> +    assert.waitFor(function () {
> +      return pageUnloaded;
> +    }, "page has been unloaded.", 2000, 10);

nit: 'Web page'

::: metrofirefox/tests/functional/testToolbar/testStopReloadShortcuts.js
@@ +34,5 @@
> +    pageUnloaded = true;
> +  }
> +
> +  // Get any element from the web page
> +  var contentElement = new elementslib.ID(controller.tabs.activeTab, "organization");

Same as for the other test file applies here.

@@ +64,5 @@
> +    // Force reload page
> +    toolBar.locationBar.reload(aMethod, true);
> +    controller.waitForPageLoad();
> +    assert.ok(contentElement.exists(), "'contentElement' exists");
> +  });

I'm not sure what you are testing here. What this test should do is to cancel the reload for both types of shortcuts. You only do that for the first one.

Comment 33

[Daniel Gherasim]

Attachment: stopReload_v6.3.patch

Updated patch now.

Comment 34

[Andrei Eftimie]

Comment on attachment 8386089 [details] [diff] [review]
stopReload_v6.3.patch

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

Bugzilla has marked this as a "Windows Patch".
Please check the line-endings. We should only have line feed (\n)

The patch doesn't apply cleanly.

::: metrofirefox/lib/ui/toolbars.js
@@ +437,5 @@
> +   *        Type of event which triggers the action
> +   * @param {Boolean} [aSpec.aForce]
> +   *        Value if the reload will be forced
> +   */
> +  reload: function locationBar_reload(aSpec) {

To avoid any possible ReferenceError if aSpec is not defined at all you should default it to an empty object.

@@ +439,5 @@
> +   *        Value if the reload will be forced
> +   */
> +  reload: function locationBar_reload(aSpec) {
> +    var type = aSpec.aEventType || "shortcut";
> +    var forceReload = !!aSpec.aForce || false;

Since you are casting this as a boolean, you can remove the alternate (default) value.
It will be `(true|false) || false`.

@@ +446,5 @@
> +      case "button":
> +        var elem = this.getElement({type: "reloadButton"});
> +        var command = (forceReload) ? 'cmd_forceReload' : 'cmd_reload';
> +        elem.getNode().setAttribute("oncommand",
> +                                    "CommandUpdater.doCommand('" + command + "');");

This is not what we want here.
Changing the commands from specific buttons does not help us.

We want to test the UI. In this case we need to hold shift pressed while clicking on the reload button.

@@ +452,5 @@
> +        break;
> +      case "shortcut":
> +        var toolbar = this.toolbar.getElement({type: "toolbar"});
> +        var cmdKey = utils.getEntity(this.toolbar.dtds, "reload.key");
> +        toolbar.keypress(cmdKey, {accelKey:true, shiftKey: forceReload});

nit: space after the colon

@@ +471,5 @@
> +   *        Information for the stop event
> +   * @param {String} [aSpec.aEventType="shortcut"]
> +   *        Type of event which triggers the action
> +   */
> +  stop: function locationBar_stop(aSpec) {

Same as above. aSpec will need a default empty object if it's undefined.

Comment 35

[Daniel Gherasim]

(In reply to Andrei Eftimie from comment #34)
> @@ +446,5 @@
> > +      case "button":
> > +        var elem = this.getElement({type: "reloadButton"});
> > +        var command = (forceReload) ? 'cmd_forceReload' : 'cmd_reload';
> > +        elem.getNode().setAttribute("oncommand",
> > +                                    "CommandUpdater.doCommand('" + command + "');");
> 
> This is not what we want here.
> Changing the commands from specific buttons does not help us.
> 
> We want to test the UI. In this case we need to hold shift pressed while
> clicking on the reload button.
> 

Looking more into this I found out that we can hold the shiftKey pressed while tapping the element, but the sad thing here is that we don't have this implemented in MozmillElement.tap(). 
! We can do it with MozmillElement.mouseEvent(). 

Henrik, do you think would pe ok to use the mouseEvent() to have the shiftKey pressed while tapping on that element, in case we want a force reload ?

Comment 36

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

Sounds fine to me, whereby we should check mozmill if it is worth to extend all the new mouse event methods to also accept the key modifiers. It should certainly filed as a bug, which you may reference in your patch here.

Comment 37

[Daniel Gherasim]

Attachment: stopReload_v6.4.patch

Thanks for review & tips,
Updated the patch now with the latest changes.

Comment 38

[Andrei Eftimie]

Comment on attachment 8387413 [details] [diff] [review]
stopReload_v6.4.patch

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

Looks good to me.
Tested and works fine.

With the nits fixed please request review from Henrik.

::: metrofirefox/lib/ui/toolbars.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +var broker = {};      Cu.import('resource://mozmill/driver/msgbroker.js', broker);

nit: you can collapse the whitespace

@@ +456,5 @@
> +        elem.mouseEvent(undefined, undefined, {
> +          clickCount: 1,
> +          inputSource: Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH,
> +          shiftKey: forceReload
> +        }, undefined);

nit: you can omit the last argument.

Comment 39

[Daniel Gherasim]

Attachment: stopReload_v6.5.patch

Thanks, fixed the nits and asked Henrik for a final check.

Comment 40

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

Comment on attachment 8387584 [details] [diff] [review]
stopReload_v6.5.patch

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

With the nits fixed you get my r+.

::: metrofirefox/lib/ui/toolbars.js
@@ +438,5 @@
> +   * @param {Object} aSpec
> +   *        Information for the reload event
> +   * @param {String} [aSpec.aEventType="shortcut"]
> +   *        Type of event which triggers the action
> +   * @param {Boolean} [aSpec.aForce]

This is optional? Then specify the default value.

@@ +457,5 @@
> +          clickCount: 1,
> +          inputSource: Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH,
> +          shiftKey: forceReload
> +        });
> +        broker.pass({'function':'MozMillElement.tap()'});

Don't use the broker. This is Mozmill internal code and has to be avoided. If such a state setter is missing please get this added to mouseEvent().

::: metrofirefox/tests/functional/testToolbar/testStopReloadButtons.js
@@ +27,5 @@
> + */
> +function testStopReloadButtons() {
> +  controller.open(TEST_DATA);
> +  controller.waitForPageLoad();
> +  // Variable to store the unload event state

missing blank line.

::: metrofirefox/tests/functional/testToolbar/testStopReloadShortcuts.js
@@ +29,5 @@
> + */
> +function testStopReloadShortcuts() {
> +  controller.open(TEST_DATA);
> +  controller.waitForPageLoad();
> +  // Variable to store the unload event state

missing blank line

Comment 41

[Daniel Gherasim]

Attachment: stopReload_v6.6.patch

Updated patch as requested.

Comment 42

[Andrei Eftimie]

Comment on attachment 8389114 [details] [diff] [review]
stopReload_v6.6.patch

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

Looks good, but it needs a small update as it doesn't apply anymore cleanly.
(in metrofirefox/tests/functional/manifest.ini)

Comment 43

[Daniel Gherasim]

Attachment: stopReload_v6.7.patch

Updated the patch to apply cleanly.

Comment 44

[Andrei Eftimie]

Comment on attachment 8390510 [details] [diff] [review]
stopReload_v6.7.patch

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

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