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

Make document url mutable and implement location.replace() #13418

Merged
merged 5 commits into from Nov 20, 2016
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Move fragment navigation into Document object

Move the `check_and_scroll_fragment()` method into Document, make the
mothod set the fragment of url after navigation, and use the
`perform_a_scroll()` method to scroll rather than an individual
method. Also removes the broken `Window.fragment` fields.
  • Loading branch information
stshine committed Nov 18, 2016
commit 986314904341fafa7caa1d32ee9d992d7c45282e
@@ -18,7 +18,7 @@ use dom::bindings::codegen::Bindings::NodeBinding::NodeMethods;
use dom::bindings::codegen::Bindings::NodeFilterBinding::NodeFilter;
use dom::bindings::codegen::Bindings::PerformanceBinding::PerformanceMethods;
use dom::bindings::codegen::Bindings::TouchBinding::TouchMethods;
use dom::bindings::codegen::Bindings::WindowBinding::WindowMethods;
use dom::bindings::codegen::Bindings::WindowBinding::{ScrollBehavior, WindowMethods};
use dom::bindings::codegen::UnionTypes::NodeOrString;
use dom::bindings::error::{Error, ErrorResult, Fallible};
use dom::bindings::inheritance::{Castable, ElementTypeId, HTMLElementTypeId, NodeTypeId};
@@ -600,6 +600,45 @@ impl Document {
}
}

/// https://html.spec.whatwg.org/multipage/#scroll-to-the-fragment-identifier
pub fn check_and_scroll_fragment(&self, fragment: &str) {
let target = self.find_fragment_node(fragment);

This comment has been minimized.

@KiChjang

KiChjang Nov 16, 2016

Member

Does .r() not work here?

This comment has been minimized.

@stshine

stshine Nov 16, 2016

Author Contributor

No... you have to keep the Root on the stack.


// Step 1
self.set_target_element(target.r());

let point = if fragment.is_empty() || fragment.to_lowercase() == "top" {

This comment has been minimized.

@emilio

emilio Nov 19, 2016

Member

nit: use eq_ignore_ascii_case("top") instead.

This comment has been minimized.

@stshine

stshine Nov 19, 2016

Author Contributor

Darnnnnn, suddenly found that I should just use Cell for ServoUrl rather than DOMRefCell. Sadface :-(

This comment has been minimized.

@stshine

stshine Nov 19, 2016

Author Contributor

ignore my previous comment, sorry.

// FIXME(stshine): this should be the origin of the stacking context space,
// which may differ under the influence of writing mode.
Some((0.0, 0.0))
} else {
target.r().map(|element| {
// FIXME(#8275, pcwalton): This is pretty bogus when multiple layers
// are involved. Really what needs to happen is that this needs to go
// through layout to ask which layer the element belongs to, and have
// it send the scroll message to the compositor.
let rect = element.upcast::<Node>().bounding_content_box();

// In order to align with element edges, we snap to unscaled pixel
// boundaries, since the paint thread currently does the same for
// drawing elements. This is important for pages that require pixel
// perfect scroll positioning for proper display (like Acid2). Since
// we don't have the device pixel ratio here, this might not be
// accurate, but should work as long as the ratio is a whole number.
// Once #8275 is fixed this should actually take into account the
// real device pixel ratio.
(rect.origin.x.to_nearest_px() as f32,
rect.origin.y.to_nearest_px() as f32)
})
};

if let Some((x, y)) = point {
// Step 3
self.window.perform_a_scroll(x, y, ScrollBehavior::Instant,
target.r());
}
}

fn get_anchor_by_name(&self, name: &str) -> Option<Root<Element>> {
let check_anchor = |node: &HTMLAnchorElement| {
let elem = node.upcast::<Element>();
@@ -40,7 +40,7 @@ impl Location {
setter: fn(&mut ServoUrl, USVString)) {
let mut url = self.window.get_url();
setter(&mut url, value);
self.window.load_url(url, false, None);
self.window.load_url(url, false, false, None);
}
}

@@ -51,7 +51,7 @@ impl LocationMethods for Location {
// _entry settings object_.
let base_url = self.window.get_url();
if let Ok(url) = base_url.join(&url.0) {
self.window.load_url(url, false, None);
self.window.load_url(url, false, false, None);
Ok(())
} else {
Err(Error::Syntax)
@@ -60,7 +60,8 @@ impl LocationMethods for Location {

// https://html.spec.whatwg.org/multipage/#dom-location-reload
fn Reload(&self) {
self.window.load_url(self.get_url(), true, None);
self.window.load_url(self.get_url(), true, true, None);
}
}

// https://html.spec.whatwg.org/multipage/#dom-location-hash
@@ -109,7 +110,7 @@ impl LocationMethods for Location {
// https://html.spec.whatwg.org/multipage/#dom-location-href
fn SetHref(&self, value: USVString) {
if let Ok(url) = self.window.get_url().join(&value.0) {
self.window.load_url(url, false, None);
self.window.load_url(url, false, false, None);
}
}

@@ -103,6 +103,7 @@ use time;
use timers::{IsInterval, TimerCallback};
#[cfg(any(target_os = "macos", target_os = "linux", target_os = "windows"))]
use tinyfiledialogs::{self, MessageBoxIcon};
use url::Position;
use util::geometry::{self, max_rect};
use util::opts;
use util::prefs::PREFS;
@@ -205,9 +206,6 @@ pub struct Window {
#[ignore_heap_size_of = "channels are hard"]
bluetooth_thread: IpcSender<BluetoothRequest>,

/// Pending scroll to fragment event, if any
fragment_name: DOMRefCell<Option<String>>,

/// An enlarged rectangle around the page contents visible in the viewport, used
/// to prevent creating display list items for content that is far away from the viewport.
page_clip_rect: Cell<Rect<Au>>,
@@ -1331,13 +1329,24 @@ impl Window {
}

/// Commence a new URL load which will either replace this window or scroll to a fragment.
pub fn load_url(&self, url: ServoUrl, replace: bool, referrer_policy: Option<ReferrerPolicy>) {
pub fn load_url(&self, url: ServoUrl, replace: bool, force_reload: bool,
referrer_policy: Option<ReferrerPolicy>) {
let doc = self.Document();
let referrer_policy = referrer_policy.or(doc.get_referrer_policy());

// https://html.spec.whatwg.org/multipage/#navigating-across-documents
if !force_reload && url.as_url().unwrap()[..Position::AfterQuery] == doc.url().as_url().unwrap()[..Position::AfterQuery] {
// Step 5
if let Some(fragment) = url.fragment() {
doc.check_and_scroll_fragment(fragment);
doc.set_url(url.clone());

This comment has been minimized.

@KiChjang

KiChjang Nov 16, 2016

Member

Does borrowck complain if you remove the .clone()?

This comment has been minimized.

@stshine

stshine Nov 16, 2016

Author Contributor

Yes, it is borrowed by if let.

return
}
}

self.main_thread_script_chan().send(
MainThreadScriptMsg::Navigate(self.upcast::<GlobalScope>().pipeline_id(),
LoadData::new(url, referrer_policy, Some(doc.url().clone())),
LoadData::new(url, referrer_policy, Some(doc.url())),
replace)).unwrap();
}

@@ -1348,14 +1357,6 @@ impl Window {
ReflowReason::Timer);
}

pub fn set_fragment_name(&self, fragment: Option<String>) {
*self.fragment_name.borrow_mut() = fragment;
}

pub fn steal_fragment_name(&self) -> Option<String> {
self.fragment_name.borrow_mut().take()
}

pub fn set_window_size(&self, size: WindowSizeData) {
self.window_size.set(Some(size));
}
@@ -59,7 +59,6 @@ use euclid::Rect;
use euclid::point::Point2D;
use hyper::header::{ContentType, HttpDate, LastModified};
use hyper::header::ReferrerPolicy as ReferrerPolicyHeader;
use hyper::method::Method;
use hyper::mime::{Mime, SubLevel, TopLevel};
use hyper_serde::Serde;
use ipc_channel::ipc::{self, IpcSender};
@@ -91,7 +90,6 @@ use script_traits::CompositorEvent::{KeyEvent, MouseButtonEvent, MouseMoveEvent,
use script_traits::CompositorEvent::{TouchEvent, TouchpadPressureEvent};
use script_traits::webdriver_msg::WebDriverScriptCommand;
use servo_url::ServoUrl;
use std::borrow::ToOwned;
use std::cell::Cell;
use std::collections::{hash_map, HashMap, HashSet};
use std::option::Option;
@@ -1231,20 +1229,8 @@ impl ScriptThread {
self.dom_manipulation_task_source.queue(handler, doc.window().upcast()).unwrap();

if let Some(fragment) = doc.url().fragment() {
self.check_and_scroll_fragment(fragment, pipeline, &doc);
}
}

fn check_and_scroll_fragment(&self, fragment: &str, pipeline_id: PipelineId, doc: &Document) {
match doc.find_fragment_node(fragment) {
Some(ref node) => {
doc.set_target_element(Some(&node));
self.scroll_fragment_point(pipeline_id, &node);
}
None => {
doc.set_target_element(None);
}
}
doc.check_and_scroll_fragment(fragment);
};

This comment has been minimized.

@KiChjang

KiChjang Sep 28, 2016

Member

nit: Remove this extra ;.

This comment has been minimized.

@stshine

stshine Sep 28, 2016

Author Contributor

This is to appease borrow checker on RefCell in if let. See rust-lang/rust#22449

}

fn collect_reports(&self, reports_chan: ReportsChan) {
@@ -1826,26 +1812,6 @@ impl ScriptThread {
}
}

fn scroll_fragment_point(&self, pipeline_id: PipelineId, element: &Element) {
// FIXME(#8275, pcwalton): This is pretty bogus when multiple layers are involved.
// Really what needs to happen is that this needs to go through layout to ask which
// layer the element belongs to, and have it send the scroll message to the
// compositor.
let rect = element.upcast::<Node>().bounding_content_box();

// In order to align with element edges, we snap to unscaled pixel boundaries, since the
// paint thread currently does the same for drawing elements. This is important for pages
// that require pixel perfect scroll positioning for proper display (like Acid2). Since we
// don't have the device pixel ratio here, this might not be accurate, but should work as
// long as the ratio is a whole number. Once #8275 is fixed this should actually take into
// account the real device pixel ratio.
let point = Point2D::new(rect.origin.x.to_nearest_px() as f32,
rect.origin.y.to_nearest_px() as f32);

let message = ConstellationMsg::ScrollFragmentPoint(pipeline_id, point, false);
self.constellation_chan.send(message).unwrap();
}

/// Reflows non-incrementally, rebuilding the entire layout tree in the process.
fn rebuild_and_force_reflow(&self, document: &Document, reason: ReflowReason) {
let window = window_from_node(&*document);
@@ -1987,25 +1953,6 @@ impl ScriptThread {
frame_id: Option<FrameId>,
load_data: LoadData,
replace: bool) {
// Step 7.
{
let nurl = &load_data.url;
if let Some(fragment) = nurl.fragment() {
let document = match self.documents.borrow().find_document(parent_pipeline_id) {
Some(document) => document,
None => return warn!("Message sent to closed pipeline {}.", parent_pipeline_id),
};
let nurl = nurl.as_url().unwrap();
if let Some(url) = document.url().as_url() {
if &url[..Position::AfterQuery] == &nurl[..Position::AfterQuery] &&
load_data.method == Method::Get {
self.check_and_scroll_fragment(fragment, parent_pipeline_id, &document);
return;
}
}
}
}

match frame_id {
Some(frame_id) => {
if let Some(iframe) = self.documents.borrow().find_iframe(parent_pipeline_id, frame_id) {
@@ -2032,13 +1979,6 @@ impl ScriptThread {
ReflowQueryType::NoQuery,
ReflowReason::WindowResize);

let fragment_node = window.steal_fragment_name()
.and_then(|name| document.find_fragment_node(&*name));
match fragment_node {
Some(ref node) => self.scroll_fragment_point(pipeline_id, &node),
None => {}
}

// http://dev.w3.org/csswg/cssom-view/#resizing-viewports
if size_type == WindowSizeType::Resize {
let uievent = UIEvent::new(&window,
@@ -2118,8 +2058,6 @@ impl ScriptThread {

// https://html.spec.whatwg.org/multipage/#the-end steps 3-4.
document.process_deferred_scripts();

window.set_fragment_name(final_url.fragment().map(str::to_owned));
}

fn handle_css_error_reporting(&self, pipeline_id: PipelineId, filename: String,
@@ -98,6 +98,10 @@ impl ServoUrl {
Arc::make_mut(&mut self.0).set_password(pass)
}

pub fn set_fragment(&mut self, fragment: Option<&str>) {
Arc::make_mut(&mut self.0).set_fragment(fragment)
}

pub fn username(&self) -> &str {
self.0.username()
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.