Bug 85758

Created attachment 140449 [details] Patch

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 on attachment 140449 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140449&action=review > Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:-54 > -class WebClipboard; You can probably guess what I'm going to say at this point. :) WebClipboard.h and its dependencies are already in Source/WebKit/chromium/public/platform, so they should be easy to move at the same time.

Created attachment 140525 [details] Patch

Comment on attachment 140525 [details] Patch Attachment 140525 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12647056

Comment on attachment 140525 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140525&action=review > Source/Platform/chromium/public/WebClipboard.h:42 > +class WebDragData; > +class WebImage; Please move these dependencies too. The idea is that there shouldn't be any dependencies from the Platform layer to the WebKit layer. (Dependencies from WebKit to Platform are fine.) Here's a diagram that's intended to illustrate that relation: https://docs.google.com/drawings/d/1GJXd6XSLEehvuqA7Xbvbz0OH6Znnr2iTijihjrEY0ts/edit

To avoid breaking the compile when moving these APIs around, we've been introducing "forwarding" headers like the following: http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/public/platform/WebURL.h Once we've finished factoring the Platform API out of the WebKit API, we'll update all the users of the API to use the new header locations and remove these forwarding headers.

Created attachment 140566 [details] WIP Patch

Still WIP.

Created attachment 140986 [details] Patch

(In reply to comment #6) > (From update of attachment 140525 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140525&action=review > > > Source/Platform/chromium/public/WebClipboard.h:42 > > +class WebDragData; > > +class WebImage; > > Please move these dependencies too. Done in latest patch.

Comment on attachment 140986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140986&action=review > Source/Platform/chromium/public/Platform.h:67 > // Must return non-null. > + virtual WebClipboard* clipboard() { return 0; } > + > + // Must return non-null. > virtual WebMimeRegistry* mimeRegistry() { return 0; } these comments are kind of weird with the default impls > Source/Platform/chromium/public/WebImage.h:40 > +#elif WEBKIT_USING_CG > +typedef struct CGImage* CGImageRef; > +#endif this isn't necessarily something you have to do, but we should delete all the WEBKIT_USING_CG code from chromium code ASAPly

(In reply to comment #12) > (From update of attachment 140986 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140986&action=review > > Source/Platform/chromium/public/WebImage.h:40 > > +#elif WEBKIT_USING_CG > > +typedef struct CGImage* CGImageRef; > > +#endif > > this isn't necessarily something you have to do, but we should delete all the WEBKIT_USING_CG code from chromium code ASAPly When I was looking at removing BUILDING_ON_LEOPARD code the other day, I looked at removing CHROMIUM branches from various bits of cg, mac and cocoa files, but it was't clear to me what parts were still in use by Chromium and which weren't.

(In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 140986 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=140986&action=review > > > Source/Platform/chromium/public/WebImage.h:40 > > > +#elif WEBKIT_USING_CG > > > +typedef struct CGImage* CGImageRef; > > > +#endif > > > > this isn't necessarily something you have to do, but we should delete all the WEBKIT_USING_CG code from chromium code ASAPly > > When I was looking at removing BUILDING_ON_LEOPARD code the other day, I looked at removing CHROMIUM branches from various bits of cg, mac and cocoa files, but it was't clear to me what parts were still in use by Chromium and which weren't. That's slightly different. We never use CoreGraphics as the backend for GraphicsContext in Chromium, but we do still use some CG stuff in various bits of system integration (like theming code, iirc). My understanding is that WEBKIT_USING_CG means "Using CoreGraphics as the raster backend", which we never set, but that may not be the meaning of all the code in cg/ mac/ cocoa/.

Comment on attachment 140986 [details] Patch Attachment 140986 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12665003

(In reply to comment #15) > (From update of attachment 140986 [details]) > Attachment 140986 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/12665003 Is this because I removed WebClipboard.h from Source/WebKit/chromium/WebKit.gyp?

...and if so, why didn't I catch this in my local build? What am I doing differently from the bots?

Looks like you forgot to svn add Source/Platform/chromium/public/WebClipboard.h

(In reply to comment #18) > Looks like you forgot to svn add Source/Platform/chromium/public/WebClipboard.h Grr.

Created attachment 141016 [details] Patch

Filed bug bug 86018 for removing code behind WEBKIT_USING_CG

Comment on attachment 141016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141016&action=review > Source/Platform/chromium/public/Platform.h:67 > // Must return non-null. > + virtual WebClipboard* clipboard() { return 0; } > + > + // Must return non-null. > virtual WebMimeRegistry* mimeRegistry() { return 0; } These comments do seem odd next to these default implementations, but I think its ok. Can we use ASSERT_NOT_REACHED() in API headers? I suspect we're not able to. > Source/WebKit/chromium/src/DragClientImpl.cpp:43 > > +#include <public/WebDragData.h> There's no need for this blank line. > Source/WebKit/chromium/src/WebDragData.cpp:42 > +#include <public/WebDragData.h> This header should have been left just after config. You probably got a style warning, but that's a bug in the style checker.

Comment on attachment 141016 [details] Patch Clearing flags on attachment: 141016 Committed r116566: <http://trac.webkit.org/changeset/116566>

All reviewed patches have been landed. Closing bug.