servo: Merge #15751 - first stab. added ServoUrl as a parameter to report_error(...) of Par… (from avinash-vijayaraghavan:testing-csserror); r=cbrewster
authoravinash <avinash@avinashs-MacBook-Pro.local>
Mon, 06 Mar 2017 07:11:51 -0800
changeset 346087 b36f1d98974f50ee6176cda988bc97dd25f94b1d
parent 346086 5823fd77c13f7413ed58e444097d7c16f53cc2a4
child 346088 940c4595cfeb766e3b25e8f6a0d8a9eaacac5aec
push id31459
push usercbook@mozilla.com
push dateTue, 07 Mar 2017 14:05:14 +0000
treeherdermozilla-central@1fb56ba248d5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscbrewster
milestone54.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #15751 - first stab. added ServoUrl as a parameter to report_error(...) of Par… (from avinash-vijayaraghavan:testing-csserror); r=cbrewster @jdm @SimonSapin <!-- Please describe your changes on the following line: --> 1. Added ServoUrl as a parameter to report_error(...) of ParseErrorReporter trait. 2. I am not sure how to handle the case of impl ParseErrorReporter for CSSErrorReporter and MemoryHoleReporter, so have not made any changes (other than adding ServoUrl arg) to report_error implementations for these. 3. In StdoutErrorReporter i have added the ServoUrl arg to the info! function, 4. I would like to know if i am on the correct path and clarify what else needs to be done. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ X] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #15708 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because wanted to clarify before writing tests <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: 0dbee36915abd926e61ca8571e11abf1f97ec830
servo/components/script_layout_interface/reporter.rs
servo/components/style/error_reporting.rs
servo/components/style/parser.rs
servo/components/style/stylesheets.rs
servo/tests/unit/style/media_queries.rs
servo/tests/unit/style/rule_tree/bench.rs
servo/tests/unit/style/stylesheets.rs
--- a/servo/components/script_layout_interface/reporter.rs
+++ b/servo/components/script_layout_interface/reporter.rs
@@ -2,35 +2,38 @@
  * 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 cssparser::{Parser, SourcePosition};
 use ipc_channel::ipc::IpcSender;
 use log;
 use msg::constellation_msg::PipelineId;
 use script_traits::ConstellationControlMsg;
