Fixes the interactive default browser UX for policy setting

chrome/browser/shell_integration.h

Index: chrome/browser/shell_integration.h
diff --git a/chrome/browser/shell_integration.h b/chrome/browser/shell_integration.h
index f2c629a57ae819495dcdee1ce6e4c02b86bcb567..2c031846384f210d31295c603d29e800341cf430 100644
--- a/chrome/browser/shell_integration.h
+++ b/chrome/browser/shell_integration.h
@@ -201,12 +201,6 @@ class ShellIntegration {
     // Called to notify the UI of the immediate result of invoking
     // SetAsDefault.
     virtual void OnSetAsDefaultConcluded(bool succeeded) {}
-    // Observer classes that return true to OwnedByWorker are automatically
-    // freed by the worker when they are no longer needed. False by default.
-    virtual bool IsOwnedByWorker();
-    // An observer can permit or decline set-as-default operation if it
-    // requires triggering user interaction. By default not allowed.
-    virtual bool IsInteractiveSetDefaultPermitted();
   };
 
   //  Helper objects that handle checking if Chrome is the default browser
@@ -214,10 +208,25 @@ class ShellIntegration {
   //  it as the default. These operations are performed asynchronously on the
   //  file thread since registry access (on Windows) or the preference database
   //  (on Linux) are involved and this can be slow.
+  //  By default, the worker will present the user with an interactive flow if
+  //  required by the platform. This can be suppressed via
+  //  set_interactive_permitted(), in which case an attempt to set Chrome as
+  //  the default handler will silently fail on such platforms.
   class DefaultWebClientWorker
       : public base::RefCountedThreadSafe<DefaultWebClientWorker> {
    public:
-    explicit DefaultWebClientWorker(DefaultWebClientObserver* observer);
+    // Constructor. The worker will post updates to |observer|. If
+    // |delete_observer| is true, the worker owns the observer and it will be
+    // freed in the destructor.

[Nico, 2016/02/09 03:01:31]

I find "optionally owned" code is usually hard to reason about. is it not
possible to have clear ownership semantics here? can this cass own observer
either always (in that case, pass a scoped_ptr/unique_ptr) or never?

[Peter Kasting, 2016/02/09 06:04:22]

I agree.

In the limit, two constructors (raw pointer and scoped_ptr) would be better than
a raw pointer + "take ownership" bool.  But a consistent model would be even
better.

Either one will also avoid Nico's complaint about bool args.

[Patrick Monette, 2016/02/09 17:15:05]

Yeah I also agree and I'm working on fixing this is another CL to limit the size
of the code reviews.
It will change the observer to a single callback owned by the
DefaultWebClientWorker class that can be invalidated via weak_ptrs.

+    DefaultWebClientWorker(DefaultWebClientObserver* observer,
+                           bool delete_observer);
+
+    // Controls whether the worker can use user interaction to set the default
+    // web client. If false, the set-as-default operation will fail on OS where
+    // it is required.
+    void set_interactive_permitted(bool interactive_permitted) {
+      interactive_permitted_ = interactive_permitted;
+    }
 
     // Checks to see if Chrome is the default web client application. The result
     // will be passed back to the observer via the SetDefaultWebClientUIState
@@ -281,6 +290,10 @@ class ShellIntegration {
       return set_as_default_initialized_;
     }
 
+    // When false, the operation to set as default will fail for interactive
+    // flows.
+    bool interactive_permitted_ = true;
+
     // Flag that indicates if the set-as-default operation is in progess to
     // prevent multiple notifications to the observer.
     bool set_as_default_in_progress_ = false;
@@ -292,10 +305,9 @@ class ShellIntegration {
     virtual void CheckIsDefault() = 0;
 
     // Sets Chrome as the default web client. Always called on the FILE thread.
-    // |interactive_permitted| will make SetAsDefault() fail if it requires
-    // interaction with the user. Subclasses are responsible for calling
-    // OnSetAsDefaultAttemptComplete() on the UI thread.
-    virtual void SetAsDefault(bool interactive_permitted) = 0;
+    // Subclasses are responsible for calling OnSetAsDefaultAttemptComplete() on
+    // the UI thread.
+    virtual void SetAsDefault() = 0;
 
     // Returns the prefix used for metrics to differentiate UMA metrics for
     // setting the default browser and setting the default protocol client.
@@ -325,6 +337,9 @@ class ShellIntegration {
 
     DefaultWebClientObserver* observer_;
 
+    // Indicates if the the observer will be automatically freed by the worker.
+    bool delete_observer_;
+
     // Flag that indicates the return value of InitializeSetAsDefault(). If
     // true, FinalizeSetAsDefault() will be called to clear what was
     // initialized.
@@ -343,7 +358,11 @@ class ShellIntegration {
   // Worker for checking and setting the default browser.
   class DefaultBrowserWorker : public DefaultWebClientWorker {
    public:
-    explicit DefaultBrowserWorker(DefaultWebClientObserver* observer);
+    // Constructor. The worker will post updates to |observer|. If
+    // |delete_observer| is true, the worker owns the observer and it will be
+    // freed in the destructor.
+    DefaultBrowserWorker(DefaultWebClientObserver* observer,
+                         bool delete_observer);
 
    private:
     ~DefaultBrowserWorker() override;
@@ -352,7 +371,7 @@ class ShellIntegration {
     void CheckIsDefault() override;
 
     // Set Chrome as the default browser.
-    void SetAsDefault(bool interactive_permitted) override;
+    void SetAsDefault() override;
 
     // Returns the histogram prefix for DefaultBrowserWorker.
     const char* GetHistogramPrefix() override;
@@ -383,8 +402,12 @@ class ShellIntegration {
   // multiple protocols you should use multiple worker objects.
   class DefaultProtocolClientWorker : public DefaultWebClientWorker {
    public:
+    // Constructor. The worker will post updates to |observer|. If
+    // |delete_observer| is true, the worker owns the observer and it will be
+    // freed in the destructor.
     DefaultProtocolClientWorker(DefaultWebClientObserver* observer,
-                                const std::string& protocol);
+                                const std::string& protocol,
+                                bool delete_observer);
 
     const std::string& protocol() const { return protocol_; }
 
@@ -396,7 +419,7 @@ class ShellIntegration {
     void CheckIsDefault() override;
 
     // Set Chrome as the default handler for this protocol.
-    void SetAsDefault(bool interactive_permitted) override;
+    void SetAsDefault() override;
 
     // Returns the histogram prefix for DefaultProtocolClientWorker.
     const char* GetHistogramPrefix() override;