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 History's back/forward APIs #5670

Closed
jdm opened this issue Apr 13, 2015 · 19 comments
Closed

Implement History's back/forward APIs #5670

jdm opened this issue Apr 13, 2015 · 19 comments
Assignees

Comments

@jdm
Copy link
Member

@jdm jdm commented Apr 13, 2015

Spec: https://html.spec.whatwg.org/multipage/browsers.html#the-history-interface, https://html.spec.whatwg.org/multipage/browsers.html#dom-history-back,
Code: https://github.com/servo/servo/wiki/Adding-a-new-WebIDL-binding

This will just end up mimicing the behaviour of the GoForward/GoBack APIs of HTMLIFrameElement (see components/script/dom/element/htmliframelement.rs)

@jdm
Copy link
Member Author

@jdm jdm commented Apr 13, 2015

cc #5669

@aneeshusa
Copy link
Member

@aneeshusa aneeshusa commented Apr 13, 2015

I'd like to try this one out for size.

@jdm jdm added the C-assigned label Apr 13, 2015
@jdm
Copy link
Member Author

@jdm jdm commented Apr 13, 2015

It's yours!

@ghost
Copy link

@ghost ghost commented May 6, 2015

Hi, could someone help me out where I could find the code relevant to fix this bug.

@jdm
Copy link
Member Author

@jdm jdm commented May 6, 2015

@jinankjain are the links in the original comment unclear? This task requires creating a new webidl interface and a new Rust implementation of the interface, but there are also existing methods in htmliframeelement.rs that can be used as models.

@snf
Copy link
Contributor

@snf snf commented May 18, 2015

Browser context needs some work and there are tons of TODO(handle browsing context) out there in the dom files so I think we should start there.

And wouldn't this struct keep alive all the Document, Window, etc context from the browsing history creating a little memory leak?:

// This isn't a DOM struct, just a convenience struct
// without a reflector, so we don't mark this as #[dom_struct]
#[must_root]
#[privatize]
#[jstraceable]
pub struct SessionHistoryEntry {
    document: JS<Document>,
    children: Vec<BrowserContext>
}

Also, should we add something else in the script_task/constellation?

@jdm
Copy link
Member Author

@jdm jdm commented May 18, 2015

It technically won't be a leak, since it will be removed when the script task shuts down. We should also add logic to prune the actual cached document at some point, but that will require extra logic to reload it if we end up trying to retrieve it from history later.

@snf
Copy link
Contributor

@snf snf commented May 22, 2015

@aneeshusa or @jinankjain still working on this one?

@vectorijk
Copy link
Contributor

@vectorijk vectorijk commented May 22, 2015

@snf are you also working on this one?
@jdm I would like to try this one.

@snf
Copy link
Contributor

@snf snf commented May 22, 2015

@vectorijk no, devs don't allow me to work in e-easy bugs anymore :'( . So all yours if no one is working on it.

@jdm
Copy link
Member Author

@jdm jdm commented May 23, 2015

Yes, go for it :)

vectorijk added a commit to vectorijk/servo that referenced this issue May 28, 2015
@boghison
Copy link
Contributor

@boghison boghison commented Jul 16, 2015

@vectorijk Excuse me, are you still working on this?

@vectorijk
Copy link
Contributor

@vectorijk vectorijk commented Jul 16, 2015

@boghison Yes,I am still working on it. Cause I am a newbie, I am doing some research
on how to implement this api and also digged into how WebKit and gecko
wrote on this api.
On Jul 16, 2015 10:04 AM, "Bogdan Cuza" notifications@github.com wrote:

@vectorijk https://github.com/vectorijk Excuse me, are you still
working on this?


Reply to this email directly or view it on GitHub
#5670 (comment).

@jdm
Copy link
Member Author

@jdm jdm commented Jul 31, 2015

Made any progress @vectorijk? Any questions I can answer?

@jdm jdm removed the C-assigned label Aug 6, 2015
@tomjakubowski
Copy link
Contributor

@tomjakubowski tomjakubowski commented Aug 15, 2015

I'd like to take this on!

@vectorijk
Copy link
Contributor

@vectorijk vectorijk commented Aug 15, 2015

@tomjakubowski I am off~.

@jdm jdm added the C-assigned label Aug 16, 2015
@jdm
Copy link
Member Author

