Move mozIccManager to WebIDL

RESOLVED FIXED in 2.0 S1 (9may)

Status

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: edgar, Assigned: tzimmermann)

Tracking

unspecified
2.0 S1 (9may)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.0, tracking-b2g:backlog)

Details

Attachments

(2 attachments, 3 obsolete attachments)

No description provided.
Assignee: nobody → allstars.chh
Blocks: 888591
Blocks: 981845
Stealing this :)
Assignee: allstars.chh → tzimmermann
Attachment #8404030 - Flags: review?(htsai)
Attachment #8404030 - Flags: review?(bugs)
This doesn't actually help with bug 981845.
No longer blocks: 981845
Attachment #8404027 - Flags: review?(bugs) → review+
Comment on attachment 8404030 [details] [diff] [review]
[02] Bug 904588: Convert MozIccManager to WebIDL


>+IccManager*
> Navigator::GetMozIccManager(ErrorResult& aRv)
> {
>   if (!mIccManager) {
>     if (!mWindow) {
>       aRv.Throw(NS_ERROR_UNEXPECTED);
>       return nullptr;
>     }
>     NS_ENSURE_TRUE(mWindow->GetDocShell(), nullptr);
> 
>-    mIccManager = new IccManager(mWindow);
>+    mIccManager = IccManager::Create(mWindow, aRv);
I don't understand why you add ::Create.
mIccManager = new IccManager(mWindow);
should be fine.

>+JS::Value
>+IccManager::GetIccIds(JSContext* aContext, ErrorResult& aRv)
> {
>   if (!mJsIccIds) {
>     nsTArray<nsString> iccIds;
>     for (uint32_t i = 0; i < mIccListeners.Length(); i++) {
>       nsRefPtr<Icc> icc = mIccListeners[i]->GetIcc();
>       if (icc) {
>         iccIds.AppendElement(icc->GetIccId());
>       }
>     }
> 
>     nsresult rv;
>     nsIScriptContext* sc = GetContextForEventHandlers(&rv);
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      aRv.Throw(rv);
>+      return JS::NullValue();
>+    }
> 
>     AutoPushJSContext cx(sc->GetNativeContext());
>     JS::Rooted<JSObject*> jsIccIds(cx);
>     rv = nsTArrayToJSArray(cx, iccIds, jsIccIds.address());
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      aRv.Throw(rv);
>+      return JS::NullValue();
>+    }
> 
>     mJsIccIds = jsIccIds;
>     Root();
>   }
This all would be a lot simpler if .webidl
had something like
[Cached, Pure] attribute sequence<DOMString> iccIds

>diff --git a/dom/webidl/MozIccManager.webidl b/dom/webidl/MozIccManager.webidl
The patch would be smaller and easier to review if you
would just rename the .idl to dom/webidl/MozIccManager.webidl and then make the changes.
(.idl and .webidl are almost the same in this case)


So, kind of r+, but would like to see a new patch with those changes.
Attachment #8404030 - Flags: review?(bugs) → review-
Comment on attachment 8404030 [details] [diff] [review]
[02] Bug 904588: Convert MozIccManager to WebIDL

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

::: dom/webidl/MozIccManager.webidl
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +interface MozIccManager : EventTarget

Please add [Pref="dom.icc.enabled"] for MozIccManager interface, thank you. :)
Comment on attachment 8404030 [details] [diff] [review]
[02] Bug 904588: Convert MozIccManager to WebIDL

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

r=me for the .webidl and .idl with the comment addressed. Thank you.

::: dom/webidl/MozIccManager.webidl
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +interface MozIccManager : EventTarget

This should be guarded by a pref.
Please add [Pref="dom.icc.enabled"] above.
Attachment #8404030 - Flags: review?(htsai) → review+
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #7)
> Comment on attachment 8404030 [details] [diff] [review]
> [02] Bug 904588: Convert MozIccManager to WebIDL
> 
> Review of attachment 8404030 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me for the .webidl and .idl with the comment addressed. Thank you.
> 
> ::: dom/webidl/MozIccManager.webidl
> @@ +2,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +interface MozIccManager : EventTarget
> 
> This should be guarded by a pref.
> Please add [Pref="dom.icc.enabled"] above.

Oh, forgot to say: after this, we are eventually able to move bug 819831 forward which requires communication with gaia first.
Changes since v1:

  - removed 'scope' argument from |IccManager::WrapObject| (see bug 991742)
  - created WebIDL file by renaming IDL file
  - removed |IccManager::Create|
  - protect |MozIccManager| interface by [Pref="dom.icc.enabled"]

Regarding |Create|, I noticed the method in several other implementations and thought it is a common pattern. Further grep'ing revealed that it's only used by a handful of classes. It's been removed from the patch.
Attachment #8404030 - Attachment is obsolete: true
Attachment #8404586 - Flags: review?(bugs)
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
So why don't you use [Cached, Pure] attribute sequence<DOMString> iccIds ?
Oh, I see, it can get a new value, so you want to be able to clear the cache.
See ClearCachedFooValue https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Cached
Comment on attachment 8404586 [details] [diff] [review]
[02] Bug 904588: Convert MozIccManager to WebIDL (v2)

So, I'd like to see that change. Getting rid of some manual JSAPI usage
is always good.
Attachment #8404586 - Flags: review?(bugs) → review-
Comment on attachment 8404586 [details] [diff] [review]
[02] Bug 904588: Convert MozIccManager to WebIDL (v2)

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

Sorry for jumping in.

::: dom/webidl/MozIccManager.webidl
@@ +238,5 @@
>     *        The identifier of the ICC.
>     *
>     * @return see MozIcc.webidl for the detail.
>     */
> +  nsISupports getIccById(DOMString iccId);

MozIcc was already WebIDL-ized, we could use MozIcc here.
Thank you. :)
(In reply to Olli Pettay [:smaug] from comment #10)
> So why don't you use [Cached, Pure] attribute sequence<DOMString> iccIds ?

I misunderstood your review comment. I'll update the patch soon.
(In reply to Edgar Chen [:edgar][:echen] from comment #13)
> Comment on attachment 8404586 [details] [diff] [review]
> [02] Bug 904588: Convert MozIccManager to WebIDL (v2)
> 
> Review of attachment 8404586 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for jumping in.
> 
> ::: dom/webidl/MozIccManager.webidl
> @@ +238,5 @@
> >     *        The identifier of the ICC.
> >     *
> >     * @return see MozIcc.webidl for the detail.
> >     */
> > +  nsISupports getIccById(DOMString iccId);
> 
> MozIcc was already WebIDL-ized, we could use MozIcc here.
> Thank you. :)

I'd prefer to not change the interface's semantics in this patch. I'll submit a follow-up bug if that's OK.
blocking-b2g: --- → backlog
Changes since v2:

  - declare iccIds with [Cached,Pure]
  - use ClearCachedIccIdsValue for free'ing old cached values
  - there are no JS values in IccManager, so I removed rooting functions
Attachment #8404586 - Attachment is obsolete: true
Attachment #8406800 - Flags: review?(bugs)
Comment on attachment 8406800 [details] [diff] [review]
[02] Bug 904588: Convert MozIccManager to WebIDL (v3)


> NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(IccManager,
>                                                DOMEventTargetHelper)
>-  NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mJsIccIds)
>   // We did not setup 'mIccListeners' being a participant of cycle collection is
>   // because in Navigator->Invalidate() it will call mIccManager->Shutdown(),
>   // then IccManager will call Shutdown() of each IccListener, this will release
>   // the reference that held by each mIccListener and break the cycle.
> NS_IMPL_CYCLE_COLLECTION_TRACE_END
You can now remove all this _TRACE_ if you 
in class declaration change NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED
to be NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED

No need to clear the cached value in dtor.
Attachment #8406800 - Flags: review?(bugs) → review+
Changes since v3:

  - cleaned up TRACE macros
  - removed cache cleanup from destructor
Attachment #8406800 - Attachment is obsolete: true
Attachment #8407364 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ad6b7fe315bd
https://hg.mozilla.org/mozilla-central/rev/0515ab940ee6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
feature-b2g: --- → 2.0
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.