Reload force-installed extensions on crash/force-close

chrome/browser/background/background_contents_service.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_BACKGROUND_BACKGROUND_CONTENTS_SERVICE_H_
 #define CHROME_BROWSER_BACKGROUND_BACKGROUND_CONTENTS_SERVICE_H_
 
 #include <map>
+#include <queue>
 #include <string>
 #include <vector>
 
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/tab_contents/background_contents.h"
 #include "components/browser_context_keyed_service/browser_context_keyed_service.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/common/window_container_type.h"
@@ skipping 26 matching lines @@
 // It is also responsible for tracking the association between
 // BackgroundContents and their parent app, and shutting them down when the
 // parent app is unloaded.
 class BackgroundContentsService : private content::NotificationObserver,
                                   public BackgroundContents::Delegate,
                                   public BrowserContextKeyedService {
  public:
   BackgroundContentsService(Profile* profile, const CommandLine* command_line);
   virtual ~BackgroundContentsService();
 
+  static void SetCrashDelaysForTesting(const base::TimeDelta& restart_delay,

[bartfab (slow), 2013/08/26 13:26:53]

Nit 1: Add comment that explain what this does.

Nit 2: The name may be a bit too generic. Since these delays apply to
force-installed extensions only, it would be best to mention that in the method
name.

[anitawoodruff, 2013/08/26 21:45:23]

Done.

+                                       const base::TimeDelta& crash_window);
+
   // Returns the BackgroundContents associated with the passed application id,
   // or NULL if none.
   BackgroundContents* GetAppBackgroundContents(const string16& appid);
 
   // Returns true if there's a registered BackgroundContents for this app. It
   // is possible for this routine to return true when GetAppBackgroundContents()
   // returns false, if the BackgroundContents closed due to the render process
   // crashing.
   bool HasRegisteredBackgroundContents(const string16& appid);
 
@@ skipping 98 matching lines @@
   void ShutdownAssociatedBackgroundContents(const string16& appid);
 
   // Returns true if this BackgroundContents is in the contents_list_.
   bool IsTracked(BackgroundContents* contents) const;
 
   // Sends out a notification when our association of background contents with
   // apps may have changed (used by BackgroundApplicationListModel to update the
   // set of background apps as new background contents are opened/closed).
   void SendChangeNotification(Profile* profile);
 
+  static base::TimeDelta restart_delay_;

[bartfab (slow), 2013/08/26 13:26:53]

Static initializers are not allowed in Chrome. You can have static PODs but not
complex classes. You will have to make the statics PODs (say ints holding the
number of milliseconds or base::TimeDelta::ToInternalValue()s).

[anitawoodruff, 2013/08/26 21:45:23]

Done. Made them ints, and only convert to TimeDelta at the last minute. Should I
still pass the ints by reference (using int&) to the SetRestartDelays method?

[bartfab (slow), 2013/08/27 09:21:29]

For PODs, passing by reference is not necessary.

+  static base::TimeDelta crash_window_;
+
   // PrefService used to store list of background pages (or NULL if this is
   // running under an incognito profile).
   PrefService* prefs_;
   content::NotificationRegistrar registrar_;
 
   // Information we track about each BackgroundContents.
   struct BackgroundContentsInfo {
     // The BackgroundContents whose information we are tracking.
     BackgroundContents* contents;
     // The name of the top level frame for this BackgroundContents.
     string16 frame_name;
   };
 
   // Map associating currently loaded BackgroundContents with their parent
   // applications.
   // Key: application id
   // Value: BackgroundContentsInfo for the BC associated with that application
   typedef std::map<string16, BackgroundContentsInfo> BackgroundContentsMap;
   BackgroundContentsMap contents_map_;
 
+  // Map associating extension IDs with their most recent crash timestamps.

[bartfab (slow), 2013/08/26 13:26:53]

Nit: Add a note that this is used for force-installed apps/extensions only.

[anitawoodruff, 2013/08/26 21:45:23]

Done.

+  // Key: extension id.
+  // Value: queue containing up to 5 most recent crash timestamps.
+  std::map<std::string, std::queue<base::TimeTicks> > extension_crashlog_map_;

[bartfab (slow), 2013/08/26 13:26:53]

Nit 1: #include <queue>
Nit 2: #include "base/time.h"

[anitawoodruff, 2013/08/26 21:45:23]

Done Nit 2. <queue> I'd already included on line 9. B-)

+
+  // Map containing ids of extensions for which we have already sent
+  // 'Extension is misbehaving' notifications.

[bartfab (slow), 2013/08/26 13:26:53]

Nit 1: Instead of "sent notifications," better say "shown ballons".
Nit 2: Add a note that this applies to force-installed apps/extensions only.

[anitawoodruff, 2013/08/26 21:45:23]

Done.
I've changed the wording of comments to say apps/extensions instead of
extensions everywhere, but should I rename all these variables too? It's gonna
get a bit verbose..

[bartfab (slow), 2013/08/27 09:21:29]

The variable naming is OK. Comments are for verbose stuff. Variable names should
be as-crisp-as-possible. I think you have the right balance here now.

+  std::set<std::string> misbehaving_extensions_;

[bartfab (slow), 2013/08/26 13:26:53]

Nit: #include <set>

[anitawoodruff, 2013/08/26 21:45:23]

Done.

+
   DISALLOW_COPY_AND_ASSIGN(BackgroundContentsService);
 };
 
 #endif  // CHROME_BROWSER_BACKGROUND_BACKGROUND_CONTENTS_SERVICE_H_