Update to string-cache 0.3 #14043

SimonSapin · 2016-11-03T13:42:32Z

Previously, string-cache defined:

An string-like Atom type,
An atom!("foo") macro that expands to a value of that type, for a set of strings known at compile-time,
A struct Namespace(Atom); type
A ns!(html) macro that maps known prefixed to Namespace values with the corresponding namespace URL.

Adding a string to the static set required making a change to the string-cache crate.

With 0.3, the Atom type is now generic, with a type parameter that provides a set of static strings. We can have multiple such sets, defined in different crates. The string_cache_codegen crate, to be used in build scripts, generates code that defines such a set, a new atom type (a type alias for Atom<_> with the type parameter set), and an atom!-like macro.

The html5ever repository has a new html5ever_atoms crate that defines three such types: Prefix, Namespace, and LocalName (with respective namespace_prefix!, namespace_url!, and local_name! macros). It also defines the ns! macro like before.

This repository has a new servo_atoms crate in components/atoms that, for now, defines a single Atom type (and atom!) macro. (servo_atoms::Atom is defined as something like type Atom = string_cache::Atom<ServoStaticStringSet>;, so overall there’s now two types named Atom.)

In this PR, servo_atoms::Atom is used for everything else that was string_cache::Atom before. But more atom types can be defined as needed. Two reasons to do this are to auto-generate the set of static strings (I’m planning to do this for CSS property names, which is the motivation for this change), or to have the type system help us avoid mix up unrelated things (this is why we had a Namespace type ever before this change).

Introducing new types helped me find a bug: when creating a new attribute dom::Element::set_style_attr, would pass Some(atom!("style")) instead of None (now Option<html5ever_atoms::Prefix> instead of Option<string_cache::Atom>) to the prefix argument of Attr::new. I suppose the author of that code confused it with the local_name argument.

Note that Stylo is not affected by any of this. The gecko_string_cache module is unchanged, with a single Atom type. The style crate conditionally compiles Prefix and LocalName re-exports for that are both gecko_string_cache::Atom on stylo.

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

nox · 2016-11-03T13:44:57Z

@bors-servo r+

bors-servo · 2016-11-03T13:45:01Z

📌 Commit b16f9da has been approved by nox

SimonSapin · 2016-11-03T13:49:08Z

@bors-servo p=1000

This will bitrot so fast

bors-servo · 2016-11-03T14:49:57Z

⌛ Testing commit b16f9da with merge 4926dd3...

Update to string-cache 0.3 Previously, `string-cache` defined: * An string-like `Atom` type, * An `atom!("foo")` macro that expands to a value of that type, for a set of strings known at compile-time, * A `struct Namespace(Atom);` type * A `ns!(html)` macro that maps known prefixed to `Namespace` values with the corresponding namespace URL. Adding a string to the static set required making a change to the `string-cache` crate. With 0.3, the `Atom` type is now generic, with a type parameter that provides a set of static strings. We can have multiple such sets, defined in different crates. The `string_cache_codegen` crate, to be used in build scripts, generates code that defines such a set, a new atom type (a type alias for `Atom<_>` with the type parameter set), and an `atom!`-like macro. The html5ever repository has a new `html5ever_atoms` crate that defines three such types: `Prefix`, `Namespace`, and `LocalName` (with respective `namespace_prefix!`, `namespace_url!`, and `local_name!` macros). It also defines the `ns!` macro like before. This repository has a new `servo_atoms` crate in `components/atoms` that, for now, defines a single `Atom` type (and `atom!`) macro. (`servo_atoms::Atom` is defined as something like `type Atom = string_cache::Atom<ServoStaticStringSet>;`, so overall there’s now two types named `Atom`.) In this PR, `servo_atoms::Atom` is used for everything else that was `string_cache::Atom` before. But more atom types can be defined as needed. Two reasons to do this are to auto-generate the set of static strings (I’m planning to do this for CSS property names, which is the motivation for this change), or to have the type system help us avoid mix up unrelated things (this is why we had a `Namespace` type ever before this change). Introducing new types helped me find a bug: when creating a new attribute `dom::Element::set_style_attr`, would pass `Some(atom!("style"))` instead of `None` (now `Option<html5ever_atoms::Prefix>` instead of `Option<string_cache::Atom>`) to the `prefix` argument of `Attr::new`. I suppose the author of that code confused it with the `local_name` argument. --- Note that Stylo is not affected by any of this. The `gecko_string_cache` module is unchanged, with a single `Atom` type. The `style` crate conditionally compiles `Prefix` and `LocalName` re-exports for that are both `gecko_string_cache::Atom` on stylo. ---  - [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).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____

bors-servo · 2016-11-03T15:12:03Z

💔 Test failed - mac-dev-unit

SimonSapin · 2016-11-03T15:23:35Z

@bors-servo r=nox

Updated CEF lockfile again post-rebase.

bors-servo · 2016-11-03T15:23:36Z

📌 Commit 53b638c has been approved by nox

bors-servo · 2016-11-03T16:19:29Z

⌛ Testing commit 53b638c with merge 4aeeed4...

