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

Some janitoring over the layout crate #22684

Merged
merged 20 commits into from Jan 14, 2019

Conversation

Projects
None yet
7 participants
@nox
Copy link
Member

nox commented Jan 14, 2019

This PR moves a bunch of code around and makes some methods from one-use traits into inherent methods.


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Jan 14, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/bindings/trace.rs, components/script/script_thread.rs, components/script/dom/window.rs
  • @KiChjang: components/script/dom/bindings/trace.rs, components/script/script_thread.rs, components/script/dom/window.rs
  • @emilio: components/layout/table_caption.rs, components/layout/table.rs, components/layout/display_list/builder.rs, components/layout/table_wrapper.rs, components/layout/table_rowgroup.rs and 7 more
@highfive

This comment has been minimized.

Copy link

highfive commented Jan 14, 2019

warning Warning warning

  • These commits modify layout and script code, but no tests are modified. Please consider adding a test!
@nox

This comment has been minimized.

Copy link
Member Author

nox commented Jan 14, 2019

@highfive highfive assigned SimonSapin and unassigned Manishearth Jan 14, 2019

@emilio

emilio approved these changes Jan 14, 2019

.unwrap()
.send(ConstellationControlMsg::ReportCSSError(
self.pipelineid,
"".to_owned(),

This comment has been minimized.

@emilio

emilio Jan 14, 2019

Member

We have the URL here so reporting it should be trivial, mind filing an issue for that?

This comment has been minimized.

@nox

nox Jan 14, 2019

Author Member

I drive-by fixed it.

self.positioning(),
established_reference_frame.is_some(),
) {
if established_reference_frame.is_some() ||

This comment has been minimized.

@emilio

emilio Jan 14, 2019

Member

can you do instead:

let abspos_containing_block = established_reference_frame.is_some() || ....;
if abspos_containing_block {
fragment.style.get_box().position,
false,
) {
if fragment.style.get_box().position != StylePosition::Static {

This comment has been minimized.

@emilio

emilio Jan 14, 2019

Member

And same here?

This looks wrong btw, an inline can also establish a reference frame.

This comment has been minimized.

@nox

nox Jan 14, 2019

Author Member

Want me to file a follow-up issue? This is just the same code as before right?


impl<'a> LayoutDamageComputation for &'a mut dyn Flow {
fn compute_layout_damage(self) -> SpecialRestyleDamage {
impl dyn Flow {

This comment has been minimized.

@emilio

emilio Jan 14, 2019

Member

Oh, this is fancy.

/// text display items it may be `TextCursor` or `VerticalTextCursor`.
#[inline]
fn get_cursor(values: &ComputedValues, default_cursor: CursorKind) -> Option<CursorKind> {
match (

This comment has been minimized.

@emilio

emilio Jan 14, 2019

Member

You could also just move it to style, your call.

In any case I think this is simpler to read as follows, if you want to make that change:

let ui = values.get_inherited_ui();
if ui.pointer_events == PointerEvents::None {
    return None;
}

match ui.cursor {
    // ...
}

This comment has been minimized.

@nox

nox Jan 14, 2019

Author Member

Made some changes like you suggested but didn't move it to style given there is no other user.

@nox nox force-pushed the next-layout branch from 7110895 to d1799e6 Jan 14, 2019

@CYBAI

This comment has been minimized.

Copy link
Collaborator

CYBAI commented Jan 14, 2019

From Travis 👀

Diff in /home/travis/build/servo/servo/components/script/dom/window.rs at line 1602:
     }
 
     pub fn scroll_offset_query(&self, node: &Node) -> Vector2D<f32> {
-        if let Some(scroll_offset) = self
-            .scroll_offsets
-            .borrow()
-            .get(&node.to_opaque())
-        {
+        if let Some(scroll_offset) = self.scroll_offsets.borrow().get(&node.to_opaque()) {
             return *scroll_offset;
         }
         Vector2D::new(0.0, 0.0)
Diff in /home/travis/build/servo/servo/components/script/dom/window.rs at line 1621:
         // The scroll offsets are immediatly updated since later calls
         // to topScroll and others may access the properties before
         // webrender has a chance to update the offsets.
-        self.scroll_offsets.borrow_mut().insert(
-            node.to_opaque(),
-            Vector2D::new(x_ as f32, y_ as f32),
-        );
+        self.scroll_offsets
+            .borrow_mut()
+            .insert(node.to_opaque(), Vector2D::new(x_ as f32, y_ as f32));
 
         let NodeScrollIdResponse(scroll_id) = self.layout_rpc.node_scroll_id();
 
Run `./mach fmt` to fix the formatting
&QueryMsg::ResolvedStyleQuery(..) |
&QueryMsg::OffsetParentQuery(_) |
&QueryMsg::StyleQuery(_) => false,
ReflowGoal::LayoutQuery(ref querymsg, _) => match *querymsg {

This comment has been minimized.

@SimonSapin

SimonSapin Jan 14, 2019

Member

No need to change this back, but in the 2018 edition you can remove & from most patterns without adding * in the matched expression.

nox added some commits Jan 14, 2019

Make Window::scroll_offsets store keys as OpaqueNode values
This is the type that is supposed to signal that we will never ever
try to get back a Node from it in an unsafe way, unlike
UntrustedNodeAddress.

@nox nox force-pushed the next-layout branch from d1799e6 to cf15336 Jan 14, 2019

@nox

This comment has been minimized.

Copy link
Member Author

nox commented Jan 14, 2019

@bors-servo r=emilio,SimonSapin

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 14, 2019

📌 Commit cf15336 has been approved by emilio,SimonSapin

@highfive highfive assigned emilio and unassigned SimonSapin Jan 14, 2019

bors-servo added a commit that referenced this pull request Jan 14, 2019

Auto merge of #22684 - servo:next-layout, r=emilio,SimonSapin
Some janitoring over the layout crate

This PR moves a bunch of code around and makes some methods from one-use traits into inherent methods.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22684)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 14, 2019

⌛️ Testing commit cf15336 with merge c242abd...

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 14, 2019

☀️ Test successful - android-mac, arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, magicleap, status-taskcluster
Approved by: emilio,SimonSapin
Pushing c242abd to master...

@bors-servo bors-servo merged commit cf15336 into master Jan 14, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Taskcluster (pull_request) TaskGroup: success
Details
homu Test successful
Details

@SimonSapin SimonSapin deleted the next-layout branch Jan 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment