Skip to content

/ servo

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::DefaultView return a null value when there's no browsi… #11502

Closed
wants to merge 1 commit into from
Closed

Make Document::DefaultView return a null value when there's no browsi… #11502

wants to merge 1 commit into from

Conversation

@kevgs
Copy link
Contributor

kevgs commented May 30, 2016 •
edited by Ms2ger

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #11469 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

…ng context

This change is Reviewable
@Ms2ger
Copy link
Contributor

Ms2ger commented May 30, 2016

First of all, thank you for your PR!

You made some changes to scrolling APIs; let's see if there are any tests for those.

@bors-servo try

If not, I'd like to ask you to write a few tests. (In that case, you can use ./mach create-wpt tests/wpt/web-platform-tests/cssom-view/scrolling-no-browsing-context.html to get started.)

+S-needs-code-changes +S-awaiting-answer

Previously, kevgs wrote…

Make Document::DefaultView return a null value when there's no browsi…

Fix for #11469

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

…ng context

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

components/script/dom/element.rs, line 1306 [r1] (raw file):

            return;
        }
        let win = w.unwrap();

This is somewhat of an unfortunate pattern; I suggest

let win = match doc.GetDefaultView() {
    None => return,
    Some(win) => win,
};

here and below.

Comments from Reviewable
@bors-servo
Copy link
Contributor

bors-servo commented May 30, 2016

⌛ Trying commit 2653143 with merge c775538...
bors-servo added a commit that referenced this pull request May 30, 2016
@bors-servo
Auto merge of #11502 - kevgs:default_view, r=<try>
c775538 
Make Document::DefaultView return a null value when there's no browsi…

<!-- Please describe your changes on the following line: -->
Fix for #11469
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

…ng context

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11502)
<!-- Reviewable:end -->
@Ms2ger Ms2ger mentioned this pull request May 30, 2016
Closed
@highfive
Copy link

highfive commented May 30, 2016

New code was committed to pull request.
@kevgs
Copy link
Contributor Author

kevgs commented May 30, 2016 •
edited

Made code a bit more good looking. Thanks for the suggestion!

Now looking for scrolling tests.

AppVeyor build failed, but it looks like something irrelevant to my patch because build failed in some c++ code.
@Ms2ger
Copy link
Contributor

Ms2ger commented May 30, 2016

You can ignore the appveyor results.
@Ms2ger Ms2ger self-assigned this May 30, 2016
@highfive
Copy link

highfive commented May 31, 2016

New code was committed to pull request.
1 similar comment
@highfive
Copy link

highfive commented May 31, 2016

New code was committed to pull request.
@kevgs
Copy link
Contributor Author

kevgs commented May 31, 2016

Sorry for the long timeout. It took me a while before I realized how this test should look like.
@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 1, 2016

Looks like the manifest update got lost; please apply the following diff:

--- a/tests/wpt/metadata/MANIFEST.json
+++ b/tests/wpt/metadata/MANIFEST.json
@@ -36028,7 +36028,16 @@
   "local_changes": {
     "deleted": [],
     "deleted_reftests": {},
-    "items": {},
+    "items": {
+      "testharness": {
+        "cssom-view/scrolling-no-browsing-context.html": [
+          {
+            "path": "cssom-view/scrolling-no-browsing-context.html",
+            "url": "/cssom-view/scrolling-no-browsing-context.html"
+          }
+        ]
+      }
+    },
     "reftest_nodes": {}
   },
   "reftest_nodes": {

and squash all commits into one.

-S-awaiting-answer -S-awaiting-review +S-needs-code-changes

Previously, kevgs wrote…

Sorry for the long timeout. It took me a while before I realized how this test should look like.

Reviewed 1 of 1 files at r2, 1 of 2 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

tests/wpt/web-platform-tests/cssom-view/scrolling-no-browsing-context.html, line 3 [r3] (raw file):

<!doctype html>
<meta charset="utf-8">
<title></title>

Can you add a title here?

tests/wpt/web-platform-tests/cssom-view/scrolling-no-browsing-context.html, line 30 [r3] (raw file):

        },  "Element get and set scrollTop, scrollLeft, scroll() and scrollTo() test");

Trailing whitespace; please remove.

Comments from Reviewable
@highfive
Copy link

highfive commented Jun 1, 2016

New code was committed to pull request.
@highfive
Copy link

highfive commented Jun 1, 2016

New code was committed to pull request.
@kevgs
Make Document::DefaultView return a null value when there's no browsi…
9619ee7 
…ng context
@kevgs kevgs force-pushed the kevgs:default_view branch from 1ba9b5f to 9619ee7 Jun 1, 2016
@highfive
Copy link

highfive commented Jun 1, 2016

New code was committed to pull request.
@kevgs
Copy link
Contributor Author

kevgs commented Jun 1, 2016

I failed to squash and created another PR #11548
@KiChjang KiChjang closed this Jun 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Ms2ger Ms2ger

Labels
S-awaiting-review
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Make Document::DefaultView return a null value when there's no browsing context
5 participants
@kevgs @Ms2ger @bors-servo @highfive @KiChjang
You can’t perform that action at this time.