Fixes the interactive default browser UX for policy setting

chrome/browser/shell_integration.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_SHELL_INTEGRATION_H_
 #define CHROME_BROWSER_SHELL_INTEGRATION_H_
 
 #include <string>
 
 #include "base/files/file_path.h"
@@ skipping 183 matching lines @@
   };
 
   class DefaultWebClientObserver {
    public:
     virtual ~DefaultWebClientObserver() {}
     // Updates the UI state to reflect the current default browser state.
     virtual void SetDefaultWebClientUIState(DefaultWebClientUIState state) = 0;
     // 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
   //  or application for a url protocol on Windows and Linux, and also setting
   //  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
     // function. If there is no observer, this function does not do anything.
     void StartCheckIsDefault();
 
     // Sets Chrome as the default web client application. If there is an
     // observer, once the operation has completed the new default will be
     // queried and the current status reported via SetDefaultWebClientUIState.
     void StartSetAsDefault();
@@ skipping 43 matching lines @@
     // is default process.
     // It is safe to call this multiple times. Only the first call is processed
     // after StartSetAsDefault() is invoked.
     void OnSetAsDefaultAttemptComplete(AttemptResult result);
 
     // Returns true if FinalizeSetAsDefault() will be called.
     bool set_as_default_initialized() const {
       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;
 
    private:
     // Checks whether Chrome is the default web client. Always called on the
     // FILE thread. Subclasses are responsible for calling
     // OnCheckIsDefaultComplete() on the UI thread.
     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.
     virtual const char* GetHistogramPrefix() = 0;
 
     // Invoked on the UI thread prior to starting a set-as-default operation.
     // Returns true if the initialization succeeded and a subsequent call to
     // FinalizeSetAsDefault() is required.
     virtual bool InitializeSetAsDefault();
 
     // Invoked on the UI thread following a set-as-default operation.
     virtual void FinalizeSetAsDefault();
 
     // Reports the result and duration for one set-as-default attempt.
     void ReportAttemptResult(AttemptResult result);
 
     // Updates the UI in our associated view with the current default web
     // client state.
     void UpdateUI(DefaultWebClientState state);
 
     // Returns true if the duration of an attempt to set the default web client
     // should be reported to UMA for |result|.
     static bool ShouldReportDurationForResult(AttemptResult result);
 
     // Returns a string based on |result|. This is used for UMA reports.
     static const char* AttemptResultToString(AttemptResult result);
 
     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.
     bool set_as_default_initialized_ = false;
 
     // Records the time it takes to set the default browser.
     base::TimeTicks start_time_;
 
     // Wait until Chrome has been confirmed as the default browser before
     // reporting a successful attempt.
     bool check_default_should_report_success_ = false;
 
     DISALLOW_COPY_AND_ASSIGN(DefaultWebClientWorker);
   };
 
   // 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;
 
     // Check if Chrome is the default browser.
     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;
 
 #if defined(OS_WIN)
     // On Windows 10+, adds the default browser callback and starts the timer
     // that determines if the operation was successful or not.
     bool InitializeSetAsDefault() override;
 
     // On Windows 10+, removes the default browser callback and stops the timer.
@@ skipping 10 matching lines @@
 
     DISALLOW_COPY_AND_ASSIGN(DefaultBrowserWorker);
   };
 
   // Worker for checking and setting the default client application
   // for a given protocol. A different worker instance is needed for each
   // protocol you are interested in, so to check or set the default for
   // 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_; }
 
    protected:
     ~DefaultProtocolClientWorker() override;
 
    private:
     // Check is Chrome is the default handler for this protocol.
     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;
 
     std::string protocol_;
 
     DISALLOW_COPY_AND_ASSIGN(DefaultProtocolClientWorker);
   };
 };
 
 #endif  // CHROME_BROWSER_SHELL_INTEGRATION_H_