Closed
Bug 480149
Opened 16 years ago
Closed 16 years ago
DOMLinkAdded and DOMLinkRemoved events for link elements should be dispatched when swapping docshells
Categories
(Core :: DOM: Navigation, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.9.1
People
(Reporter: asaf, Assigned: asaf)
References
Details
(Keywords: fixed1.9.1)
Attachments
(2 files, 3 obsolete files)
16.39 KB,
patch
|
Details | Diff | Splinter Review | |
12.88 KB,
patch
|
Details | Diff | Splinter Review |
DOMLinkAdded and DOMLinkRemoved events for link elements should be dispatched when swapping docshells (
This is required for bug 477014 and must land before b3 due to the nsIDocument change.
Flags: blocking1.9.1?
Attachment #364131 -
Flags: superreview?(bzbarsky)
Attachment #364131 -
Flags: review?(bzbarsky)
Comment 1•16 years ago
|
||
Actually, it must land before b3 due to the fact that we fire the events at all.
Comment 2•16 years ago
|
||
Comment on attachment 364131 [details] [diff] [review]
Patch
>+++ b/content/base/public/nsIDocument.h
>+ * parameter. If aStartingPoint is null, the pagehide event is dispatched
>+ * on the the ScriptGlobalObject for this document
And otherwise on aStartingPoint, right?
I'd prefer we call this argument aDispatchStartTarget, for both methods.
>+++ b/content/base/src/nsDocument.cpp
>+nsDocument::OnPageShow(PRBool aPersisted, nsIDOMEventTarget* aStartingPoint)
>+ // If a starting-point for the event, which isn't the content window is
>+ // passed, the showing state shouldn't be altered because it hasn't actually
>+ // changed (See comments in nsFrameLoader::SwapWithOtherLoader on the cases
>+ // in which the pageshow and pagehide events are dispatched).
I think we should just document that if a target is passed in mIsShowing won't be messed with, and here just check for a non-null aStartingPoint.
Similar for the other method.
This patch seems to be missing the nsDocShell.cpp builds it needs to compile. It's also missing tests.
Attachment #364131 -
Flags: superreview?(bzbarsky)
Attachment #364131 -
Flags: superreview-
Attachment #364131 -
Flags: review?(bzbarsky)
Attachment #364131 -
Flags: review-
Assignee | ||
Comment 3•16 years ago
|
||
Attachment #364131 -
Attachment is obsolete: true
Attachment #364225 -
Flags: superreview?(bzbarsky)
Attachment #364225 -
Flags: review?(bzbarsky)
Updated•16 years ago
|
Summary: DOMLinkAdded and DOMLinkRemoved events for link elements should be dispatched when swapping docshells ( → DOMLinkAdded and DOMLinkRemoved events for link elements should be dispatched when swapping docshells
Comment 4•16 years ago
|
||
Comment on attachment 364225 [details] [diff] [review]
Patch
>+++ b/content/base/public/nsIDocument.h
>+ * dispatched on the the ScriptGlobalObject for this document, otherwise
s/the the/the/
Same for the other method's comment.
>+++ b/content/base/src/nsFrameLoader.cpp
>@@ -710,17 +708,17 @@
>-
>+
nix the random whitespace change?
>+++ b/docshell/test/chrome/bug113934_window.xul
>- strs[node.id] += node.id + ".page" + type;
>+ strs[node.id] += node.id + "." + type;
Please leave that stuff as it was; I don't see why these changes are needed for your test.
>+ // The DOMLink* events may be dispatched aasynchronously, thus I cannot
s/aa/a/
>+ // just include the <link> element in the initial DOM. Instead, I'm
>+ // adding the link element now, then ignore the first DOMLinkAdded event
>+ // (which is a result of the actual addition), and only then wait for
>+ // the DOMLink* events).
Shouldn't you wait to do the swap until after that first event fires? That would make more sense to me. Add the link, wait for the event to fire, then do the swap... Cleaner than ignoring some events but not others. You would also not need the pageEventsTested boolean then, right?
>+ if (this._addedDispatched &&
>+ this._removedDispatched &&
>+ pageEventsTested) {
If we get here before pageEventsTested (can we?) then we'll never remove these listeners.
Attachment #364225 -
Flags: superreview?(bzbarsky)
Attachment #364225 -
Flags: superreview-
Attachment #364225 -
Flags: review?(bzbarsky)
Attachment #364225 -
Flags: review-
Assignee | ||
Comment 5•16 years ago
|
||
Attachment #364225 -
Attachment is obsolete: true
Attachment #364382 -
Flags: superreview?(bzbarsky)
Attachment #364382 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 6•16 years ago
|
||
I'll fix the hard-tab on checkin or in the next patch.
Comment 7•16 years ago
|
||
Comment on attachment 364382 [details] [diff] [review]
Patch
Still need to fix the second "the the" in nsIDocument.
s/are be/are/ in the test comments.
s/adeed/added/
r+sr=bzbarsky with those comments fixed.
Attachment #364382 -
Flags: superreview?(bzbarsky)
Attachment #364382 -
Flags: superreview+
Attachment #364382 -
Flags: review?(bzbarsky)
Attachment #364382 -
Flags: review+
Assignee | ||
Comment 8•16 years ago
|
||
Attachment #364382 -
Attachment is obsolete: true
Assignee | ||
Comment 9•16 years ago
|
||
4bdc1778459e
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•16 years ago
|
Attachment #364391 -
Flags: approval1.9.1?
Comment 10•16 years ago
|
||
Blocks a P2 so it is a P2.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Updated•16 years ago
|
Attachment #364391 -
Flags: approval1.9.1?
Comment 11•16 years ago
|
||
Did you mean to check in build/wince/tools/Makefile ?
http://hg.mozilla.org/mozilla-central/rev/4bdc1778459e
Assignee | ||
Comment 12•16 years ago
|
||
Fixed. Also brought back one comment and fixed the hard tab. Thanks.
Assignee | ||
Updated•16 years ago
|
Target Milestone: --- → mozilla1.9.1
Assignee | ||
Updated•16 years ago
|
Flags: in-testsuite+
Could this patch have caused the following errors on the 1.9.1 branch:
*** 33 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/docshell/test/chrome/test_bug113934.xul | Shouldn't have fired event handlers - got "", expected "<head></head><body onbeforeunload='document.documentElement.textContent = \"\"' onunload='document.documentElement.textContent = \"\"' onpagehide='document.documentElement.textContent = \"\"'>This is a test</body>"
*** 396 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/docshell/test/chrome/test_bug456980.xul | Shouldn't have fired event handlers - got "", expected "<head></head><body onbeforeunload='document.documentElement.textContent = \"\"' onunload='document.documentElement.textContent = \"\"' onpagehide='document.documentElement.textContent = \"\"'>This is a test</body>"
It looks like they've been failing consistently since this bug was checked in.
Assignee | ||
Comment 16•16 years ago
|
||
OK, I cannot run tests locally now for unknown reasons. Since the functionality works, I'm going to land this again with the two test disabled (note: there's only one test that is failing, the other one opens that test).
Filed bug 481528 figuring this out and re-enabling the tests.
Assignee | ||
Comment 17•16 years ago
|
||
1bad6093db64
Assignee | ||
Updated•16 years ago
|
Keywords: fixed1.9.1
Comment 18•16 years ago
|
||
Um. UUUUM. NO. Absolutely not. The tests that were failing are testing something that absolutely needs to not fail. If they fail, the "functionality" does NOT WORK. I guess we'll track this in bug 481528, but that bug needs to land for b3, because it's a web compat issue.
Comment 19•16 years ago
|
||
I just read through the "The 191 patch which I landed" patch, and it doesn't have the change to the end of nsDocument::OnPageHide to fire the event to the right event target. That's why the tests were failing.
Comment 20•16 years ago
|
||
Pushed the missing hunk and test re-enable to the branch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ab638d51a755
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b5401b3b7b92
You need to log in
before you can comment on or make changes to this bug.
Description
•