It is unfortunate that this fix changes unused code. Will it be covered by existing layout tests when ScriptFunctionCall and ScriptCallback start being used? The patch and bug were highly confusing. Without any explanation of why this assertion was wrong, a test case, or an explanation of why one can't be made, I had to spend considerable time figuring out why it shouldn't be rolled out immediately. - WBR, Alexey Proskuryakov Начало переадресованного сообщения:
От: commit-queue@webkit.org Дата: 23 сентября 2010 г. 8:57:01 Тихоокеанское летнее время Кому: webkit-changes@lists.webkit.org Тема: [webkit-changes] [68146] trunk/WebCore
Revision 68146 Author commit-queue@webkit.org Date 2010-09-23 08:56:59 -0700 (Thu, 23 Sep 2010) Log Message
2010-09-23 Luiz Agostini <luiz.agostini@openbossa.org>
Reviewed by Andreas Kling.
Invalid assertion in ScriptCallback https://bugs.webkit.org/show_bug.cgi?id=46348
Removing invalid ASSERT from method ScriptCallback::call().
* bindings/js/ScriptFunctionCall.cpp: (WebCore::ScriptCallback::call): Modified Paths
trunk/WebCore/ChangeLog trunk/WebCore/bindings/js/ScriptFunctionCall.cpp Diff
Modified: trunk/WebCore/ChangeLog (68145 => 68146)
--- trunk/WebCore/ChangeLog 2010-09-23 15:51:41 UTC (rev 68145) +++ trunk/WebCore/ChangeLog 2010-09-23 15:56:59 UTC (rev 68146) @@ -1,3 +1,15 @@ +2010-09-23 Luiz Agostini <luiz.agostini@openbossa.org> + + Reviewed by Andreas Kling. + + Invalid assertion in ScriptCallback + https://bugs.webkit.org/show_bug.cgi?id=46348 + + Removing invalid ASSERT from method ScriptCallback::call(). + + * bindings/js/ScriptFunctionCall.cpp: + (WebCore::ScriptCallback::call): + 2010-09-23 Martin Robinson <mrobinson@igalia.com>
Reviewed by Ariya Hidayat. Modified: trunk/WebCore/bindings/js/ScriptFunctionCall.cpp (68145 => 68146)
--- trunk/WebCore/bindings/js/ScriptFunctionCall.cpp 2010-09-23 15:51:41 UTC (rev 68145) +++ trunk/WebCore/bindings/js/ScriptFunctionCall.cpp 2010-09-23 15:56:59 UTC (rev 68146) @@ -215,9 +215,9 @@
CallData callData; CallType callType = getCallData(m_function.jsValue(), callData); + if (callType == CallTypeNone) + return ScriptValue();
- ASSERT(callType != CallTypeNone); - JSValue result = JSC::call(m_exec, m_function.jsValue(), callType, callData, m_function.jsValue(), m_arguments); hadException = m_exec->hadException();
_______________________________________________ webkit-changes mailing list webkit-changes@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes
Find some details here: https://bugs.webkit.org/show_bug.cgi?id=37205 , specially https://bugs.webkit.org/show_bug.cgi?id=43216#c15 .and followup comments. Bug and changelog in bug 46348 are poor, though, I agree. On Thu, Sep 23, 2010 at 2:03 PM, Alexey Proskuryakov <ap@webkit.org> wrote:
It is unfortunate that this fix changes unused code. Will it be covered by existing layout tests when ScriptFunctionCall and ScriptCallback start being used? The patch and bug were highly confusing. Without any explanation of why this assertion was wrong, a test case, or an explanation of why one can't be made, I had to spend considerable time figuring out why it shouldn't be rolled out immediately. - WBR, Alexey Proskuryakov Начало переадресованного сообщения:
От: commit-queue@webkit.org Дата: 23 сентября 2010 г. 8:57:01 Тихоокеанское летнее время Кому: webkit-changes@lists.webkit.org Тема: [webkit-changes] [68146] trunk/WebCore
Revision 68146 Author commit-queue@webkit.org Date 2010-09-23 08:56:59 -0700 (Thu, 23 Sep 2010)
Log Message
2010-09-23 Luiz Agostini <luiz.agostini@openbossa.org>
Reviewed by Andreas Kling.
Invalid assertion in ScriptCallback https://bugs.webkit.org/show_bug.cgi?id=46348
Removing invalid ASSERT from method ScriptCallback::call().
* bindings/js/ScriptFunctionCall.cpp: (WebCore::ScriptCallback::call):
Modified Paths
trunk/WebCore/ChangeLog trunk/WebCore/bindings/js/ScriptFunctionCall.cpp
Diff
Modified: trunk/WebCore/ChangeLog (68145 => 68146)
--- trunk/WebCore/ChangeLog 2010-09-23 15:51:41 UTC (rev 68145) +++ trunk/WebCore/ChangeLog 2010-09-23 15:56:59 UTC (rev 68146) @@ -1,3 +1,15 @@ +2010-09-23 Luiz Agostini <luiz.agostini@openbossa.org> + + Reviewed by Andreas Kling. + + Invalid assertion in ScriptCallback + https://bugs.webkit.org/show_bug.cgi?id=46348 + + Removing invalid ASSERT from method ScriptCallback::call(). + + * bindings/js/ScriptFunctionCall.cpp: + (WebCore::ScriptCallback::call): + 2010-09-23 Martin Robinson <mrobinson@igalia.com>
Reviewed by Ariya Hidayat.
Modified: trunk/WebCore/bindings/js/ScriptFunctionCall.cpp (68145 => 68146)
--- trunk/WebCore/bindings/js/ScriptFunctionCall.cpp 2010-09-23 15:51:41 UTC (rev 68145) +++ trunk/WebCore/bindings/js/ScriptFunctionCall.cpp 2010-09-23 15:56:59 UTC (rev 68146) @@ -215,9 +215,9 @@
CallData callData; CallType callType = getCallData(m_function.jsValue(), callData); + if (callType == CallTypeNone) + return ScriptValue();
- ASSERT(callType != CallTypeNone); - JSValue result = JSC::call(m_exec, m_function.jsValue(), callType, callData, m_function.jsValue(), m_arguments); hadException = m_exec->hadException();
_______________________________________________ webkit-changes mailing list webkit-changes@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
-- --Antonio Gomes
Sorry. I should really have made better comments. This code is not used yet, I just aded it yesterday and plan to use it soon. The assertion, that I have introduced, was wrong because the case callType == CallTypeNone must be handled. It is not about correcting existing problems because I just introduced this code. It will not cause any regressions because the code that will use it did not land yet, it is in review process. I am sorry if I made you spend your time. Next time please contact me, it is easy to find me. I have been working actively in webkit for a while, I am always in iRC (lca), my name, email and IRC user can be found in http://trac.webkit.org/wiki/WebKit%20Team and I am willing to help, answer, change or whatever is needed. And of course I would reply to any comment in the bug. Luiz 2010/9/23 Alexey Proskuryakov <ap@webkit.org>
It is unfortunate that this fix changes unused code. Will it be covered by existing layout tests when ScriptFunctionCall and ScriptCallback start being used?
The patch and bug were highly confusing. Without any explanation of why this assertion was wrong, a test case, or an explanation of why one can't be made, I had to spend considerable time figuring out why it shouldn't be rolled out immediately.
- WBR, Alexey Proskuryakov
Начало переадресованного сообщения:
*От: *commit-queue@webkit.org *Дата: *23 сентября 2010 г. 8:57:01 Тихоокеанское летнее время *Кому: *webkit-changes@lists.webkit.org *Тема: **[webkit-changes] [68146] trunk/WebCore*
Revision 68146 <http://trac.webkit.org/projects/webkit/changeset/68146> Author commit-queue@webkit.org Date 2010-09-23 08:56:59 -0700 (Thu, 23 Sep 2010) Log Message
2010-09-23 Luiz Agostini <luiz.agostini@openbossa.org>
Reviewed by Andreas Kling.
Invalid assertion in ScriptCallback https://bugs.webkit.org/show_bug.cgi?id=46348
Removing invalid ASSERT from method ScriptCallback::call().
* bindings/js/ScriptFunctionCall.cpp: (WebCore::ScriptCallback::call):
Modified Paths
- trunk/WebCore/ChangeLog - trunk/WebCore/bindings/js/ScriptFunctionCall.cpp
Diff Modified: trunk/WebCore/ChangeLog (68145 => 68146)
--- trunk/WebCore/ChangeLog 2010-09-23 15:51:41 UTC (rev 68145) +++ trunk/WebCore/ChangeLog 2010-09-23 15:56:59 UTC (rev 68146)@@ -1,3 +1,15 @@+2010-09-23 Luiz Agostini <luiz.agostini@openbossa.org> + + Reviewed by Andreas Kling. + + Invalid assertion in ScriptCallback + https://bugs.webkit.org/show_bug.cgi?id=46348 + + Removing invalid ASSERT from method ScriptCallback::call(). + + * bindings/js/ScriptFunctionCall.cpp: + (WebCore::ScriptCallback::call): + 2010-09-23 Martin Robinson <mrobinson@igalia.com> Reviewed by Ariya Hidayat.
Modified: trunk/WebCore/bindings/js/ScriptFunctionCall.cpp (68145 => 68146)
--- trunk/WebCore/bindings/js/ScriptFunctionCall.cpp 2010-09-23 15:51:41 UTC (rev 68145) +++ trunk/WebCore/bindings/js/ScriptFunctionCall.cpp 2010-09-23 15:56:59 UTC (rev 68146)@@ -215,9 +215,9 @@ CallData callData; CallType callType = getCallData(m_function.jsValue(), callData);+ if (callType == CallTypeNone) + return ScriptValue(); - ASSERT(callType != CallTypeNone); - JSValue result = JSC::call(m_exec, m_function.jsValue(), callType, callData, m_function.jsValue(), m_arguments); hadException = m_exec->hadException();
_______________________________________________ webkit-changes mailing list webkit-changes@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes
23.09.2010, в 11:40, Luiz Agostini написал(а):
This code is not used yet, I just aded it yesterday and plan to use it soon. The assertion, that I have introduced, was wrong because the case callType == CallTypeNone must be handled.
It is not about correcting existing problems because I just introduced this code. It will not cause any regressions because the code that will use it did not land yet, it is in review process.
Yes, I understand that this didn't break any behavior in ToT. My question was somewhat different. If you landed the code that uses ScriptCallback without fixing this mistake, would that have made any existing regression tests break? We just need test coverage for the fix, and hopefully existing tests provide it.
I am sorry if I made you spend your time. Next time please contact me, it is easy to find me. I have been working actively in webkit for a while, I am always in iRC (lca), my name, email and IRC user can be found in http://trac.webkit.org/wiki/WebKit%20Team and I am willing to help, answer, change or whatever is needed. And of course I would reply to any comment in the bug.
I'm sorry if this came across as an attack on you. My goal was to encourage reviewers to ensure that patches have adequate documentation and test coverage, and that's why I chose to bring this up on the list. In earlier years of the project, we often said that it's the reviewer who is responsible for any problems with a landed patch. But thinking about it, this hasn't been the theme lately, for better or worse. - WBR, Alexey Proskuryakov
On 09/23/2010 09:25 PM, ext Alexey Proskuryakov wrote:
I'm sorry if this came across as an attack on you. My goal was to encourage reviewers to ensure that patches have adequate documentation and test coverage, and that's why I chose to bring this up on the list.
Point taken. It was my mistake to not ask for more documentation and test coverage in the patch. It won't be repeated. Thanks for keeping an eye out :-) -Kling
participants (4)
-
Alexey Proskuryakov
-
Andreas Kling
-
Antonio Gomes
-
Luiz Agostini