Last Comment Bug 895974 - document.firstElementChild is not defined
: document.firstElementChild is not defined
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla25
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 901632 932501
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-19 10:51 PDT by Erik Arvidsson
Modified: 2013-10-31 06:22 PDT (History)
5 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement ParentNode on document fragments and documents and move previousElementSibling and nextElementSibling to ChildNode. (19.45 KB, patch)
2013-07-19 20:24 PDT, Boris Zbarsky [:bz] (still a bit busy)
bugs: review-
Details | Diff | Splinter Review
With review comments addressed (21.07 KB, patch)
2013-07-20 19:25 PDT, Boris Zbarsky [:bz] (still a bit busy)
bugs: review+
Details | Diff | Splinter Review
Interdiff (2.88 KB, patch)
2013-07-20 19:25 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review

Description Erik Arvidsson 2013-07-19 10:51:55 PDT
http://dom.spec.whatwg.org/#interface-parentnode

Document should implement the ParentNode interface
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2013-07-19 12:00:03 PDT
Yeah, might as well.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2013-07-19 20:24:42 PDT
Created attachment 778785 [details] [diff] [review]
Implement ParentNode on document fragments and documents and move previousElementSibling and nextElementSibling to ChildNode.
Comment 3 Olli Pettay [:smaug] 2013-07-20 11:03:44 PDT
Comment on attachment 778785 [details] [diff] [review]
Implement ParentNode on document fragments and documents and move previousElementSibling and nextElementSibling to ChildNode.

Could you test also .children, and the patch is missing ParentNode.webidl file.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2013-07-20 19:25:01 PDT
Created attachment 778900 [details] [diff] [review]
With review comments addressed
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2013-07-20 19:25:42 PDT
Created attachment 778901 [details] [diff] [review]
Interdiff
Comment 6 Olli Pettay [:smaug] 2013-07-21 03:09:11 PDT
Comment on attachment 778900 [details] [diff] [review]
With review comments addressed

Thanks, the interdiff was good.
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2013-07-22 01:14:03 PDT
Comment on attachment 778900 [details] [diff] [review]
With review comments addressed

Review of attachment 778900 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsINode.cpp
@@ +1407,5 @@
> +Element*
> +nsINode::GetPreviousElementSibling() const
> +{
> +  nsIContent* previousSibling = GetPreviousSibling();
> +  while (previousSibling) {

Nit: a for loop like you have in GetFirstElementChild() seems nicer.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2013-07-22 05:20:30 PDT
> Nit: a for loop like you have in GetFirstElementChild() seems nicer.

Hmm.. I guess; I just moved the code.

In any case, I pushed this before I saw the nit comment: https://hg.mozilla.org/integration/mozilla-inbound/rev/0a196c0e9f96
Comment 9 Ryan VanderMeulen [:RyanVM] 2013-07-22 15:23:17 PDT
Either this patch or the patch from bug 895974 (pushed at the same time) was causing frequent mochitest-1 asserts on the Ubuntu32 test slaves. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3794beed0e34

https://tbpl.mozilla.org/php/getParsedLog.php?id=25574572&tree=Mozilla-Inbound

13:26:23     INFO -  [Parent 2282] ###!!! ASSERTION: Clock should go forwards if the playback rate is > 0.: 'mCurrentFrameTime <= clock_time || mPlaybackRate <= 0', file ../../../content/media/MediaDecoderStateMachine.cpp, line 2435
13:26:23     INFO -  mozilla::MediaDecoderStateMachine::GetClock() [content/media/MediaDecoderStateMachine.cpp:2434]
13:26:23     INFO -  mozilla::MediaDecoderStateMachine::AdvanceFrame() [content/media/MediaDecoderStateMachine.cpp:2462]
13:26:23     INFO -  mozilla::MediaDecoderStateMachine::RunStateMachine() [content/media/MediaDecoderStateMachine.cpp:2235]
13:26:23     INFO -  mozilla::MediaDecoderStateMachine::CallRunStateMachine() [content/media/MediaDecoderStateMachine.cpp:2741]
13:26:23     INFO -  mozilla::MediaDecoderStateMachine::Run() [content/media/MediaDecoderStateMachine.cpp:2715]
13:26:23     INFO -  nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:621]
13:26:23     INFO -  NS_ProcessNextEvent(nsIThread*, bool) [obj-firefox/xpcom/build/nsThreadUtils.cpp:238]
13:26:23     INFO -  nsThread::ThreadFunc(void*) [xpcom/threads/nsThread.cpp:249]
13:26:23     INFO -  _pt_root [nsprpub/pr/src/pthreads/ptthread.c:207]
13:26:23     INFO -  libpthread.so.0 + 0x6d4c
13:26:23     INFO -  [Parent 2282] ###!!! ASSERTION: Should have positive clock time.: 'clock_time >= mStartTime', file ../../../content/media/MediaDecoderStateMachine.cpp, line 2462
13:26:23     INFO -  mozilla::MediaDecoderStateMachine::AdvanceFrame() [content/media/MediaDecoderStateMachine.cpp:2462]
13:26:23     INFO -  mozilla::MediaDecoderStateMachine::RunStateMachine() [content/media/MediaDecoderStateMachine.cpp:2235]
13:26:23     INFO -  mozilla::MediaDecoderStateMachine::CallRunStateMachine() [content/media/MediaDecoderStateMachine.cpp:2741]
13:26:23     INFO -  mozilla::MediaDecoderStateMachine::Run() [content/media/MediaDecoderStateMachine.cpp:2715]
13:26:23     INFO -  nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:621]
13:26:23     INFO -  NS_ProcessNextEvent(nsIThread*, bool) [obj-firefox/xpcom/build/nsThreadUtils.cpp:238]
13:26:23     INFO -  nsThread::ThreadFunc(void*) [xpcom/threads/nsThread.cpp:249]
13:26:23     INFO -  _pt_root [nsprpub/pr/src/pthreads/ptthread.c:207]
13:26:23     INFO -  libpthread.so.0 + 0x6d4c
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2013-07-23 11:51:40 PDT
This patch was the one that triggered those assertions, but they're due to bugs in the test.  Annotated the test, and relanded:  https://hg.mozilla.org/integration/mozilla-inbound/rev/5b59f2f35bb3
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-07-23 17:48:39 PDT
https://hg.mozilla.org/mozilla-central/rev/5b59f2f35bb3

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