Create safari extension from scratch. Clicking the extension takes us to...

[sambuddhabasu]

... the privly homepage

privly_build extension updated

[sambuddhabasu]

As per the discussion with @smcgregor on the IRC, the safari extension needs a lot of work and hence, will be created from scratch. This commit removes the old code and is a basic functioning extension which only takes us to the privly homepage till now. More work is in progress.

[smcgregor]

Let me know if you have questions.

[sambuddhabasu]

The PR has been updated with a basic working safari extension. Also, this has been tested manually locally and it seems to work with the cases. Please do let me know what you think about this.

[smcgregor]

Are you injecting remote iframes? You aren't bundling the privly-applications repo as a submodule in this PR, so I would think that is the case.

[sambuddhabasu]

Yes, currently the code makes use of injecting remote iframes. It works for some sites but for the others which specify a 'x-frame-options: SAMEORIGIN' or something of that sort, it will fail. To make the extension work for all kinds of cases, local injection needs to happen.

[smcgregor]

I recommend not working on any UI elements until you have a proof-of-concept for local injection. As soon as you have that, I will start reviewing.

[sambuddhabasu]

@smcgregor By proof of concept for local injection, do you mean just making the context script add newly created iframes into the document?

[smcgregor]

No, it goes beyond that. The iframe must have a src attribute that points to an HTML document that is packaged in the extension.

[sambuddhabasu]

As you mentioned to read the academic paper on privly on the IRC, I did read that. It came to my notice that, the current code for the safari extension in this PR, parses the privly hyperlinks and asks show.js to show them into the iframes using privly-applications. I dont quite understand what you mean by the HTML document that is packaged in the extension. As far as I understood, this be a sample HTML document to make sure that the iframe can load local documents(locak injection)

[smcgregor]

As far as I understood, this be a sample HTML document to make sure that the iframe can load local documents(locak injection)

I think you are correct there, but I don't understand what you are saying with "show.js."

A summary is:

1. The extension's content script finds a specially formatted URL
2. The Content script "injects" this URL as an iframe. The iframe's src attribute should then be the Safari equivalent of chrome-extension://privly/privly-applications/PlainPost/show.html.

[sambuddhabasu]

Thanks for the clarification, looking into it right away.

[sambuddhabasu]

@smcgregor The current code in this PR, now finds any hyperlink on the document and appends an iframe next to it with which has a src attribute equal to loadPage.html packaged with the extension. This makes sure that local injection is possible for the safari extension.

[smcgregor]

:) Excellent!

How about we move forward with packaging the privly-applications repo as a git submodule?

[sambuddhabasu]

Yes, I have added the privly-applications as a git submodule. Please review it and let me know what you think about this. Thanks for helping me out. Also, do let me know if this PR is ready to be reviewed, then, I will rebase and squash the commits into one.

[smcgregor]

You are almost ready for review:

• Is there a reason most of the extension is in a subdirectory?
• You are very close to having the extension integrated with the privly-applications repo. You should change the line that injects the proof-of-concept HTML doc to be the application selected by the link. We currently do this here in Privly-chrome.

[sambuddhabasu]

According to me, it looks good to keep the main extension files in a subdirectory and keep the other files there itself. This can be seen in most of the projects, where usually the main files that are used for development are under the src subdirectory. Also, the line that injects the proof-of-concept HTML doc now points to the application selected by the link. Please let me know if any further changes are required. Thanks

[smcgregor]

A few comments, but generally good and minimal.
Should I install and test it on Twitter? What will I see?

[sambuddhabasu]

The safari extension can now be installed and tested. It will not work on the twitter page that was specified by you as twitter links first need to be changed to normal URLs. However, I have created a test page on which the extension can be tested

[sambuddhabasu]

@smcgregor As per our discussion, you had mentioned that privly.js does not need to be coded from scratch. If we look at the previous privly.js file, most of the checks that were done with the links and the functions are outdated. This will make it difficult to change the whole existing code to the new format. Rather, I can follow the chrome and firefox extension format and start coding the privly.js file from scratch. What do you think about this?

[smcgregor]

Copy privly.js from chrome and modify it to work on safari.

[sambuddhabasu]

Sure, working on it then. Thanks for guiding me

[smcgregor]

In the future, pull requests shouldn't include the built extension.

[smcgregor]

I built the extension from the extension builder and installed it, but I get a 404 for all the iframes. Is this expected? See below.

[sambuddhabasu]

Hello @smcgregor , I have not updated this pull request with the latest changes in code. I will change the code shortly and update the pull request. I will look into why all the iframes are giving a 404.

[sambuddhabasu]

@smcgregor The extension now works perfectly fine on the privly test page. The iframes now get resized based on the message received. The parts missing in the current PR, is addition of the correctIndirection() function. Also, the submodule privly-applications has been added and the iframes are added with the help of local injection

[smcgregor]

You are in the review queue. Can I ask why correctIndirection() isn't in the PR? Were there issues porting the current version of privly.js? Much of the script should have been pretty easy to drop in.

[sambuddhabasu]

I thought that the correctIndirection() can be added as a part of the following PR. If needed, I can add it to the existing PR too. Please let me know what you think about this. Also, there were no significant issues porting the current version of privly.js

[smcgregor]

For my own notes:

1. Take a diff of privly.js to compare it to the Chrome version
2. Merge this pull request
3. Ask @sammyshj to open a subsequent pull request to update the README
4. Look at @sammyshj's development plans

[smcgregor]

It looks like there are many changes to the privly.js script. This script needs to remain largely standardized between the extensions. Which version of privly.js are you pulling your changes from?

[sambuddhabasu]

The safari privly.js was pulled from both the chrome and the firefox privly.js. I will update the code so that the script remains standardised between the extensions

[sambuddhabasu]

@smcgregor The code has been updated so that the script remains standardized between the extensions

[smcgregor]

This is almost ready.

The idea behind maintaining a single version of privly.js is you could (almost) drop it into any of the extensions without making any changes. You are almost there with privly.js, but you still ripped out platform-specific code and put in your own platform specific code.

You should wrap your platform-specific code in a platform test that indicates the script is running on Safari.

[sambuddhabasu]

The current privly.js in the chrome extension does a check whether chrome is undefined. If that check is done for the Safari extension too, an error is encountered which says, ReferenceError: Can't find variable: chrome Hence, the check inside the injectLink() function should be done with navigator.userAgent first. This makes sure that there is no error and works perfectly fine on both Chrome and Safari. Should I update the code with navigator.userAgent check in both the Chrome and Safari extensions?

[smcgregor]

Getting user agent detection right is surprisingly difficult. What about changing it to:

typeof chrome !== "undefined"

instead of chrome !== undefined?

[sambuddhabasu]

Yes, that works. I have updated privly.js with the user agent detection check for application injection

[sambuddhabasu]

@smcgregor Let me know if any other changes are required. If not, I will go ahead and squash the commits into one. Thanks

[smcgregor]

I think this is good to go. Did you want to squash your commits first?

[sambuddhabasu]

Yes, I have squashed the commits into one. This way, the overall commit message will be clear and clean. Thanks