64582 - Um...some navigator cleanup, or something

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Blake Ross]

Got bored.  jag, can you r= this?

Comment 1

[Blake Ross]

Attachment: [patch] random nav cleanup

Comment 2

[Peter "jag" Annema]

@@ -73,8 +68,7 @@
 function getContentAreaFrameCount()
 {
   var saveFrameItem = document.getElementById("savepage");
-  var focusedWindow = document.commandDispatcher.focusedWindow;
-  if (!_content.frames.length || !isDocumentFrame(focusedWindow))
+  if (!_content.frames.length ||
!isDocumentFrame(document.commandDispatcher.focusedWindow))
     saveFrameItem.setAttribute("hidden", "true");
   else
     saveFrameItem.removeAttribute("hidden");
@@ -85,11 +79,10 @@
  **/
 function contentAreaFrameFocus()
 {
-  var saveFrameItem = document.getElementById("savepage");
   var focusedWindow = document.commandDispatcher.focusedWindow;
   if (isDocumentFrame(focusedWindow)) {
     gFocusedURL = focusedWindow.location.href;
-    saveFrameItem.removeAttribute("hidden");
+    document.getElementById("savepage").removeAttribute("hidden");
   }
 }

I rewrote those bits to make them more similar and hopefully a bit more
readable. I think you effectively rewrote them back to the way they were.

Nice catch on the var startPage. I used to load it outside the if block, so
needed it declared outside it, but had to fix that for the page cycling code.

         searchDS.RememberLastSearchText(escapedSearchStr);
-
+        var searchEngineURI = null;

Keep the blank line. Or rather:

        try {
          var searchEngineURI =
pref.CopyCharPref("browser.search.defaultengine");
          if (searchEngineURI) {
            var searchURL = searchDS.GetInternetSearchURL(searchEngineURI,
escapedSearchStr);
            if (searchURL)
              defaultSearchURL = searchURL;
          }
        } catch(ex) {
        }

Next bit:

-  if (bookmarksWindow) {
-    //debug("Reuse existing bookmarks window");
+  if (bookmarksWindow) {;

Spot the stray semi-colon :-)

+function GetBrowserDocShell() {
+  var docShell = null;
+  var browserElement = document.getElementById("content");
+  docShell =
browserElement.boxObject.QueryInterface(Components.interfaces.nsIBrowserBoxObject).docShell;
+  return docShell;
+}

Nah, just remove the function, it isn't used any longer. Those indented bits are
gonna go anyway as soon as I get r= and sr= for bug 64449 (hint).

 function loadURI(uri)
 {
-  try {
-    // _content.location.href = uri;
-    getWebNavigation().loadURI(uri, nsIWebNavigation.LOAD_FLAGS_NONE);
-  } catch (e) {
-    debug("Didn't load uri: '"+uri+"'\n");
-    debug(e);
-  }
+  getWebNavigation().loadURI(uri, nsIWebNavigation.LOAD_FLAGS_NONE);
 }

I've actually rewritten that bit like this:

 function loadURI(uri)
 {
   try {
     // _content.location.href = uri;
     getWebNavigation().loadURI(uri, nsIWebNavigation.LOAD_FLAGS_NONE);
   } catch (e) {
-    debug("Didn't load uri: '"+uri+"'\n");
-    debug(e);
   }
 }

The reason for the try/catch is that the loadURI could fail when for example the
protocol is unknown (e.g. foo://hello). It should pop up a dialog, but will also
return an error value, causing an exception to be thrown in js, which we'll need
to catch, since we've already dealt with the error and don't need to scare the
user with an exception ;-). Maybe we should put in a dump or test the exception
so we only catch those we know are already dealt with correctly.

That's it. Rest looks good :-)

Comment 3

[Blake Ross]

I would rather not retrieve the saveFrameItem if !_content.frames.length is hit 
first...

eek...missed that semicolon. Thanks.

Okay, I'll remove it.  I was wondering why they were indented.  Yes, I'll get 
to that review :-)

Hmmm...ok, I'll use yours.  Do we need this? // _content.location.href = uri;

Comment 4

[Blake Ross]

err...s/saveFrameItem/focusedWindow

In the second function, I'm fine with keeping var saveFrameItem = 
document.getElementById("savepage") (although I'm not sure storing it in a 
variable is really necessary there), but it should still be moved inside the 
|if|.

Comment 5

[Blake Ross]

Attachment: [patch] addressing comments

Comment 6

[Peter "jag" Annema]

Okay, but in that case:

-  var saveFrameItem = document.getElementById("savepage");
   var focusedWindow = document.commandDispatcher.focusedWindow;
   if (isDocumentFrame(focusedWindow)) {

-->

-  var saveFrameItem = document.getElementById("savepage");
-   var focusedWindow = document.commandDispatcher.focusedWindow;
-  if (isDocumentFrame(focusedWindow)) {
+  if (isDocumentFrame(document.commandDispatcher.focusedWindow)) {

Could you keep the blank line after this one:
         searchDS.RememberLastSearchText(escapedSearchStr);

-  } catch (e) {
-    debug("Didn't load uri: '"+uri+"'\n");
-    debug(e);
+  } catch(e) {

Please keep the space between catch and (e).

One more attachment should do it :-)

Comment 7

[Blake Ross]

Attachment: [patch] addressing new comments

Comment 8

[Peter "jag" Annema]

r=jag

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

a=[e-mail]

Comment 10

[Blake Ross]

Fix checked in.

Sarah, this can just be verified if you use Navigator without any problems :-)

Comment 11

[John Morrison]

Reopening ... er, I noticed one thing that is wrong on two counts.

----------------------------------------
@@ -1424,26 +1350,18 @@
 }
 
 //Posts the currently displayed url to a native widget so third-party apps can 
observe it.
-var urlWidgetService = null;
-try {
-  urlWidgetService = Components.classes["@mozilla.org/urlwidget;1"]
-                               
.getService(Components.interfaces.nsIUrlWidget);
-} catch (exception) {
-  //debug("Error getting url widget service: " + exception + "\n");
-}
-
 function postURLToNativeWidget()
 {
-  if (urlWidgetService) {
-    // XXX This somehow causes a big leak, back to the old way
-    //     till we figure out why. See bug 61886.
-    // var url = getWebNavigation().currentURI.spec;
-    var url = _content.location.href;
-    try {
-      urlWidgetService.SetURLToHiddenControl(url, window);
-    } catch (exception) {
-      debug("SetURLToHiddenControl failed: " + exception + "\n");
-    }
+  var urlWidgetService = Components.classes["@mozilla.org/urlwidget;1"]
+                                   
.getService(Components.interfaces.nsIUrlWidget);
+
+  // XXX This somehow causes a big leak, back to the old way
+  //     till we figure out why. See bug 61886.
+  // var url = getWebNavigation().currentURI.spec;
+  var url = _content.location.href;
+  try {
+    urlWidgetService.SetURLToHiddenControl(url, window);
+  } catch(ex) {
   }
 }
----------------------------------------
 
1) '@mozilla.org/urlwidget;1' is a win32-only component, so that call to 
   .getService will fail on every page load for Mac and Linux, with the 
   exception being thrown into the console every time. See bug 45753 and 
   rev 1.204 of navigator.js

2) Since that function is called on every page load, with the new code
   we are doing the .getService every time, instead of just doing a property
   lookup on |window| for a reference to the service. Whatever it is, it
   ain't gonna be faster :-]

Comment 12

[Blake Ross]

Attachment: [patch] leave that the way it was

Comment 13

[John Morrison]

Actually, after looking at navigator.js, I see that a number of the other
onload handlers will call .getService() every time. So, maybe it's an 
inconsequential difference in time (or maybe not). Anyone know the cost?

Point (1) though is definitely true. Hey, maybe we should run a pool on what 
time the first linux user files a bug report :-]

But r=jrgm for that patch (and you can decide whether point (2) is valid or 
not).

Comment 14

[Blake Ross]

Checked in.  I don't think we should retrieve the service each time.  Sorry, I 
hadn't realized how often this was called.  Thanks for catching it.

Comment 15

[sairuh (rarely reading bugmail)]

checked lxr and vrfy'd blake's changes.