Spun from bug 307027. In the absence of a channel, or at least a request, we don't fire on*Change for security/location etc. This breaks security UI, among other things, so for the 1.8 branch we had to disable error pages in the absence of a channel.

The WPL API says that some events may be fired with a null request. I think we should make the secure browser UI function properly when given a null request. The main challenge is to figure out if there are any cases where a null request should not be treated as insecure by the secure browser UI. Note the comment in the secure browser UI code that claims that the security state should not change when given a null request.

Making the secure browser UI deal would be wonderful. Kaie, is that doable?

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp&rev=1.60&mark=90,94,514,518,549,557,561,883,507,599,634-639#627 re 599: the top level flag indicates that the request corresponds to a user request (e.g. clicking a link [possibly in a frame], or entering a url in the urlbar) not frame nesting, any gecko host is capable of checking if a load happens to be in the root window. this code or rather the result of the change for bug 307027 is hurting embedders who don't want alerts, so it seems that i'll try to post a patch (which will also hopefully improve the comments).

Kaie, any idea if we can fix this for 1.9?

I read this bug, I read bug 307027, but I don't see clear what bug you are trying to fix, so allow me to ask questions for clarification. Is there a test case? Are you saying, you want to revert the patch from bug 307027, use its test case and find a better patch?

> Are you saying, you want to revert the patch from bug 307027, use its test case > and find a better patch? Yes. We want to be able to use an error page in that situation...

(In reply to comment #6) > > Are you saying, you want to revert the patch from bug 307027, use its test case > > and find a better patch? > > Yes. We want to be able to use an error page in that situation... > Any updates on this? Running short on time? Do we need to block?

(In reply to comment #1) > The WPL API says that some events may be fired with a null request. I think we > should make the secure browser UI function properly when given a null request. > The main challenge is to figure out if there are any cases where a null request > should not be treated as insecure by the secure browser UI. Note the comment in > the secure browser UI code that claims that the security state should not change > when given a null request. That is, bug 283733 should be fixed in another way, isn't it? It seems to me that other comments are a little pointless. A null channel implies 2 scenarios. #1 nsIIOService fails to create a channel, because it can't find a proper protocol handler. #2 docShell does't pass a new channel to onLocationChange, because it is scroll-only anchor. i.e. <a href="#foo"> foo </a> Most easy way to fix bug 283733 would be, to call onLocationChange with a new special flag JUST_SCROLLING. But this is simply impossible, as nsIWebProgressListener is a frozen interface.

You could add something to nsIWebProgressListener2, if you wanted.

(In reply to comment #9) > You could add something to nsIWebProgressListener2, if you wanted. Aha. Then I have a question. Does XPCOM suppots method overloading? From: > void onLocationChang(in nsIWebProgress aWebProgress, > in nsIRequest aRequest, > in nsIURI aLocation); To: > void onLocationChange(in nsIWebProgress aWebProgress, > in nsIRequest aRequest, > in nsIURI aLocation, > in boolean aChangeIsPosition); Or a new name must be "onLocationChangeEx" or something? The goal of bug 283733 is that nsDocShell hands out a local variable |wasAnchor| to securityUI, in some way or other way. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/docshell/base/nsDocShell.cpp&rev=1.914&mark=7053,7055#7053 So, I can go there by another root, as long as I don't break the result of bug 258048. bug 258048 insists that "When URL string in the address bar changed, the color should change accordingly." That's the only reason why onLocationChange dispatches onSecurityChange. In other words, nsDocShell *is able to* trigger to make securityUI dispatch onSecurityChange without onLocationChange, for securityUI is a member variable of nsDocShell. All I have to do in this root is to add a new method forceUpdateUIperURIChange(...) to nsISecureBrowserUI, and make nsDocShell call it along with onLocationChange. Then docshell may suppress onSecurityChange when |wasAnchor| is true. The third root is to expose |wasAnchor| to the XPCOM world. For nsDocShell is the first pram of onLocationChange.

> Does XPCOM suppots method overloading? In brief, no. It's not a matter of "XPCOM"; JS doesn't support it.

Some languages do, some don't -- OMG IDL does not support overloading either (at least AFAIK, and not in Mozilla's implementation that parses IDL to generate type libraries, etc.), and it is the interface description language for XPCOM. So no overloading. /be

Thank you both for explanations. I understand. Then I approached it without any XPCOM change. I'm sure this should work as everyone expects, but it is getting a little complicated. Boris Zbarsky, will you review this patch?

Hmm. Would it work to just keep track of the location in security UI and not update the security state if the channel is null and the new location is the same as the old one, up to URI ref? I'm really not all that happy adding so much complexity for something like this...

I'm not too sure what you worry about, except complexity. (In reply to comment #14) > if the channel is null and the new location > is the same as the old one, up to URI ref? I'd like security UI not to mind whether the channel is null or non-null. I am about to back out attachment 175629 [details] [diff] [review], for the assumption in that comment conflicts with this bug. > + // If the location change does not have a corresponding request, then we > + // assume that it does not impact the security state. Other part of the patch is follow-up for bug 283733, as it will certainly regress.

Another approach. Maybe simpler than attachment 375463 [details] [diff] [review]. My only concern is that probably security UI would be the only consumer of |wasAnchor|. I happen to know this property will be updated just before OnLocationChange, but other users have no idea of the update frequency/timing, unless they read the source in detail.

I'm sorry. I overlooked a lot of cases where |wasAnchor| can be wrong.

> I'm not too sure what you worry about, except complexity. Complexity. Period. Docshell is already too complex. We should be removing code from it, not adding more. > I'd like security UI not to mind whether the channel is null or non-null That's fine. Then it should stop assuming things about OnLocationChange which are not true. And we should consider just adding another notification for "page changed" if that's feasible.... But I don't like directly calling stuff on the security UI in the docshell very much: that's making assumptions about the implementation which don't make me happy. I like adding the member and API as your latest patch does even less. It seems to me that the right approach is something like the first patch, but with an nsIWebProgressListener2 notification that indicates the page, not just the URI, has changed.

Comment on attachment 375594 [details] [diff] [review] Patch 2.1 Per comments.

nsIWebProgressListener2 version, addressing comment #18 (and comment #8). Note for myself: The goal of this bug is to back out the patch by bug 307027. This patch is actually a patch for bug 283733.

|aCloneSHChildren| in OnNewURI(...), which is introduced by bug 629559, seems to me doing something similar to this patch. In other words, PRBool(aFlags & LOCATION_NO_DOCUMENT_LOADED) and aCloneSHChildren has always same value. If this assumption is correct, the patch can be a little smaller. But I'm not sure they describe the same logic. mainly due to its name.

Given the comments in this patch, I think those are equivalent, yes.

Simple OnNewURI(...).

Comment on attachment 526028 [details] [diff] [review] Patch 3.3 (In reply to comment #23) > Given the comments in this patch, I think those are equivalent, yes. Thanks for the clarification. Will you review it then?

Comment on attachment 526028 [details] [diff] [review] Patch 3.3 smaug, let me know if you really need me to review this, ok?

browser-chrome-mochitest for bug 283733 and bug 307027.

I'll try to review this today or tomorrow.

Comment on attachment 526028 [details] [diff] [review] Patch 3.3 >+ /** >+ * Flags for onLocationChange2 >+ * >+ * LOCATION_NO_DOCUMENT_LOADED >+ * This flag is on when |aWebProgress| did not load a new document. >+ * For example, >+ * 1. The user clicks that anchor in a document which is linking to >+ * [1.a.] the document itself. >+ * [1.b.] a location only <#hash> portion changed. >+ * 2. The user inputs [1.b.]'s URI in the location bar. >+ * 3. Session history navigation. >+ * [3.a.] history.pushState(...), history.replaceState(...) or >+ * history.popState(...) is called and |aWebProgress| wants to >+ * update the UI. >+ * [3.b.] Back/Forward/Go to URI of [1.a.], [1.b.] or [3.a.]. >+ * [3.c.] After session history is cleared. >+ * >+ * LOCATION_NORMAL >+ * None of the above. >+ */ >+ >+ const unsigned long LOCATION_NO_DOCUMENT_LOADED = 0x00000001; >+ const unsigned long LOCATION_NORMAL = 0x00000000; Looking good but could you change these flags. LOCATION_NORMAL should have some bit set, IMO, so that flag & LOCATION_NORMAL works. const unsigned long LOCATION_NORMAL = 0x00000001; const unsigned long LOCATION_NO_DOCUMENT_LOADED = 0x00000002; > PRBool SetCurrentURI(nsIURI *aURI, nsIRequest *aRequest, >- PRBool aFireOnLocationChange); >+ PRBool aFireOnLocationChange, >+ PRUint32 aLocationFlags = >+ nsIWebProgressListener2::LOCATION_NORMAL); Could you actually not set the default value for aLocationFlags. That way whoever uses this version of SetCurrentURI needs to really think about the flag.

(In reply to comment #29) Thank you for taking the time. > LOCATION_NORMAL should have some bit set, IMO, so that > flag & LOCATION_NORMAL works. > > const unsigned long LOCATION_NORMAL = 0x00000001; > const unsigned long LOCATION_NO_DOCUMENT_LOADED = 0x00000002; That's OK, but is it useful? Now, this interface has only one abnormal flag, LOCATION_NO_DOCUMENT_LOADED. By "_NORMAL", I mean "we are actually loading a document (for security UI notification)". In the future, on the other hand, almost certainly we need new flags, say, LOCATION_ERROR_PAGE, LOCATION_REDIRECTED, LOCATION_HASHCHANGE, LOCATION_SH_PUSH, LOCATION_SH_DUMMY, LOCATION_TABSWITCH or so. In such a stage, the word "normal" will be something vague, without a solid definition. So I'd rather like nobody to write such an expression like if (flag & LOCATION_NORMAL) until we share the meaning of "Normal onLocationChange(...)". For the time being, if (!(flag & LOCATION_NO_DOCUMENT_LOADED)) should work as expected, and it will, even after various flags are added to the interface. > Could you actually not set the default value for aLocationFlags. That way > whoever > uses this version of SetCurrentURI needs to really think about the flag. I see.

Comments addressed.

Well, having a const for LOCATION_NORMAL, yet in such way that flags & LOCATION_NORMAL is always false is quite strange.

Comment on attachment 527749 [details] [diff] [review] Patch 3.4 >+ const unsigned long LOCATION_NO_DOCUMENT_LOADED = 0x00000002; >+ const unsigned long LOCATION_NORMAL = 0x00000001; Please put LOCATION_NORMAL before LOCATION_NO_DOCUMENT_LOADED >- listener->OnLocationChange(aWebProgress, aRequest, aUri); >+ nsCOMPtr<nsIWebProgressListener2> >+ listener2(do_QueryReferent(info->mWeakListener)); >+ >+ if (listener2) { >+ listener2->OnLocationChange2(aWebProgress, aRequest, aUri, aFlags); >+ } >+ else { } else { >@@ -5860,17 +5863,18 @@ nsDocShell::OnStateChange(nsIWebProgress > } > > // This is a document.write(). Get the made-up url > // from the channel and store it in session history. > // Pass false for aCloneChildren, since we're creating > // a new DOM here. > rv = AddToSessionHistory(uri, wcwgChannel, nsnull, PR_FALSE, > getter_AddRefs(mLSHE)); >- SetCurrentURI(uri, aRequest, PR_TRUE); >+ SetCurrentURI(uri, aRequest, PR_TRUE, >+ nsIWebProgressListener2::LOCATION_NORMAL); This one is interesting case, but I guess it is safer to use LOCATION_NORMAL. > // Pass true for aCloneSHChildren, since we're not > // changing documents here, so all of our subframes are > // still relevant to the new session history entry. >+ // And we are going to suppress security check. > OnNewURI(aURI, nsnull, owner, mLoadType, PR_TRUE, PR_TRUE, PR_TRUE); The comment is not clear. What security check? > // We need to call FireOnLocationChange so that the browser's address bar > // gets updated and the back button is enabled, but we only need to > // explicitly call FireOnLocationChange if we're not calling SetCurrentURI, > // since SetCurrentURI will call FireOnLocationChange for us. >+ // >+ // However, we are not going to touch the security UI. I would expect some explanation that LOCATION_NO_DOCUMENT_LOADED is used so that security UI isn't updated. LOCATION_NO_DOCUMENT_LOADED as such has nothing to do with security UI, it is just that security UI happens to use it.

(In reply to comment #33) I hope these comments are descriptive enough. > >@@ -5860,17 +5863,18 @@ nsDocShell::OnStateChange(nsIWebProgress > > } > > > > // This is a document.write(). Get the made-up url > > // from the channel and store it in session history. > > // Pass false for aCloneChildren, since we're creating > > // a new DOM here. > > rv = AddToSessionHistory(uri, wcwgChannel, nsnull, PR_FALSE, > > getter_AddRefs(mLSHE)); > >- SetCurrentURI(uri, aRequest, PR_TRUE); > >+ SetCurrentURI(uri, aRequest, PR_TRUE, > >+ nsIWebProgressListener2::LOCATION_NORMAL); > This one is interesting case, but I guess it is safer to use LOCATION_NORMAL. And again aCloneChildren seems corresponding it, although they are different function's params.

Comment on attachment 527882 [details] [diff] [review] Patch 3.5 Thanks. Then I need super-review.

Comment on attachment 527882 [details] [diff] [review] Patch 3.5 nsDownload::OnLocationChange2 should probably just call nsDownload::OnLocationChange, right? I'd prefer that the two flags be called LOCATION_CHANGE_NORMAL and LOCATION_CHANGE_SAME_DOCUMENT. sr=me with those two changes. I'm sorry for the lag here. :(

(In reply to comment #36) > nsDownload::OnLocationChange2 should probably just call > nsDownload::OnLocationChange, right? Right, but nsDownload::OnLocationChange is something like >NS_IMETHODIMP >nsDownload::OnLocationChange(nsIWebProgress *aWebProgress, > nsIRequest *aRequest, nsIURI *aLocation) >{ > return NS_OK; >} ... > I'd prefer that the two flags be called LOCATION_CHANGE_NORMAL and > LOCATION_CHANGE_SAME_DOCUMENT. The problem is, the prefix for onStateChange(...) flags is STATE_* or STATE_IS_*, not STATE_CHANGE_*. In my opinion, somebody should merge nsIProgressListener and nsIProgressListener2 in the (near) future, and hopefully with a little more consistent naming rule. I like _SAME_DOCUMENT btw. Anyway, I addressed your comments.

> The problem is, the prefix for onStateChange(...) flags is STATE_* or > STATE_IS_*, Yes, because those describe _which_ states are changing. Your flags are describing _how_ the location is changing. Totally different meaning. ;) Is that last patch ready to land, then? If so, could you say what commit message and From field you want on it (or just upload an exported diff with that information in it already)?

(In reply to comment #38) > Yes, because those describe _which_ states are changing. > > Your flags are describing _how_ the location is changing. > > Totally different meaning. ;) I see. > Is that last patch ready to land, then? Yes, I hope so.

fwiw, the "checkin needed" marker is a keyword, not a status whiteboard annotation.... I'll get this landed.

(In reply to comment #40) > fwiw, the "checkin needed" marker is a keyword, not a status whiteboard > annotation.... Oh... Thank you very much. I'd like to do something for bug 659910.

Looking at http://mxr.mozilla.org/mozilla-central/ident?i=onLocationChange&filter=browser%2Fbase%2Fcontent%2Fbrowser.js I'd say +++ a/browser/base/content/browser.js @@ -4366,6 +4366,10 @@ } }, onLocationChange2: function (aWebProgress, aRequest, aLocationURI, aFlags) { - onLocationChange(aWebProgress, aRequest, aLocationURI); + this.onLocationChange(aWebProgress, aRequest, aLocationURI); }, Correct?

(In reply to comment #44) > Correct? Yes, absolutely. And the more serious problem is I overlooked tabbrowser.xml for some reason...

What a sad-making API :(

If I understand right, thanks to nsBrowserStatusFilter in toolkit, this method (and maybe onProgressChange64(...), too) is invisible from front-end. That's why it hasn't hit my JS console for nearly 2 months so far. So, in practice, anything, including an empty function, will do, because nobody calls it. But, anyway it should look more reasonable than it is.

Presumably this is why my console has lots of Error: [Exception... "'JavaScript component does not have a method named: "onLocationChange2"' when calling method: [nsIWebProgressListener2::onLocationChange2]" nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)" location: "<unknown>" data: no] Error: [Exception... "'JavaScript component does not have a method named: "onLocationChange2"' when calling method: [nsIWebProgressListener2::onLocationChange2]" nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)" location: "JS frame :: chrome://browser/content/browser.js :: toOpenWindowByType :: line 8775" data: no] Source File: chrome://browser/content/browser.js Line: 8775 Error: [Exception... "'JavaScript component does not have a method named: "onLocationChange2"' when calling method: [nsIWebProgressListener2::onLocationChange2]" nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)" location: "JS frame :: chrome://global/content/printUtils.js :: <TOP_LEVEL> :: line 113" data: no] Source File: chrome://global/content/printUtils.js Line: 113

(In reply to comment #48) > Presumably this is why my console has lots of > > Error: [Exception... "'JavaScript component does not have a method named: > "onLocationChange2"' when calling method: > [nsIWebProgressListener2::onLocationChange2]" nsresult: "0x80570030 > (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)" location: "<unknown>" data: > no] Yes, but those messages are likely from extensions implementing nsIWebProgressListener2 and unaware of this change. One was NoScript, and that's the reason why I first looked into this bug: if you've got, you should grab latest dev build from http://noscript.net/getit#devel

addressing comment 44.

Comment on attachment 538684 [details] [diff] [review] Followup v1 onLocationChange2(..) in /browser/ 8-lines diff of tabbrowser.xml >@@ -658,16 +658,21 @@ > [aWebProgress, aRequest, aState]); > }, > > onRefreshAttempted: function (aWebProgress, aURI, aDelay, aSameURI) { > return this._callProgressListeners("onRefreshAttempted", > [aWebProgress, aURI, aDelay, aSameURI]); > }, > >+ onLocationChange2: function (aWebProgress, aRequest, aLocation, >+ aFlags) { >+ this.onLocationChange(aWebProgress, aRequest, aLocation); >+ }, >+ > QueryInterface: function (aIID) { > if (aIID.equals(Components.interfaces.nsIWebProgressListener) || > aIID.equals(Components.interfaces.nsIWebProgressListener2) || > aIID.equals(Components.interfaces.nsISupportsWeakReference) || > aIID.equals(Components.interfaces.nsISupports)) > return this; > throw Components.results.NS_NOINTERFACE; > }

Comment on attachment 538684 [details] [diff] [review] Followup v1 onLocationChange2(..) in /browser/ How is providing onLocationChange2 and having every consumer redirect it to onLocationChange better than adding the flags argument to onLocationChange?

(In reply to comment #53) > How is providing onLocationChange2 and having every consumer redirect it to > onLocationChange better than adding the flags argument to onLocationChange? Hmm, if it can be such a radical interface, I'm happy to write a patch to merge nsIWebProgressListener and nsIWebProgressListene2. But it depends on the owner, and probably this bug can't cover that big change. Another bug should be filed. By the way, I can't browse security bugs. In that sense, I'm not a proper assignee of this bug.

Changing the signature of onLocationChange doesn't necessarily mean merging nsIWebProgressListener and nsIWebProgressListene2.

(In reply to comment #55) I'm not too sure what you mean. nsIWebProgressListener was a frozen interface. If we can modify nsIWebProgressListener that easily, there's few reasons to keep supporting nsIWebProgressListener2, if any.

It's not frozen any longer. Yes, merging the two might be a good thing, but as I said, that's independent from touching onLocationChange.

> and having every consumer redirect Only listeners that implement nsIWebProgressListener2. We could change the other interface, but it would break all extensions that implement onLocationChange but not nsIWebProgressListener2 right now (whereas the patch that was checked in breaks those that implement nsIWebProgressListener2). Checking what these sets look like is worthwhile.... Requesting tracking for Fx7, since this has already landed code that has extension compat implications.

(In reply to comment #58) > We could change the other interface, but it would break all extensions that > implement onLocationChange but not nsIWebProgressListener2 right now How exactly would it break them? Isn't the number of arguments spelled out in the JS function header irrelevant?

Hmm. I guess as long as they don't make onLocationChange calls themselves they'll be ok; we'll need to update all existing onLocationChange callers, but that might not be too bad.

In SeaMonkey Bug 663345 we made onLocationChange call onLocationChange2. See Attachment 538552 [details] [diff]

Seems like we should back this out before others start doing the same.

Yeah, it sounds like we should just change onLocationChange instead.... I won't have a tree on hand until tomorrow, sadly.

Do remember to drop the Thunderbird people a line. A quick grep in comm-central/mailnews indicates a bunch of .cpp code references nsIWebProgressListener

backed out: http://hg.mozilla.org/mozilla-central/rev/5e3cf50377aa

unsetting tracking 7 nomination since this was backed out and I don't think we're any longer worried about the extension compat issue. Please re-nominate if that is an incorrect assumption.

Addressing comment #53

Well, I grepped 3 times.

Comment on attachment 553325 [details] [diff] [review] Patch v4.1.1 part1 (idl, docshell, secureUI) The difference between this patch and previous one is the interface: from nsIWebProgressListener2 to nsIWebProgressListener, along with the discussion above.

Comment on attachment 553327 [details] [diff] [review] Patch v4.1.1 part2 (Rest of onLocationChange(...)s) This patch is about all of onLocationChange()s, I think, in the tree. But this is not enough for mail, editor, chatzilla, etc, as is pointed out comment #64.

I'm not too sure about chatzilla and venkman though.

> I'm not too sure about chatzilla and venkman though. Please file separate bugs in: https://bugzilla.mozilla.org/query.cgi?format=advanced&product=Other%20Applications&component=ChatZilla And in: https://bugzilla.mozilla.org/query.cgi?format=advanced&product=Other%20Applications&component=Venkman%20JS%20Debugger Thank you.

Comment on attachment 553325 [details] [diff] [review] Patch v4.1.1 part1 (idl, docshell, secureUI) >@@ -175,16 +175,39 @@ interface nsIWebProgressListener : nsISu > * or stop of activity for restoring a previously-rendered presentation. > * As such, there is no actual network activity associated with this > * request, and any modifications made to the document or presentation > * when it was originally loaded will still be present. > */ > const unsigned long STATE_RESTORING = 0x01000000; > > /** >+ * Flags for onLocationChange >+ * >+ * LOCATION_CHANGE_NORMAL >+ * Nothing special. >+ * >+ * LOCATION_CHANGE_SAME_DOCUMENT >+ * This flag is on when |aWebProgress| did not load a new document. >+ * For example, >+ * 1. The user clicks that anchor in a document which is linking to >+ * [1.a.] the document itself. >+ * [1.b.] a location only <#hash> portion changed. >+ * 2. The user inputs [1.b.]'s URI in the location bar. >+ * 3. Session history navigation. >+ * [3.a.] history.pushState(...), history.replaceState(...) or >+ * history.popState(...) is called and |aWebProgress| wants to >+ * update the UI. >+ * [3.b.] Back/Forward/Go to URI of [1.a.], [1.b.] or [3.a.]. >+ * [3.c.] After session history is cleared. >+ */ >+ const unsigned long LOCATION_CHANGE_NORMAL = 0x00000001; >+ const unsigned long LOCATION_CHANGE_SAME_DOCUMENT = 0x00000002; >+ >+ /** > * State Security Flags > * > * These flags describe the security state reported by a call to the > * onSecurityChange method. These flags are mutually exclusive. > * > * STATE_IS_INSECURE > * This flag indicates that the data corresponding to the request > * was received over an insecure channel. >@@ -311,20 +334,24 @@ interface nsIWebProgressListener : nsISu > * this new page here. > * > * @param aWebProgress > * The nsIWebProgress instance that fired the notification. > * @param aRequest > * The associated nsIRequest. This may be null in some cases. > * @param aLocation > * The URI of the location that is being loaded. >+ * @param aFlags >+ * This is a value which explains the situation or the reason why >+ * the location has changed. > */ > void onLocationChange(in nsIWebProgress aWebProgress, > in nsIRequest aRequest, >- in nsIURI aLocation); >+ in nsIURI aLocation, >+ in unsigned long aFlags); Actually, now that we're changing this method and interface, would it make sense to have [optional] in unsigned long aFlags and not have LOCATION_CHANGE_NORMAL at all. LOCATION_CHANGE_SAME_DOCUMENT could be then 0x00000001; Boris, what do you think?

Torisugari, would the patch be simpler if the last parameter was optional?

(In reply to Olli Pettay [:smaug] from comment #78) > Torisugari, would the patch be simpler if the last parameter was optional? Probably, yes, it would. However, my concern is, most of 3rd party applications won't call onLocationChange, but will be called. Is it possible to set a default value (0, I assume) to the optional argument? If the JS value is |undefined|, those applications can be more complicated than I expect. For example, in case I were to implement nsIWebProgressListener in JavaScript, the callback is something like: >function onLocationChange(aWebProgress, aRequest, aURI, aFlag) { > if (aFlags != undefined) { > if (aFlag & LOCATION_CHANGE_FOO) { > ... > } > ... > } > ... >} in the future, from now on? I wonder like what other scriptable XPCOM callbacks which contain "[optional]" arguments are.

Default value for optional numeric parameters is 0 (at least IIRC)

My last concern is, in front end, there are actually some JavaScript onLocationChange call without XPConnect, i.e. some onLocationChange()s may miss the chance to set the default value, and certainly it will affect to 3rd-party applications' listeners. But [optional]'s merit will win, as long as the upstream side makes sure to set |aFlags| always. Though apparently no JavaScript onLocationChange wants to use this arg, for the time being...

Comment on attachment 553673 [details] [diff] [review] Test v3 >--- a/docshell/test/chrome/window.template.txt >+++ b/docshell/test/chrome/window.template.txt >@@ -1,12 +1,12 @@ > <?xml version="1.0"?> > <?xml-stylesheet href="chrome://global/skin" type="text/css"?> > >-<window id="303267Test" >+<window id="{BUGNUMBER}Test" > xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" > width="600" > height="600" > onload="setTimeout(nextTest,0);" > title="bug {BUGNUMBER} test"> > > <script type="application/javascript" > src="docshell_helpers.js"> This is unrelated, but looks ok.

Comment on attachment 553327 [details] [diff] [review] Patch v4.1.1 part2 (Rest of onLocationChange(...)s) +++ b/browser/base/content/tabbrowser.xml This needs to be changed to pass aFlags on to the listeners registered on the tabbrowser, right? This could perhaps use a test. What about the comm-central changes and whatnot?

Comment on attachment 557113 [details] [diff] [review] Patch v4.2 part1 (idl, docshell, secureUI) >+ * 1. The user clicks that anchor in a document which is linking to >+ * [1.a.] the document itself. >+ * [1.b.] a location only <#hash> portion changed. 1a and 1b both have to be true, right? I'd phrase this comment as follows: * For example, the location change is due to an anchor scroll or a * pushState/popState/replaceState. and get rid of the whole list thing. sr=me with that

(In reply to Boris Zbarsky (:bz) from comment #83) > Comment on attachment 553327 [details] [diff] [review] [diff] [details] [review] > Patch v4.1.1 part2 (Rest of onLocationChange(...)s) > > +++ b/browser/base/content/tabbrowser.xml > > This needs to be changed to pass aFlags on to the listeners registered on the > tabbrowser, right? That is always confusing me, and, well, I think I should not touch tabbrowser's listener. Firefox's <tabbrowser/> surely dispatches "onLocationChange", but this "onLocationChange" is not nsIWebProgressListener's onLocationChange. nsIWebProgressListner (without this patch): onLocationChange(aWebProgressListner, aRequest, aURI); Firefox's <tabbrowser/>: onLocationChange(aBrowserElement, aWebProgressListner, aRequest, aURI); So basically one can't QI tabbrowser's progress listeners. There's 3 types of methods called "onLocationChange" at frontend (browser.js), and the last one is implemented in the zoom in/out script, by the way. On the other hand, comm-central's <tabbrowser/> always dispatches QI-able onLocationChange. I should add |aFlags| arg to each of them, I think. (In reply to Boris Zbarsky (:bz) from comment #84) > Comment on attachment 557113 [details] [diff] [review] [diff] [details] [review] > Patch v4.2 part1 (idl, docshell, secureUI) > > >+ * 1. The user clicks that anchor in a document which is linking to > >+ * [1.a.] the document itself. > >+ * [1.b.] a location only <#hash> portion changed. > > 1a and 1b both have to be true, right? I'd phrase this comment as follows: > > * For example, the location change is due to an anchor scroll or a > * pushState/popState/replaceState. > > and get rid of the whole list thing. Right. I see.

<tabbrowser> has addProgressListener as well as addTabsProgressListener. The latter is for listening to progress changes in any browser; therefore the respective browser is passed to the listeners.

Specifically, <method name="_callProgressListeners"> does this: 411 this.mProgressListeners.forEach(function (p) { 412 if (aMethod in p) { 413 try { 414 if (!p[aMethod].apply(p, aArguments)) which is for normal progress listeners. Then it unshifts the browser into aArguments and does the same for mTabsProgressListeners. But that first one really does call into nsIWebProgressListeners and should pass the new argument. The second one can safely do so too.

Aha. Now I get it. Because _callProgressListeners may call both addProgressListener's listenenrs and addTabsProgressListener's listeners, I should always set |aFlags| arg. Then, a new problem is value of its param, though I will set 0.

> Then, a new problem is value of its param, Whatever we got passed to start with, no?

Yes, it makes no big difference for the time being.

Comment on attachment 568768 [details] [diff] [review] Patch v4.3 (for comm-central) > this.mTabBrowser._callProgressListeners(this.mBrowser, "onLocationChange", >- [aWebProgress, aRequest, aLocation], >+ [aWebProgress, aRequest, aLocation, aFlags], >- element.onLocationChange(aWebProgress, aRequest, aLocation); >+ element.onLocationChange(aWebProgress, aRequest, aLocation, 0); I take it these are the only "important" changes, right? The rest of the patch looks like it is just ensuring the correct number of parameters. >- onSecurityChange: function( aWebProgress, aRequest, aState, aDownload ) { >+ onSecurityChange: function( aWebProgress, aRequest, aState ) { Heh, I must have copied that from progressDialog.js - thanks for fixing it!

(In reply to neil@parkwaycc.co.uk from comment #93) > Comment on attachment 568768 [details] [diff] [review] [diff] [details] [review] > Patch v4.3 (for comm-central) > > > this.mTabBrowser._callProgressListeners(this.mBrowser, "onLocationChange", > >- [aWebProgress, aRequest, aLocation], > >+ [aWebProgress, aRequest, aLocation, aFlags], > > >- element.onLocationChange(aWebProgress, aRequest, aLocation); > >+ element.onLocationChange(aWebProgress, aRequest, aLocation, 0); > I take it these are the only "important" changes, right? The rest of the > patch looks like it is just ensuring the correct number of parameters. Yes. I should request sr, too?

Comment on attachment 568735 [details] [diff] [review] Patch v4.3 (all in one except for comm-central's) sr=me; sorry for the lag....

up to date, for check-in. (In reply to Boris Zbarsky (:bz) from comment #95) > sr=me; sorry for the lag.... I think it's not a big problem for a 6-year-old bug.

up to date, per bug 686347. (specialTabs.js and uriListener.js)

Comment on attachment 572798 [details] [diff] [review] Patch v4.3.1 (for comm-central) I'll see if this fixes the build bustage that is bug 701679

Comment on attachment 572798 [details] [diff] [review] Patch v4.3.1 (for comm-central) I'll land this, along with a separate editor bustage fix in a bit (after I verify that my build works)