Bug 83851 - [chromium] Move WebVideoFrame into Platform and remove WebCore::VideoFrameChromium wrapper API
Summary: [chromium] Move WebVideoFrame into Platform and remove WebCore::VideoFrameChr...
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
Depends on: 83857
  Show dependency treegraph
Reported: 2012-04-12 18:53 PDT by James Robinson
Modified: 2012-04-13 13:43 PDT (History)
12 users (show)

See Also:

Patch (50.20 KB, patch)
2012-04-12 18:56 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (51.21 KB, patch)
2012-04-13 13:18 PDT, James Robinson
dpranke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-04-12 18:53:38 PDT
[chromium] Move WebVideoFrame into Platform and remove WebCore::VideoFrameChromium wrapper API
Comment 1 James Robinson 2012-04-12 18:56:31 PDT
Created attachment 137024 [details]
Comment 2 WebKit Review Bot 2012-04-12 19:00:52 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 Adam Barth 2012-04-12 19:05:46 PDT
Comment on attachment 137024 [details]

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

> Source/Platform/chromium/src/WebVideoFrame.cpp:43
> +const unsigned WebVideoFrame::maxPlanes = 3;
> +const unsigned WebVideoFrame::numRGBPlanes = 1;
> +const unsigned WebVideoFrame::rgbPlane = 0;
> +const unsigned WebVideoFrame::numYUVPlanes = 3;
> +const unsigned WebVideoFrame::yPlane = 0;
> +const unsigned WebVideoFrame::uPlane = 1;
> +const unsigned WebVideoFrame::vPlane = 2;

We can't put these in the header?
Comment 4 James Robinson 2012-04-12 19:47:13 PDT
They probably could live in the header, but I would have to verify that they don't need storage (or convert them from ints to enums).  Keeping the existing mechanism in place seemed safer since the patch is also doing other things.
Comment 5 James Robinson 2012-04-12 19:53:12 PDT
Committed r114075: <http://trac.webkit.org/changeset/114075>
Comment 6 James Robinson 2012-04-13 13:06:30 PDT
This failed because some .cpp's in webkit_unit_tests #included headers in WebCore that include WebVideoFrame.h as "#include <public/WebVideoFrame.h>", and webkit_unit_tests was being compiled without Source/Platform/chromium/ on the include path.  webkit_unit_tests depend on WebKit.gyp:webkit and nothing else.  Since webkit_unit_tests is an executable, it can't depend directly on webkit_platform since webkit_platform cannot compile as a standalone library - it has to compile as part of webkit.dll since it depends on symbols in WTF that are actually defined in Source/WebKit/chromium/src/...  Since webkit_unit_tests is special in that it can #include WebCore headers while no other users of WebKit.gyp:webkit are allowed to, it seems that the best option is to put the Platform API include path directly on the webkit_unit_test target.
Comment 7 James Robinson 2012-04-13 13:06:50 PDT
Reopening due to revert: http://trac.webkit.org/changeset/114078
Comment 8 James Robinson 2012-04-13 13:17:19 PDT
diff --git a/Source/WebKit/chromium/WebKitUnitTests.gyp b/Source/WebKit/chromium/WebKitUnitTests.gyp
index 4776dc4..6f1ed11 100644
--- a/Source/WebKit/chromium/WebKitUnitTests.gyp
+++ b/Source/WebKit/chromium/WebKitUnitTests.gyp
@@ -72,6 +72,11 @@
             'include_dirs': [
+                # WebKit unit tests are allowed to include WebKit and WebCore header files, which may include headers in the
+                # Platform API as <public/WebFoo.h>. Thus we need to have the Platform API include path, but we can't depend
+                # directly on Platform.gyp:webkit_platform since platform cannot link as a separate library. Instead, we just
+                # add the include path directly.
+                '../../Platform/chromium',
             'conditions': [
                 ['inside_chromium_build==1 and component=="shared_library"', {
Comment 9 James Robinson 2012-04-13 13:18:54 PDT
Created attachment 137134 [details]
Comment 10 James Robinson 2012-04-13 13:43:36 PDT
Committed r114164: <http://trac.webkit.org/changeset/114164>