Closed Bug 1372041 Opened 7 years ago Closed 7 years ago

stylo: <link media> doesn't respect the "media" attribute.

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
56 Branch
Platform:
x86_64
Linux
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Depends on 1 open bug, Blocks 1 open bug,
URL
)

Details

Attachments

(5 files, 1 obsolete file)

Bug 1372041: Refactor a bunch of stuff so link rel="media" is honored.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1372041: Refactor @import so it also respects the stylesheet's media properly.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1372041: Update test expectations.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1372041: Test.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1372041: Test for 1376092, while we're here.
(deleted), text/x-review-board-request
heycam
: review+
Details
See the bug url for example. The homepage seems ok, but any other page seems garbled, presumably because we lost track of some stylesheet? Some of the stylesheets that look as-if they didn't apply are stylesheets that look like the ones that bug 1357583 handles, but disabling that optimization doesn't help, so presumably something else is going on...
Blocks: stylo
Ok, so this is because they have: <link rel="stylesheet" type="text/css" media="print" href="About%20Meetup%20%7C%20Meetup_files/print.css"> And that still applies to the document, for some reason.
Summary: stylo: Meetup.com seems garbled. → stylo: <link media> doesn't respect the "media" attribute.
So this is because we don't do anything particularly in StyleSheet::SetMedia, and we don't tell that the media changed to Servo, which keeps its own MediaList. I bet we would have the same problem with "disabled" if disabled sheets weren't out of the stylesheet list. So there's some state that doesn't belong to StyleSheet and belongs to CSSStyleSheet or Servo's style::stylesheets::StyleSheet, which are: * StyleSheet::mMedia -> Should reflect servo's Media, so we should swap the inner MediaList with the underlying media list that gets provided to SetMedia. * StyleSheet::mDisabled -> Should probably be moved to CSSStyleSheet, and grab it from the Servo stylesheet object, or at least document in the Servo stylesheet object that this isn't used for Gecko. Brad, I mentioned the mDisabled thing a while ago while reviewing a patch of you, and I think there isn't any bug on file for that, would you be able to take this? Here's a simple test-case: * print.css: ``` * { color: red } ``` * test.html: ``` <!doctype html> <link media="print" rel="stylesheet" href="print.css"> This shouldn't be red. ```
Flags: needinfo?(bwerth)
Actually I guess it's time for Stylesheet to be an Arc<Locked<Stylesheet>> instead of just Arc<Stylesheet>... That would allow us to get rid of the ugly RwLock<Namespaces>, and potentially of the Arc<Locked<CssRules>> too?
(In reply to Emilio Cobos Álvarez [:emilio] from comment #3) > Actually I guess it's time for Stylesheet to be an Arc<Locked<Stylesheet>> > instead of just Arc<Stylesheet>... That would allow us to get rid of the > ugly RwLock<Namespaces>, and potentially of the Arc<Locked<CssRules>> too? That being said, should that sheet be getting even downloaded?
> That being said, should that sheet be getting even downloaded? Yes. You don't want to be trying to sync-download it (which isn't even a concept) when the user goes to print the page. See also discussion in bug 79021. The current Gecko behavior is to download regardless of media (detectable vis CSSOM whether this happened), to enable or not regardless of media (again detectable via CSSOM), but not apply the rules if the media does not match, in the usual way.
Ok, so what's happening here is quite tricky, and is the following: * A preload gets triggered, when the parser sees the <link>, presumably. That triggers a load, that starts right away, and creates a new sheet with empty media, let's call this ServoStyleSheet (a). * Then the proper <link> gets inserted into the document, and goes to the loader with the proper media, clones (a), and sets the media to "print", let's call this clone (b). At this point the stylesheet isn't parsed yet, for obvious reasons. Now the load ends, and we call ParseSheet _on (a)_, so we don't see the media attribute on (b), and create a servo-side sheet with no media, which always applies. When parsed, it notifies both observers, the stub one from the preload and the link sheet, which inserts (b) into the document. (b) has the correct state in the Gecko side, but has an empty media from Servo's point of view. So the problem is somehow deeper than what I thought, and has implications with stylesheet reusing... In particular, we can't use Servo's enabled/disabled state, nor Servo's media stuff, because they don't correspond to the proper Gecko stuff... Now regarding what to do, I think there are a few solutions. The simpler (IMO) is diverging between Servo's stylesheet and Gecko's stylesheet. We already parameterize on an iterator, so it should be easy-ish. In particular, I'm proposing something like: trait StylesheetInDocument : Sized { // Or a better name fn media(&self) -> Option<&Locked<MediaList>>; fn enabled(&self) -> bool; fn rules(&self) -> &Locked<CssRules>; } In Servo, StylesheetInDocument would be just Stylesheet. In Gecko, we'd compile out the |media| and |disabled| fields of Stylesheet, and use our StylesheetSetEntry, which right now is: pub struct StylesheetSetEntry { unique_id: u64, sheet: Arc<Locked<Stylesheet>>, } But could be something like: pub struct StylesheetSetEntry { unique_id: u64, media: Option<Arc<Locked<MediaList>>, sheet: Arc<Locked<Stylesheet>>, } And always return |true| for |enabled()|, because the ServoStyleSet takes care of that. Of course, we may need to do a similar thing for @import. What do you think Cameron/Xidorn/Brad?
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(cam)
Depends on: 1372439
Real webpage is garbled, P1.
Priority: -- → P1
But ServoStyleSet doesn't take care of media on @import rule, does it? So it seems the main problem is that, in Gecko, media is stored in StyleSheet, but the raw Servo Stylesheet, which also has media field, is referenced from Inner, and the Inner is supposed to be media-neutral from Gecko's point of view. Then, what about, moving media into Inner and have Inner handles all media changes instead? With that, we sync the model between Gecko and Servo, and don't need lots of extra abstractions. We would lose the ability to share a same sheet content when media is different, but do we really care about that? Why would author want to reference a single stylesheet with different media in the same document, especially given that they can just list multiple media queries in a single media list? That sounds weird enough that we probably don't need to care too much... To do that, we need to carefully ensure unique inner when media is going to differ, but I guess that's probably simpler and more maintainable than further special-casing Gecko case from Servo code.
Flags: needinfo?(xidorn+moz)
Alternatively, if Servo is going to implement some kind of stylesheet sharing, maybe it would eventually need to split inner from stylesheet? In that case, is it possible to do the split now? But that also means Gecko's StyleSheet and Inner would probably both need to hold a raw pointer to Servo object, and we would have more stuff to keep sync. Not sure whether it's worth it.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #8) > But ServoStyleSet doesn't take care of media on @import rule, does it? Well, we do care, that's why @import url(...) media; works. It's on @import's Arc<Stylesheet> right now. > So it seems the main problem is that, in Gecko, media is stored in > StyleSheet, but the raw Servo Stylesheet, which also has media field, is > referenced from Inner, and the Inner is supposed to be media-neutral from > Gecko's point of view. Right. > Then, what about, moving media into Inner and have Inner handles all media > changes instead? With that, we sync the model between Gecko and Servo, and > don't need lots of extra abstractions. > We would lose the ability to share a same sheet content when media is > different, but do we really care about that? Why would author want to > reference a single stylesheet with different media in the same document, > especially given that they can just list multiple media queries in a single > media list? That sounds weird enough that we probably don't need to care too > much... Fair enough, but... read below. > To do that, we need to carefully ensure unique inner when media is going to > differ, but I guess that's probably simpler and more maintainable than > further special-casing Gecko case from Servo code. The problem with that is that it may make us clone too many stylesheets. See the case exposed in comment 6. Following your suggestion, when loading the second (non-preloaded) sheet, which has the media attribute, we'd need to call EnsureUniqueInner(), cloning the whole stylesheet. I don't think that's acceptable IMO, and fixing that may be equally (or more) non-trivial than my solution posted above, isn't it?
(In reply to Emilio Cobos Álvarez [:emilio] from comment #10) > (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #8) > > But ServoStyleSet doesn't take care of media on @import rule, does it? > > Well, we do care, that's why @import url(...) media; works. It's on > @import's Arc<Stylesheet> right now. IIUC it works because we check it when iterating effective rules inside Servo code, not filtered by Gecko's ServoStyleSet? If you are going to move media from Servo's Stylesheet to StylesheetSetEntry, what would you reference in ImportRule? Change it to StylesheetSetEntry as well presumably? > The problem with that is that it may make us clone too many stylesheets. See > the case exposed in comment 6. > > Following your suggestion, when loading the second (non-preloaded) sheet, > which has the media attribute, we'd need to call EnsureUniqueInner(), > cloning the whole stylesheet. I don't think that's acceptable IMO, and > fixing that may be equally (or more) non-trivial than my solution posted > above, isn't it? I'm not actually sure why we really need to clone in that case at the beginning... It sounds like eliminating that would be hard? If (a) is not going to be used anywhere, can we somehow drop the reference from (a) so that we don't need to clone parsed inner? FWIW, it may interact with Brad's work on bug 1368381 to some extent I guess...
Assignee: nobody → bwerth
Flags: needinfo?(bwerth)
Note that bz thinks that fixing this would make http://www.ardmediathek.de/tv start working.
Blocks: 1375927
Blocks: 1375983
Blocks: 1375929
Also think of those cases: (Darkspirit from bug 1375927 comment 5) > 2) http://www.niedersachsen.de (a german state) fixed with removing > <link rel="stylesheet" id="mobil" href="/assets/css/cssmobil.css" type="text/css" media="screen and (max-width: 768px)">
Blocks: 1376092
(In reply to Darkspirit from comment #14) > Also think of those cases: > > (Darkspirit from bug 1375927 comment 5) > > 2) http://www.niedersachsen.de (a german state) fixed with removing > > <link rel="stylesheet" id="mobil" href="/assets/css/cssmobil.css" type="text/css" media="screen and (max-width: 768px)"> Yeah, that's exactly the same bug, not necessarily just media="print". Brad, when you fix this, make sure to include too something similar to the test-case in bug 1376092, which tests dynamic changes to the media attribute of the <link>, and we seem to have really poor test coverage for that... :/ I can reduce it more if you want to, btw.
I talked with Brad today and I'm taking this one.
Assignee: bwerth → emilio+bugs
Flags: needinfo?(cam)
So this is what I have for now. It works fine and fixes all the dependent bugs, as expected. I still need to implement import rule cloning properly, but that's presumably not too hard, and I'll finish it up tomorrow.
Attachment #8882070 - Flags: review?(cam)
Comment on attachment 8882069 [details] style: split a stylesheet in Stylesheet and StylesheetContents. https://reviewboard.mozilla.org/r/153166/#review158792 ::: servo/components/style/gecko/data.rs:90 (Diff revision 2) > - self.stylist.rebuild(iter, > + self.stylist.rebuild( > + iter, > - &StylesheetGuards::same(guard), > + &StylesheetGuards::same(guard), > - /* ua_sheets = */ None, > + /* ua_sheets = */ None, > - /* stylesheets_changed = */ true, > + /* stylesheets_changed = */ true, > - author_style_disabled, > + author_style_disabled, > - &mut extra_data); > + &mut extra_data > + ); Pure reformatting changes should go in a separate patch. (Otherwise it takes me longer to review, looking to see what substantive change it's making!) ::: servo/components/style/stylesheets/stylesheet.rs:45 (Diff revision 2) > pub struct Namespaces { > pub default: Option<(Namespace, NamespaceId)>, > pub prefixes: FnvHashMap<Prefix, (Namespace, NamespaceId)>, > } > > -/// The structure servo uses to represent a stylesheet. > +/// The contents of a given stylesheet. This effectively maps to an s/an/a/ ::: servo/components/style/stylesheets/stylesheet.rs:46 (Diff revision 2) > pub default: Option<(Namespace, NamespaceId)>, > pub prefixes: FnvHashMap<Prefix, (Namespace, NamespaceId)>, > } > > -/// The structure servo uses to represent a stylesheet. > +/// The contents of a given stylesheet. This effectively maps to an > +/// StylesheetInner in Gecko. s/StylesheetInner/StyleSheetInner/ ::: servo/components/style/stylesheets/stylesheet.rs:90 (Diff revision 2) > + error_reporter, > + quirks_mode, > + line_number_offset, > + ); > + > + Self { I don't mind too much, but I wonder if we have a style guideline for whether to use Self or the struct name, when constructing objects like this? It looks like we don't constrct objects using Self very often under components/style. ::: servo/components/style/stylesheets/stylesheet.rs:197 (Diff revision 2) > + /// Get the stylesheet origin. > + fn origin(&self) -> Origin { > + self.contents().origin > + } > + > + /// Get the media associated with this stylesheet. Please update the comment to say that this takes a function that operates on the media, rather than gets it. ::: servo/ports/geckolib/glue.rs:979 (Diff revision 2) > - Stylesheet::as_arc(&sheet).malloc_size_of_children(&guard, malloc_size_of) > + Stylesheet::as_arc(&sheet) > + .contents.malloc_size_of_children(&guard, malloc_size_of) We should add a comment here, and somewhere near Stylesheet, that we don't currently measure any children of Stylesheet objects themselves. (Or, if you like, have a malloc_size_of_children that currently returns 0, so it's more obvious that we might add things to it and so we don't forget to add a call to it.) ::: servo/ports/geckolib/glue.rs:1039 (Diff revision 2) > - index as usize, > + index as usize, > - nested, > + nested, > - loader) { > + loader > + ); > + > + match result{ Nit: space before "{".
Attachment #8882069 - Flags: review?(cam) → review+
Comment on attachment 8882069 [details] style: split a stylesheet in Stylesheet and StylesheetContents. https://reviewboard.mozilla.org/r/153166/#review158826 ::: commit-message-c219b:1 (Diff revision 2) > +style: split a stylesheet in Stylesheet and StylesheetContents. r?heycam > + s/in/into/ Also it would be nice if the commit message had an explanation of why we are doing this.
Comment on attachment 8882070 [details] Bug 1372041: Refactor a bunch of stuff so link rel="media" is honored. https://reviewboard.mozilla.org/r/153168/#review158830 Looks good, but a few questions. ::: layout/style/Loader.h:295 (Diff revision 2) > * @param aServoChildSheet the child stylesheet of the @import rule, when > * using Servo's style system. Please remove this. ::: layout/style/ServoBindingList.h:65 (Diff revision 2) > SERVO_BINDING_FUNC(Servo_StyleSet_Drop, void, RawServoStyleSetOwned set) > SERVO_BINDING_FUNC(Servo_StyleSet_CompatModeChanged, void, > RawServoStyleSetBorrowed raw_data) > SERVO_BINDING_FUNC(Servo_StyleSet_AppendStyleSheet, void, > RawServoStyleSetBorrowed set, > - RawServoStyleSheetBorrowed sheet, > + const mozilla::ServoStyleSheet* gecko_sheet) It would have been better if we could have separate refactorings in separate patches. The change from uint64_t to ServoStyleSheet* seems like it's logically separable from the other changes. Also, can you remove the definition of the UniqueIDForSheet function now? ::: layout/style/ServoBindingList.h (Diff revision 2) > -SERVO_BINDING_FUNC(Servo_StyleSet_UpdateStyleSheet, void, > - RawServoStyleSetBorrowed set, > - RawServoStyleSheetBorrowed sheet, > - uint64_t unique_id) Why can we get rid of this and its callers? It's not obvious. ::: layout/style/ServoCSSRuleList.h:74 (Diff revision 2) > template<typename Func> > void EnumerateInstantiatedRules(Func aCallback); > > void DropAllRules(); > > - template<typename ChildSheetGetter> > + inline void ConstructImportRule(uint32_t aIndex); This shouldn't be inline any more. ::: layout/style/ServoCSSRuleList.cpp (Diff revision 2) > - // Only @charset can be put before @import rule, but @charset > - // rules don't have corresponding object, so if a rule is not > - // @import rule, there is definitely no @import rule after it. > - break; > - } > - ConstructImportRule(i, [&stylesheets](const RawServoStyleSheet* raw) { Why do we no longer need this? ::: layout/style/ServoCSSRuleList.cpp (Diff revision 2) > - // There is one case where we don't construct a child sheet for an import > - // rule: when the URL doesn't resolve. In that cases we still want to create > - // an import rule object, though we note it as an exceptional condition > - // in debug builds. > - NS_WARNING_ASSERTION(childSheet, > - "stylo: failed to get child sheet for @import rule"); How/where do we now handle this case? (Or was it already working?) ::: layout/style/ServoStyleSheet.h:45 (Diff revision 2) > > StyleSheetInfo* CloneFor(StyleSheet* aPrimarySheet) override; > > size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const; > > - RefPtr<const RawServoStyleSheet> mSheet; > + RefPtr<const RawServoStyleSheetContents> mContents; (I wonder if mSheetContents might be a better name? Not sure.) ::: layout/style/ServoStyleSheet.cpp:47 (Diff revision 2) > ServoStyleSheet* aPrimarySheet) > : StyleSheetInfo(aCopy, aPrimarySheet) > { > MOZ_COUNT_CTOR(ServoStyleSheetInner); > > // Actually clone aCopy's mSheet and use that as our mSheet. Please update this comment. ::: servo/components/style/gecko/data.rs:28 (Diff revision 2) > -use stylesheets::Origin; > +use stylesheets::{Origin, StylesheetContents, StylesheetInDocument}; > use stylist::{ExtraStyleData, Stylist}; > > +/// Little wrapper to a Gecko style sheet. > +#[derive(PartialEq, Clone, Eq)] > +pub struct GeckoStyleSheet(*const ServoStyleSheet); Funny looking at those two type names. :-) ::: servo/components/style/stylesheet_set.rs:14 (Diff revision 2) > /// Entry for a StylesheetSet. We don't bother creating a constructor, because > /// there's no sensible defaults for the member variables. > -pub struct StylesheetSetEntry { > - unique_id: u64, > - sheet: Arc<Stylesheet>, > +pub struct StylesheetSetEntry<S> > +where > + S: StylesheetInDocument + PartialEq + 'static, Can you comment here that we make this 'static because we know that the C++ ServoStyleSet will always hold a strong reference to sheets added here. ::: servo/components/style/stylesheets/stylesheet.rs:203 (Diff revision 2) > + fn quirks_mode(&self, guard: &SharedRwLockReadGuard) -> QuirksMode { > + self.contents(guard).quirks_mode > } > > /// Get the media associated with this stylesheet. > - fn media<F, R>(&self, f: F) -> R > + fn media<'a>(&'a self, guard: &'a SharedRwLockReadGuard) -> Option<&'a MediaList>; OK, since this changed back to returning the MediaList, you don't need to update the comment in the previous patch. :-)
Comment on attachment 8882070 [details] Bug 1372041: Refactor a bunch of stuff so link rel="media" is honored. https://reviewboard.mozilla.org/r/153168/#review158870 (Questions answered in person.)
Attachment #8882070 - Flags: review?(cam) → review+
Ok, last version of the patch is totally green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b7a4cf72b772a1bb7cf37889cd7c3598ff0adbc&selectedJob=111240436 I can try to split it tomorrow if you want to, though I'd rather land it as-is, because most of it is pretty much entangled.
Comment on attachment 8882475 [details] Bug 1372041: Refactor @import so it also respects the stylesheet's media properly. https://reviewboard.mozilla.org/r/153596/#review159038 Looks good, thanks! ::: layout/style/ServoBindings.cpp:2369 (Diff revision 3) > + !aParent->GetFirstChild() || > + aParent->GetFirstChild() == previousFirstChild) { Why do we do this check? Is it possible for the LoadChildSheet to return a failing nsresult but for it to still append the child sheet? Also, I think we can remove the `!aParent->GetFirstChild()` check since it should be covered by previousFirstChild being null. ::: layout/style/ServoImportRule.cpp:44 (Diff revision 3) > +NS_IMPL_ADDREF_INHERITED(ServoImportRule, dom::CSSImportRule) > +NS_IMPL_RELEASE_INHERITED(ServoImportRule, dom::CSSImportRule) > + > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(ServoImportRule, > + dom::CSSImportRule) > + // Note the child sheet twice, since it' Comment is cut off? ::: layout/style/ServoImportRule.cpp:47 (Diff revision 3) > + MOZ_ASSERT_IF(tmp->mRawRule, > + Servo_ImportRule_GetSheet(tmp->mRawRule) == tmp->mChildSheet); > + cb.NoteXPCOMChild(static_cast<nsIDOMCSSStyleSheet*>(tmp->mChildSheet)); > + NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mChildSheet"); Maybe use a different edge name, e.g. "mRawRule.stylesheet" instead of "mChildSheet"? ::: layout/style/ServoStyleSheet.h:77 (Diff revision 3) > + static already_AddRefed<ServoStyleSheet> CreateEmptyChildSheet( > + const ServoStyleSheet& aParent, > + already_AddRefed<dom::MediaList> aMediaList); Just make this a non-static method on ServoStyleSheet and drop the aParent argument. ::: layout/style/ServoStyleSheet.cpp:74 (Diff revision 3) > + if (!import) { > + break; > + } Please comment here about the fact that only @charset rules can occur before @import rules, so we @import rules must all be at the start of the rule list. ::: layout/style/StyleSheet.h:172 (Diff revision 3) > mOwnerRule = aOwnerRule; /* Not ref counted */ > } > dom::CSSImportRule* GetOwnerRule() const { return mOwnerRule; } > > void PrependStyleSheet(StyleSheet* aSheet); > + void PrependStyleSheetSilently(StyleSheet* aSheet); Please comment here what silently means. ::: servo/components/script/stylesheet_loader.rs:281 (Diff revision 3) > + if !url.is_in > + What's this? ::: servo/components/style/gecko/data.rs:44 (Diff revision 3) > impl GeckoStyleSheet { > /// Create a `GeckoStyleSheet` from a raw `ServoStyleSheet` pointer. > #[inline] > pub unsafe fn new(s: *const ServoStyleSheet) -> Self { > debug_assert!(!s.is_null()); > + unsafe { bindings::Gecko_StyleSheet_AddRef(s) }; I guess this unsafe block isn't needed since we're in an unsafe fn. ::: servo/components/style/gecko/data.rs:56 (Diff revision 3) > + pub unsafe fn from_addrefed(s: *const ServoStyleSheet) -> Self { > + debug_assert!(!s.is_null()); > GeckoStyleSheet(s) > } > > - fn raw(&self) -> &ServoStyleSheet { > + /// Get the raw servo stylesheet that we're wrapping. Nit: Maybe s/raw servo stylesheet/raw `ServoStyleSheet`/. ::: servo/components/style/shared_lock.rs:234 (Diff revision 3) > } > > +/// Parameters needed for deep clones. > +#[cfg(feature = "gecko")] > +pub struct DeepCloneParams { > + /// The new sheet originating from the sheet that hold us. Needed for Bit confused by this wording. Maybe "The new sheet we are cloning rules into."? ::: servo/components/style/stylesheets/import_rule.rs:17 (Diff revision 3) > use style_traits::ToCss; > -use stylearc::Arc; > +use stylesheets::{StylesheetContents, StylesheetInDocument}; > -use stylesheets::stylesheet::Stylesheet; > use values::specified::url::SpecifiedUrl; > > +/// A sheet that holds from an import rule. s/that holds/that is held/ ::: servo/components/style/stylesheets/import_rule.rs:42 (Diff revision 3) > + }; > + ImportSheet(unsafe { GeckoStyleSheet::from_addrefed(clone) }) > + } > +} > + > +/// A sheet that holds from an import rule. s/that holds/that is held/ ::: servo/components/style/stylesheets/import_rule.rs:66 (Diff revision 3) > + lock: &SharedRwLock, > + guard: &SharedRwLockReadGuard, > + params: &DeepCloneParams, Underscores on those argument identifiers? ::: servo/ports/geckolib/glue.rs:1080 (Diff revision 3) > + if index >= rules.0.len() { > + return Strong::null(); > + } Why do we need to check this and return null rather than panic?
Attachment #8882475 - Flags: review?(cam) → review+
Comment on attachment 8882731 [details] Bug 1372041: Update test expectations. https://reviewboard.mozilla.org/r/153822/#review159040
Attachment #8882731 - Flags: review?(cam) → review+
Comment on attachment 8882733 [details] Bug 1372041: Test for 1376092, while we're here. https://reviewboard.mozilla.org/r/153826/#review159044
Attachment #8882733 - Flags: review?(cam) → review+
Comment on attachment 8882070 [details] Bug 1372041: Refactor a bunch of stuff so link rel="media" is honored. https://reviewboard.mozilla.org/r/153168/#review158830 > Can you comment here that we make this 'static because we know that the C++ ServoStyleSet will always hold a strong reference to sheets added here. Well, the `'static` doesn't mean that really (that'd be something like `&'static S`. The `S: 'static` simply forces the type to not have a lifetime (in rust types have lifetimes, and it's somewhat confusing), and allows to simplify the where clauses.
Comment on attachment 8882070 [details] Bug 1372041: Refactor a bunch of stuff so link rel="media" is honored. https://reviewboard.mozilla.org/r/153168/#review158830 > This shouldn't be inline any more. I removed this altogether.
Comment on attachment 8882475 [details] Bug 1372041: Refactor @import so it also respects the stylesheet's media properly. https://reviewboard.mozilla.org/r/153596/#review159038 > Why do we do this check? Is it possible for the LoadChildSheet to return a failing nsresult but for it to still append the child sheet? > > Also, I think we can remove the `!aParent->GetFirstChild()` check since it should be covered by previousFirstChild being null. No, but it is possible for it to return a successful nsresult and _not_ append the child sheet, like for recursive imports. Good catch on the useless check :) > Just make this a non-static method on ServoStyleSheet and drop the aParent argument. Good call :) > What's this? Err, forgot to finishing fixing up the servo loader, will do.
Attachment #8882069 - Attachment is obsolete: true
Comment on attachment 8882475 [details] Bug 1372041: Refactor @import so it also respects the stylesheet's media properly. https://reviewboard.mozilla.org/r/153596/#review159038 > Why do we need to check this and return null rather than panic? Because when we iterate over import rules, there may be the case we're iterating over an empty stylesheet, in which case we'd access an out-of-boud index (or, panic).
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0fb6eef0222c Refactor a bunch of stuff so link rel="media" is honored. r=heycam https://hg.mozilla.org/integration/autoland/rev/91914706f61a Refactor @import so it also respects the stylesheet's media properly. r=heycam https://hg.mozilla.org/integration/autoland/rev/0fd275a484c1 Update test expectations. r=heycam https://hg.mozilla.org/integration/autoland/rev/f3f64df05c9b Test. r=heycam https://hg.mozilla.org/integration/autoland/rev/bd5f97b7e78c Test for 1376092, while we're here. r=heycam
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e6419d86fd49 Further update test expectations. r=heycam https://hg.mozilla.org/integration/autoland/rev/afa14374f151 Assert we're on the main thread when addreffing/releasing stylesheets. r=me
No longer blocks: 1376750
No longer blocks: 1376743
No longer blocks: 1376742
No longer blocks: 1375983
No longer blocks: 1375929
No longer blocks: 1376092
Nightly 56 x64 20170703100343 @ Debian Testing (Linux 4.9.0-3-amd64, Radeon RX480) about:support > Stylo: true (enabled by user) URL of this bug: https://www.meetup.com/Rust-Bay-Area/ is the same as without stylo. All duplicates are verified as fixed. Nice work! Thank you!
No longer blocks: 1375927
Status: RESOLVED → VERIFIED
Has STR: --- → yes
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
Version: unspecified → 56 Branch
(In reply to Darkspirit from comment #62) > Nightly 56 x64 20170703100343 @ Debian Testing (Linux 4.9.0-3-amd64, Radeon > RX480) > about:support > Stylo: true (enabled by user) > > URL of this bug: https://www.meetup.com/Rust-Bay-Area/ is the same as > without stylo. > > All duplicates are verified as fixed. Nice work! Thank you! Yay!
Depends on: 1385193
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: