Bug 140323

Summary: Log navigation types using DiagnosticLoggingClient
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Page LoadingAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, barraclough, commit-queue, japhet, jer.noble, sam
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 140463    
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Dumez 2015-01-09 16:06:38 PST
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>
Comment 1 Chris Dumez 2015-01-09 16:10:51 PST
Created attachment 244385 [details]
Patch
Comment 2 Darin Adler 2015-01-12 10:52:11 PST
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*?
Comment 3 Darin Adler 2015-01-12 10:52:44 PST
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.
Comment 4 Anders Carlsson 2015-01-12 10:54:02 PST
(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; }
Comment 5 Chris Dumez 2015-01-12 12:23:38 PST
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.
Comment 6 Chris Dumez 2015-01-12 12:26:39 PST
Created attachment 244455 [details]
Patch
Comment 7 WebKit Commit Bot 2015-01-12 14:31:04 PST
Comment on attachment 244455 [details]
Patch

Clearing flags on attachment: 244455

Committed r178298: <http://trac.webkit.org/changeset/178298>
Comment 8 WebKit Commit Bot 2015-01-12 14:31:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Chris Dumez 2015-01-12 14:45:28 PST
(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.