1118768 - PopupNotifications.transitionsEnabled doesn't do anything but is still used by tests

Status: RESOLVED FIXED

(Toolkit :: Notifications and Alerts, defect)

Description

[Florian Quèze [:florian]]

Bug 1079309 removed the transitionsEnabled setter from PopupNotifications but there are still 21 lines using it in tests:

http://mxr.mozilla.org/mozilla-central/search?string=PopupNotifications.transitionsEnabled&tree=mozilla-central

Comment 1

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #0)
> Bug 1079309

I meant bug 1079303 of course.

Comment 2

[:Gijs (he/him)]

Judging by the lack of intermittent-failure armageddon, we can probably just nuke all the uses.

Comment 3

[Prathik Sai [:Skandan]]

Attachment: Bug-1118768.diff (Removed redundant code)

Please let me know if I missed something or did something wrong.

Comment 4

[Prathik Sai [:Skandan]]

Attachment: Bug-1118768.diff (Removed redundant code)

Sorry first patch did not have my user name tag in it.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8546448 [details] [diff] [review]
Bug-1118768.diff (Removed redundant code)

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

Almost there! I noted some more comments / empty lines to remove; after that I think it's ready for a review request. :-)

::: browser/base/content/test/popupNotifications/head.js
@@ -87,5 @@
>  
>  function setup() {
>    // Disable transitions as they slow the test down and we want to click the
>    // mouse buttons in a predictable location.
> -  PopupNotifications.transitionsEnabled = false;

Nit: please remove the comment above this as well.

::: browser/base/content/test/social/head.js
@@ -78,5 @@
>    finish();
>  }
>  
>  function runSocialTestWithProvider(manifest, callback, finishcallback) {
> -  PopupNotifications.transitionsEnabled = false;

Nit: Please remove the empty line after this line as well.

@@ -171,5 @@
>    let providersAtStart = Social.providers.length;
>    info("runSocialTests: start test run with " + providersAtStart + " providers");
>    window.focus();
>  
> -  PopupNotifications.transitionsEnabled = false;

Nit: Please remove the empty line after this line as well.

::: browser/modules/test/browser_SignInToWebsite.js
@@ -276,5 @@
>      finish();
>      return;
>    }
>  
> -  PopupNotifications.transitionsEnabled = false;

Nit: please remove the empty line after this line as well.

@@ -315,5 @@
>  function cleanUp() {
>    info("cleanup");
>    resetState();
>  
> -  PopupNotifications.transitionsEnabled = true;

Nit: please remove the empty line after this line as well.

Comment 6

[Prathik Sai [:Skandan]]

Attachment: Bug-1118768.diff (nits cleaned up)

Sorry I didn't clean up the code properly everywhere! :) Here is a patch with the changes.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8546555 [details] [diff] [review]
Bug-1118768.diff (nits cleaned up)

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

Awesome, thanks for the quick turnaround! I'll land this shortly.

Comment 8

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/569130fd40b5

Comment 9

[:Gijs (he/him)]

Nice work on this patch - maybe you want to take a look at, say, bug 1119682 next? :-)

Comment 10

[Prathik Sai [:Skandan]]

Anything else I must do here? Like add the reviewer tag in the commit message?

Comment 11

[:Gijs (he/him)]

(In reply to Prathik Sai  [:Skandan] from comment #10)
> Anything else I must do here? Like add the reviewer tag in the commit
> message?

Nah, I did that for you when I landed it (see comment #8). :-)

Comment 12

[Prathik Sai [:Skandan]]

Thank You! :-) Having an amazing time as a newbie here.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/569130fd40b5