@jdm jdm commented Aug 16, 2015

Sounds like it's all yours, @tomjakubowski.

tomjakubowski added a commit to tomjakubowski/servo that referenced this issue Aug 17, 2015
Unfortunately, not many once-failing tests are now expected to pass; the
history interface tests often depend on things like History#go working.

Closes servo#5670
tomjakubowski added a commit to tomjakubowski/servo that referenced this issue Aug 18, 2015
Unfortunately, not many once-failing tests are now expected to pass; the
history interface tests often depend on things like History#go working.

Closes servo#5670
tomjakubowski added a commit to tomjakubowski/servo that referenced this issue Aug 18, 2015
Unfortunately, not many once-failing tests are now expected to pass; the
history interface tests often depend on things like History#go working.

Closes servo#5670
@jdm
Copy link
Member Author

@jdm jdm commented Feb 26, 2016

#7520 was proposed to implement this. It got stuck on the requirement to write tests; I'm not comfortable leaving this marked as E-easy until we come up with suggestions for how to write tests for it.

cbrewster added a commit to cbrewster/servo that referenced this issue May 1, 2016
Unfortunately, not many once-failing tests are now expected to pass; the
history interface tests often depend on things like History#go working.

Closes servo#5670
cbrewster added a commit to cbrewster/servo that referenced this issue May 1, 2016
Unfortunately, not many once-failing tests are now expected to pass; the
history interface tests often depend on things like History#go working.

Closes servo#5670
cbrewster added a commit to cbrewster/servo that referenced this issue May 1, 2016
Unfortunately, not many once-failing tests are now expected to pass; the
history interface tests often depend on things like History#go working.

Closes servo#5670
bors-servo added a commit that referenced this issue May 1, 2016
Implement History back/forward APIs

Rebasing @tomjakubowski's work.
I still need to add a test for this (which is why this was not merged before).

Original PR:
> Unfortunately, not many once-failing tests are now expected to pass; the
> history interface tests often depend on things like History#go working.
>
> Closes #5670

<!-- 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/10956)
<!-- Reviewable:end -->
cbrewster added a commit to cbrewster/servo that referenced this issue May 1, 2016
Unfortunately, not many once-failing tests are now expected to pass; the
history interface tests often depend on things like History#go working.

Closes servo#5670
bors-servo added a commit that referenced this issue May 1, 2016
Implement History back/forward APIs

Rebasing @tomjakubowski's work.
I still need to add a test for this (which is why this was not merged before).

Original PR:
> Unfortunately, not many once-failing tests are now expected to pass; the
> history interface tests often depend on things like History#go working.
>
> Closes #5670

<!-- 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/10956)
<!-- Reviewable:end -->
@cbrewster
Copy link
Member

@cbrewster cbrewster commented May 2, 2016

I am picking this one back up. I am implementing pushState so the history tests can be enabled

@frewsxcv frewsxcv added the C-assigned label May 2, 2016
@cbrewster cbrewster mentioned this issue May 3, 2016
18 of 24 tasks complete
cbrewster added a commit to cbrewster/servo that referenced this issue May 3, 2016
Unfortunately, not many once-failing tests are now expected to pass; the
history interface tests often depend on things like History#go working.

Closes servo#5670
@cbrewster cbrewster mentioned this issue Jul 22, 2016
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Jul 22, 2016
History interface Go, Back, and Forward

<!-- Please describe your changes on the following line: -->
r? @asajeffrey

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

<!-- Either: -->
- [X] 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. -->

implement go, forward, back
bors-servo added a commit that referenced this issue Jul 22, 2016
History interface Go, Back, and Forward

<!-- Please describe your changes on the following line: -->
r? @asajeffrey

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

<!-- Either: -->
- [X] 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. -->

implement go, forward, back

<!-- 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/12552)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 22, 2016
History interface Go, Back, and Forward

<!-- Please describe your changes on the following line: -->
r? @asajeffrey

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

<!-- Either: -->
- [X] 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. -->

implement go, forward, back

<!-- 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/12552)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 22, 2016
History interface Go, Back, and Forward

<!-- Please describe your changes on the following line: -->
r? @asajeffrey

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

<!-- Either: -->
- [X] 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. -->

implement go, forward, back

<!-- 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/12552)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

8 participants
You can’t perform that action at this time.