Page MenuHomePhabricator

Bug 1552708 - Use cbindgen for URIs. r=heycam
ClosedPublic

Authored by emilio on May 19 2019, 7:51 PM.

Details

Summary

This doesn't clean up as much as a whole, but it's a step in the right
direction. In particular, it allows us to start using simple bindings for:

  • Filters
  • Shapes and images, almost. Need to:
    • Get rid of the complex -moz- gradient parsing (let layout.css.simple-moz-gradient.enabled get to release).
  • Counters, almost. Need to:
    • Share the Attr representation with Gecko, by not using Option<>.
      • Just another variant should be enough (ContentItem::{Attr,Prefixedattr}, maybe).

Which in turn allows us to remove a whole lot of bindings in followups to this.

The setup changes a bit. This also removes the double pointer I complained about
while reviewing the shared UA sheet patches. The old setup is:

SpecifiedUrl
 * CssUrl
   * Arc<CssUrlData>
     * String
     * UrlExtraData
 * UrlValueSource
   * Arc<CssUrlData>
   * load id
   * resolved uri
   * CORS mode.
   * ...

The new one removes the double reference to the url data via URLValue, and looks
like:

SpecifiedUrl
 * CssUrl
   * Arc<CssUrlData>
     * String
     * UrlExtraData
     * CorsMode
     * LoadData
       * load id
       * resolved URI

The LoadData is the only mutable bit that C++ can change, and is not used from
Rust. Ideally, in the future, we could just use rust-url to resolve the URL
after parsing or something, and make it all immutable. Maybe.

I've verified that this approach still works with the UA sheet patches (via the
LoadDataSource::Lazy).

The reordering of mWillChange is to avoid nsStyleDisplay from going over the
size limit. We want to split it up anyway in bug 1552587, but mBinding gains a
tag member, which means that we were having a bit of extra padding.

One thing I want to explore is to see if we can abuse rustc's non-zero
optimizations to predict the layout from C++, but that's something to explore at
some other point in time and with a lot of care and help from Michael (who sits
next to me and works on rustc ;)).

Diff Detail

Repository
rMOZILLACENTRAL mozilla-central
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

emilio created this revision.May 19 2019, 7:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2019, 7:51 PM
phab-bot requested review of this revision.May 19 2019, 7:51 PM
phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: Restricted Project.
emilio updated this revision to Diff 106092.May 19 2019, 8:46 PM

Whoops, do not change -moz-image-rect parsing

emilio edited the summary of this revision. (Show Details)May 19 2019, 8:47 PM
heycam accepted this revision.May 27 2019, 7:03 AM

Thanks, looks good! Couple of questions, r=me with good answers. :-) Sorry for the delay!

dom/base/Element.cpp
599–600

Maybe this comment can go away.

layout/style/ImageLoader.cpp
211

Is there much of a worry of doing calling this from another thread?

layout/style/ServoStyleConstsInlines.h
171

Why is this called UnsafeRelease?

layout/style/nsStyleStruct.cpp
82

Before we weren't comparing the principal for properties other than -moz-binding. I guess this is OK, since although we might end up repainting if switching to a URL in one style sheet to the same URL specified in another style sheets, it's not likely to be a common thing...

2159–2160

mImageURL

2825–2826

What about if one is null and the other is non-null?

servo/components/style/gecko/url.rs
237

"in a stable location"

336

What's the difference between extern fn and extern "C" fn here?

This revision is now accepted and ready to land.May 27 2019, 7:03 AM
emilio added inline comments.May 27 2019, 7:24 AM
layout/style/ImageLoader.cpp
211

Not really, will revert this bit.

layout/style/ServoStyleConstsInlines.h
171

Because it's public, but it frees the pointer... But I guess this is C++ code so I may as well just call it ReleaseRef, will do that.

servo/components/style/gecko/url.rs
336

no difference, can keep the "C" if you want :)

emilio marked 8 inline comments as done.May 27 2019, 11:39 AM
emilio added inline comments.
layout/style/ServoStyleConstsInlines.h
171

Ah, I did make it private, so Release() it is I guess.

layout/style/nsStyleStruct.cpp
82

Yeah, I don't think it's a thing that matters much either way.

2825–2826

Good point, fixed, and also removed the pointer comparison above which is redundant since mImage != aOther.mImage also handles it.

emilio updated this revision to Diff 110348.May 27 2019, 11:45 AM
emilio marked 2 inline comments as done.
emilio edited the summary of this revision. (Show Details)

Fix nits

emilio edited the summary of this revision. (Show Details)May 27 2019, 11:45 AM
This revision was automatically updated to reflect the committed changes.
cbrindusan reopened this revision.May 27 2019, 2:03 PM
This revision is now accepted and ready to land.May 27 2019, 2:03 PM
emilio updated this revision to Diff 110421.May 27 2019, 2:07 PM
emilio edited the summary of this revision. (Show Details)

Update tests

emilio edited the summary of this revision. (Show Details)May 27 2019, 2:07 PM
This revision was automatically updated to reflect the committed changes.