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

move inline js into external files #4374

Merged
merged 7 commits into from Jul 18, 2013

Conversation

@maks
Copy link
Contributor

commented Jun 30, 2013

chrome packaged apps are not allowed to use inline script elements and functionally there doesn't seem to be any reason to keep these as inline in index.html

@tomByrer

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2013

I personally prefer to have fewer files; would reduce program boot time & HD space.

@maks

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2013

@tomByrer The problem though is that inline script elements are not permitted for chrome packaged apps (or extensions): http://developer.chrome.com/apps/app_csp.html
hence my pull request as this is one thing that can't be placed behind runtime checks the way global.inBrowser does for example. Also when I raised this on the mailinglist, @jasonsanjose seemed ok with making this change.

@maks

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2013

I've also made some fixes that should fix the CI build failures due to JSLint errors as well as adding the std adobe license header (as copied from brackets.js).

@tomByrer

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2013

Ah, I see ty.
If that is the case, then wouldn't it be wise to put in the reasoning & the link you posted for me, so future editors don't repeat the same mistake?

<!-- All scripts must be external: http://developer.chrome.com/apps/app_csp.html -->
@maks

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2013

@tomByrer yes very good point! I've added that comment in.

@ghost ghost assigned JeffryBooher Jul 10, 2013
@JeffryBooher

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2013

@maks I think you just need to fix the jsHint error and this can be merged in to master. You just need to move the || up to the previous line before the break. I know this is legacy but now that it's in a a js file it is getting scanned by Travis for jsHint failures. I would fix it but I don't want to commit to your private fork and I probably don't have permission to commit directly to it as-is.

Here's a good jsHint discussion on Automatic Semicolon Insertion (ASI): http://stackoverflow.com/questions/2846283/what-are-the-rules-for-javascripts-automatic-semicolon-insertion-asi

@TomMalbran

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2013

The second file xorigin.js, might need the copyright too.

@JeffryBooher

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2013

Good Catch @TomMalbran. @maks please add copyright statement to xorigin.js. Thanks!

@maks

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2013

@TomMalbran @JeffryBooher thanks both for the review. Having trailing operators is my preferred style as well and my local jshint was complaining about it to me as well - so happily fixed.

@JeffryBooher

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2013

@maks you're gonna hate me but I failed to notice that there were several lint errors in both of these files. Can you fix these and I will merge?

@maks

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2013

@JeffryBooher no probs, happy to fix it, but I'm not seeing anymore JShint errors here on my local machine, could you let me know what errors you are getting?

@JeffryBooher

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2013

@maks, I found these by turning on JSLint in Brackets and opening the newly created JS files:

in xorigin.js:

32  'navigator' was used before it was defined.  if (navigator.userAgent.search(" Chrome/") !== -1) {
45   Unexpected '(space)'.                       if (typeof (brackets) !== "undefined" ||
46  'document' was used before it was defined.   document.location.href.substr(0, 7) !== "file://" ||
52  'window' was used before it was defined.     var previousErrorHandler = window.onerror;

In dependencies.js

31   The body of a for in should be wrapp...     for (key in deps) {
40   'document' was used before it was defined.  document.write("<h1>Missing libraries</h1>");
43   The body of a for in should be wrapped...   for (key in deps) {
maks added 2 commits Jul 18, 2013
loop code in dependencies.js refactored to meet Brackets coding
conventions.
because they need to brought in via require which shouldn't be used here.
@JeffryBooher

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2013

Changes look good!

  • All Lint errors fixed
  • All Unit Tests Pass
  • All Integration Tests Pass!
    Merging....
JeffryBooher added a commit that referenced this pull request Jul 18, 2013
move inline js into external files
@JeffryBooher JeffryBooher merged commit f5978ec into adobe:master Jul 18, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@JeffryBooher

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2013

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.