Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement performance interface extension for PerformanceResourceTiming #22431

Closed
wants to merge 4 commits into from
Closed
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

patch-1 restored

  • Loading branch information
aditj committed Dec 30, 2018
commit 195e29d5f37f1a1c50f6b116daa7b35af349d777
@@ -71,6 +71,7 @@ reftest-wait
rejectionhandled
reset
resize
resourcetimingbufferfull
right
rtl
sans-serif
@@ -13,7 +13,6 @@ use crate::dom::bindings::inheritance::Castable;
use crate::dom::bindings::num::Finite;
use crate::dom::bindings::reflector::{reflect_dom_object, DomObject};
use crate::dom::bindings::root::DomRoot;
use crate::dom::event::Event;
use crate::dom::bindings::str::DOMString;
use crate::dom::eventtarget::EventTarget;
use crate::dom::globalscope::GlobalScope;
@@ -27,8 +26,7 @@ use dom_struct::dom_struct;
use metrics::ToMs;
use std::cell::Cell;
use std::cmp::Ordering;
use std::collections::VecDeque;
use std::borrow::BorrowMut;
use std::collections::VecDeque;

const INVALID_ENTRY_NAMES: &'static [&'static str] = &[
"navigationStart",
@@ -63,7 +61,7 @@ pub struct PerformanceEntryList {

impl PerformanceEntryList {
pub fn new(entries: DOMPerformanceEntryList) -> Self {
PerformanceEntryList { entries }
PerformanceEntryList { entries }
}

pub fn get_entries_by_name_and_type(
@@ -144,8 +142,6 @@ pub struct Performance {
resource_timing_buffer_current_size: Cell<usize>,
resource_timing_buffer_pending_full_event: Cell<bool>,
resource_timing_secondary_entries: DomRefCell<VecDeque<DomRoot<PerformanceEntry>>>,


}

impl Performance {
@@ -159,8 +155,7 @@ impl Performance {
resource_timing_buffer_size_limit: Cell::new(250),
resource_timing_buffer_current_size: Cell::new(0),
resource_timing_buffer_pending_full_event: Cell::new(false),
resource_timing_secondary_entries:DomRefCell::new(VecDeque::new()),

resource_timing_secondary_entries: DomRefCell::new(VecDeque::new()),
}
}

@@ -220,7 +215,12 @@ impl Performance {
///
/// Algorithm spec:
/// <https://w3c.github.io/performance-timeline/#queue-a-performanceentry>
/// Also this algorithm has been extented according to :
/// <https://w3c.github.io/resource-timing/#sec-extensions-performance-interface>
pub fn queue_entry(&self, entry: &PerformanceEntry, add_to_performance_entries_buffer: bool) {
if entry.entry_type() == "resource" && !self.should_queue_resource_entry(entry) {
return;
}
// Steps 1-3.
// Add the performance entry to the list of performance entries that have not
// been notified to each performance observer owner, filtering the ones it's
@@ -234,31 +234,21 @@ impl Performance {
o.observer.queue_entry(entry);
}


// Step 4.
// If the "add to performance entry buffer flag" is set, add the
// new entry to the buffer.
if add_to_performance_entries_buffer && self.can_add_resource_timing_entry() && self.resource_timing_buffer_pending_full_event.get() == false{
if add_to_performance_entries_buffer {
self.entries
.borrow_mut()
.entries
.push(DomRoot::from_ref(entry));
let mut resource_timing_buffer_current_size =self.resource_timing_buffer_current_size.get();
self.resource_timing_buffer_current_size.set(resource_timing_buffer_current_size+1);
return;
}
else if self.resource_timing_buffer_pending_full_event.get() == false{
self.resource_timing_buffer_pending_full_event.set(true);
self.fire_buffer_full_event();
self.resource_timing_secondary_entries.borrow_mut()
.push_back(DomRoot::from_ref(entry));
return;
}

// Step 5.
// If there is already a queued notification task, we just bail out.
if self.pending_notification_observers_task.get() {
return;
}
}
// Step 6.
// Queue a new notification task.
self.pending_notification_observers_task.set(true);
@@ -288,7 +278,6 @@ impl Performance {
o.observer.callback(),
o.observer.entries(),
)

})
.collect();

@@ -301,27 +290,65 @@ impl Performance {
(time::precise_time_ns() - self.navigation_start_precise).to_ms()
}
fn can_add_resource_timing_entry(&self) -> bool {
if self.resource_timing_buffer_current_size<=self.resource_timing_buffer_size_limit{
return true;
}
else {
return false;
}
}
fn copy_secondary_resource_timing_buffer(&self) {
while self.can_add_resource_timing_entry() && self.resource_timing_secondary_entries.borrow_mut().len()>0 {
self.queue_entry(self.resource_timing_secondary_entries.borrow_mut().pop_front().unwrap().deref(),true); //not meant to build
}
}
fn fire_buffer_full_event(&self) {
while self.resource_timing_secondary_entries.borrow_mut().len() > 0 {
if self.can_add_resource_timing_entry()==false{
let resourcetimingbufferfull = Event::new_inherited();
resourcetimingbufferfull.fire(&(self.eventtarget));
}
}
self.resource_timing_buffer_pending_full_event.set(false);
}
self.resource_timing_buffer_current_size.get() <=
self.resource_timing_buffer_size_limit.get()
}
fn copy_secondary_resource_timing_buffer(&self) {
while self.can_add_resource_timing_entry() {
let entry = self
.resource_timing_secondary_entries
.borrow_mut()
.pop_front();
if let Some(ref entry) = entry {
self.queue_entry(entry, true);
} else {
break;
}
}
This conversation was marked as resolved by KiChjang

This comment has been minimized.

@ferjm

ferjm Dec 21, 2018

Member

A different but safer way (no unwraps) of doing the same thing:

loop {
    if self.can_add_resource_timing_entry() {
        break;
    }
    let entry = self
        .resource_timing_secondary_entries
        .borrow_mut()
        .pop_front();
    let Some(ref entry) = entry {
        self.queue_entry(entry, true);
        continue;
    }
    break;
}

This comment has been minimized.

@aditj

aditj Dec 21, 2018

Author Contributor

I think we still need to unwrap as entry is an option and the compiler is giving the error

self.queue_entry(entry, true);                            
    |                                  ^^^^^ expected reference, found enum `std::option::Option`

This comment has been minimized.

@KiChjang

KiChjang Dec 22, 2018

Member

There's a typo in @ferjm's code: it should be if let Some(ref entry) = entry {.

This comment has been minimized.

@KiChjang

KiChjang Dec 22, 2018

Member

Also, I think he really meant this:

loop {
    if !self.can_add_resource_timing_entry() {
        break;
    }
    let entry = self
        .resource_timing_secondary_entries
        .borrow_mut()
        .pop_front();
    if let Some(ref entry) = entry {
        self.queue_entry(entry, true);
    } else {
        break;
    }
}

I'd probably collapse the loop and use while like so:

while self.can_add_resource_timing_entry() {
    let entry = self
        .resource_timing_secondary_entries
        .borrow_mut()
        .pop_front();
    if let Some(ref entry) = entry {
        self.queue_entry(entry, true);
    } else {
        break;
    }
}

This comment has been minimized.

@aditj

aditj Dec 22, 2018

Author Contributor

That makes sense.

}
// `fire a buffer full event` paragraph of
// https://w3c.github.io/resource-timing/#sec-extensions-performance-interface
fn fire_buffer_full_event(&self) {
This conversation was marked as resolved by ferjm

This comment has been minimized.

@ferjm

ferjm Dec 21, 2018

Member

nit: one thing that helps reading the code and comparing it with the spec is adding the link to the specified algorithm above the function definition and a comment identifying the different steps. For example:

// `fire a buffer full event` paragraph of
// https://w3c.github.io/resource-timing/#sec-extensions-performance-interface
fn fire_buffer_full_event(&self) {
    // Step 1.
            while !self
            .resource_timing_secondary_entries
            .borrow()
            .is_empty()
    {
        // Step 1.1.
        let no_of_excess_entries_before =
                self.resource_timing_secondary_entries.borrow().len();
    
[...]

This comment has been minimized.

@aditj

aditj Dec 21, 2018

Author Contributor

Cool! Should I document all the methods I wrote?

This comment has been minimized.

@ferjm

ferjm Dec 24, 2018

Member

Yes, please.

while !self.resource_timing_secondary_entries.borrow().is_empty() {
let no_of_excess_entries_before = self.resource_timing_secondary_entries.borrow().len();

if !self.can_add_resource_timing_entry() {
self.upcast::<EventTarget>()
.fire_event(atom!("resourcetimingbufferfull"));
This conversation was marked as resolved by ferjm

This comment has been minimized.

@ferjm

ferjm Dec 21, 2018

Member
self.upcast::<EventTarget>()
      .fire_event(atom!("resourcetimingbufferfull"));

This comment has been minimized.

@aditj

aditj Dec 21, 2018

Author Contributor

Changed the code, btw what does up casting mean and does in rust ?

}
self.copy_secondary_resource_timing_buffer();
let no_of_excess_entries_after = self.resource_timing_secondary_entries.borrow().len();
if no_of_excess_entries_before <= no_of_excess_entries_after {
self.resource_timing_secondary_entries.borrow_mut().clear();
break;
}
}
self.resource_timing_buffer_pending_full_event.set(false);
}
/// `add a PerformanceResourceTiming entry` paragraph of
/// https://w3c.github.io/resource-timing/#sec-extensions-performance-interface
fn should_queue_resource_entry(&self, entry: &PerformanceEntry) -> bool {
// Step 1 is done in the args list.
if !self.resource_timing_buffer_pending_full_event.get() {
// Step 2.
if self.can_add_resource_timing_entry() {
// Step 2.a is done in `queue_entry`
// Step 2.b.
self.resource_timing_buffer_current_size
.set(self.resource_timing_buffer_current_size.get() + 1);
// Step 2.c.
return true;
}
// Step 3.
self.resource_timing_buffer_pending_full_event.set(true);
self.fire_buffer_full_event();
}
// Steps 4 and 5.
self.resource_timing_secondary_entries
.borrow_mut()
.push_back(DomRoot::from_ref(entry));
false
}
This conversation was marked as resolved by aditj

This comment has been minimized.

@ferjm

ferjm Dec 27, 2018

Member

Let's clean this up a little bit and add some documentation, so it's more clear how we implement the spec:

    /// `add a PerformanceResourceTiming entry` paragraph of
    /// https://w3c.github.io/resource-timing/#sec-extensions-performance-interface
    fn should_queue_resource_entry(&self, entry: &PerformanceEntry) -> bool {
        // Step 1 is done in the args list.
        if !self.resource_timing_buffer_pending_full_event.get() {
            // Step 2.
            if self.can_add_resource_timing_entry() {
                // Step 2.a is done in `queue_entry`
                // Step 2.b.
                self.resource_timing_buffer_current_size
                    .set(self.resource_timing_buffer_current_size.get() + 1);
                // Step 2.c.
                return true;
            }
            // Step 3.
            self.resource_timing_buffer_pending_full_event.set(true);
            self.fire_buffer_full_event();
        }
        // Steps 4 and 5.
        self.resource_timing_secondary_entries
            .borrow_mut()
            .push_back(DomRoot::from_ref(entry));
        false
    }

This comment has been minimized.

@aditj

aditj Dec 27, 2018

Author Contributor

Thanks for help with the commenting! :)

}

impl PerformanceMethods for Performance {
@@ -450,20 +477,21 @@ impl PerformanceMethods for Performance {
.borrow_mut()
.clear_entries_by_name_and_type(measure_name, Some(DOMString::from("measure")));
}
/// https://w3c.github.io/resource-timing/#dom-performance-clearresourcetimings
/// https://w3c.github.io/resource-timing/#dom-performance-clearresourcetimings

fn ClearResourceTimings(&self) {
This conversation was marked as resolved by ferjm

This comment has been minimized.

@ferjm

ferjm Dec 14, 2018

Member

As the linter says here, we need to add a comment with a link to the spec. In this case, the comment should be:

/// https://w3c.github.io/resource-timing/#dom-performance-clearresourcetimings
self.entries
.borrow_mut()
.clear_entries_by_name_and_type(None, Some(DOMString::from("resource")));
This conversation was marked as resolved by ferjm

This comment has been minimized.

@ferjm

ferjm Dec 14, 2018

Member

We need to set resource_timing_buffer_current_size to 0 here as well.

self.resource_timing_buffer_current_size.set(0_usize);
self.resource_timing_buffer_current_size.set(0_usize);
This conversation was marked as resolved by ferjm

This comment has been minimized.

@ferjm

ferjm Dec 21, 2018

Member

No need to specify the type here. 0 alone should work.

}
/// https://w3c.github.io/resource-timing/#dom-performance-setresourcetimingbuffersize
/// https://w3c.github.io/resource-timing/#dom-performance-setresourcetimingbuffersize

fn SetResourceTimingBufferSize(&self, max_size: u32) {
This conversation was marked as resolved by ferjm

This comment has been minimized.

@ferjm

ferjm Dec 14, 2018

Member

Change the comment to:

/// https://w3c.github.io/resource-timing/#dom-performance-setresourcetimingbuffersize
self.resource_timing_buffer_size_limit.set(max_size as usize);
self.resource_timing_buffer_size_limit
.set(max_size as usize);
}
/// https://w3c.github.io/resource-timing/#dom-performance-onresourcetimingbufferfull
/// https://w3c.github.io/resource-timing/#dom-performance-onresourcetimingbufferfull

event_handler!(
resourcetimingbufferfull,
@@ -27,9 +27,3 @@ interface PerformanceResourceTiming : PerformanceEntry {
/// readonly attribute unsigned long long decodedBodySize;
// [Default] object toJSON();
};

// partial interface Performance {
// void clearResourceTimings();
// void setResourceTimingBufferSize(unsigned long maxSize);
// attribute EventHandler onresourcetimingbufferfull;
// };
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.