Evgeniy Ivanov Reporter
	Description • 17 years ago

At this moment they are encoded. It's very inconvinient to work with such urls (for example wiki uses non-ASCII urls)

	Evgeniy Ivanov Reporter
	Comment 1 • 17 years ago

As I saw ff had some problems after changing the behavior of urlbar with autocomplition and bookmarks.
Useful links (ff code): Bug 366797 (hasn't required code, but the start point), Bug 397815, Bug 389465, Bug 410429, Bug 415460...

	Evgeniy Ivanov Reporter
	Comment 2 • 17 years ago

It Fixes the urlbar only. Autocomplition and bookmarks should be fixed too (they still show encoded text), but I want to do it in the separate bug.

	Evgeniy Ivanov Reporter
	Comment 3 • 17 years ago

Comment on attachment 312449 [details] [diff] [review]
try1, fixes only urlbar

Don't know if it is neil's mail...

	Evgeniy Ivanov Reporter
	Comment 4 • 17 years ago

Comment on attachment 312449 [details] [diff] [review]
try1, fixes only urlbar

Since it is a port of patch from Bug 397815 I ask Gavin to review it.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 5 • 17 years ago

Comment on attachment 312449 [details] [diff] [review]
try1, fixes only urlbar

I can't review this because I'm not familiar with the code you're porting it to. You should ask a SeaMonkey peer to review it.

	Robert Kaiser
	Comment 6 • 17 years ago

Comment on attachment 312449 [details] [diff] [review]
try1, fixes only urlbar

Forwarding to Neil, additionally requesting sr.

	neil@parkwaycc.co.uk
	Comment 7 • 16 years ago

Comment on attachment 312449 [details] [diff] [review]
try1, fixes only urlbar

>+  var value = getBrowser().userTypedValue;
I don't see what the point of this is.

>+      // Replace "about:blank" with an empty string
>+      // only if there's no opener (bug 370555).
>+      if (!content.opener)
This needs an additional check to match nsBrowserStatusHandler (see below).

>+      // Encode bidirectional formatting characters.
>+      // (RFC 3987 sections 3.2 and 4.1 paragraph 6)
>+      value = value.replace(/[\u200e\u200f\u202a\u202b\u202c\u202d\u202e]/g,
>+                            encodeURIComponent);
Surely these can have only been created by the decode above, in which case they can be included in the reencode?

>+  gBrowser.userTypedValue = null;
I don't see the point of these other changes either.

>+      //a part of URLBarSetURI(), calling URLBarSetURI() doesn't work
I think it would be better to move all the URLBar setting code into SetPageProxyState (when the state is "valid", of course), i.e. the fixup, the blanking (nsBrowserStatusHandler version), and the decoding. You would have to fix both(!) the other callers to pass in the correct URI.

	Misak Khachatryan
	Comment 8 • 16 years ago

I've updated Evgeniy's patch to address Neil comments. Now decoding is called in SetPageProxyState.

	Philip Chee
	Comment 9 • 16 years ago

More context (8 is recommended) and show function names in your diff please.
Suggest that you add the following to your hgrc:

[diff]
git = True
showfunc = True

[defaults]

diff =-p -U 8
qdiff =-U 8

	Misak Khachatryan
	Comment 10 • 16 years ago

Thanks Philip, now done as you suggested

	neil@parkwaycc.co.uk
	Comment 11 • 16 years ago

Comment on attachment 336802 [details] [diff] [review]
Corrected as Philip suggests, plus little nit.

>+function URLBarSetURI(aURI) {
>+  if (aURI) {
You need to fix up the URI even if you get the current URI when aURI is null.

>+  } else {
>+    // Try to decode as UTF-8 if there's no encoding sequence that we would break.
>+    if (!/%25(?:3B|2F|3F|3A|40|26|3D|2B|24|2C|23)/i.test(value))
Nit: } else if {

>@@ -2061,28 +2104,24 @@ function URLBarClickHandler(aEvent)
>       gURLBar.select();
> }
> 
>-// This function gets the shell service and has it check its setting
>-// This will do nothing on platforms without a shell service.
>+// This function gets the "windows hooks" service and has it check its setting
>+// This will do nothing on platforms other than Windows.
I think this is an accidental diff.

>-    if (url != "about:blank" || content.opener) {
>-      gURLBar.value = url;
>+    // If the value isn't empty and the urlbar has focus, select the value.
>+    if (gURLBar.value && gURLBar.hasAttribute("focused")) {
>       gURLBar.select();
>       SetPageProxyState("valid", null); // XXX Build a URI and pass it in here.
>-    } else { //if about:blank, urlbar becomes ""
>-      gURLBar.value = "";
I think you need to move the call to SetPageProxyState before the test for gURLBar.value that SetPageProxyState updates! Also, you don't need to check for the focused attribute, this function is called by a keypress handler on the urlbar so it must already be focused.

>+  return isScrolling; //TODO: FF returns !isScrolling to fix Bug 293758
Wrong bug#? I don't see the relevance.

>     gLastValidURLStr = gURLBar.value;
>     gURLBar.addEventListener("input", UpdatePageProxyState, false);
>+    URLBarSetURI(aURI);
Again, you need to set the urlbar value before reading it!

There's one other call to SetPageProxyState (in nsBrowserStatusHandler.js) and since that call now updates the URLbar the caller doesn't have to.

	Misak Khachatryan
	Comment 12 • 16 years ago

adds some code removal related with this patch. Also I've found in nsBrowserStatusHandler this chunk of code begins at line 322:

    var locationURI = null;
    var location = "";    

    if (aLocation) {
      try {
        if (!gURIFixup)
          gURIFixup = Components.classes["@mozilla.org/docshell/urifixup;1"]
                                .getService(Components.interfaces.nsIURIFixup);
        // If the url has "wyciwyg://" as the protocol, strip it off.
        // Nobody wants to see it on the urlbar for dynamically generated
        // pages.
        locationURI = gURIFixup.createExposableURI(aLocation);
        location = locationURI.spec;
      }
      catch(ex) {
        location = aLocation.spec;
      }
    }

I suspect that this code should be removed or modified - but i'm scared of touching it.

	neil@parkwaycc.co.uk
	Comment 13 • 16 years ago

(In reply to comment #12)
> adds some code removal related with this patch. Also I've found in
> nsBrowserStatusHandler this chunk of code begins at line 322:

> I suspect that this code should be removed or modified - but i'm scared of
> touching it.
Yep, it should all go, plus the two lines after it too.

	neil@parkwaycc.co.uk
	Comment 14 • 16 years ago

Comment on attachment 337025 [details] [diff] [review]
Fixed all comments

>-        browser.userTypedValue = null;
>+        SetPageProxyState("valid", null);
I think we need to keep this userTypedValue code (see the comment in nsBrowserStatusHandler.js) but perhaps it belongs in URLBarSetURI instead.

>+    gURIFixup = Cc["@mozilla.org/docshell/urifixup;1"]
>+                  .getService(Ci.nsIURIFixup);
gURIFixup = Components.classes["@mozilla.org/docshell/urifixup;1"]
                      .getService(Components.interfaces.nsIURIFixup);

	neil@parkwaycc.co.uk
	Comment 16 • 16 years ago

Comment on attachment 337414 [details] [diff] [review]
corrected patch, addresses comments #13 #14

>         browser.userTypedValue = null;
>+        SetPageProxyState("valid", null);
Incorrect order ;-) sr=me with this fixed, or the code moved as described.

>+    gURIFixup = Components.classes["@mozilla.org/docshell/urifixup;1"]
>+                  .getService(Components.interfaces.nsIURIFixup);
Nit: line up the .getService with the .classes

>+  gURLBar.value = value;
>+}
Or, set userTypedValue = null here and remove it from the callers.

>         // Setting the urlBar value in some cases causes userTypedValue to
>         // become set because of oninput, so reset it to null
Include this comment if you move the userTypedValue code.

	Misak Khachatryan
	Comment 17 • 16 years ago

carrying sr from previous comment

	Serge Gautherie (:sgautherie) Assignee
	Comment 18 • 16 years ago

Comment on attachment 337442 [details] [diff] [review]
final patch, nits fixed

(In reply to comment #16)
> >+  gURLBar.value = value;
> >+}
> Or, set userTypedValue = null here and remove it from the callers.

Misak, maybe that would be even better; I don't know...

	Misak Khachatryan
	Comment 19 • 16 years ago

There is a call to SetPageProxyState (which calls URLBarSetURI) without setting userTypedValue to null, also in one place code looks like browser.userTypedValue = null; and in another - gBrowser.userTypedValue = null. I don't know well code, maybe they are not the same, so i decided not to include browser.userTypedValue = null to URLBarSetURI code.

	Misak Khachatryan
	Comment 20 • 16 years ago

Previous patch has bad behavior - when you start typing in location bar and switch to another tab and back, location bar lost typed text and replace it by previous tab URL. There are two lines of code in suite/browser/nsBrowserStatusHandler.js that should not be deleted.

	Misak Khachatryan
	Comment 21 • 16 years ago

Neil pointed that first line can be deleted.

	Philip Chee
	Comment 22 • 16 years ago

Branch patch as requested by Misak. Seems to have applied cleanly using -p3

Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff --git a/suite/browser/navigator.js b/suite/browser/navigator.js
|--- a/suite/browser/navigator.js
|+++ b/suite/browser/navigator.js
--------------------------
Patching file navigator.js using Plan A...
Hunk #1 succeeded at 1509 (offset -33 lines).
Hunk #2 succeeded at 1924 (offset 49 lines).
Hunk #3 succeeded at 2127 (offset -14 lines).
Hunk #4 succeeded at 2197 (offset 49 lines).
Hunk #5 succeeded at 2145 (offset -14 lines).
Hunk #6 succeeded at 2241 (offset 49 lines).
Hmm...  The next patch looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff --git a/suite/browser/nsBrowserStatusHandler.js b/suite/browser/nsBrowserStatusHandler.js
|--- a/suite/browser/nsBrowserStatusHandler.js
|+++ b/suite/browser/nsBrowserStatusHandler.js
--------------------------
Patching file nsBrowserStatusHandler.js using Plan A...
Hunk #1 succeeded at 272 (offset -47 lines).
Hunk #2 succeeded at 338 with fuzz 1 (offset 4 lines).
done

	Misak Khachatryan
	Comment 23 • 16 years ago

I know that only security fixes should go to branch, but this patch make life easier for many non-english users, so it worth to ask approval. It should not be risky. Thanks Philip for patch.

	Philip Chee
	Comment 24 • 16 years ago

Misak, have you built SeaMonkey 1.1 from cvs with this patch? Have you tested to see that there are no regressions? Does this fix exist in Firefox 2.0.0.*? If so was it done differently?

	Misak Khachatryan
	Comment 25 • 16 years ago

I think the answer is no to all your questions :( , but this is JS only fix, so no build problems should happen. I don't have cvs for branch, so can't test it. It's up to maintainers to use patch in branch. I just thinks that it worth asking.

	Philip Chee
	Comment 26 • 16 years ago

Comment on attachment 339254 [details] [diff] [review]
Branch patch

Setting flags on behalf of Misak.

	Robert Kaiser
	Comment 27 • 16 years ago

Comment on attachment 339254 [details] [diff] [review]
Branch patch

1.1.12 is done, current 1.8 branch is going 1.1.13, a=me for that one.

	Misak Khachatryan
	Comment 28 • 16 years ago

I've patched my Seamonkey 1.1.11 build, which cames with Fedora 9, works ok.

	Serge Gautherie (:sgautherie) Assignee
	Comment 29 • 16 years ago

*Updated some comments and styles.
*Added missing |var|.

(In reply to comment #7)
> >+      // Replace "about:blank" with an empty string
> >+      // only if there's no opener (bug 370555).
> >+      if (!content.opener)
> This needs an additional check to match nsBrowserStatusHandler (see below).

Added.

> >+      // Encode bidirectional formatting characters.
> >+      // (RFC 3987 sections 3.2 and 4.1 paragraph 6)
> >+      value = value.replace(/[\u200e\u200f\u202a\u202b\u202c\u202d\u202e]/g,
> >+                            encodeURIComponent);
> Surely these can have only been created by the decode above, in which case they
> can be included in the reencode?

Re-added.

(In reply to comment #9)
> More context (8 is recommended) and show function names in your diff please.

(Yep.)
NB: Misak, you got the ShowFunction part right, but not the 8-lines one.

(In reply to comment #19)
> I don't know well code, maybe they are not the same, so i decided not to
> include browser.userTypedValue = null to URLBarSetURI code.

I did.
|.selectedBrowser| is superfluous, isn't it ?

	neil@parkwaycc.co.uk
	Comment 30 • 16 years ago

(In reply to comment #29)
> > >+      // Encode bidirectional formatting characters.
> > >+      // (RFC 3987 sections 3.2 and 4.1 paragraph 6)
> > >+      value = value.replace(/[\u200e\u200f\u202a\u202b\u202c\u202d\u202e]/g,
> > >+                            encodeURIComponent);
> > Surely these can have only been created by the decode above, in which case they
> > can be included in the reencode?
> Re-added.
I think what I meant here was that you could include them in the previous line.

	Serge Gautherie (:sgautherie) Assignee
	Comment 31 • 16 years ago

(In reply to comment #30)
> I think what I meant here was that you could include them in the previous line.

Yes, that's how I understood your sentence, "but":

<http://www.ietf.org/rfc/rfc3987.txt> (3.2)
states "4. Re-percent-encode all octets produced in step 3 [...]",
which I understand as not to merge them !?

***

Firefox has them even more separated (= always done), but I'm not sure why :-|

That (additional) code was added by
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.940&mark=1962-1965#1939
(also at http://hg.mozilla.org/mozilla-central/rev/4d28cdb13dca)
But
https://bugzilla.mozilla.org/attachment.cgi?id=299247&action=diff
gives me "You are not authorized to access bug #388372." :-/

Dao, where do you think this "Encode bidirectional formatting characters" part should be (merged, separate, always) ?

	Serge Gautherie (:sgautherie) Assignee
	Comment 32 • 16 years ago

(In reply to comment #31)
> Dao, where do you think this "Encode bidirectional formatting characters" part
> should be (merged, separate, always) ?

It would be worth to use some of the RFC examples as tests... (ToDo)

	Dão Gottwald [:dao]
	Comment 33 • 16 years ago

(In reply to comment #31)
> Dao, where do you think this "Encode bidirectional formatting characters" part
> should be (merged, separate, always) ?

It should always be done, as it doesn't depend on decodeURI. Bug 388372 also applies to Firefox 2, although we never landed a fix there.

	Dão Gottwald [:dao]
	Comment 34 • 16 years ago

See also bug 415491.

	neil@parkwaycc.co.uk
	Comment 35 • 16 years ago

Comment on attachment 339740 [details] [diff] [review]
(Av4) fixed and extended

>+                // 3. Encode bidirectional formatting characters.
>+                //    (RFC 3987 sections 3.2 and 4.1 paragraph 6)
>+                .replace(/[\u200e\u200f\u202a\u202b\u202c\u202d\u202e]/g,
>+                         encodeURIComponent);
>+    } catch (e) {}
>+  }
>+
>+  gURLBar.value = value;
OK, so conveniently I am able to read bug 388372 so I now understand that the replace for step 3 should go on this line and not inside the try/catch.

I wonder whether the regexp can be simplified using a character range.

	Serge Gautherie (:sgautherie) Assignee
	Comment 36 • 16 years ago

Av4, with comment 35 suggestion(s).

I tested the simplified regexp in the error/js console.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 37 • 16 years ago

You will probably want the (yet unlanded) fix for bug 452979, as well.

	Philip Chee
	Comment 38 • 16 years ago

> You will probably want the (yet unlanded) fix for bug 452979, as well.
It's a security sensitive bug. I don't think us proles have access to that at the moment.

	neil@parkwaycc.co.uk
	Comment 39 • 16 years ago

Comment on attachment 339880 [details] [diff] [review]
(Av5) fixed and extended

>+    // Encode bidirectional formatting characters.
>+    // (RFC 3987 sections 3.2 and 4.1 paragraph 6)
>+    value = value.replace(/[\u200e\u200f\u202a-\u202e]/g, encodeURIComponent);
Bug 452979 adds invisible control characters (\x0B, \x0C, \x1C to \x1F), the soft hyphen (\xAD), the zero-width space (\u200b), separators (\u2028, \u2029) and the BOM (\ufeff) to the list, although I'm tempted to suggest \x0A and \x0D too, or maybe all of \x00 to \x1F.

	Misak Khachatryan
	Comment 40 • 16 years ago

Bug 452979 marked as fixed, this bug can proceed too.

	Misak Khachatryan
	Comment 41 • 16 years ago

This patch is contain fix for bug 452979. Do we need branch patch ?

	neil@parkwaycc.co.uk
	Comment 42 • 16 years ago

Comment on attachment 360690 [details] [diff] [review]
above patch with fix for bug 452979

>-        if (oldURL != "about:blank" || content.opener) {
>-          gURLBar.value = oldURL;
>-          SetPageProxyState("valid", null);
>-        } else
>-          gURLBar.value = "";
>-
>-        browser.userTypedValue = null;
>+        // Reset url in the urlbar
>+        SetPageProxyState("valid", null);
With this patch, pressing Escape on a blank window makes the proxy state valid, when it should still be invalid when the URL bar is empty.

>+    // Replace "about:blank" with an empty string,
>+    // only if there's no history and no opener (bug 370555).
>+    if (!(getWebNavigation().canGoBack || content.opener))
I think this would be slightly more readable as
if (!getWebNavigation().canGoBack && !content.opener)

>+        value = decodeURI(value)
>+                  // 1. decodeURI decodes %25 to %, which creates unintended
>+                  //    encoding sequences. Re-encode it, unless it's part of
>+                  //    a sequence that survived decodeURI, i.e. one for:
>+                  //    ';', '/', '?', ':', '@', '&', '=', '+', '$', ',', '#'
>+                  //    (RFC 3987 section 3.2)
>+                  // 2. Re-encode whitespace so that it doesn't get eaten away
>+                  //    by the location bar (bug 410726).
>+                  .replace(/%(?!3B|2F|3F|3A|40|26|3D|2B|24|2C|23)|[\r\n\t]/ig,
>+                           encodeURIComponent);
Please can you move the whitespace re-encode to the next replace instead.
(And update the comments accordingly, of course).

>+    // Encode invisible characters (soft hyphen, zero-width space, BOM,
>+    // line and paragraph separator, word joiner, invisible times,
>+    // invisible separator, object replacement character) (bug 452979)
>+    value = value.replace(/[\v\x0c\x1c\x1d\x1e\x1f\u00ad\u200b\ufeff\u2028\u2029\u2060\u2062\u2063\ufffc]/g,
>+                          encodeURIComponent);
Can you a) replace the letter codes with hex codes (\v is \x0b?) b) sort the codes into order c) use character ranges to reduce the length of the regexp?

	neil@parkwaycc.co.uk
	Comment 44 • 16 years ago

Comment on attachment 361773 [details] [diff] [review]
comments addressed

OK, so maybe I confused you last time by commenting on BrowserLoadURL, but both it and handleURLBarRevert have the same problem, and you undid the change to BrowserLoadURL, which only makes things worse, as sometimes you will now not decode the URL, while you didn't change handleURLBarRevert at all...

>+        value = decodeURI(value);
>+        // Re-encode whitespace so that it doesn't get eaten away
>+        // by the location bar (bug 410726).
>+        value = value.replace(/%(?!3B|2F|3F|3A|40|26|3D|2B|24|2C|23)|[\r\n\t]/ig,
>+                           encodeURIComponent);
>+      } catch (e) {}
>+
>+    // Encode invisible characters (soft hyphen, zero-width space, BOM,
>+    // line and paragraph separator, word joiner, invisible times,
>+    // invisible separator, object replacement character) (bug 452979)
>+    value = value.replace(/[\x0b\x0c\x1c-\x1f\u00ad\u200b\u2028\u2029\u2060\u2062\u2063\ufeff\ufffc]/g,
>+                          encodeURIComponent);
>+
>+    // Encode bidirectional formatting characters.
>+    // (RFC 3987 sections 3.2 and 4.1 paragraph 6)
>+    value = value.replace(/[\u200e\u200f\u202a-\u202e]/g, encodeURIComponent);
I managed to quote the wrong bits of code here too, which didn't help, but I just want one replace for the specific hex codes, and one replace for all the invisible, whitespace and bidirectional characters.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 45 • 16 years ago

I think it would be in everyone's best interest if the code used for location bar URL unescaping was identical in Firefox and SeaMonkey - could you file a followup on making Neil's proposed changes to Firefox's copy?

We could even consider consolidating the [un]escaping code in particular somewhere in the core (e.g. as a JS module in toolkit/, or rolled into UnescapeURIForUI, perhaps as part of bug 415491).

	neil@parkwaycc.co.uk
	Comment 47 • 16 years ago

Comment on attachment 361972 [details] [diff] [review]
fixed replace's

>+                  // 1. decodeURI decodes %25 to %, which creates unintended
>+                  //    encoding sequences. Re-encode it, unless it's part of
>+                  //    a sequence that survived decodeURI, i.e. one for:
>+                  //    ';', '/', '?', ':', '@', '&', '=', '+', '$', ',', '#'
>+                  //    (RFC 3987 section 3.2)
Nit: doesn't need the 1. (and the hanging indent) any more.

>   if (!throbberElement.hasAttribute("busy") && !isScrolling) {
>+    SetPageProxyState("valid", null);
This is the bit that isn't right. The page proxy state isn't necessarily valid at this point. It depends on whether the URLbar is blank or not.

	neil@parkwaycc.co.uk
	Comment 48 • 16 years ago

Oh, and you need to fix it so that it works for that "copied" code too.

	Misak Khachatryan
	Comment 49 • 16 years ago

As discussed on IRC, i've made consolidated patch, which includes fix for bug bug455877 too. Patch has one noticeable glich - it prevents to drag from location bar, trying to understand why.

	neil@parkwaycc.co.uk
	Comment 50 • 16 years ago

Comment on attachment 362211 [details] [diff] [review]
consolidated patch

>+        // Reset url in the urlbar
>+        if (url != "about:blank" || content.opener)
>+          URLBarSetURI();
>+        else //if about:blank, urlbar becomes ""
URLBarSetURI now handles the page proxy state correctly, right? so we don't need to check the url any more. (Same applies to handleURLBarRevert.)

>+  // If the url has "wyciwyg://" as the protocol, strip it off.
>+  // Nobody wants to see it on the urlbar for dynamically generated pages.
>+  if (!gURIFixup)
>+    gURIFixup = Components.classes["@mozilla.org/docshell/urifixup;1"]
>+                          .getService(Components.interfaces.nsIURIFixup);
>+  try {
>+    aURI = gURIFixup.createExposableURI(aURI);
>+  } catch (ex) {}
>+
>+  let uri = aURI || getWebNavigation().currentURI;
I think we should do this before fixup. Also call me old-fashioned but I prefer var unless there's a specific advantage to using let.

>+  SetPageProxyState(valid ? "valid" : "invalid");
No point using a temporary variable here, just inline the test.

>+  value = value.replace(/[\x09-\x0d\x1c-\x1f\u00ad\u200b\u200e\u200f\u2028-\u202e\u2060\u2062\u2063\ufeff\ufffc]/g,
>+                        encodeURIComponent);
>+
>+  return value;
Could do
return value.replace(/[...]/g, encodeURIComponent);

>-function SetPageProxyState(aState, aURI)
>+function SetPageProxyState(aState)
Don't change this, we'll look at that in bug 455877 if it needs it.

>     var observerService = Components.classes["@mozilla.org/observer-service;1"]
>-                                    .getService(Components.interfaces.nsIObserverService);
>+                            .getService(Components.interfaces.nsIObserverService);
What's this change for?

>+        URLBarSetURI(aLocation, "valid");
I'd prefer this to be URLBarSetURI(aLocation, true);

>+    if (gURLBar &&
Don't need to test for gURLBar because it always exists.

>-    const nsIChannel = Components.interfaces.nsIChannel;
>-    var urlStr = aRequest.QueryInterface(nsIChannel).originalURI.spec;
>+    var urlStr = aRequest.QueryInterface(Components.interfaces.nsIChannel).originalURI.spec;
Why this change?

	neil@parkwaycc.co.uk
	Comment 52 • 16 years ago

Comment on attachment 362408 [details] [diff] [review]
comments addressed

>+function URLBarSetURI(aURI, aValid) {
>+  var uri = aURI || getWebNavigation().currentURI;
>+
>+  // If the url has "wyciwyg://" as the protocol, strip it off.
>+  // Nobody wants to see it on the urlbar for dynamically generated pages.
>+  if (!gURIFixup)
>+    gURIFixup = Components.classes["@mozilla.org/docshell/urifixup;1"]
>+                          .getService(Components.interfaces.nsIURIFixup);
>+  try {
>+    aURI = gURIFixup.createExposableURI(aURI);
You need to switch this to uri too.

> function SetPageProxyState(aState, aURI)
When I said "Don't change this", I meant restore the original function and all of its callers. It's neither part of this bug nor indeed bug 455887. It's not always better to fix two, or in this case three, bugs in one patch.

>     const nsIChannel = Components.interfaces.nsIChannel;
>     var urlStr = aRequest.QueryInterface(nsIChannel).URI.spec;
>+    var uri = aRequest.QueryInterface(Components.interfaces.nsIChannel).URI;
You changed the wrong version here, my comment was for endDocumentLoad.

>+    if (gURLBar.value == "" &&
>+        getWebNavigation().currentURI.spec == "about:blank")
This version fits on one line ;-)
if (!gURLBar.value && getWebNavigation().currentURI.spec == "about:blank")

	neil@parkwaycc.co.uk
	Comment 54 • 16 years ago

Comment on attachment 362569 [details] [diff] [review]
v 2

r+sr=me except for the following portion which is from some other bug.

>@@ -2128,29 +2171,41 @@ function SetPageProxyState(aState, aURI)
>   if (!gProxyDeck)
>     gProxyDeck = document.getElementById("page-proxy-deck");
> 
>-  gProxyButton.setAttribute("pageproxystate", aState);
>+  gURLBar.setAttribute("pageproxystate", aState);
>+  gProxyFavIcon.setAttribute("pageproxystate", aState);
> 
>+  // the page proxy state is set to valid via OnLocationChange, which
>+  // gets called when we switch tabs.
>   if (aState == "valid") {
>     gLastValidURLStr = gURLBar.value;
>     gURLBar.addEventListener("input", UpdatePageProxyState, false);
>-    if (gBrowser.shouldLoadFavIcon(aURI)) {
>-      var favStr = gBrowser.buildFavIconString(aURI);
>-      if (favStr != gProxyFavIcon.src) {
>-        gBrowser.loadFavIcon(aURI, "src", gProxyFavIcon);
>-        gProxyDeck.selectedIndex = 0;
>-      }
>-      else gProxyDeck.selectedIndex = 1;
>-    }
>-    else {
>-      gProxyDeck.selectedIndex = 0;
>-      gProxyFavIcon.removeAttribute("src");
>-    }
>+
>+    PageProxySetIcon(gBrowser.mCurrentBrowser.mIconURL);
>   } else if (aState == "invalid") {
>     gURLBar.removeEventListener("input", UpdatePageProxyState, false);
>-    gProxyDeck.selectedIndex = 0;
>+    PageProxyClearIcon();
>   }
> }
> 
>+function PageProxySetIcon (aURL)
>+{
>+  if (!gProxyFavIcon)
>+    return;
>+
>+  if (!aURL)
>+    PageProxyClearIcon();
>+  else if (gProxyFavIcon.getAttribute("src") != aURL) {
>+    gProxyFavIcon.setAttribute("src", aURL);
>+    gProxyDeck.selectedIndex = 1;
>+  }
>+}
>+
>+function PageProxyClearIcon ()
>+{
>+  gProxyDeck.selectedIndex = 0;
>+  gProxyFavIcon.removeAttribute("src");
>+}
>+
> function PageProxyDragGesture(aEvent)
> {
>   if (gProxyButton.getAttribute("pageproxystate") == "valid") {

	Misak Khachatryan
	Comment 55 • 16 years ago

ok, removed that bit, it will go to bug 455877.

	Dão Gottwald [:dao]
	Comment 56 • 16 years ago

Comment on attachment 362579 [details] [diff] [review]
for checkin

>+function losslessDecodeURI(aURI) {
...
>+  // Re-encode whitespace

This comment (and the corresponding code) makes only sense if the string actually has been decoded.

	Dão Gottwald [:dao]
	Comment 57 • 16 years ago

Comment on attachment 362579 [details] [diff] [review]
for checkin

>     const nsIChannel = Components.interfaces.nsIChannel;
>-    var urlStr = aRequest.QueryInterface(nsIChannel).URI.spec;
>+    var uri = aRequest.QueryInterface(Components.interfaces.nsIChannel).URI;

Looks like this makes const nsIChannel unused.

	neil@parkwaycc.co.uk
	Comment 58 • 16 years ago

(In reply to comment #57)
>(From update of attachment 362579 [details] [diff] [review])
>>     const nsIChannel = Components.interfaces.nsIChannel;
>>-    var urlStr = aRequest.QueryInterface(nsIChannel).URI.spec;
>>+    var uri = aRequest.QueryInterface(Components.interfaces.nsIChannel).URI;
>Looks like this makes const nsIChannel unused.
Good catch. But as for comment #56, it at least suggests that this covers the case where the previous decode created the whitespace in the first place ;-)

	Dão Gottwald [:dao]
	Comment 59 • 16 years ago

(In reply to comment #58)
> But as for comment #56, it at least suggests that this covers the
> case where the previous decode created the whitespace in the first place ;-)

The point is that it covers more than it should. aURI.spec alone wouldn't contain whitespace.

	neil@parkwaycc.co.uk
	Comment 60 • 16 years ago

(In reply to comment #59)
> (In reply to comment #58)
> > But as for comment #56, it at least suggests that this covers the
> > case where the previous decode created the whitespace in the first place ;-)
> The point is that it covers more than it should. aURI.spec alone wouldn't
> contain whitespace.
Exactly, which is why it wouldn't have to encode whitespace, only reencode it.

	Dão Gottwald [:dao]
	Comment 61 • 16 years ago

Except that it's less obvious why it needs to be /re/-encoded... I don't quite get why that code was moved down. The way it is in browser.js seems fine to me.

	neil@parkwaycc.co.uk
	Comment 62 • 16 years ago

(In reply to comment #61)
> I don't quite get why that code was moved down.
I just wanted to distinguish the %-escaping case from the nonprinting case.

	Dão Gottwald [:dao]
	Comment 63 • 16 years ago

I think distinguishing the decodeURI fixup from the fundamental escaping is more useful.
On the other hand, it's possible that the code in browser.js already fails to make the distinction consistently.

	Misak Khachatryan
	Comment 64 • 16 years ago

addressed comment #57, for the comment #56 IMHO it's better to file followup, when i get clear vision what to do.

	Robert Kaiser
	Comment 65 • 16 years ago

Pushed attachment 362590 [details] [diff] [review] as http://hg.mozilla.org/comm-central/rev/7f5a9e84aa8a - please mark FIXED if this is solved now, I don't want to read through the bug to figure that out as well as if attachment 339880 [details] [diff] [review] still applies (if not, please mark it obsolete).

	Robert Kaiser
	Comment 66 • 15 years ago

Comment on attachment 339254 [details] [diff] [review]
Branch patch

can't see any branch checkin here, so revoking the branch approval that has happened to remove fluke on old radars.