Closed
Bug 953124
Opened 11 years ago
Closed 10 years ago
[Australis] Remove the "Open Location" dialog (openLocation.xul)
Categories
(Firefox :: Address Bar, defect)
Firefox
Address Bar
Tracking
()
RESOLVED
FIXED
Firefox 29
People
(Reporter: dao, Assigned: marlio)
References
Details
(Whiteboard: [good first bug][lang=xul][mentor=dao])
Attachments
(1 file, 3 obsolete files)
25.34 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
Since the location bar cannot be hidden or removed anymore, there should be no way for the openLocation function to spawn openLocation.xul: http://hg.mozilla.org/mozilla-central/annotate/cd3e9359fd64/browser/base/content/browser.js#l1712 We should remove that dialog.
Assignee | ||
Comment 1•11 years ago
|
||
I would like to work on this bug. Can you please assign me in?
Flags: needinfo?
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → malinthak2
Flags: needinfo?
Assignee | ||
Comment 2•11 years ago
|
||
There is no way to load the openLocation dialog box from the browser except using the chrome url I think. I tried with ctrl+L, but didn't give any result except highlighting the location bar. So do we just need to remove code snippets from browser.js or disable from the chrome url too?
Reporter | ||
Comment 3•11 years ago
|
||
(In reply to Malintha Fernando from comment #2) > There is no way to load the openLocation dialog box from the browser except > using the chrome url I think. I tried with ctrl+L, but didn't give any > result except highlighting the location bar. So do we just need to remove > code snippets from browser.js or disable from the chrome url too? We should disable the chrome URL too, as well as completely remove openLocation.xul from the source tree.
Assignee | ||
Comment 4•10 years ago
|
||
openLocation.xul has also mentioned in http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/openLocation.dtd#5 , but seems .dtd file can't be removed(build gives return code 2). Is it no problem to leave it as it is?
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8362234 -
Flags: review?(dao)
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8362234 [details] [diff] [review] openlocation_removed.patch Review of attachment 8362234 [details] [diff] [review]: ----------------------------------------------------------------- This removes access to openLocation.xul even through the chrome url.
Reporter | ||
Comment 7•10 years ago
|
||
Comment on attachment 8362234 [details] [diff] [review] openlocation_removed.patch This is a good start but needs a couple of adjustments. >--- a/browser/base/content/browser.js >+++ b/browser/base/content/browser.js >-function openLocation() { >- if (focusAndSelectUrlBar()) >- return; >- >-#ifdef XP_MACOSX >- if (window.location.href != getBrowserURL()) { >- var win = getTopWin(); >- if (win) { >- // If there's an open browser window, it should handle this command >- win.focus() >- win.openLocation(); >- } >- else { >- // If there are no open browser windows, open a new one >- win = window.openDialog("chrome://browser/content/", "_blank", >- "chrome,all,dialog=no", BROWSER_NEW_TAB_URL); >- win.addEventListener("load", openLocationCallback, false); >- } >- return; >- } >-#endif >- openDialog("chrome://browser/content/openLocation.xul", "_blank", >- "chrome,modal,titlebar", window); >-} We actually need to keep most of this function since it's responsible for letting Ctrl+L focus the location bar. Just remove the part that opens openLocation.xul. >-Components.utils.import("resource:///modules/openLocationLastURL.jsm", openLocationModule); This module is now unused as well and can be removed. It's located at browser/modules/openLocationLastURL.jsm and also needs to be removed from browser/modules/moz.build. >-<!DOCTYPE dialog SYSTEM "chrome://browser/locale/openLocation.dtd"> This is also unused. You can find it at browser/locales/en-US/chrome/browser/openLocation.dtd. Also remove it from browser/locales/jar.mn. >- <stringbundle id="openLocationBundle" src="chrome://browser/locale/openLocation.properties"/> ditto >--- a/browser/components/privatebrowsing/test/browser/browser.ini >+++ b/browser/components/privatebrowsing/test/browser/browser.ini >@@ -28,17 +28,17 @@ support-files = > [browser_privatebrowsing_geoprompt.js] > [browser_privatebrowsing_lastpbcontextexited.js] > [browser_privatebrowsing_localStorage.js] > [browser_privatebrowsing_localStorage_before_after.js] > [browser_privatebrowsing_noSessionRestoreMenuOption.js] > [browser_privatebrowsing_nonbrowser.js] > [browser_privatebrowsing_openLocationLastURL.js] > [browser_privatebrowsing_opendir.js] >-[browser_privatebrowsing_openlocation.js] >+ > [browser_privatebrowsing_placesTitleNoUpdate.js] > [browser_privatebrowsing_placestitle.js] Please remove the blank line. We also need to update browser/base/content/sanitize.js and remove its use of "general.open_location.last_url".
Attachment #8362234 -
Flags: review?(dao)
Attachment #8362234 -
Flags: review-
Attachment #8362234 -
Flags: feedback+
Assignee | ||
Comment 8•10 years ago
|
||
Thank you for the feedback. I'm working on it.
Assignee | ||
Comment 9•10 years ago
|
||
Added suggested changes.
Attachment #8363842 -
Flags: review?(dao)
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8363842 [details] [diff] [review] remove_openLocation_2.patch >--- a/browser/base/content/browser.js >+++ b/browser/base/content/browser.js >@@ -1689,42 +1689,18 @@ function focusAndSelectUrlBar() { > } > return false; > } > > function openLocation() { > if (focusAndSelectUrlBar()) > return; > >-#ifdef XP_MACOSX >- if (window.location.href != getBrowserURL()) { >- var win = getTopWin(); >- if (win) { >- // If there's an open browser window, it should handle this command >- win.focus() >- win.openLocation(); >- } >- else { >- // If there are no open browser windows, open a new one >- win = window.openDialog("chrome://browser/content/", "_blank", >- "chrome,all,dialog=no", BROWSER_NEW_TAB_URL); >- win.addEventListener("load", openLocationCallback, false); >- } >- return; >- } >-#endif We need to keep this. >- openDialog("chrome://browser/content/openLocation.xul", "_blank", >- "chrome,modal,titlebar", window); This is the only part of this function that should be removed. >--- a/browser/base/content/sanitize.js >+++ b/browser/base/content/sanitize.js >@@ -218,20 +218,16 @@ Sanitizer.prototype = { > .getService(Components.interfaces.nsIObserverService); > os.notifyObservers(null, "browser:purge-session-history", ""); > } > catch (e) { } > > // Clear last URL of the Open Web Location dialog > var prefs = Components.classes["@mozilla.org/preferences-service;1"] > .getService(Components.interfaces.nsIPrefBranch); >- try { >- prefs.clearUserPref("general.open_location.last_url"); >- } >- catch (e) { } Please also remove the comment and the prefs variable. Getting closer :)
Attachment #8363842 -
Flags: review?(dao)
Attachment #8363842 -
Flags: review-
Attachment #8363842 -
Flags: feedback+
Reporter | ||
Updated•10 years ago
|
Attachment #8362234 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
Added changes to sanitizer.js,Browser.js and openLocation.properties
Attachment #8363842 -
Attachment is obsolete: true
Attachment #8364780 -
Flags: review?(dao)
Reporter | ||
Comment 12•10 years ago
|
||
Comment on attachment 8364780 [details] [diff] [review] bug_953124_remove_openLocation.patch >--- a/browser/base/content/browser.js >+++ b/browser/base/content/browser.js >@@ -1689,42 +1689,20 @@ function focusAndSelectUrlBar() { > } > return false; > } > > function openLocation() { > if (focusAndSelectUrlBar()) > return; > >-#ifdef XP_MACOSX >- if (window.location.href != getBrowserURL()) { >- var win = getTopWin(); >- if (win) { >- // If there's an open browser window, it should handle this command >- win.focus() >- win.openLocation(); >- } >- else { >- // If there are no open browser windows, open a new one >- win = window.openDialog("chrome://browser/content/", "_blank", >- "chrome,all,dialog=no", BROWSER_NEW_TAB_URL); >- win.addEventListener("load", openLocationCallback, false); >- } >- return; >- } >-#endif No, we need to keep this code. > openDialog("chrome://browser/content/openLocation.xul", "_blank", > "chrome,modal,titlebar", window); And remove this code. Looks good otherwise.
Attachment #8364780 -
Flags: review?(dao)
Attachment #8364780 -
Flags: review-
Attachment #8364780 -
Flags: feedback+
Assignee | ||
Comment 13•10 years ago
|
||
Sorry, I was just confused by seeing the suggestion below the code snippet. It's corrected now.
Attachment #8364780 -
Attachment is obsolete: true
Attachment #8365479 -
Flags: review?(dao)
Reporter | ||
Comment 14•10 years ago
|
||
Comment on attachment 8365479 [details] [diff] [review] bug_953124_remove_openLocation.patch >--- a/browser/base/content/browser.js >+++ b/browser/base/content/browser.js >@@ -1706,25 +1706,19 @@ function openLocation() { > // If there are no open browser windows, open a new one > win = window.openDialog("chrome://browser/content/", "_blank", > "chrome,all,dialog=no", BROWSER_NEW_TAB_URL); > win.addEventListener("load", openLocationCallback, false); > } > return; > } > #endif Actually, we do need to remove the 'win.addEventListener("load", openLocationCallback, false);' line since you (correctly) removed the openLocationCallback function. I can make this change before landing this patch. Thanks!
Attachment #8365479 -
Flags: review?(dao) → review+
Reporter | ||
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/1a635b8dc7fa
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1a635b8dc7fa
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Assignee | ||
Comment 17•10 years ago
|
||
Thank you for the support Dao. :)
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•