Closed
Bug 1246931
Opened 9 years ago
Closed 9 years ago
Clean up DBus helper code
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
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.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8717422 -
Flags: review?(shuang)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8717423 -
Flags: review?(shuang)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8717424 -
Flags: review?(shuang)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8717425 -
Flags: review?(shuang)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8717428 -
Flags: review?(shuang)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8717429 -
Flags: review?(shuang)
Assignee | ||
Comment 7•9 years ago
|
||
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)
Assignee | ||
Comment 11•9 years ago
|
||
Changes since v1:
- rebased onto bug 1261340
Attachment #8717422 -
Attachment is obsolete: true
Attachment #8739391 -
Flags: review+
Assignee | ||
Comment 12•9 years ago
|
||
Changes since v1:
- shift released pointer into |Unused|; fixes build
Attachment #8717423 -
Attachment is obsolete: true
Flags: needinfo?(tzimmermann)
Attachment #8739392 -
Flags: review+
Assignee | ||
Comment 13•9 years ago
|
||
Changes since v1:
- rebased onto latest m-c
Attachment #8717424 -
Attachment is obsolete: true
Attachment #8739399 -
Flags: review+
Assignee | ||
Comment 14•9 years ago
|
||
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)
Assignee | ||
Comment 15•9 years ago
|
||
Changes since v1:
- specialize |RefPtrTraits| for |DBusMessage|
Attachment #8717428 -
Attachment is obsolete: true
Attachment #8717428 -
Flags: review?(shuang)
Attachment #8739434 -
Flags: review?(shuang)
Assignee | ||
Comment 16•9 years ago
|
||
Changes since v1:
- specialize |RefPtrTraits| for |DBusPendingCall|
Attachment #8717429 -
Attachment is obsolete: true
Attachment #8717429 -
Flags: review?(shuang)
Attachment #8739435 -
Flags: review?(shuang)
Assignee | ||
Comment 17•9 years ago
|
||
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+
Attachment #8739440 -
Flags: review?(shuang)
Assignee | ||
Comment 18•9 years ago
|
||
Comment on attachment 8739440 [details] [diff] [review]
[07] Bug 1246931: Add DBus I/O helpers (v2)
You're not going to review this patch?
Assignee | ||
Updated•9 years ago
|
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)
Assignee | ||
Comment 21•9 years ago
|
||
> @@ +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.
Assignee | ||
Comment 22•9 years ago
|
||
Changes since v2:
- added more assertions
Attachment #8739440 -
Attachment is obsolete: true
Attachment #8740421 -
Flags: review+
Comment 23•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0aa22bba65ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4a77716c6ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/aba4ac8639b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/e615179b56a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/acc96f61b9a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/2af67c46b799
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fb15d5b02de
Assignee | ||
Comment 24•9 years ago
|
||
Comment 25•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0aa22bba65ea
https://hg.mozilla.org/mozilla-central/rev/e4a77716c6ea
https://hg.mozilla.org/mozilla-central/rev/aba4ac8639b1
https://hg.mozilla.org/mozilla-central/rev/e615179b56a2
https://hg.mozilla.org/mozilla-central/rev/acc96f61b9a8
https://hg.mozilla.org/mozilla-central/rev/2af67c46b799
https://hg.mozilla.org/mozilla-central/rev/1fb15d5b02de
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.6 S12 - 4/22
You need to log in
before you can comment on or make changes to this bug.
Description
•