Closed Bug 1246931 Opened 4 years ago Closed 4 years ago

Clean up DBus helper code

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

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.