Last Comment Bug 311007 - state change events not fired without a channel
: state change events not fired without a channel
Status: RESOLVED FIXED
[psm-cert-errors]
: dev-doc-needed
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: P5 normal (vote)
: mozilla11
Assigned To: O. Atsushi (Torisugari)
:
:
Mentors:
Depends on: 768915
Blocks: 656343 312680 663345
  Show dependency treegraph
 
Reported: 2005-10-03 21:18 PDT by Mike Connor [:mconnor]
Modified: 2012-06-27 08:40 PDT (History)
25 users (show)
mbeltzner: wanted‑next+
dao+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (15.99 KB, patch)
2009-05-02 05:35 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Splinter Review
Patch 2 (5.93 KB, patch)
2009-05-03 21:38 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Splinter Review
Patch 2.1 (11.00 KB, patch)
2009-05-04 04:59 PDT, O. Atsushi (Torisugari)
bzbarsky: review-
Details | Diff | Splinter Review
Patch 3 (34.30 KB, patch)
2011-04-11 13:02 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Splinter Review
Patch 3.1 (27.42 KB, patch)
2011-04-12 11:46 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Splinter Review
Patch 3.2 (27.33 KB, patch)
2011-04-14 06:12 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Splinter Review
Patch 3.3 (25.69 KB, patch)
2011-04-14 09:46 PDT, O. Atsushi (Torisugari)
bugs: review-
Details | Diff | Splinter Review
Test (4.49 KB, patch)
2011-04-16 18:17 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Splinter Review
Patch 3.4 (28.50 KB, patch)
2011-04-22 03:14 PDT, O. Atsushi (Torisugari)
bugs: review+
Details | Diff | Splinter Review
Patch 3.5 (28.99 KB, patch)
2011-04-22 16:48 PDT, O. Atsushi (Torisugari)
bugs: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Patch 3.6 (29.20 KB, patch)
2011-05-27 05:56 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Splinter Review
Patch 3.6.1 (18.17 KB, patch)
2011-06-03 21:44 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Splinter Review
Test v2 (9.24 KB, patch)
2011-06-11 03:16 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Splinter Review
Followup v1 onLocationChange2(..) in /browser/ (1.41 KB, patch)
2011-06-11 04:44 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Splinter Review
Patch v4 part1 (idl, docshell, secureUI) (20.05 KB, patch)
2011-08-15 02:22 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Splinter Review
Patch v4 part2 (Rest of onLocationChange(...)s) (43.24 KB, patch)
2011-08-15 02:26 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Splinter Review
Patch v4.1 part1 (idl, docshell, secureUI) (45.09 KB, patch)
2011-08-15 17:46 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Splinter Review
Patch v4.1.1 part1 (idl, docshell, secureUI) (20.07 KB, patch)
2011-08-15 17:48 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Splinter Review
Patch v4.1.1 part2 (Rest of onLocationChange(...)s) (45.09 KB, patch)
2011-08-15 17:51 PDT, O. Atsushi (Torisugari)
bugs: review+
bzbarsky: superreview-
Details | Diff | Splinter Review
Test v3 (8.71 KB, patch)
2011-08-16 20:50 PDT, O. Atsushi (Torisugari)
bugs: review+
Details | Diff | Splinter Review
for comm-central (26.64 KB, patch)
2011-08-21 02:47 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Splinter Review
Patch v4.2 part1 (idl, docshell, secureUI) (19.11 KB, patch)
2011-08-31 03:38 PDT, O. Atsushi (Torisugari)
bugs: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Patch v4.3 (all in one except for comm-central's) (75.55 KB, patch)
2011-10-21 12:18 PDT, O. Atsushi (Torisugari)
bzbarsky: superreview+
Details | Diff | Splinter Review
Patch v4.3 (for comm-central) (26.42 KB, patch)
2011-10-21 14:04 PDT, O. Atsushi (Torisugari)
neil: review+
Details | Diff | Splinter Review
Patch v4.3.1 (for mozilla-central) (75.68 KB, patch)
2011-11-08 05:43 PST, O. Atsushi (Torisugari)
no flags Details | Diff | Splinter Review
Patch v4.3.1 (for comm-central) (27.19 KB, patch)
2011-11-08 06:19 PST, O. Atsushi (Torisugari)
mozilla: review+
Details | Diff | Splinter Review

Description Mike Connor [:mconnor] 2005-10-03 21:18:18 PDT
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.
Comment 1 Darin Fisher 2005-10-03 21:38:55 PDT
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.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2005-10-06 13:28:22 PDT
Making the secure browser UI deal would be wonderful.  Kaie, is that doable?
Comment 3 Sudarsana Nagineni 2007-01-31 11:39:16 PST
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).
Comment 4 Mike Connor [:mconnor] 2007-06-15 10:49:31 PDT
Kaie, any idea if we can fix this for 1.9?
Comment 5 Kai Engert (:kaie) 2007-08-07 11:17:54 PDT
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?
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2007-08-07 18:23:52 PDT
> 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...
Comment 7 Mike Schroepfer 2008-01-07 19:39:28 PST
(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?
Comment 8 O. Atsushi (Torisugari) 2009-04-30 08:59:27 PDT
(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.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2009-04-30 10:12:04 PDT
You could add something to nsIWebProgressListener2, if you wanted.
Comment 10 O. Atsushi (Torisugari) 2009-05-01 07:09:21 PDT
(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.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2009-05-01 08:32:37 PDT
> Does XPCOM suppots method overloading?

In brief, no.  It's not a matter of "XPCOM"; JS doesn't support it.
Comment 12 Brendan Eich [:brendan] 2009-05-01 10:42:30 PDT
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
Comment 13 O. Atsushi (Torisugari) 2009-05-02 05:35:54 PDT
Created attachment 375463 [details] [diff] [review]
Patch

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?
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2009-05-03 11:23:57 PDT
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...
Comment 15 O. Atsushi (Torisugari) 2009-05-03 20:22:17 PDT
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.
Comment 16 O. Atsushi (Torisugari) 2009-05-03 21:38:56 PDT
Created attachment 375566 [details] [diff] [review]
Patch 2

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.
Comment 17 O. Atsushi (Torisugari) 2009-05-04 04:59:48 PDT
Created attachment 375594 [details] [diff] [review]
Patch 2.1

I'm sorry. I overlooked a lot of cases where |wasAnchor| can be wrong.
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2009-05-04 06:38:54 PDT
> 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 19 Boris Zbarsky [:bz] (still a bit busy) 2009-08-12 15:36:44 PDT
Comment on attachment 375594 [details] [diff] [review]
Patch 2.1

Per comments.
Comment 20 O. Atsushi (Torisugari) 2011-04-11 13:02:48 PDT
Created attachment 525139 [details] [diff] [review]
Patch 3

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.
Comment 21 O. Atsushi (Torisugari) 2011-04-12 11:46:16 PDT
Created attachment 525455 [details] [diff] [review]
Patch 3.1
Comment 22 O. Atsushi (Torisugari) 2011-04-14 06:12:55 PDT
Created attachment 525981 [details] [diff] [review]
Patch 3.2

|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.
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2011-04-14 06:30:57 PDT
Given the comments in this patch, I think those are equivalent, yes.
Comment 24 O. Atsushi (Torisugari) 2011-04-14 09:46:09 PDT
Created attachment 526028 [details] [diff] [review]
Patch 3.3

Simple OnNewURI(...).
Comment 25 O. Atsushi (Torisugari) 2011-04-14 10:44:37 PDT
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 26 Boris Zbarsky [:bz] (still a bit busy) 2011-04-14 10:47:39 PDT
Comment on attachment 526028 [details] [diff] [review]
Patch 3.3

smaug, let me know if you really need me to review this, ok?
Comment 27 O. Atsushi (Torisugari) 2011-04-16 18:17:34 PDT
Created attachment 526543 [details] [diff] [review]
Test

browser-chrome-mochitest for bug 283733 and bug 307027.
Comment 28 Olli Pettay [:smaug] (reviewing overload) 2011-04-20 10:33:55 PDT
I'll try to review this today or tomorrow.
Comment 29 Olli Pettay [:smaug] (reviewing overload) 2011-04-21 08:04:31 PDT
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.
Comment 30 O. Atsushi (Torisugari) 2011-04-22 02:38:11 PDT
(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.
Comment 31 O. Atsushi (Torisugari) 2011-04-22 03:14:59 PDT
Created attachment 527749 [details] [diff] [review]
Patch 3.4

Comments addressed.
Comment 32 Olli Pettay [:smaug] (reviewing overload) 2011-04-22 03:29:10 PDT
Well, having a const for LOCATION_NORMAL, yet in such way that
flags & LOCATION_NORMAL is always false is quite strange.
Comment 33 Olli Pettay [:smaug] (reviewing overload) 2011-04-22 03:52:46 PDT
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.
Comment 34 O. Atsushi (Torisugari) 2011-04-22 16:48:21 PDT
Created attachment 527882 [details] [diff] [review]
Patch 3.5

(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 35 O. Atsushi (Torisugari) 2011-04-23 05:57:55 PDT
Comment on attachment 527882 [details] [diff] [review]
Patch 3.5

Thanks.

Then I need super-review.
Comment 36 Boris Zbarsky [:bz] (still a bit busy) 2011-05-25 11:51:30 PDT
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.  :(
Comment 37 O. Atsushi (Torisugari) 2011-05-27 05:56:02 PDT
Created attachment 535612 [details] [diff] [review]
Patch 3.6

(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.
Comment 38 Boris Zbarsky [:bz] (still a bit busy) 2011-05-27 11:32:04 PDT
> 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)?
Comment 39 O. Atsushi (Torisugari) 2011-06-03 21:44:03 PDT
Created attachment 537309 [details] [diff] [review]
Patch 3.6.1

(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.
Comment 40 Boris Zbarsky [:bz] (still a bit busy) 2011-06-07 12:22:34 PDT
fwiw, the "checkin needed" marker is a keyword, not a status whiteboard annotation....

I'll get this landed.
Comment 41 Boris Zbarsky [:bz] (still a bit busy) 2011-06-07 15:33:19 PDT
http://hg.mozilla.org/projects/cedar/rev/dc3780dd8a43
Comment 42 O. Atsushi (Torisugari) 2011-06-08 04:31:28 PDT
(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.
Comment 44 Giorgio Maone [:mao] 2011-06-10 03:04:07 PDT
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?
Comment 45 O. Atsushi (Torisugari) 2011-06-10 05:52:12 PDT
(In reply to comment #44)
> Correct?

Yes, absolutely. And the more serious problem is I overlooked tabbrowser.xml for some reason...
Comment 46 Dão Gottwald [:dao] 2011-06-10 06:21:26 PDT
What a sad-making API :(
Comment 47 O. Atsushi (Torisugari) 2011-06-10 07:31:13 PDT
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.
Comment 48 John P Baker 2011-06-10 07:43:55 PDT
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
Comment 49 Giorgio Maone [:mao] 2011-06-10 08:11:02 PDT
(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
Comment 50 O. Atsushi (Torisugari) 2011-06-11 03:16:43 PDT
Created attachment 538675 [details] [diff] [review]
Test v2
Comment 51 O. Atsushi (Torisugari) 2011-06-11 04:44:02 PDT
Created attachment 538684 [details] [diff] [review]
Followup v1 onLocationChange2(..) in /browser/

addressing comment 44.
Comment 52 O. Atsushi (Torisugari) 2011-06-11 18:07:36 PDT
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 53 Dão Gottwald [:dao] 2011-06-11 22:38:47 PDT
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?
Comment 54 O. Atsushi (Torisugari) 2011-06-12 00:09:06 PDT
(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.
Comment 55 Dão Gottwald [:dao] 2011-06-12 00:22:41 PDT
Changing the signature of onLocationChange doesn't necessarily mean merging nsIWebProgressListener and nsIWebProgressListene2.
Comment 56 O. Atsushi (Torisugari) 2011-06-12 00:40:46 PDT
(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.
Comment 57 Dão Gottwald [:dao] 2011-06-12 00:46:42 PDT
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.
Comment 58 Boris Zbarsky [:bz] (still a bit busy) 2011-06-12 14:22:13 PDT
> 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.
Comment 59 Dão Gottwald [:dao] 2011-06-12 14:35:52 PDT
(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?
Comment 60 Boris Zbarsky [:bz] (still a bit busy) 2011-06-12 14:56:14 PDT
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.
Comment 61 Philip Chee 2011-06-13 03:33:08 PDT
In SeaMonkey Bug 663345 we made onLocationChange call onLocationChange2. See Attachment 538552 [details] [diff]
Comment 62 Dão Gottwald [:dao] 2011-06-13 03:36:55 PDT
Seems like we should back this out before others start doing the same.
Comment 63 Boris Zbarsky [:bz] (still a bit busy) 2011-06-13 11:51:54 PDT
Yeah, it sounds like we should just change onLocationChange instead....

I won't have a tree on hand until tomorrow, sadly.
Comment 64 Philip Chee 2011-06-13 12:14:26 PDT
Do remember to drop the Thunderbird people a line. A quick grep in comm-central/mailnews indicates a bunch of .cpp code references nsIWebProgressListener
Comment 65 Dão Gottwald [:dao] 2011-06-13 15:09:41 PDT
backed out: http://hg.mozilla.org/mozilla-central/rev/5e3cf50377aa
Comment 66 Asa Dotzler [:asa] 2011-07-05 19:43:54 PDT
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.
Comment 67 O. Atsushi (Torisugari) 2011-08-15 02:22:29 PDT
Created attachment 553132 [details] [diff] [review]
Patch v4 part1 (idl, docshell, secureUI)

Addressing comment #53
Comment 68 O. Atsushi (Torisugari) 2011-08-15 02:26:39 PDT
Created attachment 553134 [details] [diff] [review]
Patch v4 part2 (Rest of onLocationChange(...)s)

Well, I grepped 3 times.
Comment 69 O. Atsushi (Torisugari) 2011-08-15 17:46:06 PDT
Created attachment 553320 [details] [diff] [review]
Patch v4.1 part1 (idl, docshell, secureUI)
Comment 70 O. Atsushi (Torisugari) 2011-08-15 17:48:33 PDT
Created attachment 553325 [details] [diff] [review]
Patch v4.1.1 part1 (idl, docshell, secureUI)
Comment 71 O. Atsushi (Torisugari) 2011-08-15 17:51:56 PDT
Created attachment 553327 [details] [diff] [review]
Patch v4.1.1 part2 (Rest of onLocationChange(...)s)
Comment 72 O. Atsushi (Torisugari) 2011-08-16 20:50:35 PDT
Created attachment 553673 [details] [diff] [review]
Test v3
Comment 73 O. Atsushi (Torisugari) 2011-08-17 03:06:07 PDT
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 74 O. Atsushi (Torisugari) 2011-08-17 03:18:24 PDT
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.
Comment 75 O. Atsushi (Torisugari) 2011-08-21 02:47:01 PDT
Created attachment 554703 [details] [diff] [review]
for comm-central

I'm not too sure about chatzilla and venkman though.
Comment 77 Olli Pettay [:smaug] (reviewing overload) 2011-08-26 04:02:48 PDT
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?
Comment 78 Olli Pettay [:smaug] (reviewing overload) 2011-08-29 05:19:52 PDT
Torisugari, would the patch be simpler if the last parameter was optional?
Comment 79 O. Atsushi (Torisugari) 2011-08-29 06:43:16 PDT
(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.
Comment 80 Olli Pettay [:smaug] (reviewing overload) 2011-08-29 07:59:41 PDT
Default value for optional numeric parameters is 0 (at least IIRC)
Comment 81 O. Atsushi (Torisugari) 2011-08-31 03:38:20 PDT
Created attachment 557113 [details] [diff] [review]
Patch v4.2 part1 (idl, docshell, secureUI)

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 82 Olli Pettay [:smaug] (reviewing overload) 2011-10-10 12:01:33 PDT
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 83 Boris Zbarsky [:bz] (still a bit busy) 2011-10-18 12:54:32 PDT
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 84 Boris Zbarsky [:bz] (still a bit busy) 2011-10-18 12:56:10 PDT
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
Comment 85 O. Atsushi (Torisugari) 2011-10-19 09:52:21 PDT
(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.
Comment 86 Dão Gottwald [:dao] 2011-10-19 11:42:36 PDT
<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.
Comment 87 Boris Zbarsky [:bz] (still a bit busy) 2011-10-19 12:09:43 PDT
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.
Comment 88 O. Atsushi (Torisugari) 2011-10-20 10:25:41 PDT
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.
Comment 89 Boris Zbarsky [:bz] (still a bit busy) 2011-10-20 10:28:05 PDT
> Then, a new problem is value of its param, 

Whatever we got passed to start with, no?
Comment 90 O. Atsushi (Torisugari) 2011-10-21 11:59:15 PDT
Yes, it makes no big difference for the time being.
Comment 91 O. Atsushi (Torisugari) 2011-10-21 12:18:50 PDT
Created attachment 568735 [details] [diff] [review]
Patch v4.3 (all in one except for comm-central's)
Comment 92 O. Atsushi (Torisugari) 2011-10-21 14:04:48 PDT
Created attachment 568768 [details] [diff] [review]
Patch v4.3 (for comm-central)
Comment 93 neil@parkwaycc.co.uk 2011-10-27 11:46:06 PDT
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!
Comment 94 O. Atsushi (Torisugari) 2011-10-27 13:36:17 PDT
(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 95 Boris Zbarsky [:bz] (still a bit busy) 2011-11-07 13:00:15 PST
Comment on attachment 568735 [details] [diff] [review]
Patch v4.3 (all in one except for comm-central's)

sr=me; sorry for the lag....
Comment 96 O. Atsushi (Torisugari) 2011-11-08 05:43:17 PST
Created attachment 572789 [details] [diff] [review]
Patch v4.3.1 (for mozilla-central)

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.
Comment 97 O. Atsushi (Torisugari) 2011-11-08 06:19:32 PST
Created attachment 572798 [details] [diff] [review]
Patch v4.3.1 (for comm-central)

up to date, per bug 686347. (specialTabs.js and uriListener.js)
Comment 99 Marco Bonardo [::mak] 2011-11-11 02:15:09 PST
https://hg.mozilla.org/mozilla-central/rev/f18be484cb5a
Comment 100 David :Bienvenu 2011-11-11 11:56:06 PST
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 101 David :Bienvenu 2011-11-11 12:48:40 PST
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)

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