Last Comment Bug 425480 - non-ASCII characters should be decoded in the urlbar (like in FF3)
: non-ASCII characters should be decoded in the urlbar (like in FF3)
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.0a3
Assigned To: Serge Gautherie (:sgautherie)
:
:
Mentors:
Depends on: 397815 412458 452979 479212 481340 482147
Blocks: 479945 455877 479107
  Show dependency treegraph
 
Reported: 2008-03-27 12:03 PDT by Evgeniy Ivanov
Modified: 2011-07-07 11:10 PDT (History)
14 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
try1, fixes only urlbar (6.78 KB, patch)
2008-03-29 04:21 PDT, Evgeniy Ivanov
neil: superreview-
Details | Diff | Splinter Review
fixed Neil comments (3.33 KB, patch)
2008-09-03 13:55 PDT, Misak Khachatryan
no flags Details | Diff | Splinter Review
Corrected as Philip suggests, plus little nit. (5.31 KB, patch)
2008-09-04 00:36 PDT, Misak Khachatryan
neil: superreview-
Details | Diff | Splinter Review
Fixed all comments (4.88 KB, patch)
2008-09-05 04:18 PDT, Misak Khachatryan
no flags Details | Diff | Splinter Review
corrected patch, addresses comments #13 #14 (5.91 KB, patch)
2008-09-08 03:20 PDT, Misak Khachatryan
neil: superreview+
Details | Diff | Splinter Review
final patch, nits fixed (5.87 KB, patch)
2008-09-08 06:53 PDT, Misak Khachatryan
neil: review+
Details | Diff | Splinter Review
fixes ugly behavior (5.31 KB, patch)
2008-09-12 04:23 PDT, Misak Khachatryan
no flags Details | Diff | Splinter Review
final patch (5.65 KB, patch)
2008-09-12 04:40 PDT, Misak Khachatryan
neil: review+
neil: superreview+
Details | Diff | Splinter Review
Branch patch (8.22 KB, patch)
2008-09-18 07:57 PDT, Philip Chee
kairo: approval‑seamonkey1.1.12-
Details | Diff | Splinter Review
(Av4) fixed and extended (8.26 KB, patch)
2008-09-21 23:30 PDT, Serge Gautherie (:sgautherie)
neil: superreview-
Details | Diff | Splinter Review
(Av5) fixed and extended (8.27 KB, patch)
2008-09-22 17:04 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
above patch with fix for bug 452979 (6.62 KB, patch)
2009-02-05 03:12 PST, Misak Khachatryan
neil: review-
neil: superreview-
Details | Diff | Splinter Review
comments addressed (6.24 KB, patch)
2009-02-11 07:59 PST, Misak Khachatryan
neil: review-
neil: superreview-
Details | Diff | Splinter Review
fixed replace's (6.21 KB, patch)
2009-02-12 02:53 PST, Misak Khachatryan
neil: review-
neil: superreview-
Details | Diff | Splinter Review
consolidated patch (9.87 KB, patch)
2009-02-13 06:36 PST, Misak Khachatryan
no flags Details | Diff | Splinter Review
comments addressed (9.27 KB, patch)
2009-02-14 09:46 PST, Misak Khachatryan
neil: review-
Details | Diff | Splinter Review
v 2 (8.36 KB, patch)
2009-02-16 05:35 PST, Misak Khachatryan
neil: review+
Details | Diff | Splinter Review
for checkin (6.71 KB, patch)
2009-02-16 07:22 PST, Misak Khachatryan
kairo: approval‑seamonkey2.0a3+
Details | Diff | Splinter Review
fixed for checkin (6.71 KB, patch)
2009-02-16 08:48 PST, Misak Khachatryan
no flags Details | Diff | Splinter Review

Description Evgeniy Ivanov 2008-03-27 12:03:52 PDT
At this moment they are encoded. It's very inconvinient to work with such urls (for example wiki uses non-ASCII urls)
Comment 1 Evgeniy Ivanov 2008-03-27 12:12:53 PDT
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...
Comment 2 Evgeniy Ivanov 2008-03-29 04:21:09 PDT
Created attachment 312449 [details] [diff] [review]
try1, fixes only urlbar

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.
Comment 3 Evgeniy Ivanov 2008-03-29 04:24:46 PDT
Comment on attachment 312449 [details] [diff] [review]
try1, fixes only urlbar

