mozbrowsersecuritychange event #9244

paulrouget · 2016-01-11T05:02:34Z

No test yet. Is there a way to mock a https connection?

Also, I wish I could use the HTTPSState enum instead of a String when calling trigger_mozbrowser_event (https://github.com/servo/servo/compare/master...paulrouget:securitychange?expand=1#diff-30a18e04d7e0b66aafdf192e416cad44R306) but that would require constellation_msg.rs to know about HTTPSState, which is defined in document.rs, which would add a dependency to components/msg. I could define HTTPSState somewhere else maybe? Or maybe it's fine to use a String. But then, should I use the HTTPSState strings ("modern/deprecated/none") or the mozbrowser strings ("secure/insecure/broken") (as it is now)

frewsxcv · 2016-01-11T05:11:47Z

components/script/dom/document.rs, line 126 [r1] (raw file):
This enum already exists in net_traits/response.rs.

Relevant PRs: #9153, #9153

Comments from the review on Reviewable.io

frewsxcv · 2016-01-11T05:18:22Z

components/script/dom/document.rs, line 126 [r1] (raw file):
Second PR link was supposed to be #9165

Comments from the review on Reviewable.io

paulrouget · 2016-01-11T12:43:18Z

Using the existing HttpsState enum.

3 questions:

how do I test this feature? Anyway to mock a https connection?
as explained in the first comment, is it possible to send a HttpsState object instead of a string when calling trigger_mozbrowser_event? I haven't found a way to add HttpsState to constellation_msg.rs.
how to catch broken/deprecated https connections? Maybe this could be done in a followup PR.

jdm · 2016-01-11T16:21:29Z

We don't support SSL in our WPT harness yet due to #6919 . It would be nice to write a test for this, though, even if it's expected to fail in Servo until #6919 is fixed.
This will probably be easier after #9225.
Definitely followup.

bors-servo · 2016-01-13T19:40:25Z

☔ The latest upstream changes (presumably #9271) made this pull request unmergeable. Please resolve the merge conflicts.

paulrouget · 2016-01-22T10:11:20Z

Added a test. It fails, as expected (#6919). If I use my own http/https servers, it passes.

Now I can use HttpState to send a message. That looks a lot cleaner.

paulrouget · 2016-01-28T02:57:15Z

@frewsxcv / @jdm: review ping

jdm · 2016-02-03T16:33:16Z

tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsersecuritychange_event.html

+  iframe.mozbrowser = "true";
+  iframe.src = urls[url_index++];
+
+  iframe.addEventListener("mozbrowsersecuritychange", t.step_func(e => {


Let's make a separate async_test for each iframe so we get better granularity for test failures.

I'm not sure to understand how that would help. There's only one iframe, and what matters is having the events to chain properly.

Whoops, I misread the test. You're right.

jdm · 2016-02-03T16:35:02Z

The code changes looks fine, but we can improve the test a bit more.

jdm · 2016-02-04T13:34:31Z

tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsersecuritychange_event.html

@@ -0,0 +1,49 @@
+<head>


Let's give this a <title> and update the ini to use that instead of Untitled.

jdm · 2016-02-04T13:34:45Z

tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsersecuritychange_event.html

+    receivedEvents.push({ state: e.detail.state });
+
+    if (receivedEvents.length == expectedEvents.length) {
+      for (var i = 0; i < expectedEvents.length; i++) {


assert_array_equals

jdm · 2016-02-04T13:35:29Z

I will therefore reinstate two of my previous comments that I deleted after making the request to split the test.

paulrouget · 2016-02-05T07:14:42Z

I will therefore reinstate two of my previous comments

Addressed.

jdm · 2016-02-08T19:57:43Z

tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsersecuritychange_event.html

@@ -0,0 +1,46 @@
+<head>
+  <title>mozbrowsersecuritychange</title>


How about "mozbrowsersecuritychange event is dispatched when content security state changes"

jdm · 2016-02-08T19:59:35Z

tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsersecuritychange_event.html

+      var expectedEvents = [
+        "insecure",
+        "secure",
+        "insecure",


Let's load each page twice in a row to ensure that we don't get events when the state doesn't change. On the other hand, if that's not something we care about, the text at https://developer.mozilla.org/en-US/docs/Web/Events/mozbrowsersecuritychange should be updated to be more precise.

paulrouget · 2016-02-09T06:53:05Z

Let's load each page twice in a row to ensure that we don't get events when the state doesn't change. On the other hand, if that's not something we care about, the text at https://developer.mozilla.org/en-US/docs/Web/Events/mozbrowsersecuritychange should be updated to be more precise.

Gecko fires this event even if the state doesn't actually change. I'll update MDN.


        mozbrowsersercuritychange event

jdm · 2016-02-09T07:30:28Z

@bors-servo: r+
Thanks!

bors-servo · 2016-02-09T07:30:28Z

📌 Commit 63519c3 has been approved by jdm

bors-servo · 2016-02-09T07:30:30Z

⌛ Testing commit 63519c3 with merge 3d63f09...


        Auto merge of #9244 - paulrouget:securitychange, r=jdm

mozbrowsersecuritychange event Fixes #8544 No test yet. Is there a way to mock a https connection? Also, I wish I could use the `HTTPSState` enum instead of a `String` when calling `trigger_mozbrowser_event` (https://github.com/servo/servo/compare/master...paulrouget:securitychange?expand=1#diff-30a18e04d7e0b66aafdf192e416cad44R306) but that would require `constellation_msg.rs` to know about `HTTPSState`, which is defined in `document.rs`, which would add a dependency to `components/msg`. I could define `HTTPSState` somewhere else maybe? Or maybe it's fine to use a `String`. But then, should I use the HTTPSState strings (`"modern/deprecated/none"`) or the mozbrowser strings (`"secure/insecure/broken"`) (as it is now)  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9244)

bors-servo · 2016-02-09T08:33:57Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review label Jan 11, 2016

paulrouget force-pushed the paulrouget:securitychange branch from 5207986 to 3745913 Jan 11, 2016

paulrouget force-pushed the paulrouget:securitychange branch from 3745913 to 4aaf250 Jan 11, 2016

highfive added the S-needs-rebase label Jan 13, 2016

paulrouget force-pushed the paulrouget:securitychange branch from 4aaf250 to c90c4a4 Jan 22, 2016

jdm removed the S-needs-rebase label Jan 22, 2016

paulrouget mentioned this pull request Jan 27, 2016

requestAnimationFrame is called ~1500 times per second #7782

Closed

jdm self-assigned this Jan 28, 2016

jdm reviewed Feb 3, 2016
View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Feb 3, 2016

jdm reviewed Feb 4, 2016
View changes

tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsersecuritychange_event.html

@@ -0,0 +1,49 @@

<head>

This comment has been minimized.

Sign in to view

Copy link

jdm Feb 4, 2016
Member

Let's give this a <title> and update the ini to use that instead of Untitled.

jdm reviewed Feb 4, 2016
View changes

paulrouget force-pushed the paulrouget:securitychange branch from c90c4a4 to b1170a1 Feb 5, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 5, 2016

jdm reviewed Feb 8, 2016
View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Feb 8, 2016

paulrouget force-pushed the paulrouget:securitychange branch from b1170a1 to 18b5d78 Feb 9, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 9, 2016

mozbrowsersercuritychange event

Loading status checks…

63519c3

paulrouget force-pushed the paulrouget:securitychange branch from 18b5d78 to 63519c3 Feb 9, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 9, 2016

bors-servo merged commit 63519c3 into servo:master Feb 9, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

servo / servo

mozbrowsersecuritychange event #9244

mozbrowsersecuritychange event #9244

paulrouget commented Jan 11, 2016

frewsxcv commented Jan 11, 2016

frewsxcv commented Jan 11, 2016

paulrouget commented Jan 11, 2016

jdm commented Jan 11, 2016

bors-servo commented Jan 13, 2016

paulrouget commented Jan 22, 2016

paulrouget commented Jan 28, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Feb 3, 2016

This comment has been minimized.

This comment has been minimized.

jdm commented Feb 4, 2016

paulrouget commented Feb 5, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

paulrouget commented Feb 9, 2016

jdm commented Feb 9, 2016

bors-servo commented Feb 9, 2016

bors-servo commented Feb 9, 2016

bors-servo commented Feb 9, 2016

		@@ -0,0 +1,46 @@
		<head>
		<title>mozbrowsersecuritychange</title>

servo / servo

Join GitHub today

mozbrowsersecuritychange event #9244

mozbrowsersecuritychange event #9244

Conversation

paulrouget commented Jan 11, 2016

frewsxcv commented Jan 11, 2016

frewsxcv commented Jan 11, 2016

paulrouget commented Jan 11, 2016

jdm commented Jan 11, 2016

bors-servo commented Jan 13, 2016

paulrouget commented Jan 22, 2016

paulrouget commented Jan 28, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Feb 3, 2016

This comment has been minimized.

This comment has been minimized.

jdm commented Feb 4, 2016

paulrouget commented Feb 5, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

paulrouget commented Feb 9, 2016

jdm commented Feb 9, 2016

bors-servo commented Feb 9, 2016

bors-servo commented Feb 9, 2016

bors-servo commented Feb 9, 2016