[Australis] Remove the "Open Location" dialog (openLocation.xul)

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Location Bar
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: dao, Assigned: marlio)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=xul][mentor=dao])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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

4 years ago
I would like to work on this bug. Can you please assign me in?
Flags: needinfo?
(Reporter)

Updated

4 years ago
Assignee: nobody → malinthak2
Flags: needinfo?
(Assignee)

Comment 2

4 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

4 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

4 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

4 years ago
Created attachment 8362234 [details] [diff] [review]
openlocation_removed.patch
Attachment #8362234 - Flags: review?(dao)
(Assignee)

Comment 6

4 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

4 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

4 years ago
Thank you for the feedback. I'm working on it.
(Assignee)

Comment 9

4 years ago
Created attachment 8363842 [details] [diff] [review]
remove_openLocation_2.patch

Added suggested changes.
Attachment #8363842 - Flags: review?(dao)
(Reporter)

Comment 10

4 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

4 years ago
Attachment #8362234 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
Created attachment 8364780 [details] [diff] [review]
bug_953124_remove_openLocation.patch

Added changes to sanitizer.js,Browser.js and openLocation.properties
Attachment #8363842 - Attachment is obsolete: true
Attachment #8364780 - Flags: review?(dao)
(Reporter)

Comment 12

4 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

4 years ago
Created attachment 8365479 [details] [diff] [review]
bug_953124_remove_openLocation.patch

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

4 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

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/1a635b8dc7fa
https://hg.mozilla.org/mozilla-central/rev/1a635b8dc7fa
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
(Assignee)

Comment 17

3 years ago
Thank you for the support Dao. :)

Updated

3 years ago
Blocks: 1012755

Updated

3 years ago
Blocks: 1008793
No longer blocks: 1012755

Updated

3 years ago
No longer blocks: 1008793
You need to log in before you can comment on or make changes to this bug.