Bug 1325878: Don't use nsMediaList for loading imports. r?xidorn draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 10 Apr 2017 11:17:51 +0800
changeset 561040 bf60fc0d3eede29edf080c383a057a5eb1af170e
parent 561039 6759d82023fbcb86e5cbe70ca2e0d43c06ad5a72
child 561041 58835e9c2d6de1041c6dfaf86e75379e3557c4f5
push id53604
push userbmo:emilio+bugs@crisal.io
push dateWed, 12 Apr 2017 05:05:04 +0000
reviewersxidorn
bugs1325878
milestone55.0a1
Bug 1325878: Don't use nsMediaList for loading imports. r?xidorn MozReview-Commit-ID: HR23bqZcmcA
layout/style/ServoBindings.cpp
layout/style/ServoBindings.h
servo/components/script/stylesheet_loader.rs
servo/components/style/stylesheets.rs
servo/ports/geckolib/stylesheet_loader.rs
--- a/layout/style/ServoBindings.cpp
+++ b/layout/style/ServoBindings.cpp
@@ -33,26 +33,27 @@
 #include "nsNameSpaceManager.h"
 #include "nsNetUtil.h"
 #include "nsRuleNode.h"
 #include "nsString.h"
 #include "nsStyleStruct.h"
 #include "nsStyleUtil.h"
 #include "nsTArray.h"
 
+#include "mozilla/DeclarationBlockInlines.h"
 #include "mozilla/EffectCompositor.h"
 #include "mozilla/EffectSet.h"
 #include "mozilla/EventStates.h"
 #include "mozilla/Keyframe.h"
 #include "mozilla/Mutex.h"
 #include "mozilla/ServoElementSnapshot.h"
 #include "mozilla/ServoRestyleManager.h"
 #include "mozilla/StyleAnimationValue.h"
 #include "mozilla/SystemGroup.h"
-#include "mozilla/DeclarationBlockInlines.h"
+#include "mozilla/ServoMediaList.h"
 #include "mozilla/dom/Element.h"
 #include "mozilla/dom/ElementInlines.h"
 #include "mozilla/dom/HTMLTableCellElement.h"
 #include "mozilla/dom/HTMLBodyElement.h"
 #include "mozilla/LookAndFeel.h"
 #include "mozilla/URLExtraData.h"
 
 using namespace mozilla;
