Bug 140323 – Log navigation types using DiagnosticLoggingClient

• Component: Page Loading
• Status: RESOLVED FIXED

[Chris Dumez]

Log navigation types using DiagnosticLoggingClient to help us understand what types of navigations are common and give us an estimate on the total number of navigations.

Radar: <rdar://problem/19432920>

[Chris Dumez]

Created attachment 244385 [details]
Patch

[Darin Adler]

Comment on attachment 244385 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:1422
> +    if (page->settings().diagnosticLoggingEnabled()) {

If settings() were hanging off the main frame, then we would not need to involve the page at all. It would be way nicer if this was a main frame thing and not a page thing.

> Source/WebCore/loader/FrameLoader.cpp:1424
> +        if (DiagnosticLoggingClient* diagnosticLoggingClient = page->mainFrame().diagnosticLoggingClient())
> +            diagnosticLoggingClient->logDiagnosticMessage(DiagnosticLoggingKeys::navigationKey(), navigationDescription);

I’d name this local variable “client”, not “diagnosticLoggingClient” since its scope is just this tiny if statement.

I’d use auto* to avoid repeating the long type name.

It seems a shame that logDiagnosticMessage takes a String. If these are all string literals then why doesn’t it just take a const char*?

[Darin Adler]

Comment on attachment 244385 [details]
Patch

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

> Source/WebCore/page/DiagnosticLoggingKeys.h:46
> +    static String navigationKey();

I think these should all be const char*. No reason to reference count all these string literals.

[Anders Carlsson]

(In reply to comment #2)
> Comment on attachment 244385 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244385&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:1422
> > +    if (page->settings().diagnosticLoggingEnabled()) {
> 
> If settings() were hanging off the main frame, then we would not need to
> involve the page at all. It would be way nicer if this was a main frame
> thing and not a page thing.
> 

Settings are ref-counted now, and each frame has a Settings object:

        Settings& settings() const { return *m_settings; }

[Chris Dumez]

Comment on attachment 244385 [details]
Patch

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

>>> Source/WebCore/loader/FrameLoader.cpp:1422
>>> +    if (page->settings().diagnosticLoggingEnabled()) {
>> 
>> If settings() were hanging off the main frame, then we would not need to involve the page at all. It would be way nicer if this was a main frame thing and not a page thing.
> 
> Settings are ref-counted now, and each frame has a Settings object:
> 
>         Settings& settings() const { return *m_settings; }

Ok, I will get the settings from the MainFrame instead.

>> Source/WebCore/loader/FrameLoader.cpp:1424
>> +            diagnosticLoggingClient->logDiagnosticMessage(DiagnosticLoggingKeys::navigationKey(), navigationDescription);
> 
> I’d name this local variable “client”, not “diagnosticLoggingClient” since its scope is just this tiny if statement.
> 
> I’d use auto* to avoid repeating the long type name.
> 
> It seems a shame that logDiagnosticMessage takes a String. If these are all string literals then why doesn’t it just take a const char*?

Yes, I think those can be const char*. Do you mind mind if I do this in a separate patch? There is quite a bit of existing code to update.

[Chris Dumez]

Created attachment 244455 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 244455 [details]
Patch

Clearing flags on attachment: 244455

Committed r178298: <http://trac.webkit.org/changeset/178298>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

(In reply to comment #3)
> Comment on attachment 244385 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244385&action=review
> 
> > Source/WebCore/page/DiagnosticLoggingKeys.h:46
> > +    static String navigationKey();
> 
> I think these should all be const char*. No reason to reference count all
> these string literals.

In several cases, logDiagnosticMessage() is called with non-literals (http status codes constructed using String::number(), MIME types, ...). So the logDiagnosticMessage*() take Strings in arguments.