Bug 139753 – [iOS] Log navigation types using FeatureCounter API

• Component: Page Loading
• Status: RESOLVED FIXED

[Chris Dumez]

We should log navigation types using FeatureCounter API.

Radar: <rdar://problem/19285978>

[Chris Dumez]

Created attachment 243467 [details]
Patch

[Chris Dumez]

Created attachment 243478 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 243498 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 243498 [details]
Patch

Clearing flags on attachment: 243498

Committed r177504: <http://trac.webkit.org/changeset/177504>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.