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

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Dão Gottwald [:dao]]

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.

Comment 1

[Malintha Fernando [:marlio]]

I would like to work on this bug. Can you please assign me in?

Comment 2

[Malintha Fernando [:marlio]]

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?

Comment 3

[Dão Gottwald [:dao]]

(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.

Comment 4

[Malintha Fernando [:marlio]]

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?

Comment 5

[Malintha Fernando [:marlio]]

Attachment: openlocation_removed.patch

Comment 6

[Malintha Fernando [:marlio]]

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.

Comment 7

[Dão Gottwald [:dao]]

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".

Comment 8

[Malintha Fernando [:marlio]]

Thank you for the feedback. I'm working on it.

Comment 9

[Malintha Fernando [:marlio]]

Attachment: remove_openLocation_2.patch

Added suggested changes.

Comment 10

[Dão Gottwald [:dao]]

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 :)

Comment 11

[Malintha Fernando [:marlio]]

Attachment: bug_953124_remove_openLocation.patch

Added changes to sanitizer.js,Browser.js and openLocation.properties

Comment 12

[Dão Gottwald [:dao]]

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.

Comment 13

[Malintha Fernando [:marlio]]

Attachment: bug_953124_remove_openLocation.patch

Sorry, I was just confused by seeing the suggestion below the code snippet. It's corrected now.

Comment 14

[Dão Gottwald [:dao]]

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!

Comment 15

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/1a635b8dc7fa

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/1a635b8dc7fa

Comment 17

[Malintha Fernando [:marlio]]

Thank you for the support Dao. :)