Skip to content
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.

Sign up for GitHub

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

Extension: Fix multiple inits (onload) #307

Closed
wants to merge 2 commits into from
Closed

Extension: Fix multiple inits (onload) #307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
56e54e8
Extension: Fix multiple inits (onload)
yoyurec Apr 30, 2022
f076f1c
add flow type for webNavigation.onCompleted
karlicoss May 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 20 additions & 30 deletions extension/src/background.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ type IconStyle = {
text: string,
}

// https://developer.chrome.com/docs/extensions/reference/webNavigation/#type-onCompleted-callback-details
type WebNavigationInfo = {
frameId: number,
tabId: number,
url: string,
};

// TODO this can be tested?
function getIconStyle(result: Result): IconStyle {
Expand Down Expand Up @@ -99,9 +105,9 @@ function getIconStyle(result: Result): IconStyle {
}


async function updateState (tab: chrome$Tab) {
async function onTabLoaded(tab: WebNavigationInfo) {
const url = unwrap(tab.url);
const tabId = unwrap(tab.id);
const tabId = unwrap(tab.tabId);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, looks like this is causing these for me

requesting! https://news.ycombinator.com/item?id=31224707
common.js:19 Uncaught (in promise) Error: undefined or null!
    at unwrap (common.js:19:11)
    at shouldProcessPage (background.js:583:62)
    at openSidebar (background.js:702:24)
    at eval (background.js:248:22)


if (ignored(url)) {
// todo reflect in the sidebar/popup?
Expand Down Expand Up @@ -214,7 +220,7 @@ async function updateState (tab: chrome$Tab) {
visits = new Visits(url, url, [visits])
}
console.assert(visits instanceof Visits)

// right, we can't inject code into error pages (effectively, internal). For these, display popup instead of sidebar?
// TODO and show system wide notification instead of tab notification?
// https://stackoverflow.com/questions/32761782/can-a-chrome-extension-run-code-on-a-chrome-error-page-i-e-err-internet-disco
Expand Down Expand Up @@ -454,40 +460,25 @@ chrome.webNavigation.onDOMContentLoaded.addListener(detail => {
// chrome.tabs.onReplaced.addListener(updateState);

// $FlowFixMe
chrome.tabs.onUpdated.addListener(defensify(async (tabId: number, info, tab: chrome$Tab) => {
// too spammy in logs
delete tab.favIconUrl
delete info.favIconUrl
//
console.debug('onUpdated %o %o', tab, info)
webNavigation = chrome.webNavigation;
webNavigation.onCompleted.addListener(defensify(async (info: WebNavigationInfo) => {
console.debug('onCompleted %o %o', info)

const url = tab.url
const url = info.url
const ireason = ignored(url)
if (ireason != null) {
/* on Vivaldi I've seen url being "" */
console.debug('onUpdated %s: ignoring, reason: %s', url, ireason)
console.debug('onCompleted %s: ignoring, reason: %s', url, ireason)
}
// right, tab updated triggered quite a lot, e.g. when the title is blinking
// ok, so far there are basically two cases
// 1. you open new tab. in that case 'url' won't be passed but onDomContentLoaded will be triggered
// 2. you navigate within the same tab, e.g. on youtube. then url will be passed, but onDomContentLoaded doesn't trigger. TODO not sure if it's always the case. maybe it's only YT
// TODO shit, so we might need to hide previous dots? ugh...

// TODO vvvv these might need to be cleaned up; not sure how relevant...
// page refresh: loading -> complete (no url at any point)
// clicking on link: loading (url) -> complete
// opening new link: loading -> loading (url) -> complete
// ugh. looks like 'complete' is the most realiable???
// but, I checked with 'complete' and sometimes it would reload many things with loading -> complete..... shit.

// also if you, say, go to web.telegram.org it's gonna show multiple notifications due to redirect... but perhaps this can just be suppressed..
if (info.frameId !== 0) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this check go first (before the URL check)? would make it faster and less logging

return
}

if (info['status'] != 'complete') {
return
}
console.info('requesting! %s', url)

try {
await updateState(tab);
await onTabLoaded(info);
} catch (error) {
const message = error.message;
if (message == null) {
Expand All @@ -505,8 +496,7 @@ chrome.tabs.onUpdated.addListener(defensify(async (tabId: number, info, tab: chr
}
throw error;
}
}, 'onUpdated'));

}, 'onCompleted'));

export async function getActiveTab(): Promise<?chrome$Tab> {
const tabs = await achrome.tabs.query({
Expand Down
1 change: 1 addition & 0 deletions extension/src/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

"tabs",
"activeTab",
"webNavigation",

"notifications"
],
Expand Down