Closed Bug 1447950 Opened 3 years ago Closed 3 years ago

Use switch statement in AsyncTabSwitcher.handleEvent

Categories

(Firefox :: Tabbed Browser, enhancement, P3)

Product:
Firefox
Component:
Tabbed Browser
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: dao, Assigned: dwlsalmeida, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(1 file, 3 obsolete files)

(fourth attempt) 0001-Bug-1447950-use-a-switch-statement-in-place-of-chain.patch
2.09 KB, patch
dao
: review+
Details | Diff | Splinter Review
status-firefox61: affected → ---
Priority: -- → P3
This seems nice for a very first contribution!

Please assign it to me, I'll try to submitting a patch.

Thanks.
Okay :)
Assignee: nobody → dwlsalmeida
Status: NEW → ASSIGNED
I have a question:

How can I make sure my new code has no obvious mistakes such as syntax errors? All I did was successfully run ./mach build again, is this enough?
(In reply to Daniel de Almeida from comment #4)
> I have a question:
> 
> How can I make sure my new code has no obvious mistakes such as syntax
> errors? All I did was successfully run ./mach build again, is this enough?

./mach eslint browser/modules/AsyncTabSwitcher.jsm
Please check out my second attempt after running eslint
Attachment #8961806 - Attachment is obsolete: true
Attachment #8961828 - Attachment is obsolete: true
Comment on attachment 8961829 [details] [diff] [review]
(third attempt) 0001-Bug-1447950-use-a-switch-statement-in-place-of-chain.patch

>+    switch (event.type) {
>+
>+      case "MozLayerTreeReady":
>+        this.onLayersReady(event.originalTarget);
>+        break;
>+
>+      case "MozAfterPaint":
>+        this.onPaint();
>+        break;
>+
>+      case "MozLayerTreeCleared":
>+        this.onLayersCleared(event.originalTarget);
>+        break;
>+
>+      case "TabRemotenessChange":
>+        this.onRemotenessChange(event.target);
>+        break;
>+
>+      case "sizemodechange":
>+      case "occlusionstatechange":
>+        this.onSizeModeOrOcclusionStateChange();
>+        break;
>+
>+      case "SwapDocShells":
>+        this.onSwapDocShells(event.originalTarget, event.detail);
>+        break;
>+
>+
>+      case "EndSwapDocShells":
>+        this.onEndSwapDocShells(event.originalTarget, event.detail);
>+        break;
>+
>     }

Could you please remove the empty lines within the switch statement?

Looks good otherwise!
Comment on attachment 8961853 [details] [diff] [review]
(fourth attempt) 0001-Bug-1447950-use-a-switch-statement-in-place-of-chain.patch

Yep. Thanks!
Attachment #8961853 - Flags: review+
Keywords: checkin-needed
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c616e0e41de0
use a switch statement in place of chained if-else in AsyncTabSwitcher.handleEvent. r=dao
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c616e0e41de0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.