Fixed behavior in case of disabled devtools

chrome/browser/devtools/devtools_window.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_DEVTOOLS_DEVTOOLS_WINDOW_H_
 #define CHROME_BROWSER_DEVTOOLS_DEVTOOLS_WINDOW_H_
 
 #include "chrome/browser/devtools/devtools_contents_resizing_strategy.h"
 #include "chrome/browser/devtools/devtools_toggle_action.h"
 #include "chrome/browser/devtools/devtools_ui_bindings.h"
@@ skipping 45 matching lines @@
   // If |out_strategy| is not NULL, it will contain resizing strategy.
   // For immediately-ready-to-use but maybe not yet fully initialized DevTools
   // use |GetInstanceForInspectedRenderViewHost| instead.
   static content::WebContents* GetInTabWebContents(
       content::WebContents* inspected_tab,
       DevToolsContentsResizingStrategy* out_strategy);
 
   static bool IsDevToolsWindow(content::WebContents* web_contents);
 
   // Open or reveal DevTools window, and perform the specified action.
-  static DevToolsWindow* OpenDevToolsWindow(
-      content::WebContents* inspected_web_contents,
-      const DevToolsToggleAction& action);
+  static void OpenDevToolsWindow(content::WebContents* inspected_web_contents,

[dgozman, 2015/03/03 20:04:54]

Let's just mention in the comment to each method that returned window may be
null, and modify all the callsites to check for that. It's not convenient to use
DevToolsAgentHost and FindDevToolsWindow anytime I need the return value.

[Andrew T Wilson (Slow), 2015/03/04 13:32:42]

In my experience, this will lead to crashes because people will misread the
documentation or forget to do the check correctly, and won't test with dev tools
disabled - this happens all the time, for example, when sync is disabled by
command-line flags/policy. The current CL is a bit more foolproof.

If you feel strongly that we should continue to return a value, then we can make
this change, but I intentionally asked peletskyi@ to remove the return value
since only two callsites use it currently, and one of them already had a
DevToolsAgentHost.

Anyhow, let me know how strongly you feel about this - we can change it back to
returning a window if you still prefer, but I wanted to explain the motivation a
bit more.

[dgozman, 2015/03/04 13:44:48]

I don't feel strongly. Let's go with void.

+                                 const DevToolsToggleAction& action);
 
   // Open or reveal DevTools window, with no special action.
-  static DevToolsWindow* OpenDevToolsWindow(
-      content::WebContents* inspected_web_contents);
+  static void OpenDevToolsWindow(content::WebContents* inspected_web_contents);
 
   // Perform specified action for current WebContents inside a |browser|.
   // This may close currently open DevTools window.
   static DevToolsWindow* ToggleDevToolsWindow(

[dgozman, 2015/03/03 20:04:54]

This method should be affected by the policy as well.

[Andrew T Wilson (Slow), 2015/03/04 13:32:42]

This uses OpenDevToolsWindow() so it does the right thing, but agreed we should
update the documentation for both routines to reflect policy behavior.

[dgozman, 2015/03/04 13:44:48]

The other way around: OpenDevToolsWindow uses ToggleDevToolsWindow, so the check
should actually move there.

       Browser* browser,
       const DevToolsToggleAction& action);
 
   // External frontend is always undocked.
   static void OpenExternalFrontend(
       Profile* profile,
       const std::string& frontend_uri,
       const scoped_refptr<content::DevToolsAgentHost>& agent_host,
       bool isWorker);
 
@@ skipping 80 matching lines @@
   static bool HasFiredBeforeUnloadEventForDevToolsBrowser(Browser* browser);
 
   // Returns true if devtools window would like to hook beforeunload event
   // of this |contents|.
   static bool NeedsToInterceptBeforeUnload(content::WebContents* contents);
 
   // Notify devtools window that closing of |contents| was cancelled
   // by user.
   static void OnPageCloseCanceled(content::WebContents* contents);
 
+  static DevToolsWindow* FindDevToolsWindow(content::DevToolsAgentHost*);
+
  private:
   friend class DevToolsWindowTesting;
 
   // DevTools lifecycle typically follows this way:
   // - Toggle/Open: client call;
   // - Create;
   // - ScheduleShow: setup window to be functional, but not yet show;
   // - DocumentOnLoadCompletedInMainFrame: frontend loaded;
   // - SetIsDocked: frontend decided on docking state;
   // - OnLoadCompleted: ready to present frontend;
@@ skipping 23 matching lines @@
                                 bool shared_worker_frontend,
                                 const std::string& remote_frontend,
                                 bool can_dock,
                                 const std::string& settings);
   static GURL GetDevToolsURL(Profile* profile,
                              const GURL& base_url,
                              bool shared_worker_frontend,
                              const std::string& remote_frontend,
                              bool can_dock,
                              const std::string& settings);
-  static DevToolsWindow* FindDevToolsWindow(content::DevToolsAgentHost*);
+
   static DevToolsWindow* AsDevToolsWindow(content::WebContents*);
   static DevToolsWindow* CreateDevToolsWindowForWorker(Profile* profile);
   static DevToolsWindow* ToggleDevToolsWindow(
       content::WebContents* web_contents,
       bool force_open,
       const DevToolsToggleAction& action,
       const std::string& settings);
 
   // content::WebContentsDelegate:
   content::WebContents* OpenURLFromTab(
@@ skipping 76 matching lines @@
   base::Closure close_callback_;
 
   base::TimeTicks inspect_element_start_time_;
   scoped_ptr<DevToolsEventForwarder> event_forwarder_;
 
   friend class DevToolsEventForwarder;
   DISALLOW_COPY_AND_ASSIGN(DevToolsWindow);
 };
 
 #endif  // CHROME_BROWSER_DEVTOOLS_DEVTOOLS_WINDOW_H_