Alex Vincent [:WeirdAl] Reporter
	Description • 17 years ago

See the URL field.  The IDL's JavaDoc clearly states the listener is added; in reality, it replaces whatever listener may have already been there.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 1 • 12 years ago

I just recently hit this bug which took me a while to figure out that addSHistoryListener() is completely different from e.g. addObserver() in that it silently overrides the current listener and allows only one at a time. I have some ideas for session store that would benefit from being able to register multiple shistory listeners.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 2 • 12 years ago

I'm still not sure what to do about the canReload/canNavigate argument. I think we should stop iterating/notifying listeners once one of them returned 'false'. There's not really a way to let previously called listeners know that the action will be cancelled... How could we handle those situations?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 12 years ago

Comment on attachment 662477 [details] [diff] [review]
make session history support multiple listeners


>+// This macro calls a given method on all registered session history
>+// listeners.
>+#define NOTIFY_LISTENERS(method, ...)                      \
>+  PR_BEGIN_MACRO                                           \
>+  {                                                        \
>+    nsAutoTObserverArray<nsWeakPtr, 2>::EndLimitedIterator \
>+      iter(mListeners);                                    \
>+    while (iter.HasMore()) {                               \
>+      nsCOMPtr<nsISHistoryListener> listener =             \
>+        do_QueryReferent(iter.GetNext());                  \
>+      if (listener) {                                      \
>+          listener->method(__VA_ARGS__);                   \
>+      }                                                    \
>+    }                                                      \
>+  }                                                        \
>+  PR_END_MACRO
>+

Does this work with all the compilers? If not, don't use __VA_ARGS__ but
do something similar to FORWARD_TO_INNER in nsGlobalWindow.cpp where parameters
need to be in () and you call the method
listener->method args


>   bool purgeHistory = true;
>-  // Notify the listener about the history purge
>-  if (mListener) {
>-    nsCOMPtr<nsISHistoryListener> listener(do_QueryReferent(mListener));
>-    if (listener) {
>-      listener->OnHistoryPurge(aEntries, &purgeHistory);
>-    } 
>-  }
>+  NOTIFY_LISTENERS(OnHistoryPurge, aEntries, &purgeHistory);
> 
>   if (!purgeHistory) {
>     // Listener asked us not to purge
>     return NS_SUCCESS_LOSS_OF_INSIGNIFICANT_DATA;
>   }
What if first listener returns false but second one true.
We'd ignore the return value of the first one.
I think we should not do that.


>+nsSHistory::NotifyOnHistoryReload(nsIURI* aReloadURI, uint32_t aReloadFlags,
>+                                  bool* aCanReload)
> {
>-  NS_ENSURE_ARG_POINTER(aListener);
>-  if (mListener) 
>-    CallQueryReferent(mListener.get(),  aListener);
>-  // Don't addref aListener. It is a weak pointer.
>+  NOTIFY_LISTENERS(OnHistoryReload, aReloadURI, aReloadFlags, aCanReload);
>   return NS_OK;
> }
Similar problem here and elsewhere.
All the listeners should be called, but if one says please-don't-reload, then
reload should be prevented.
That way listeners would work like DOM Event Listeners where if anyone calls preventDefault(), the default handling
is cancelled.

>+  if (aHistCmd == HIST_CMD_BACK) {
>+    // We are going back one entry. Send GoBack notifications
>+    NOTIFY_LISTENERS(OnHistoryGoBack, nextURI, &canNavigate);
>+  }
>+  else if (aHistCmd == HIST_CMD_FORWARD) {
>+    // We are going forward. Send GoForward notification
>+    NOTIFY_LISTENERS(OnHistoryGoForward, nextURI, &canNavigate);
>+  }
>+  else if (aHistCmd == HIST_CMD_GOTOINDEX) {
>+    // We are going somewhere else. This is not reload either
>+    NOTIFY_LISTENERS(OnHistoryGotoIndex, aIndex, nextURI, &canNavigate);
>   }
> 

In new code, please use Gecko coding style
if (...) {
  ...
} else if (...) {
  ...
} else {
..
}

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 4 • 12 years ago

(In reply to Olli Pettay [:smaug] from comment #3)
> Does this work with all the compilers? If not, don't use __VA_ARGS__ but
> do something similar to FORWARD_TO_INNER in nsGlobalWindow.cpp where
> parameters
> need to be in () and you call the method
> listener->method args

Done.

> What if first listener returns false but second one true.
> We'd ignore the return value of the first one.
> I think we should not do that.

Fixed.

> Similar problem here and elsewhere.
> All the listeners should be called, but if one says please-don't-reload, then
> reload should be prevented.
> That way listeners would work like DOM Event Listeners where if anyone calls
> preventDefault(), the default handling
> is cancelled.

Fixed.

> In new code, please use Gecko coding style

Done.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 12 years ago

Comment on attachment 662526 [details] [diff] [review]
make session history support multiple listeners v2

>+#define NOTIFY_LISTENERS_CANCELABLE(method, retval, args)  \
>+  PR_BEGIN_MACRO                                           \
>+  {                                                        \
>+    bool canceled = false;                                 \
>+    ITERATE_LISTENERS(                                     \
>+      listener->method args;                               \
>+      if (!retval) {                                       \
>+        canceled = true;                                   \
>+      }                                                    \
>+    );                                                     \
>+    if (canceled) {                                        \
>+      retval = false;                                      \
>+    }                                                      \
>+  }                                                        \
>+  PR_END_MACRO
>+
This is a bit ugly, but I don't know better ways to do this.

>+    // If a listener has changed mIndex, we need to get currentTxn again,
>+    // otherwise we'll be left at an inconsistent state (see bug 320742)
>+    if (currentIndex != mIndex)
>+      GetTransactionAtIndex(mIndex, getter_AddRefs(currentTxn));

I know you just moved this code, but could you use
if (expr) {
  stmt;
}


>+nsSHistory::NotifyOnHistoryReload(nsIURI* aReloadURI, uint32_t aReloadFlags,
>+                                  bool* aCanReload)
> {
>-  NS_ENSURE_ARG_POINTER(aListener);
>-  if (mListener) 
>-    CallQueryReferent(mListener.get(),  aListener);
>-  // Don't addref aListener. It is a weak pointer.
>+  NOTIFY_LISTENERS_CANCELABLE(OnHistoryReload, *aCanReload,
>+                              (aReloadURI, aReloadFlags, aCanReload));
>   return NS_OK;
> }
I think *aCanReload should be set to true before NOTIFY_LISTENERS_CANCELABLE, so that if there are no listeners, 
this returns true.
Or actually, perhaps the macro should set retval to true before doing anything. That would fix all the cases it is being used.


r=me, but this really needs some tests.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 6 • 12 years ago

(In reply to Olli Pettay [:smaug] from comment #5)
> This is a bit ugly, but I don't know better ways to do this.

My thoughts, exactly :/

> >+    // If a listener has changed mIndex, we need to get currentTxn again,
> >+    // otherwise we'll be left at an inconsistent state (see bug 320742)
> >+    if (currentIndex != mIndex)
> >+      GetTransactionAtIndex(mIndex, getter_AddRefs(currentTxn));
> 
> I know you just moved this code, but could you use
> if (expr) {
>   stmt;
> }

Done.

> Or actually, perhaps the macro should set retval to true before doing
> anything. That would fix all the cases it is being used.

Good catch, done.

> r=me, but this really needs some tests.

Yes, will work on a patch for this.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 8 • 12 years ago

Comment on attachment 662966 [details] [diff] [review]
part 2 - add tests for multiple session history listeners

I wonder if at some point we'll need to add something new to the API, so that
listeners which don't cancel something, get notified that some other listener
canceled the operation.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 9 • 12 years ago

(In reply to Olli Pettay [:smaug] from comment #8)
> I wonder if at some point we'll need to add something new to the API, so that
> listeners which don't cancel something, get notified that some other listener
> canceled the operation.

Yeah, I thought about the same. Not sure how to achieve that, though. Now we're notifying listeners *before* an action takes places and they can cancel it but don't know about previous or subsequent listeners' decisions.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 10 • 12 years ago

https://hg.mozilla.org/integration/fx-team/rev/98a4a0177f22
https://hg.mozilla.org/integration/fx-team/rev/196ca3ef5b57

	Dave Camp (:dcamp)
	Comment 11 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/98a4a0177f22
https://hg.mozilla.org/mozilla-central/rev/196ca3ef5b57

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

Comment on attachment 662626 [details] [diff] [review]
part 1 - make session history support multiple listeners v3

>+        body;                                              \
Nit: body is a statement, so the semicolon is unnecessary.

>-  // Don't addref aListener. It is a weak pointer.
Good riddance. (Comment was right for the wrong reason.)

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 13 • 12 years ago

(In reply to neil@parkwaycc.co.uk from comment #12)
> >+        body;                                              \
> Nit: body is a statement, so the semicolon is unnecessary.

Ah, right. We can push a little follow-up for that.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 14 • 12 years ago

https://hg.mozilla.org/integration/fx-team/rev/6cd4d5d108d6

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 15 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/6cd4d5d108d6