8000 feat: extend navigationHistory API by vitekcerny · Pull Request #42014 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: extend navigationHistory API #42014

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Aug 19, 2024
Merged

feat: extend navigationHistory API #42014

merged 6 commits into from
Aug 19, 2024

Conversation

vitekcerny
Copy link
Contributor
@vitekcerny vitekcerny commented May 1, 2024 •
edited by jkleinsc
Loading

Description of Change

This PR builds on #41577 and adds 2 more functions to webContents.navigationHistory.
The motivation is to provide an API for developers of browser-type applications.

New API:

  • navigationHistory.removeEntryAtIndex(index) - Removes the navigation entry at the given index.
  • navigationHistory.getAllEntries() - Returns webContents complete history.

This enables browser-type applications to save (getAllEntries) browsing sessions.
Also partially resolves these feature requests: #33899 #41250

Checklist

Release Notes

Notes: Extended navigationHistory API with 2 new functions for better history management.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label May 1, 2024
@vitekcerny vitekcerny force-pushed the extend-navigationHistory branch from e90a424 to 6235bee Compare May 1, 2024 12:19
@vitekcerny vitekcerny marked this pull request as ready for review May 1, 2024 13:02
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label May 8, 2024
@codebytere codebytere requested a review from a team May 13, 2024 08:30
@codebytere codebytere added the semver/minor backwards-compatible functionality label May 13, 2024
@codebytere
Copy link
Member

@alicelovescake would you be able to help review this?

codebytere
codebytere reviewed May 13, 2024
View reviewed changes
Copy link
Member
@codebytere codebytere left a comment •
edited
Loading

Choose a reason for hiding this comment

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

At a high level, without taking a deeper look at the implementation details, I think these methods will be useful. I'll try to more thoroughly review sometime soon, but we'll need to add some tests for these changes regardless!

alicelovescake
alicelovescake reviewed May 13, 2024
View reviewed changes
alicelovescake
alicelovescake reviewed May 13, 2024
View reviewed changes
alicelovescake
alicelovescake reviewed May 13, 2024
View reviewed changes
alicelovescake
alicelovescake reviewed May 13, 2024
View reviewed changes
alicelovescake
alicelovescake reviewed May 13, 2024
View reviewed changes
@vitekcerny vitekcerny force-pushed the extend-navigationHistory branch 3 times, most recently from 3ec3bf4 to 1d8afbc Compare May 19, 2024 15:25
samuelmaddock
samuelmaddock reviewed Jun 12, 2024
View reviewed changes
@vitekcerny vitekcerny force-pushed the extend-navigationHistory branch 2 times, most recently from 309b8b1 to d6b8cb3 Compare June 12, 2024 19:39
@vitekcerny
Copy link
Contributor Author
vitekcerny commented Jun 12, 2024 •
edited
Loading

Sorry for my late reply. I now work for a different company, and I no longer have access to the building infrastructure. On the topic of the replaceHistory method, I agree with @alicelovescake. Chromium internal implementation of navigation entry is more complicated and contains many properties. But we are setting only title and url, leaving other properties at default values. Here is the constructor of the NavigationEntryImpl: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_entry_impl.cc;l=392

So my suggestion would be to remove the replace method here, so I don't block this pull request. Maybe someone will come up with a better way to recreate NavigationEntries. What do you think @samuelmaddock ?

samuelmaddock
samuelmaddock requested changes Jun 14, 2024
View reviewed changes
Copy link
Member
@samuelmaddock samuelmaddock left a comment

Choose a reason for hiding this comment

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

I left some comments about aligning naming with existing APIs. Following these changes, happy to give this a lgtm!

So my suggestion would be to remove the replace method here, so I don't block this pull request.

I think this is a reasonable approach. What's proposed in this PR is not far off, so I'd welcome finding a new design in a future PR. 👍

@alicelovescake
Copy link
Member
alicelovescake commented Jun 14, 2024 •
edited
Loading

I left some comments about aligning naming with existing APIs. Following these changes, happy to give this a lgtm!

So my suggestion would be to remove the replace method here, so I don't block this pull request.

I think this is a reasonable approach. What's proposed in this PR is not far off, so I'd welcome finding a new design in a future PR. 👍

