Bug 82677

Summary: [Chromium] Delete WebKit/chromium/bridge
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, fishd, jamesr, tkent, tommyw, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch jamesr: review+

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.