Closed Bug 419656 Opened 17 years ago Closed 17 years ago

Location bar instrumentation misses unencoded urls

Categories

(Firefox :: Address Bar, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 3 beta5

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

(Whiteboard: [fixes bug 417441 and bug 415397])

Attachments

(1 file, 4 obsolete files)

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.
Flags: blocking-firefox3?
Attached patch v1 (obsolete) (deleted) — Splinter Review
r?dietrich for the backend bit (1 line) r?gavin for the autocomplete binding changes
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #305788 - Flags: review?(gavin.sharp)
Attachment #305788 - Flags: review?(dietrich)
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.
Heh, we keep missing this ...
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
Attachment #305788 - Flags: review?(dietrich) → review+
Whiteboard: [has patch][needs review gavin]
This helps fix bug 415397 because the front-end will retain the escaped version of the url that the back-end expects when deleting.
Blocks: 417441
Attached patch v2 (obsolete) (deleted) — Splinter Review
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.
Attachment #305788 - Attachment is obsolete: true
Attachment #306955 - Flags: review?(gavin.sharp)
Attachment #305788 - Flags: review?(gavin.sharp)
Whiteboard: [has patch][needs review gavin] → [has patch][needs review gavin][fixes bug 417441]
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.
Attachment #306955 - Flags: review?(gavin.sharp) → review-
Attached patch v3 (obsolete) (deleted) — Splinter Review
(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.
Attachment #306955 - Attachment is obsolete: true
Attachment #307884 - Flags: review?(gavin.sharp)
Attached patch v3.1 (obsolete) (deleted) — Splinter Review
Now with more try/catching for fallback.
Attachment #307884 - Attachment is obsolete: true
Attachment #307958 - Flags: review?(gavin.sharp)
Attachment #307884 - Flags: review?(gavin.sharp)
Blocks: 421980
This should also fix P3 blocking bug 415397.
Blocks: 415397
Whiteboard: [has patch][needs review gavin][fixes bug 417441] → [has patch][needs review gavin][fixes bug 417441 and bug 415397]
Blocks: 421315
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?
Attachment #307958 - Flags: review?(gavin.sharp) → review+
Whiteboard: [has patch][needs review gavin][fixes bug 417441 and bug 415397] → [has patch][fixes bug 417441 and bug 415397]
Attached patch v3.2 (deleted) — Splinter Review
(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);
Attachment #307958 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch][fixes bug 417441 and bug 415397] → [fixes bug 417441 and bug 415397]
Target Milestone: --- → Firefox 3 beta5
Depends on: 424251
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: