Refactor basic auth #233
Refactor basic auth #233
Conversation
| @@ -99,96 +100,86 @@ function onMessage(request, sender, sendResponse) { | |||
| // spawn a new tab with pre-provided credentials | |||
| if (request.action == "launch") { | |||
| chrome.tabs.create({ url: request.url }, function(tab) { | |||
| var tabLoaded = false; | |||
There was a problem hiding this comment.
I didn't see a particular use for tabLoaded, given that we discard onAuthListener immediately after tab is loaded. Did you have a point for this that I'm missing?
There was a problem hiding this comment.
That can go. I went through a couple of different versions of the bugfix; that was left over from one of them but is no longer applicable.
|
|
||
| // mark tab as loaded & wipe credentials | ||
| tabLoaded = true; | ||
| request.username = request.password = null; |
There was a problem hiding this comment.
The reference to this object is in this function and now in onAuthRequired, as soon as both functions are gone the credentials are not accessible anyway - is there a point in additionally wiping them here?
There was a problem hiding this comment.
As above - this was leftover from an earlier iteration of the bugfix; it can go.
- Use a shared tabs.onUpdated instead of subscribing multiple times - Extract onAuthRequired into a separate global function
maximbaz
force-pushed
the
refactor-basic-auth
branch
from
3f67f02
to
baa90ca
Compare
|
Thanks @erayd for the confirmations, any other feedback before this is merged? |
|
@maximbaz No other feedback at this point - I like what you've done, go ahead and merge it :-). |
@erayd I'll comment inline on a couple of particularly note-worthy points.