1351957 - stylo: Use a single thread-safe holder for base uri, referrer, and principal

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P1)

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1351957 - Create URLExtraData for holding base uri, referrer, and principal.

Review commit: https://reviewboard.mozilla.org/r/125000/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125000/

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Update binding files.

Review commit: https://reviewboard.mozilla.org/r/125014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125014/

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b5c65c0b20cb97aaced45a275e5a1007bcf5ab7

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8852842 [details]
Bug 1351957 - Create URLExtraData for holding base uri, referrer, and principal.

https://reviewboard.mozilla.org/r/125000/#review128340

::: layout/style/ServoBindings.cpp:1533
(Diff revision 1)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(aLoader, "Should've catched this before");
>    MOZ_ASSERT(aParent, "Only used for @import, so parent should exist!");
>    MOZ_ASSERT(aURLString, "Invalid URLs shouldn't be loaded!");
> -  MOZ_ASSERT(aBaseURI, "Need a base URI");
> +  MOZ_ASSERT(aBaseURLData, "Need a base URL data");

Nit: s/a base/base/

::: layout/style/nsCSSValue.h:134
(Diff revision 1)
>    // For both constructors aString must not be null.
>    // For both constructors aOriginPrincipal must not be null.
>    // Construct with a base URI; this will create the actual URI lazily from
>    // aString and aBaseURI.

Please update this comment to talk about the requirements of the URLExtraData object.

::: layout/style/nsCSSValue.cpp:2864
(Diff revision 1)
> -          (mOriginPrincipal == aOther.mOriginPrincipal ||
> -           self.mOriginPrincipal.get()->Equals(other.mOriginPrincipal.get())) &&
> +          (self->GetPrincipal() == other->GetPrincipal() ||
> +           self->GetPrincipal()->Equals(other->GetPrincipal())) &&

Does this mean the principal can never be null?

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8852842 [details]
Bug 1351957 - Create URLExtraData for holding base uri, referrer, and principal.

https://reviewboard.mozilla.org/r/125000/#review128340

> Does this mean the principal can never be null?

The assertions in constructors of `URLValueData` should enforce that. We cannot make `URLExtraData` check that because there seem to be cases where the principal of `URLExtraData` can be null. (I forgot the details, but if I check that at the constructor of `URLExtraData`, it asserts in many tests.)

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8852842 [details]
Bug 1351957 - Create URLExtraData for holding base uri, referrer, and principal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125000/diff/1-2/

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

servo/servo#16235

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21926c2cc38d
Create URLExtraData for holding base uri, referrer, and principal. r=heycam

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/21926c2cc38d