Closed
Bug 904588
Opened 11 years ago
Closed 10 years ago
Move mozIccManager to WebIDL
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(feature-b2g:2.0, tracking-b2g:backlog)
People
(Reporter: edgar, Assigned: tzimmermann)
References
Details
Attachments
(2 files, 3 obsolete files)
3.93 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
18.16 KB,
patch
|
tzimmermann
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee: nobody → allstars.chh
Blocks: 888591
Blocks: 819831
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8404027 -
Flags: review?(bugs)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8404030 -
Flags: review?(htsai)
Attachment #8404030 -
Flags: review?(bugs)
Updated•10 years ago
|
Attachment #8404027 -
Flags: review?(bugs) → review+
Comment 5•10 years ago
|
||
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-
Reporter | ||
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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+
Comment 8•10 years ago
|
||
(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.
Assignee | ||
Comment 9•10 years ago
|
||
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)
Reporter | ||
Updated•10 years ago
|
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
Comment 10•10 years ago
|
||
So why don't you use [Cached, Pure] attribute sequence<DOMString> iccIds ?
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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-
Reporter | ||
Comment 13•10 years ago
|
||
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. :)
Assignee | ||
Comment 14•10 years ago
|
||
(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.
Assignee | ||
Comment 15•10 years ago
|
||
(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.
Updated•10 years ago
|
blocking-b2g: --- → backlog
Assignee | ||
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
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+
Assignee | ||
Comment 18•10 years ago
|
||
Changes since v3: - cleaned up TRACE macros - removed cache cleanup from destructor
Attachment #8406800 -
Attachment is obsolete: true
Attachment #8407364 -
Flags: review+
Assignee | ||
Comment 19•10 years ago
|
||
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
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ad6b7fe315bd https://hg.mozilla.org/mozilla-central/rev/0515ab940ee6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
Updated•10 years ago
|
feature-b2g: --- → 2.0
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•