Bug 85758 – [Chromium] Move clipboard to Platform.h

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Mark Pilgrim (Google)]

[Chromium] Move clipboard to Platform.h

[Mark Pilgrim (Google)]

Created attachment 140449 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

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.

[Mark Pilgrim (Google)]

Created attachment 140525 [details]
Patch

[WebKit Review Bot]

Comment on attachment 140525 [details]
Patch

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

[Adam Barth]

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

[Adam Barth]

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.

[Mark Pilgrim (Google)]

Created attachment 140566 [details]
WIP Patch

[Mark Pilgrim (Google)]

Still WIP.

[Mark Pilgrim (Google)]

Created attachment 140986 [details]
Patch

[Mark Pilgrim (Google)]

(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.

[James Robinson]

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

[Eric Seidel (no email)]

(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.

[James Robinson]

(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/.

[WebKit Review Bot]

Comment on attachment 140986 [details]
Patch

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

[Mark Pilgrim (Google)]

(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?

[Mark Pilgrim (Google)]

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

[Adam Barth]

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

[Mark Pilgrim (Google)]

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

Grr.

[Mark Pilgrim (Google)]

Created attachment 141016 [details]
Patch

[Mark Pilgrim (Google)]

Filed bug bug 86018 for removing code behind WEBKIT_USING_CG

[Adam Barth]

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.

[WebKit Review Bot]

Comment on attachment 141016 [details]
Patch

Clearing flags on attachment: 141016

Committed r116566: <http://trac.webkit.org/changeset/116566>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.