Issue 5965 - Handle edge case when searching for options page tab

Issue 5965 - Handle edge case when searching for options page tab

[kzar, 2017-11-11 16:37:49]

Patch Set 1

[kzar, 2017-11-11 16:42:11]

Patch Set 2 : Correct mistake in callback logic

[Wladimir Palant, 2017-11-11 18:34:15]

https://codereview.adblockplus.org/29604555/diff/29604558/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29604555/diff/29604558/lib/options.js#newc...
lib/options.js:44: callback(optionsTab);
Maybe reduce the indentation level here:

  if (optionsTab)
  {
    callback(optionsTab);
    return;
  }

https://codereview.adblockplus.org/29604555/diff/29604558/lib/options.js#newc...
lib/options.js:50: // we expect (minus the protocol part).
That's too many details IMHO. We don't really know which Firefox versions and
which operating systems are affected. All we know is that there are conditions
where we might get a tab that isn't fully initialized yet. So maybe something
like: "Newly created tabs might have about:blank as their URL in Firefox rather
than the final options page URL, we need to wait for those to finish loading."

https://codereview.adblockplus.org/29604555/diff/29604558/lib/options.js#newc...
lib/options.js:54: tab.title == hostname + pathname &&
I don't think that we should rely on the tab title. We should just watch all
about:blank tabs that are still loading, these are potentially our options tab.

https://codereview.adblockplus.org/29604555/diff/29604558/lib/options.js#newc...
lib/options.js:58: if (potentialOptionsTab)
Maybe reduce indentation level here as well:

  if (!potentialOptionsTabs.length)
  {
    callback();
    return;
  }

https://codereview.adblockplus.org/29604555/diff/29604558/lib/options.js#newc...
lib/options.js:61: let updateListener = (tabId, changeInfo, tab) =>
What about consistent naming? Meaning either removeListener or onUpdated.

[kzar, 2017-11-13 12:32:58]

Patch Set 3 : Addressed Wladimir's initial feedback

https://codereview.adblockplus.org/29604555/diff/29604558/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29604555/diff/29604558/lib/options.js#newc...
lib/options.js:44: callback(optionsTab);
On 2017/11/11 18:34:14, Wladimir Palant wrote:
> Maybe reduce the indentation level here:
> 
>   if (optionsTab)
>   {
>     callback(optionsTab);
>     return;
>   }

Done.

https://codereview.adblockplus.org/29604555/diff/29604558/lib/options.js#newc...
lib/options.js:50: // we expect (minus the protocol part).
On 2017/11/11 18:34:14, Wladimir Palant wrote:
> That's too many details IMHO. We don't really know which Firefox versions and
> which operating systems are affected. All we know is that there are conditions
> where we might get a tab that isn't fully initialized yet. So maybe something
> like: "Newly created tabs might have about:blank as their URL in Firefox
rather
> than the final options page URL, we need to wait for those to finish loading."

Done.

https://codereview.adblockplus.org/29604555/diff/29604558/lib/options.js#newc...
lib/options.js:54: tab.title == hostname + pathname &&
On 2017/11/11 18:34:14, Wladimir Palant wrote:
> I don't think that we should rely on the tab title. We should just watch all
> about:blank tabs that are still loading, these are potentially our options
tab.

Done.

https://codereview.adblockplus.org/29604555/diff/29604558/lib/options.js#newc...
lib/options.js:58: if (potentialOptionsTab)
On 2017/11/11 18:34:14, Wladimir Palant wrote:
> Maybe reduce indentation level here as well:
> 
>   if (!potentialOptionsTabs.length)
>   {
>     callback();
>     return;
>   }

Done.

https://codereview.adblockplus.org/29604555/diff/29604558/lib/options.js#newc...
lib/options.js:61: let updateListener = (tabId, changeInfo, tab) =>
On 2017/11/11 18:34:14, Wladimir Palant wrote:
> What about consistent naming? Meaning either removeListener or onUpdated.

Done.

[Wladimir Palant, 2017-11-23 11:37:13]

LGTM, one nit below.

https://codereview.adblockplus.org/29604555/diff/29606555/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29604555/diff/29606555/lib/options.js#newc...
lib/options.js:67: let urlMatches = tab.url == fullOptionsUrl;
Nit: Variable names are supposed to be nouns, not verbs. urlMatch maybe?

[kzar, 2017-11-23 11:51:08]

Patch Set 4 : Renamed urlMatch variable

Will retest this now.

https://codereview.adblockplus.org/29604555/diff/29606555/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29604555/diff/29606555/lib/options.js#newc...
lib/options.js:67: let urlMatches = tab.url == fullOptionsUrl;
On 2017/11/23 11:37:12, Wladimir Palant wrote:
> Nit: Variable names are supposed to be nouns, not verbs. urlMatch maybe?

Done. (It's quite nice in Clojure you can use symbols like "?" so you could have
a variable name url-matches?.)

[Wladimir Palant, 2017-11-23 13:51:35]

LGTM