Issue 6292 - Make issue reporter compatible with test server

ext/content.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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 83 matching lines @@
         resolvePromise = resolve;
       });
     }
   };
 
   if (!("tabs" in browser))
     browser.tabs = new Map([[0, {url: "example.com"}]]);
 
   browser.tabs.get = (...args) =>
   {
-    let result = Map.prototype.get.apply(browser.tabs, args);

[a.giammarchi, 2018/03/07 17:11:04]

if you can use `Map.prototype.get.apply` with `browser.tabs` it means you expect
`browser.tabs` to be an `instanceof Map`, right?

If that's the case, the following is all you need.

let result = browser.tabs.get(...args);

[a.giammarchi, 2018/03/07 17:17:49]

actually, on a second thought, I don't understand why this callback needs to be
so vague. It needs just one single argument that should be the tabId so you can
just have that as parameter and use it right away as `browser.tabs.get(id);`,
right?

I'm not too fond of generic `...arg`, uinless really needed, because it's just
like a meaningless `arguments` in a modern fashion.

[saroyanm, 2018/03/08 15:14:27]

This implementation is just use uses prototype.get method of Map to avoid
infinite loop and return promise, so it's merely extends get method for this
particular Map object.

[a.giammarchi, 2018/03/08 17:27:34]

it's not so clear from the code. Maybe a comment might help others asking
themself what is this about?

[saroyanm, 2018/03/08 18:16:47]

Noted, I'll add one.

[saroyanm, 2018/03/12 15:45:21]

Done.

-    return new Promise((resolve, reject) =>

[a.giammarchi, 2018/03/07 17:11:04]

If I understand correctly result is synchronous so there's no need to create a
Promise and its callback.

return result ? Promise.resolve(result) : Promise.reject(new Error("Tab cannot
be found"));

[saroyanm, 2018/03/08 15:14:27]

This is the Mock implmenetation of tabs.get ->
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/get 

So I do need to return Promise here, in order to mimic actual implementation for
our test server.

[a.giammarchi, 2018/03/08 17:27:34]

when the result is already known, there is no difference whatsoever between:

`new Promise(resolve => resolve(data));`

and

`Promise.resolve(data);`

As you can guess the latter doesn't need to create callbacks, pass through new,
etc.

The result is exactly a promise.

`Promise.resolve(123).then(console.log);`

Same goes for `Promise.reject(error)`.

My point is that you can simplify code and return a resolved or a rejected
promise right away.

You are doing this already, but through an unnecessary callback and a `new`
operation.

As summary:

```
new Promise((resolve, reject) => {
  if (data) resolve(data);
  else reject(error);
});
```

is the exact equivalent of:

`data ? Promise.resolve(data) : Promise.reject(error);`

I hope my previous comment is more clear now/

[saroyanm, 2018/03/08 18:16:47]

Sorry I think I misread your initial I though you were asking why we are
creating a promise because in chrome is only callbacks were used.. Thanks for
elaborating, will update.

[saroyanm, 2018/03/12 15:45:21]

Done.

-    {
-      if (result)
-        resolve(result);
-      else
-        reject(new Error("Tab cannot be found"));
-    });
-  }
+    // Extend browser.tabs.get()
+    const result = Map.prototype.get.apply(browser.tabs, args);
+    return (result ? Promise.resolve(result) :
+      Promise.reject(new Error("Tab cannot be found")));
+  };
 }());