Refactor sudden termination

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index fb5f2b51b94291be69fbca171ff0649e3d82116e..0c304754f04b73eb70ae61ff7d8c7304d84347db 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -477,6 +477,16 @@ CommonNavigationParams MakeCommonNavigationParams(
   return params;
 }
 
+// Returns true if the disablers tracked by counter go from being absent to

[Charlie Reis, 2015/01/23 06:01:32]

nit: |counter|

[clamy, 2015/01/26 16:38:40]

Removed this function which was no longer needed.

+// present or vice-versa.
+bool HandleSuddenTerminationDisablerChange(int* counter, int variation) {
+  bool presence_before = *counter;
+  *counter += variation;
+  DCHECK(*counter >= 0);
+  bool presence_after = *counter;
+  return presence_before == presence_after;
+}
+
 #if !defined(OS_ANDROID)
 media::Context3D GetSharedMainThreadContext3D() {
   cc::ContextProvider* provider =
@@ -653,6 +663,8 @@ RenderFrameImpl::RenderFrameImpl(RenderViewImpl* render_view, int routing_id)
       manifest_manager_(NULL),
       accessibility_mode_(AccessibilityModeOff),
       renderer_accessibility_(NULL),
+      beforeunload_handlers_(0),
+      unload_handlers_(0),
       weak_factory_(this) {
   std::pair<RoutingIDFrameMap::iterator, bool> result =
       g_routing_id_frame_map.Get().insert(std::make_pair(routing_id_, this));
@@ -3540,6 +3552,43 @@ bool RenderFrameImpl::exitFullscreen() {
   return true;
 }
 
+void RenderFrameImpl::suddenTerminationDisablerChanged(

[Charlie Reis, 2015/01/23 06:01:32]

This seems unnecessarily complex.  Why do we need to track counters in
RenderFrameImpl?  We could avoid storing any state here (and the complex
HandleSuddenTerminationDisablerChange method) if Blink did that work for us.

In other words, just have Blink tell WebFrameClient if it goes between zero and
non-zero handlers for beforeunload or unload.  Should be easy, since it's just
checking set.isEmpty() before and after.

Then RenderFrameImpl just passes that message on to RFH without any extra logic
needed.

[clamy, 2015/01/26 16:38:40]

Done.

+    int variation,
+    blink::WebFrameClient::SuddenTerminationDisablerType type) {
+  int* relevant_counter;
+  switch (type) {
+    case blink::WebFrameClient::BeforeUnloadHandler:
+      relevant_counter = &beforeunload_handlers_;
+      break;
+    case blink::WebFrameClient::UnloadHandler:
+      relevant_counter = &unload_handlers_;
+      break;
+    default:
+      NOTREACHED();
+  }
+  DCHECK(relevant_counter);
+
+  bool presence_changed = HandleSuddenTerminationDisablerChange(
+      relevant_counter, variation);
+  if (!presence_changed)
+    return;
+
+  // The presence status of one of the sudden termination disablers changed,
+  // inform the browser.
+  switch (type) {
+    case blink::WebFrameClient::BeforeUnloadHandler:
+      Send(new FrameHostMsg_BeforeUnloadHandlersPresenceChanged(
+          routing_id_, beforeunload_handlers_));

[Charlie Reis, 2015/01/23 06:01:32]

This is a count, not a bool.  Prefix with !! to make the intention clear (and it
might be necessary for compiling on Windows anyway).

[clamy, 2015/01/26 16:38:40]

Switched to booleans.

+      break;
+    case blink::WebFrameClient::UnloadHandler:
+      Send(new FrameHostMsg_UnloadHandlersPresenceChanged(
+          routing_id_, unload_handlers_));
+      break;
+    default:
+      NOTREACHED();
+  }
+}
+
 void RenderFrameImpl::DidPlay(blink::WebMediaPlayer* player) {
   Send(new FrameHostMsg_MediaPlayingNotification(
       routing_id_, reinterpret_cast<int64>(player), player->hasVideo(),