Issue 154 - Added UI for devtools panel on Chrome

ext/devtools.js

 /*
- * This file is part of Adblock Plus <http://adblockplus.org/>,
- * Copyright (C) 2006-2015 Eyeo GmbH
+ * 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-ext.devtools = {
-  connect: function()
-  {
-    return {
-      postMessage: function() {},
+"use strict";
 
-      onMessage: {
-        addListener: function(listener)

[Wladimir Palant, 2015/01/10 08:33:29]

Quite frankly, I don't like the idea of having a different API here - I think
that the Chrome abstraction layer (ext/content.js in particular) can handle this
implicitly rather than increasing the complexity for all implementations. Seeing
that chrome.runtime.connect is available to content scripts as well, it should
probably connect unconditionally and send all messages through the connection.
The background page can maintain a list of ports mapped by tab ID so that
sending *to* the content script will also succeed. This will also have the
advantage that using webNavigation won't be necessary any more,
Port.onDisconnect() will do. This will solve the same issue for the popup as
well - it isn't specific to the devtools. Shouldn't be too complicated?

[Sebastian Noack, 2015/01/10 08:53:45]

Yes, it will be fairly complicated. Ports are very low-level, and doesn't
support sending responses to individual messages. So we would need to implement
response handling like we did on Safari also for Chrome. And even then devtools
panels aren't tabs. So the abstraction layer as it is right now can't simply
handle it (without extending the API) anyway.

With the devtools panel we don't need to bother about responses, and using ports
is rather convenient here, beside its the only option anyway. Yes, we are going
to have a similar issue with the popup when switching to async messages, but
this is a separate issue.

[Thomas Greiner, 2015/01/12 10:37:54]

Right, the existing abstraction layer would probably be insufficient for this
case.

However, I do think we should look into performance differences between regular
message passing and using ports because we're sending quite a bunch of messages
so it might make sense for us to use ports. That's outside of the scope of this
review, of course.

[Wladimir Palant, 2015/01/13 07:28:04]

I see, that's making things too complicated indeed. However, I definitely see
this as a limitation of the Chrome abstraction layer - and that's where we
should resolve it (e.g. if ports don't work then we can still do broadcasting or
direct queuing of messages). Either way, yesterday we saw that a devpanel is
treated either as a tab or a frame inside a tab - so we don't have any problems
using regular messaging there, and we can solve the hard issues later.
Additional API is definitely unnecessary at this point.

[Sebastian Noack, 2015/02/04 14:17:29]

As discussed some weeks ago, we can actually use regular messaging here, since
it turned out that the devtool panels actually has a tab/page. So I'm reusing
the mechanism for the first run page now, to have the messaging properly
emulated.

-        {
-          // blocked request
-          listener({
-            type: "add-record",
-            request: {
-              url: "http://adserver.example.com/ad_banner.png",
-              type: "IMAGE",
-              docDomain: "example.com"
-            },
-            filter: {
-              text: "/ad_banner*$domain=example.com",
-              whitelisted: false,
-              userDefined: false,
-              subscription: "EasyList"
-            }
-          });
+(function(global)
+{
+  if (!global.ext)
+    global.ext = {};
 
-          // whiletisted request
-          listener({
-            type: "add-record",
-            request: {
-              url: "http://example.com/looks_like_an_ad_but_isnt_one.html",
-              type: "SUBDOCUMENT",
-              docDomain: "example.com"
-            },
-            filter: {
-              text: "@@||example.com/looks_like_an_ad_but_isnt_one.html",
-              whitelisted: true,
-              userDefined: false,
-              subscription: "EasyList"
-            }
-          });
+  global.ext.devtools = {
+    panels: {
+      openResource: function() {}
+    },
 
-          // request with long URL and no filter matches
-          listener({
-            type: "add-record",
-            request: {
-              url: "https://this.url.has.a.long.domain/and_a_long_path_maybe_not_long_enough_so_i_keep_typing?there=are&a=couple&of=parameters&as=well&and=even&some=more",
-              type: "XMLHTTPREQUEST",
-              docDomain: "example.com"
-            },
-            filter: null
-          });
+    inspectedWindow: {
+      reload: function() {}
+    }
+  };
 
-          // matching element hiding filter
-          listener({
-            type: "add-record",
-            request: {
-              type: "ELEMHIDE",
-              docDomain: "example.com"
-            },
-            filter: {
-              text: "example.com##.ad_banner",
-              whitelisted: false,
-              userDefined: false,
-              subscription: "EasyList"
-            }
-          });
-
-          // user-defined filter
-          listener({
-            type: "add-record",
-            request: {
-              url: "http://example.com/some-annoying-popup",
-              type: "POPUP",
-              docDomain: "example.com"
-            },
-            filter: {
-              text: "||example.com/some-annoying-popup$popup",
-              whitelisted: false,
-              userDefined: true,
-              subscription: null
-            }
-          });

[Wladimir Palant, 2015/01/10 08:33:29]

I'd rather have proper message exchange simulated, particularly simulating the
"requests.listen" message as well. The corresponding code should really be in
background.js where we have the "application logic" - when receiving
"requests.listen" it should send these responses immediately and maybe also send
another response in a timed interval.

[Sebastian Noack, 2015/01/10 08:53:45]

This would makes things significantly more complex. And I don't see any
advantage. This is just that you can run the UI standalone. Nobody except
ourselves will ever do that. So I don't think it's worth making thing
unnecessary complicated here.

[Thomas Greiner, 2015/01/12 10:37:54]

More complex because we're using two different methods for passing messages or
are there other issues you're thinking of?

[Sebastian Noack, 2015/01/12 11:55:07]

What Wladimir is suggesting is to inject an iframe running a simulated
background page, passing messages with postMessage. We have to do this for the
first run page anyway. But since we can't do regular messaging, but have to use
ports here, this would make things more complicated, for no benefit whatsoever.

[Wladimir Palant, 2015/01/13 07:28:04]

Yes, this is just us testing that thing properly and independently of any
browser ;)

Given that we don't need any special messaging after all, I guess that there is
no issue with implementing the messages in the background page mock?

[Sebastian Noack, 2015/02/04 14:17:29]

Yep, done, see above.

-
-        }
-      }
-    };
-  },
-
-  panels: {
-    openResource: function() {}
-  },
-
-  inspectedWindow: {
-    reload: function() {}
-  }
-}
-
+  global.ext.backgroundPage._sendRawMessage({type: "devtools"});
+})(this);