Update to string-cache 0.3 Previously, `string-cache` defined: * An string-like `Atom` type, * An `atom!("foo")` macro that expands to a value of that type, for a set of strings known at compile-time, * A `struct Namespace(Atom);` type * A `ns!(html)` macro that maps known prefixed to `Namespace` values with the corresponding namespace URL. Adding a string to the static set required making a change to the `string-cache` crate. With 0.3, the `Atom` type is now generic, with a type parameter that provides a set of static strings. We can have multiple such sets, defined in different crates. The `string_cache_codegen` crate, to be used in build scripts, generates code that defines such a set, a new atom type (a type alias for `Atom<_>` with the type parameter set), and an `atom!`-like macro. The html5ever repository has a new `html5ever_atoms` crate that defines three such types: `Prefix`, `Namespace`, and `LocalName` (with respective `namespace_prefix!`, `namespace_url!`, and `local_name!` macros). It also defines the `ns!` macro like before. This repository has a new `servo_atoms` crate in `components/atoms` that, for now, defines a single `Atom` type (and `atom!`) macro. (`servo_atoms::Atom` is defined as something like `type Atom = string_cache::Atom<ServoStaticStringSet>;`, so overall there’s now two types named `Atom`.) In this PR, `servo_atoms::Atom` is used for everything else that was `string_cache::Atom` before. But more atom types can be defined as needed. Two reasons to do this are to auto-generate the set of static strings (I’m planning to do this for CSS property names, which is the motivation for this change), or to have the type system help us avoid mix up unrelated things (this is why we had a `Namespace` type ever before this change). Introducing new types helped me find a bug: when creating a new attribute `dom::Element::set_style_attr`, would pass `Some(atom!("style"))` instead of `None` (now `Option<html5ever_atoms::Prefix>` instead of `Option<string_cache::Atom>`) to the `prefix` argument of `Attr::new`. I suppose the author of that code confused it with the `local_name` argument. --- Note that Stylo is not affected by any of this. The `gecko_string_cache` module is unchanged, with a single `Atom` type. The `style` crate conditionally compiles `Prefix` and `LocalName` re-exports for that are both `gecko_string_cache::Atom` on stylo. ---  - [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).  - [ ] 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/14043)

bors-servo · 2016-11-03T16:19:40Z

⛄ The build was interrupted to prioritize another pull request.

bors-servo · 2016-11-03T16:19:45Z

⌛ Testing commit 53b638c with merge 5b4cc95...

Update to string-cache 0.3 Previously, `string-cache` defined: * An string-like `Atom` type, * An `atom!("foo")` macro that expands to a value of that type, for a set of strings known at compile-time, * A `struct Namespace(Atom);` type * A `ns!(html)` macro that maps known prefixed to `Namespace` values with the corresponding namespace URL. Adding a string to the static set required making a change to the `string-cache` crate. With 0.3, the `Atom` type is now generic, with a type parameter that provides a set of static strings. We can have multiple such sets, defined in different crates. The `string_cache_codegen` crate, to be used in build scripts, generates code that defines such a set, a new atom type (a type alias for `Atom<_>` with the type parameter set), and an `atom!`-like macro. The html5ever repository has a new `html5ever_atoms` crate that defines three such types: `Prefix`, `Namespace`, and `LocalName` (with respective `namespace_prefix!`, `namespace_url!`, and `local_name!` macros). It also defines the `ns!` macro like before. This repository has a new `servo_atoms` crate in `components/atoms` that, for now, defines a single `Atom` type (and `atom!`) macro. (`servo_atoms::Atom` is defined as something like `type Atom = string_cache::Atom<ServoStaticStringSet>;`, so overall there’s now two types named `Atom`.) In this PR, `servo_atoms::Atom` is used for everything else that was `string_cache::Atom` before. But more atom types can be defined as needed. Two reasons to do this are to auto-generate the set of static strings (I’m planning to do this for CSS property names, which is the motivation for this change), or to have the type system help us avoid mix up unrelated things (this is why we had a `Namespace` type ever before this change). Introducing new types helped me find a bug: when creating a new attribute `dom::Element::set_style_attr`, would pass `Some(atom!("style"))` instead of `None` (now `Option<html5ever_atoms::Prefix>` instead of `Option<string_cache::Atom>`) to the `prefix` argument of `Attr::new`. I suppose the author of that code confused it with the `local_name` argument. --- Note that Stylo is not affected by any of this. The `gecko_string_cache` module is unchanged, with a single `Atom` type. The `style` crate conditionally compiles `Prefix` and `LocalName` re-exports for that are both `gecko_string_cache::Atom` on stylo. ---  - [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).  - [ ] 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/14043)

bors-servo · 2016-11-03T18:04:46Z

☀️ 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

highfive assigned nox Nov 3, 2016

highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 3, 2016

SimonSapin mentioned this pull request Nov 3, 2016

Add more interned strings; bump version. servo/html5ever#230

Merged

frewsxcv mentioned this pull request Nov 3, 2016

A couple improvements to EventTarget event firing. #14036

Merged

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 3, 2016

Update to string-cache 0.3

53b638c

SimonSapin force-pushed the string-cache-up branch from b16f9da to 53b638c Compare November 3, 2016 15:23

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 3, 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. labels Nov 3, 2016

bors-servo merged commit 53b638c into master Nov 3, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 3, 2016

This was referenced Nov 3, 2016

ImageMaps: Implemented support for parsing coord attribute to a shape… #13972

Merged

fix getElementsByTagName() #14047

Merged

This was referenced Nov 3, 2016

Support sequential focus navigation #13460

Closed

Add support for fullscreen #10102 #13489

Merged

Implement synchronous about:blank loading. #13996

Merged

Move some types out of msg. #13816

Merged

SimonSapin deleted the string-cache-up branch November 3, 2016 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Update to string-cache 0.3 #14043

Update to string-cache 0.3 #14043

Uh oh!

SimonSapin commented Nov 3, 2016 •

edited by larsbergstrom

Loading

Uh oh!

nox commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

SimonSapin commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

SimonSapin commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

Uh oh!

Uh oh!

Update to string-cache 0.3 #14043

Update to string-cache 0.3 #14043

Uh oh!

Conversation

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

Uh oh!

Uh oh!

nox commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

SimonSapin commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

SimonSapin commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

Uh oh!

SimonSapin commented Nov 3, 2016 •

edited by larsbergstrom

Loading