Bug 95745 - [GTK] Move user agent helpers to WebCore
: [GTK] Move user agent helpers to WebCore
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Martin Robinson
:
Depends on:
Blocks: 95697
  Show dependency treegraph
 
Reported: 2012-09-04 07:11 PDT by Martin Robinson
Modified: 2012-09-07 10:51 PDT (History)
1 user (show)

See Also:


Attachments
Patch (14.62 KB, patch)
2012-09-06 14:24 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (14.58 KB, patch)
2012-09-07 09:36 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2012-09-04 07:11:46 PDT
We can move user agent helpers to WebCore so that they can more easily be shared with WebKit2.
Comment 1 Martin Robinson 2012-09-06 14:24:22 PDT
Created attachment 162584 [details]
Patch
Comment 2 Carlos Garcia Campos 2012-09-07 00:42:27 PDT
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 3 Martin Robinson 2012-09-07 08:49:56 PDT
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.
Comment 4 Carlos Garcia Campos 2012-09-07 08:57:11 PDT
(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.
Comment 5 Martin Robinson 2012-09-07 09:36:29 PDT
Created attachment 162788 [details]
Patch
Comment 6 Martin Robinson 2012-09-07 10:51:38 PDT
Comment on attachment 162788 [details]
Patch

Clearing flags on attachment: 162788

Committed r127889: <http://trac.webkit.org/changeset/127889>
Comment 7 Martin Robinson 2012-09-07 10:51:41 PDT
All reviewed patches have been landed.  Closing bug.