Bug 82677 - [Chromium] Delete WebKit/chromium/bridge
Summary: [Chromium] Delete WebKit/chromium/bridge
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-29 16:53 PDT by Adam Barth
Modified: 2012-03-31 00:47 PDT (History)
6 users (show)

See Also:


Attachments
Patch (232.97 KB, patch)
2012-03-29 17:00 PDT, Adam Barth
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-03-29 16:53:16 PDT
[Chromium] Delete WebKit/chromium/bridge
Comment 1 Adam Barth 2012-03-29 17:00:15 PDT
Created attachment 134695 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-29 17:03:53 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 WebKit Review Bot 2012-03-29 17:04:25 PDT
Attachment 134695 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/platform/chromium/support/WebMediaStreamSourcesRequest.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/chromium/support/WebMediaStreamSourcesRequest.cpp:38:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/chromium/support/WebMediaHints.cpp:37:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/chromium/support/WebICEOptions.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/chromium/support/WebMediaStreamComponent.cpp:37:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/chromium/support/WebICECandidateDescriptor.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/chromium/support/WebMediaStreamSource.cpp:37:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/chromium/support/WebMediaStreamDescriptor.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/chromium/support/WebMediaStreamDescriptor.cpp:38:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 9 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Tommy Widenflycht 2012-03-30 01:51:33 PDT
I like this patch a lot  and it can be further simplified by deleting the *Internal classes (to my eternal joy and happiness). I'll be happy to do this in a follow-up patch.
Comment 5 Tommy Widenflycht 2012-03-30 02:13:27 PDT
There seems to be a major problem with the patch in that the files going to Source/Platform/chromium/public are missing. 

You could try generating the patch yourself with 
git diff --no-renames --no-prefix master..HEAD >some_filename
and uploading it manually. If you are using git that is.
Comment 6 James Robinson 2012-03-30 10:33:20 PDT
From the actual patch:

Index: Source/Platform/chromium/public/WebICECandidateDescriptor.h
===================================================================
--- Source/Platform/chromium/public/WebICECandidateDescriptor.h	(revision 112571)	(from Source/WebKit/chromium/public/platform/WebICECandidateDescriptor.h:112571)
+++ Source/Platform/chromium/public/WebICECandidateDescriptor.h	(working copy)
Index: Source/Platform/chromium/public/WebICEOptions.h
===================================================================
--- Source/Platform/chromium/public/WebICEOptions.h	(revision 112571)	(from Source/WebKit/chromium/public/platform/WebICEOptions.h:112571)
+++ Source/Platform/chromium/public/WebICEOptions.h	(working copy)


that looks fine to me. I'll patch this in locally to review
Comment 7 James Robinson 2012-03-30 10:40:38 PDT
Comment on attachment 134695 [details]
Patch

Looks great! R=me. You've got merge conflicts in the .gypis, but that's not a huge surprise.
Comment 8 Adam Barth 2012-03-30 10:57:27 PDT
Unfortunately, some of the earlier patches were rolled back, so I'll need to figure them out first,
Comment 9 Adam Barth 2012-03-31 00:45:05 PDT
Committed r112784: <http://trac.webkit.org/changeset/112784>
Comment 10 Adam Barth 2012-03-31 00:47:22 PDT
> Unfortunately, some of the earlier patches were rolled back, so I'll need to figure them out first,

I managed to beat the Windows component build into shape and was able to re-land the earlier patches.

> I like this patch a lot  and it can be further simplified by deleting the *Internal classes (to my eternal joy and happiness). I'll be happy to do this in a follow-up patch.

Yay!  That's the goal.  Getting rid of the "Internal" classes would be quite welcome.  Depending on how that works, we might need to do some more build system work.  Let me know if you run into any difficulties.