Bug 139753 - [iOS] Log navigation types using FeatureCounter API
Summary: [iOS] Log navigation types using FeatureCounter API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 139652
Blocks:
  Show dependency treegraph
 
Reported: 2014-12-17 15:58 PST by Chris Dumez
Modified: 2014-12-18 10:42 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.47 KB, patch)
2014-12-17 16:01 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (4.51 KB, patch)
2014-12-17 19:11 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (4.74 KB, patch)
2014-12-18 10:08 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2014-12-17 15:58:02 PST
We should log navigation types using FeatureCounter API.

Radar: <rdar://problem/19285978>
Comment 1 Chris Dumez 2014-12-17 16:01:31 PST
Created attachment 243467 [details]
Patch
Comment 2 Chris Dumez 2014-12-17 19:11:23 PST
Created attachment 243478 [details]
Patch
Comment 3 Darin Adler 2014-12-18 09:32:20 PST
Comment on attachment 243478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243478&action=review

> Source/WebCore/loader/FrameLoader.cpp:1383
> +    const char* key = nullptr;

Given the current code exactly the way it is, there is no reason to initialize key since it’s initialized in all code paths.

> Source/WebCore/loader/FrameLoader.cpp:1407
> +    default:
> +        return;

Why would we return without logging or asserting when we saw an unexpected frame load type? Is this a real case that we intentionally handle this way because we want to leave this data out of the feature counter, or perhaps an unexpected case where we should be asserting instead?

By including a default in the switch, we won’t get a warning if we add a new frame load type and forget to add a case to this switch statement. It would be better to not have default, and thus get the warning if a case is missing. My preference would be a separate function where we use a return statement to exit the function inside each case. That way, we could catch unknown frame load types at runtime, with code after the switch, but also get a reminder to handle new frame load types at compile time, because of the compiler’s checking of the switch.

> Source/WebCore/loader/FrameLoader.cpp:1409
> +    ASSERT(key);

I don’t think this assertion is valuable.
Comment 4 Darin Adler 2014-12-18 09:33:40 PST
Comment on attachment 243478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243478&action=review

> Source/WebCore/ChangeLog:11
> +        No new tests, no behavior change.

I think that’s misleading. There is indeed a behavior change! We added new feature counting. The reason we have no new tests is that it’s not our practice to add tests for feature counters, for some reason. It would be better to state that rather than calling this “no behavior change”. Even “no behavior change other than additional feature counting” would be better than what we have written here.
Comment 5 Chris Dumez 2014-12-18 09:48:53 PST
Comment on attachment 243478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243478&action=review

>> Source/WebCore/loader/FrameLoader.cpp:1407
>> +        return;
> 
> Why would we return without logging or asserting when we saw an unexpected frame load type? Is this a real case that we intentionally handle this way because we want to leave this data out of the feature counter, or perhaps an unexpected case where we should be asserting instead?
> 
> By including a default in the switch, we won’t get a warning if we add a new frame load type and forget to add a case to this switch statement. It would be better to not have default, and thus get the warning if a case is missing. My preference would be a separate function where we use a return statement to exit the function inside each case. That way, we could catch unknown frame load types at runtime, with code after the switch, but also get a reminder to handle new frame load types at compile time, because of the compiler’s checking of the switch.

Ok, I will explicitly state all possible values. I am currently not logging RedirectWithLockedBackForwardList / Replace because I wasn't sure we were interested in such navigations.
Comment 6 Chris Dumez 2014-12-18 10:08:33 PST
Created attachment 243498 [details]
Patch
Comment 7 WebKit Commit Bot 2014-12-18 10:42:33 PST
Comment on attachment 243498 [details]
Patch

Clearing flags on attachment: 243498

Committed r177504: <http://trac.webkit.org/changeset/177504>
Comment 8 WebKit Commit Bot 2014-12-18 10:42:39 PST
All reviewed patches have been landed.  Closing bug.