Write mozmill metro test for stop navigation and reload a page

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: AndreeaMatei, Assigned: Daniel Gherasim)

Tracking

unspecified
All
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(firefox30 fixed)

Details

(Whiteboard: [metro][mentor=andrei.eftimie])

Attachments

(1 attachment, 19 obsolete attachments)

8.34 KB, patch
Andrei Eftimie
: review+
Andrei Eftimie
: checkin+
Details | Diff | Splinter Review
(Reporter)

Updated

4 years ago
Priority: -- → P2

Updated

4 years ago
Blocks: 922197
(Reporter)

Updated

4 years ago
Whiteboard: [metro] → [metro][mentor=andrei.eftimie]

Updated

4 years ago
Assignee: nobody → mario.garbi

Updated

4 years ago
Status: NEW → ASSIGNED
(Reporter)

Updated

4 years ago
Duplicate of this bug: 880760

Comment 2

4 years ago
Created attachment 830800 [details] [diff] [review]
newMetro_stopReloadPage.patch

This is dependent on the restructure and metrolib patches and will only apply cleanly after importing those two first.
Attachment #830800 - Flags: feedback?(andrei.eftimie)
Attachment #830800 - Flags: feedback?(andreea.matei)
(Reporter)

Comment 3

4 years ago
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
Attachment #830800 - Flags: feedback?(andrei.eftimie)
Attachment #830800 - Flags: feedback?(andreea.matei)
Attachment #830800 - Flags: feedback-

Comment 4

4 years ago
Created attachment 8362533 [details] [diff] [review]
stopReload.patch

Updated the patch and tested, unfortunately we can't get dashboard reports yet.
Attachment #830800 - Attachment is obsolete: true
Attachment #8362533 - Flags: review?(andrei.eftimie)
Attachment #8362533 - Flags: review?(andreea.matei)

Comment 5

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

Comment 6

4 years ago
Created attachment 8362864 [details] [diff] [review]
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
Attachment #8362533 - Attachment is obsolete: true
Attachment #8362533 - Flags: review?(andrei.eftimie)
Attachment #8362533 - Flags: review?(andreea.matei)
Attachment #8362864 - Flags: review?(andrei.eftimie)
Attachment #8362864 - Flags: review?(andreea.matei)

Updated

4 years ago
Depends on: 880417

Comment 7

4 years ago
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.
Attachment #8362864 - Flags: review?(andrei.eftimie)
Attachment #8362864 - Flags: review?(andreea.matei)
Attachment #8362864 - Flags: review-

Comment 8

4 years ago
Created attachment 8367926 [details] [diff] [review]
stopReload_v2.patch

Updated patch as requested
Attachment #8362864 - Attachment is obsolete: true
Attachment #8367926 - Flags: review?(andrei.eftimie)
Attachment #8367926 - Flags: review?(andreea.matei)

Comment 9

4 years ago
Created attachment 8367933 [details] [diff] [review]
stopReload_v3.patch

Moved the test to functional folder as we no longer use remote pages.
Attachment #8367926 - Attachment is obsolete: true
Attachment #8367926 - Flags: review?(andrei.eftimie)
Attachment #8367926 - Flags: review?(andreea.matei)

Comment 10

4 years ago
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.
Attachment #8367933 - Flags: review?(andrei.eftimie)
Attachment #8367933 - Flags: review?(andreea.matei)
(Reporter)

Comment 11

4 years ago
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 ()
Attachment #8367933 - Flags: review?(andrei.eftimie)
Attachment #8367933 - Flags: review?(andreea.matei)
Attachment #8367933 - Flags: review-

Updated

4 years ago
Assignee: mario.garbi → daniel.gherasim
(Assignee)

Comment 12

4 years ago
Created attachment 8369382 [details] [diff] [review]
stopReload_v4.patch

Updated the patch as requested. I also added the teardownModule where we close all tabs and navigate to homepage.
Attachment #8367933 - Attachment is obsolete: true
Attachment #8369382 - Flags: review?(andrei.eftimie)
Attachment #8369382 - Flags: review?(andreea.matei)
(Reporter)

