Storage notifications routed via the constellation. #14023

asajeffrey · 2016-11-02T14:18:50Z

At the moment, storage notifications are only sent within a script thread, not to same-origin pipelines in different script threads. This PR fixes that.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #5196
These changes do not require tests because existing tests now pass

This change is

highfive · 2016-11-02T14:18:54Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/storage.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/script_thread.rs, components/script_traits/lib.rs, components/script_traits/lib.rs
@KiChjang: components/script/dom/storage.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/script_thread.rs, components/script_traits/lib.rs, components/script_traits/lib.rs

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

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

nox · 2016-11-07T10:06:52Z

components/constellation/constellation.rs

+                let msg = ConstellationControlMsg::DispatchStorageEvent(
+                    pipeline.id, storage, url.clone(), key.clone(), old_value.clone(), new_value.clone()
+                );
+                let _ = pipeline.script_chan.send(msg);


Why ignore failure here?

nox · 2016-11-07T10:06:52Z

components/constellation/constellation.rs

+        };
+
+        for pipeline in self.pipelines.values() {
+            if (pipeline.id != pipeline_id) && (pipeline.url.origin() == origin) {


Nit: remove parens.

Hmm, I prefer explicit parens rather than relying on operator pecedence to resolve ambiguity.

nox · 2016-11-07T10:06:52Z

components/constellation/constellation.rs

+        let origin = match Url::parse(&*url) {
+            Ok(url) => url.origin(),
+            Err(err) => return warn!("Failed to parse url {}: {:?}.", url, err),
+        };


Why ignore failure here? Why isn't that method taking an Url directly? I see below that the String comes directly from an Url, so please just unwrap the result of Url::parse or pass an actual Url around.

Oh I see you did that in second commit. Could you squash them?

I'll squash them before merging.

nox · 2016-11-07T10:06:52Z

components/script/dom/storage.rs

-            DOMString::from(ev_url.to_string()),
-            Some(&storage)
+            DOMString::from(this.url),
+            Some(&*storage)


Why the sudden *?

Apparently no good reason, not sure how this snuck in. Fixed.

nox · 2016-11-07T10:06:52Z

components/script_traits/script_msg.rs

@@ -59,6 +60,9 @@ pub enum LogEntry {
 /// Messages from the script to the constellation.
 #[derive(Deserialize, Serialize)]
 pub enum ScriptMsg {
+    /// Broadcast a storage event to every same-origin pipeline.
+    /// The strings are url, key, old value and new value.
+    BroadcastStorageEvent(PipelineId, StorageType, String, Option<String>, Option<String>, Option<String>),


Maybe use a struct variant instead of documenting each field in prose?

I agree with you, but we should be consistent about it, not just adopt struct variants piecemeal.

nox · 2016-11-07T10:07:46Z

GitHub somehow decided that some of my comments are outdated when they are not, please expand them when you read this @asajeffrey.

asajeffrey · 2016-11-08T17:10:44Z

We should probably hold off on merging this until #14013 has landed, c.f. https://github.com/servo/servo/pull/14013/files#diff-ab86862fdf6e275f1f412558ce78bc2aR195.

bors-servo · 2016-11-08T18:25:42Z

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

nox · 2016-11-15T15:54:32Z

Squash and r=me.


        Storage notifications routed via the constellation.

asajeffrey · 2016-11-15T15:58:01Z

Squashed. @bors-servo r=nox

bors-servo · 2016-11-15T15:58:03Z

📌 Commit c91ef86 has been approved by nox

bors-servo · 2016-11-15T16:50:21Z

⌛ Testing commit c91ef86 with merge 07b9dbe...


        Auto merge of #14023 - asajeffrey:storage-notify-via-constellation, r…

…=nox Storage notifications routed via the constellation.  At the moment, storage notifications are only sent within a script thread, not to same-origin pipelines in different script threads. This PR fixes that. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #5196 - [X] These changes do not require tests because existing tests now pass   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14023)

bors-servo · 2016-11-15T18:47:18Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

asajeffrey added A-content/script A-constellation labels Nov 2, 2016

highfive assigned Ms2ger Nov 2, 2016

highfive added the S-awaiting-review label Nov 2, 2016

asajeffrey added the S-needs-squash label Nov 3, 2016

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

asajeffrey force-pushed the asajeffrey:storage-notify-via-constellation branch from a8b66da to 75867ef Nov 4, 2016

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

nox requested changes Nov 7, 2016

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 7, 2016

asajeffrey added the S-awaiting-review label Nov 8, 2016

highfive removed the S-needs-code-changes label Nov 8, 2016

asajeffrey force-pushed the asajeffrey:storage-notify-via-constellation branch from 0ee6c6d to 2711a0e Nov 8, 2016

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

asajeffrey force-pushed the asajeffrey:storage-notify-via-constellation branch from 2711a0e to 4c09ef4 Nov 8, 2016

jdm removed the S-needs-rebase label Nov 15, 2016

asajeffrey force-pushed the asajeffrey:storage-notify-via-constellation branch from 4c09ef4 to d8a9112 Nov 15, 2016

Storage notifications routed via the constellation.

Loading status checks…

c91ef86

asajeffrey force-pushed the asajeffrey:storage-notify-via-constellation branch from d8a9112 to c91ef86 Nov 15, 2016

highfive unassigned Ms2ger Nov 15, 2016

highfive assigned nox Nov 15, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Nov 15, 2016

bors-servo merged commit c91ef86 into servo:master Nov 15, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 15, 2016

This was referenced Nov 15, 2016

Share script threads by tab and by eTLD+1 #14211

Merged

Implement synchronous about:blank loading. #13996

Merged

Make document url mutable and implement location.replace() #13418

Merged

servo / servo

Storage notifications routed via the constellation. #14023

Storage notifications routed via the constellation. #14023

asajeffrey commented Nov 2, 2016 •

edited by larsbergstrom

highfive commented Nov 2, 2016

bors-servo commented Nov 4, 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.

nox commented Nov 7, 2016

asajeffrey commented Nov 8, 2016

bors-servo commented Nov 8, 2016

nox commented Nov 15, 2016

asajeffrey commented Nov 15, 2016

bors-servo commented Nov 15, 2016

bors-servo commented Nov 15, 2016

bors-servo commented Nov 15, 2016

servo / servo

Join GitHub today

Storage notifications routed via the constellation. #14023

Storage notifications routed via the constellation. #14023

Conversation

asajeffrey commented Nov 2, 2016 • edited by larsbergstrom

highfive commented Nov 2, 2016

bors-servo commented Nov 4, 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.

nox commented Nov 7, 2016

asajeffrey commented Nov 8, 2016

bors-servo commented Nov 8, 2016

nox commented Nov 15, 2016

asajeffrey commented Nov 15, 2016

bors-servo commented Nov 15, 2016

bors-servo commented Nov 15, 2016

bors-servo commented Nov 15, 2016

asajeffrey commented Nov 2, 2016 •

edited by larsbergstrom