1036825 - Refactor tabs opening methods from tabs.js

Status: RESOLVED FIXED

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

Description

[Daniel Gherasim]

We have 3 methods in tabs.js (openTab, openInNewTab, reopen) with a lot of duplicated code. We can create a private method and call that, & also openTab & openInNewTab can be combined.

This will also offer the possibility to give a callback to open a tab (needed from situations like opening the in-content preferences).

This is blocking a P1 bug.

Comment 1

[Daniel Gherasim]

Attachment: tabs-refactoring-v1.patch

Comment 2

[Daniel Gherasim]

Attachment: tabs-refactoring-v1.1.patch

Forgot to add the callback case.

Comment 3

[Daniel Gherasim]

This is blocking 2 P1 bugs!

Comment 4

[Andrei Eftimie]

Comment on attachment 8453733 [details] [diff] [review]
tabs-refactoring-v1.1.patch

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

Really nice work.
I have just a few nits and I would add another switch for the target case.

Otherwise this looks good to me. Please ask a review from Henrik once updated.
Thanks

::: firefox/lib/tabs.js
@@ +745,5 @@
> +   * @param {object} [aSpec]
> +   *        Information about how to open the tab
> +   * @param {string} [aSpec.type="menu"]
> +   *        Method used for opening the tab
> +   * @param {string} [aSpec.target]

this is a MozElement

@@ +747,5 @@
> +   * @param {string} [aSpec.type="menu"]
> +   *        Method used for opening the tab
> +   * @param {string} [aSpec.target]
> +   *        Element from where to open the tab
> +   * @param {string} [aSpec.callback]

this is a function

@@ +785,5 @@
> +            // TODO: Calculate the correct x position
> +            this._controller.doubleClick(tabStrip, tabStrip.getNode().clientWidth - 100, 3);
> +          }
> +          break;
> +        case "target_contextMenu":

I would split these into 2 properties.
We would have: {type: 'target', subtype: 'contextmenu'}
And on the 'target' case we make another switch based on 'subtype' where we have 2 cases.

Comment 5

[Daniel Gherasim]

Attachment: tabs-refactoring-v1.2.patch

Comment 6

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

Comment on attachment 8456857 [details] [diff] [review]
tabs-refactoring-v1.2.patch

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

Great clean-up of the code! That was indeed necessary. I kinda like it.

Please check my inline comments and requests.

::: firefox/lib/tabs.js
@@ +744,5 @@
> +   *
> +   * @param {object} [aSpec]
> +   *        Information about how to open the tab
> +   * @param {function} [aSpec.callback]
> +   *        Callback uesd to open the tab

spelling mistake, also make the wording similar to the type property

@@ +745,5 @@
> +   * @param {object} [aSpec]
> +   *        Information about how to open the tab
> +   * @param {function} [aSpec.callback]
> +   *        Callback uesd to open the tab
> +   * @param {string} [aSpec.type="menu"]

I would call this 'method'. 'type' is not appropriate here.

