[WIP] Implement History State #13781

cbrewster · 2016-10-15T22:28:27Z

This is a work in progress, I still need to figure out how to handle title and url in push/replace state as well as throwing the proper errors according to the spec. Also when the URL changes there needs to be a way to change the actual document url.

@asajeffrey this is my first draft idea of how to handle history states, let me know what you think.

r? @asajeffrey

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-10-15T22:28:30Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/constellation.rs
@fitzgen: components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/browsingcontext.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/dom/history.rs, components/script/script_thread.rs, components/script/dom/webidls/History.webidl, components/script/dom/bindings/trace.rs
@KiChjang: components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/browsingcontext.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/dom/history.rs, components/script/script_thread.rs, components/script/dom/webidls/History.webidl, components/script/dom/bindings/trace.rs

emilio · 2016-10-15T23:18:24Z

components/script/dom/history.rs

@@ -80,4 +149,22 @@ impl HistoryMethods for History {
    fn Forward(&self) {
        self.traverse_history(TraversalDirection::Forward(1));
    }
+
+    // https://html.spec.whatwg.org/multipage/#dom-history-pushtstae


This link seems to go nowhere? (also, it looks like it was manually written, given pushtsae instead of pushstate.

Woops, yeah a typo it should be https://html.spec.whatwg.org/multipage/#dom-history-pushstate

bors-servo · 2016-10-18T01:18:34Z

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

asajeffrey · 2016-10-21T15:00:56Z

Review status: 0 of 32 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

components/constellation/constellation.rs, line 221 at r14 (raw file):

    pipeline_id: PipelineId,
    frame_id: FrameId,
    history_state_id: HistoryStateId,

Make this optional?

components/constellation/constellation.rs, line 221 at r14 (raw file):

    pipeline_id: PipelineId,
    frame_id: FrameId,
    history_state_id: HistoryStateId,

Rather than storing multiple FrameState entries for the same pipeline, we could do something like pushed_states: Vec<PushedState> where PushedState is a struct with a HistoryStateId, Url, etc.

components/constellation/constellation.rs, line 248 at r14 (raw file):

            id: id,
            prev: vec!(),
            // TODO(ConnorGBrewster): Should HistoryStateId(0) be hardcoded like this?

If we made the history state id optional, we'd get rid of the hard-coded state id.

components/constellation/constellation.rs, line 269 at r14 (raw file):

    fn push_state(&mut self, pipeline_id: PipelineId, history_state_id: HistoryStateId) {
        self.prev.push(self.current.clone());

We should be able to avoid cloning here, using mem::swap.

components/constellation/constellation.rs, line 342 at r14 (raw file):

                },
            };
            // The same pipeline can exist in multiple entries across a Frame, so we need to

If we used a vector of pushed states, then each pipeline would only have one entry in the frame tree.

components/constellation/constellation.rs, line 864 at r14 (raw file):

            }
            // Handle a history state pushed request.
            FromScriptMsg::HistoryStatePushed(pipeline_id, history_state_id) => {

At some point, this would need the url and title?

components/constellation/constellation.rs, line 1557 at r14 (raw file):

        };
        if frame.current.pipeline_id != pipeline_id {
            // Pipeline is not fully active. Abort.

Add a warning?

components/constellation/constellation.rs, line 2255 at r14 (raw file):

            }
            // Notify the BrowsingContext in the script thread to remove the StateEntries that are
            // now no longer accesible.

Oh good, distributed gc. At least I think we don't have to deal with cycles, as we can reclaim Frame objects when their iframe becomes detached from the DOM.

Comments from Reviewable

asajeffrey · 2016-10-21T15:04:42Z

Reviewed 1 of 3 files at r8, 1 of 6 files at r10.
Review status: 2 of 32 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

components/script/dom/browsingcontext.rs, line 78 at r14 (raw file):

            children: DOMRefCell::new(vec![]),
            active_document: Default::default(),
            active_state: Cell::new(HistoryStateId(0)),

I think all we need to store is the hash map from HistoryStateId to JS objects, the stack of history objects can be tracked just by the constellation. Also, we might want to keep this info in the global rather than the browsing context?

Comments from Reviewable

asajeffrey · 2016-10-21T15:10:55Z

Reviewed 1 of 3 files at r1, 1 of 3 files at r8, 1 of 6 files at r10, 1 of 4 files at r12, 1 of 1 files at r13, 1 of 1 files at r14.
Review status: 8 of 32 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

