Skip to content

script: Allow DOMString to convert efficiently from static strings - #47479

Merged
yezhizhen merged 4 commits into
servo:mainfrom
Narfinger:static-domstring
Aug 26, 2026
Merged

yezhizhen merged 4 commits into
servo:mainfrom
Narfinger:static-domstring

Conversation

@Narfinger

Copy link
Copy Markdown
Contributor

We allow DOMStrings to efficiently convert from static strings by introducing the DOMString::from_static method.
We achieve this by having another entry in our enum that holds the static string. This will safe on memory for DOMStrings that never change.

The rest of the changes are mechanical and the static lifetime guarantees that the string will never go out of scope.

Adapted the original idea from in house fork.

Testing: WPT tests should find anything that breaks.

@Narfinger
Narfinger requested a review from gterzian as a code owner August 24, 2026 13:19
@servo-highfive servo-highfive added S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Aug 24, 2026
Signed-off-by: Narfinger <Narfinger@users.noreply.github.com>
@servo-highfive servo-highfive removed the S-needs-rebase There are merge conflict errors. label Aug 24, 2026
Signed-off-by: Narfinger <Narfinger@users.noreply.github.com>
@yezhizhen
yezhizhen self-requested a review August 25, 2026 08:10

@yezhizhen yezhizhen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to adapt this, otherwise we fall into Latin1 encoding:

pub fn encoded_bytes(&self) -> EncodedBytes<'_> {
let inner = self.0.borrow();
match &*inner {
DOMStringType::Rust(..) => {
EncodedBytes::Utf8(Ref::map(inner, |inner| inner.as_raw_bytes()))
},
_ => EncodedBytes::Latin1(Ref::map(inner, |inner| inner.as_raw_bytes())),

@servo-highfive servo-highfive 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 Aug 25, 2026
Signed-off-by: Narfinger <Narfinger@users.noreply.github.com>
@servo-highfive servo-highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Aug 25, 2026
@Narfinger

Copy link
Copy Markdown
Contributor Author

Oh good find!

@Narfinger
Narfinger requested a review from yezhizhen August 25, 2026 11:41

@TimvdLippe TimvdLippe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mechanical changes LGTM. Maybe we can improve some linter to make this more obvious to future usages? I think this is an easy thing to backslide

Comment thread components/script_bindings/domstring.rs
@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Aug 25, 2026
@yezhizhen

Copy link
Copy Markdown
Member

We should add some tests for the new variant and fn. Similar to those already in the file.

Signed-off-by: Narfinger <Narfinger@users.noreply.github.com>
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 26, 2026
@Narfinger

Copy link
Copy Markdown
Contributor Author

Added some tests.

@TimvdLippe
TimvdLippe dismissed yezhizhen’s stale review August 26, 2026 08:11

Re-requesting review button doesn't work, but tests have been added.

@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Aug 26, 2026
@yezhizhen
yezhizhen added this pull request to the merge queue Aug 26, 2026
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Aug 26, 2026
Merged via the queue into servo:main with commit 36cc4ea Aug 26, 2026
36 checks passed
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Aug 26, 2026
@Narfinger
Narfinger deleted the static-domstring branch August 26, 2026 14:08
RichardTjokroutomo pushed a commit to RichardTjokroutomo/servo that referenced this pull request Oct 6, 2026
…ervo#47479)

We allow DOMStrings to efficiently convert from static strings by
introducing the DOMString::from_static method.
We achieve this by having another entry in our enum that holds the
static string. This will safe on memory for DOMStrings that never
change.


The rest of the changes are mechanical and the static lifetime
guarantees that the string will never go out of scope.

Adapted the original idea from in house fork.

Testing: WPT tests should find anything that breaks.

---------

Signed-off-by: Narfinger <Narfinger@users.noreply.github.com>
Psychpsyo pushed a commit to Psychpsyo/servo that referenced this pull request Oct 12, 2026
…ervo#47479)

We allow DOMStrings to efficiently convert from static strings by
introducing the DOMString::from_static method.
We achieve this by having another entry in our enum that holds the
static string. This will safe on memory for DOMStrings that never
change.


The rest of the changes are mechanical and the static lifetime
guarantees that the string will never go out of scope.

Adapted the original idea from in house fork.

Testing: WPT tests should find anything that breaks.

---------

Signed-off-by: Narfinger <Narfinger@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants