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

webContents: handle POST navigation for new windows #7540

Merged
merged 15 commits into from Nov 14, 2016

Conversation

Projects
None yet
5 participants
@deepak1556
Member

deepak1556 commented Oct 9, 2016

  • Add docs and specs

Depends on electron/libchromiumcontent#236

Fixes #2816

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Oct 20, 2016

Member

@zeke @MarshallOfSound can i get verification on the docs ? Currently CI builds don't pass because of docs lint. Will the linter be updated to provide support for the new type ? Thanks!

Member

deepak1556 commented Oct 20, 2016

@zeke @MarshallOfSound can i get verification on the docs ? Currently CI builds don't pass because of docs lint. Will the linter be updated to provide support for the new type ? Thanks!

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Oct 20, 2016

Member

@deepak1556 If I get time at the weekend I'll PR in to get the linter working with this style. I'm heading into exams soon though so my time is becoming limited 😆

Member

MarshallOfSound commented Oct 20, 2016

@deepak1556 If I get time at the weekend I'll PR in to get the linter working with this style. I'm heading into exams soon though so my time is becoming limited 😆

@zeke zeke self-assigned this Oct 20, 2016

@zeke

This comment has been minimized.

Show comment
Hide comment
@zeke

zeke Oct 22, 2016

Member

Sorry to leave this open for so long, @deepak1556. If @MarshallOfSound doesn't get to it this weekend (school is important!), I will take care of updating the linter early next week.

Member

zeke commented Oct 22, 2016

Sorry to leave this open for so long, @deepak1556. If @MarshallOfSound doesn't get to it this weekend (school is important!), I will take care of updating the linter early next week.

@MarshallOfSound MarshallOfSound referenced this pull request Oct 22, 2016

Merged

Multi type values #61

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Oct 22, 2016

Member

@zeke electron/electron-docs-linter#61

Forgot about this 😆 Had the branch on my machine locally 👍

Member

MarshallOfSound commented Oct 22, 2016

@zeke electron/electron-docs-linter#61

Forgot about this 😆 Had the branch on my machine locally 👍

@zeke

This comment has been minimized.

Show comment
Hide comment
@zeke

zeke Oct 23, 2016

Member

@deepak1556 the docs linter has been updated to support multiple types, thanks to @MarshallOfSound. Give this a try and the PR should be good to go:

npm i -D electron-docs-linter@latest
Member

zeke commented Oct 23, 2016

@deepak1556 the docs linter has been updated to support multiple types, thanks to @MarshallOfSound. Give this a try and the PR should be good to go:

npm i -D electron-docs-linter@latest
@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Oct 23, 2016

Member

Have updated, Thanks! @zeke @MarshallOfSound

Member

deepak1556 commented Oct 23, 2016

Have updated, Thanks! @zeke @MarshallOfSound

@zeke

This comment has been minimized.

Show comment
Hide comment
@zeke

zeke Oct 24, 2016

Member

@deepak1556 tests are now passing but this needs a rebase.

Member

zeke commented Oct 24, 2016

@deepak1556 tests are now passing but this needs a rebase.

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Oct 24, 2016

Member

@zeke Done, thanks!

Member

deepak1556 commented Oct 24, 2016

@zeke Done, thanks!

@zeke

zeke approved these changes Oct 31, 2016

LGTM

@zeke

This comment has been minimized.

Show comment
Hide comment
@zeke

zeke Nov 4, 2016

Member

I'm happy with the docs and specs in this PR, but want input from @kevinsawicki or @zcbenz on the actual implementation before shipping.

Member

zeke commented Nov 4, 2016

I'm happy with the docs and specs in this PR, but want input from @kevinsawicki or @zcbenz on the actual implementation before shipping.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Nov 9, 2016

Contributor

I created some conflicts in this pull request via merging #7906 which landed electron/libchromiumcontent#236 on master, will resolve the conflicts and push them up here.

Contributor

kevinsawicki commented Nov 9, 2016

I created some conflicts in this pull request via merging #7906 which landed electron/libchromiumcontent#236 on master, will resolve the conflicts and push them up here.

@kevinsawicki kevinsawicki self-assigned this Nov 9, 2016

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Nov 9, 2016

Contributor

@deepak1556 I was trying to verify that the sample app from #2816 now worked and I hit an issue. It seems like POST requests made end up getting sent with no Content-Type header and so servers (like express) will fail to parse the data.

I tried with the following app:

main.js

var express = require('express');
var http = require('http');
var app = express();
var bodyParser = require('body-parser')

http.createServer(app).listen(8000);

app.use(bodyParser.json());
app.use(bodyParser.urlencoded({
  extended: true
}));

app.get('/', (request, response) => {
  response.sendFile(__dirname + '/index.html')
})

app.post('/bar', (request, response) => {
  response.send(request.body.data1).end()
})

let window

const {BrowserWindow} = require('electron')

require('electron').app.once('ready', () => {
  window = new BrowserWindow({
    width: 800,
    height: 600
  })
  window.loadURL('http://localhost:8000')
})

index.html

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <title></title>
  </head>
  <body>
    <form method='post' id='bar' name='bar' target='_blank' action='bar'>
      <textarea id='data1' name='data1'>Some text</textarea><br>
      <input type='submit' value='Submit'/>
    </form>
  </body>
</html>

This can be explicitly set by passing in extraHeaders to loadURL but that isn't exposed in <form> tag posts. Do you think this is feasible to support? Otherwise everything in this pull request looks good to me.

Contributor

kevinsawicki commented Nov 9, 2016

@deepak1556 I was trying to verify that the sample app from #2816 now worked and I hit an issue. It seems like POST requests made end up getting sent with no Content-Type header and so servers (like express) will fail to parse the data.

I tried with the following app:

main.js

var express = require('express');
var http = require('http');
var app = express();
var bodyParser = require('body-parser')

http.createServer(app).listen(8000);

app.use(bodyParser.json());
app.use(bodyParser.urlencoded({
  extended: true
}));

app.get('/', (request, response) => {
  response.sendFile(__dirname + '/index.html')
})

app.post('/bar', (request, response) => {
  response.send(request.body.data1).end()
})

let window

const {BrowserWindow} = require('electron')

require('electron').app.once('ready', () => {
  window = new BrowserWindow({
    width: 800,
    height: 600
  })
  window.loadURL('http://localhost:8000')
})

index.html

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <title></title>
  </head>
  <body>
    <form method='post' id='bar' name='bar' target='_blank' action='bar'>
      <textarea id='data1' name='data1'>Some text</textarea><br>
      <input type='submit' value='Submit'/>
    </form>
  </body>
</html>

This can be explicitly set by passing in extraHeaders to loadURL but that isn't exposed in <form> tag posts. Do you think this is feasible to support? Otherwise everything in this pull request looks good to me.

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Nov 9, 2016

Member

@kevinsawicki thanks for the test case! it should be feasible, will look into it.

Member

deepak1556 commented Nov 9, 2016

@kevinsawicki thanks for the test case! it should be feasible, will look into it.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Nov 11, 2016

Contributor

Added a few more tests and rebased and resolved merge conflicts with the docs, will merge this once CI passes 👍

Contributor

kevinsawicki commented Nov 11, 2016

Added a few more tests and rebased and resolved merge conflicts with the docs, will merge this once CI passes 👍

kevinsawicki and others added some commits Nov 11, 2016

@kevinsawicki kevinsawicki merged commit 4867475 into electron:master Nov 14, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Nov 14, 2016

Contributor

Thanks for this @deepak1556 🚢 🎉

Contributor

kevinsawicki commented Nov 14, 2016

Thanks for this @deepak1556 🚢 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment