Closed
Bug 422543
Opened 17 years ago
Closed 12 years ago
nsISHistory.addSHistoryListener replaces the listener, doesn't really add it
Categories
(Core :: DOM: Navigation, defect)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: WeirdAl, Assigned: ttaubert)
References
(Depends on 1 open bug, )
Details
(Keywords: dev-doc-needed)
Attachments
(3 files, 2 obsolete files)
16.38 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
5.54 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
1.23 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
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.
Component: History: Session → Document Navigation
QA Contact: history.session → docshell
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.
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?
Attachment #662477 -
Flags: feedback?(bugs)
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 {
..
}
Attachment #662477 -
Flags: feedback?(bugs) → feedback-
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.
Attachment #662477 -
Attachment is obsolete: true
Attachment #662526 -
Flags: review?(bugs)
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.
Attachment #662526 -
Flags: review?(bugs) → review+
Assignee | ||
Updated•12 years ago
|
Keywords: dev-doc-needed
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.
Attachment #662526 -
Attachment is obsolete: true
Attachment #662626 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Attachment #662626 -
Attachment description: make session history support multiple listeners v3 → part 1 - make session history support multiple listeners v3
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #662966 -
Flags: review?(bugs)
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.
Attachment #662966 -
Flags: review?(bugs) → review+
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.
Assignee | ||
Comment 10•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/98a4a0177f22
https://hg.mozilla.org/integration/fx-team/rev/196ca3ef5b57
Whiteboard: [fixed-in-fx-team]
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/98a4a0177f22
https://hg.mozilla.org/mozilla-central/rev/196ca3ef5b57
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Assignee | ||
Updated•12 years ago
|
Whiteboard: [fixed-in-fx-team]
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.)
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.
Attachment #663326 -
Flags: review?(neil)
Updated•12 years ago
|
Attachment #663326 -
Flags: review?(neil) → review+
Assignee | ||
Comment 14•12 years ago
|
||
Assignee | ||
Comment 15•12 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•