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.

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

add fallback if document.currentScript is not available (e.g. with Webkitties) #694

Merged
merged 1 commit into from
Jul 4, 2014

Conversation

kossebau
Copy link
Contributor

@kossebau kossebau commented Jul 3, 2014

Fixes #693

@kogmbh-ci
Copy link

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1898/

a, pathname, pos;

if (document.currentScript && document.currentScript.src) {
scriptElement = document.currentScript.src;
} else {
scriptElement = (function() {
Copy link
Member

Choose a reason for hiding this comment

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

Please make this one or more separate documented functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@kogmbh-ci
Copy link

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1899/

@kossebau kossebau added this to the WebODF 0.5.1 milestone Jul 3, 2014
pathname = a.pathname;

pos = pathname.lastIndexOf("/");
if (pos !== -1) {
path = pathname.substr(0, pos);
}
} else {
throw "Could not estimate installationPath of the Wodo.TextEditor.";
Copy link
Member

Choose a reason for hiding this comment

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

Throwing is nice, but who catches this?
This script is run globally, so throwing here means wodo namespace is undefined.
Since that's exceptional i.e. an unexpected runtime environment or bad setup, showing an alert is permissible in my opinion. At any rate, documenting the expected global outcome here makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Throwing is nice, but who catches this?

Hm, right. If that is included by a <script> element in the header, I have no idea who could catch that. Written without thinking more it seems (perhaps not wanting to spent time on the unexpected, meh).
So an alert is the usual pattern on an error during code run on inclusion of a JS file? Okay, changed.
Not clear to me: where would you expect the documentation to be done, and what exactly do you mean with "expected global outcome"?

@kogmbh-ci
Copy link

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1904/

@vandenoever
Copy link
Member

Works in chromium and firefox. Tried in KHTML 4.12.4 but that does not work, I'm not sure if this patch is supposed to fix that.

vandenoever pushed a commit that referenced this pull request Jul 4, 2014
add fallback if document.currentScript is not available (e.g. with Webkitties)
@vandenoever vandenoever merged commit 471ccbe into webodf:master Jul 4, 2014
@kossebau kossebau deleted the workaroundMissingCurrentScript branch July 4, 2014 23:03
@ronnicek
Copy link

ronnicek commented Jul 5, 2014

Thanks, WebODF is now working in Safari 👍

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

Successfully merging this pull request may close these issues.

WebODF editor demo in Safari
4 participants