Philip Chee Assignee
	Description • 12 years ago

See http://hg.mozilla.org/mozilla-central/rev/3eab6ea25342

	Philip Chee Assignee
	Comment 1 • 12 years ago

> -      channel.owner = Services.scriptSecurityManager.getCodebasePrincipal(aURI);
> +      channel.owner = null;
This bit is from Bug 776577 (Update usage in SeaMonkey of GetCodebasePrincipal) but I needed this fix in order to get the test going.

Includes test from Bug 623155

Test passes before and after this patch so no regressions.

*** Start BrowserChrome Test Results ***
TEST-INFO | checking window state
TEST-START | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | There is a new tab.
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | onLocationChange catches only redirected URI.
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | This is abackground tab.
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | The content area is redirected. aIsSelectedTab:false
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | The URL bar shows the content URI. aIsSelectedTab:false
Error loading URL https://example.com/browser/suite/browser/test/redirect_bug623155.sjs#FG : 805a2ff4
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | There is a new tab.
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | onLocationChange catches only redirected URI.
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | This is aforeground tab.
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | The content area is redirected. aIsSelectedTab:true
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | The URL bar shows the content URI. aIsSelectedTab:true
INFO TEST-END | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | finished in 1063ms
TEST-INFO | checking window state

INFO TEST-START | Shutdown
Browser Chrome Test Summary
        Passed: 10
        Failed: 0
        Todo: 0

	Philip Chee Assignee
	Comment 2 • 12 years ago

> -      channel.owner = Services.scriptSecurityManager.getCodebasePrincipal(aURI);
> +      channel.owner = null;
This bit is from Bug 776577 (Update usage in SeaMonkey of GetCodebasePrincipal) but I needed this fix in order to get the test going.

Includes test from Bug 623155

Test passes before and after this patch so no regressions.

*** Start BrowserChrome Test Results ***
TEST-INFO | checking window state
TEST-START | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | There is a new tab.
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | onLocationChange catches only redirected URI.
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | This is abackground tab.
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | The content area is redirected. aIsSelectedTab:false
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | The URL bar shows the content URI. aIsSelectedTab:false
Error loading URL https://example.com/browser/suite/browser/test/redirect_bug623155.sjs#FG : 805a2ff4
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | There is a new tab.
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | onLocationChange catches only redirected URI.
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | This is aforeground tab.
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | The content area is redirected. aIsSelectedTab:true
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | The URL bar shows the content URI. aIsSelectedTab:true
INFO TEST-END | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | finished in 1063ms
TEST-INFO | checking window state

INFO TEST-START | Shutdown
Browser Chrome Test Summary
        Passed: 10
        Failed: 0
        Todo: 0

	neil@parkwaycc.co.uk
	Comment 3 • 12 years ago

Don't we have another case in nsBrowserStatusHandler.js?

	Philip Chee Assignee
	Comment 4 • 12 years ago

> Don't we have another case in nsBrowserStatusHandler.js?
I tried changing that as well but when I did, the test failed with:

INFO TEST-START | Shutdown
Browser Chrome Test Summary
        Passed: 8
        Failed: 2
        Todo: 0

*** End BrowserChrome Test Results ***

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | The URL bar shows the content URI. aIsSelectedTab:false - Got , expected https://www.bank1.com/#BG
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_bug623155.js | The URL bar shows the content URI. aIsSelectedTab:true - Got , expected https://www.bank1.com/#FG

	Philip Chee Assignee
	Comment 5 • 12 years ago

Grr. Forgot to qrefresh.

	neil@parkwaycc.co.uk
	Comment 6 • 12 years ago

Comment on attachment 651128 [details] [diff] [review]
Patch v1.1 with added nsBrowserStatusHandler.js.

Sadly this doesn't actually work for two reasons:

