Last Comment Bug 735305 - Update WebTelephony impl to use the new DOMEventTargetHelper API
: Update WebTelephony impl to use the new DOMEventTargetHelper API
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla14
Assigned To: [:fabrice] Fabrice Desré
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: webtelephony 719491 734057
  Show dependency treegraph
 
Reported: 2012-03-13 10:23 PDT by Olli Pettay [:smaug]
Modified: 2012-03-13 17:17 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.17 KB, patch)
2012-03-13 11:41 PDT, [:fabrice] Fabrice Desré
bugs: feedback+
Details | Diff | Splinter Review
patch v2 (3.12 KB, patch)
2012-03-13 12:14 PDT, [:fabrice] Fabrice Desré
bent.mozilla: review+
bugs: review+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2012-03-13 10:23:53 PDT

    
Comment 1 Philipp von Weitershausen [:philikon] 2012-03-13 11:20:39 PDT
smaug, which ones are you specifically talking about?
Comment 2 Olli Pettay [:smaug] 2012-03-13 11:23:18 PDT
Ask fabrice. I'm not familiar with b2g code, but apparently there are some
classes which extend nsDOMEventTargetHelper.
Comment 3 [:fabrice] Fabrice Desré 2012-03-13 11:41:59 PDT
Created attachment 605466 [details] [diff] [review]
patch

This patch fixes the build issues, but I'm not sure *at all* it's doing the right thing.
Comment 4 Olli Pettay [:smaug] 2012-03-13 11:47:50 PDT
Comment on attachment 605466 [details] [diff] [review]
patch

>+++ b/dom/telephony/Telephony.h
>@@ -118,16 +118,12 @@ public:
>     return mRIL;
>   }
> 
>-  nsPIDOMWindow*
>-  Owner() const
>+  nsIScriptContext*
>+  ScriptContext()
>   {
>-    return mOwner;
>-  }
>-
>-  nsIScriptContext*
>-  ScriptContext() const
>-  {
>-    return mScriptContext;
>+    nsresult rv;
>+    nsIScriptContext* sc = GetContextForEventHandlers(&rv);
>+    return sc;
>   }
I don't know why you some times use GetContextForEventHandlers, and yet have
this helper method.


> 
>   nsRefPtr<TelephonyCall> call = new TelephonyCall();
> 
>-  call->mOwner = aTelephony->Owner();
>-  call->mScriptContext = aTelephony->ScriptContext();
>   call->mTelephony = aTelephony;
>   call->mNumber = aNumber;
You don't bind the object to owner?
Comment 5 [:fabrice] Fabrice Desré 2012-03-13 12:14:49 PDT
Created attachment 605475 [details] [diff] [review]
patch v2

updated patch, thanks Olli for the quick review on the first one!
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-03-13 15:00:45 PDT
Comment on attachment 605475 [details] [diff] [review]
patch v2

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

r=me with this addressed:

::: dom/telephony/Telephony.cpp
@@ +329,5 @@
>    }
>  
> +  nsresult rv;
> +  nsIScriptContext* sc = GetContextForEventHandlers(&rv);
> +  if (NS_SUCCEEDED(rv) && sc) {

Why not do NS_ENSURE_SUCCESS(rv, rv) here?

@@ +346,5 @@
>    JSObject* calls = mCallsArray;
>    if (!calls) {
> +    nsresult rv;
> +    nsIScriptContext* sc = GetContextForEventHandlers(&rv);
> +    if (NS_SUCCEEDED(rv) && sc) {

Here too.
Comment 7 [:fabrice] Fabrice Desré 2012-03-13 15:12:39 PDT
https://hg.mozilla.org/mozilla-central/rev/c71845b3b2a6

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