You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by sosahvictor <gi...@git.apache.org> on 2014/11/06 23:22:45 UTC
[GitHub] cordova-browser pull request: CB-7978 Fixed launching chrome in Li...
GitHub user sosahvictor opened a pull request:
https://github.com/apache/cordova-browser/pull/4
CB-7978 Fixed launching chrome in Linux
- Fixed launching chrome in linux. It either launches chrome or google-chrome processes
- Reads config.xml looking for the source file to be used at the lauching of the browser. This will work for Linux, Windows and MacOs
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sosahvictor/cordova-browser master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-browser/pull/4.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #4
----
commit 87e19e2d69fadcc4909e9858ec94655436857ffe
Author: Victor Sosa <so...@gmail.com>
Date: 2014-11-06T22:13:46Z
CB-7978 Fixed launching chrome in Linux
- Fixed launching chrome in linux. It either launches chrome or google-chrome processes
- Reads config.xml looking for the source file to be used at the lauching of the browser. This will work for Linux, Windows and MacOs
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-browser pull request: CB-7978 Fixed launching chrome in Li...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-browser/pull/4
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-browser pull request: CB-7978 Fixed launching chrome in Li...
Posted by sosahvictor <gi...@git.apache.org>.
Github user sosahvictor commented on a diff in the pull request:
https://github.com/apache/cordova-browser/pull/4#discussion_r20829122
--- Diff: bin/templates/project/cordova/run ---
@@ -20,10 +20,33 @@
*/
var shell = require('shelljs'),
- spawn = require('child_process').spawn,
- project = 'file://' + shell.pwd() + '/platforms/browser/www/index.html';
+ fs = require('fs');
+var configFile = shell.pwd() + '/config.xml';
+var configXML = fs.readFileSync(configFile, 'utf8');
+var sourceFile = /<content[\s\S]*?src\s*=\s*"(.*?)"/i.exec(configXML);
+var spawn = require('child_process').spawn,
+ tmpDir = '/tmp/temp_chrome_user_data_dir_for_cordova_browser/',
+ project = 'file://' + shell.pwd() + '/platforms/browser/www/' + sourceFile[1];
+
+fs.mkdir(tmpDir);
--- End diff --
Fixed in new commit
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-browser pull request: CB-7978 Fixed launching chrome in Li...
Posted by mmocny <gi...@git.apache.org>.
Github user mmocny commented on a diff in the pull request:
https://github.com/apache/cordova-browser/pull/4#discussion_r20839578
--- Diff: bin/templates/project/cordova/run ---
@@ -20,10 +20,33 @@
*/
var shell = require('shelljs'),
- spawn = require('child_process').spawn,
- project = 'file://' + shell.pwd() + '/platforms/browser/www/index.html';
+ fs = require('fs');
+var configFile = shell.pwd() + '/config.xml';
+var configXML = fs.readFileSync(configFile, 'utf8');
+var sourceFile = /<content[\s\S]*?src\s*=\s*"(.*?)"/i.exec(configXML);
+var spawn = require('child_process').spawn,
+ tmpDir = '/tmp/temp_chrome_user_data_dir_for_cordova_browser/',
+ project = 'file://' + shell.pwd() + '/platforms/browser/www/' + sourceFile[1];
+
+fs.mkdir(tmpDir);
switch (process.platform) {
+ case 'linux':
+ var args = ['--disable-web-security', '--user-data-dir=/tmp/temp_chrome_user_data_dir_for_cordova_browser', project];
+ var chromeProcess = spawn('chrome', args);
+
+ chromeProcess.on('close', function(code) {
+ if (code != 0) {
--- End diff --
I'll land as-is and we can iterate. Not a big deal, but lets try to do the right thing on all platforms & targets, whatever that is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-browser pull request: CB-7978 Fixed launching chrome in Li...
Posted by mmocny <gi...@git.apache.org>.
Github user mmocny commented on a diff in the pull request:
https://github.com/apache/cordova-browser/pull/4#discussion_r20823674
--- Diff: bin/templates/project/cordova/run ---
@@ -20,10 +20,33 @@
*/
var shell = require('shelljs'),
- spawn = require('child_process').spawn,
- project = 'file://' + shell.pwd() + '/platforms/browser/www/index.html';
+ fs = require('fs');
+var configFile = shell.pwd() + '/config.xml';
+var configXML = fs.readFileSync(configFile, 'utf8');
+var sourceFile = /<content[\s\S]*?src\s*=\s*"(.*?)"/i.exec(configXML);
+var spawn = require('child_process').spawn,
+ tmpDir = '/tmp/temp_chrome_user_data_dir_for_cordova_browser/',
+ project = 'file://' + shell.pwd() + '/platforms/browser/www/' + sourceFile[1];
+
+fs.mkdir(tmpDir);
switch (process.platform) {
+ case 'linux':
+ var args = ['--disable-web-security', '--user-data-dir=/tmp/temp_chrome_user_data_dir_for_cordova_browser', project];
+ var chromeProcess = spawn('chrome', args);
+
+ chromeProcess.on('close', function(code) {
+ if (code != 0) {
--- End diff --
Actually, I'm not sure at all why you spawn & unref `google-chrome` but not `chrome`. Is this intentional? Do they run differently somehow?
Also a comment about the fallback to `google-chrome` if `chrome` fails to launch would be useful.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-browser pull request: CB-7978 Fixed launching chrome in Li...
Posted by sosahvictor <gi...@git.apache.org>.
Github user sosahvictor commented on a diff in the pull request:
https://github.com/apache/cordova-browser/pull/4#discussion_r20828764
--- Diff: bin/templates/project/cordova/run ---
@@ -20,10 +20,33 @@
*/
var shell = require('shelljs'),
- spawn = require('child_process').spawn,
- project = 'file://' + shell.pwd() + '/platforms/browser/www/index.html';
+ fs = require('fs');
+var configFile = shell.pwd() + '/config.xml';
+var configXML = fs.readFileSync(configFile, 'utf8');
+var sourceFile = /<content[\s\S]*?src\s*=\s*"(.*?)"/i.exec(configXML);
+var spawn = require('child_process').spawn,
+ tmpDir = '/tmp/temp_chrome_user_data_dir_for_cordova_browser/',
+ project = 'file://' + shell.pwd() + '/platforms/browser/www/' + sourceFile[1];
--- End diff --
Thanks for the tip. Adding this in the PR
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-browser pull request: CB-7978 Fixed launching chrome in Li...
Posted by sosahvictor <gi...@git.apache.org>.
Github user sosahvictor commented on a diff in the pull request:
https://github.com/apache/cordova-browser/pull/4#discussion_r20828724
--- Diff: bin/templates/project/cordova/run ---
@@ -20,10 +20,33 @@
*/
var shell = require('shelljs'),
- spawn = require('child_process').spawn,
- project = 'file://' + shell.pwd() + '/platforms/browser/www/index.html';
+ fs = require('fs');
+var configFile = shell.pwd() + '/config.xml';
+var configXML = fs.readFileSync(configFile, 'utf8');
+var sourceFile = /<content[\s\S]*?src\s*=\s*"(.*?)"/i.exec(configXML);
+var spawn = require('child_process').spawn,
+ tmpDir = '/tmp/temp_chrome_user_data_dir_for_cordova_browser/',
+ project = 'file://' + shell.pwd() + '/platforms/browser/www/' + sourceFile[1];
+
+fs.mkdir(tmpDir);
switch (process.platform) {
+ case 'linux':
+ var args = ['--disable-web-security', '--user-data-dir=/tmp/temp_chrome_user_data_dir_for_cordova_browser', project];
+ var chromeProcess = spawn('chrome', args);
+
+ chromeProcess.on('close', function(code) {
+ if (code != 0) {
--- End diff --
You are right, something I didn't notice at that time. The `google-chrome` process is detached, so if the user wants to keep using Cordova CLI for other tasks he can do it, but this is not the same as for `chrome` process.
Moreover, the other platforms (Mac and Windows) don't have a handler like the one in Linux I'm proposing. My question to you is, should I get rid of the `unref` and let the child process block the parent (and hence the user won't be able to use the CLI until he/she closes chrome) or unify the behavior for other platforms as well? Keep in mind that the change will affect other platforms, not just Linux as in this PR
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-browser pull request: CB-7978 Fixed launching chrome in Li...
Posted by mmocny <gi...@git.apache.org>.
Github user mmocny commented on a diff in the pull request:
https://github.com/apache/cordova-browser/pull/4#discussion_r20823394
--- Diff: bin/templates/project/cordova/run ---
@@ -20,10 +20,33 @@
*/
var shell = require('shelljs'),
- spawn = require('child_process').spawn,
- project = 'file://' + shell.pwd() + '/platforms/browser/www/index.html';
+ fs = require('fs');
+var configFile = shell.pwd() + '/config.xml';
+var configXML = fs.readFileSync(configFile, 'utf8');
+var sourceFile = /<content[\s\S]*?src\s*=\s*"(.*?)"/i.exec(configXML);
+var spawn = require('child_process').spawn,
+ tmpDir = '/tmp/temp_chrome_user_data_dir_for_cordova_browser/',
+ project = 'file://' + shell.pwd() + '/platforms/browser/www/' + sourceFile[1];
--- End diff --
Again, `path.resolve(path.join('platforms', 'browser', 'www', sourceFile[1]))`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-browser pull request: CB-7978 Fixed launching chrome in Li...
Posted by mmocny <gi...@git.apache.org>.
Github user mmocny commented on a diff in the pull request:
https://github.com/apache/cordova-browser/pull/4#discussion_r20823525
--- Diff: bin/templates/project/cordova/run ---
@@ -20,10 +20,33 @@
*/
var shell = require('shelljs'),
- spawn = require('child_process').spawn,
- project = 'file://' + shell.pwd() + '/platforms/browser/www/index.html';
+ fs = require('fs');
+var configFile = shell.pwd() + '/config.xml';
+var configXML = fs.readFileSync(configFile, 'utf8');
+var sourceFile = /<content[\s\S]*?src\s*=\s*"(.*?)"/i.exec(configXML);
+var spawn = require('child_process').spawn,
+ tmpDir = '/tmp/temp_chrome_user_data_dir_for_cordova_browser/',
+ project = 'file://' + shell.pwd() + '/platforms/browser/www/' + sourceFile[1];
+
+fs.mkdir(tmpDir);
switch (process.platform) {
+ case 'linux':
+ var args = ['--disable-web-security', '--user-data-dir=/tmp/temp_chrome_user_data_dir_for_cordova_browser', project];
+ var chromeProcess = spawn('chrome', args);
+
+ chromeProcess.on('close', function(code) {
+ if (code != 0) {
--- End diff --
The other platforms don't do output redirection. If you think this is a good idea, please consider moving it out to a separate PR. I wouldn't want each platform to do something different.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-browser pull request: CB-7978 Fixed launching chrome in Li...
Posted by mmocny <gi...@git.apache.org>.
Github user mmocny commented on a diff in the pull request:
https://github.com/apache/cordova-browser/pull/4#discussion_r20823313
--- Diff: bin/templates/project/cordova/run ---
@@ -20,10 +20,33 @@
*/
var shell = require('shelljs'),
- spawn = require('child_process').spawn,
- project = 'file://' + shell.pwd() + '/platforms/browser/www/index.html';
+ fs = require('fs');
+var configFile = shell.pwd() + '/config.xml';
--- End diff --
Consider using `path.join` in the future, but in this case, perhaps use `path.resolve('config.xml')` instead.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-browser pull request: CB-7978 Fixed launching chrome in Li...
Posted by mmocny <gi...@git.apache.org>.
Github user mmocny commented on a diff in the pull request:
https://github.com/apache/cordova-browser/pull/4#discussion_r20823145
--- Diff: bin/templates/project/cordova/run ---
@@ -20,10 +20,33 @@
*/
var shell = require('shelljs'),
- spawn = require('child_process').spawn,
- project = 'file://' + shell.pwd() + '/platforms/browser/www/index.html';
+ fs = require('fs');
+var configFile = shell.pwd() + '/config.xml';
+var configXML = fs.readFileSync(configFile, 'utf8');
+var sourceFile = /<content[\s\S]*?src\s*=\s*"(.*?)"/i.exec(configXML);
+var spawn = require('child_process').spawn,
+ tmpDir = '/tmp/temp_chrome_user_data_dir_for_cordova_browser/',
+ project = 'file://' + shell.pwd() + '/platforms/browser/www/' + sourceFile[1];
+
+fs.mkdir(tmpDir);
--- End diff --
I get an error that dir is already created if I run this twice. Also, this call is async so is dangerous as is.
Probably you want to use `shelljs` instead: `shelljs.mkdir('-p', tmpDir).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org