Closed Bug 825974 Opened 7 years ago Closed 7 years ago

Use "switch" instead of "else if" series in BrowserApp.observe()

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 20

People

(Reporter: tetsuharu, Assigned: tetsuharu)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch v1 (obsolete) — Splinter Review
No description provided.
Attachment #697082 - Attachment filename: switch → patch
Attachment #697082 - Attachment is patch: true
Attached patch patch v2 (obsolete) — Splinter Review
Fix some mistake.
Attachment #697082 - Attachment is obsolete: true
Attachment #697083 - Flags: review?(margaret.leibovic)
Summary: Use `switch` instead of `else if` series in BrowserApp.observe() → Use "switch" instead of "else if" series in BrowserApp.observe()
Comment on attachment 697083 [details] [diff] [review]
patch v2

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

hi Tetsuharu, thanks for the patch! Here are some comments. It looks good to me, but Margaret should take a look, too.

::: mobile/android/chrome/content/browser.js
@@ +1065,5 @@
>  
> +    switch (aTopic) {
> +      case "Session:Back":
> +        browser.goBack();
> +        break;

I think we should insert a blank line between each break and case line. I think the normal coding style is without blank lines, but this function is so long, that I think the blank lines will improve readability here.

@@ +1094,5 @@
> +          tabID: data.tabID,
> +          isPrivate: (data.isPrivate === true),
> +          pinned: (data.pinned === true),
> +          delayLoad: (delayLoad === true),
> +          desktopMode: (data.desktopMode === true)

The old code tested equality with `==`, not `===`. I agree that `===` is usually preferred, but we don't want to risk regressions by mixing up multiple changes in the same patch.

@@ +1110,5 @@
> +
> +        // Don't show progress throbber for about:home or about:reader
> +        if (!shouldShowProgress(url)) {
> +          params.showProgress = false;
> +        }

The old code didn't have {} braces, so we should just leave them off here.

@@ +1116,5 @@
> +        if (data.newTab) {
> +          this.addTab(url, params);
> +        }
> +        else {
> +          this.loadURI(url, browser, params);

The old code didn't have {} braces, so we should just leave them off here.

@@ +1150,5 @@
> +        break;
> +      case "Viewport:Change":
> +        if (this.isBrowserContentDocumentDisplayed()) {
> +          this.selectedTab.setViewport(JSON.parse(aData));
> +        }

The old code didn't have {} braces, so we should just leave them off here.

@@ +1164,5 @@
> +        sendMessageToJava({gecko: { type: "Passwords:Init:Return" }});
> +        Services.obs.removeObserver(this, "Passwords:Init", false);
> +        break;
> +      }
> +      case "FormHistory:Init": {

I see that your patch v2 adds the `break` here that was missing in patch v1. :)

@@ +1192,1 @@
>      }

Let's add a `default` case that prints a warning to logcat using the dump() function. Something like dump('BrowserApp.observe: unexpected topic "' + aTopic + '"');
Attachment #697083 - Flags: feedback+
Assignee: nobody → saneyuki.s.snyk
Status: NEW → ASSIGNED
Attached patch patch v3Splinter Review
Thank you feedback, Chris.

> The old code tested equality with `==`, not `===`. I agree that `===` is
> usually preferred, but we don't want to risk regressions by mixing up
> multiple changes in the same patch.

OK. I'll file the new bug for replacing from `==` to `===` in this method.
Attachment #697083 - Attachment is obsolete: true
Attachment #697083 - Flags: review?(margaret.leibovic)
Attachment #697120 - Flags: review?(margaret.leibovic)
Comment on attachment 697120 [details] [diff] [review]
patch v3

This looks good to me and feels like a nice cleanup.
Attachment #697120 - Flags: review?(margaret.leibovic) → review+
Thank you for speedy review, Margaret.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/826872a472a1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
You need to log in before you can comment on or make changes to this bug.