Fix Test case crash for WebRTCPeerConnection. (issue 1152123002 by changbin.shao@intel.com)

1 view
Skip to first unread message

changb...@intel.com

unread,
May 21, 2015, 9:53:40 PM5/21/15
to har...@chromium.org, to...@chromium.org, tom...@chromium.org, blink-...@chromium.org, tommyw+w...@chromium.org
Reviewers: haraken, tommi, Tommy Widenflycht,

Message:
PTAL.

Description:
Fix Test case crash for WebRTCPeerConnection.

BUG=490620

Please review this at https://codereview.chromium.org/1152123002/

Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Affected files (+2, -1 lines):
M Source/modules/mediastream/RTCVoidRequestImpl.cpp


Index: Source/modules/mediastream/RTCVoidRequestImpl.cpp
diff --git a/Source/modules/mediastream/RTCVoidRequestImpl.cpp
b/Source/modules/mediastream/RTCVoidRequestImpl.cpp
index
0b467467dfd90bac4501616c384e6d702295d49b..08d22cb0dbfd59ed056265a0dafb4dafc2949288
100644
--- a/Source/modules/mediastream/RTCVoidRequestImpl.cpp
+++ b/Source/modules/mediastream/RTCVoidRequestImpl.cpp
@@ -61,7 +61,8 @@ RTCVoidRequestImpl::~RTCVoidRequestImpl()
void RTCVoidRequestImpl::requestSucceeded()
{
bool shouldFireCallback = m_requester ?
m_requester->shouldFireDefaultCallbacks() : false;
- m_requester->requestSucceeded(m_requestType);
+ if (m_requester)
+ m_requester->requestSucceeded(m_requestType);

if (shouldFireCallback && m_successCallback)
m_successCallback->handleEvent();


har...@chromium.org

unread,
May 21, 2015, 10:00:19 PM5/21/15
to changb...@intel.com, to...@chromium.org, tom...@chromium.org, blink-...@chromium.org, tommyw+w...@chromium.org

https://codereview.chromium.org/1152123002/diff/1/Source/modules/mediastream/RTCVoidRequestImpl.cpp
File Source/modules/mediastream/RTCVoidRequestImpl.cpp (right):

https://codereview.chromium.org/1152123002/diff/1/Source/modules/mediastream/RTCVoidRequestImpl.cpp#newcode68
Source/modules/mediastream/RTCVoidRequestImpl.cpp:68:
m_successCallback->handleEvent();

Can we tidy up the logic a bit?

if (m_requester) {
m_requester->requestSucceeded(m_requestType);
if (m_requester->shouldFireDefaultCallbacks() && m_successCallback)
m_successCallback->handleEvent();
}

?

https://codereview.chromium.org/1152123002/

changb...@intel.com

unread,
May 22, 2015, 12:30:19 AM5/22/15
to har...@chromium.org, to...@chromium.org, tom...@chromium.org, blink-...@chromium.org, tommyw+w...@chromium.org
Update CL. PTAL:)
haraken@


https://codereview.chromium.org/1152123002/diff/1/Source/modules/mediastream/RTCVoidRequestImpl.cpp
File Source/modules/mediastream/RTCVoidRequestImpl.cpp (right):

https://codereview.chromium.org/1152123002/diff/1/Source/modules/mediastream/RTCVoidRequestImpl.cpp#newcode68
Source/modules/mediastream/RTCVoidRequestImpl.cpp:68:
m_successCallback->handleEvent();
On 2015/05/22 02:00:17, haraken wrote:

> Can we tidy up the logic a bit?

> if (m_requester) {
> m_requester->requestSucceeded(m_requestType);
> if (m_requester->shouldFireDefaultCallbacks() && m_successCallback)
> m_successCallback->handleEvent();
> }

> ?

Done.

https://codereview.chromium.org/1152123002/

har...@chromium.org

unread,
May 22, 2015, 12:45:05 AM5/22/15
to changb...@intel.com, to...@chromium.org, tom...@chromium.org, blink-...@chromium.org, tommyw+w...@chromium.org
LGTM


https://codereview.chromium.org/1152123002/

to...@chromium.org

unread,
May 22, 2015, 4:13:27 AM5/22/15
to changb...@intel.com, har...@chromium.org, tom...@chromium.org, blink-...@chromium.org, tommyw+w...@chromium.org
lgtm



https://codereview.chromium.org/1152123002/

commi...@chromium.org

unread,
May 22, 2015, 4:13:39 AM5/22/15
to changb...@intel.com, to...@chromium.org, har...@chromium.org, tom...@chromium.org, blink-...@chromium.org, tommyw+w...@chromium.org
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1152123002/20001


https://codereview.chromium.org/1152123002/

commi...@chromium.org

unread,
May 22, 2015, 7:35:40 AM5/22/15
to changb...@intel.com, to...@chromium.org, har...@chromium.org, tom...@chromium.org, blink-...@chromium.org, tommyw+w...@chromium.org
Failed to apply patch for Source/modules/mediastream/RTCVoidRequestImpl.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/modules/mediastream/RTCVoidRequestImpl.cpp
Hunk #1 FAILED at 60.
1 out of 1 hunk FAILED -- saving rejects to file
Source/modules/mediastream/RTCVoidRequestImpl.cpp.rej

Patch: Source/modules/mediastream/RTCVoidRequestImpl.cpp
Index: Source/modules/mediastream/RTCVoidRequestImpl.cpp
diff --git a/Source/modules/mediastream/RTCVoidRequestImpl.cpp
b/Source/modules/mediastream/RTCVoidRequestImpl.cpp
index
0b467467dfd90bac4501616c384e6d702295d49b..4e2f37a009085b4892cac74e1040a610dafb6b57
100644
--- a/Source/modules/mediastream/RTCVoidRequestImpl.cpp
+++ b/Source/modules/mediastream/RTCVoidRequestImpl.cpp
@@ -60,11 +60,11 @@ RTCVoidRequestImpl::~RTCVoidRequestImpl()

void RTCVoidRequestImpl::requestSucceeded()
{
- bool shouldFireCallback = m_requester ?
m_requester->shouldFireDefaultCallbacks() : false;
- m_requester->requestSucceeded(m_requestType);
-
- if (shouldFireCallback && m_successCallback)
- m_successCallback->handleEvent();
+ if (m_requester) {
+ m_requester->requestSucceeded(m_requestType);
+ if (m_requester->shouldFireDefaultCallbacks() && m_successCallback)
+ m_successCallback->handleEvent();
+ }

clear();
}


https://codereview.chromium.org/1152123002/
Reply all
Reply to author
Forward
0 new messages