1064280 - Add automated test for changing the URL in a pinned tab

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Mihaela Velimiroviciu (:mihaelav)]

Add automated test for bug 1039904:

Steps:
1. Start Firefox
2. Pin a tab
3. Change the URL in the pinned tab

Expected result: URL loads in the same (pinned) tab; no additional tabs are created

Comment 1

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: test v1

Dão, can you please review the test, since you also fixed the bug?

Thank you!

Comment 2

[Mihaela Velimiroviciu (:mihaelav)]

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #1)
> Created attachment 8490623 [details] [diff] [review]
> test v1
> 
> Dão, can you please review the test, since you also fixed the bug?
> 
> Thank you!

Try is green: https://tbpl.mozilla.org/?tree=Try&rev=07da0d8b2b85

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8490623 [details] [diff] [review]
test v1

>+add_task(function(){
>+  info("Test that changing the URL in a pinned tab works correctly");

Please make this a comment instead:

// Test that changing the URL in a pinned tab works correctly

>+add_task(function asyncCleanup() {
>+  gBrowser.removeTab(gBrowser.selectedTab);

Can you use registerCleanupFunction?

>+  info("App tab was closed");

Please remove this.

>+function waitForPageLoad() {
>+  let deferred = Promise.defer();
>+
>+  function onTabLoad(event) {
>+    gBrowser.removeEventListener("load", onTabLoad, true);
>+    info("Load tab event received");
>+    deferred.resolve();
>+  }
>+
>+  gBrowser.addEventListener("load", onTabLoad, true, true);
>+  return deferred.promise;
>+}

Can you just use promiseTabLoadEvent?

Comment 4

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v1.1

updated based on review

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8493036 [details] [diff] [review]
v1.1

>+  let promisePageload = promiseTabLoadEvent(appTab); //waitForPageLoad();

please remove that comment

>+  is(appTab.linkedBrowser.contentDocument.location.href, TEST_LINK_CHANGED,
>+     "New page loaded in the app tab");

please use appTab.linkedBrowser.currentURI.spec instead of appTab.linkedBrowser.contentDocument.location.href

>+registerCleanupFunction(function () {
>+    gBrowser.removeTab(gBrowser.selectedTab);
>+  });

indentation is off, should be like this:

registerCleanupFunction(function () {
  gBrowser.removeTab(gBrowser.selectedTab);
});

Comment 6

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v1.2

Thanks Dão for the review! Fixed the nits. Carrying over the r+

Comment 7

[Mihaela Velimiroviciu (:mihaelav)]

Try is green:
https://tbpl.mozilla.org/?tree=Try&rev=07da0d8b2b85

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/daddf970831d

Comment 9

[Carsten Book [:Tomcat]]

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