Remove gURLBar null-checks

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: dao, Assigned: malayaleecoder, Mentored)

Tracking

(Depends on 1 bug)

Trunk
Firefox 48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

3 years ago
The location bar can't be removed anymore, so gURLBar should always exist. We should stop null-checking it.
Assignee

Comment 1

3 years ago
Posted patch Bug1254496_v1.diff (obsolete) — Splinter Review
Hello Dao,

I have attached an initial patch for this Bug. Please have a look.
After the removal of checks, I see that ,

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#988
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1838

are not looking so good. What's your opinion here?
Assignee: nobody → malayaleecoder
Status: NEW → ASSIGNED
Flags: needinfo?(dao)
Reporter

Comment 2

3 years ago
You shouldn't remove the code blocks altogether, but just the null-checks.

e.g.:

  if (gURLBar) {
    whatever();
  }

should become:

  whatever();
Flags: needinfo?(dao)
Assignee

Comment 3

3 years ago
Posted patch Bug1254496_v2.diff (obsolete) — Splinter Review
Very Sorry, I thought of gURLBar being always null :P
Please have a look now.
Attachment #8727935 - Attachment is obsolete: true
Flags: needinfo?(dao)
Attachment #8728286 - Flags: review?(dao)
Reporter

Comment 4

3 years ago
Comment on attachment 8728286 [details] [diff] [review]
Bug1254496_v2.diff

> function focusAndSelectUrlBar() {
>-  if (gURLBar) {
>-    if (window.fullScreen)
>-      FullScreen.showNavToolbox();
>-
>-    gURLBar.select();
>-    if (document.activeElement == gURLBar.inputField)
>-      return true;
>-  }
>+  if (window.fullScreen)
>+    FullScreen.showNavToolbox();
>+
>+  gURLBar.select();
>+  if (document.activeElement == gURLBar.inputField)
>+    return true;
>   return false;
> }

nit: please add a blank line between the two return statements

>+      BookmarkingUI.onLocationChange();
>+      SocialUI.updateState();
>+      UITour.onLocationChange(location);
>+      gTabletModePageCounter.inc();

nit: please insert blank lines between these lines as well

>-    if (gURLBar) {
>-      let value = gURLBar.getAttribute("autocompletesearchparam") || "";
>-      if (!PrivateBrowsingUtils.permanentPrivateBrowsing &&
>-          !value.includes("disable-private-actions")) {
>-        // Disable switch to tab autocompletion for private windows.
>-        // We leave it enabled for permanent private browsing mode though.
>-        value += " disable-private-actions";
>-      }
>-      if (!value.includes("private-window")) {
>-        value += " private-window";
>-      }
>-      gURLBar.setAttribute("autocompletesearchparam", value);
>-    }
>+    let value = gURLBar.getAttribute("autocompletesearchparam") || "";
>+    if (!PrivateBrowsingUtils.permanentPrivateBrowsing &&
>+        !value.includes("disable-private-actions")) {
>+      // Disable switch to tab autocompletion for private windows.
>+      // We leave it enabled for permanent private browsing mode though.
>+      value += " disable-private-actions";
>+    }
>+    if (!value.includes("private-window")) {
>+      value += " private-window";
>+    }
>+    gURLBar.setAttribute("autocompletesearchparam", value);

Now that this code doesn't have its own separate block anymore, please rename "value" to something more distinctive, e.g. "urlbarSearchParam".

Looks good otherwise. Thanks!
Flags: needinfo?(dao)
Attachment #8728286 - Flags: review?(dao) → review+
Assignee

Comment 5

3 years ago
Posted patch Bug1254496_v3.diff (obsolete) — Splinter Review
Dao, I have made the suggested changes. Please have a look :)
Attachment #8728286 - Attachment is obsolete: true
Attachment #8729023 - Flags: review?(dao)
Reporter

Updated

3 years ago
Attachment #8729023 - Flags: review?(dao) → review+
Reporter

Comment 6

3 years ago
Comment on attachment 8729023 [details] [diff] [review]
Bug1254496_v3.diff

Oh wait. We actually need to keep the null-check in focusAndSelectUrlBar, because on OS X this function can be called via openLocation in non-browser windows. Sorry about that.
Attachment #8729023 - Flags: review+ → review-
Reporter

Comment 7

3 years ago
Comment on attachment 8729023 [details] [diff] [review]
Bug1254496_v3.diff

>@@ -462,30 +460,28 @@ var gPopupBlockerObserver = {
>     if (aEvent.originalTarget != gBrowser.selectedBrowser)
>       return;
> 
>     if (!this._reportButton && gURLBar)
>       this._reportButton = document.getElementById("page-report-button");
> 
>     if (!gBrowser.selectedBrowser.blockedPopups) {
>       // Hide the icon in the location bar (if the location bar exists)
>-      if (gURLBar)
>-        this._reportButton.hidden = true;
>+      this._reportButton.hidden = true;

While you're at it, you can also remove "&& gURLBar" from "if (!this._reportButton && gURLBar)".
Assignee

Comment 8

3 years ago
Sorry for missing that gURLBar :P
Have done the said changes.
Attachment #8729023 - Attachment is obsolete: true
Attachment #8729085 - Flags: review?(dao)
Reporter

Updated

3 years ago
Attachment #8729085 - Flags: review?(dao) → review+
Assignee

Comment 9

3 years ago
Dao, I am adding checkin-needed for this. Hope it's the right thing to do and won't be a problem.
Flags: needinfo?(dao)
Keywords: checkin-needed

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dd5aaa1e47ad
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Reporter

Updated

3 years ago
Flags: needinfo?(dao)

Comment 12

2 years ago
This fix seems to have caused bug 1332391.
Depends on: 1332391
You need to log in before you can comment on or make changes to this bug.