1016343 - Add reload method to toolbars.js/locationBar class

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Daniel Gherasim]

Let's add a reload method to the LocationBar class in firefox/lib/toolbars.js.

This will make our tests more understandable when we only want to reload the current page.

Comment 1

[Daniel Gherasim]

Attachment: add-reload-method.patch

Comment 2

[Andrei Eftimie]

Comment on attachment 8429246 [details] [diff] [review]
add-reload-method.patch

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

Good to have this functionality.
Thanks for filing the bug and coming up with a patch.

This looks good to me. Just a couple of nits mentioned inline.
Please request a review from Henrik with those fixed.

::: firefox/lib/toolbars.js
@@ +604,5 @@
>      this.type(url);
>      this._controller.keypress(this.urlbar, "VK_RETURN", {});
>    },
>  
> +    /**

nit: indentation

@@ +607,5 @@
>  
> +    /**
> +   * Reload page
> +   *
> +   * @param {Object} aSpec

nit: native types should be lowercase

@@ +616,5 @@
> +   *        Value if the reload will be forced
> +   */
> +  reload : function locationBar_reload(aSpec) {
> +    var spec = aSpec || {};
> +    var type = spec.aEventType || "shortcut";

You don't have to prefix argument properties with "a".

Comment 3

[Daniel Gherasim]

Attachment: add-reload-method_v2.patch

Thanks Andrei! Henrik, can you have a look ?

Comment 4

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

Comment on attachment 8429873 [details] [diff] [review]
add-reload-method_v2.patch

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

While this looks mostly fine, I would like to see where we can already make use of this method. I'm sure we have some cases when we are reloading a page. Changing that will prove that the new method works. r- because of the remaining things to fix.

::: firefox/lib/toolbars.js
@@ +605,5 @@
>      this._controller.keypress(this.urlbar, "VK_RETURN", {});
>    },
>  
>    /**
> +   * Reload page

Better descriptions please like 'Reload the currently open web page'.

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

This doesn't take the force reload parameter into account.

Comment 5

[Daniel Gherasim]

Attachment: add-reload-method_v3.patch

Updated & changed the lib/tests/testToolbar.js file to test the functionality.
We can file a new _good first bug_ to replace in tests code with this method.

Comment 6

[Daniel Gherasim]

Attachment: add-reload-method_v3.1.patch

Some issues with the latest patch.

Comment 7

[Andrei Eftimie]

Comment on attachment 8430779 [details] [diff] [review]
add-reload-method_v3.1.patch

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

Looks good.
Just one thing, it would be great to use a local page.

With that I'd happily land this.
Thanks Daniel.

::: firefox/lib/tests/testToolbar.js
@@ +6,5 @@
>  var {expect} = require("../../../lib/assertions");
>  var toolbars = require("../toolbars");
>  var utils = require("../utils");
>  
> +const TEST_DATA = "http://www.mozilla.org/en-US/about/contact";

Can we use a local page here?

Comment 8

[Daniel Gherasim]

Attachment: add-reload-method_v3.2.patch

Done that!

Comment 9

[Andrei Eftimie]

Comment on attachment 8431365 [details] [diff] [review]
add-reload-method_v3.2.patch

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

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

Comment 10

[Andrei Eftimie]

Let's get this into mozilla-beta for bug 1008913

Comment 11

[Andrei Eftimie]

Transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/2a0e48a85541 (mozilla-beta)

--Ran without problems--
Functional: http://mozmill-crowd.blargon7.com/#/functional/report/883d8406dab55a771a00934158bc363c
Remote: http://mozmill-crowd.blargon7.com/#/remote/report/883d8406dab55a771a00934158bbcbe3