Yeah agreed that this PR looks great after removing the replace method. We need a way to construct Chromium's definition of a NavigationEntry before can pass a correct argument to void NavigationControllerImpl::Restore.

@vitekcerny Do you mind adding tests to the existing test suite for navigationHistory?

@vitekcerny vitekcerny force-pushed the extend-navigationHistory branch 2 times, most recently from aa9c902 to 2d50cf7 Compare June 30, 2024 19:48
@alicelovescake alicelovescake mentioned this pull request Jul 1, 2024
Closed
2 tasks
@vitekcerny
Copy link
Contributor Author

I renamed the methods to be consistent with the rest of the webContents API. But I need help with tests. I don't have much experience with javascript unit testing (I’m a C++ guy). Would you mind writing those tests @alicelovescake ? Or if you refer me to the documentation, I will try to write them myself.

@alicelovescake
Copy link
Member

I renamed the methods to be consistent with the rest of the webContents API. But I need help with tests. I don't have much experience with javascript unit testing (I’m a C++ guy). Would you mind writing those tests @alicelovescake ? Or if you refer me to the documentation, I will try to write them myself.

Sure! Here are tests that you can add based on the existing format:
Note: add these tests below line 569 and above line 709 in api-web-contents-spec.ts

  1. removeEntryAtIndex
describe('navigationHistory.removeEntryAtIndex(index) API', () => {
  it('should remove a navigation entry given a valid index', async () => {
    await w.loadURL(urlPage1);
    await w.loadURL(urlPage2);
    await w.loadURL(urlPage3);
    const initialLength = w.webContents.navigationHistory.length();
    const wasRemoved = w.webContents.navigationHistory.removeEntryAtIndex(1); // Attempt to remove the second entry
    const newLength = w.webContents.navigationHistory.length();
    expect(wasRemoved).to.be.true();
    expect(newLength).to.equal(initialLength - 1);
  });

  it('should not remove the current active navigation entry', async () => {
    await w.loadURL(urlPage1);
    await w.loadURL(urlPage2);
    const activeIndex = w.webContents.navigationHistory.getActiveIndex();
    const wasRemoved = w.webContents.navigationHistory.removeEntryAtIndex(activeIndex);
    expect(wasRemoved).to.be.false();
  });

  it('should return false given an invalid index larger than history length', async () => {
    await w.loadURL(urlPage1);
    const wasRemoved = w.webContents.navigationHistory.removeEntryAtIndex(5); // Index larger than history length
    expect(wasRemoved).to.be.false();
  });

  it('should return false given an invalid negative index', async () => {
    await w.loadURL(urlPage1);
    const wasRemoved = w.webContents.navigationHistory.removeEntryAtIndex(-1); // Negative index
    expect(wasRemoved).to.be.false();
  });
});
  1. getAllEntries
describe('navigationHistory.getAllEntries() API', () => {
  it('should return all navigation entries as an array of NavigationEntry objects', async () => {
    await w.loadURL(urlPage1);
    await w.loadURL(urlPage2);
    await w.loadURL(urlPage3);
    const entries = w.webContents.navigationHistory.getAllEntries();
    expect(entries.length).to.equal(3);
    expect(entries[0]).to.deep.equal({ url: urlPage1, title: 'Page 1' });
    expect(entries[1]).to.deep.equal({ url: urlPage2, title: 'Page 2' });
    expect(entries[2]).to.deep.equal({ url: urlPage3, title: 'Page 3' });
  });

  it('should return an empty array when there is no navigation history', async () => {
    const entries = w.webContents.navigationHistory.getAllEntries();
    expect(entries.length).to.equal(0);
  });
});

If you have e tools, run e build then e test api-web-contents-spec.ts to make sure the new tests passes locally.
Good luck and let me know if you run into any issues!

@samuelmaddock
Copy link
Member

Adding a wip label until additional tests can be added. Feel free to ping @electron/wg-api for another round of review 🙇

@vitekcerny
Copy link
Contributor Author

@vitekcerny was there an error you ran into? Sometimes I encounter an error when installing test dependencies. If that's the case, you may need to run e build node:headers first (using build-tools).

Here is my error log. But I believe this error is only in my environment. If you try to run the tests yourself, or if you run them in CI, they should pass.

Triggering runners: main

Running: Main process specs