components/script/dom/browsingcontext.rs, line 78 at r14 (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

I think all we need to store is the hash map from HistoryStateId to JS objects, the stack of history objects can be tracked just by the constellation. Also, we might want to keep this info in the global rather than the browsing context?

Silly me, you need the active state object, I still don't think you need the next state id though.

Comments from Reviewable

asajeffrey · 2016-10-21T15:13:35Z

Reviewed 21 of 23 files at r11, 1 of 4 files at r12.
Review status: 30 of 32 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

tests/wpt/metadata/html/browsers/history/the-history-interface/005.html.ini, line 3 at r14 (raw file):

[005.html]
  type: testharness
  expected: TIMEOUT

TIMEOUT?

tests/wpt/metadata/html/browsers/history/the-history-interface/007.html.ini, line 10 at r14 (raw file):

  [history.state should reflect pushed state]
    expected: FAIL

Yay, passing tests!

tests/wpt/metadata/html/browsers/history/the-history-interface/combination_history_004.html.ini, line 3 at r14 (raw file):

[combination_history_004.html]
  type: testharness
  expected: TIMEOUT

TIMEOUT?

Comments from Reviewable

asajeffrey · 2016-10-21T15:13:55Z

Reviewed 2 of 23 files at r11.
Review status: all files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

tests/wpt/metadata/html/browsers/history/the-history-interface/history_back.html.ini, line 3 at r14 (raw file):

[history_back.html]
  type: testharness
  expected: TIMEOUT

TIMEOUT?

Comments from Reviewable

asajeffrey · 2016-10-21T15:16:24Z

This is looking pretty good! The main question is whether to store a vector of pushed states or just a single pushed state in the constellation. Also, I'm not sure how we're handling the url and title?

Review status: all files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

Comments from Reviewable

cbrewster · 2016-10-25T17:05:08Z

Review status: all files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

components/constellation/constellation.rs, line 221 at r14 (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Rather than storing multiple FrameState entries for the same pipeline, we could do something like pushed_states: Vec<PushedState> where PushedState is a struct with a HistoryStateId, Url, etc.

I would like to avoid this, since each pushed state has its own entry that is sorted and interleaved when calculating the joint session history. This would add complexity to the jsh calculation. Also I don't think an optional is the right way either since there is still an active history state for brand new browsing contexts. `None` would suggest that there isn't.

components/constellation/constellation.rs, line 342 at r14 (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

If we used a vector of pushed states, then each pipeline would only have one entry in the frame tree.

I think this will be unavoidable, this logic is also needed for when we add entries for hash changes

components/constellation/constellation.rs, line 864 at r14 (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

At some point, this would need the url and title?

Yes, should we store the url and title on the script thread or have the constellation manage this?

components/script/dom/browsingcontext.rs, line 78 at r14 (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Silly me, you need the active state object, I still don't think you need the next state id though.

How would the next state id be avoided? I think it would be silly to have the constellation to manage it since that will add extra messages

tests/wpt/metadata/html/browsers/history/the-history-interface/005.html.ini, line 3 at r14 (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

TIMEOUT?

Woops! I still need to fire the `popstate` event

Comments from Reviewable

asajeffrey · 2016-10-26T19:04:44Z

Review status: all files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

components/constellation/constellation.rs, line 221 at r14 (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

I would like to avoid this, since each pushed state has its own entry that is sorted and interleaved when calculating the joint session history. This would add complexity to the jsh calculation. Also I don't think an optional is the right way either since there is still an active history state for brand new browsing contexts. None would suggest that there isn't.

Hmm, what's the state object for browsing contexts which don't use pushState? null? Might be easiest to use None for that, since it would allow the constellation to record which browsing contexts have an associated state.

components/constellation/constellation.rs, line 342 at r14 (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

I think this will be unavoidable, this logic is also needed for when we add entries for hash changes

oh, good point. We could record hash changes in the same way as pushState, but this might be getting over-complicated.

components/constellation/constellation.rs, line 864 at r14 (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

Yes, should we store the url and title on the script thread or have the constellation manage this?

I'd guess we want it in the constellation.

components/script/dom/browsingcontext.rs, line 78 at r14 (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

How would the next state id be avoided? I think it would be silly to have the constellation to manage it since that will add extra messages

Similarly to pipeline and frame ids?

Comments from Reviewable

cbrewster · 2016-11-03T05:53:14Z

Added popstate event, I still need to investigate the failures and see if its an issue with my implementation.

bors-servo · 2016-11-04T00:11:40Z

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

cbrewster · 2016-11-04T02:48:12Z

Rebased. I enable 001 and 002 which were disabled due to #12580 and added them to the intermittent checker. I fixed which state value was turned and the test results look much better now.

Review status: 1 of 40 files reviewed at latest revision, 13 unresolved discussions, some commit checks pending.

components/constellation/constellation.rs, line 221 at r14 (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Hmm, what's the state object for browsing contexts which don't use pushState? null? Might be easiest to use None for that, since it would allow the constellation to record which browsing contexts have an associated state.

Its easiest to just keep a default single active entry with the state set to `null`. Otherwise, if `replaceState` gets called, we would need an additional message to the Constellation to notify it that we have a new state entry but we just want to adjust the active state id without pushing a new entry and we would only do this in the case where we are replacing the first state entry (no previous states have been pushed). I think it is just simpler to always start with an active entry with state as `null`.

components/constellation/constellation.rs, line 248 at r14 (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

If we made the history state id optional, we'd get rid of the hard-coded state id.

See above. I don't think this is a huge issue, maybe we should implement Default as to make this look a bit cleaner.

components/constellation/constellation.rs, line 269 at r14 (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

We should be able to avoid cloning here, using mem::swap.

Done.

components/constellation/constellation.rs, line 1557 at r14 (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Add a warning?

Done.

components/script/dom/browsingcontext.rs, line 78 at r14 (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Similarly to pipeline and frame ids?

The history state id just needs to be unique to the specific browsing context is it worth having it be unique inside its thread? Would this be weird in combination with the initial state entry having an id of 0?

Comments from Reviewable

cbrewster · 2016-11-04T02:49:53Z

@bors-servo try

bors-servo · 2016-11-04T02:49:56Z

⌛ Trying commit 7a5f21d with merge 78a68cd...


        Auto merge of #13781 - ConnorGBrewster:history_state, r=<try>

@asajeffrey

[WIP] Implement History State  This is a work in progress, I still need to figure out how to handle `title` and `url` in push/replace state as well as throwing the proper errors according to the spec. Also when the URL changes there needs to be a way to change the actual document url. @asajeffrey this is my first draft idea of how to handle history states, let me know what you think. r? @asajeffrey ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13781)

bors-servo · 2016-11-04T03:04:42Z

💔 Test failed - windows-dev

bors-servo · 2016-12-07T03:28:26Z

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

bors-servo · 2016-12-24T14:44:23Z

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


        add methods to ensure frame state stays consistent


        Implement history state, pushState, replaceState

Add HistoryStateId Added state id to browsing context add history state removal Updated test expectations fix tidy issues and remove document parameter fix spec link Add popstate event address review comments enable 001 and 002 history interface tests Add 001 and 002 to the intermittent check rebased. fix tidy issues Update test expectations rebase Store and set Url, compare FrameState during traversal

asajeffrey · 2017-01-20T18:01:31Z

components/constellation/constellation.rs

@@ -1713,6 +1718,21 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
        let _ = sender.send(length as u32);
    }

+    fn handle_history_state_pushed(&mut self, pipeline_id: PipelineId, state_id: HistoryStateId) {
+        if !self.pipeline_is_in_current_frame(pipeline_id) {


Might be worth doing this check after looking up the frame, to avoid looking up the frame twice.

asajeffrey · 2017-01-20T18:12:41Z

components/constellation/frame.rs

@@ -29,6 +29,9 @@ pub struct Frame {
    /// The pipeline for the current session history entry
    pub pipeline_id: PipelineId,

+    /// The id that maps to the history state entry in the script thread
+    pub state_id: HistoryStateId,


Is the state_id optional?

I suppose we could make it optional and have the BC return null for history.state if the active state_id is None

asajeffrey · 2017-01-20T18:19:02Z

components/constellation/frame.rs

    }

+    /// Add an entry representing a new history state.
+    pub fn push_state(&mut self, state_id: HistoryStateId) {


Shouldn't we also be pushing a URL?

asajeffrey · 2017-01-20T18:19:29Z

components/constellation/frame.rs

@@ -97,6 +119,19 @@ pub struct FrameState {

    /// The frame that this session history entry is part of
    pub frame_id: FrameId,
+
+    /// The id that maps to the history state entry in the script thread
+    pub state_id: HistoryStateId,


asajeffrey · 2017-01-20T18:19:59Z

components/constellation/frame.rs

+    pub fn replace_pipeline(&mut self, pipeline_id: PipelineId, url: ServoUrl) {
+        self.pipeline_id = Some(pipeline_id);
+        self.url = url;
+        self.state_id = HistoryStateId(0);


asajeffrey · 2017-01-20T18:23:39Z

components/script/dom/browsingcontext.rs

@@ -33,24 +44,38 @@ use std::cell::Cell;
 pub struct BrowsingContext {


This data shouldn't be stored in the browsing context.

asajeffrey · 2017-01-20T18:25:28Z

components/script/dom/browsingcontext.rs

    /// Has this browsing context been discarded?
    discarded: Cell<bool>,

+    active_state: Cell<HistoryStateId>,
+
+    next_state_id: Cell<HistoryStateId>,


asajeffrey · 2017-01-20T18:30:05Z

components/script/dom/browsingcontext.rs

+                                         state: HandleValue) {
+        let mut states = self.states.borrow_mut();
+        states.insert(self.active_state.get(), HistoryState::new(Some(state), Some(title), url));
+        // NOTE: We do not need to notify the constellation, as the history state id


Not sure about that, what if the document is discarded then reloaded? The constellation needs to know the new URL.

asajeffrey · 2017-01-20T18:30:54Z

components/script/dom/browsingcontext.rs

+        self.states.borrow_mut().insert(next_id, HistoryState::new(Some(state), Some(title), url));
+        self.active_state.set(next_id);
+
+        // Notify the constellation about this new entry so it can be added to the


Do we need to fire a mozbrowser event?

asajeffrey · 2017-01-20T18:37:39Z

components/script/dom/history.rs

+                };
+
+                // 6.4
+                if url.scheme() != document_url.scheme() ||


Can use the url ranges to select everything up to the port?

asajeffrey · 2017-01-20T18:39:00Z

tests/wpt/metadata/html/browsers/history/the-history-interface/001.html.ini

  [history.length should update when setting location.hash]
    expected: FAIL

-  [history.pushState must exist]


asajeffrey · 2017-01-20T18:40:28Z

tests/wpt/metadata/html/browsers/history/the-history-interface/002.html.ini

  [history.state should also reference a clone of the original object (2)]
    expected: FAIL

  [history.state should be a separate clone of the object, not a reference to the object passed to the event handler]
    expected: FAIL

+  [replaceState should not actually load the new URL]
+    expected: FAIL


Can we get this to pass?

bors-servo · 2017-01-21T15:15:58Z

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

cbrewster · 2017-01-27T05:32:47Z

Superseded by #15261
It is much cleaner to move all the state logic to History instead of having the browsing context handle it.

highfive assigned asajeffrey Oct 15, 2016

highfive added the S-awaiting-review label Oct 15, 2016

cbrewster mentioned this pull request Oct 15, 2016

History API metabug #10992

Open

18 of 24 tasks complete

emilio reviewed Oct 15, 2016

View changes

cbrewster force-pushed the cbrewster:history_state branch from 16ad29a to 5cded23 Oct 16, 2016

highfive added the S-needs-rebase label Oct 18, 2016

cbrewster force-pushed the cbrewster:history_state branch from 5cded23 to 11ff2cc Nov 3, 2016

cbrewster removed the S-needs-rebase label Nov 3, 2016

highfive added the S-needs-rebase label Nov 4, 2016

cbrewster force-pushed the cbrewster:history_state branch 2 times, most recently from f0e7d97 to 6d7e5f2 Nov 4, 2016

cbrewster removed the S-needs-rebase label Nov 4, 2016

highfive added the S-tests-failed label Nov 4, 2016

highfive added the S-needs-rebase label Dec 7, 2016

cbrewster force-pushed the cbrewster:history_state branch from 3b2aa9b to 7f730c9 Dec 19, 2016

highfive removed the S-tests-failed label Dec 19, 2016

highfive added the S-needs-rebase label Dec 24, 2016

cbrewster mentioned this pull request Jan 12, 2017

hashchange DOM event should be dispatched when the document location's hash is changed #14970

Closed

cbrewster added 2 commits Jan 18, 2017

add methods to ensure frame state stays consistent

Loading status checks…

eedff27

cbrewster force-pushed the cbrewster:history_state branch from 7f730c9 to 92bf862 Jan 18, 2017

cbrewster mentioned this pull request Jan 18, 2017

Document fully_activate may not be correct #15093

Closed

asajeffrey reviewed Jan 20, 2017

View changes

components/constellation/frame.rs

}

/// Add an entry representing a new history state.

pub fn push_state(&mut self, state_id: HistoryStateId) {

This comment has been minimized.

Sign in to view

Copy link

asajeffrey Jan 20, 2017
Member

Shouldn't we also be pushing a URL?

asajeffrey reviewed Jan 20, 2017

View changes

components/script/dom/browsingcontext.rs

@@ -33,24 +44,38 @@ use std::cell::Cell;

pub struct BrowsingContext {

This comment has been minimized.

Sign in to view

Copy link

asajeffrey Jan 20, 2017
Member

This data shouldn't be stored in the browsing context.

asajeffrey reviewed Jan 20, 2017

View changes

components/script/dom/browsingcontext.rs

/// Has this browsing context been discarded?

discarded: Cell<bool>,

active_state: Cell<HistoryStateId>,

next_state_id: Cell<HistoryStateId>,

This comment has been minimized.

Sign in to view

Copy link

asajeffrey Jan 20, 2017
Member

Use uuids?

asajeffrey reviewed Jan 20, 2017

View changes

components/script/dom/history.rs

};

// 6.4

if url.scheme() != document_url.scheme() ||

This comment has been minimized.

Sign in to view

Copy link

asajeffrey Jan 20, 2017
Member

Can use the url ranges to select everything up to the port?

asajeffrey reviewed Jan 20, 2017

View changes

tests/wpt/metadata/html/browsers/history/the-history-interface/001.html.ini

[history.length should update when setting location.hash]

expected: FAIL

[history.pushState must exist]

This comment has been minimized.

Sign in to view

Copy link

asajeffrey Jan 20, 2017
Member

Yay!

asajeffrey reviewed Jan 20, 2017

View changes

asajeffrey added S-awaiting-answer and removed S-awaiting-review S-needs-rebase labels Jan 20, 2017

highfive added the S-needs-rebase label Jan 21, 2017

cbrewster closed this Jan 27, 2017

servo / servo

[WIP] Implement History State #13781

[WIP] Implement History State #13781

cbrewster commented Oct 15, 2016 •

edited by larsbergstrom

highfive commented Oct 15, 2016

This comment has been minimized.

This comment has been minimized.

bors-servo commented Oct 18, 2016

asajeffrey commented Oct 21, 2016

asajeffrey commented Oct 21, 2016

asajeffrey commented Oct 21, 2016

asajeffrey commented Oct 21, 2016

asajeffrey commented Oct 21, 2016

asajeffrey commented Oct 21, 2016

cbrewster commented Oct 25, 2016

asajeffrey commented Oct 26, 2016

cbrewster commented Nov 3, 2016

bors-servo commented Nov 4, 2016

cbrewster commented Nov 4, 2016

cbrewster commented Nov 4, 2016

bors-servo commented Nov 4, 2016

bors-servo commented Nov 4, 2016

bors-servo commented Dec 7, 2016

bors-servo commented Dec 24, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Jan 21, 2017

cbrewster commented Jan 27, 2017

		@@ -33,24 +44,38 @@ use std::cell::Cell;
		pub struct BrowsingContext {

servo / servo

Join GitHub today

[WIP] Implement History State #13781

[WIP] Implement History State #13781

Conversation

cbrewster commented Oct 15, 2016 • edited by larsbergstrom

highfive commented Oct 15, 2016

This comment has been minimized.

This comment has been minimized.

bors-servo commented Oct 18, 2016

asajeffrey commented Oct 21, 2016

asajeffrey commented Oct 21, 2016

asajeffrey commented Oct 21, 2016

asajeffrey commented Oct 21, 2016

asajeffrey commented Oct 21, 2016

asajeffrey commented Oct 21, 2016

cbrewster commented Oct 25, 2016

asajeffrey commented Oct 26, 2016

cbrewster commented Nov 3, 2016

bors-servo commented Nov 4, 2016

cbrewster commented Nov 4, 2016

cbrewster commented Nov 4, 2016

bors-servo commented Nov 4, 2016

bors-servo commented Nov 4, 2016

bors-servo commented Dec 7, 2016

bors-servo commented Dec 24, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Jan 21, 2017

cbrewster commented Jan 27, 2017

cbrewster commented Oct 15, 2016 •

edited by larsbergstrom