Closed Bug 1254496 Opened 9 years ago Closed 9 years ago

Remove gURLBar null-checks

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: dao, Assigned: malayaleecoder, Mentored)

References

(Depends on 1 open bug, )

Details

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

Attachments

(1 file, 3 obsolete files)

The location bar can't be removed anymore, so gURLBar should always exist. We should stop null-checking it.
Attached 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)
You shouldn't remove the code blocks altogether, but just the null-checks. e.g.: if (gURLBar) { whatever(); } should become: whatever();
Flags: needinfo?(dao)
Attached 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)
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+
Attached 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)
Attachment #8729023 - Flags: review?(dao) → review+
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-
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)".
Sorry for missing that gURLBar :P Have done the said changes.
Attachment #8729023 - Attachment is obsolete: true
Attachment #8729085 - Flags: review?(dao)
Attachment #8729085 - Flags: review?(dao) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Flags: needinfo?(dao)
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.

Attachment

General

Created:
Updated:
Size: