Bug 73036 - Add simple implementation for web intents chromium API data classes.
Summary: Add simple implementation for web intents chromium API data classes.
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: Nobody
URL:
Keywords:
Depends on:
Blocks: 75123
  Show dependency treegraph
 
Reported: 2011-11-23 11:51 PST by Greg Billock
Modified: 2011-12-22 14:09 PST (History)
3 users (show)

See Also:


Attachments
Patch (10.79 KB, patch)
2011-11-23 11:51 PST, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (9.73 KB, patch)
2011-11-23 16:52 PST, Greg Billock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Billock 2011-11-23 11:51:22 PST
Add simple implementation for web intents chromium API data classes.
Comment 1 Greg Billock 2011-11-23 11:51:54 PST
Created attachment 116388 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-23 11:54:38 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Darin Fisher (:fishd, Google) 2011-11-23 12:14:20 PST
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 4 Greg Billock 2011-11-23 13:17:18 PST
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?
Comment 5 Darin Fisher (:fishd, Google) 2011-11-23 14:19:55 PST
(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?
Comment 6 Greg Billock 2011-11-23 16:48:50 PST
(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.
Comment 7 Greg Billock 2011-11-23 16:52:07 PST
Created attachment 116455 [details]
Patch
Comment 8 WebKit Review Bot 2011-11-23 23:46:22 PST
Comment on attachment 116455 [details]
Patch

Clearing flags on attachment: 116455

Committed r101123: <http://trac.webkit.org/changeset/101123>
Comment 9 WebKit Review Bot 2011-11-23 23:46:27 PST
All reviewed patches have been landed.  Closing bug.