{"action":"created","issue":{"url":"https://api.github.com/repos/servo/servo/issues/17083","repository_url":"https://api.github.com/repos/servo/servo","labels_url":"https://api.github.com/repos/servo/servo/issues/17083/labels{/name}","comments_url":"https://api.github.com/repos/servo/servo/issues/17083/comments","events_url":"https://api.github.com/repos/servo/servo/issues/17083/events","html_url":"https://github.com/servo/servo/pull/17083","id":232094240,"number":17083,"title":"\"javascript:\" urls: execute in correct global scope","user":{"login":"danielj41","id":5705347,"avatar_url":"https://avatars3.githubusercontent.com/u/5705347?v=4","gravatar_id":"","url":"https://api.github.com/users/danielj41","html_url":"https://github.com/danielj41","followers_url":"https://api.github.com/users/danielj41/followers","following_url":"https://api.github.com/users/danielj41/following{/other_user}","gists_url":"https://api.github.com/users/danielj41/gists{/gist_id}","starred_url":"https://api.github.com/users/danielj41/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/danielj41/subscriptions","organizations_url":"https://api.github.com/users/danielj41/orgs","repos_url":"https://api.github.com/users/danielj41/repos","events_url":"https://api.github.com/users/danielj41/events{/privacy}","received_events_url":"https://api.github.com/users/danielj41/received_events","type":"User","site_admin":false},"labels":[{"id":152310043,"url":"https://api.github.com/repos/servo/servo/labels/S-awaiting-review","name":"S-awaiting-review","color":"d4c5f9","default":false},{"id":152311003,"url":"https://api.github.com/repos/servo/servo/labels/S-needs-rebase","name":"S-needs-rebase","color":"d4c5f9","default":false}],"state":"open","locked":false,"assignee":{"login":"mbrubeck","id":5920,"avatar_url":"https://avatars1.githubusercontent.com/u/5920?v=4","gravatar_id":"","url":"https://api.github.com/users/mbrubeck","html_url":"https://github.com/mbrubeck","followers_url":"https://api.github.com/users/mbrubeck/followers","following_url":"https://api.github.com/users/mbrubeck/following{/other_user}","gists_url":"https://api.github.com/users/mbrubeck/gists{/gist_id}","starred_url":"https://api.github.com/users/mbrubeck/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mbrubeck/subscriptions","organizations_url":"https://api.github.com/users/mbrubeck/orgs","repos_url":"https://api.github.com/users/mbrubeck/repos","events_url":"https://api.github.com/users/mbrubeck/events{/privacy}","received_events_url":"https://api.github.com/users/mbrubeck/received_events","type":"User","site_admin":false},"assignees":[{"login":"mbrubeck","id":5920,"avatar_url":"https://avatars1.githubusercontent.com/u/5920?v=4","gravatar_id":"","url":"https://api.github.com/users/mbrubeck","html_url":"https://github.com/mbrubeck","followers_url":"https://api.github.com/users/mbrubeck/followers","following_url":"https://api.github.com/users/mbrubeck/following{/other_user}","gists_url":"https://api.github.com/users/mbrubeck/gists{/gist_id}","starred_url":"https://api.github.com/users/mbrubeck/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mbrubeck/subscriptions","organizations_url":"https://api.github.com/users/mbrubeck/orgs","repos_url":"https://api.github.com/users/mbrubeck/repos","events_url":"https://api.github.com/users/mbrubeck/events{/privacy}","received_events_url":"https://api.github.com/users/mbrubeck/received_events","type":"User","site_admin":false}],"milestone":null,"comments":8,"created_at":"2017-05-29T20:35:36Z","updated_at":"2017-08-10T15:16:03Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/servo/servo/pulls/17083","html_url":"https://github.com/servo/servo/pull/17083","diff_url":"https://github.com/servo/servo/pull/17083.diff","patch_url":"https://github.com/servo/servo/pull/17083.patch"},"body":"<!-- Please describe your changes on the following line: -->\r\n\r\n#### Summary\r\n\r\nThis pull request makes `javascript:` urls execute in the correct global scope.\r\n\r\n#### Example\r\n\r\n```html\r\n<script> var x = 4; </script>\r\n\r\n<!-- this branch: logs \"4\" -->\r\n<!-- master: undefined variable error -->\r\n<a href=\"javascript:console.log(x)\">link</a>\r\n```\r\n\r\n#### Questions\r\n\r\nI'm new to servo and rust, so I'm unsure about these changes. In particular:\r\n  * What's the appropriate place to evaluate the js?\r\n    * I moved it to `handle_navigate`, but I'm not sure if this will catch all occurrences of `javascript:` urls. I also don't know if this will execute in the correct thread and the correct window.\r\n  * What should I do with the result of the js evaluation?\r\n    * I just ignored it. The previous behavior displayed it as the content of a new page load.\r\n\r\n---\r\n<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->\r\n- [x] `./mach build -d` does not report any errors\r\n- [x] `./mach test-tidy` does not report any errors\r\n- [x] These changes fix #15147, #16718\r\n\r\n<!-- Either: -->\r\n- [x] There are tests for these changes\r\n\r\n<!-- Also, please make sure that \"Allow edits from maintainers\" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->\r\n\r\n<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->\r\n\r\n<!-- Reviewable:start -->\r\n---\r\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/servo/servo/17083)\r\n<!-- Reviewable:end -->\r\n"},"comment":{"url":"https://api.github.com/repos/servo/servo/issues/comments/321582362","html_url":"https://github.com/servo/servo/pull/17083#issuecomment-321582362","issue_url":"https://api.github.com/repos/servo/servo/issues/17083","id":321582362,"user":{"login":"jdm","id":27658,"avatar_url":"https://avatars1.githubusercontent.com/u/27658?v=4","gravatar_id":"","url":"https://api.github.com/users/jdm","html_url":"https://github.com/jdm","followers_url":"https://api.github.com/users/jdm/followers","following_url":"https://api.github.com/users/jdm/following{/other_user}","gists_url":"https://api.github.com/users/jdm/gists{/gist_id}","starred_url":"https://api.github.com/users/jdm/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jdm/subscriptions","organizations_url":"https://api.github.com/users/jdm/orgs","repos_url":"https://api.github.com/users/jdm/repos","events_url":"https://api.github.com/users/jdm/events{/privacy}","received_events_url":"https://api.github.com/users/jdm/received_events","type":"User","site_admin":false},"created_at":"2017-08-10T15:16:03Z","updated_at":"2017-08-10T15:16:03Z","body":"Sorry for the extreme delay here! I kept putting off responding because I thought it would take a lot of time to wrap my head around the various complexities here, but your last comment did a really good job of laying out the issues you're facing.\r\n\r\nI agree that neither of your attempted solutions quite fit the existing model. Here is what I recommend instead:\r\n* continue performing the JS evaluation in ScriptThread::handle_navigation (for the time being that's fine)\r\n* store the resulting body in a new field in the LoadData structure\r\n* in ScriptThread::handle_new_layout, obtain the body from the LoadData embedded in the NewLayoutInfo struct\r\n* modify ScriptThread::start_page_load_about_blank to support passing in the desired response body from the caller\r\n* add an extra check for javascript urls to ScriptThread::handle_new_layout that passes the body to the previously modified method\r\n\r\nDoes that make sense?"}}
{"actor":{"display_login":"jdm"}}