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)

Product:
Core
Component:
DOM: Navigation
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.1

People

(Reporter: asaf, Assigned: asaf)

References

Details

(Keywords: fixed1.9.1)

Attachments

(2 files, 3 obsolete files)

as checked in
(deleted), patch
Details | Diff | Splinter Review
The 191 patch which I landed
(deleted), patch
Details | Diff | Splinter Review
Attached patch Patch (obsolete) (deleted) — — 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)
Actually, it must land before b3 due to the fact that we fire the events at all.
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-
Attached patch Patch (obsolete) (deleted) — — Splinter Review
Attachment #364131 - Attachment is obsolete: true
Attachment #364225 - Flags: superreview?(bzbarsky)
Attachment #364225 - Flags: review?(bzbarsky)
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 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-
Attached patch Patch (obsolete) (deleted) — — Splinter Review
Attachment #364225 - Attachment is obsolete: true
Attachment #364382 - Flags: superreview?(bzbarsky)
Attachment #364382 - Flags: review?(bzbarsky)
I'll fix the hard-tab on checkin or in the next patch.
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+
Attached patch as checked in (deleted) — — Splinter Review
Attachment #364382 - Attachment is obsolete: true
4bdc1778459e
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Attachment #364391 - Flags: approval1.9.1?
Blocks a P2 so it is a P2.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Did you mean to check in build/wince/tools/Makefile ? http://hg.mozilla.org/mozilla-central/rev/4bdc1778459e
Fixed. Also brought back one comment and fixed the hard tab. Thanks.
1.9.1: 8947f6d07927
Keywords: fixed1.9.1
Target Milestone: --- → mozilla1.9.1
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.
I backed this out of the branch.
Keywords: fixed1.9.1
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.
Attached patch The 191 patch which I landed (deleted) — — Splinter Review
1bad6093db64
Keywords: fixed1.9.1
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.
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.
Depends on: 481528
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: