Summary: | [GTK] Move user agent helpers to WebCore | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||||
Component: | WebKitGTK | Assignee: | Martin Robinson <mrobinson> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cgarcia | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 95697 | ||||||||
Attachments: |
|
Description
Martin Robinson
2012-09-04 07:11:46 PDT
Created attachment 162584 [details]
Patch
Comment on attachment 162584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162584&action=review Looks good to me, just consider adding a default application name when not provided. > Source/WebCore/platform/gtk/UserAgentGtk.cpp:102 > + return String::format("%s %s/%s", staticUA.utf8().data(), applicationName.utf8().data(), finalApplicationVersion.utf8().data()); So, if application name is empty, we end up with something like "/1.2.3", it's a bit weird. If we don't want to use the program name as default we could use something like "WebKitGTK+ based browser/1.2.3" > Source/WebCore/platform/gtk/UserAgentGtk.h.in:38 > +const char* platformForUAString(); > +String platformVersionForUAString(); Do we need these to be public? Comment on attachment 162584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162584&action=review >> Source/WebCore/platform/gtk/UserAgentGtk.cpp:102 >> + return String::format("%s %s/%s", staticUA.utf8().data(), applicationName.utf8().data(), finalApplicationVersion.utf8().data()); > > So, if application name is empty, we end up with something like "/1.2.3", it's a bit weird. If we don't want to use the program name as default we could use something like "WebKitGTK+ based browser/1.2.3" If the application name is empty, we've already returned early by this point because at line 95 we have: if (applicationName.isEmpty()) return staticUA; >> Source/WebCore/platform/gtk/UserAgentGtk.h.in:38 >> +String platformVersionForUAString(); > > Do we need these to be public? Nope! They were left over from a previous design. I'll make them static. (In reply to comment #3) > (From update of attachment 162584 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162584&action=review > > >> Source/WebCore/platform/gtk/UserAgentGtk.cpp:102 > >> + return String::format("%s %s/%s", staticUA.utf8().data(), applicationName.utf8().data(), finalApplicationVersion.utf8().data()); > > > > So, if application name is empty, we end up with something like "/1.2.3", it's a bit weird. If we don't want to use the program name as default we could use something like "WebKitGTK+ based browser/1.2.3" > > If the application name is empty, we've already returned early by this point because at line 95 we have: > > if (applicationName.isEmpty()) > return staticUA; Perfect then, I missed that, sorry. > >> Source/WebCore/platform/gtk/UserAgentGtk.h.in:38 > >> +String platformVersionForUAString(); > > > > Do we need these to be public? > > Nope! They were left over from a previous design. I'll make them static. Ok. Created attachment 162788 [details]
Patch
Comment on attachment 162788 [details] Patch Clearing flags on attachment: 162788 Committed r127889: <http://trac.webkit.org/changeset/127889> All reviewed patches have been landed. Closing bug. |