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

Move VIEW_HISTORY_MEMORY constant to view_history.js #6587

Merged
merged 2 commits into from
Nov 2, 2015

Conversation

timvandermeij
Copy link
Contributor

Currently this constant is present in viewer.js, but it is not used there at all. Instead, it is used in view_history.js where we have a global for it. We might as well move the constant to view_history.js as that is the only place where it is used, thereby removing a global and an unused constant from viewer.js.

Currently this constant is present in `viewer.js`, but it is not used there at all. Instead, it is used in `view_history.js` where we have a global for it. We might as well move the constant to `view_history.js` as that is the only place where it is used, thereby removing a global and an unused constant from `viewer.js`.
@timvandermeij
Copy link
Contributor Author

It took a while, but I finally had a bit of time to create a new patch. @Snuffleupagus Could you review this one if you have time?

@Snuffleupagus
Copy link
Collaborator

If you're touching this code anyway, I'm wondering if we should take it a bit further by making the size configurable when initializing ViewHistory? E.g. something like this:

  • in view_history.js, change function ViewHistory(fingerprint) { to function ViewHistory(fingerprint, size) {.
  • define var cacheSize = size || DEFAULT_VIEW_HISTORY_SIZE; (if we're doing this, a slight rename of the constant might be in order), and compare the length against that value at view_history.js#L42.

@timvandermeij How do you feel about the above?

@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Nov 2, 2015

From: Bot.io (Linux)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/946f6bfc082e149/output.txt

@timvandermeij
Copy link
Contributor Author

@Snuffleupagus I have implemented that in the second commit. It seems like a good addition to me too. Note that I have kept the commits separate on purpose as the first one is just moving code and the second one is adding new functionality. For the sake of consistency I did not use var cacheSize, but this.cacheSize instead as that has also been done for the fingerprint in the code (and it seems good to make this a class variable).

Could you check this patch again?

Snuffleupagus added a commit that referenced this pull request Nov 2, 2015
Move VIEW_HISTORY_MEMORY constant to `view_history.js`
@Snuffleupagus Snuffleupagus merged commit cbce05b into mozilla:master Nov 2, 2015
@Snuffleupagus
Copy link
Collaborator

Looks good, thanks for the clean-up!

@timvandermeij timvandermeij deleted the view-history-memory branch November 3, 2015 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants