The default bug view has changed. See this FAQ.

Port Bug 562649 set and correctly handle userTypedValue when loading external URIs.

RESOLVED FIXED in seamonkey2.1b1

Status

SeaMonkey
General
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

(Blocks: 1 bug)

Trunk
seamonkey2.1b1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

7.81 KB, patch
neil@parkwaycc.co.uk
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
Straight port.
(Assignee)

Comment 1

7 years ago
Created attachment 464870 [details] [diff] [review]
Patch v1.0 initial port.

Port Bug 562649.

> +  isBusy: false,
> +      this.isBusy = true;
> +      this.isBusy = false
For some reason without these lines the attached test fails. Not sure what it is testing for.
Attachment #464870 - Flags: review?(neil)

Comment 2

7 years ago
Comment on attachment 464870 [details] [diff] [review]
Patch v1.0 initial port.

>         var isRelated = referrer ? true : false;
[Hmm, how did I let that slip through?]

>+  isBusy: false,
What's the point of this, given that it's only used in some test?

>-    // Set the URI now if it isn't already set, so that the user can tell which
>-    // site is loading. Only do this if user requested the load via chrome UI,
>-    // to minimise spoofing risk.
>-    if (!content.opener &&
>-        !gURLBar.value &&
>-        getWebNavigation().currentURI.spec == "about:blank")
Why was this removed?

