Skip to content

Webview navigation history getter #72

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
Open
janRucka wants to merge 3 commits into
base: nw22
Choose a base branch
from
Open

Webview navigation history getter #72

janRucka wants to merge 3 commits into from

Conversation

@janRucka
Copy link
Contributor

@janRucka janRucka commented Mar 7, 2017 •
edited
Loading

This was referenced Mar 7, 2017
Closed
Closed
@rogerwang rogerwang force-pushed the nw21 branch 7 times, most recently from 65c8d20 to 482e210 Compare March 12, 2017 13:59
rogerwang
rogerwang requested changes Mar 13, 2017
View reviewed changes
extensions/renderer/resources/guest_view/web_view/web_view.js
baseUrlForDataUrl, currentEntryIndex, entryCount,
processId, url, isTopLevel) {
processId, url, isTopLevel, pagesHistory) {
this.baseUrlForDataUrl = baseUrlForDataUrl;
Copy link
Member

@rogerwang rogerwang Mar 13, 2017

Choose a reason for hiding this comment

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

This would send the whole history from browser to renderer process on every navigation, which could hurt performance. Is there any better way to do this?

Copy link
Contributor Author

@janRucka janRucka Mar 13, 2017

Choose a reason for hiding this comment

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

It’s true it is send with every navigation. But I consider it to be insignificant. After all it is just few strings. I guess it would be possible to make changes just in javascript code. Even without ‘pagesHistory’ you get ‘url’ and ‘currentEntryIndex’ from which you can create array of everything you need. But then all logic needs to be there. E.g. if you go back and then navigate to other page you should delete some record.

Copy link
Member

@rogerwang rogerwang Mar 13, 2017

Choose a reason for hiding this comment

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

The history will grow longer and longer. So eventually the data will grow big enough to hurt performance. And there is a size limit of IPC message. Should find another way to do this.

Copy link
Contributor Author

@janRucka janRucka Mar 13, 2017

Choose a reason for hiding this comment

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

I don’t know about limit for IPC message. But it will not grow longer and longer. For navigation history there is some limit. When you have about 30 or so items it will remove the oldest. Or at least it seems to be the case after quick test.

@rogerwang rogerwang force-pushed the nw21 branch 8 times, most recently from 426fc70 to f78c16a Compare March 21, 2017 05:50
@rogerwang rogerwang force-pushed the nw21 branch 3 times, most recently from da3929b to b060cdf Compare March 28, 2017 04:57
@janRucka
Copy link
Contributor Author

janRucka commented Mar 29, 2017

Updated to current nw21.
The session history is really limited see "const int kMaxSessionHistoryEntries = 50;" here:
https://github.com/nwjs/chromium.src/blob/nw20/content/public/common/content_constants.cc

so it will not grow longer and longer. In some cases, even 50 entries could be quite big. But we never encountered any issue related to this.

Regarding the performance, I don't think it is significant. Currently we send everything at once synchronously. This suit us well since we use it to show session history. We could send it one at a time asynchronously but I guess that would be even more performance demanding. Other way could be to restrict it more e.g. to last 10 entries. On the other hand, if use case would be to get just one entry at a time it seems to be Ok.

@rogerwang rogerwang force-pushed the nw21 branch 5 times, most recently from 9396d7f to 8adf1c9 Compare April 4, 2017 13:14
@rogerwang rogerwang force-pushed the nw21 branch 3 times, most recently from f1e4ded to 4fd96a3 Compare April 6, 2017 01:18
@rogerwang rogerwang force-pushed the nw21 branch 3 times, most recently from 27692f7 to f854061 Compare April 12, 2017 13:01
chrome-release-bot and others added 3 commits May 1, 2017 11:54
Publish DEPS for Chromium 58.0.3029.96
ab55a22
@rogerwang
nwjs master
5e7b9cd
@janRucka
Webview navigation history getter.
a6710b6 
Webview now has:
-getPagesHistory function, returning array of URLs, titles and favicons of pages in history. Titles and favicons are not known for current page because array is created before page is fully loaded.
-getCurrentHistoryIndex function, returning current history index
@janRucka janRucka force-pushed the historyGetter21 branch from 1884f31 to a6710b6 Compare May 5, 2017 13:55
@janRucka janRucka changed the base branch from nw21 to nw22 May 5, 2017 13:55
@janRucka janRucka mentioned this pull request May 5, 2017
Open
@rogerwang rogerwang force-pushed the nw22 branch 4 times, most recently from 65748ff to 90fd209 Compare May 10, 2017 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rogerwang rogerwang rogerwang requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@janRucka @rogerwang