Comment 13

4 years ago
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
Attachment #8369382 - Flags: review?(andrei.eftimie)
Attachment #8369382 - Flags: review?(andreea.matei)
Attachment #8369382 - Flags: review-
(Assignee)

Comment 14

4 years ago
Created attachment 8369948 [details] [diff] [review]
stopReload_v4.1.patch

Updated patch as requested.
Attachment #8369382 - Attachment is obsolete: true
Attachment #8369948 - Flags: review?(andrei.eftimie)
Attachment #8369948 - Flags: review?(andreea.matei)
(Reporter)

Comment 15

4 years ago
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.
Attachment #8369948 - Flags: review?(hskupin)
Attachment #8369948 - Flags: review?(andrei.eftimie)
Attachment #8369948 - Flags: review?(andreea.matei)
Attachment #8369948 - Flags: review+
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().
Attachment #8369948 - Flags: review?(hskupin) → review-
(Assignee)

Comment 17

4 years ago
Created attachment 8371293 [details] [diff] [review]
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.
Attachment #8369948 - Attachment is obsolete: true
Attachment #8371293 - Flags: review?(andrei.eftimie)
Attachment #8371293 - Flags: review?(andreea.matei)
(Reporter)

Comment 18

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

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

Looks good.
Attachment #8371293 - Flags: review?(hskupin)
Attachment #8371293 - Flags: review?(andrei.eftimie)
Attachment #8371293 - Flags: review?(andreea.matei)
Attachment #8371293 - Flags: review+
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.
Attachment #8371293 - Flags: review?(hskupin) → review-
(Assignee)

Comment 20

4 years ago
Created attachment 8373974 [details] [diff] [review]
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.
Attachment #8371293 - Attachment is obsolete: true
Attachment #8373974 - Flags: review?(andrei.eftimie)
Attachment #8373974 - Flags: review?(andreea.matei)
(Assignee)

Comment 21

4 years ago
Created attachment 8373980 [details] [diff] [review]
stopReload_v5.1.patch

Given that we are not checking just for the buttons now, renamed test from testCloseReloadButtons to testCloseReload.
Attachment #8373974 - Attachment is obsolete: true
Attachment #8373974 - Flags: review?(andrei.eftimie)
Attachment #8373974 - Flags: review?(andreea.matei)
(Assignee)

Updated

4 years ago
Attachment #8373980 - Flags: review?(andrei.eftimie)
Attachment #8373980 - Flags: review?(andreea.matei)

Comment 22

4 years ago
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 ?
Attachment #8373980 - Flags: review?(andrei.eftimie)
Attachment #8373980 - Flags: review?(andreea.matei)
Attachment #8373980 - Flags: review-
(Assignee)

Comment 23

4 years ago
Created attachment 8374683 [details] [diff] [review]
6.patch

Updated the patch as requested.
Attachment #8373980 - Attachment is obsolete: true
Attachment #8374683 - Flags: review?(andrei.eftimie)
Attachment #8374683 - Flags: review?(andreea.matei)
(Assignee)

Comment 24

4 years ago
Created attachment 8374789 [details] [diff] [review]
openCloseTab_v6.patch

Sorry for the wrong patch!
Updated now.
Attachment #8374683 - Attachment is obsolete: true
Attachment #8374683 - Flags: review?(andrei.eftimie)
Attachment #8374683 - Flags: review?(andreea.matei)
Attachment #8374789 - Flags: review?(andrei.eftimie)
Attachment #8374789 - Flags: review?(andreea.matei)
(Assignee)

Comment 25

4 years ago
Created attachment 8374791 [details] [diff] [review]
stopReload_v6.patch

Sorry again, seems like not a very good day for me with selecting the right patches.
Attachment #8374789 - Attachment is obsolete: true
Attachment #8374789 - Flags: review?(andrei.eftimie)
Attachment #8374789 - Flags: review?(andreea.matei)
Attachment #8374791 - Flags: review?(andrei.eftimie)
Attachment #8374791 - Flags: review?(andreea.matei)
(Reporter)

Comment 26

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

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