1. Typo in nsBrowserStatusHandler.js [mismatached ()s].
   & has higher precedence than || so you can remove the ( if you prefer.

2. tabbrowser.xml doesn't pass its aFlags to the main progress listener.
   This case goes via updateUrlBar rather than _callProgressListeners.
   Back in April I failed to predict that aFlags might not be zero.
   So it needs an extra fourth aFlags parameter too.

	Philip Chee Assignee
	Comment 7 • 12 years ago

> Sadly this doesn't actually work for two reasons:
> 
> 1. Typo in nsBrowserStatusHandler.js [mismatached ()s].
>    & has higher precedence than || so you can remove the ( if you prefer.
Oops. Fixed.

> 2. tabbrowser.xml doesn't pass its aFlags to the main progress listener.
>    This case goes via updateUrlBar rather than _callProgressListeners.
>    Back in April I failed to predict that aFlags might not be zero.
>    So it needs an extra fourth aFlags parameter too.
I *think* I've done this correctly.

	neil@parkwaycc.co.uk
	Comment 8 • 12 years ago

Comment on attachment 651519 [details] [diff] [review]
Patch v1.2 with added updateUrlBar.

>       if (aRequest && this.popupNotifications &&
>           (aWebProgress.isLoadingDocument ||
>-           !Components.isSuccessCode(aRequest.status)))
>+           aFlags & nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE))
[So, it turns out to be a little harder than that, in that aRequest can be null for an error page....]

>             onLocationChange: function(aWebProgress, aRequest, aLocation, aFlags) {
>+              const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;
>               if (aRequest && aWebProgress.DOMWindow == this.mBrowser.contentWindow) {
>                 this.mBrowser.mIconURL = "";
>                 this.mFeeds = [];
[Here, things are trickier... dealing with null aRequest is going to need a follow-up bug.]

>                 this.mTabBrowser.updateUrlBar(aWebProgress, aRequest, aLocation,
>-                                              null, this.mBrowser, this.mFeeds);
>+                                              null, this.mBrowser, this.mFeeds,
>+                                              aFlags);
I was hoping that you would put aFlags 4th, to match onLocationChange ;-)

>+                              tabListener.mStateFlags);
State flags are not the same as location flags! Pass 0 for now. r=me with that fixed.

	Philip Chee Assignee
	Comment 9 • 12 years ago

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

>>-           !Components.isSuccessCode(aRequest.status)))
>>+           aFlags & nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE))
> [So, it turns out to be a little harder than that, in that aRequest can be null for an error page....]
> 
>>             onLocationChange: function(aWebProgress, aRequest, aLocation, aFlags) {
>>+              const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;
>>               if (aRequest && aWebProgress.DOMWindow == this.mBrowser.contentWindow) {
>>                 this.mBrowser.mIconURL = "";
>>                 this.mFeeds = [];
> [Here, things are trickier... dealing with null aRequest is going to need a follow-up bug.]
OK.

>>                 this.mTabBrowser.updateUrlBar(aWebProgress, aRequest, aLocation,
>>-                                              null, this.mBrowser, this.mFeeds);
>>+                                              null, this.mBrowser, this.mFeeds,
>>+                                              aFlags);
> I was hoping that you would put aFlags 4th, to match onLocationChange ;-)
Fixed.

>>+                              tabListener.mStateFlags);
> State flags are not the same as location flags! Pass 0 for now. r=me with that fixed.
Fixed.

	neil@parkwaycc.co.uk
	Comment 10 • 12 years ago

(In reply to Philip Chee from comment #9)
> Pushed to comm-central:
> http://hg.mozilla.org/comm-central/rev/76ef05e53378
> >>+                              tabListener.mStateFlags);
> > State flags are not the same as location flags! Pass 0 for now. r=me with that fixed.
> Fixed.
Oops. Firstly, you passed the 0 in the wrong argument. (Partly my fault for getting you to move it.) And also for bug 782516 I think you actually need to pass LOCATION_CHANGE_SAME_DOCUMENT anyway.