>+            var aFromExternal;
>             var params = aReferrerURI;
>             if (!params || params instanceof Components.interfaces.nsIURI) {
>               params = {
>                 referrerURI: aReferrerURI,
>                 charset: aCharset,
>                 postData: aPostData,
>                 inBackground: aLoadInBackground,
>-                allowThirdPartyFixup: aAllowThirdPartyFixup
>+                allowThirdPartyFixup: aAllowThirdPartyFixup,
>+                fromExternal: aFromExternal
Unnecessary.

>+              if (aFromExternal)
>+                flags |= nsIWebNavigation.LOAD_FLAGS_FROM_EXTERNAL;
[LOAD_FLAGS_FROM_EXTERNAL is only really useful for loading into existing windows. All it does here is to stop you loading chrome: URLs. And we already filter them out before we ever get here. Oh well...]
Attachment #464870 - Flags: review?(neil) → review-
(Assignee)

Updated

7 years ago
Depends on: 577939
(Assignee)

Comment 3

7 years ago
Created attachment 465173 [details] [diff] [review]
Patch v1.1 Address comments.

>>         var isRelated = referrer ? true : false;
> [Hmm, how did I let that slip through?]

Oops. I'll fix it here.

>>+  isBusy: false,
> What's the point of this, given that it's only used in some test?
> 
>>-    // Set the URI now if it isn't already set, so that the user can tell which
>>-    // site is loading. Only do this if user requested the load via chrome UI,
>>-    // to minimise spoofing risk.
>>-    if (!content.opener &&
>>-        !gURLBar.value &&
>>-        getWebNavigation().currentURI.spec == "about:blank")
> Why was this removed?
Good point. Restoring.

In Bug 562649:
Gavin: what replaces the call to URLBarSetURI() that you're removing?
Dao: updateCurrentBrowser calling XULBrowserWindow.onLocationChange.

Unfortunately our version of updateCurrentBrowser doesn't do that...
But I think Misak is adding that in Bug 577939. Marking a dependency.

>>+            var aFromExternal;
>>             var params = aReferrerURI;
>>             if (!params || params instanceof Components.interfaces.nsIURI) {
>>               params = {
>>                 referrerURI: aReferrerURI,
>>                 charset: aCharset,
>>                 postData: aPostData,
>>                 inBackground: aLoadInBackground,
>>-                allowThirdPartyFixup: aAllowThirdPartyFixup
>>+                allowThirdPartyFixup: aAllowThirdPartyFixup,
>>+                fromExternal: aFromExternal
> Unnecessary.
Fixed.

>>+              if (aFromExternal)
>>+                flags |= nsIWebNavigation.LOAD_FLAGS_FROM_EXTERNAL;
> [LOAD_FLAGS_FROM_EXTERNAL is only really useful for loading into existing
> windows. All it does here is to stop you loading chrome: URLs. And we already
> filter them out before we ever get here. Oh well...]
What needs fixing here? Your comment is unclear.
Attachment #464870 - Attachment is obsolete: true
Attachment #465173 - Flags: review?(neil)

Comment 4

7 years ago
Comment on attachment 465173 [details] [diff] [review]
Patch v1.1 Address comments.

>-          try {
>-            browser.loadURIWithFlags(aURI.spec, loadflags, referrer);
>-          } catch (e) {}
You can't delete this completely, but you can move it to tabbrowser.xml

>+        var newTab = gBrowser.loadOneTab(uri, {inBackground: bgLoad,
>+                                               fromExternal: isExternal,
>+                                               referrerURI: referrer});
Actually we still want isRelated here, don't we? It was the ? true : false; that I was wondering about!!
Attachment #465173 - Flags: review?(neil) → review-
(Assignee)

Comment 5

7 years ago
>>-          try {
>>-            browser.loadURIWithFlags(aURI.spec, loadflags, referrer);
>>-          } catch (e) {}
> You can't delete this completely, but you can move it to tabbrowser.xml

Not sure what you mean here. Immediately after this I call loadOneTab() passing it isExternal which then passes it to addTab() which then calls loadURIWithFlags() with the LOAD_FLAGS_FROM_EXTERNAL flag.

>>+        var newTab = gBrowser.loadOneTab(uri, {inBackground: bgLoad,
>>+                                               fromExternal: isExternal,
>>+                                               referrerURI: referrer});
> Actually we still want isRelated here, don't we? It was the ? true : false;
> that I was wondering about!!
Hmm. So do you mean to just put it inline inside the call to:
loadOneTab(uri, {relatedToCurrent: referrer ? true : false, ...}
?

Comment 6

7 years ago
(In reply to comment #5)
>>>-          try {
>>>-            browser.loadURIWithFlags(aURI.spec, loadflags, referrer);
>>>-          } catch (e) {}
>> You can't delete this completely, but you can move it to tabbrowser.xml
>Not sure what you mean here. Immediately after this I call loadOneTab() passing
>it isExternal which then passes it to addTab() which then calls
>loadURIWithFlags() with the LOAD_FLAGS_FROM_EXTERNAL flag.
Which can throw...

>>>+        var newTab = gBrowser.loadOneTab(uri, {inBackground: bgLoad,
>>>+                                               fromExternal: isExternal,
>>>+                                               referrerURI: referrer});
>>Actually we still want isRelated here, don't we? It was the ? true : false;
>>that I was wondering about!!
>Hmm. So do you mean to just put it inline inside the call to:
>loadOneTab(uri, {relatedToCurrent: referrer ? true : false, ...}
That is an option, but there are better ways than ? true : false
(Assignee)

Updated

7 years ago
Depends on: 583317
No longer depends on: 577939
(Assignee)

Comment 7

7 years ago
Created attachment 466310 [details] [diff] [review]
Patch v1.2 fix nits.

> (In reply to comment #5)
>>>>-          try {
>>>>-            browser.loadURIWithFlags(aURI.spec, loadflags, referrer);
>>>>-          } catch (e) {}
>>> You can't delete this completely, but you can move it to tabbrowser.xml
> >Not sure what you mean here. Immediately after this I call loadOneTab() passing
> >it isExternal which then passes it to addTab() which then calls
> >loadURIWithFlags() with the LOAD_FLAGS_FROM_EXTERNAL flag.
> Which can throw...
Moved try/catch block to the tabbrowser.

>>>>+        var newTab = gBrowser.loadOneTab(uri, {inBackground: bgLoad,
>>>>+                                               fromExternal: isExternal,
>>>>+                                               referrerURI: referrer});
>>>Actually we still want isRelated here, don't we? It was the ? true : false;
>>>that I was wondering about!!
>>Hmm. So do you mean to just put it inline inside the call to:
>>loadOneTab(uri, {relatedToCurrent: referrer ? true : false, ...}
>That is an option, but there are better ways than ? true : false
As discussed over IRC, leaving this alone for the time being.
Attachment #465173 - Attachment is obsolete: true
Attachment #466310 - Flags: superreview?(neil)
Attachment #466310 - Flags: review?(neil)

Comment 8

7 years ago
Comment on attachment 466310 [details] [diff] [review]
Patch v1.2 fix nits.

>               b.userTypedValue = aURI;
> 
>+              let nsIWebNavigation = Components.interfaces.nsIWebNavigation;
>+              let flags = nsIWebNavigation.LOAD_FLAGS_NONE;
>+              if (aAllowThirdPartyFixup)
>+                flags |= nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
>+              if (aFromExternal)
>+                flags |= nsIWebNavigation.LOAD_FLAGS_FROM_EXTERNAL;
>+              try {
>+                b.loadURIWithFlags(aURI, flags, aReferrerURI, aCharset, aPostData);
>+              }
>+              catch (ex) { }
Ah yes, I forgot browser.xml also checks LOAD_FLAGS_FROM_EXTERNAL.
Attachment #466310 - Flags: superreview?(neil)
Attachment #466310 - Flags: superreview+
Attachment #466310 - Flags: review?(neil)
Attachment #466310 - Flags: review+
(Assignee)

Comment 9

7 years ago
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/afb32cda3438
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b1
(Assignee)

Updated

7 years ago
Blocks: 467867
You need to log in before you can comment on or make changes to this bug.