+use servo_url::ServoUrl;
 use std::sync::{Mutex, Arc};
 use style::error_reporting::ParseErrorReporter;
 
 #[derive(HeapSizeOf)]
 pub struct CSSErrorReporter {
     pub pipelineid: PipelineId,
     // Arc+Mutex combo is necessary to make this struct Sync,
     // which is necessary to fulfill the bounds required by the
     // uses of the ParseErrorReporter trait.
     #[ignore_heap_size_of = "Arc is defined in libstd"]
     pub script_chan: Arc<Mutex<IpcSender<ConstellationControlMsg>>>,
 }
 
 impl ParseErrorReporter for CSSErrorReporter {
-     fn report_error(&self, input: &mut Parser, position: SourcePosition, message: &str) {
-         let location = input.source_location(position);
-         if log_enabled!(log::LogLevel::Info) {
-             info!("{}:{} {}", location.line, location.column, message)
-         }
+     fn report_error(&self, input: &mut Parser, position: SourcePosition, message: &str,
+        url: &ServoUrl) {
+        let location = input.source_location(position);
+        if log_enabled!(log::LogLevel::Info) {
+             info!("Url:\t{}\n{}:{} {}", url.as_str(), location.line, location.column, message)
+        }
+
          //TODO: report a real filename
          let _ = self.script_chan.lock().unwrap().send(
              ConstellationControlMsg::ReportCSSError(self.pipelineid,
                                                      "".to_owned(),
                                                      location.line,
                                                      location.column,
                                                      message.to_owned()));
      }
--- a/servo/components/style/error_reporting.rs
+++ b/servo/components/style/error_reporting.rs
@@ -3,38 +3,40 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 //! Types used to report parsing errors.
 
 #![deny(missing_docs)]
 
 use cssparser::{Parser, SourcePosition};
 use log;
+use servo_url::ServoUrl;
 
 /// A generic trait for an error reporter.
 pub trait ParseErrorReporter {
     /// Called the style engine detects an error.
     ///
     /// Returns the current input being parsed, the source position it was
     /// reported from, and a message.
-    fn report_error(&self, input: &mut Parser, position: SourcePosition, message: &str);
+    fn report_error(&self, input: &mut Parser, position: SourcePosition, message: &str, url: &ServoUrl);
     /// Clone this error reporter.
     ///
     /// TODO(emilio): I'm pretty sure all the box shenanigans can go away.
     fn clone(&self) -> Box<ParseErrorReporter + Send + Sync>;
 }
 
 /// An error reporter that reports the errors to the `info` log channel.
 ///
 /// TODO(emilio): The name of this reporter is a lie, and should be renamed!
 pub struct StdoutErrorReporter;
 impl ParseErrorReporter for StdoutErrorReporter {
-    fn report_error(&self, input: &mut Parser, position: SourcePosition, message: &str) {
-         if log_enabled!(log::LogLevel::Info) {
-             let location = input.source_location(position);
-             info!("{}:{} {}", location.line, location.column, message)
-         }
+    fn report_error(&self, input: &mut Parser, position: SourcePosition, message: &str,
+        url: &ServoUrl) {
+        if log_enabled!(log::LogLevel::Info) {
+            let location = input.source_location(position);
+            info!("Url:\t{}\n{}:{} {}", url.as_str(), location.line, location.column, message)
+        }
     }
 
     fn clone(&self) -> Box<ParseErrorReporter + Send + Sync> {
         Box::new(StdoutErrorReporter)
     }
 }
--- a/servo/components/style/parser.rs
+++ b/servo/components/style/parser.rs
@@ -85,17 +85,18 @@ impl<'a> ParserContext<'a> {
         Self::new(Origin::User, base_url, Box::new(MemoryHoleReporter))
     }
 }
 
 /// Defaults to a no-op.
 /// Set a `RUST_LOG=style::errors` environment variable
 /// to log CSS parse errors to stderr.
 pub fn log_css_error(input: &mut Parser, position: SourcePosition, message: &str, parsercontext: &ParserContext) {
-    parsercontext.error_reporter.report_error(input, position, message);
+    let servo_url = parsercontext.base_url;
+    parsercontext.error_reporter.report_error(input, position, message, servo_url);
 }
 
 // XXXManishearth Replace all specified value parse impls with impls of this
 // trait. This will make it easy to write more generic values in the future.
 /// A trait to abstract parsing of a specified value given a `ParserContext` and
 /// CSS input.
 pub trait Parse : Sized {
     /// Parse a value of this type.
--- a/servo/components/style/stylesheets.rs
+++ b/servo/components/style/stylesheets.rs
@@ -247,17 +247,18 @@ pub enum CssRuleType {
 
 /// Error reporter which silently forgets errors
 pub struct MemoryHoleReporter;
 
 impl ParseErrorReporter for MemoryHoleReporter {
     fn report_error(&self,
             _: &mut Parser,
             _: SourcePosition,
-            _: &str) {
+            _: &str,
+            _: &ServoUrl) {
         // do nothing
     }
     fn clone(&self) -> Box<ParseErrorReporter + Send + Sync> {
         Box::new(MemoryHoleReporter)
     }
 }
 
 #[allow(missing_docs)]
--- a/servo/tests/unit/style/media_queries.rs
+++ b/servo/tests/unit/style/media_queries.rs
@@ -13,18 +13,20 @@ use style::parser::ParserContextExtraDat
 use style::servo::media_queries::*;
 use style::stylesheets::{Stylesheet, Origin, CssRule};
 use style::values::specified;
 use style_traits::ToCss;
 
 pub struct CSSErrorReporterTest;
 
 impl ParseErrorReporter for CSSErrorReporterTest {
-     fn report_error(&self, _input: &mut Parser, _position: SourcePosition, _message: &str) {
-     }
+    fn report_error(&self, _input: &mut Parser, _position: SourcePosition, _message: &str,
+        _url: &ServoUrl) {
+    }
+
      fn clone(&self) -> Box<ParseErrorReporter + Send + Sync> {
         Box::new(CSSErrorReporterTest)
      }
 }
 
 fn test_media_rule<F>(css: &str, callback: F)
     where F: Fn(&MediaList, &str),
 {
--- a/servo/tests/unit/style/rule_tree/bench.rs
+++ b/servo/tests/unit/style/rule_tree/bench.rs
@@ -12,18 +12,19 @@ use style::media_queries::MediaList;
 use style::parser::ParserContextExtraData;
 use style::properties::{longhands, DeclaredValue, Importance, PropertyDeclaration, PropertyDeclarationBlock};
 use style::rule_tree::{CascadeLevel, RuleTree, StrongRuleNode, StyleSource};
 use style::stylesheets::{Origin, Stylesheet, CssRule};
 use test::{self, Bencher};
 
 struct ErrorringErrorReporter;
 impl ParseErrorReporter for ErrorringErrorReporter {
-    fn report_error(&self, _: &mut Parser, position: SourcePosition, message: &str) {
-        panic!("CSS error: {:?} {}", position, message);
+    fn report_error(&self, input: &mut Parser, position: SourcePosition, message: &str,
+        url: &ServoUrl) {
+        panic!("CSS error: {}\t\n{:?} {}", url.as_str(), position, message);
     }
 
     fn clone(&self) -> Box<ParseErrorReporter + Send + Sync> {
         Box::new(ErrorringErrorReporter)
     }
 }
 
 struct AutoGCRuleTree<'a>(&'a RuleTree);
--- a/servo/tests/unit/style/stylesheets.rs
+++ b/servo/tests/unit/style/stylesheets.rs
@@ -271,16 +271,17 @@ fn test_parse_stylesheet() {
 
         ]),
     };
 
     assert_eq!(format!("{:#?}", stylesheet), format!("{:#?}", expected));
 }
 
 struct CSSError {
+    pub url : ServoUrl,
     pub line: usize,
     pub column: usize,
     pub message: String
 }
 
 struct CSSInvalidErrorReporterTest {
     pub errors: Arc<Mutex<Vec<CSSError>>>
 }
@@ -289,24 +290,27 @@ impl CSSInvalidErrorReporterTest {
     pub fn new() -> CSSInvalidErrorReporterTest {
         return CSSInvalidErrorReporterTest{
             errors: Arc::new(Mutex::new(Vec::new()))
         }
     }
 }
 
 impl ParseErrorReporter for CSSInvalidErrorReporterTest {
-    fn report_error(&self, input: &mut CssParser, position: SourcePosition, message: &str) {
+    fn report_error(&self, input: &mut CssParser, position: SourcePosition, message: &str,
+        url: &ServoUrl) {
+
         let location = input.source_location(position);
 
         let errors = self.errors.clone();
         let mut errors = errors.lock().unwrap();
 
         errors.push(
             CSSError{
+                url: url.clone(),
                 line: location.line,
                 column: location.column,
                 message: message.to_owned()
             }
         );
     }
 
     fn clone(&self) -> Box<ParseErrorReporter + Send + Sync> {
@@ -328,25 +332,28 @@ fn test_report_error_stylesheet() {
         invalid: true;
     }
     ";
     let url = ServoUrl::parse("about::test").unwrap();
     let error_reporter = Box::new(CSSInvalidErrorReporterTest::new());
 
     let errors = error_reporter.errors.clone();
 
-    Stylesheet::from_str(css, url, Origin::UserAgent, Default::default(),
+    Stylesheet::from_str(css, url.clone(), Origin::UserAgent, Default::default(),
                          None,
                          error_reporter,
                          ParserContextExtraData::default());
 
     let mut errors = errors.lock().unwrap();
 
     let error = errors.pop().unwrap();
     assert_eq!("Unsupported property declaration: 'invalid: true;'", error.message);
     assert_eq!(5, error.line);
     assert_eq!(9, error.column);
 
     let error = errors.pop().unwrap();
     assert_eq!("Unsupported property declaration: 'display: invalid;'", error.message);
     assert_eq!(4, error.line);
     assert_eq!(9, error.column);
+
+    // testing for the url
+    assert_eq!(url, error.url);
 }