Last Comment Bug 586340 - Port Bug 562649 set and correctly handle userTypedValue when loading external URIs.
: Port Bug 562649 set and correctly handle userTypedValue when loading external...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b1
Assigned To: Philip Chee
:
Mentors:
Depends on: 562649 583317
Blocks: SMtabAPI
  Show dependency treegraph
 
Reported: 2010-08-11 10:52 PDT by Philip Chee
Modified: 2010-08-24 01:27 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 initial port. (11.79 KB, patch)
2010-08-11 10:58 PDT, Philip Chee
neil: review-
Details | Diff | Splinter Review
Patch v1.1 Address comments. (7.55 KB, patch)
2010-08-12 03:03 PDT, Philip Chee
neil: review-
Details | Diff | Splinter Review
Patch v1.2 fix nits. (7.81 KB, patch)
2010-08-16 08:40 PDT, Philip Chee
neil: review+
neil: superreview+
Details | Diff | Splinter Review

Description Philip Chee 2010-08-11 10:52:30 PDT
Straight port.
Comment 1 Philip Chee 2010-08-11 10:58:37 PDT
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.
Comment 2 neil@parkwaycc.co.uk 2010-08-11 15:49:20 PDT
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...]
Comment 3 Philip Chee 2010-08-12 03:03:34 PDT
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.
Comment 4 neil@parkwaycc.co.uk 2010-08-12 03:46:14 PDT
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!!
Comment 5 Philip Chee 2010-08-13 20:44:50 PDT
>>-          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 neil@parkwaycc.co.uk 2010-08-14 06:26:40 PDT
(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
Comment 7 Philip Chee 2010-08-16 08:40:32 PDT
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.
Comment 8 neil@parkwaycc.co.uk 2010-08-23 07:29:36 PDT
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.
Comment 9 Philip Chee 2010-08-23 08:04:59 PDT
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/afb32cda3438

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