Ledger does not track windows created before the wallet

[alexwykoff]

Did you search for similar issues before submitting this one?
Yes

Describe the issue you encountered:
I opened a window and visited a site. I then opened preferences and created a wallet. I then returned to the original window and viewed sites. The ledger was never populated.

This is also true for sites which were visited in a new(post-wallet creation) window and then visited with an older(pre-wallet creation) window.

Expected behavior:
The ledger should work for any open Brave window.

• Platform (Win7, 8, 10? macOS? Linux distro?):
  Tested on Windows 7 x64
• Brave Version:
  Discovered in 0.12.2 RC4, confirmed in 0.12.1
• Steps to reproduce:
  1. With a clean install, open a window and visit a site.
  2. In a new window, go to preferences and create a wallet.
  3. In the original window, navigate sites to populate the ledger.
  4. Check the ledger.
• Screenshot if needed:
  
• Any related issues:

[mrose17]

Design intent. Will not fix.

[bradleyrichter]

@mrose17 I think any new browsing done after the Brave Payments switch is enabled should count towards the ledger list. So for currently open tabs, reactivating the tab would count as a visit with a fresh timer.

Is this possible with the current ledger implementation?

[mrose17]

if brave payments is not enabled, we don't keep track of the user's publisher history.

if the user turns it off, visits some sites, and turns it on, then whatever they did while brave payments was off is not recorded.

that is by design.

[bradleyrichter]

I think the devil is in the details on this one and could be renamed. Or we can create a new issue.

What I read is that User was browsing in various open tabs. Then finds brave payments and enables it.

They keep browsing in those tabs that were open already but now they are viewing the same or new content in those tabs.

1. If those pages are not added to ledger, it will feel broken.
2. User would only expect to add previous pages that became "new" because they were now interacted with after enabling ledger.

[mrose17]

you have to visit a tab and then leave it while brave payments is on in order for it to be counted.

that seems pretty consistent to me. i suppose we could tell folks "Enable Brave Payments (requires restart)" if there is that much of an issue.

[alexwykoff]

Reopening and putting on 0.12.10.

[mrose17]

@alexwykoff @bbondy @luixxiul @srirambv - i need volunteers!

build from this branch -- https://github.com/brave/browser-laptop/tree/keep-track-of-publishers -- and see if you still see sluggishness. thanks!

[bbondy]

Sorry to be the bearer of bad news but I think there's still a perf problem with this.

My steps to notice the difference is:
i) load 24 tabs (I use a few different sites repeated, a fast way to do it is by command+click on bookmark toolbar items)
ii) wait for them all to be loaded fully
iii) Click on a tab to do a tab switch

On master:
it switches right away

With this branch:
It takes about a full second to do a tab switch.

[bbondy]

Hey @mrose just in case I was crazy, I verified if this is a problem again from scratch. I tested the steps below 16 times total. I reproduced the slow down 8 times in your branch, and 8 times not reproduced in master. I switched between the 2 branches on each test run. I documented exact steps I took.

There is definitely a bigger than linear slow down as the number of tabs increase. Something like O(N^2) on tab switches whereas before it was O(1). Not only tab switches but the whole browser gets sluggish. But I suggest to notice with tab switches since it's easy to notice with that.

We can't merge this in because we'd bleed users. I'm going to move it to 0.12.11 to give more time to work on it.

1. Start with a fresh profile (npm run clean-session-store)
2. Bookmark brave.com
3. Show your bookmark toolbar
4. Open up 5 pages of tabs by command+clicking on brave.com on the bookmark toolbar
5. Wait for all the tabs to load fully
6. Click on a tab to change

On master:
Tab changes go instantly, even if you double the amount of tab pages to 10.

On keep-track-of-publishers:
Tab changes take about 2s with 5 tab pages. With 10 tab pages, it takes over 10s to do a tab switch.

[mrose]

Brian - memory says someone (likely you) has done this before.
The mrose you want isn't me.

thanks,
.m

ps looks like an issue on that branch!

On Wed, Nov 16, 2016 at 10:46 PM, Brian R. Bondy notifications@github.com
wrote:

Hey @mrose https://github.com/mrose just in case I was crazy, I
verified if this is a problem again from scratch. I tested the steps below
16 times total. I reproduced the slow down 8 times in your branch, and 8
times not reproduced in master. I switched between the 2 branches on each
test run. I documented exact steps I took.

There is definitely a bigger than linear slow down as the number of tabs
increase. Something like O(N^2) on tab switches whereas before it was O(1).
Not only tab switches but the whole browser gets sluggish. But I suggest to
notice with tab switches since it's easy to notice with that.

We can't merge this in because we'd bleed users. I'm going to move it to
0.12.11 to give more time to work on it.

1. Start with a fresh profile (npm run clean-session-store)
2. Bookmark brave.com
3. Show your bookmark toolbar
4. Open up 5 pages of tabs by command+clicking on brave.com on the
   bookmark toolbar
5. Wait for all the tabs to load fully
6. Click on a tab to change

On master:
Tab changes go instantly, even if you double the amount of tab pages to 10.

On keep-track-of-publishers:
Tab changes take about 2s with 5 tab pages. With 10 tab pages, it takes
over 10s to do a tab switch.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4274 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AACuIkd2HE74ggvHeovGvik-gycNNqS6ks5q-84WgaJpZM4KFyiL
.

Mitchell Rose
Cog Data Systems, llc
v. 212.362.6362

[bbondy]

Sorry about that, it was a different Brian last time though, we have many.
20375fb

[luixxiul]

Test plan: #5721

I'm removing the labels which have been set before #5721 was merged. Please check this again.