Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ npm install

### Build

#### Including depdencies
#### Including dependencies

For testing the extension in the browser, you'll need to ensure that the 3rd party dependencies have been copied to the
source directory. This is performed by the following grunt task:
Expand Down Expand Up @@ -103,7 +103,7 @@ In both cases the extension will intercept the URL, remove the identifier and pa
existing tabs/windows open to the requested URL, the URL is handled as normal. If one already exists it will open to
that existing tab instead. In the case of `refreshsametab.morphic.org`, it will also trigger the page to reload.

For example, `http://opensametab.morphic.org/en.wikipedia.org/wiki/URL` will open `https://en.wikipedia.org/wiki/URL`.
For example, `http://opensametab.morphic.org/redirect/https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FURL` will open `https://en.wikipedia.org/wiki/URL`.

#### Caveats

Expand Down
24 changes: 17 additions & 7 deletions src/background/openURLs.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@
const openURLs = {};

openURLs.openTab = async (url, refresh) => {
const urlObj = new URL(url);
const queryURL = urlObj.href.replace(urlObj.protocol, "*:");
const queryURL = url.href.replace(url.protocol, "*:");

let [matchedTab] = await browser.tabs.query({url: queryURL});
let [loadingTab] = await browser.tabs.query({"status": "loading", "windowId": browser.windows.WINDOW_ID_CURRENT});
Expand All @@ -37,7 +36,7 @@ openURLs.openTab = async (url, refresh) => {
browser.tabs.reload(matchedTab.id);
}
} else {
browser.tabs.update(loadingTab.id, {url});
browser.tabs.update(loadingTab.id, {url: url.href});
}
};

Expand All @@ -53,12 +52,23 @@ openURLs.getFilter = identifiers => {
};

openURLs.handleRequest = details => {
const refresh = details.url.indexOf(openURLs.identifiers.refreshTab) >= 0;
const url = details.url.replace(`${openURLs.identifiers[refresh ? "refreshTab" : "openTab"]}/`, "");
const url = new URL(details.url);
const refresh = url.hostname.indexOf(openURLs.identifiers.refreshTab) >= 0;
// URL will look like: "http://opensametab.morphic.org/redirect/https%3A%2F%2Fexample.com%2F"
const destination = url.pathname.replace(/^\/(redirect\/)?(.*)/, (match, c1, c2) => decodeURIComponent(c2));

openURLs.openTab(url, refresh);
let destinationUrl;
try {
destinationUrl = new URL(destination);
} catch (e) {
// ignored
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wonder if it might be helpful to log the error in a meaningful way. It likely won't make a difference for most end users, but for someone who is developing and runs into this case, it will hopefully help track things down quicker.

}

if (destinationUrl) {
openURLs.openTab(destinationUrl, refresh);
}

return {cancel: true};
return {cancel: !!destinationUrl};
};

openURLs.bindListener = () => {
Expand Down
27 changes: 18 additions & 9 deletions tests/js/openURLsTests.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@
blankTab: {
id: 1
},
url: "https://actual.org/url",
url: new URL("https://actual.org/url"),
queryURL: "*://actual.org/url"
};

Expand Down Expand Up @@ -134,7 +134,7 @@
jqUnit.assertTrue("Tab isn't highlighted", browser.tabs.highlight.notCalled);
jqUnit.assertTrue("Loading tab isnt' removed", browser.tabs.remove.notCalled);
jqUnit.assertTrue("Tab isn't reloaded", browser.tabs.reload.notCalled);
let isUpdatedCalled = browser.tabs.update.calledOnceWithExactly(loadingTab.id, {url: openTabTestsProps.url});
let isUpdatedCalled = browser.tabs.update.calledOnceWithExactly(loadingTab.id, {url: openTabTestsProps.url.href});
jqUnit.assertTrue("Loading tab is updated with correct URL", isUpdatedCalled);
}

Expand All @@ -151,29 +151,38 @@
const handlestRequestTestCases = [{
name: "Open same tab",
details: {
url: "https://opensametab.morphic.org/actual.org/url"
url: "https://opensametab.morphic.org/redirect/https%3A%2F%2Factual.org/url"
},
expected: {
response: {cancel: true},
args: ["https://actual.org/url", false]
args: [new URL("https://actual.org/url"), false]
}
}, {
name: "Refresh tab",
details: {
url: "http://refreshsametab.morphic.org/actual.org/url"
url: "http://refreshsametab.morphic.org/redirect/http%3A%2F%2Factual.org/url"
},
expected: {
response: {cancel: true},
args: ["http://actual.org/url", true]
args: [new URL("http://actual.org/url"), true]
}
}, {
name: "Unfiltered URL",
details: {
url: "http://morphic.org/actual.org/url"
url: "http://morphic.org/redirect/https%3A%2F%2Factual.org/url"
},
expected: {
response: {cancel: true},
args: ["http://morphic.org/actual.org/url", false]
args: [new URL("http://morphic.org/redirect/https%3A%2F%2Factual.org/url"), false]
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Need test cases for when the response is {cancel: false}.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

}, {
name: "Bad URL",
details: {
url: "http://morphic.org/redirect/stupid"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see where you're going with this, but wonder if it would be more appropriate to replace "stupid" with something like "incorrect", "broken", "invalid" or something along those lines.

Copy link
Copy Markdown

@GreggVan GreggVan Sep 14, 2020

Choose a reason for hiding this comment

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

Yes please change to what you think is most accurate term

},
expected: {
response: {cancel: false},
args: null
}
}];

Expand All @@ -184,7 +193,7 @@
let response = openURLs.handleRequest(testCase.details);
jqUnit.assertDeepEq(`${testCase.name}: the response is returned correctly`, testCase.expected.response, response);

let isOpenTabCalledProperly = openTabStub.calledOnceWithExactly.apply(openTabStub, testCase.expected.args);
let isOpenTabCalledProperly = !testCase.expected.args || openTabStub.calledOnceWithExactly.apply(openTabStub, testCase.expected.args);
Copy link
Copy Markdown
Collaborator

@jobara jobara Jun 2, 2020

Choose a reason for hiding this comment

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

I worry that by just passing any test that has a falsey value for args would allow tests that forget to add the args value to keep erroneously pass. Instead, I think we should provide an error flag in the expected object. If that is present, we make sure that openTabStub is not called at all; which I believe can be done using openTabStub.notCalled.

jqUnit.assertTrue(`${testCase.name}: the openURLs.openTab method was called with the correct args`, isOpenTabCalledProperly);

// clean up
Expand Down