1420179 - test framework: shared-head refreshTab takes an unused tab argument

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

See https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/devtools/client/framework/test/shared-head.js#179-185 

The argument is not used.

- remove the argument
- udpate JSDoc
- update all callers
- rename method to refreshSelectedTab

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1420179 - support tab argument in devtools shared-head refreshTab;

The tab argument was unused in the method. All the call sites have been updated to
stop providing this argument. The method now supports an optional tab argument.
It defaults to the selected tab if no argument is provided.

Review commit: https://reviewboard.mozilla.org/r/202530/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202530/

Comment 2

[Julian Descottes [:jdescottes]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84766303856478970c4dc71df042205f76cbca4d

Ended up keeping the name and adding support for the argument as an optional thing. It's not used for now, but refreshSelectedTab felt like too many letters and not enough flexible, could bite us in the future.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931399 [details]
Bug 1420179 - support tab argument in devtools shared-head refreshTab;

https://reviewboard.mozilla.org/r/202530/#review207884

Seems legit

::: devtools/client/framework/test/shared-head.js:179
(Diff revision 1)
> -var refreshTab = Task.async(function*(tab) {
> +var refreshTab = Task.async(function* (tab) {
>    info("Refreshing tab.");
>    const finished = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> -  gBrowser.reloadTab(gBrowser.selectedTab);
> +  gBrowser.reloadTab(tab || gBrowser.selectedTab);
>    yield finished;
>    info("Tab finished refreshing.");
>  });

since we're refactoring things, could this be transformed to a plain async await function ?

::: devtools/client/framework/test/shared-head.js:179
(Diff revision 1)
>  /**
> - * Refresh the given tab.
> - * @param {Object} tab The tab to be refreshed.
> + * Refresh the provided tab.
> + * @param {Object} tab The tab to be refreshed. Defaults to the currently selected tab.
>   * @return Promise<undefined> resolved when the tab is successfully refreshed.
>   */
> -var refreshTab = Task.async(function*(tab) {
> +var refreshTab = Task.async(function* (tab) {

could we use (tab = gBrowser.selectedTab) ?

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8931399 [details]
Bug 1420179 - support tab argument in devtools shared-head refreshTab;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202530/diff/1-2/

Comment 5

[Julian Descottes [:jdescottes]]

Thanks for the review, fixed the raised issues.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc14f9701349
support tab argument in devtools shared-head refreshTab;r=nchevobbe

Comment 7

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/dc14f9701349