@@ -1682,35 +1683,24 @@ Gecko_GetFontMetrics(RawGeckoPresContext
 
 void
 Gecko_LoadStyleSheet(css::Loader* aLoader,
                      ServoStyleSheet* aParent,
                      RawServoStyleSheetBorrowed aChildSheet,
                      RawGeckoURLExtraData* aBaseURLData,
                      const uint8_t* aURLString,
                      uint32_t aURLStringLength,
-                     const uint8_t* aMediaString,
-                     uint32_t aMediaStringLength)
+                     RawServoMediaListStrong aMediaList)
 {
   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(aBaseURLData, "Need base URL data");
-  RefPtr<nsMediaList> media = new nsMediaList();
-  if (aMediaStringLength) {
-    MOZ_ASSERT(aMediaString);
-    // TODO(emilio, bug 1325878): This is not great, though this is going away
-    // soon anyway, when we can have a Servo-backed nsMediaList.
-    nsDependentCSubstring medium(reinterpret_cast<const char*>(aMediaString),
-                                 aMediaStringLength);
-    nsCSSParser mediumParser(aLoader);
-    mediumParser.ParseMediaList(
-        NS_ConvertUTF8toUTF16(medium), nullptr, 0, media);
-  }
+  RefPtr<dom::MediaList> media = new ServoMediaList(aMediaList.Consume());
 
   nsDependentCSubstring urlSpec(reinterpret_cast<const char*>(aURLString),
                                 aURLStringLength);
   nsCOMPtr<nsIURI> uri;
   nsresult rv = NS_NewURI(getter_AddRefs(uri), urlSpec, nullptr,
                           aBaseURLData->BaseURI());
 
   if (NS_FAILED(rv)) {
--- a/layout/style/ServoBindings.h
+++ b/layout/style/ServoBindings.h
@@ -126,18 +126,17 @@ RawGeckoElementBorrowedOrNull Gecko_GetP
 RawGeckoElementBorrowedOrNull Gecko_GetNextSiblingElement(RawGeckoElementBorrowed element);
 RawGeckoElementBorrowedOrNull Gecko_GetDocumentElement(RawGeckoDocumentBorrowed document);
 void Gecko_LoadStyleSheet(mozilla::css::Loader* loader,
                           mozilla::ServoStyleSheet* parent,
                           RawServoStyleSheetBorrowed child_sheet,
                           RawGeckoURLExtraData* base_url_data,
                           const uint8_t* url_bytes,
                           uint32_t url_length,
-                          const uint8_t* media_bytes,
-                          uint32_t media_length);
+                          RawServoMediaListStrong media_list);
 
 // By default, Servo walks the DOM by traversing the siblings of the DOM-view
 // first child. This generally works, but misses anonymous children, which we
 // want to traverse during styling. To support these cases, we create an
 // optional heap-allocated iterator for nodes that need it. If the creation
 // method returns null, Servo falls back to the aforementioned simpler (and
 // faster) sibling traversal.
 StyleChildrenIteratorOwnedOrNull Gecko_MaybeCreateStyleChildrenIterator(RawGeckoNodeBorrowed node);
--- a/servo/components/script/stylesheet_loader.rs
+++ b/servo/components/script/stylesheet_loader.rs
@@ -266,23 +266,23 @@ impl<'a> StylesheetLoader<'a> {
 
         document.fetch_async(LoadType::Stylesheet(url), request, action_sender);
     }
 }
 
 impl<'a> StyleStylesheetLoader for StylesheetLoader<'a> {
     fn request_stylesheet(
         &self,
-        media: MediaList,
-        make_import: &mut FnMut(MediaList) -> ImportRule,
+        media: Arc<Locked<MediaList>>,
+        make_import: &mut FnMut(Arc<Locked<MediaList>>) -> ImportRule,
         make_arc: &mut FnMut(ImportRule) -> Arc<StyleLocked<ImportRule>>,
     ) -> Arc<StyleLocked<ImportRule>> {
         let import = make_import(media);
         let url = import.url.url().expect("Invalid urls shouldn't enter the loader").clone();
 
-        //TODO (mrnayak) : Whether we should use the original loader's CORS setting?
-        //Fix this when spec has more details.
+        // TODO (mrnayak) : Whether we should use the original loader's CORS
+        // setting? Fix this when spec has more details.
         let source = StylesheetContextSource::Import(import.stylesheet.clone());
         self.load(source, url, None, "".to_owned());
 
         make_arc(import)
     }
 }
--- a/servo/components/style/stylesheets.rs
+++ b/servo/components/style/stylesheets.rs
@@ -819,29 +819,29 @@ pub trait StylesheetLoader {
     ///
     /// The called code is responsible to update the `stylesheet` rules field
     /// when the sheet is done loading.
     ///
     /// The convoluted signature allows impls to look at MediaList and ImportRule
     /// before they’re locked, while keeping the trait object-safe.
     fn request_stylesheet(
         &self,
-        media: MediaList,
-        make_import: &mut FnMut(MediaList) -> ImportRule,
+        media: Arc<Locked<MediaList>>,
+        make_import: &mut FnMut(Arc<Locked<MediaList>>) -> ImportRule,
         make_arc: &mut FnMut(ImportRule) -> Arc<Locked<ImportRule>>,
     ) -> Arc<Locked<ImportRule>>;
 }
 
 struct NoOpLoader;
 
 impl StylesheetLoader for NoOpLoader {
     fn request_stylesheet(
         &self,
-        media: MediaList,
-        make_import: &mut FnMut(MediaList) -> ImportRule,
+        media: Arc<Locked<MediaList>>,
+        make_import: &mut FnMut(Arc<Locked<MediaList>>) -> ImportRule,
         make_arc: &mut FnMut(ImportRule) -> Arc<Locked<ImportRule>>,
     ) -> Arc<Locked<ImportRule>> {
         make_arc(make_import(media))
     }
 }
 
 
 struct TopLevelRuleParser<'a> {
