Check Firewall state only initializing mDns from PrintPreview.

Check Firewall state only initializing mDns from PrintPreview.

BUG=366408
Committed: https://crrev.com/13d9e53bd729f1c60152136beae54d8f1a410008
Cr-Commit-Position: refs/heads/master@{#292200}

[Vitaly Buka (NO REVIEWS), 2014-08-27 01:07:02]

vitalybuka@chromium.org changed reviewers:
+ noamsml@chromium.org

[Noam Samuel, 2014-08-27 17:58:56]

lgtm

https://codereview.chromium.org/504223005/diff/60001/chrome/browser/local_dis...
File chrome/browser/local_discovery/service_discovery_shared_client.cc (right):

https://codereview.chromium.org/504223005/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/service_discovery_shared_client.cc:87: return new
ServiceDiscoveryClientMdns();
Maybe do

if (g_is_firewall_state_reported) {
 return g_is_firewall_ready ? new ServiceDiscoveryClientMdns() : new
ServiceDiscoveryClientUtility();
} else {
 return new ServiceDiscoveryClientUtility();
}

here so users don't get two different ServiceDiscoveryClients? Also if we do
this and then migrate notifications, GetInstance() will almost always do the
right thing.

https://codereview.chromium.org/504223005/diff/60001/chrome/browser/local_dis...
File chrome/browser/local_discovery/service_discovery_shared_client.h (right):

https://codereview.chromium.org/504223005/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/service_discovery_shared_client.h:16: static
scoped_refptr<ServiceDiscoverySharedClient> GetInstance();
Maybe add a comment here that this is deprecated and a todo to remove it? Or do
you think this should stay?

[Vitaly Buka (NO REVIEWS), 2014-08-27 18:08:27]

https://codereview.chromium.org/504223005/diff/60001/chrome/browser/local_dis...
File chrome/browser/local_discovery/service_discovery_shared_client.cc (right):

https://codereview.chromium.org/504223005/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/service_discovery_shared_client.cc:87: return new
ServiceDiscoveryClientMdns();
Goal is always use ServiceDiscoveryClientMdns. Only preview is exceptional.

On 2014/08/27 17:58:56, Noam Samuel wrote:
> Maybe do
> 
> if (g_is_firewall_state_reported) {
>  return g_is_firewall_ready ? new ServiceDiscoveryClientMdns() : new
> ServiceDiscoveryClientUtility();
> } else {
>  return new ServiceDiscoveryClientUtility();
> }
> 
> here so users don't get two different ServiceDiscoveryClients? Also if we do
> this and then migrate notifications, GetInstance() will almost always do the
> right thing.

https://codereview.chromium.org/504223005/diff/60001/chrome/browser/local_dis...
File chrome/browser/local_discovery/service_discovery_shared_client.h (right):

https://codereview.chromium.org/504223005/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/service_discovery_shared_client.h:16: static
scoped_refptr<ServiceDiscoverySharedClient> GetInstance();
Actually this is still primary call.
GetInstanceWithoutAlert would be gone in future.

On 2014/08/27 17:58:56, Noam Samuel wrote:
> Maybe add a comment here that this is deprecated and a todo to remove it? Or
do
> you think this should stay?

[Vitaly Buka (NO REVIEWS), 2014-08-27 18:08:33]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-08-27 18:09:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/504223005/60001

[commit-bot: I haz the power, 2014-08-27 19:01:03]

Committed patchset #4 (id:60001) as dba7fecc8fc61969dba15fd6d57ca3cf0aff843d

[commit-bot: I haz the power, 2014-09-10 02:53:16]

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