419656 - Location bar instrumentation misses unencoded urls

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Ed Lee :Mardak]

I just realized with bug 415460 and bug 419324 about not unescaping urls until the UI.. there's a problem with the instrumentation.

We give the autocomplete unescaped urls but the feedback mechanism is expecting escaped urls because it queries on the url string.

Comment 1

[Ed Lee :Mardak]

Attachment: v1

r?dietrich for the backend bit (1 line)
r?gavin for the autocomplete binding changes

Comment 2

[Ed Lee :Mardak]

Basically, this patch gives the AutoCompleteResult an encoded url and the UI is in charge of displaying an unencoded one. So when the user selects an entry, the AutoCompleteController uses the Result's encoded url for feedback.

So we keep the invariant that URIs/specs are passed around as encoded, but are decoded for UI.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Heh, we keep missing this ...

Comment 4

[Ed Lee :Mardak]

This helps fix bug 415397 because the front-end will retain the escaped version of the url that the back-end expects when deleting.

Comment 5

[Ed Lee :Mardak]

Attachment: v2

Addresses gavin's comments from irc about unnecessary code for checking nsIURI, so I just inlined it instead. Also, there's a difference when unescaping between selecting an entry and showing it in the popup.

Also this will fix P2 blocking bug 417441 by not unescaping ascii characters when putting the value into the autocomplete textbox.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 306955 [details] [diff] [review]
v2

>diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets/autocomplete.xml

>       <property name="textValue"
>                 onget="return this.value;">
>         <setter><![CDATA[
>+          // Unescape the value safely for using it to visit the page
>+          this.value = Components.classes["@mozilla.org/intl/texttosuburi;1"].
>+            getService(Components.interfaces.nsITextToSubURI).
>+            unEscapeNonAsciiURI("UTF-8", val);

I don't think this part belongs in the generic toolkit binding. Talked to Edward on IRC about putting this in urlbarbindings.xml.

Comment 7

[Ed Lee :Mardak]

Attachment: v3

(In reply to comment #6)
> >       <property name="textValue"
> >+          this.value = Components.classes["@mozilla.org/intl/texttosuburi;1"].
> >+            getService(Components.interfaces.nsITextToSubURI).
> >+            unEscapeNonAsciiURI("UTF-8", val);
> I don't think this part belongs in the generic toolkit binding. Talked to
> Edward on IRC about putting this in urlbarbindings.xml.
So I added an extra parameter to URLBarSetURI to force it to load the URI because otherwise, I had to do this.value = "" then trigger the input event to cause userTypedValue to become empty which would then cause URLBarSetURI to actually look at the uri.

But that's not fun because it causes the urlbar to temporarily be empty and have the favicon switch to the default.

Comment 8

[Ed Lee :Mardak]

Attachment: v3.1

Now with more try/catching for fallback.

Comment 9

[Ed Lee :Mardak]

This should also fix P3 blocking bug 415397.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 307958 [details] [diff] [review]
v3.1

>Index: browser/base/content/urlbarBindings.xml

>+      <property name="textValue"
>+                onget="return this.value;">
>+        <setter>
>+          <![CDATA[
>+          // Force load the value into the urlbar to get it unescaped
>+          try {
>+            URLBarSetURI(Cc["@mozilla.org/network/io-service;1"].
>+                         getService(Ci.nsIIOService).
>+                         newURI(val, null, null), true);

Put the URI in a temporary for readability?

Comment 11

[Ed Lee :Mardak]

Attachment: v3.2

(In reply to comment #10)
> >+            URLBarSetURI(Cc["@mozilla.org/network/io-service;1"].
> >+                         getService(Ci.nsIIOService).
> >+                         newURI(val, null, null), true);
> Put the URI in a temporary for readability?
Sure can. URLBarSetURI(uri, true);

Comment 12

[Ed Lee :Mardak]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.1004; previous revision: 1.1003
done
Checking in browser/base/content/urlbarBindings.xml;
/cvsroot/mozilla/browser/base/content/urlbarBindings.xml,v  <--  urlbarBindings.xml
new revision: 1.57; previous revision: 1.56
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.47; previous revision: 1.46
done
Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.xml
new revision: 1.123; previous revision: 1.122
done