@@ -900,31 +900,32 @@ impl<'a> AtRuleParser for TopLevelRulePa
         match_ignore_ascii_case! { name,
             "import" => {
                 if self.state.get() <= State::Imports {
                     self.state.set(State::Imports);
                     let url_string = input.expect_url_or_string()?;
                     let specified_url = SpecifiedUrl::parse_from_string(url_string, &self.context)?;
 
                     let media = parse_media_query_list(input);
+                    let media = Arc::new(self.shared_lock.wrap(media));
 
                     let noop_loader = NoOpLoader;
                     let loader = if !specified_url.is_invalid() {
                         self.loader.expect("Expected a stylesheet loader for @import")
                     } else {
                         &noop_loader
                     };
 
                     let mut specified_url = Some(specified_url);
                     let arc = loader.request_stylesheet(media, &mut |media| {
                         ImportRule {
                             url: specified_url.take().unwrap(),
                             stylesheet: Arc::new(Stylesheet {
                                 rules: CssRules::new(Vec::new(), self.shared_lock),
-                                media: Arc::new(self.shared_lock.wrap(media)),
+                                media: media,
                                 shared_lock: self.shared_lock.clone(),
                                 origin: self.context.stylesheet_origin,
                                 url_data: self.context.url_data.clone(),
                                 namespaces: RwLock::new(Namespaces::default()),
                                 dirty_on_viewport_size_change: AtomicBool::new(false),
                                 disabled: AtomicBool::new(false),
                             })
                         }
--- a/servo/ports/geckolib/stylesheet_loader.rs
+++ b/servo/ports/geckolib/stylesheet_loader.rs
@@ -1,60 +1,48 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use std::sync::Arc;
 use style::gecko_bindings::bindings::Gecko_LoadStyleSheet;
 use style::gecko_bindings::structs::{Loader, ServoStyleSheet};
-use style::gecko_bindings::sugar::ownership::HasArcFFI;
+use style::gecko_bindings::sugar::ownership::{HasArcFFI, FFIArcHelpers};
 use style::media_queries::MediaList;
 use style::shared_lock::Locked;
 use style::stylesheets::{ImportRule, Stylesheet, StylesheetLoader as StyleStylesheetLoader};
-use style_traits::ToCss;
 
 pub struct StylesheetLoader(*mut Loader, *mut ServoStyleSheet);
 
 impl StylesheetLoader {
     pub fn new(loader: *mut Loader, parent: *mut ServoStyleSheet) -> Self {
         StylesheetLoader(loader, parent)
     }
 }
 
 impl StyleStylesheetLoader for StylesheetLoader {
     fn request_stylesheet(
         &self,
-        media: MediaList,
-        make_import: &mut FnMut(MediaList) -> ImportRule,
+        media: Arc<Locked<MediaList>>,
+        make_import: &mut FnMut(Arc<Locked<MediaList>>) -> ImportRule,
         make_arc: &mut FnMut(ImportRule) -> Arc<Locked<ImportRule>>,
     ) -> Arc<Locked<ImportRule>> {
-        // TODO(emilio): We probably want to share media representation with
-        // Gecko in Stylo.
-        //
-        // This also allows us to get rid of a bunch of extra work to evaluate
-        // and ensure parity, and shouldn't be much Gecko work given we always
-        // evaluate them on the main thread.
-        //
-        // Meanwhile, this works.
-        let media_string = media.to_css_string();
-
-        let import = make_import(media);
+        let import = make_import(media.clone());
 
         // After we get this raw pointer ImportRule will be moved into a lock and Arc
         // and so the Arc<Url> pointer inside will also move,
         // but the Url it points to or the allocating backing the String inside that Url won’t,
         // so this raw pointer will still be valid.
         let (spec_bytes, spec_len): (*const u8, usize) = import.url.as_slice_components();
 
         let base_url_data = import.url.extra_data.get();
         unsafe {
             Gecko_LoadStyleSheet(self.0,
                                  self.1,
                                  Stylesheet::arc_as_borrowed(&import.stylesheet),
                                  base_url_data,
                                  spec_bytes,
                                  spec_len as u32,
-                                 media_string.as_bytes().as_ptr(),
-                                 media_string.len() as u32);
+                                 media.into_strong())
         }
         make_arc(import)
     }
 }