Issue 4364 - Drop support for Chrome 29-40 and remove legacy code

chrome/ext/background.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 92 matching lines @@
     onActivated: new ext._EventTarget(),
     onRemoved: new ext._EventTarget()
   };
 
   chrome.tabs.onUpdated.addListener(function(tabId, changeInfo, tab)
   {
     if (changeInfo.status == "loading")
       ext.pages.onLoading._dispatch(new Page(tab));
   });
 
+  function createFrame(tabId, frameId)
+  {
+    var frames = framesOfTabs[tabId];
+    if (!frames)
+      frames = framesOfTabs[tabId] = Object.create(null);
+
+    var frame = frames[frameId];
+    if (!frame)
+      frame = frames[frameId] = {};
+
+    return frame;
+  }
+
   chrome.webNavigation.onBeforeNavigate.addListener(function(details)
+  {
+    // Capture parent frame here because onCommitted doesn't get this info.
+    var frame = createFrame(details.tabId, details.frameId);
+    frame.parent = framesOfTabs[details.tabId][details.parentFrameId] || null;
+  });
+
+  chrome.webNavigation.onCommitted.addListener(function(details)
   {
     if (details.frameId == 0)
     {
       ext._removeFromAllPageMaps(details.tabId);
 
       chrome.tabs.get(details.tabId, function()
       {
         // If the tab is prerendered, chrome.tabs.get() sets
         // chrome.runtime.lastError and we have to dispatch the onLoading event,
         // since the onUpdated event isn't dispatched for prerendered tabs.
         // However, we have to keep relying on the unUpdated event for tabs that
         // are already visible. Otherwise browser action changes get overridden
         // when Chrome automatically resets them on navigation.
         if (chrome.runtime.lastError)
         {
           ext.pages.onLoading._dispatch(
             new Page({
               id: details.tabId,
               url: details.url
             })
           );
         }
       });
     }
 
-    // Add or update frame in frame structure
-    var frames = framesOfTabs[details.tabId];
-    if (!frames)
-      frames = framesOfTabs[details.tabId] = Object.create(null);
-
-    frames[details.frameId] = {
-      parent: frames[details.parentFrameId] || null,
-      url: new URL(details.url)
-    };
+    // Update frame URL in frame structure
+    var frame = createFrame(details.tabId, details.frameId);
+    frame.url = new URL(details.url);
   });
 
   function forgetTab(tabId)
   {
     ext.pages.onRemoved._dispatch(tabId);
 
     ext._removeFromAllPageMaps(tabId);
     delete framesOfTabs[tabId];
   }
 
@@ skipping 304 matching lines @@
             if (parentFrameId != -1)
               frames[details[i].frameId].parent = frames[parentFrameId];
           }
         }
       });
     });
   });
 
   chrome.webRequest.onBeforeRequest.addListener(function(details)
   {
-    // the high-level code isn't interested in requests that aren't
+    // The high-level code isn't interested in requests that aren't
     // related to a tab or requests loading a top-level document,
-    // those should never be blocked
+    // those should never be blocked.
     if (details.tabId == -1 || details.type == "main_frame")
       return;
 
-    // we are looking for the frame that contains the element that

[kzar, 2016/08/30 13:14:09]

Mind making this comment a bit clearer (or removing it) while at it? I find it
quite hard to understand.

[Sebastian Noack, 2016/09/09 14:39:50]

Well, this comment isn't new and it makes sense to me. We are looking for the
frame that contains the element this request belongs to. If that element is
loading a subdocument (e.g. if the element is an <iframe>), the frame that
contains it is indicated by the parentFrameId property. However, if for example
it's an <img> element, the frame that contains it is indicated by the frameId
property. Feel free to make a suggestion how to better phrase it.

[kzar, 2016/09/13 12:30:01]

Fair enough, well how about this?

"We are looking for the frame that contains the element which has triggered this
request. For most requests (e.g. images) we can just use the request's frame ID,
but for subdocument requests (e.g. iframes) we must instead use the request's
parent frame ID."

[Sebastian Noack, 2016/09/13 15:21:30]

Done.

[kzar, 2016/09/13 17:08:58]

Looks great, thanks.

-    // is about to load, however if a frame is loading the surrounding
-    // frame is indicated by parentFrameId instead of frameId
+    // We are looking for the frame that contains the element which
+    // has triggered this request. For most requests (e.g. images) we
+    // can just use the request's frame ID, but for subdocument requests
+    // (e.g. iframes) we must instead use the request's parent frame ID.
     var frameId;
     var requestType;
     if (details.type == "sub_frame")
     {
       frameId = details.parentFrameId;
       requestType = "SUBDOCUMENT";
     }
     else
     {
       frameId = details.frameId;
@@ skipping 121 matching lines @@
   ext.windows = {
     create: function(createData, callback)
     {
       chrome.windows.create(createData, function(createdWindow)
       {
         afterTabLoaded(callback)(createdWindow.tabs[0]);
       });
     }
   };
 })();