Closed
Bug 1254496
Opened 9 years ago
Closed 9 years ago
Remove gURLBar null-checks
Categories
(Firefox :: General, defect)
Firefox
General
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)
8.72 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
The location bar can't be removed anymore, so gURLBar should always exist. We should stop null-checking it.
Reporter | ||
Updated•9 years ago
|
Assignee | ||
Comment 1•9 years ago
|
||
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•9 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•9 years ago
|
||
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•9 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•9 years ago
|
||
Dao, I have made the suggested changes. Please have a look :)
Attachment #8728286 -
Attachment is obsolete: true
Attachment #8729023 -
Flags: review?(dao)
Reporter | ||
Updated•9 years ago
|
Attachment #8729023 -
Flags: review?(dao) → review+
Reporter | ||
Comment 6•9 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•9 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•9 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•9 years ago
|
Attachment #8729085 -
Flags: review?(dao) → review+
Assignee | ||
Comment 9•9 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 10•9 years ago
|
||
Keywords: checkin-needed
Comment 11•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(dao)
Comment 12•8 years ago
|
||
This fix seems to have caused bug 1332391.
You need to log in
before you can comment on or make changes to this bug.
Description
•