Yt twitch integration fixes #64
Conversation
| // } | ||
| // } | ||
| // } | ||
| void getTwitchParts(const std::string& query, std::vector<std::map<std::string, std::string>>& parts) { |
There was a problem hiding this comment.
I added params into OnXHRLoad so we could parse the query string using the chromium libs. Can we use that here? Seems safer to use the chromium parsing and decode
There was a problem hiding this comment.
https://github.com/brave/brave-core/blob/rewards/components/brave_rewards/browser/rewards_service_impl.cc#L262
I forgot to delete the TODO after implementing
There was a problem hiding this comment.
or add it to OnPostData if we're parsing from there?
There was a problem hiding this comment.
the post data is based64 value and it's not really a query, it's a json object like that [{"event":"experiment_branch","properties":{"platform":"mobile_web","language":"en-us","gitHash":"964ae46c471c406f935380d04b8d8013904d88de","location":"channel","time":1535684252.833,"experiment_id":"aa19fae8-0e00-4922-8f1f-6699cc654671","experiment_name":"mobile_web_singapore_first_byte","experiment_group":"control","app_version":"964ae46c471c406f935380d04b8d8013904d88de","browser":"Mozilla/5.0 (Linux; Android) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.91 Mobile Safari/537.36 Brave","device_id":"ZTjPk6OrfHoCSCzSOcJ5hWwZgwExewqn","domain":"m.twitch.tv","host":"https://m.twitch.tv","app_session_id":"sUlnF57mQVSgT7z2Ewid1tlnAkX8I9d5","url":"https://m.twitch.tv/sypherpk","viewport_height":604,"viewport_width":412,"device_orientation":"portrait"}}] and there are can be several events in one upload data
There was a problem hiding this comment.
I see, so you're splitting out the JSON chunks and then parsing?
There was a problem hiding this comment.
it's just a one big chunk that's actually an array of objects, in most cases there is just one element in that array.
| const uint64_t& current_time) = 0; | ||
| const std::string& first_party_url, | ||
| const std::string& referrer, | ||
| const VisitData& visit_data) = 0; |
There was a problem hiding this comment.
what goes in this VisitData that is different from the OnLoad call?
There was a problem hiding this comment.
or is this because we're considering this an independent visit?
There was a problem hiding this comment.
yep we consider it as independent visit
| media_publisher_info->twitchEventInfo_.status_ = getTwitchStatus(oldInfo, twitchEventInfo); | ||
|
|
||
| ledger_->SetMediaPublisherInfo(realDuration, std::move(media_publisher_info), updated_visit_data, | ||
| std::bind(&onVisitSavedDummy, _1, _2)); |
There was a problem hiding this comment.
do we want to get notified if this doesn't save successfully?
There was a problem hiding this comment.
maybe a TODO for some error handling?
There was a problem hiding this comment.
probably it's just has to be removed, I'm going to remove it
There was a problem hiding this comment.
better to keep it probably as it goes the same way right now as other publishers, the same function call
| ledger::Result result, | ||
| std::unique_ptr<ledger::MediaPublisherInfo> info) { | ||
| SaveMediaVisit(visit_data, duration); | ||
| callback(result, std::move(info)); |
There was a problem hiding this comment.
SaveMediaVisit is async so shouldn't this run when it actually completes?
There was a problem hiding this comment.
the callback is a callback above that we want to remove onVisitSavedDummy
There was a problem hiding this comment.
right, but either way don't we want to wait until SaveMediaVisit actually completes to run the callback? SaveMediaVisit might fail and even if it succeeds, it hasn't succeeded yet
There was a problem hiding this comment.
I guess it's a general problem with saveVisit because it doesn't currently have any way to report success/failure so probably a separate issue we should file
There was a problem hiding this comment.
we don't need to wait any result at all as we don't need to do anything there, we can just remove that callback call
There was a problem hiding this comment.
can you put a TODO here and I'll create an issue to report success/failure from saveVisit?
There was a problem hiding this comment.
I strongly disagree about not having a callback at all. We need callbacks for async operations to report success/failure. We can't go back to the kind of code we had in browser-laptop that ignored errors
There was a problem hiding this comment.
we don't need to fix the callback issue in this PR, but we should fix it
There was a problem hiding this comment.
I captured in the ticket above
| @@ -61,6 +64,10 @@ class LEDGER_EXPORT LedgerClient { | |||
| PublisherInfoCallback callback) = 0; | |||
| virtual void LoadPublisherInfo(PublisherInfoFilter filter, | |||
| PublisherInfoCallback callback) = 0; | |||
| virtual void LoadMediaPublisherInfo(const std::string& publisher_id, | |||
There was a problem hiding this comment.
I thought we wanted to go the other direction we these and tell the client what to save? This seems to do the opposite of the changes we want to make for PublisherInfo. Shouldn't we just pass the serialized json here for load and save?
There was a problem hiding this comment.
just like we do with state and previously did with publisher info before converting to sqlite
No description provided.