An error occurred while running the spec runner
electron/spec/api-utility-process-spec.ts(121,22): error TS2339: Property 'type' does not exist on type 'Details'.
electron/spec/api-utility-process-spec.ts(122,22): error TS2339: Property 'serviceName' does not exist on type 'Details'.
electron/spec/api-utility-process-spec.ts(123,22): error TS2339: Property 'name' does not exist on type 'Details'.
electron/spec/api-utility-process-spec.ts(124,22): error TS2339: Property 'reason' does not exist on type 'Details'.
electron/spec/api-utility-process-spec.ts(130,22): error TS2339: Property 'type' does not exist on type 'Details'.
electron/spec/api-utility-process-spec.ts(131,22): error TS2339: Property 'serviceName' does not exist on type 'Details'.
electron/spec/api-utility-process-spec.ts(132,22): error TS2339: Property 'name' does not exist on type 'Details'.
electron/spec/api-utility-process-spec.ts(133,22): error TS2339: Property 'reason' does not exist on type 'Details'.

Electron tests failed with code 0x1.

@vitekcerny vitekcerny force-pushed the extend-navigationHistory branch from 3979309 to c1ea71c Compare August 3, 2024 18:27
@vitekcerny
Copy link
Contributor Author

I managed to run the tests. I just had to comment out a few lines that were causing the errors above (irrelevant for this pr). Anyway, the tests are passing, so I thing this pull request is ready for review @samuelmaddock

samuelmaddock
samuelmaddock approved these changes Aug 12, 2024
View reviewed changes
Copy link
Member
@samuelmaddock samuelmaddock left a comment

Choose a reason for hiding this comment

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

API LGTM

@itsananderson
Copy link
Member

Notes: Extended navigationHistory API with 3 new functions to allow restoring browsing sessions and for better history management.

It sounds like we're skipping the replaceHistory API for now. Can you update the release notes to match what's being added in this PR?

alicelovescake
alicelovescake approved these changes Aug 12, 2024
View reviewed changes
Copy link
Member
@alicelovescake alicelovescake left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the effort to see this through!

jkleinsc
jkleinsc approved these changes Aug 12, 2024
View reviewed changes
Copy link
Member
@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

API LGTM

@codebytere
Copy link
Member

@vitekcerny this repo requires commits to be signed. Once that's done, i think we can get this merged :)

@vitekcerny vitekcerny force-pushed the extend-navigationHistory branch 3 times, most recently from bd33864 to 4f1a2f2 Compare August 17, 2024 09:43
@vitekcerny vitekcerny force-pushed the extend-navigationHistory branch from 4f1a2f2 to d9304e2 Compare August 17, 2024 09:45
@vitekcerny
Copy link
Contributor Author

The commits are now signed and the release notes are edited @codebytere

@jkleinsc jkleinsc merged commit 1896755 into electron:main Aug 19, 2024
48 of 49 checks passed
@welcome Welcome
Copy link
welcome bot commented Aug 19, 2024

Congrats on merging your first pull request! 🎉🎉🎉

@release-clerk
Copy link
release-clerk bot commented Aug 19, 2024

Release Notes Persisted

Extended navigationHistory API with 2 new functions for better history management.

@ckerr ckerr mentioned this pull request Sep 23, 2024
Closed
@samuelmaddock samuelmaddock mentioned this pull request Feb 10, 2025
Merged
5 tasks
9846
LeUser111 pushed a commit to LeUser111/electron that referenced this pull request Feb 11, 2025
@vitekcerny @LeUser111
feat: extend navigationHistory API (electron#42014)
0dd5dd7 
* feat: extend navigationHistory API

* refactor: simplify index checking

* refactor: rename 'getHistory' and 'replaceHistory' methods of navigationHistory

* refactor: rename delete*() methods to remove*()

* feat: remove navigationHistory.replaceHistory()

* tests: add tests for removeEntryAtIndex and getAllEntries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkleinsc jkleinsc jkleinsc approved these changes

@samuelmaddock samuelmaddock samuelmaddock approved these changes

@alicelovescake alicelovescake alicelovescake approved these changes

@codebytere codebytere Awaiting requested review from codebytere

Assignees
No one assigned
Labels
api-review/approved ✅ no-backport semver/minor backwards-compatible functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vitekcerny @codebytere @alicelovescake @samuelmaddock @itsananderson @jkleinsc
0