Closed Bug 1246931 Opened 9 years ago Closed 9 years ago

Clean up DBus helper code

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
2.6 S12 - 4/22
Tracking Status
firefox48 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(7 files, 8 obsolete files)

939 bytes, patch
tzimmermann
: review+
Details | Diff | Splinter Review
10.11 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
2.97 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
3.58 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
1.54 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
1.65 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
15.96 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
We should clean up the DBus IPC helpers in ipc/dbus. If Firefox OS moves to Linux, we'll probably need this code more often.
Attachment #8717430 - Flags: review?(shuang)
Comment on attachment 8717423 [details] [diff] [review] [02] Bug 1246931: Move |DBusWatcher| into its own files Review of attachment 8717423 [details] [diff] [review]: ----------------------------------------------------------------- r=me, this looks like move things around.
Attachment #8717423 - Flags: review?(shuang) → review+
Comment on attachment 8717425 [details] [diff] [review] [04] Bug 1246931: Add support for |RefPtr<DBusConnection>| and convert callers Review of attachment 8717425 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/dbus/DBusConnectionRefPtr.h @@ +25,5 @@ > + } > +}; > + > +template<> template<> > +struct RefPtr<DBusConnection>::AddRefTraits<const DBusConnection> https://dxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h?from=mfbt%2FRefPtr.h#383 Do we need to have const version? I guess const will be removed.
Comment on attachment 8717425 [details] [diff] [review] [04] Bug 1246931: Add support for |RefPtr<DBusConnection>| and convert callers Review of attachment 8717425 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/dbus/DBusConnectionRefPtr.h @@ +25,5 @@ > + } > +}; > + > +template<> template<> > +struct RefPtr<DBusConnection>::AddRefTraits<const DBusConnection> https://dxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h?from=mfbt%2FRefPtr.h#30
Attachment #8717425 - Flags: review?(shuang)
Changes since v1: - rebased onto bug 1261340
Attachment #8717422 - Attachment is obsolete: true
Attachment #8739391 - Flags: review+
Changes since v1: - shift released pointer into |Unused|; fixes build
Attachment #8717423 - Attachment is obsolete: true
Flags: needinfo?(tzimmermann)
Attachment #8739392 - Flags: review+
Changes since v1: - rebased onto latest m-c
Attachment #8717424 - Attachment is obsolete: true
Attachment #8739399 - Flags: review+
Shawn, there's now |RefPtrTraits| for adapting the ref-counting functions of |RefPtr|. I updated the patch sets [04] to [06] accordingly. |ConstRemovingRefPtrTraits| is in internal helper. I don't think it has to be modified. Changes since v1: - specialize |RefPtrTraits| for DBusConnection
Attachment #8717425 - Attachment is obsolete: true
Attachment #8739433 - Flags: review?(shuang)
Changes since v1: - specialize |RefPtrTraits| for |DBusMessage|
Attachment #8717428 - Attachment is obsolete: true
Attachment #8717428 - Flags: review?(shuang)
Attachment #8739434 - Flags: review?(shuang)
Changes since v1: - specialize |RefPtrTraits| for |DBusPendingCall|
Attachment #8717429 - Attachment is obsolete: true
Attachment #8717429 - Flags: review?(shuang)
Attachment #8739435 - Flags: review?(shuang)
Changes since v1: - use DBus helper functions in |RawDBusConnection|
Attachment #8717430 - Attachment is obsolete: true
Attachment #8717430 - Flags: review?(shuang)
Attachment #8739440 - Flags: review?(shuang)
Comment on attachment 8739440 [details] [diff] [review] [07] Bug 1246931: Add DBus I/O helpers (v2) You're not going to review this patch?
Flags: needinfo?(shuang)
Comment on attachment 8739440 [details] [diff] [review] [07] Bug 1246931: Add DBus I/O helpers (v2) Review of attachment 8739440 [details] [diff] [review]: ----------------------------------------------------------------- r=me, with additional nit addressed. ::: ipc/dbus/DBusHelpers.cpp @@ +101,5 @@ > + > +nsresult > +DBusWatchConnection(DBusConnection* aConnection) > +{ > + MOZ_ASSERT(!NS_IsMainThread()); MOZ_ASSERT(aConnection); @@ +120,5 @@ > + > +void > +DBusUnwatchConnection(DBusConnection* aConnection) > +{ > + MOZ_ASSERT(!NS_IsMainThread()); MOZ_ASSERT(aConnection); @@ +127,5 @@ > + nullptr, nullptr, nullptr, > + nullptr, nullptr); > + if (!success) { > + CHROMIUM_LOG("dbus_connection_set_watch_functions failed"); > + } Why not return 'NS_ERROR_FAILURE'? @@ +196,5 @@ > + const char* aFunc, > + int aFirstArgType, > + va_list aArgs) > +{ > + MOZ_ASSERT(!NS_IsMainThread()); MOZ_ASSERT(aConnection); MOZ_ASSERT(aMessage); @@ +219,5 @@ > + const char* aIntf, > + const char* aFunc, > + int aFirstArgType, > + ...) > +{ MOZ_ASSERT(aConnection); MOZ_ASSERT(aCallback);
Attachment #8739440 - Flags: review+
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #18) > Comment on attachment 8739440 [details] [diff] [review] > [07] Bug 1246931: Add DBus I/O helpers (v2) > > You're not going to review this patch? Sorry. I'm not sure why the browser doesn't send my local draft.
Flags: needinfo?(shuang)
> @@ +127,5 @@ > > + nullptr, nullptr, nullptr, > > + nullptr, nullptr); > > + if (!success) { > > + CHROMIUM_LOG("dbus_connection_set_watch_functions failed"); > > + } > > Why not return 'NS_ERROR_FAILURE'? There isn't much that we can do. So I didn't bother with returning a value.
Changes since v2: - added more assertions
Attachment #8739440 - Attachment is obsolete: true
Attachment #8740421 - Flags: review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: