Bug 97405 - Integrate most of GoogleURL in WTFURL
Summary: Integrate most of GoogleURL in WTFURL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks: 97422
  Show dependency treegraph
 
Reported: 2012-09-22 23:31 PDT by Benjamin Poulain
Modified: 2012-09-24 14:30 PDT (History)
2 users (show)

See Also:


Attachments
Patch (402.05 KB, patch)
2012-09-22 23:45 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (401.59 KB, patch)
2012-09-22 23:54 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (401.59 KB, patch)
2012-09-23 00:04 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (414.87 KB, patch)
2012-09-23 15:44 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2012-09-22 23:31:55 PDT
Let's get this project going.
Comment 1 Benjamin Poulain 2012-09-22 23:45:17 PDT
Created attachment 165280 [details]
Patch
Comment 2 WebKit Review Bot 2012-09-22 23:48:35 PDT
Attachment 165280 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.xc..." exit_code: 1
Source/WTF/wtf/url/src/URLCanonInternal.cpp:285:  _itoa_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.cpp:302:  _itow_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:363:  _itoa_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:364:  _itow_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:368:  _itoa_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:374:  _itow_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:380:  _strtoui64 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 7 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Benjamin Poulain 2012-09-22 23:54:21 PDT
Created attachment 165281 [details]
Patch
Comment 4 WebKit Review Bot 2012-09-22 23:56:57 PDT
Attachment 165281 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.xc..." exit_code: 1
Source/WTF/wtf/url/src/URLCanonInternal.cpp:285:  _itoa_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.cpp:302:  _itow_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:363:  _itoa_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:364:  _itow_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:368:  _itoa_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:374:  _itow_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:380:  _strtoui64 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 7 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Build Bot 2012-09-22 23:57:45 PDT
Comment on attachment 165281 [details]
Patch

Attachment 165281 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13984349
Comment 6 Benjamin Poulain 2012-09-23 00:04:11 PDT
Created attachment 165282 [details]
Patch
Comment 7 WebKit Review Bot 2012-09-23 00:07:18 PDT
Attachment 165282 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.xc..." exit_code: 1
Source/WTF/wtf/url/src/URLCanonInternal.cpp:285:  _itoa_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.cpp:302:  _itow_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:363:  _itoa_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:364:  _itow_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:368:  _itoa_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:374:  _itow_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:380:  _strtoui64 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 7 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Kenneth Rohde Christiansen 2012-09-23 05:20:14 PDT
Comment on attachment 165282 [details]
Patch

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

> Source/WTF/wtf/url/api/ParsedURL.cpp:184
> +//    ASSERT_WITH_MESSAGE(!segment.isEmpty(), "A valid URL component should not be empty.");

:-)

> Source/WTF/wtf/url/src/URLCanon.h:540
> +bool ReplaceStandardURL(const char* base,
> +                                 const URLSegments& baseParsed,
> +                                 const Replacements<char>&,
> +                                 CharsetConverter* queryConverter,
> +                                 URLBuffer<char>&,
> +                                 URLSegments* outputParsed);

Shouldn't we put this on one line. That seems to be webkit style

> Source/WTF/wtf/url/src/URLCanonFileurl.cpp:3
> +// Copyright 2007, Google Inc. All rights reserved.
> +// Copyright 2012, Apple Inc. All rights reserved.
> +//

Shouldnt we fix the license style to be like the rest of webkit? like no comma after the year etc.

/*
 *  Copyright (C) 2007 Google Inc. All rights reserved.
 *  Copyright (C) 2012  Apple Inc. All rights reserved.
 *
Comment 9 Adam Barth 2012-09-23 11:57:53 PDT
Ben and I discussed this patch on #webkit for a bit before he posted this patch.  He would like to land this patch, get things wired together, and then work on refactoring the code in place.  This approach isn't the one we typically use when adding code to WebKit, but our thinking is that this code is more like a third-party library like the ones in Source/ThirdParty.

Perhaps we should first land the code in Source/ThirdParty, refactor it there, and then move it to into WebKit proper when its ready?  The downside of that approach is that the build system integration is trickier.

I'm going to rs=me this patch, but please consider whether it would be better to stage the code in Source/ThirdParty for a while.
Comment 10 Benjamin Poulain 2012-09-23 13:48:38 PDT
> Ben and I discussed this patch on #webkit for a bit before he posted this patch.  He would like to land this patch, get things wired together, and then work on refactoring the code in place.  This approach isn't the one we typically use when adding code to WebKit, but our thinking is that this code is more like a third-party library like the ones in Source/ThirdParty.

The problem is this code is in desperate need for cleaning and refactoring. Doing so without relying on the tests is working in the dark.

Every hour I spent renaming stuff would be better spent refactoring the code toward the template version.

All the code is contained in the url/ directory, so it is quite self contained already.

Next, I want to have all the tests passing, add new tests to cover the component parsing. Then eventually refactor everything and fix performance.
Comment 11 Benjamin Poulain 2012-09-23 15:44:09 PDT
Created attachment 165294 [details]
Patch
Comment 12 Benjamin Poulain 2012-09-23 15:44:27 PDT
Same patch, more cleaning.
Comment 13 WebKit Review Bot 2012-09-23 15:47:00 PDT
Attachment 165294 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.xc..." exit_code: 1
Source/WTF/wtf/url/src/URLCanonInternal.cpp:267:  _itoa_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.cpp:284:  _itow_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:365:  _itoa_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:366:  _itow_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:370:  _itoa_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:376:  _itow_s is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/url/src/URLCanonInternal.h:382:  _strtoui64 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 7 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Adam Barth 2012-09-23 21:33:20 PDT
Comment on attachment 165294 [details]
Patch

ok
Comment 15 Benjamin Poulain 2012-09-24 14:30:32 PDT
Comment on attachment 165294 [details]
Patch

Clearing flags on attachment: 165294

Committed r129412: <http://trac.webkit.org/changeset/129412>
Comment 16 Benjamin Poulain 2012-09-24 14:30:35 PDT
All reviewed patches have been landed.  Closing bug.