feat: extend navigationHistory API #42014
Conversation
vitekcerny
force-pushed
the
extend-navigationHistory
branch
from
e90a424
to
6235bee
Compare
|
@alicelovescake would you be able to help review this? |
| if (history_item.Get("url", &url)) | ||
| entry->SetURL(GURL(url)); | ||
| if (history_item.Get("title", &title)) | ||
| entry->SetTitle(title); |
There was a problem hiding this comment.
My concern here is that ReplaceHistory may not correctly handle other important properties of the navigation entries, potentially leading to unexpected behavior or inconsistencies in the navigation history. In Electron's version of NavigationEntry, we are just exposing url and title. But Chromium's NavigationEntry has page state with scroll positions and other client side info/ referrer url and other properties that might introduce inconsistencies if we just change the url and title. What do you think?
| @@ -2492,6 +2492,58 @@ content::NavigationEntry* WebContents::GetNavigationEntryAtIndex( | |||
| return web_contents()->GetController().GetEntryAtIndex(index); | |||
| } | |||
|
|
|||
| bool WebContents::DeleteNavigationEntryAtIndex(int index) { | |||
| if (index < 0 || index >= GetHistoryLength()) | |||
There was a problem hiding this comment.
You can use CanGoToIndex
| // If the history is empty, it contains only one entry and that is | ||
| // "InitialEntry" | ||
| if (history_length == 1 && controller.GetEntryAtIndex(0)->IsInitialEntry()) | ||
| return std::vector<content::NavigationEntry*>(); |
| getEntryAtIndex: this._getNavigationEntryAtIndex.bind(this), | ||
| deleteEntryAtIndex: this._deleteNavigationEntryAtIndex.bind(this), | ||
| getHistory: this._getHistory.bind(this), | ||
| replaceHistory: this._replaceHistory.bind(this) |
There was a problem hiding this comment.
What do you think of renaming to just navigationHistory.get() and navigationHistory.replace() to reduce redundancy?
| return false; | ||
|
|
||
| controller.PruneAllButLastCommitted(); | ||
| controller.Restore(index, content::RestoreType::kRestored, &history); |
There was a problem hiding this comment.
My gut says this API is a security concern in general and correct me if you disagree. We are allowing the override of the navigation history without any input validation. Anyone could potentially inject malicious scripts the the url or page titles. We are also circumventing user's privacy/ tracking mechanisms if they didn't explicitly intend to navigate to a particular entry.
vitekcerny
force-pushed
the
extend-navigationHistory
branch
2 times, most recently
from
a80ffb0
to
3ec3bf4
Compare
vitekcerny
force-pushed
the
extend-navigationHistory
branch
from
3ec3bf4
to
1d8afbc
Compare
Description of Change
This PR builds on #41577 and adds 3 more functions to
webContents.navigationHistory.The motivation is to provide an API for developers of browser-type applications.
New API:
navigationHistory.deleteEntryAtIndex(index)- Deletes the navigation entry at the given index.navigationHistory.getHistory()- Returns webContents complete history.navigationHistory.replaceHistory(newHistory, index)- Replaces current history withnewHistoryand sets the active index toindex.This enables browser-type applications to save (
getHistory) and restore (replaceHistory) browsing sessions.Also resolves these feature requests: #33899 #41250
Checklist
npm testpassesRelease Notes
Notes: Extended
navigationHistoryAPI with 3 new functions to allow restoring browsing sessions and for better history management.