[CLOSED] move inline js into external files

[core-ai-bot]

Issue by maks
Sunday Jun 30, 2013 at 10:59 GMT
Originally opened as adobe/brackets#4374

---

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

---

maks included the following code: https://github.com/adobe/brackets/pull/4374/commits

[core-ai-bot]

Comment by tomByrer
Monday Jul 01, 2013 at 11:18 GMT

---

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

[core-ai-bot]

Comment by maks
Monday Jul 01, 2013 at 11:43 GMT

---

@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.

[core-ai-bot]

Comment by maks
Monday Jul 01, 2013 at 11:54 GMT

---

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).

[core-ai-bot]

Comment by tomByrer
Monday Jul 01, 2013 at 16:07 GMT

---

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 -->

[core-ai-bot]

Comment by maks
Wednesday Jul 03, 2013 at 00:04 GMT

---

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

[core-ai-bot]

Comment by JeffryBooher
Monday Jul 15, 2013 at 18:52 GMT

---

@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

[core-ai-bot]

Comment by TomMalbran
Monday Jul 15, 2013 at 20:51 GMT

---

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

[core-ai-bot]

Comment by JeffryBooher
Monday Jul 15, 2013 at 21:18 GMT

---

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

[core-ai-bot]

Comment by maks
Monday Jul 15, 2013 at 23:05 GMT

---

@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.

[core-ai-bot]

Comment by JeffryBooher
Tuesday Jul 16, 2013 at 18:01 GMT

---

@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?

[core-ai-bot]

Comment by maks
Wednesday Jul 17, 2013 at 12:39 GMT

---

@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?

[core-ai-bot]

Comment by JeffryBooher
Wednesday Jul 17, 2013 at 16:35 GMT

---

@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) {

[core-ai-bot]

Comment by JeffryBooher
Thursday Jul 18, 2013 at 18:38 GMT

---

Changes look good!

• All Lint errors fixed
• All Unit Tests Pass
• All Integration Tests Pass!
  Merging....

[core-ai-bot]

Comment by JeffryBooher
Thursday Jul 18, 2013 at 18:38 GMT

---

Merged