Issue 29527877: Issue 5582 - Workaround Edge bug in tabs.query (Closed)
|
Created:
Aug. 25, 2017, 10:55 p.m. by Oleksandr Modified:
Aug. 29, 2017, 4:14 p.m. Visibility:
Public. |
DescriptionIssue 5582 - Workaround Edge bug in tabs.query
Patch Set 1 #
Total comments: 2
Patch Set 2 : Manually look for the options tab #
Total comments: 3
Patch Set 3 : Streamline the code #
Total comments: 3
Patch Set 4 : Rebase to master tip #
Total comments: 2
Patch Set 5 : Update the comment about Firefox on Android #MessagesTotal messages: 13
https://codereview.adblockplus.org/29527877/diff/29527878/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29527877/diff/29527878/ext/background.js#n... ext/background.js:743: let queryInfo = {title: "Adblock Plus Options"}; This is somewhat questionable, of course. It is possible for other third-party pages to have that title as well. But I couldn't come up with any security issues this could introduce, so I guess this should be fine. At least until the bug is fixed.
https://codereview.adblockplus.org/29527877/diff/29527878/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29527877/diff/29527878/ext/background.js#n... ext/background.js:743: let queryInfo = {title: "Adblock Plus Options"}; On 2017/08/25 22:59:31, Oleksandr wrote: > This is somewhat questionable, of course. It is possible for other third-party > pages to have that title as well. But I couldn't come up with any security > issues this could introduce, so I guess this should be fine. At least until the > bug is fixed. The title indeed doesn't seem to be translated, but this is a bug. Also hard-coding this kind of details here, makes it harder and error-prone for other products that are based on Adblock Plus. Can't we just go through all tabs and match the URL ourselves?
https://codereview.adblockplus.org/29527877/diff/29528564/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29527877/diff/29528564/ext/background.js#n... ext/background.js:753: chrome.tabs.query({}, tabs => Passing {} is the only way to get all tabs, it seems. Providing windowId or url: "<all_urls>" doesn't return extension pages.
https://codereview.adblockplus.org/29527877/diff/29528564/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29527877/diff/29528564/ext/background.js#o... ext/background.js:740: let open = win => It seems the `win` argument is ignored now. Mind removing the argument and simplifying the calling code? https://codereview.adblockplus.org/29527877/diff/29528564/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29527877/diff/29528564/ext/background.js#n... ext/background.js:755: if (tabs && (tabs.findIndex(isOptionsTab) >= 0)) Calling findIndex()/find() twice is redundant. Just use find() once, store the return value in a variable and check it. Also it might make the code easier to read to use an arrow function here, rather than defining a named function above.
It would probably make sense to have Manish look at this as well, since it touches some of the Firefox for Android code he recently wrote.
https://codereview.adblockplus.org/29527877/diff/29529796/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29527877/diff/29529796/ext/background.js#n... ext/background.js:749: let tab = tabs == null ? null : tabs.find(element => In which case is `tabs == null`? According to my understanding of this API that should never happen. If this check is indeed necessary `tabs && ...` would be more concise than the tertiary operator. https://codereview.adblockplus.org/29527877/diff/29529796/ext/background.js#n... ext/background.js:751: return element.url == fullOptionsUrl; The braces + return statement is redundant for single assignment arrow functions.
https://codereview.adblockplus.org/29527877/diff/29529796/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29527877/diff/29529796/ext/background.js#n... ext/background.js:749: let tab = tabs == null ? null : tabs.find(element => On 2017/08/29 04:04:26, Sebastian Noack wrote: > In which case is `tabs == null`? According to my understanding of this API that > should never happen. tabs was null on Firefox for Android when we were querying for options.html but we're not doing that anymore so it should not be null for that reason. Firefox for Android avoids this code path entirely now (changeset 4f89fd4b176a), if we simply rebase this patch it should be a lot clearer.
On 2017/08/29 04:18:33, Manish Jethani wrote: > https://codereview.adblockplus.org/29527877/diff/29529796/ext/background.js > File ext/background.js (right): > > https://codereview.adblockplus.org/29527877/diff/29529796/ext/background.js#n... > ext/background.js:749: let tab = tabs == null ? null : tabs.find(element => > On 2017/08/29 04:04:26, Sebastian Noack wrote: > > In which case is `tabs == null`? According to my understanding of this API > that > > should never happen. > > tabs was null on Firefox for Android when we were querying for options.html but > we're not doing that anymore so it should not be null for that reason. Oh, let me clarify. tabs may be null if there's any error (runtime.lastError is set). It's probably a good idea to check for null.
https://codereview.adblockplus.org/29527877/diff/29530565/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29527877/diff/29530565/ext/background.js#o... ext/background.js:738: // nor does Firefox for Android before version 57, Remove this line, since this is really an Edge specific clause now.
https://codereview.adblockplus.org/29527877/diff/29530565/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29527877/diff/29530565/ext/background.js#n... ext/background.js:768: // Firefox for Android does not support the windows API. Since there is Maybe it's a good idea to update this comment here since the other one has been removed. Something like: "Firefox for Android before version 57 does not support runtime.openOptionsPage, nor does it support the windows API. Since ..."
On 2017/08/29 16:03:19, Oleksandr wrote: LGTM
LGTM 
|