@@ +759,5 @@
> +
> +    this._waitForNewTab(() => {
> +      switch (type) {
> +        case "callback":
> +          assert.ok(spec.callback, "Callback function is defined");

You want to check for the function type.

@@ +795,5 @@
> +            case "contextMenu":
> +              var contextMenuItem = findElement.ID(this._controller.window.document,
> +                                                   "context-openlinkintab");
> +              spec.target.rightClick();
> +              contextMenuItem.click();

I don't see why this has to be a subtype. I would assume this as method "contextMenu", which operates on the passed-in target.

@@ +799,5 @@
> +              contextMenuItem.click();
> +              utils.closeContentAreaContextMenu(this._controller);
> +              break;
> +            case "middleClick":
> +              spec.target.middleClick();

Same here.

@@ +817,5 @@
> +   *
> +   * @param  {string} [aEventType="shortcut"]
> +   *         Type of event which triggers the action
> +   */
> +  reopen: function tabBrowser_reopen(aEventType) {

I would enforce the identical calling convention across all methods. So I would use aSpec.method.

@@ +853,5 @@
> +   *
> +   * @param {function} aCallback
> +   *        Function that triggeres the callback
> +   */
> +  _waitForNewTab: function tabBrowser_waitForTab(aCallback) {

_waitForTabOpened please. Also adjust the internal name. newTab is not appropriate for reopening a closed tab.

@@ +879,2 @@
>          return self.opened && self.transitioned;
> +      }, "New tab has been opened");

Kill 'new' here.

::: firefox/tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js
@@ +40,5 @@
>    var link1 = new elementslib.Name(controller.tabs.activeTab, "link_1");
>    var link2 = new elementslib.Name(controller.tabs.activeTab, "link_2");
>  
>    assert.waitFor(function () {
> +    tabBrowser.openTab({type: "target", subtype: "middleClick", target: link1});

{method: middleClick, target: link1}

@@ +64,5 @@
>    });
>    obs.observe(scrollButtonDown.getNode(), config);
>  
>    // Open one more tab but with another link for later verification
> +  tabBrowser.openTab({type: "target", subtype: "middleClick", target: link2});

Similar as above.

::: firefox/tests/functional/testTabbedBrowsing/testOpenInBackground.js
@@ +48,5 @@
>      else {
>        // Open the first link via context menu in a new tab:
> +      tabBrowser.openTab({type: "target",
> +                          subtype: "contextMenu",
> +                          target: currentLink});

Same here for both.

Comment 7

[Daniel Gherasim]

Attachment: tabs-refactoring-v1.3.patch

Comment 8

[Daniel Gherasim]

Attachment: tabs-refactoring-v1.4.patch

Updated commit message.

Comment 9

[Andrei Eftimie]

Comment on attachment 8460229 [details] [diff] [review]
tabs-refactoring-v1.4.patch

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

Looks good to me, only one nit to be fixed.

::: firefox/lib/tabs.js
@@ +811,5 @@
> +   * Method for reopening the last closed tab
> +   *
> +   * @param {object} [aSpec]
> +   *        Information about how to open the tab
> +   * @param {string} [aSpec.method="menu"]

The default is actually "shortcut".

Comment 10

[Daniel Gherasim]

Attachment: tabs-refactoring-v1.5.patch

Comment 11

[Daniel Gherasim]

Testruns on linux & windows:
http://mozmill-crowd.blargon7.com/#/functional/report/096bceac12dd83f9ad7f3a1bb4290950
http://mozmill-crowd.blargon7.com/#/functional/report/096bceac12dd83f9ad7f3a1bb4293c96

Comment 12

[Andrei Eftimie]

Comment on attachment 8460867 [details] [diff] [review]
tabs-refactoring-v1.5.patch

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

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/4185b60b454e (default)

Comment 13

[Andrei Eftimie]

We want to backport this.

Comment 14

[Daniel Gherasim]

Patch applies cleanly on all except esr24.
I'll create a new one for that.

aurora: 

http://mozmill-crowd.blargon7.com/#/functional/report/096bceac12dd83f9ad7f3a1bb4460557
http://mozmill-crowd.blargon7.com/#/functional/report/096bceac12dd83f9ad7f3a1bb443d170
http://mozmill-crowd.blargon7.com/#/functional/report/096bceac12dd83f9ad7f3a1bb4444e51


beta:
http://mozmill-crowd.blargon7.com/#/functional/report/096bceac12dd83f9ad7f3a1bb4494dca
http://mozmill-crowd.blargon7.com/#/functional/report/096bceac12dd83f9ad7f3a1bb44926e2
http://mozmill-crowd.blargon7.com/#/functional/report/096bceac12dd83f9ad7f3a1bb4488df4


release:
http://mozmill-crowd.blargon7.com/#/functional/report/096bceac12dd83f9ad7f3a1bb44f86c5
http://mozmill-crowd.blargon7.com/#/functional/report/096bceac12dd83f9ad7f3a1bb4504584
http://mozmill-crowd.blargon7.com/#/functional/report/096bceac12dd83f9ad7f3a1bb4502d0e

Comment 15

[Daniel Gherasim]

Attachment: tabs-refactoring-esr24.patch

Patch for esr24.
http://mozmill-crowd.blargon7.com/#/functional/report/096bceac12dd83f9ad7f3a1bb4519d43

Comment 16

[Andrei Eftimie]

Transplanted on Aurora:
https://hg.mozilla.org/qa/mozmill-tests/rev/1ede4c435758 (mozilla-aurora)

Comment 17

[Andrei Eftimie]

Also ran some tests, everything works fine.

Transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/2a13462edf1a (mozilla-beta)
https://hg.mozilla.org/qa/mozmill-tests/rev/bb1bc5e0c37c (mozilla-release)
https://hg.mozilla.org/qa/mozmill-tests/rev/fd0f9bb0dd23 (mozilla-esr31)

Comment 18

[Andrei Eftimie]

Comment on attachment 8461513 [details] [diff] [review]
tabs-refactoring-esr24.patch

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

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/04d121853d57 (mozilla-esr24)

Comment 19

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

Just to mention here. I never gave my final review for this patch to be landed! Please obey our review guidelines! While I was away you should have asked Dave.

Comment 20

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

Comment on attachment 8461513 [details] [diff] [review]
tabs-refactoring-esr24.patch

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

Please get the two mentioned issues fixed with a follow-up patch.

::: firefox/lib/tabs.js
@@ +459,5 @@
> +        case "contextMenu":
> +          assert.ok(spec.target, "Target element has to be specified");
> +
> +          var contextMenuItem = findElement.ID(this._controller.window.document,
> +                                               "context-openlinkintab");

findElement should not be part of this method. It has to be located in getElements.

@@ +511,5 @@
> +      }
> +    });
> +
> +    assert.notEqual(tabCount, sessionStore.getClosedTabCount(this._controller),
> +                    "'Recently Closed Tabs' sub menu entries have changed");

This assert can fail because the difference can be >1, or a tab has been closed. Both we wont catch here.

Comment 21

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #19)
> Just to mention here. I never gave my final review for this patch to be
> landed! Please obey our review guidelines! While I was away you should have
> asked Dave.

*sigh* 

You gave a review in comment 6 where you mentioned 6 nits (spelling, rewording or function name changes) and one change to the argument properties (which reflects in the library itself and in 3 consumers).

Which were all fixed.

If you wanted to change anything else, why didn't you mention in in the review?
Why aren't you considering comment 6 a "final review" since nothing major needed changing.

Comment 22

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

I don't see that I mentioned 'nits' anywhere in comment 6. I totally wanted to review this patch again due to the subtype changes. Also as I have said the last time this happened, I always give a statement if it is ok without another review. Please be more careful.

Comment 23

[Daniel Gherasim]

Attachment: follow-up-v1.patch

Comment 24

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8468408 [details] [diff] [review]
follow-up-v1.patch

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

Looks good to me.

Comment 25

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

Comment on attachment 8468408 [details] [diff] [review]
follow-up-v1.patch

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

With the nit fixed it can be landed.

::: firefox/lib/tabs.js
@@ +606,4 @@
>      var document = this._controller.window.document;
>      var elem = null;
>  
>      switch(aSpec.type) {

nit: please fix that missing blank.

Comment 26

[Daniel Gherasim]

Attachment: follow-up-v1.1.patch

Comment 27

[Andrei Eftimie]

Comment on attachment 8469260 [details] [diff] [review]
follow-up-v1.1.patch

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

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/d594c00911db (default)

Comment 28

[Andrei Eftimie]

Transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/f697c56e40ea (mozilla-aurora)

Doesn't apply cleanly on beta and lower, Daniel please backport this patch.

Comment 29

[Daniel Gherasim]

Attachment: follow-up-beta-release-esr.patch

Comment 30

[Andrei Eftimie]

Looks good, tests are green:
https://hg.mozilla.org/qa/mozmill-tests/rev/7ae8bc2e9ba2 (mozilla-beta)
https://hg.mozilla.org/qa/mozmill-tests/rev/dee10fe1f1ab (mozilla-release)
https://hg.mozilla.org/qa/mozmill-tests/rev/abe9b42c1328 (mozilla-esr31)
https://hg.mozilla.org/qa/mozmill-tests/rev/e98387ba9bd2 (mozilla-esr24)

Comment 31

[Andrei Eftimie]

Comment on attachment 8471517 [details] [diff] [review]
follow-up-beta-release-esr.patch

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

Forgot to change the flag.