Ledger beta integration #3195
Ledger beta integration #3195
Conversation
|
@bridiver @diracdeltas need you reviewing too pls (for whichever parts you did not write) |
| var setLocation = () => { | ||
| var duration, publisher | ||
|
|
||
| if (location !== currentLocation) console.log('new location: ' + location) |
There was a problem hiding this comment.
@mrose17 can we remove this console.log? URLs from private browsing should not be logged
There was a problem hiding this comment.
Sure! Feel free to remove it... Just there for debug!
/mtr
On Mon, Aug 15, 2016 at 4:51 PM -0700, "yan zhu (@bcrypt)" notifications@github.com wrote:
In app/ledger.js:
- return data
+}
+/*
- * publisher utilities
- */
+
+var currentLocation = 'NOOP'
+var currentTimestamp = underscore.now()
+
+var visit = (location, timestamp) => {- var setLocation = () => {
- var duration, publisher
- if (location !== currentLocation) console.log('new location: ' + location)
@mrose17 can we remove this console.log? URLs from private browsing should not be logged
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
||
| if (isNaN(amount) || (amount <= 0)) return | ||
|
|
||
| underscore.extend(bravery.fee, { amount: amount }) |
There was a problem hiding this comment.
does this expect amount to be in whatever the specified currency is, or is it just USD?
There was a problem hiding this comment.
i just changed it so now this code is correct as long as amount corresponds to the currency in ledger-state
better updateLedgerInfo() invocations
As of brave/node-anonize2-relic-emscripten#1 we shouldn't need this anymore. Auditors: @mrose17
# Conflicts: # CHANGELOG.md # app/extensions.js # app/extensions/brave/locales/en-US/preferences.properties # app/ledger.js # app/menu.js # app/sessionStore.js # docs/state.md # js/about/preferences.js # js/actions/windowActions.js # js/components/frame.js # js/components/main.js # js/components/navigationBar.js # js/components/tab.js # js/components/tabs.js # js/components/tabsToolbar.js # js/components/urlBar.js # js/components/urlBarSuggestions.js # js/constants/appConfig.js # js/constants/appConstants.js # js/constants/messages.js # js/constants/settings.js # js/contextMenus.js # js/data/searchProviders.js # js/flash.js # js/state/frameStateUtil.js # js/stores/eventStore.js # js/stores/windowStore.js # less/variables.less # test/unit/state/frameStateUtilTest.js
phase 2 fix in process…
diracdeltas
force-pushed
the
ledger-delta-rebase
branch
from
613eef9 to
20944c8
Compare
| @@ -111,6 +113,7 @@ module.exports = { | |||
| 'shutdown.clear-cache': false, | |||
| 'shutdown.clear-all-site-cookies': false | |||
| }, | |||
| defaultFavicon: 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYAAADgdz34AAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAA0xpVFh0WE1MOmNvbS5hZG9iZS54bXAAAAAAADw/eHBhY2tldCBiZWdpbj0i77u/IiBpZD0iVzVNME1wQ2VoaUh6cmVTek5UY3prYzlkIj8+IDx4OnhtcG1ldGEgeG1sbnM6eD0iYWRvYmU6bnM6bWV0YS8iIHg6eG1wdGs9IkFkb2JlIFhNUCBDb3JlIDUuNi1jMTExIDc5LjE1ODMyNSwgMjAxNS8wOS8xMC0wMToxMDoyMCAgICAgICAgIj4gPHJkZjpSREYgeG1sbnM6cmRmPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5LzAyLzIyLXJkZi1zeW50YXgtbnMjIj4gPHJkZjpEZXNjcmlwdGlvbiByZGY6YWJvdXQ9IiIgeG1sbnM6eG1wTU09Imh0dHA6Ly9ucy5hZG9iZS5jb20veGFwLzEuMC9tbS8iIHhtbG5zOnN0UmVmPSJodHRwOi8vbnMuYWRvYmUuY29tL3hhcC8xLjAvc1R5cGUvUmVzb3VyY2VSZWYjIiB4bWxuczp4bXA9Imh0dHA6Ly9ucy5hZG9iZS5jb20veGFwLzEuMC8iIHhtcE1NOkRvY3VtZW50SUQ9InhtcC5kaWQ6MUNFNTM2NTcxQzQyMTFFNjhEODk5OTY1MzJCOUU0QjEiIHhtcE1NOkluc3RhbmNlSUQ9InhtcC5paWQ6MUNFNTM2NTYxQzQyMTFFNjhEODk5OTY1MzJCOUU0QjEiIHhtcDpDcmVhdG9yVG9vbD0iQWRvYmUgUGhvdG9zaG9wIENDIDIwMTUgKE1hY2ludG9zaCkiPiA8eG1wTU06RGVyaXZlZEZyb20gc3RSZWY6aW5zdGFuY2VJRD0iYWRvYmU6ZG9jaWQ6cGhvdG9zaG9wOjUxZDUzZDBmLTYzOWMtMTE3OS04Yjk3LTg3Y2M5YTUyOWRmMSIgc3RSZWY6ZG9jdW1lbnRJRD0iYWRvYmU6ZG9jaWQ6cGhvdG9zaG9wOjUxZDUzZDBmLTYzOWMtMTE3OS04Yjk3LTg3Y2M5YTUyOWRmMSIvPiA8L3JkZjpEZXNjcmlwdGlvbj4gPC9yZGY6UkRGPiA8L3g6eG1wbWV0YT4gPD94cGFja2V0IGVuZD0iciI/PmF3+n4AAAAoSURBVHja7M1BAQAABAQw9O98SvDbCqyT1KepZwKBQCAQCAQ3VoABAAu6Ay00hnjWAAAAAElFTkSuQmCC', | |||
There was a problem hiding this comment.
I'd prefer if we don't store a base64 encoded image url and instead make this an actual image and put a link here
There was a problem hiding this comment.
@bbondy - i'd prefer that as well. however, the link has to be stored locally for privacy considerations.
so, what is required is a small API that allows ledger.js to store/update an image locally and get back the associated URL...
There was a problem hiding this comment.
app/ledger.js can overwrite the file when it needs to update the image
There was a problem hiding this comment.
if someone will tell me what the API is, i can modify ledger.js ... but (like so many things!), i don't know the API...
There was a problem hiding this comment.
@bbondy @diracdeltas tells me i misunderstood what you were asking... sorry! i've got a PR that fixes the default icon issue...
|
So much cleaner than last time I looked! Thanks for all the great work here. |
|
I know this is already merged so I'm a little late to the party, but I didn't see any tests. Just a reminder that we made a decision a while ago to require tests for all new features so we should add some to cover this as a follow-up |
|
Thanks! |
Auditors: @bbondy