Don't know if it is neil's mail...
Comment 4 Evgeniy Ivanov 2008-03-29 11:11:19 PDT
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.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-03-29 22:41:59 PDT
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.
Comment 6 Robert Kaiser 2008-04-01 12:00:56 PDT
Comment on attachment 312449 [details] [diff] [review]
try1, fixes only urlbar

Forwarding to Neil, additionally requesting sr.
Comment 7 neil@parkwaycc.co.uk 2008-05-23 08:03:37 PDT
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.
Comment 8 Misak Khachatryan 2008-09-03 13:55:15 PDT
Created attachment 336708 [details] [diff] [review]
fixed Neil comments

I've updated Evgeniy's patch to address Neil comments. Now decoding is called in SetPageProxyState.
Comment 9 Philip Chee 2008-09-03 17:21:07 PDT
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
Comment 10 Misak Khachatryan 2008-09-04 00:36:34 PDT
Created attachment 336802 [details] [diff] [review]
Corrected as Philip suggests, plus little nit.

Thanks Philip, now done as you suggested
Comment 11 neil@parkwaycc.co.uk 2008-09-04 08:23:38 PDT
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.
Comment 12 Misak Khachatryan 2008-09-05 04:18:14 PDT
Created attachment 337025 [details] [diff] [review]
Fixed all comments

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.
Comment 13 neil@parkwaycc.co.uk 2008-09-08 02:44:40 PDT
(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.
Comment 14 neil@parkwaycc.co.uk 2008-09-08 02:47:50 PDT
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);
Comment 15 Misak Khachatryan 2008-09-08 03:20:58 PDT
Created attachment 337414 [details] [diff] [review]
corrected patch, addresses comments #13 #14
Comment 16 neil@parkwaycc.co.uk 2008-09-08 06:40:30 PDT
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.
Comment 17 Misak Khachatryan 2008-09-08 06:53:28 PDT
Created attachment 337442 [details] [diff] [review]
final patch, nits fixed

carrying sr from previous comment
Comment 18 Serge Gautherie (:sgautherie) 2008-09-08 07:35:42 PDT
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...
Comment 19 Misak Khachatryan 2008-09-08 08:15:30 PDT
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.
Comment 20 Misak Khachatryan 2008-09-12 04:23:44 PDT
Created attachment 338290 [details] [diff] [review]
fixes ugly behavior

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.
Comment 21 Misak Khachatryan 2008-09-12 04:40:48 PDT
Created attachment 338295 [details] [diff] [review]
final patch

Neil pointed that first line can be deleted.
Comment 22 Philip Chee 2008-09-18 07:57:43 PDT
Created attachment 339254 [details] [diff] [review]
Branch patch

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
Comment 23 Misak Khachatryan 2008-09-19 01:24:42 PDT
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.
Comment 24 Philip Chee 2008-09-19 03:11:13 PDT
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?
Comment 25 Misak Khachatryan 2008-09-19 04:17:37 PDT
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.
Comment 26 Philip Chee 2008-09-19 04:26:15 PDT
Comment on attachment 339254 [details] [diff] [review]
Branch patch

Setting flags on behalf of Misak.
Comment 27 Robert Kaiser 2008-09-19 04:45:35 PDT
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.
Comment 28 Misak Khachatryan 2008-09-19 05:47:29 PDT
I've patched my Seamonkey 1.1.11 build, which cames with Fedora 9, works ok.
Comment 29 Serge Gautherie (:sgautherie) 2008-09-21 23:30:50 PDT
Created attachment 339740 [details] [diff] [review]
(Av4) fixed and extended

