Closed Bug 1118768 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Notifications and Alerts, defect)

defect
Not set
minor
Points:
1

Tracking

()

RESOLVED FIXED
mozilla37
Iteration:
37.3 - 12 Jan

People

(Reporter: florian, Assigned: Skandan, Mentored)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 2 obsolete files)

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
(In reply to Florian Quèze [:florian] [:flo] from comment #0)
> Bug 1079309

I meant bug 1079303 of course.
Blocks: 1079303
No longer blocks: 1079309
Judging by the lack of intermittent-failure armageddon, we can probably just nuke all the uses.
Mentor: florian
Whiteboard: [good first bug]
Please let me know if I missed something or did something wrong.
Attachment #8546446 - Flags: feedback?(florian)
Sorry first patch did not have my user name tag in it.
Attachment #8546446 - Attachment is obsolete: true
Attachment #8546446 - Flags: feedback?(florian)
Attachment #8546448 - Flags: feedback?(florian)
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.
Attachment #8546448 - Flags: feedback?(florian) → feedback+
Sorry I didn't clean up the code properly everywhere! :) Here is a patch with the changes.
Attachment #8546448 - Attachment is obsolete: true
Attachment #8546555 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8546555 - Flags: review?(gijskruitbosch+bugs) → review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/569130fd40b5
Assignee: nobody → prathikcoding167
Status: NEW → ASSIGNED
Iteration: --- → 37.2
Points: --- → 1
Flags: qe-verify-
Flags: in-testsuite+
Flags: firefox-backlog+
Nice work on this patch - maybe you want to take a look at, say, bug 1119682 next? :-)
Iteration: 37.2 → ---
Anything else I must do here? Like add the reviewer tag in the commit message?
(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). :-)
Thank You! :-) Having an amazing time as a newbie here.
https://hg.mozilla.org/mozilla-central/rev/569130fd40b5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Iteration: --- → 37.3 - 12 Jan
You need to log in before you can comment on or make changes to this bug.