Bug 73036 - Add simple implementation for web intents chromium API data classes.
: Add simple implementation for web intents chromium API data classes.
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 75123
  Show dependency treegraph
 
Reported: 2011-11-23 11:51 PST by
Modified: 2011-12-22 14:09 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-11-23 11:51:22 PST
Add simple implementation for web intents chromium API data classes.
------- Comment #1 From 2011-11-23 11:51:54 PST -------
Created an attachment (id=116388) [details]
Patch
------- Comment #2 From 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 From 2011-11-23 12:14:20 PST -------
(From update of attachment 116388 [details])
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 From 2011-11-23 13:17:18 PST -------
(From update of attachment 116388 [details])
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 From 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 From 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 From 2011-11-23 16:52:07 PST -------
Created an attachment (id=116455) [details]
Patch
------- Comment #8 From 2011-11-23 23:46:22 PST -------
(From update of attachment 116455 [details])
Clearing flags on attachment: 116455

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