*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 ?
Comment 30 neil@parkwaycc.co.uk 2008-09-22 01:23:15 PDT
(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.
Comment 31 Serge Gautherie (:sgautherie) 2008-09-22 09:23:47 PDT
(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) ?
Comment 32 Serge Gautherie (:sgautherie) 2008-09-22 09:35:07 PDT
(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)
Comment 33 Dão Gottwald [:dao] 2008-09-22 09:40:44 PDT
(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.
Comment 34 Dão Gottwald [:dao] 2008-09-22 09:43:19 PDT
See also bug 415491.
Comment 35 neil@parkwaycc.co.uk 2008-09-22 16:24:49 PDT
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.
Comment 36 Serge Gautherie (:sgautherie) 2008-09-22 17:04:51 PDT
Created attachment 339880 [details] [diff] [review]
(Av5) fixed and extended

Av4, with comment 35 suggestion(s).

I tested the simplified regexp in the error/js console.
Comment 37 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-09-23 11:34:52 PDT
You will probably want the (yet unlanded) fix for bug 452979, as well.
Comment 38 Philip Chee 2008-09-23 20:27:55 PDT
> 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.
Comment 39 neil@parkwaycc.co.uk 2008-09-24 01:54:54 PDT
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.
Comment 40 Misak Khachatryan 2009-01-28 06:10:03 PST
Bug 452979 marked as fixed, this bug can proceed too.
Comment 41 Misak Khachatryan 2009-02-05 03:12:15 PST
Created attachment 360690 [details] [diff] [review]
above patch with fix for bug 452979

This patch is contain fix for bug 452979. Do we need branch patch ?
Comment 42 neil@parkwaycc.co.uk 2009-02-11 06:11:08 PST
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?
Comment 43 Misak Khachatryan 2009-02-11 07:59:17 PST
Created attachment 361773 [details] [diff] [review]
comments addressed
Comment 44 neil@parkwaycc.co.uk 2009-02-11 08:17:05 PST
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.
Comment 45 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-02-11 10:49:44 PST
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).
Comment 46 Misak Khachatryan 2009-02-12 02:53:22 PST
Created attachment 361972 [details] [diff] [review]
fixed replace's
Comment 47 neil@parkwaycc.co.uk 2009-02-12 05:53:35 PST
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.
Comment 48 neil@parkwaycc.co.uk 2009-02-12 05:54:58 PST
Oh, and you need to fix it so that it works for that "copied" code too.
Comment 49 Misak Khachatryan 2009-02-13 06:36:25 PST
Created attachment 362211 [details] [diff] [review]
consolidated patch

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.
Comment 50 neil@parkwaycc.co.uk 2009-02-13 07:26:35 PST
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?
Comment 51 Misak Khachatryan 2009-02-14 09:46:21 PST
Created attachment 362408 [details] [diff] [review]
comments addressed
Comment 52 neil@parkwaycc.co.uk 2009-02-14 16:11:44 PST
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")
Comment 53 Misak Khachatryan 2009-02-16 05:35:07 PST
Created attachment 362569 [details] [diff] [review]
v 2
Comment 54 neil@parkwaycc.co.uk 2009-02-16 06:44:43 PST
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") {
Comment 55 Misak Khachatryan 2009-02-16 07:22:48 PST
Created attachment 362579 [details] [diff] [review]
for checkin

ok, removed that bit, it will go to bug 455877.
Comment 56 Dão Gottwald [:dao] 2009-02-16 07:26:43 PST
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.
Comment 57 Dão Gottwald [:dao] 2009-02-16 07:30:21 PST
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.
Comment 58 neil@parkwaycc.co.uk 2009-02-16 07:49:08 PST
(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 ;-)
Comment 59 Dão Gottwald [:dao] 2009-02-16 07:57:43 PST
(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.
Comment 60 neil@parkwaycc.co.uk 2009-02-16 08:10:02 PST
(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.
Comment 61 Dão Gottwald [:dao] 2009-02-16 08:13:19 PST
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.
Comment 62 neil@parkwaycc.co.uk 2009-02-16 08:25:23 PST
(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.
Comment 63 Dão Gottwald [:dao] 2009-02-16 08:43:19 PST
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.
Comment 64 Misak Khachatryan 2009-02-16 08:48:29 PST
Created attachment 362590 [details] [diff] [review]
fixed for checkin

addressed comment #57, for the comment #56 IMHO it's better to file followup, when i get clear vision what to do.
Comment 65 Robert Kaiser 2009-02-17 10:55:29 PST
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).
Comment 66 Robert Kaiser 2009-12-20 07:56:28 PST
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.
Comment 67 Philip Chee 2011-07-07 11:10:37 PDT
*** Bug 562845 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.