Fix minor issues about DBusThreadManager.

Fix minor issues about DBusThreadManager.

- Remove the global unstub_client_mask_ and accordingly make DBusThreadManager::IsUsingStub a non-static function. This makes it obvious when it's ok to call this function (and prevents misuse), namely only after the Manager is initialized.

- By moving the mask to the DBusClientBundle, the cyclic dependency between bundle and manager is broken. The Bundle does not know about the Manager anymore.

- Fix the difference between DBusClientTypeMask ("A set of DBusClients") and DBusClientType ("A single DBusClient").

BUG=408617
Committed: https://crrev.com/ef009f18ca95c0d7b3b4a330c190a2f24b29e003
Cr-Commit-Position: refs/heads/master@{#294361}

[pneubeck (no reviews), 2014-09-09 11:56:30]

Patchset #1 (id:1) has been deleted

[pneubeck (no reviews), 2014-09-09 12:27:34]

Patchset #1 (id:20001) has been deleted

[pneubeck (no reviews), 2014-09-09 12:36:41]

Patchset #1 (id:40001) has been deleted

[pneubeck (no reviews), 2014-09-09 12:48:14]

pneubeck@chromium.org changed reviewers:
+ hashimoto@chromium.org, satorux@chromium.org

[pneubeck (no reviews), 2014-09-09 12:48:15]

This is smaller cleanup about issues that I stumbled upon while working on the
real CL.

ptal

[satorux1, 2014-09-10 05:25:33]

https://codereview.chromium.org/556833003/diff/60001/chromeos/dbus/bluetooth_...
File chromeos/dbus/bluetooth_adapter_client.cc (right):

https://codereview.chromium.org/556833003/diff/60001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_adapter_client.cc:54: : object_manager_(NULL),
weak_ptr_factory_(this) {}
good catch! thank you for fixing.

https://codereview.chromium.org/556833003/diff/60001/chromeos/dbus/dbus_clien...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/556833003/diff/60001/chromeos/dbus/dbus_clien...
chromeos/dbus/dbus_client_bundle.cc:83:
~static_cast<DBusClientBundle::DBusClientTypeMask>(0);
I don't think it's a good idea to move these outside of DBusClientType. In fact,
GetDBusClientType() becomes a bit complicated because NO_CLIENTS is outside of
DBusClientType.

I think it's a common practice to include 0 as NONE value in bit mask types, so
I'd suggest to move NO_CLIENTS back to DBusClientType. Then, I think it also
makes sense to move ALL_CLIENTS back to DBusClientType.

% git grep -B1 -A2 'NONE = 0'
ash/media_delegate.h-enum MediaCaptureState {
ash/media_delegate.h:  MEDIA_CAPTURE_NONE = 0,
ash/media_delegate.h-  MEDIA_CAPTURE_AUDIO = 1 << 0,
ash/media_delegate.h-  MEDIA_CAPTURE_VIDEO = 1 << 1,
--
ash/system/tray_accessibility.cc-enum AccessibilityState {
ash/system/tray_accessibility.cc:  A11Y_NONE = 0,
ash/system/tray_accessibility.cc-  A11Y_SPOKEN_FEEDBACK = 1 << 0,
ash/system/tray_accessibility.cc-  A11Y_HIGH_CONTRAST = 1 << 1,
--
athena/input/public/accelerator_manager.h-enum AcceleratorFlags {
athena/input/public/accelerator_manager.h:  AF_NONE = 0,
athena/input/public/accelerator_manager.h-  // Used for accelerators that should
not be fired on auto repeated
athena/input/public/accelerator_manager.h-  // key event, such as toggling
fullscrren.
--
base/message_loop/message_pump_dispatcher.h-  enum PostDispatchAction {
base/message_loop/message_pump_dispatcher.h:    POST_DISPATCH_NONE = 0x0,
base/message_loop/message_pump_dispatcher.h-    POST_DISPATCH_QUIT_LOOP = 0x1,
base/message_loop/message_pump_dispatcher.h-    POST_DISPATCH_PERFORM_DEFAULT =
0x2,
...

[hashimoto, 2014-09-10 06:01:28]

https://codereview.chromium.org/556833003/diff/60001/chromeos/dbus/dbus_clien...
File chromeos/dbus/dbus_client_bundle.h (right):

https://codereview.chromium.org/556833003/diff/60001/chromeos/dbus/dbus_clien...
chromeos/dbus/dbus_client_bundle.h:89: // Returns true if any regular DBusClient
is used.
What does "regular" here mean?
DBusClientImplementationType is using the word stub/real.
Please use the same wording.

[pneubeck (no reviews), 2014-09-10 07:58:13]

https://codereview.chromium.org/556833003/diff/60001/chromeos/dbus/bluetooth_...
File chromeos/dbus/bluetooth_adapter_client.cc (right):

https://codereview.chromium.org/556833003/diff/60001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_adapter_client.cc:54: : object_manager_(NULL),
weak_ptr_factory_(this) {}
On 2014/09/10 05:25:33, satorux1 wrote:
> good catch! thank you for fixing.

Acknowledged.

https://codereview.chromium.org/556833003/diff/60001/chromeos/dbus/dbus_clien...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/556833003/diff/60001/chromeos/dbus/dbus_clien...
chromeos/dbus/dbus_client_bundle.cc:83:
~static_cast<DBusClientBundle::DBusClientTypeMask>(0);
I agree that 'None' makes some sense in the enum itself, meaning "This thing has
no type". I'll add it back.

But, I still think that 'ALL' does not belong in the enum.
If we add ALL, then a value of type 'DBusClientType' doesn't refer to a single
Client anymore and the meaning of the function IsUsingStub is becoming more
complicated, e.g. "Returns true if _all_ clients described by 'client_mask' are
stubbed.", or the result of GetDBusClientType is not obvious anymore from its
type, ...

Actually, ALL is used exactly once for constructing a Bundle with only real
clients. Alternatively, we could just get rid of 'ALL' and inline it into the
Bundle constructor.

What do you think?

On 2014/09/10 05:25:33, satorux1 wrote:
> I don't think it's a good idea to move these outside of DBusClientType. In
fact,
> GetDBusClientType() becomes a bit complicated because NO_CLIENTS is outside of
> DBusClientType.
> 
> I think it's a common practice to include 0 as NONE value in bit mask types,
so
> I'd suggest to move NO_CLIENTS back to DBusClientType. Then, I think it also
> makes sense to move ALL_CLIENTS back to DBusClientType.
> 
> % git grep -B1 -A2 'NONE = 0'
> ash/media_delegate.h-enum MediaCaptureState {
> ash/media_delegate.h:  MEDIA_CAPTURE_NONE = 0,
> ash/media_delegate.h-  MEDIA_CAPTURE_AUDIO = 1 << 0,
> ash/media_delegate.h-  MEDIA_CAPTURE_VIDEO = 1 << 1,
> --
> ash/system/tray_accessibility.cc-enum AccessibilityState {
> ash/system/tray_accessibility.cc:  A11Y_NONE = 0,
> ash/system/tray_accessibility.cc-  A11Y_SPOKEN_FEEDBACK = 1 << 0,
> ash/system/tray_accessibility.cc-  A11Y_HIGH_CONTRAST = 1 << 1,
> --
> athena/input/public/accelerator_manager.h-enum AcceleratorFlags {
> athena/input/public/accelerator_manager.h:  AF_NONE = 0,
> athena/input/public/accelerator_manager.h-  // Used for accelerators that
should
> not be fired on auto repeated
> athena/input/public/accelerator_manager.h-  // key event, such as toggling
> fullscrren.
> --
> base/message_loop/message_pump_dispatcher.h-  enum PostDispatchAction {
> base/message_loop/message_pump_dispatcher.h:    POST_DISPATCH_NONE = 0x0,
> base/message_loop/message_pump_dispatcher.h-    POST_DISPATCH_QUIT_LOOP = 0x1,
> base/message_loop/message_pump_dispatcher.h-    POST_DISPATCH_PERFORM_DEFAULT
=
> 0x2,
> ...

https://codereview.chromium.org/556833003/diff/60001/chromeos/dbus/dbus_clien...
File chromeos/dbus/dbus_client_bundle.h (right):

https://codereview.chromium.org/556833003/diff/60001/chromeos/dbus/dbus_clien...
chromeos/dbus/dbus_client_bundle.h:89: // Returns true if any regular DBusClient
is used.
On 2014/09/10 06:01:28, hashimoto wrote:
> What does "regular" here mean?
> DBusClientImplementationType is using the word stub/real.
> Please use the same wording.

Regular was used in the function name 'InitializeRegular'
I agree, it's not a self-explaining name. I'll replace all occurrences of
Regular by Real.

[pneubeck (no reviews), 2014-09-10 21:35:10]

ptal

[satorux1, 2014-09-11 04:23:30]

LGTM

https://codereview.chromium.org/556833003/diff/60001/chromeos/dbus/dbus_clien...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/556833003/diff/60001/chromeos/dbus/dbus_clien...
chromeos/dbus/dbus_client_bundle.cc:83:
~static_cast<DBusClientBundle::DBusClientTypeMask>(0);
On 2014/09/10 07:58:13, pneubeck wrote:
> I agree that 'None' makes some sense in the enum itself, meaning "This thing
has
> no type". I'll add it back.
> 
> But, I still think that 'ALL' does not belong in the enum.
> If we add ALL, then a value of type 'DBusClientType' doesn't refer to a single
> Client anymore and the meaning of the function IsUsingStub is becoming more
> complicated, e.g. "Returns true if _all_ clients described by 'client_mask'
are
> stubbed.", or the result of GetDBusClientType is not obvious anymore from its
> type, ...
> 
> Actually, ALL is used exactly once for constructing a Bundle with only real
> clients. Alternatively, we could just get rid of 'ALL' and inline it into the
> Bundle constructor.
> 
> What do you think?

SGTM

> 
> On 2014/09/10 05:25:33, satorux1 wrote:
> > I don't think it's a good idea to move these outside of DBusClientType. In
> fact,
> > GetDBusClientType() becomes a bit complicated because NO_CLIENTS is outside
of
> > DBusClientType.
> > 
> > I think it's a common practice to include 0 as NONE value in bit mask types,
> so
> > I'd suggest to move NO_CLIENTS back to DBusClientType. Then, I think it also
> > makes sense to move ALL_CLIENTS back to DBusClientType.
> > 
> > % git grep -B1 -A2 'NONE = 0'
> > ash/media_delegate.h-enum MediaCaptureState {
> > ash/media_delegate.h:  MEDIA_CAPTURE_NONE = 0,
> > ash/media_delegate.h-  MEDIA_CAPTURE_AUDIO = 1 << 0,
> > ash/media_delegate.h-  MEDIA_CAPTURE_VIDEO = 1 << 1,
> > --
> > ash/system/tray_accessibility.cc-enum AccessibilityState {
> > ash/system/tray_accessibility.cc:  A11Y_NONE = 0,
> > ash/system/tray_accessibility.cc-  A11Y_SPOKEN_FEEDBACK = 1 << 0,
> > ash/system/tray_accessibility.cc-  A11Y_HIGH_CONTRAST = 1 << 1,
> > --
> > athena/input/public/accelerator_manager.h-enum AcceleratorFlags {
> > athena/input/public/accelerator_manager.h:  AF_NONE = 0,
> > athena/input/public/accelerator_manager.h-  // Used for accelerators that
> should
> > not be fired on auto repeated
> > athena/input/public/accelerator_manager.h-  // key event, such as toggling
> > fullscrren.
> > --
> > base/message_loop/message_pump_dispatcher.h-  enum PostDispatchAction {
> > base/message_loop/message_pump_dispatcher.h:    POST_DISPATCH_NONE = 0x0,
> > base/message_loop/message_pump_dispatcher.h-    POST_DISPATCH_QUIT_LOOP =
0x1,
> > base/message_loop/message_pump_dispatcher.h-   
POST_DISPATCH_PERFORM_DEFAULT
> =
> > 0x2,
> > ...
>

[hashimoto, 2014-09-11 06:50:46]

lgtm

https://codereview.chromium.org/556833003/diff/100001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.h (right):

https://codereview.chromium.org/556833003/diff/100001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.h:158:
DBusThreadManager(scoped_ptr<DBusClientBundle> client_bundle);
'explicit' missing?

https://codereview.chromium.org/556833003/diff/100001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.h:181: // creates a stub network environment.
This comment is not always true as DBusClientBundle::SetupDefaultEnvironment()
results in no-op on the production environment.
Also, this comment is fragile to changes in the implementation of
DBusClientBundle.

I'd just say something like "... and performs additional setup."

[pneubeck (no reviews), 2014-09-11 08:07:53]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-09-11 08:08:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/556833003/120001

[pneubeck (no reviews), 2014-09-11 08:08:19]

https://codereview.chromium.org/556833003/diff/100001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.h (right):

https://codereview.chromium.org/556833003/diff/100001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.h:158:
DBusThreadManager(scoped_ptr<DBusClientBundle> client_bundle);
On 2014/09/11 06:50:45, hashimoto wrote:
> 'explicit' missing?

hm. wasn't catched by presubmit.
Done.

https://codereview.chromium.org/556833003/diff/100001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.h:181: // creates a stub network environment.
On 2014/09/11 06:50:45, hashimoto wrote:
> This comment is not always true as DBusClientBundle::SetupDefaultEnvironment()
> results in no-op on the production environment.
> Also, this comment is fragile to changes in the implementation of
> DBusClientBundle.
> 
> I'd just say something like "... and performs additional setup."

Done.

[commit-bot: I haz the power, 2014-09-11 09:09:39]

Committed patchset #4 (id:120001) as d8ea9768d7fb138307c5d3b5a4faf59be28744e1

[commit-bot: I haz the power, 2014-09-11 09:11:39]

Patchset 4 (id:??) landed as
https://crrev.com/ef009f18ca95c0d7b3b4a330c190a2f24b29e003
Cr-Commit-Position: refs/heads/master@{#294361}