Opaque DOMString #8477

asajeffrey · 2015-11-12T00:21:25Z

This patch makes DOMString an opaque wrapper round String (currently it's a transparent wrapper).

The changes are:

Replacing DOMString(foo) by DOMString::from(foo).
Replacing foo.0 by String::from(foo).
Adding functions clear, push_str and extend for in-place mutation of DOMStrings.
Replacing DOMString by String in other threads (devtools, storage and filereader).
Making DOMString implement !Send.
Removing the pub attribute from the contents of DOMString.

This enables experimenting with other string representations in the DOM.

highfive · 2015-11-12T00:21:28Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify layout code, but no reftests are modified. Please consider adding a reftest!

pcwalton · 2015-11-12T00:29:59Z

Neat!

jdm · 2015-11-12T16:51:02Z

Thanks for splitting the commits up this way!
-S-awaiting-review +S-needs-code-changes

Reviewed 1 of 1 files at r1, 4 of 4 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, 3 of 3 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 63 of 63 files at r8, 1 of 1 files at r9, 5 of 5 files at r10, 1 of 1 files at r11.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

^{components/devtools/actors/inspector.rs, line 209 [r2] (raw file):}
I don't think these String::from calls are necessary any more.

^{components/script/dom/create.rs, line 85 [r8] (raw file):}
No need for {}.

^{components/script/dom/htmlinputelement.rs, line 579 [r10] (raw file):}
We should probably be fetching the placeholder attribute on-demand in the layout code instead of storing a duplicate. File an issue?

^{components/script/dom/storage.rs, line 131 [r5] (raw file):}
I don't have any better ideas.

^{components/script/script_task.rs, line 1705 [r3] (raw file):}
I'd prefer to keep this as DOMString and perform the conversion explicitly inside this function.

^{components/util/mem.rs, line 106 [r7] (raw file):}
What if we made it really obvious what we're doing by transmuting to &String and calling heap_size_of_children instead?

Comments from the review on Reviewable.io

asajeffrey · 2015-11-12T17:03:35Z

I'm getting better at rewriting my git history :)

Review status: all files reviewed at latest revision, 1 unresolved discussion.

^{components/util/mem.rs, line 106 [r7] (raw file):}
Well, this code is going to vanish pretty soon anyway, since the next task is to move str.rs out of util and into script/dom, at which point we can derive HeapSizeOf.

Comments from the review on Reviewable.io

jdm · 2015-11-12T17:29:38Z

@bors-servo: delegate+
-S-awaiting-review +S-needs-squash

Reviewed 1 of 1 files at r12, 1 of 1 files at r13, 1 of 1 files at r14.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

bors-servo · 2015-11-12T17:29:39Z

✌️ @asajeffrey can now approve this pull request

asajeffrey · 2015-11-12T19:27:08Z

Rebased, and updated the unit tests.

jdm · 2015-11-12T19:46:40Z

@bors-servo: r+

Reviewed 1 of 1 files at r15.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

bors-servo · 2015-11-12T19:46:41Z

📌 Commit 99c8a37 has been approved by jdm

jdm · 2015-11-12T19:47:08Z

@bors-servo: p=1

bors-servo · 2015-11-12T19:58:26Z

⌛ Testing commit 99c8a37 with merge e592382...


        Auto merge of #8477 - asajeffrey:opaque-domstring, r=jdm

Opaque DOMString This patch makes DOMString an opaque wrapper round String (currently it's a transparent wrapper). The changes are: * Replacing DOMString(foo) by DOMString::from(foo). * Replacing foo.0 by String::from(foo). * Adding functions clear, push_str and extend for in-place mutation of DOMStrings. * Replacing DOMString by String in other threads (devtools, storage and filereader). * Making DOMString implement !Send. * Removing the pub attribute from the contents of DOMString. This enables experimenting with other string representations in the DOM.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8477)

bors-servo · 2015-11-12T20:02:16Z

💔 Test failed - gonk

jdm · 2015-11-12T20:05:06Z

@bors-servo: retry

#8416

asajeffrey · 2015-11-12T23:40:52Z

Ah the trying-to-land-a-patch dance, how I love it. Rebased again.


        Added conversion functions between DOMString and String.

Implemented From<String> and From<&str> for DOMString, and From<DOMString> for String.


        Replaced DOMString by String in devtools.


        Replaced DOMString by String in filereader.


        Replaced DOMString by String in storage APIs.


        Made DOMString implement !Send.

This change makes DOMStrings only accessible from the main JS thread.


        Made DOMString implement HeapSizeOf.

We have to do this by hand because DOMString is defined in util.


        Replaced DOMString constructor by conversion functions.

Replaced DOMString(...) by DOMString::from(...). Replaced ....0 by String::from(...). Removed any uses of .to_owner() in DOMString::from("...").


        Code generation uses DOMString::from("...") rather than DOMString("..…

….".to_owned()).


        Added in-place mutation to DOMString.

The methods which are currently implemented are the ones on String that are currently being used: string.push_str(...), string.clear() and string.extend(...). We may want to revisit this API.


        Made DOMString opaque.

Removed the "pub" attribute from the String field of DOMString. This enables experimenting with other string representations.

asajeffrey · 2015-11-12T23:56:40Z

@bors-servo: r+

bors-servo · 2015-11-12T23:56:41Z

📌 Commit 0da1623 has been approved by asajeffrey

bors-servo · 2015-11-13T00:21:20Z

⌛ Testing commit 0da1623 with merge 1cb98c3...


        Auto merge of #8477 - asajeffrey:opaque-domstring, r=asajeffrey

Opaque DOMString This patch makes DOMString an opaque wrapper round String (currently it's a transparent wrapper). The changes are: * Replacing DOMString(foo) by DOMString::from(foo). * Replacing foo.0 by String::from(foo). * Adding functions clear, push_str and extend for in-place mutation of DOMStrings. * Replacing DOMString by String in other threads (devtools, storage and filereader). * Making DOMString implement !Send. * Removing the pub attribute from the contents of DOMString. This enables experimenting with other string representations in the DOM.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8477)

bors-servo · 2015-11-13T00:52:39Z

💔 Test failed - linux-rel

eefriedman · 2015-11-13T01:42:08Z

@bors-servo retry #8487

bors-servo · 2015-11-13T01:47:31Z

⌛ Testing commit 0da1623 with merge 62acdd3...


        Auto merge of #8477 - asajeffrey:opaque-domstring, r=asajeffrey

Opaque DOMString This patch makes DOMString an opaque wrapper round String (currently it's a transparent wrapper). The changes are: * Replacing DOMString(foo) by DOMString::from(foo). * Replacing foo.0 by String::from(foo). * Adding functions clear, push_str and extend for in-place mutation of DOMStrings. * Replacing DOMString by String in other threads (devtools, storage and filereader). * Making DOMString implement !Send. * Removing the pub attribute from the contents of DOMString. This enables experimenting with other string representations in the DOM.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8477)

bors-servo · 2015-11-13T02:28:19Z

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

highfive added the S-awaiting-review label Nov 12, 2015

asajeffrey assigned jdm Nov 12, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 12, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 12, 2015

jdm added S-needs-squash and removed S-awaiting-review labels Nov 12, 2015

asajeffrey force-pushed the asajeffrey:opaque-domstring branch from 1341a4b to 0816dda Nov 12, 2015

highfive added the S-awaiting-review label Nov 12, 2015

asajeffrey removed the S-needs-squash label Nov 12, 2015

asajeffrey force-pushed the asajeffrey:opaque-domstring branch from 0816dda to 99c8a37 Nov 12, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 12, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 12, 2015

asajeffrey removed the S-needs-rebase label Nov 12, 2015

asajeffrey added 10 commits Nov 10, 2015

Added conversion functions between DOMString and String.

9cbc439

Implemented From<String> and From<&str> for DOMString, and From<DOMString> for String.

Replaced DOMString by String in devtools.

5101506

Replaced DOMString by String in filereader.

582273e

Replaced DOMString by String in storage APIs.

cf340be

Made DOMString implement !Send.

c8af5b6

This change makes DOMStrings only accessible from the main JS thread.

Made DOMString implement HeapSizeOf.

736323a

We have to do this by hand because DOMString is defined in util.

Replaced DOMString constructor by conversion functions.

84bde75

Replaced DOMString(...) by DOMString::from(...). Replaced ....0 by String::from(...). Removed any uses of .to_owner() in DOMString::from("...").

Code generation uses DOMString::from("...") rather than DOMString("..…

034769f

….".to_owned()).

Added in-place mutation to DOMString.

5db67b5

The methods which are currently implemented are the ones on String that are currently being used: string.push_str(...), string.clear() and string.extend(...). We may want to revisit this API.

Made DOMString opaque.

Loading status checks…

0da1623

Removed the "pub" attribute from the String field of DOMString. This enables experimenting with other string representations.

asajeffrey force-pushed the asajeffrey:opaque-domstring branch from 2dadea7 to 0da1623 Nov 12, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 12, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 13, 2015

highfive added S-awaiting-merge and removed S-tests-failed labels Nov 13, 2015

paulrouget mentioned this pull request Nov 14, 2015

mozbrowsericonchange event (Browser API) #8449

Merged

servo / servo

Opaque DOMString #8477

Opaque DOMString #8477

asajeffrey commented Nov 12, 2015

highfive commented Nov 12, 2015

pcwalton commented Nov 12, 2015

jdm commented Nov 12, 2015

asajeffrey commented Nov 12, 2015

jdm commented Nov 12, 2015

bors-servo commented Nov 12, 2015

asajeffrey commented Nov 12, 2015

jdm commented Nov 12, 2015

bors-servo commented Nov 12, 2015

jdm commented Nov 12, 2015

bors-servo commented Nov 12, 2015

bors-servo commented Nov 12, 2015

jdm commented Nov 12, 2015

asajeffrey commented Nov 12, 2015

asajeffrey commented Nov 12, 2015

bors-servo commented Nov 12, 2015

bors-servo commented Nov 13, 2015

bors-servo commented Nov 13, 2015

eefriedman commented Nov 13, 2015

bors-servo commented Nov 13, 2015

bors-servo commented Nov 13, 2015

servo / servo

Join GitHub today

Opaque DOMString #8477

Opaque DOMString #8477

Conversation

asajeffrey commented Nov 12, 2015

highfive commented Nov 12, 2015

pcwalton commented Nov 12, 2015

jdm commented Nov 12, 2015

asajeffrey commented Nov 12, 2015

jdm commented Nov 12, 2015

bors-servo commented Nov 12, 2015

asajeffrey commented Nov 12, 2015

jdm commented Nov 12, 2015

bors-servo commented Nov 12, 2015

jdm commented Nov 12, 2015

bors-servo commented Nov 12, 2015

bors-servo commented Nov 12, 2015

jdm commented Nov 12, 2015

asajeffrey commented Nov 12, 2015

asajeffrey commented Nov 12, 2015

bors-servo commented Nov 12, 2015

bors-servo commented Nov 13, 2015

bors-servo commented Nov 13, 2015

eefriedman commented Nov 13, 2015

bors-servo commented Nov 13, 2015

bors-servo commented Nov 13, 2015