Clean up DBus helper code

RESOLVED FIXED in Firefox 48

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

unspecified
2.6 S12 - 4/22
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(7 attachments, 8 obsolete attachments)

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.
Created attachment 8717422 [details] [diff] [review]
[01] Bug 1246931: Include dbus.h in DBus IPC headers
Attachment #8717422 - Flags: review?(shuang)
Created attachment 8717423 [details] [diff] [review]
[02] Bug 1246931: Move |DBusWatcher| into its own files
Attachment #8717423 - Flags: review?(shuang)
Created attachment 8717424 [details] [diff] [review]
[03] Bug 1246931: Use |DBusConnection| in |DBusWatcher|
Attachment #8717424 - Flags: review?(shuang)
Created attachment 8717425 [details] [diff] [review]
[04] Bug 1246931: Add support for |RefPtr<DBusConnection>| and convert callers
Attachment #8717425 - Flags: review?(shuang)
Created attachment 8717428 [details] [diff] [review]
[05] Bug 1246931: Add support for |RefPtr<DBusMessage>|
Attachment #8717428 - Flags: review?(shuang)
Created attachment 8717429 [details] [diff] [review]
[06] Bug 1246931: Add support for |RefPtr<DBusPendingCall>|
Attachment #8717429 - Flags: review?(shuang)
Created attachment 8717430 [details] [diff] [review]
[07] Bug 1246931: Add DBus I/O helpers
Attachment #8717430 - Flags: review?(shuang)
Attachment #8717422 - Flags: review?(shuang) → review+
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+
Attachment #8717424 - 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)
Flags: needinfo?(tzimmermann)
Created attachment 8739391 [details] [diff] [review]
[01] Bug 1246931: Include dbus.h in DBus IPC headers (v2)

Changes since v1:

  - rebased onto bug 1261340
Attachment #8717422 - Attachment is obsolete: true
Attachment #8739391 - Flags: review+
Created attachment 8739392 [details] [diff] [review]
[02] Bug 1246931: Move |DBusWatcher| into its own files (v2)

Changes since v1:

  - shift released pointer into |Unused|; fixes build
Attachment #8717423 - Attachment is obsolete: true
Flags: needinfo?(tzimmermann)
Attachment #8739392 - Flags: review+
Created attachment 8739399 [details] [diff] [review]
[03] Bug 1246931: Use |DBusConnection| in |DBusWatcher| (v2)

Changes since v1:

  - rebased onto latest m-c
Attachment #8717424 - Attachment is obsolete: true
Attachment #8739399 - Flags: review+
Created attachment 8739433 [details] [diff] [review]
[04] Bug 1246931: Add support for |RefPtr<DBusConnection>| and convert callers (v2)

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)
Created attachment 8739434 [details] [diff] [review]
[05] Bug 1246931: Add support for |RefPtr<DBusMessage>| (v2)

Changes since v1:

  - specialize |RefPtrTraits| for |DBusMessage|
Attachment #8717428 - Attachment is obsolete: true
Attachment #8717428 - Flags: review?(shuang)
Attachment #8739434 - Flags: review?(shuang)
Created attachment 8739435 [details] [diff] [review]
[06] Bug 1246931: Add support for |RefPtr<DBusPendingCall>| (v2)

Changes since v1:

  - specialize |RefPtrTraits| for |DBusPendingCall|
Attachment #8717429 - Attachment is obsolete: true
Attachment #8717429 - Flags: review?(shuang)
Attachment #8739435 - Flags: review?(shuang)
Created attachment 8739440 [details] [diff] [review]
[07] Bug 1246931: Add DBus I/O helpers (v2)

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)
Attachment #8739433 - Flags: review?(shuang) → review+
Attachment #8739434 - Flags: review?(shuang) → review+
Attachment #8739435 - Flags: review?(shuang) → review+
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.
Created attachment 8740421 [details] [diff] [review]
[07] Bug 1246931: Add DBus I/O helpers (v3)

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.