Bug 73036

Created attachment 116388 [details] Patch

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

Comment on attachment 116388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116388&action=review > Source/WebKit/chromium/public/WebIntent.h:44 > + WebIntent(const WebString& action, const WebString& type, const WebString& data, int identifier); public, non-inline methods need to be annotated with WEBKIT_EXPORT or else you will break the shared library build. also, please note that we normally do not export constructors. instead, we write initialize() methods, and export those. the constructor can call the initialize() method. in this case, though... I wonder... will there be no WebCore::Intent class? if there is such a class, maybe you want the constructor to take that instead? the same issue applies to WebIntentServiceInfo.

Comment on attachment 116388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116388&action=review >> Source/WebKit/chromium/public/WebIntent.h:44 >> + WebIntent(const WebString& action, const WebString& type, const WebString& data, int identifier); > > public, non-inline methods need to be annotated with WEBKIT_EXPORT or else you will break the shared library build. > > also, please note that we normally do not export constructors. instead, we write initialize() methods, and export > those. the constructor can call the initialize() method. > > in this case, though... I wonder... will there be no WebCore::Intent class? if there is such a class, maybe you > want the constructor to take that instead? > > the same issue applies to WebIntentServiceInfo. Right. I didn't export the constructor for that reason. Will this break the shared lib? There will be a WebCore::Intent class; I have that constructor in a separate CL which I'm going to send along soon. For now I just wanted some simple method implementations so I can check in the adjustments to our Chromium code that will use these accessors. I could take the constructors out completely. Shall I just do that?

(In reply to comment #4) > Right. I didn't export the constructor for that reason. Will this break the shared lib? Ah, OK... since the class is only ever created by WebKit, then you should hide this constructor behind #if WEBKIT_IMPLEMENTATION. That way we can avoid the possibility of someone trying to construct it outside of WebKit. > There will be a WebCore::Intent class; I have that constructor in a separate CL which I'm going to send along soon. For now I just wanted some simple method implementations so I can check in the adjustments to our Chromium code that will use these accessors. I could take the constructors out completely. Shall I just do that? I guess you can initialize these classes by just calling the setFoo methods, right?

(In reply to comment #5) > (In reply to comment #4) > > Right. I didn't export the constructor for that reason. Will this break the shared lib? > > Ah, OK... since the class is only ever created by WebKit, then you should hide this constructor behind #if WEBKIT_IMPLEMENTATION. That way we can avoid the possibility of someone trying to construct it outside of WebKit. Done. > > > > There will be a WebCore::Intent class; I have that constructor in a separate CL which I'm going to send along soon. For now I just wanted some simple method implementations so I can check in the adjustments to our Chromium code that will use these accessors. I could take the constructors out completely. Shall I just do that? > > I guess you can initialize these classes by just calling the setFoo methods, right? Sounds good. I just replaced them with empty constructors; I'll adapt to that in the other CLs when I merge.

Created attachment 116455 [details] Patch

Comment on attachment 116455 [details] Patch Clearing flags on attachment: 116455 Committed r101123: <http://trac.webkit.org/changeset/101123>

All reviewed patches have been landed. Closing bug.