GPII-4410: Making links work without the extension #2
Conversation
src/background/openURLs.js
Outdated
| try { | ||
| const destinationUrl = new URL(destination); | ||
| openURLs.openTab(destinationUrl, refresh); | ||
| success = true; | ||
| } catch (e) { | ||
| success = false; | ||
| } |
There was a problem hiding this comment.
It appears that this try/catch block is to guard against malformed URLs used for the destinationUrl. e.g. refresh/https%3A%2F%2Fexample.com%2F. However, openURLs is an async function. So I don't believe that it will wait to make sure that it doesn't throw an error. You could use await on openURLs but will need to convert openURLs.handleRequest into an async function as well. According to webRequest.onBeforeRequest the handler can return a promise that resolves with a BlockingResponse. You'll also have to test that the polyfill can handle this feature for use in Chrome.
There was a problem hiding this comment.
YEs. In fact, that's why I moved the url parsing out of openURLs.
However, I've now moved the call outside of the try block to make my intention clearer.
| }, | ||
| 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] | ||
| } |
There was a problem hiding this comment.
Need test cases for when the response is {cancel: false}.
| try { | ||
| destinationUrl = new URL(destination); | ||
| } catch (e) { | ||
| // ignored |
There was a problem hiding this comment.
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.
| }, { | ||
| name: "Bad URL", | ||
| details: { | ||
| url: "http://morphic.org/redirect/stupid" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes please change to what you think is most accurate term
| @@ -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); | |||
There was a problem hiding this comment.
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.
This makes the entire destination URL be specified. That way, a https link can be specified, without requiring the request url to be https.
Also,
/requestis at the start of the path to allow it to be passed easily to the shared listener on morphic, for when the extension isn't installed - so the url will always be a valid url which points to the destination.Example:
http://opensametab.morphic.org/redirect/https%3A%2F%2Fexample.com