Philip Chee Assignee
	Description • 14 years ago

Straight port.

	Philip Chee Assignee
	Comment 1 • 14 years ago

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.

	neil@parkwaycc.co.uk
	Comment 2 • 14 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...]

	Philip Chee Assignee
	Comment 3 • 14 years ago

>>         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.

	neil@parkwaycc.co.uk
	Comment 4 • 14 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!!

	Philip Chee Assignee
	Comment 5 • 14 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, ...}
?

	neil@parkwaycc.co.uk
	Comment 6 • 14 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

	Philip Chee Assignee
	Comment 7 • 14 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...
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.

	neil@parkwaycc.co.uk
	Comment 8 • 14 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.

	Philip Chee Assignee
	Comment 9 • 14 years ago

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/afb32cda3438