Storage notifications routed via the constellation. by asajeffrey · Pull Request #14023 · servo/servo

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 Dispatch StorageEvent events to other windows when modifying storage instances #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:01:58Z

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:02:08Z

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:02:46Z

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:04:44Z

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:05:50Z

components/script_traits/script_msg.rs

 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.

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...

…=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 Related to the script thread A-constellation Involves the constellation labels Nov 2, 2016

highfive assigned Ms2ger Nov 2, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 2, 2016

asajeffrey added the S-needs-squash Some (or all) of the commits in the PR should be combined. label Nov 3, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Nov 4, 2016

asajeffrey force-pushed the storage-notify-via-constellation branch from a8b66da to 75867ef Compare November 4, 2016 02:38

asajeffrey removed the S-needs-rebase There are merge conflict errors. label Nov 4, 2016

nox suggested changes Nov 7, 2016

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 7, 2016

asajeffrey added the S-awaiting-review There is new code that needs to be reviewed. label Nov 8, 2016

highfive removed the S-needs-code-changes Changes have not yet been made that were requested by a reviewer. label Nov 8, 2016

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

highfive added the S-needs-rebase There are merge conflict errors. label Nov 8, 2016

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

jdm removed the S-needs-rebase There are merge conflict errors. label Nov 15, 2016

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

Storage notifications routed via the constellation.

c91ef86

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

highfive unassigned Ms2ger Nov 15, 2016

highfive assigned nox Nov 15, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Nov 15, 2016

bors-servo merged commit c91ef86 into servo:master Nov 15, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. 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

Uh oh!

Conversation

asajeffrey commented Nov 2, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Nov 2, 2016

Uh oh!

bors-servo commented Nov 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nox commented Nov 7, 2016

Uh oh!

asajeffrey commented Nov 8, 2016

Uh oh!

bors-servo commented Nov 8, 2016

Uh oh!

nox commented Nov 15, 2016

Uh oh!

asajeffrey commented Nov 15, 2016

Uh oh!

bors-servo commented Nov 15, 2016

Uh oh!

bors-servo commented Nov 15, 2016

Uh oh!

bors-servo commented Nov 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

asajeffrey commented Nov 2, 2016 •

edited by larsbergstrom

Loading