Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1610143002:
Adds UMA for slow and fast back/forward WKWebController navigations. (Closed)
|
Created:
4 years, 11 months ago by stkhapugin Modified:
3 years, 8 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds UMA for slow and fast back/forward WKWebController navigations.
Adds new UMA histogram: Navigation.IOSWKWebViewSlowFastBackForward to
track the slow/fast back and forward navigations. Fast navigations are
navigations through WKBackForwardList.
BUG=535622
Patch Set 1 #Patch Set 2 : pre-review cleanup #
Total comments: 8
Patch Set 3 : Overriding goDelta: #
Total comments: 5
Patch Set 4 : Factored out logic to a helper. Added an early return. #
Messages
Total messages: 11 (1 generated)
stkhapugin@chromium.org changed reviewers: + eugenebut@chromium.org, stuartmorgan@chromium.org
PTAL
https://codereview.chromium.org/1610143002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/1610143002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:176: FAST_BACK = 0, Back usually means previous (not negative delta), also distinguishing between back/forward/delta in probably unnecessary. With that I think we need 2 metrics: FAST and SLOW. https://codereview.chromium.org/1610143002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1780: if ([self isKindOfClass:[CRWWKWebViewWebController class]]) { I think you should move this code to subclass' goDelta: Hardcoding classname is not maintainable. Also this way you won't need canPerformFastNavigationForSessionEntry: https://codereview.chromium.org/1610143002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1785: BackForwardNavigationType navigationType; Please always initialize local variables.
PTAL. Stuart, can you please respond to comment about the metric? https://codereview.chromium.org/1610143002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/1610143002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:176: FAST_BACK = 0, On 2016/01/20 16:45:04, eugenebut wrote: > Back usually means previous (not negative delta), also distinguishing between > back/forward/delta in probably unnecessary. With that I think we need 2 metrics: > FAST and SLOW. Stuart mentioned that he'd like to have a distinction between back/forward, although it's not necessary. Now that I understand how to implement it, why don't we keep the two? I think there is little reason to distinguish between "go back one page" and "go back (>1) pages", too. Stuart, what do you think? https://codereview.chromium.org/1610143002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1780: if ([self isKindOfClass:[CRWWKWebViewWebController class]]) { On 2016/01/20 16:45:04, eugenebut wrote: > I think you should move this code to subclass' goDelta: Hardcoding classname is > not maintainable. Also this way you won't need > canPerformFastNavigationForSessionEntry: Done. https://codereview.chromium.org/1610143002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1785: BackForwardNavigationType navigationType; On 2016/01/20 16:45:04, eugenebut wrote: > Please always initialize local variables. Done.
https://codereview.chromium.org/1610143002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/1610143002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:176: FAST_BACK = 0, On 2016/01/21 10:39:36, stkhapugin wrote: > On 2016/01/20 16:45:04, eugenebut wrote: > > Back usually means previous (not negative delta), also distinguishing between > > back/forward/delta in probably unnecessary. With that I think we need 2 > metrics: > > FAST and SLOW. > > Stuart mentioned that he'd like to have a distinction between back/forward, > although it's not necessary. Now that I understand how to implement it, why > don't we keep the two? I think there is little reason to distinguish between "go > back one page" and "go back (>1) pages", too. > Stuart, what do you think? The problem here is that FAST_BACK is misleading, because it also contains negative delta. https://codereview.chromium.org/1610143002/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1610143002/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:494: if (delta != 0) { NIT: Please use early return to avoid indentations.
https://codereview.chromium.org/1610143002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/1610143002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:176: FAST_BACK = 0, On 2016/01/21 15:03:49, eugenebut wrote: > On 2016/01/21 10:39:36, stkhapugin wrote: > > On 2016/01/20 16:45:04, eugenebut wrote: > > > Back usually means previous (not negative delta), also distinguishing > between > > > back/forward/delta in probably unnecessary. With that I think we need 2 > > metrics: > > > FAST and SLOW. > > > > Stuart mentioned that he'd like to have a distinction between back/forward, > > although it's not necessary. Now that I understand how to implement it, why > > don't we keep the two? I think there is little reason to distinguish between > "go > > back one page" and "go back (>1) pages", too. > > Stuart, what do you think? > The problem here is that FAST_BACK is misleading, because it also contains > negative delta. By "negative delta" do you mean going back N>1 pages? I think that's fine personally, but having either more (1 vs >1 in each direction) or fewer (just fast or slow history) buckets is fine with me as well. https://codereview.chromium.org/1610143002/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1610143002/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:501: [self isBackForwardListItemValid:holder->back_forward_list_item()]); The fact that the logic for measuring fast back/forward and actually deciding whether to do fast back-forward is in totally different places makes me nervous. I'm not sure we'd ever know if this became wrong. At the very least I think this conditional should be extracted to a helper rather than duplicated, but there's other logic too (e.g., "is it a post?") so I'm wondering if we could find a way to actually log it where the decision is really made. I know there was some investigation of that, but how many options were considered? For instance, could we propagate a fast/slow value out of the implementing function, and check its return value? Or piggy back some extra state somewhere?
PTAL https://codereview.chromium.org/1610143002/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1610143002/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:494: if (delta != 0) { On 2016/01/21 15:03:49, eugenebut wrote: > NIT: Please use early return to avoid indentations. Done. https://codereview.chromium.org/1610143002/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:501: [self isBackForwardListItemValid:holder->back_forward_list_item()]); On 2016/01/28 23:25:44, stuartmorgan wrote: > The fact that the logic for measuring fast back/forward and actually deciding > whether to do fast back-forward is in totally different places makes me nervous. > I'm not sure we'd ever know if this became wrong. > > At the very least I think this conditional should be extracted to a helper > rather than duplicated, but there's other logic too (e.g., "is it a post?") so > I'm wondering if we could find a way to actually log it where the decision is > really made. I know there was some investigation of that, but how many options > were considered? For instance, could we propagate a fast/slow value out of the > implementing function, and check its return value? Or piggy back some extra > state somewhere? Keeping the reported metric as is for now. Please if you feel like this is not correct, explain in detail what metric do you want to see instead. The way I understand the reasoning behind the introduction of this metric, what is reported in this patch is sufficient. I factored out the logic here and in the method. As was discussed here, in emails, on previous CL and in the bug, I don't see a 100% clean way of implementing this because I'm not super familiar with this part of the code. I don't think I can come up with anything significantly better (propagating delta or navigation type through the series of calls) than current implementation in reasonable time, so if you find current solution fundamentally bad, let's find someone else to find a better way. Personally I think that this is good enough for reporting one UMA metric.
https://codereview.chromium.org/1610143002/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1610143002/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:501: [self isBackForwardListItemValid:holder->back_forward_list_item()]); On 2016/02/05 15:40:15, stkhapugin wrote: > Keeping the reported metric as is for now. Please if you feel like this is not > correct, explain in detail what metric do you want to see instead. The way I > understand the reasoning behind the introduction of this metric, what is > reported in this patch is sufficient. It's not a question of the divisions in the metric, it's whether the reported values actually match reality. > Personally I think that this is good enough for reporting one UMA metric. I don't think it matters that it's one metric; if we're going to report a metric, we should report it correctly. Having a metric that is likely to give increasingly inaccurate data over time seems like a bad idea, since arguably having an incorrect metric is worse than having no metric. Having the metric and the actual back/forward code in different places, with partial copies of the decision tree, is a recipe for things getting out of sync. It's in fact *already* not actually correct in all cases as I noted in the last comment (POST is not handled here). There's an argument to be made that that's an edge case that may not matter in the metric, but the fact that it exists is a demonstration of the structural problem here. I think if the actual back/forward logic were to change, there's a good chance this piece of code would be forgotten and we'd end up reporting something completely unrelated to the actual behavior of the app. I'll leave the final call to Eugene as to whether he thinks the risk here warrants more effort.
Eugene, can you tell me what are the things you want me to change to proceed?
On 2016/03/04 18:45:39, stkhapugin wrote: > Eugene, can you tell me what are the things you want me to change to proceed? Stuart expressed some concerns in the last comment. Do you think you can address them?
On 2016/03/04 18:57:43, eugenebut wrote: > On 2016/03/04 18:45:39, stkhapugin wrote: > > Eugene, can you tell me what are the things you want me to change to proceed? > > Stuart expressed some concerns in the last comment. Do you think you can address > them? It looks like Stuart's concerns are still valid. I think that the general approach of using the presence of the WKBackForwardListItem to indicate fast vs. slow is correct, but we should probably do it in |-didFinishNavigation|. I'm not 100% sure off the top of my head whether this will be called for all back/forward navigations + POST navigations, since I know that there are some navigations where this isn't called (i.e. hash change events). If you verify that all codepaths in CRWWebViewWebController's |-loadRequestForCurrentNavigationItem| go through |-didFinishNavigation|, then that will work. Otherwise, we should probably create a separate utility function to record the UMA metric and call them from the navigation blocks themselves. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
