Skip to content

/ ghostery-extension

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GIS-866: Search Communication #647

Merged
merged 17 commits into from Jan 7, 2021
+80 −22
Merged

GIS-866: Search Communication #647

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
653632e
add search messager class and allow calling of refreshToken
fcjr Dec 22, 2020
0985b0f
only init on gb
fcjr Dec 22, 2020
0e51222
lint fixes
fcjr Dec 23, 2020
952a9f4
add since
fcjr Dec 23, 2020
b225f58
ensure browser info is populated
fcjr Dec 24, 2020
3e28136
remove gb IS const
fcjr Jan 4, 2021
935998f
fix isRefreshing invocation
fcjr Jan 5, 2021
496448e
move to main func, use finally
fcjr Jan 5, 2021
4f90be7
move window dispatch
fcjr Jan 5, 2021
0ca6af2
handle previously unhandled fetch rejections
fcjr Jan 5, 2021
f3d3ff8
refactor to use a single promise
fcjr Jan 5, 2021
5d70dab
Merge branch 'develop' into GIS-866_search_communication
fcjr Jan 5, 2021
e49e6bc
remove old event type
fcjr Jan 5, 2021
c878115
Merge branch 'GIS-866_search_communication' of github.com:ghostery/gh…
fcjr Jan 5, 2021
d8219d2
handle refreshToken errors
fcjr Jan 5, 2021
8e7bebd
update cc
fcjr Jan 6, 2021
eea4e9d
Merge branch 'develop' into GIS-866_search_communication
jsignanini Jan 6, 2021
File filter
Filter file types
Clear filters
Jump to
Jump to file
Failed to load files.

Always

Just for now

Prev Next
Loading status checks…

lint fixes

  • Loading branch information
@fcjr
fcjr committed Dec 23, 2020
Verified
This commit was signed with the committer’s verified signature.
fcjr Frank Chiarulli Jr.
GPG key ID: 262758B51E7A9804 Learn about vigilant mode.
commit 0e512227637bd405f661879a3061461fb495193a
6 src/background.js
View file
Open in desktop
@@ -40,7 +40,7 @@ import tabInfo from './classes/TabInfo';
import metrics from './classes/Metrics';
import account from './classes/Account';
import promoModals from './classes/PromoModals';
import searchMessager from './classes/SearchMessager';
import SearchMessager from './classes/SearchMessager';
// utilities
import { allowAllwaysC2P } from './utils/click2play';
import * as common from './utils/common';
@@ -1655,8 +1655,8 @@ function initializeGhosteryModules() {
* @memberOf Background
*/
function initializeSearchMessageHandler() {
const sm = new searchMessager();
sm.init()
const sm = new SearchMessager();
sm.init();
}

/**
10 src/classes/SearchMessager.js
View file
Open in desktop
@@ -35,17 +35,17 @@ export default class SearchMessager {

_messageHandler(message, sender, sendResponse) {
if (sender.id !== this.extensionId) {
return;
return false;
}

// allow search extension to refresh token
if (message === "refreshToken") {
if (message === 'refreshToken') {
account.refreshToken()
.then(() => sendResponse({ success: true }))
.catch(error => sendResponse({ success: false, error }));
return true
} else {
log('SearchMessager error: Unhandled message', message);
return true;
}
log('SearchMessager error: Unhandled message', message);
return false;
}
}
2 src/utils/api.js
View file
Open in desktop
@@ -28,7 +28,7 @@ class Api {
}

refreshToken() {
This conversation was marked as resolved by jsignanini

This comment has been minimized.

Sign in to view
@jsignanini

jsignanini Jan 4, 2021
Member

@fcjr I'm thinking renaming this to something like refreshTokenAndHandleIsRefreshing to make it super clear what the purpose of this new function is

This comment has been minimized.

Sign in to view
@fcjr

fcjr Jan 4, 2021
Author Member

That sounds reasonable, I'll make the change. My thought with just naming it refreshToken was that as an external user of the api class the inner "isRefreshing" state is an implementation detail that external users of the function don't really need to know about, but since this should only be used in special cases anyways there shouldn't be a harm in making it explicit

This comment has been minimized.

Sign in to view
@jsignanini

jsignanini Jan 4, 2021
Member

@fcjr actually you do make a good point as it’s only used internally in the other “private” function. Ok you can just leave it as is then. Thanks.
return this._refreshToken().then(this.isRefreshing = false);
return this._refreshToken().then(this.isRefreshing = false); // eslint-disable-line no-return-assign
This conversation was marked as resolved by chrmod

This comment has been minimized.

Sign in to view
@chrmod

chrmod Jan 5, 2021
Member

this.isRefreshing is set to false immediately after this._refreshToken is being called - it does not wait for a promise to resolve.

This comment has been minimized.

Sign in to view
@chrmod

chrmod Jan 5, 2021
Member

Also, why this.isRefreshing is not set to false by _refreshToken() ?
This conversation was marked as resolved by fcjr

This comment has been minimized.

Sign in to view
@chrmod

chrmod Jan 5, 2021
Member

Suggested change
return this._refreshToken().then(this.isRefreshing = false); // eslint-disable-line no-return-assign
return this._refreshToken().then(() => { this.isRefreshing = false; });
}

_refreshToken() {
ProTip! Use n and p to navigate between commits in a pull request.