Bug 65265 - [chromium] Make WebImage::operator=(CGImageRef) a WEBKIT_API
Summary: [chromium] Make WebImage::operator=(CGImageRef) a WEBKIT_API
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: Nico Weber
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-27 09:45 PDT by Nico Weber
Modified: 2011-07-28 20:25 PDT (History)
3 users (show)

See Also:

Attachments
Patch (1.98 KB, patch)
2011-07-27 09:46 PDT, Nico Weber 		no flags Details | Formatted Diff | Diff
Patch (1.25 KB, patch)
2011-07-28 19:31 PDT, Nico Weber 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Nico Weber 2011-07-27 09:45:01 PDT 
[chromium] Make WebImage::operator=(CGImageRef) a WEBKIT_API
Comment 1 Nico Weber 2011-07-27 09:46:37 PDT 
Created attachment 102152 [details]
Patch
Comment 2 Nico Weber 2011-07-27 15:09:36 PDT 
Darin: jamesr says this is for you
Comment 3 Darin Fisher (:fishd, Google) 2011-07-27 15:16:36 PDT 
Comment on attachment 102152 [details]
Patch

This change is technically fine, but it would be nice to know why you want to make this change.  Can you please explain the motivation for making this non-inline?  Normally, the WebKit API implements operators inline as calls to exported functions.
Comment 4 Nico Weber 2011-07-27 15:20:18 PDT 
The larger motivation is the components build for mac.

As mentioned in the changelog, webkit/glue/webcursor_mac.mm ( http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/webkit/glue/webcursor_mac.mm&exact_package=chromium&q=webcursor_ma&type=cs ) calls operator=, so it needs to be exported. assign(CGImageRef) is currently private. If it's preferrable, I can make assign(CGImageRef) public, turn that into WEBKIT_API, keep operator=() inline, and update the call site (after waiting for the roll).

Your call :-)
Comment 5 Darin Fisher (:fishd, Google) 2011-07-27 15:23:25 PDT 
(In reply to comment #4)
> The larger motivation is the components build for mac.
> 
> As mentioned in the changelog, webkit/glue/webcursor_mac.mm ( http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/webkit/glue/webcursor_mac.mm&exact_package=chromium&q=webcursor_ma&type=cs ) calls operator=, so it needs to be exported. assign(CGImageRef) is currently private. If it's preferrable, I can make assign(CGImageRef) public, turn that into WEBKIT_API, keep operator=() inline, and update the call site (after waiting for the roll).

Oh, I see.  Yeah, the convention would be to tag assign(CGImageRef) with WEBKIT_API.  We shouldn't need to make it public.
Comment 6 Darin Fisher (:fishd, Google) 2011-07-27 22:43:58 PDT 
Comment on attachment 102152 [details]
Patch

As discussed, please tag the assign(CGImageRef) function with WEBKIT_API, and leave the rest as is.
Comment 7 Nico Weber 2011-07-28 19:31:09 PDT 
Created attachment 102325 [details]
Patch
Comment 8 James Robinson 2011-07-28 19:32:39 PDT 
Comment on attachment 102325 [details]
Patch

R+ based on Darin's comment.
Comment 9 WebKit Review Bot 2011-07-28 20:25:53 PDT 
Comment on attachment 102325 [details]
Patch

Clearing flags on attachment: 102325

Committed r91970: <http://trac.webkit.org/changeset/91970>
Comment 10 WebKit Review Bot 2011-07-28 20:25:57 PDT 
All reviewed patches have been landed.  Closing bug.