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 location.reload() #7064

Merged
merged 2 commits into from
Sep 21, 2015
Merged

Implement location.reload() #7064

merged 2 commits into from
Sep 21, 2015

Conversation

paulrouget
Copy link
Contributor

This is a naive implementation of window.location.reload().
I'd appreciate any feedback.

I was wondering if it'd be better to implement ConstellationMsg::Reload instead of using load_url.

Also, what kind of test should I write?

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 7, 2015
@highfive
Copy link

highfive commented Aug 7, 2015

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @larsbergstrom (or someone else) soon.

@paulrouget paulrouget force-pushed the reload branch 2 times, most recently from 227c3a6 to 2379d19 Compare August 7, 2015 09:24
@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 7, 2015

I'm not sure I want to add more naive navigation code, we already have way to much of that.

@paulrouget
Copy link
Contributor Author

@Ms2ger said we should wait until he has rewritten all the navigation code

@jdm jdm added S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 7, 2015
@Ms2ger Ms2ger self-assigned this Aug 19, 2015
@Ms2ger Ms2ger added S-needs-rebase There are merge conflict errors. and removed S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. labels Sep 2, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 2, 2015

-S-blocked-on-external +S-needs-tests +S-needs-rebase

Add some kind of wpt test?


Reviewed 2 of 2 files at r1.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@Ms2ger Ms2ger added the S-needs-tests New tests have been requested by a reviewer. label Sep 2, 2015
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 3, 2015
@paulrouget
Copy link
Contributor Author

This is the only way I found to test location.reload(). Apparently, unload events and iframe.contentWindow don't work. So I used this hacky mechanism. Hope that's ok.

Also, ./mach test-wpt --manifest-update made some unrelated changes to tests/wpt/metadata/MANIFEST.json. Is that normal?

@Ms2ger r?

@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 3, 2015

LGTM, but avoid new JS features like const and () => {}.

@Ms2ger Ms2ger added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. S-needs-tests New tests have been requested by a reviewer. labels Sep 3, 2015
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Sep 3, 2015

test(function() {
assert_equals(url, innerURL, "iframe url");
}, "iframe url should not change");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed this last time. You're creating multiple tests with the same name, which would be painful if we failed one of them. Please just put everything in one async_test, like:

async_test(function() {
  var url = new URL("./location_reload-iframe.html", window.location).href;
  ...
  window._ping = this.step_func(function() {
    ...
    assert_equals(url, innerURL, "iframe url");
    ...
    this.done();
  });

  iframe.src = url;
});

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Sep 4, 2015
@highfive highfive removed the S-needs-code-changes Changes have not yet been made that were requested by a reviewer. label Sep 7, 2015
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 7, 2015
@paulrouget
Copy link
Contributor Author

I don't know why, but this.step_func doesn't appear to work.

I use function(){this.step(…)} instead.

@paulrouget
Copy link
Contributor Author

nvm, step_func works.

@paulrouget
Copy link
Contributor Author

@Ms2ger review ping

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #7657) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Sep 17, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 21, 2015

Sorry for the delay here.

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit e742e7e has been approved by Ms2ger

@bors-servo
Copy link
Contributor

⌛ Testing commit e742e7e with merge 4dc986b...

@highfive highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Sep 21, 2015
bors-servo pushed a commit that referenced this pull request Sep 21, 2015
Implement location.reload()

This is a naive implementation of `window.location.reload()`.
I'd appreciate any feedback.

I was wondering if it'd be better to implement `ConstellationMsg::Reload` instead of using  `load_url`.

Also, what kind of test should I write?

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7064)
<!-- Reviewable:end -->
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-needs-rebase There are merge conflict errors. labels Sep 21, 2015
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@bors-servo bors-servo merged commit e742e7e into servo:master Sep 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants