Stealing this :)

This doesn't actually help with bug 981845.

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.

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.

(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.

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.

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.

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

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.

Changes since v3: - cleaned up TRACE macros - removed cache cleanup from destructor

Thanks for the review. https://hg.mozilla.org/integration/b2g-inbound/rev/ad6b7fe315bd https://hg.mozilla.org/integration/b2g-inbound/rev/0515ab940ee6 https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=0515ab940ee6

https://hg.mozilla.org/mozilla-central/rev/ad6b7fe315bd https://hg.mozilla.org/mozilla-central/rev/0515ab940ee6