Looks good to me.
Attachment #8374791 - Flags: review?(hskupin)
Attachment #8374791 - Flags: review?(andrei.eftimie)
Attachment #8374791 - Flags: review?(andreea.matei)
Attachment #8374791 - Flags: review+
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.
Attachment #8374791 - Flags: review?(hskupin) → review-
(Assignee)

Comment 28

4 years ago
Created attachment 8378846 [details] [diff] [review]
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
Attachment #8374791 - Attachment is obsolete: true
Attachment #8378846 - Flags: review?(hskupin)
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.
Attachment #8378846 - Flags: review?(hskupin)
Attachment #8378846 - Flags: review?(andrei.eftimie)
Attachment #8378846 - Flags: review?(andreea.matei)

Comment 30

4 years ago
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
Attachment #8378846 - Flags: review?(andrei.eftimie)
Attachment #8378846 - Flags: review?(andreea.matei)
Attachment #8378846 - Flags: review-
(Assignee)

Comment 31

4 years ago
Created attachment 8383587 [details] [diff] [review]
stopReload_v6.2.patch

Thanks,
Updated the patch and asked Henrik for review.
Attachment #8378846 - Attachment is obsolete: true
Attachment #8383587 - Flags: review?(hskupin)
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.
Attachment #8383587 - Flags: review?(hskupin) → review-
(Assignee)

Comment 33

4 years ago
Created attachment 8386089 [details] [diff] [review]
stopReload_v6.3.patch

Updated patch now.
Attachment #8383587 - Attachment is obsolete: true
Attachment #8386089 - Flags: review?(andrei.eftimie)
Attachment #8386089 - Flags: review?(andreea.matei)

Comment 34

4 years ago
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.
Attachment #8386089 - Flags: review?(andrei.eftimie)
Attachment #8386089 - Flags: review?(andreea.matei)
Attachment #8386089 - Flags: review-
(Assignee)

Comment 35

4 years ago
(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 ?
Flags: needinfo?(hskupin)
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.
Flags: needinfo?(hskupin)
(Assignee)

Comment 37

4 years ago
Created attachment 8387413 [details] [diff] [review]
stopReload_v6.4.patch

Thanks for review & tips,
Updated the patch now with the latest changes.
Attachment #8386089 - Attachment is obsolete: true
Attachment #8387413 - Flags: review?(andrei.eftimie)
Attachment #8387413 - Flags: review?(andreea.matei)

Comment 38

4 years ago
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.
Attachment #8387413 - Flags: review?(andrei.eftimie)
Attachment #8387413 - Flags: review?(andreea.matei)
Attachment #8387413 - Flags: review+
(Assignee)

Comment 39

4 years ago
Created attachment 8387584 [details] [diff] [review]
stopReload_v6.5.patch

Thanks, fixed the nits and asked Henrik for a final check.
Attachment #8387413 - Attachment is obsolete: true
Attachment #8387584 - Flags: review?(hskupin)
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
Attachment #8387584 - Flags: review?(hskupin) → review+
(Assignee)

Comment 41

4 years ago
Created attachment 8389114 [details] [diff] [review]
stopReload_v6.6.patch

Updated patch as requested.
Attachment #8387584 - Attachment is obsolete: true
Attachment #8389114 - Flags: review?(andrei.eftimie)
Attachment #8389114 - Flags: review?(andreea.matei)

Comment 42

4 years ago
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)
Attachment #8389114 - Flags: review?(andrei.eftimie)
Attachment #8389114 - Flags: review?(andreea.matei)
Attachment #8389114 - Flags: review+
(Assignee)

Comment 43

4 years ago
Created attachment 8390510 [details] [diff] [review]
stopReload_v6.7.patch

Updated the patch to apply cleanly.
Attachment #8389114 - Attachment is obsolete: true
Attachment #8390510 - Flags: review?(andrei.eftimie)

Comment 44

4 years ago
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)
Attachment #8390510 - Flags: review?(andrei.eftimie)
Attachment #8390510 - Flags: review+
Attachment #8390510 - Flags: checkin+

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox30: --- → fixed
Resolution: --- → FIXED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.