Closed Bug 1246931 Opened 8 years ago Closed 8 years ago

Clean up DBus helper code

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox48 fixed)

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

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(7 files, 8 obsolete files)

[01] Bug 1246931: Include dbus.h in DBus IPC headers (v2)
939 bytes, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[02] Bug 1246931: Move |DBusWatcher| into its own files (v2)
10.11 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[03] Bug 1246931: Use |DBusConnection| in |DBusWatcher| (v2)
2.97 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[04] Bug 1246931: Add support for |RefPtr<DBusConnection>| and convert callers (v2)
3.58 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
[05] Bug 1246931: Add support for |RefPtr<DBusMessage>| (v2)
1.54 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
[06] Bug 1246931: Add support for |RefPtr<DBusPendingCall>| (v2)
1.65 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
[07] Bug 1246931: Add DBus I/O helpers (v3)
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

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: