Skip to content
This repository has been archived by the owner on Dec 27, 2023. It is now read-only.

JSON Object recieving #92

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

bhavul
Copy link
Contributor

@bhavul bhavul commented Mar 2, 2015

Recieving and resizing works. This is in reference to #19 .
However, I'm confused as to what to proceed to at this point. We want two methods showInjectedFrame and destroyInjectedFrame in host_integration_page.js which have to send another message to privly.js. So, it's more like an extra argument or variable in the JSON Object that privly.js recieves. Am I right?

But, what after that?

//Get the element by name.
var data = message.data.split(",");
var iframe = document.getElementsByName(data[0])[0];
var frameName = msg.frame_id;
Copy link
Member

Choose a reason for hiding this comment

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

Many messages are sent between many different contexts. Here you should also check that the message you are expecting was sent by looking at the keys in the message. You should also handle improperly formatted JSON.

@smcgregor
Copy link
Member

I think you first need to make a document that gives a (very basic) API between the injected application and the content script (privly.js).

Something like:
{command: "resize|remove|show|hide", size: "NUMBER OF PIXELS"}

But stated more formally so we can put it on the wiki.

After that, you only need to ensure the content script supports the API since the privly-application can decide how to use the commands without you needing to change the apps.

@bhavul
Copy link
Contributor Author

bhavul commented Mar 8, 2015

@smcgregor by making a document that provides an API, you mean a doc, a manual, a readme basically, right? Which tells about various commands that can be sent and the options that can be given for them.

@smcgregor
Copy link
Member

Yep!

@bhavul
Copy link
Contributor Author

bhavul commented Mar 9, 2015

@smcgregor all right. Done with most of the work, but what's the difference between remove and hide here?
What should the content script do in each case?
And, do I need to add functions for any other commands other than the 4 you described? i've made a readme/manual file detailing an API that has been used.

@smcgregor
Copy link
Member

smcgregor commented Mar 9, 2015 via email

@bhavul
Copy link
Contributor Author

bhavul commented Mar 9, 2015

@smcgregor and in both cases we replace it with the original URL too? (Actually this was my question)

@smcgregor
Copy link
Member

smcgregor commented Mar 9, 2015 via email

@bhavul
Copy link
Contributor Author

bhavul commented Mar 11, 2015

@smcgregor Let me know if things look good now.

@@ -390,6 +390,9 @@ var privly = {
//Set the source URL
iFrame.setAttribute("src", applicationUrl);

//URL which it will be replacing
iFrame.setAttribute("data-privlyHref",object.getAttribute("data-privlyHref"));
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't this set elsewhere? Why are you setting it now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smcgregor In which field is it set? I checked the whole iframe element by going through it, the original link wasn't there. So, I added this and have used it.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smcgregor Yes. It is set for the element 'a'. And, 'a' is local to that method. I just have access to iframe element here, so had to put the original link as a new attribute into iframe.
Is there a way I could get 'a' corresponding to the iframe directly?

@bhavul
Copy link
Contributor Author

bhavul commented Mar 16, 2015

@smcgregor fixed the pointed out errors and some more nits I observed.

window.removeEventListener("message", privly.showIframePostedMessage,
false);

window.removeEventListener("message", privly.hideIframePostedMessage,
Copy link
Member

Choose a reason for hiding this comment

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

Leftovers.

@bhavul
Copy link
Contributor Author

bhavul commented Mar 19, 2015

@smcgregor I'm done with the edits in privly.js. Now, should I add more functions like dispatchResize for other commands in host_page_integration.js and then create unit test n integration tests for those OR, create unit tests and integration tests for privly.js itself, and then add more functions to host_page_integration.js ?

@smcgregor
Copy link
Member

Although any well written test is a welcome addition, I think it is best to concentrate on the feature set you are developing here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants