Um...some navigator cleanup, or something

VERIFIED FIXED

Status

SeaMonkey
UI Design
VERIFIED FIXED
17 years ago
9 years ago

People

(Reporter: Blake Ross, Assigned: Blake Ross)

Tracking

Trunk
x86
Windows ME

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

17 years ago
Got bored.  jag, can you r= this?
(Assignee)

Comment 1

17 years ago
Created attachment 21990 [details] [diff] [review]
[patch] random nav cleanup

Comment 2

17 years ago
@@ -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 :-)
(Assignee)

Comment 3

17 years ago
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;
Status: NEW → ASSIGNED
(Assignee)

Comment 4

17 years ago
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|.
(Assignee)

Comment 5

17 years ago
Created attachment 21994 [details] [diff] [review]
[patch] addressing comments

Comment 6

17 years ago
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 :-)
(Assignee)

Comment 7

17 years ago
Created attachment 22002 [details] [diff] [review]
[patch] addressing new comments

Comment 8

17 years ago
r=jag
a=ben@netscape.com
(Assignee)

Comment 10

17 years ago
Fix checked in.

Sarah, this can just be verified if you use Navigator without any problems :-)
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 11

17 years ago
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 :-]

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

17 years ago
Created attachment 22034 [details] [diff] [review]
[patch] leave that the way it was

Comment 13

17 years ago
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).
(Assignee)

Comment 14

17 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
checked lxr and vrfy'd blake's changes.
Status: RESOLVED → VERIFIED
Product: Core → Mozilla Application Suite

Updated

9 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.