Bug 73051 – WebCore implementation of the Intent object.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Greg Billock]

[Web Intents] Flagged-off WebCore implementation of navigator.startActivity

[Greg Billock]

Created attachment 116428 [details]
Patch

[WebKit Review Bot]

Comment on attachment 116428 [details]
Patch

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

[Dominic Cooney]

Comment on attachment 116428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116428&action=review

I am super excited about Web Intents being available in WebKit! I have made some suggestions inline.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Probably worth either:

- adding tests
- commenting why no new tests are required

> Source/WebCore/ChangeLog:9
> +

It might be useful to add a link to the draft spec of what you’re implementing here.

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:47
> +v8::Handle<v8::Value> V8Intent::constructorCallback(const v8::Arguments& args)

FYI This has to be a custom method for now, but haraken is working on making this unnecessary using Constructor(…) metadata. I think the only thing blocking you is that Constructor attribute does not support overloads yet.

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:55
> +        fprintf(stderr, "Creating empty intent\n");

Probably want to remove this ad-hoc logging?

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:107
> +v8::Handle<v8::Value> V8Intent::dataAccessorGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)

Code generation will *almost* do this for you. I believe that the only difference is that it would give null for empty script values, whereas you give undefined. How does specing returning undefined line up with other DOM attributes that are SerializedScriptValue? If others return null, you should just use generation. If others return undefined, you might consider teaching the code generators about SerializedScriptValue?.

> Source/WebCore/loader/FrameLoaderClient.h:83
>      class IntSize;

Isn’t S < e?

> Source/WebCore/loader/FrameLoaderClient.h:327
> +        virtual void dispatchIntent(const Intent& intent, int id) { }

The "intent" parameter name does not add anything that isn’t in the type, so omit the name. id is OK, natch.

> Source/WebCore/page/Intent.cpp:38
> +Intent::Intent()

Why not simply have one constructor and put the default values in ::create overloads?

> Source/WebCore/page/Intent.cpp:52
> +    setAction(action);

It might be more typical in WebKit to use : m_action(action), … style initializers… why call the setters?

> Source/WebCore/page/Intent.cpp:82
> +void Intent::setData(PassRefPtr<SerializedScriptValue> prpData)

I believe the WebKit style guide on naming parameters prpFoo is that prpFoo is an acceptable prefix when the use of the parameter is non-trivial and to that end it is being transferred to a RefPtr at the start of the function. That isn’t what is happening here.

> Source/WebCore/page/Intent.cpp:87
> +      m_data = SerializedScriptValue::nullValue();

This needs to be indented two more spaces.

> Source/WebCore/page/Intent.idl:33
> +        readonly attribute DOMString action;

Why have all of the setters in C++ then?

> Source/WebCore/page/IntentResultCallback.idl:32
> +        boolean handleEvent(in SerializedScriptValue result);

This looks to be indented too far… I think IDL just indents four.

> Source/WebCore/page/IntentsController.h:44
> +    int getIntentId(PassRefPtr<IntentResultCallback> successCallback, PassRefPtr<IntentResultCallback> errorCallback);

It seems odd to me that a method named *get*-something to not be idempotent. Why not call it

assignIntentId

or something to hint at its side-effecting nature?

> Source/WebCore/page/Navigator.cpp:331
> +        ec = VALIDATION_ERR;

FYI DOM4 seems to deprecate this: <http://www.w3.org/TR/domcore/#dom-domexception-validation_err>

[Dominic Cooney]

FYI I have filed bug 73064 for implementing overloaded constructor support in the generators. There is no need for that to block this though; we can delete the custom constructor in a clean-up pass.

[Adam Barth]

Comment on attachment 116428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116428&action=review

> Source/WebCore/WebCore.gypi:2972
> +            'page/Intent.cpp',
> +            'page/Intent.h',
> +            'page/IntentsController.cpp',
> +            'page/IntentsController.h',
> +            'page/IntentResultCallback.h',

Can you make an intents directory in Modules?  "page" shouldn't really be a dumping group for every random feature.

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:111
> +    SerializedScriptValue* ssv = impl->data();

ssv => please use complete words in variable names.

> Source/WebCore/page/DOMWindow.idl:622
> +#if defined(ENABLE_WEB_INTENTS) && ENABLE_WEB_INTENTS
> +        attribute IntentConstructor Intent; // Usable with the new operator
> +#endif

This is an ideal candidate for Supplemental (which doesn't yet exist).

> Source/WebCore/page/Intent.h:50
> +    static PassRefPtr<Intent> create(
> +        const String& action,
> +        const String& type) {

One line please.

> Source/WebCore/page/Intent.h:71
> +    Intent();
> +    Intent(String action, String type);
> +    Intent(String action, String type, PassRefPtr<SerializedScriptValue> prpData);

Can these all be combined with default values?

> Source/WebCore/page/Navigator.idl:72
> +#if defined(ENABLE_WEB_INTENTS) && ENABLE_WEB_INTENTS
> +        void startActivity(in Intent intent, in [Callback, Optional] IntentResultCallback successCallback, in [Callback, Optional] IntentResultCallback failureCallback)
> +            raises(DOMException);
> +#endif

Another good candidate for Supplemental.

> Source/WebCore/page/Page.cpp:149
> +#if ENABLE(WEB_INTENTS)
> +    , m_intentsController(adoptPtr(new IntentsController()))
> +#endif

Why is this controller per-page?  Naively, I would have expected it to be per-ScriptExecutionContext.

[Greg Billock]

Comment on attachment 116428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116428&action=review

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> Probably worth either:
> 
> - adding tests
> - commenting why no new tests are required

I definitely need tests. Is there an example patch that has flagged-off tests that I can use for inspiration?

>> Source/WebCore/ChangeLog:9
>> +
> 
> It might be useful to add a link to the draft spec of what you’re implementing here.

Done.

>> Source/WebCore/WebCore.gypi:2972
>> +            'page/IntentResultCallback.h',
> 
> Can you make an intents directory in Modules?  "page" shouldn't really be a dumping group for every random feature.

Done.

>> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:47
>> +v8::Handle<v8::Value> V8Intent::constructorCallback(const v8::Arguments& args)
> 
> FYI This has to be a custom method for now, but haraken is working on making this unnecessary using Constructor(…) metadata. I think the only thing blocking you is that Constructor attribute does not support overloads yet.

That'd be nice!

>> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:55
>> +        fprintf(stderr, "Creating empty intent\n");
> 
> Probably want to remove this ad-hoc logging?

Oops! Thought I got it all. Done.

>> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:107
>> +v8::Handle<v8::Value> V8Intent::dataAccessorGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
> 
> Code generation will *almost* do this for you. I believe that the only difference is that it would give null for empty script values, whereas you give undefined. How does specing returning undefined line up with other DOM attributes that are SerializedScriptValue? If others return null, you should just use generation. If others return undefined, you might consider teaching the code generators about SerializedScriptValue?.

I had a lot of trouble with the SSV accessors. It looks like the V8 code gen special cases it to basically bypass the accessor in the class and return built-in values, which is not what I need. I'm not exactly sure what to do about it at this point.

>> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:111
>> +    SerializedScriptValue* ssv = impl->data();
> 
> ssv => please use complete words in variable names.

Done.

>> Source/WebCore/loader/FrameLoaderClient.h:83
>>      class IntSize;
> 
> Isn’t S < e?

Done.

>> Source/WebCore/page/DOMWindow.idl:622
>> +#endif
> 
> This is an ideal candidate for Supplemental (which doesn't yet exist).

So this would be like

DOMWindow implements IntentObjectConstructorInterface

which is then defined separately? (It looks to me from the WebIDL doc that supplemental interfaces are just a category of interface. Is there something syntactic in the works?)

>> Source/WebCore/page/Intent.cpp:38
>> +Intent::Intent()
> 
> Why not simply have one constructor and put the default values in ::create overloads?

I realized this was all redundant to the custom constructor, so I just put the setup there.

>> Source/WebCore/page/Intent.cpp:52
>> +    setAction(action);
> 
> It might be more typical in WebKit to use : m_action(action), … style initializers… why call the setters?

Done.

>> Source/WebCore/page/Intent.cpp:82
>> +void Intent::setData(PassRefPtr<SerializedScriptValue> prpData)
> 
> I believe the WebKit style guide on naming parameters prpFoo is that prpFoo is an acceptable prefix when the use of the parameter is non-trivial and to that end it is being transferred to a RefPtr at the start of the function. That isn’t what is happening here.

Drat. I thought I read that right. Changed to just 'data'.

>> Source/WebCore/page/Intent.cpp:87
>> +      m_data = SerializedScriptValue::nullValue();
> 
> This needs to be indented two more spaces.

Done.

>> Source/WebCore/page/Intent.h:50
>> +        const String& type) {
> 
> One line please.

Erased these methods.

>> Source/WebCore/page/Intent.h:71
>> +    Intent(String action, String type, PassRefPtr<SerializedScriptValue> prpData);
> 
> Can these all be combined with default values?

Yes. See above.

>> Source/WebCore/page/Intent.idl:33
>> +        readonly attribute DOMString action;
> 
> Why have all of the setters in C++ then?

Oops. That code didn't evolve with this interface. Fixed now.

>> Source/WebCore/page/IntentResultCallback.idl:32
>> +        boolean handleEvent(in SerializedScriptValue result);
> 
> This looks to be indented too far… I think IDL just indents four.

Fixed.

>> Source/WebCore/page/IntentsController.h:44
>> +    int getIntentId(PassRefPtr<IntentResultCallback> successCallback, PassRefPtr<IntentResultCallback> errorCallback);
> 
> It seems odd to me that a method named *get*-something to not be idempotent. Why not call it
> 
> assignIntentId
> 
> or something to hint at its side-effecting nature?

Makes sense. Done.

>> Source/WebCore/page/Navigator.cpp:331
>> +        ec = VALIDATION_ERR;
> 
> FYI DOM4 seems to deprecate this: <http://www.w3.org/TR/domcore/#dom-domexception-validation_err>

So should this be SYNTAX_ERR?

>> Source/WebCore/page/Page.cpp:149
>> +#endif
> 
> Why is this controller per-page?  Naively, I would have expected it to be per-ScriptExecutionContext.

I put it here just because other similar controllers seemed to be here. (Device orientation, speech, client geolocation, ...) The DB controllers are on ScriptExecutionContext, however. I am happy to move it. Are there lifetime or scoping considerations that point one way or another?

I'm using this controller to connect up which intent flows need to be notified back to which callbacks, so the scoping needs to be for as long as that callback can be reached.

[Greg Billock]

Created attachment 116825 [details]
Patch

[WebKit Review Bot]

Attachment 116825 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/Modules/intents/IntentResultCallback.h:44:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:54:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Adam Barth]

Comment on attachment 116825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116825&action=review

> Source/WebCore/page/IntentsController.h:39
> +class IntentsController {

This should go in Modules/intents

[Early Warning System Bot]

Comment on attachment 116825 [details]
Patch

Attachment 116825 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10676468

[Gyuyoung Kim]

Comment on attachment 116825 [details]
Patch

Attachment 116825 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10678436

[Darin Fisher (:fishd, Google)]

Comment on attachment 116825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116825&action=review

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1623
> +    WebIntent webIntent(intent, intentId);

if the identifier is not part of WebCore::Intent, then it probably shouldn't be part of WebIntent either.
i know we went back-n-forth on the API design, and so you may be loath to change it, but just wanted to
point out that we should be consistent here.

[WebKit Review Bot]

Comment on attachment 116825 [details]
Patch

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

[David Levin]

Comment on attachment 116825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116825&action=review

Just a few drive by comments.

> Source/WebCore/Modules/intents/Intent.cpp:40
> +    : m_action(action),

fyi, the , goes on the next line. (Here and other places.)

> Source/WebCore/Modules/intents/Intent.h:49
> +        PassRefPtr<SerializedScriptValue> data) {

{ on the next line.

> Source/WebCore/Modules/intents/IntentResultCallback.h:41
> +class IntentResultCallback : public ThreadSafeRefCounted<IntentResultCallback> {

Why is this a ThreadSafeRefCounted? That is a lot to force on another class (and it seems odd to make it part of a callback interface).

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:43
> +#include <wtf/MathExtras.h>

Is the code using something from this header? (I don't see it.)

> Source/WebCore/page/Page.cpp:148
> +    , m_intentsController(adoptPtr(new IntentsController()))

adoptPtr(new IntentsController()) should be IntentsController::create() (like what you did for Intent except in this case it returns a PassOwnPtr).

[Sam Weinig]

Comment on attachment 116825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116825&action=review

> Source/WebCore/Modules/intents/Intent.h:58
> +    Intent(String action, String type, PassRefPtr<SerializedScriptValue> data);

These should be const String&.

> Source/WebCore/page/IntentsController.h:47
> +    void postResult(String& data, int intentId);
> +    void postFailure(String& data, int intentId);

Are these String parameters used as out data? If not, they should be const String&.

[Greg Billock]

Question for reviewers: the bots are giving me build errors, which look like they aren't processing the includes updates in WebCore.gyp/WebCore.gyp. Do I need to split out that change to add the Modules/intents directory?

[Greg Billock]

Comment on attachment 116825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116825&action=review

>> Source/WebCore/Modules/intents/Intent.cpp:40
>> +    : m_action(action),
> 
> fyi, the , goes on the next line. (Here and other places.)

Done.

>> Source/WebCore/Modules/intents/Intent.h:49
>> +        PassRefPtr<SerializedScriptValue> data) {
> 
> { on the next line.

Done.

>> Source/WebCore/Modules/intents/Intent.h:58
>> +    Intent(String action, String type, PassRefPtr<SerializedScriptValue> data);
> 
> These should be const String&.

Done.

>> Source/WebCore/Modules/intents/IntentResultCallback.h:41
>> +class IntentResultCallback : public ThreadSafeRefCounted<IntentResultCallback> {
> 
> Why is this a ThreadSafeRefCounted? That is a lot to force on another class (and it seems odd to make it part of a callback interface).

I think I was looking at storage/DatabaseCallback.h, but I think that misfired, and the callback interfaces more aligned with this use just RefCounted<>. Fixed.

>> Source/WebCore/Modules/intents/IntentResultCallback.h:44
>> +    virtual bool handleEvent(RefPtr<SerializedScriptValue> result) = 0;
> 
> The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]

Can I just pass a pointer here? PassRefPtr definitely doesn't work.

>> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:43
>> +#include <wtf/MathExtras.h>
> 
> Is the code using something from this header? (I don't see it.)

Thanks. Must be left over from a copy-paste.

>> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:54
>> +    if (args.Length() == 0) {
> 
> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]

Is this right? I think it'd look odd to see "if (!args.Length())," and v8::Arguments doesn't have an empty(). I see instances in the code of both forms, though.

>> Source/WebCore/page/IntentsController.h:39
>> +class IntentsController {
> 
> This should go in Modules/intents

Done.

>> Source/WebCore/page/IntentsController.h:47
>> +    void postFailure(String& data, int intentId);
> 
> Are these String parameters used as out data? If not, they should be const String&.

Done.

>> Source/WebCore/page/Page.cpp:148
>> +    , m_intentsController(adoptPtr(new IntentsController()))
> 
> adoptPtr(new IntentsController()) should be IntentsController::create() (like what you did for Intent except in this case it returns a PassOwnPtr).

OK. I was looking at parallels in this file, where adoptPtr(new Xxx) is pretty ubiquitous.

>> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1623
>> +    WebIntent webIntent(intent, intentId);
> 
> if the identifier is not part of WebCore::Intent, then it probably shouldn't be part of WebIntent either.
> i know we went back-n-forth on the API design, and so you may be loath to change it, but just wanted to
> point out that we should be consistent here.

I'll add it here.

[David Levin]

(In reply to comment #18)
> (From update of attachment 116825 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116825&action=review

> >> Source/WebCore/Modules/intents/IntentResultCallback.h:44
> >> +    virtual bool handleEvent(RefPtr<SerializedScriptValue> result) = 0;
> > 
> > The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
> 
> Can I just pass a pointer here? PassRefPtr definitely doesn't work.

I wonder why PassRefPtr doesn't work. Anyway it all depends on what the method does.

If it takes ownership, make it PassRefPtr. If it just needs a pointer, then pass a pointer.

> >> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:54
> >> +    if (args.Length() == 0) {
> > 
> > Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
> 
> Is this right? I think it'd look odd to see "if (!args.Length())," and v8::Arguments doesn't have an empty(). I see instances in the code of both forms, though.


Yep, it is correct. Odd but true. See WebKit style document.


> >> Source/WebCore/page/Page.cpp:148
> >> +    , m_intentsController(adoptPtr(new IntentsController()))
> > 
> > adoptPtr(new IntentsController()) should be IntentsController::create() (like what you did for Intent except in this case it returns a PassOwnPtr).
> 
> OK. I was looking at parallels in this file, where adoptPtr(new Xxx) is pretty ubiquitous.

Feel free to fix them as well :).

[Greg Billock]

Created attachment 117044 [details]
Patch

[WebKit Review Bot]

Attachment 117044 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/Modules/intents/IntentResultCallback.h:44:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:52:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 117044 [details]
Patch

Attachment 117044 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10679747

[Gyuyoung Kim]

Comment on attachment 117044 [details]
Patch

Attachment 117044 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10677830

[Greg Billock]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 116825 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=116825&action=review
> 
> > >> Source/WebCore/Modules/intents/IntentResultCallback.h:44
> > >> +    virtual bool handleEvent(RefPtr<SerializedScriptValue> result) = 0;
> > > 
> > > The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
> > 
> > Can I just pass a pointer here? PassRefPtr definitely doesn't work.
> 
> I wonder why PassRefPtr doesn't work. Anyway it all depends on what the method does.
> 
> If it takes ownership, make it PassRefPtr. If it just needs a pointer, then pass a pointer.

I believe it needs to take ownership. (It passes it to the Javascript callback; I don't keep a copy in the controller.) But looking at generate-bindings.pl, it looks to me like it'll always want this prototype to be a RefPtr or a pointer. (GetNativeTypeForCallbacks->GetNativeType, line 3344). (There are special cases for particular types, but those look like historical allowances.)

I admit though that my understanding of how the flow of control passes between my code and the generated code is imperfect. Perhaps the JS layer takes care of the ownership if I pass a pointer?

> 
> > >> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:54
> > >> +    if (args.Length() == 0) {
> > > 
> > > Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
> > 
> > Is this right? I think it'd look odd to see "if (!args.Length())," and v8::Arguments doesn't have an empty(). I see instances in the code of both forms, though.
> 
> 
> Yep, it is correct. Odd but true. See WebKit style document.

I changed this. 
 
> > >> Source/WebCore/page/Page.cpp:148
> > >> +    , m_intentsController(adoptPtr(new IntentsController()))
> > > 
> > > adoptPtr(new IntentsController()) should be IntentsController::create() (like what you did for Intent except in this case it returns a PassOwnPtr).
> > 
> > OK. I was looking at parallels in this file, where adoptPtr(new Xxx) is pretty ubiquitous.
> 
> Feel free to fix them as well :).

Can do. I'll do it in a separate changelist though, so as to keep this already-long one from ballooning.

[David Levin]

/levin@ ducks out of this review and stops making more drive by comments.

[WebKit Review Bot]

Comment on attachment 117044 [details]
Patch

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

[Collabora GTK+ EWS bot]

Comment on attachment 117044 [details]
Patch

Attachment 117044 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10696473

[Greg Billock]

Created attachment 118908 [details]
Patch

[WebKit Review Bot]

Attachment 118908 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/Modules/intents/IntentResultCallback.h:44:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 118908 [details]
Patch

Attachment 118908 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10851134

[Gyuyoung Kim]

Comment on attachment 118908 [details]
Patch

Attachment 118908 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10845145

[Gustavo Noronha (kov)]

Comment on attachment 118908 [details]
Patch

Attachment 118908 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10845184

[Eric Seidel (no email)]

Comment on attachment 118908 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118908&action=review

> Source/WebCore/ChangeLog:11
> +        No new tests. (OOPS!)

Tests are required for changes to WebCore.  Especailly ones which touch the web apis. :)

[Greg Billock]

Created attachment 120372 [details]
Patch

[Greg Billock]

Comment on attachment 118908 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118908&action=review

>> Source/WebCore/ChangeLog:11
>> +        No new tests. (OOPS!)
> 
> Tests are required for changes to WebCore.  Especailly ones which touch the web apis. :)

:-) Definitely. I've added a test for the basic API functionality. Is there a good example to follow for testing the propagation of the startActivity() call? I have my own tests I run, but since the code requires a user gesture, and simply forwards out, it'll be a more complex test than these API ones.

[Adam Barth]

EventSender can be used in testing to trigger a user gesture.

[Early Warning System Bot]

Comment on attachment 120372 [details]
Patch

Attachment 120372 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10955021

[Adam Barth]

Comment on attachment 120372 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120372&action=review

This patch is too large and contains many problems.  I'd recommend breaking it down into smaller pieces so we can make sure everything gets a good review.  The most problematic piece of this patch is the IntentsController.  Can you help me understand what scope that object should have?

> Source/WebCore/Modules/intents/Intent.cpp:38
> +

Extra blank line here.

> Source/WebCore/Modules/intents/Intent.cpp:47
> +String Intent::action() const

const String&

> Source/WebCore/Modules/intents/Intent.cpp:52
> +String Intent::type() const

const String&

> Source/WebCore/Modules/intents/Intent.h:49
> +    static PassRefPtr<Intent> create(
> +        const String& action,
> +        const String& type,
> +        PassRefPtr<SerializedScriptValue> data)

All on one line pls

> Source/WebCore/Modules/intents/Intent.h:62
> +protected:
> +    Intent(const String& action, const String& type, PassRefPtr<SerializedScriptValue> data);

Why protected?  Does this class have subclasses?

> Source/WebCore/Modules/intents/IntentResultCallback.idl:29
> +      LegacyDefaultOptionalArguments,

This attribute should never be added to any IDL files.  It's only for legacy IDL files (and I think we've eradicated it for all of them by now).

> Source/WebCore/Modules/intents/IntentsController.cpp:60
> +    if (m_successCallbacks[intentId].get())

You should take() the callback out of the map and then handle the event.  The way you've written this code, you've got a potential re-entrancy problem.

> Source/WebCore/Modules/intents/IntentsController.cpp:71
> +    if (m_errorCallbacks[intentId].get())
> +        m_errorCallbacks[intentId]->handleEvent(value);

Same problem.  handleEvent can do arbitrary things.  There's no guantee this object even exists afterwards.

> Source/WebCore/Modules/intents/IntentsController.h:40
> +class IntentsController {

Should this be non-copyable?

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:45
> +v8::Handle<v8::Value> V8Intent::constructorCallback(const v8::Arguments& args)

We shouldn't need a custom constructor for this object.

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:94
> +v8::Handle<v8::Value> V8Intent::dataAccessorGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)

We shouldn't need custom code here either.

> Source/WebCore/page/DOMWindow.idl:623
> +        attribute IntentConstructor Intent; // Usable with the new operator

You can just used Conditional=WEB_INTENTS

> Source/WebCore/page/Navigator.cpp:328
> +    if (!frame() || !frame()->loader() || !frame()->loader()->client() || !intent.get()) {

frame()->loader() is always non-null.  frame()->loader()->client() is always non-null.

"!intent.get()" can just be "!intent"

> Source/WebCore/page/Navigator.cpp:343
> +    int id = m_frame->page()->intentsController()->assignIntentId(successCallback, errorCallback);

You did a bunch of extra null checking, but you forgot to null check m_frame->page(), which actually can be null.

It's very unlikely that intentsController should be a page-scoped object.  Shouldn't we have one per ScriptExecutionContext?

> Source/WebCore/page/Navigator.h:85
> +    void startActivity(PassRefPtr<Intent>, PassRefPtr<IntentResultCallback> successCallback, PassRefPtr<IntentResultCallback> errorCallback, ExceptionCode&);

PassRefPtr<Intent> should just be const Intent&.  This function doesn't take ownership of the intent.  Please see http://www.webkit.org/coding/RefPtr.html

> Source/WebCore/page/Page.cpp:158
> +#if ENABLE(WEB_INTENTS)
> +    , m_intentsController(IntentsController::create())
> +#endif

Why Page-lifetime?  This seems unlikely.  Can you show me where in the spec it says how long we need to retain this state?

> Source/WebKit/chromium/public/WebIntent.h:65
> +    WebIntent(const WebCore::Intent&);

explicit

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1820
> +    WTF::String replyString = reply.operator String();

Do we really have to use the word "operator" here?  Shouldn't the compiler be able to figure that out for us?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1821
> +    m_frame->page()->intentsController()->postResult(replyString, intentIdentifier);

How do you know this result is going back to the right security context?  Page can contain many different documents over its lifetime.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1831
> +    WTF::String replyString = reply.operator String();

Are we really not "using WTF::String" in this file?

> Source/WebKit/chromium/src/WebIntent.cpp:46
> +    : m_action(intent.action())
> +    , m_type(intent.type())
> +    , m_data(intent.data()->toWireString())
> +    , m_identifier(intent.identifier())

Why do we copy out all these fields instead of just take a reference to the WebCore::Intent object?  That's the usual pattern for API objects that wrap WebCore concepts.

> LayoutTests/platform/wincairo/Skipped:2005
> +web-intents/

This should probably be one word, without the -

[WebKit Review Bot]

Attachment 120372 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/platform/efl/Skipped', u'Layou..." exit_code: 1

Source/WebCore/Modules/intents/IntentResultCallback.h:44:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 1 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Greg Billock]

Comment on attachment 120372 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120372&action=review

>> Source/WebCore/Modules/intents/Intent.cpp:38
>> +
> 
> Extra blank line here.

Done.

>> Source/WebCore/Modules/intents/Intent.cpp:47
>> +String Intent::action() const
> 
> const String&

Done. (I was following some other classes in page/ which use this idiom and perhaps need updating -- Geolocation.h, SecurityOrigin.h, Location.h, probably more.)

>> Source/WebCore/Modules/intents/Intent.cpp:52
>> +String Intent::type() const
> 
> const String&

Done.

>> Source/WebCore/Modules/intents/Intent.h:49
>> +        PassRefPtr<SerializedScriptValue> data)
> 
> All on one line pls

Done.

>> Source/WebCore/Modules/intents/Intent.h:62
>> +    Intent(const String& action, const String& type, PassRefPtr<SerializedScriptValue> data);
> 
> Why protected?  Does this class have subclasses?

No good reason at this point, although we've discussed subclasses as an option as an implementation details for some use cases. There's no reason to have this now, though. I was likely mimicking another class in an attempt to figure out how the IDL and codegen'ed headers relate. (I'm still uncertain about some of the details there, like handleEvent in the callback, in which the codegen won't work with what seems like the right PassRefPtr argument.)

>> Source/WebCore/Modules/intents/IntentResultCallback.idl:29
>> +      LegacyDefaultOptionalArguments,
> 
> This attribute should never be added to any IDL files.  It's only for legacy IDL files (and I think we've eradicated it for all of them by now).

Removed. Are there other IDL arguments I need for a callback type?

>> Source/WebCore/Modules/intents/IntentsController.cpp:60
>> +    if (m_successCallbacks[intentId].get())
> 
> You should take() the callback out of the map and then handle the event.  The way you've written this code, you've got a potential re-entrancy problem.

Thanks. I don't feel like I have a good handle yet on how the thread hand-offs and ownerships transfers work between this code and the Javascript engine. 

I've changed the code to be robust against re-entrancy now. I added a mutex to the ID generation process. I'm not sure whether the Javascript engine needs that or not. Is is possible for invocations from JS to be re-entrant?

>> Source/WebCore/Modules/intents/IntentsController.cpp:71
>> +        m_errorCallbacks[intentId]->handleEvent(value);
> 
> Same problem.  handleEvent can do arbitrary things.  There's no guantee this object even exists afterwards.

Fixed.

>> Source/WebCore/Modules/intents/IntentsController.h:40
>> +class IntentsController {
> 
> Should this be non-copyable?

Yes. Done.

>> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:45
>> +v8::Handle<v8::Value> V8Intent::constructorCallback(const v8::Arguments& args)
> 
> We shouldn't need a custom constructor for this object.

There's work afoot on this (perhaps it is now done).

>> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:94
>> +v8::Handle<v8::Value> V8Intent::dataAccessorGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
> 
> We shouldn't need custom code here either.

I think this is the result of v8 treatment of the SerializedScriptValue type. Basically I want this field to contain anything that SerializedScriptValue can handle, and nothing it can't. There are a couple other instances where we use this type in IDL, but it has some semantic problems when we do. See Dominic's comment #3 above in this thread.

>> Source/WebCore/page/DOMWindow.idl:623
>> +        attribute IntentConstructor Intent; // Usable with the new operator
> 
> You can just used Conditional=WEB_INTENTS

Done.

>> Source/WebCore/page/Navigator.cpp:328
>> +    if (!frame() || !frame()->loader() || !frame()->loader()->client() || !intent.get()) {
> 
> frame()->loader() is always non-null.  frame()->loader()->client() is always non-null.
> 
> "!intent.get()" can just be "!intent"

Done.

>> Source/WebCore/page/Navigator.cpp:343
>> +    int id = m_frame->page()->intentsController()->assignIntentId(successCallback, errorCallback);
> 
> You did a bunch of extra null checking, but you forgot to null check m_frame->page(), which actually can be null.
> 
> It's very unlikely that intentsController should be a page-scoped object.  Shouldn't we have one per ScriptExecutionContext?

Yeah, this is a good question, and I'm not really sure. The constraint is that when an intent reply comes back, we can't map it to an incorrect callback. The way I understand the Page/ScriptExecutionContext lifetimes, we could get a new ScriptExecutionContext which might then grab an ID which is duplicate to one that had already been issued, so we need to make sure the counter is scoped at least to the Page. I've switched this code to just use a static counter, though, so I believe that means it can be moved to share lifetime with the ScriptExecutionContext. How does ref counting on the callbacks work? Since I'm storing those in the registry, will they be inactivated if the ScriptExecutionContext they are associated with gets deleted? If they can be deleted out from under the registry then the lifetime needs to be identical to the script context, or if it is hard to keep track of the lifetime, the same argument applies.

>> Source/WebCore/page/Navigator.h:85
>> +    void startActivity(PassRefPtr<Intent>, PassRefPtr<IntentResultCallback> successCallback, PassRefPtr<IntentResultCallback> errorCallback, ExceptionCode&);
> 
> PassRefPtr<Intent> should just be const Intent&.  This function doesn't take ownership of the intent.  Please see http://www.webkit.org/coding/RefPtr.html

I'm setting the identifier on this object. I can pass a pointer though.

>> Source/WebCore/page/Page.cpp:158
>> +#endif
> 
> Why Page-lifetime?  This seems unlikely.  Can you show me where in the spec it says how long we need to retain this state?

Discussed above.

>> Source/WebKit/chromium/public/WebIntent.h:65
>> +    WebIntent(const WebCore::Intent&);
> 
> explicit

Done

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1821
>> +    m_frame->page()->intentsController()->postResult(replyString, intentIdentifier);
> 
> How do you know this result is going back to the right security context?  Page can contain many different documents over its lifetime.

The ID should ensure that only the registered callbacks get notified for a given intent. I think I'm doing this correctly now.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1831
>> +    WTF::String replyString = reply.operator String();
> 
> Are we really not "using WTF::String" in this file?

Deleted namespace.

>> Source/WebKit/chromium/src/WebIntent.cpp:46
>> +    , m_identifier(intent.identifier())
> 
> Why do we copy out all these fields instead of just take a reference to the WebCore::Intent object?  That's the usual pattern for API objects that wrap WebCore concepts.

Is there an example of doing that and how it interacts with wanting to flag off the WebCore type?

>> LayoutTests/platform/wincairo/Skipped:2005
>> +web-intents/
> 
> This should probably be one word, without the -

Done.

[Greg Billock]

Created attachment 120415 [details]
Patch

[Greg Billock]

Addressed comments, but splitting this up sounds good to me. Shall I split into several smaller WebCore pieces and then one to do the rest of the API to take the invocation out to client code? There's several interdependencies, but I think I can find a good ordering.

[WebKit Review Bot]

Attachment 120415 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/platform/efl/Skipped', u'Layou..." exit_code: 1

Source/WebCore/Modules/intents/IntentResultCallback.h:44:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 1 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

(In reply to comment #40)
> (From update of attachment 120372 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120372&action=review
> 
> >> Source/WebCore/Modules/intents/Intent.cpp:47
> >> +String Intent::action() const
> > 
> > const String&
> 
> Done. (I was following some other classes in page/ which use this idiom and perhaps need updating -- Geolocation.h, SecurityOrigin.h, Location.h, probably more.)

Generally it depends on whether someone is keeping the string alive for you.  It's possible we don't do this correctly everywhere, but this case is clearly just an accessor for a member variable.  There's no reason to churn the reference count on the string buffer.

> >> Source/WebCore/Modules/intents/Intent.h:62
> >> +    Intent(const String& action, const String& type, PassRefPtr<SerializedScriptValue> data);
> > 
> > Why protected?  Does this class have subclasses?
> 
> No good reason at this point, although we've discussed subclasses as an option as an implementation details for some use cases.

Ok.  Let's make this private for now.  We can always make it protected later if there's a good reason to do that.

> >> Source/WebCore/Modules/intents/IntentResultCallback.idl:29
> >> +      LegacyDefaultOptionalArguments,
> > 
> > This attribute should never be added to any IDL files.  It's only for legacy IDL files (and I think we've eradicated it for all of them by now).
> 
> Removed. Are there other IDL arguments I need for a callback type?

Not that I know of.  Your tests should demonstrate that one way or another.

> >> Source/WebCore/Modules/intents/IntentsController.cpp:60
> >> +    if (m_successCallbacks[intentId].get())
> > 
> > You should take() the callback out of the map and then handle the event.  The way you've written this code, you've got a potential re-entrancy problem.
> 
> Thanks. I don't feel like I have a good handle yet on how the thread hand-offs and ownerships transfers work between this code and the Javascript engine. 

There are no thread hand-offs.  :)

WebCore proper needs to keep everything alive itself.  Anything that the JavaScript engine is keeping alive can disappear at any moment when you execute JavaScript.

> I've changed the code to be robust against re-entrancy now. I added a mutex to the ID generation process. I'm not sure whether the Javascript engine needs that or not.

It's very unlikely that we need a mutex.  There is only one thread in the DOM.

> Is is possible for invocations from JS to be re-entrant?

Yes.  JavaScript can WebCore can be on the stack interleaved an arbitrary number of times.

> >> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:94
> >> +v8::Handle<v8::Value> V8Intent::dataAccessorGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
> > 
> > We shouldn't need custom code here either.
> 
> I think this is the result of v8 treatment of the SerializedScriptValue type. Basically I want this field to contain anything that SerializedScriptValue can handle, and nothing it can't. There are a couple other instances where we use this type in IDL, but it has some semantic problems when we do. See Dominic's comment #3 above in this thread.

We should use the code generator here.  If we're an inch away from generating the code we need, then we should advance the code generator that inch.  It's also possible, however, that the code generator is right and that the WebIntents spec is wrong w.r.t. null/undefined.  That's one of the reasons we like the code generator: it helps us be consistent.

> >> Source/WebCore/page/Navigator.cpp:343
> >> +    int id = m_frame->page()->intentsController()->assignIntentId(successCallback, errorCallback);
> > 
> > You did a bunch of extra null checking, but you forgot to null check m_frame->page(), which actually can be null.
> > 
> > It's very unlikely that intentsController should be a page-scoped object.  Shouldn't we have one per ScriptExecutionContext?
> 
> Yeah, this is a good question, and I'm not really sure. The constraint is that when an intent reply comes back, we can't map it to an incorrect callback. The way I understand the Page/ScriptExecutionContext lifetimes, we could get a new ScriptExecutionContext which might then grab an ID which is duplicate to one that had already been issued, so we need to make sure the counter is scoped at least to the Page. I've switched this code to just use a static counter, though, so I believe that means it can be moved to share lifetime with the ScriptExecutionContext.

Great.  A static counter is almost certainly the right thing.  Having a per-Page counter is a recipe for sadness.

> How does ref counting on the callbacks work?

I'm not sure I can answer your question at this level of generality.  Callbacks are RefCounted objects.  If you want to keep them alive, you need to store them in a RefPtr.

> Since I'm storing those in the registry, will they be inactivated if the ScriptExecutionContext they are associated with gets deleted?

Yes, but if their script execution context is gone, you can't execute them anyway.

> If they can be deleted out from under the registry then the lifetime needs to be identical to the script context, or if it is hard to keep track of the lifetime, the same argument applies.

I don't really understand what you're saying here.  If you hold a RefPtr to them, they'll stay alive.  If you call them, they'll run.  It's unlikely that we'll want to run them if their script execution context is no longer active (i.e., displayed in a Frame).

The WebIntents spec should contain all of these details.  If the spec is missing these details, we need to add them.  The general philosophy is that the web platform should not expose any details of JavaScript garbage collection to the platform.  That means the behavior of these callbacks needs to be defined without depending on any details of garbage collection.  Concretely, that means we need to define precisely when the callback will and will not be called in the specification and then we need to implement the specification.

> >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1821
> >> +    m_frame->page()->intentsController()->postResult(replyString, intentIdentifier);
> > 
> > How do you know this result is going back to the right security context?  Page can contain many different documents over its lifetime.
> 
> The ID should ensure that only the registered callbacks get notified for a given intent. I think I'm doing this correctly now.

Ok.

> >> Source/WebKit/chromium/src/WebIntent.cpp:46
> >> +    , m_identifier(intent.identifier())
> > 
> > Why do we copy out all these fields instead of just take a reference to the WebCore::Intent object?  That's the usual pattern for API objects that wrap WebCore concepts.
> 
> Is there an example of doing that and how it interacts with wanting to flag off the WebCore type?

http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/public/WebNode.h is the canonical example.  I'm not sure what you mean by "flag off" but I believe we define the API assuming all features are enabled and then have stub implementations for features that are not enabled.

[Adam Barth]

Comment on attachment 120415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120415&action=review

Anyway, this review is mixing together many issues because this patch is way too large.  Please break the patch up in the small, reviewable pieces and we'll cover these issues as we get to them.

> Source/WebCore/Modules/intents/IntentsController.cpp:53
> +    static Mutex lastUsedIntentIdMutex;
> +    MutexLocker locker(lastUsedIntentIdMutex);

We shouldn't have a mutex.  The DOM has only one thread.

[David Levin]

Comment on attachment 120415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120415&action=review

>> Source/WebCore/Modules/intents/IntentsController.cpp:53
>> +    MutexLocker locker(lastUsedIntentIdMutex);
> 
> We shouldn't have a mutex.  The DOM has only one thread.

If it did, a static mutex would likely be incorrect and in this case, it should use atomicIncrement anyway.

Personally, I'd add an ASSERT(isMainThread()); just to document the lack of thread safety (when you get rid of the mutex, etc.) in case one day this comes to be used with Web Workers. (The assert will make it easier to find and fix this place.)

[Greg Billock]

Created attachment 121006 [details]
Patch

[Greg Billock]

OK, I've reduced the scope of this patch quite a bit. I'm going to make the intents controller co-exist with the ScriptExecutionContext separately and send that once we're happy with this. (It'll obviously depend on this code.) This patch now just focuses on the Intent object, IDL, "new Intent", and the layout test for that.

The biggest obstacle I think is the wish to get the code generator working for this case. I've left in the custom code for now, since the test passes and that's a good baseline, but I think we agree that the right approach is to get rid of that and do what's necessary for code gen to get us there. Dominic, did you get in patches that help close this gap? If there's anything left, I can work on it.

[Adam Barth]

Comment on attachment 121006 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121006&action=review

This looks great.  We just need to remove the custom bindings code.

> Source/WebCore/ChangeLog:3
> +        [Web Intents] Flagged-off WebCore implementation of navigator.startActivity

It would be nice to update this text to match the updated name of the bug.

> Source/WebKit/chromium/ChangeLog:5153
> +        * public/WebIntentsClient.h: Added.
> +        (WebKit::WebIntentsClient::~WebIntentsClient):
> +        * public/WebIntentsController.h: Added.
> +        * public/WebViewClient.h:
> +        (WebKit::WebViewClient::webIntentsClient):
> +        * src/WebIntentsController.cpp: Added.
> +        (WebKit::WebIntentsController::webIntentReply):

These changes look spurious

> Source/WebCore/Modules/intents/Intent.cpp:61
> +void Intent::setData(PassRefPtr<SerializedScriptValue> data)

Should this just be a SerializedScriptValue* ?  This function doesn't take ownership of the data.

> Source/WebCore/Modules/intents/Intent.cpp:63
> +    if (data.get())

No need for ".get()".  PassRefPtr has a bool operator.

> Source/WebCore/Modules/intents/Intent.idl:31
> +      CustomConstructFunction,
> +      V8CustomConstructor,

Why does this need a custom constructor?

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:45
> +v8::Handle<v8::Value> V8Intent::constructorCallback(const v8::Arguments& args)

This function is full of bugs.  We should just auto-generate it.

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:94
> +v8::Handle<v8::Value> V8Intent::dataAccessorGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)

This should be auto-generated as well.

[Adam Barth]

> OK, I've reduced the scope of this patch quite a bit.

Thanks for doing that.

> The biggest obstacle I think is the wish to get the code generator working for this case. I've left in the custom code for now, since the test passes and that's a good baseline, but I think we agree that the right approach is to get rid of that and do what's necessary for code gen to get us there. Dominic, did you get in patches that help close this gap? If there's anything left, I can work on it.

The problem is that the custom bindings code in the patch is full of bugs.  (Almost every line of code contains a bug of some sort.)  Don't feel bad about that.  It's true of almost all the hand-written bindings code.  That's why we autogenerate it.

[Adam Barth]

It shouldn't be too hard to teach the code generator how to handle these cases.  I think it might actually handle most of them already.  In the worst case, we can start with just one of the constructors and add the overloaded one once we've improved the code generator.

[Adam Barth]

I'm curious why you re-titled this bug back to it's old title.  I changed the title for you because the current title isn't the way we usually name bugs in WebKit.

1) A prefix like [Web Intents] is usually used to tell most reviewers to ignore your patch because it only affects a subset of ports (e.g., [Qt] or [V8]).  That's not appropriate here because we'd like to welcome all reviewers to look at this bug.

2) All feature work in WebKit occurs behind ifdefs, which means the phrase "flagged-off" isn't helpful.  Having that in the title just creates the impression that you'll looking for some sort of lax review because this feature isn't enabled by default in some configuration you have in mind.

[Greg Billock]

I've been doing some exploration with the code generator. Here's what I've found:

Setting the type to 'any' results in undefined objects -- the code generator doesn't treat these specially, and just in-place uses the string. So I think we're at quite a distance from using that convention, and it isn't really what we want anyway.

Using 'SerializedScriptValue' simply refuses to generate the right attribute functions. I think this is because of the exclusion in 1874 (GenerateSingleBatchedAttribute).

I also tried 'DOMObject', following MessageEvent. That generates code, but misses the SerializedScriptValue pathway in NativeToJSValue, resulting in a missing SerializedScriptValue::v8Value() call. Adding that isn't too hard, though.

So what's a good direction? I suspect the right approach is to make the codegen support SerializedScriptValue. It seems like it might be close, and expresses what we want to say in the spec and IDL. Seeing the MessageEvent, though, makes me suspect there are complications (it uses all custom code as well).

[Greg Billock]

Sorry for the description confusion. I think we may have mid-air collided. I thought I was changing the original title to something that didn't include "navigator.startActivity", which isn't really accurate. Removed the other miscues. Feel free to change it to something appropriate and helpful, though.

[Adam Barth]

(In reply to comment #54)

Thanks.  (Looking at your code generator question now.)

[Adam Barth]

It looks like http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm already has some special cases for SerializedScriptValue. 

http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/test/TestSerializedScriptValueInterface.idl seems to imply that we've already thought about the case of a SerializedScriptValue attribute.  Does using the type "SerializedScriptValue" not work the way you want?

[Greg Billock]

Created attachment 121125 [details]
Patch

[Greg Billock]

(In reply to comment #56)
> It looks like http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm already has some special cases for SerializedScriptValue. 
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/test/TestSerializedScriptValueInterface.idl seems to imply that we've already thought about the case of a SerializedScriptValue attribute.  Does using the type "SerializedScriptValue" not work the way you want?

I've uploaded a new patch with some adjustments to the code generator. As you'll see, it's not meant to be final; I'm not sure why the constraints on the codegen were in to eliminate SerializedScriptValue args from the constructor and getter functions. It looks like they are on purpose, though, and disabling them and making some changes to the object API results in things basically working.

Does this look like the thing to do? I need to adjust the bindings test to account for this, and add a test for the constructor. It looks to me like the SSV is intended to be supported in the constructor, but it is not, currently, without something like this patch. (There's no usage or test for it.) The getter exclusion I don't understand. Does it have to do with resolving the ambiguity of what happens if you did this:

var mine = {a: 'b'};
var i = new Intent("a", "t", mine);
mine.a = c;

now what is i.data.a?

It seems to me that documenting the param as readonly resolves this (it should be 'b'). But is V8 doing things behind the scenes with reusing these objects that makes that contract undesirable?

In my create() function, I'm just copying off the input, so I can do error checking and so forth there (I haven't moved that logic from the custom constructor to the Intent object in the patch yet, pending feedback over whether this is the right approach.)

[Adam Barth]

Comment on attachment 121125 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121125&action=review

I don't fully understand the issue you're running into.  I'm going to try to find some time today to apply this patch locally and see if I can figure out what issues you're running into.

> Source/WebCore/Modules/intents/Intent.idl:35
> +        readonly attribute [InitializedByConstructor] DOMString action;
> +        readonly attribute [InitializedByConstructor] DOMString type;
> +        readonly attribute [InitializedByConstructor] SerializedScriptValue data;

I don't think you need the InitializedByConstructor attribute.  That's for subclasses of Event.

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:94
> +v8::Handle<v8::Value> V8Intent::dataAccessorGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)

data isn't marked custom anymore.  I'm not sure how this compiles....

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:100
> +    SerializedScriptValue* serializedValue = impl->data();
> +    if (serializedValue)
> +        return serializedValue->deserialize();

So, the general problem is that this function will return a new object ever time the getter is called.  Presumably that's not what you intend.

[Greg Billock]

Comment on attachment 121125 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121125&action=review

>> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:94
>> +v8::Handle<v8::Value> V8Intent::dataAccessorGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
> 
> data isn't marked custom anymore.  I'm not sure how this compiles....

I commented it out of the build. Basically this patch version is to get feedback on the direction to go with the codegen. I plan on deleting this entire file from the patch once we get settled on what to do there.

[Adam Barth]

Ah, I understand now.

[Adam Barth]

I'm having trouble finding the specification for WebIntents.  I found http://webintents.org/ but the "Specification" link just goes back to the home page, but that's clearly not the spec because it doesn't even have the WebIDL for Intent.

[Greg Billock]

http://www.chromium.org/developers/design-documents/webintentsapi (It's in the ChangeLog on this patch.)

But the official W3C draft version is in process as we speak. There should be a URL for that soon.

[Adam Barth]

Thanks.

[Adam Barth]

Created attachment 121158 [details]
Work in progress

[Adam Barth]

Ok.  I understand the problem you're running into now.  The next question I have is what behavior you're trying to create.

The spec is pretty vague, but it says that data is a structured clone "that constitutes the payload to send to the service."

I'm assuming what you want is that we create a structured clone of the parameter to the constructor.  In the IDL in the spec, |data| is not marked readonly.  What is the expected behavior when you assign to |data|?  Should |data| then become a structured clone of the value you assigned?

In the short term, I'm going to recommend we keep this value as readonly and populate the JS property in the constructor.  I'll try to create a proof-of-concept patch.

[Adam Barth]

So, the proximate cause of the problem is that V8Intent::constructorCallback doesn't call SerializedScriptValue::deserializeAndSetProperty like V8Intent::wrapSlow does.

[Adam Barth]

Created attachment 121233 [details]
Seems to work

[Adam Barth]

Greg, I spent some time looking at this issue, and the attached patch seems to work.  If this approach works for you, we should probably split off the CodeGenerator changes into a separate patch and then landing the Intents patch.

[Greg Billock]

(In reply to comment #66)
> Ok.  I understand the problem you're running into now.  The next question I have is what behavior you're trying to create.
> 
> The spec is pretty vague, but it says that data is a structured clone "that constitutes the payload to send to the service."
> 
> I'm assuming what you want is that we create a structured clone of the parameter to the constructor.  In the IDL in the spec, |data| is not marked readonly.  What is the expected behavior when you assign to |data|?  Should |data| then become a structured clone of the value you assigned?

There's a W3C version now at http://dvcs.w3.org/hg/web-intents/raw-file/tip/spec/Overview.html

It fixes this error of |data| not being marked readonly. Yes, the goal is that when the constructor is called, the |data| member of the Intent object is a snapshot of the value in the third constructor argument, provided it passes the structured clone algorithm, and an error is thrown if it cannot pass the structured clone algorithm. I think I've expressed this in the webkit IDL, but the exception isn't in the IDL in the spec yet.

 
> In the short term, I'm going to recommend we keep this value as readonly and populate the JS property in the constructor.  I'll try to create a proof-of-concept patch.

Thanks! I agree that splitting that off first is a good plan.

[Greg Billock]

(In reply to comment #69)
> Greg, I spent some time looking at this issue, and the attached patch seems to work.  If this approach works for you, we should probably split off the CodeGenerator changes into a separate patch and then landing the Intents patch.

I think I understand this: The way I'm reading it, the codegen will still not generate the getter, but that's OK, because the DOM wrapper object now has access to a copy of the data. Is that right?

Does that mean there's two copies of the data, one in the Intent object and another in the DOM wrapper?

Also, just to make sure I get it, will that allow the user to set values into an Intent object data field after initialization? Or will that throw an error given the modifiers in line 222 in the codegen (marking the object v8::ReadOnly).

I believe my test should catch any issues like this, so if it passes, I'm happy, modulo worries about potentially duplicating a large payload. :-) Just technically, though, is it better to make a copy of the data initially so that it is accessible to JS, rather than making a copy on demand (which the accessor would end up doing, I think, right). That may depend on usage. I would expect client code would very rarely access this field, since they are in control of setting it and presumably have better access to the data in its original form while they're preparing the Intent object.

[Adam Barth]

> I think I understand this: The way I'm reading it, the codegen will still not generate the getter, but that's OK, because the DOM wrapper object now has access to a copy of the data. Is that right?

Right, the object gets snapshotted and stored in the wrapper object as a JavaScript object.

> Does that mean there's two copies of the data, one in the Intent object and another in the DOM wrapper?

Yes.  The one attached to the wrapper can be mutated by the page, but the one on the Intent object is frozen in time.

> Also, just to make sure I get it, will that allow the user to set values into an Intent object data field after initialization?

Correct.  It's just like an expando DOM property at that point.

> Or will that throw an error given the modifiers in line 222 in the codegen (marking the object v8::ReadOnly).

That only affects the |data| slot.  So, you can't wholesale assign to the |data| member of the object, but you can mutate the object stored as the |data| member.

> I believe my test should catch any issues like this, so if it passes, I'm happy, modulo worries about potentially duplicating a large payload. :-)

Yep.  Your test passes (after I modified it to know that a zero argument constructor throws and exception).

> Just technically, though, is it better to make a copy of the data initially so that it is accessible to JS, rather than making a copy on demand (which the accessor would end up doing, I think, right).

This seems to be the pattern we're using for SerializedScriptValues elsewhere.  This case wasn't handled because you had a constructible object.

> That may depend on usage. I would expect client code would very rarely access this field, since they are in control of setting it and presumably have better access to the data in its original form while they're preparing the Intent object.

Ok.  The way I would approach this problem is to put a FIXME into the code and worry about this optimization later.  My sense is that will let us move forward and get the rest of this features reviewed and landed.

[Greg Billock]

(In reply to comment #72)
> > I think I understand this: The way I'm reading it, the codegen will still not generate the getter, but that's OK, because the DOM wrapper object now has access to a copy of the data. Is that right?
> 
> Right, the object gets snapshotted and stored in the wrapper object as a JavaScript object.
> 
> > Does that mean there's two copies of the data, one in the Intent object and another in the DOM wrapper?
> 
> Yes.  The one attached to the wrapper can be mutated by the page, but the one on the Intent object is frozen in time.
> 
> > Also, just to make sure I get it, will that allow the user to set values into an Intent object data field after initialization?
> 
> Correct.  It's just like an expando DOM property at that point.
> 
> > Or will that throw an error given the modifiers in line 222 in the codegen (marking the object v8::ReadOnly).
> 
> That only affects the |data| slot.  So, you can't wholesale assign to the |data| member of the object, but you can mutate the object stored as the |data| member.
> 
> > I believe my test should catch any issues like this, so if it passes, I'm happy, modulo worries about potentially duplicating a large payload. :-)
> 
> Yep.  Your test passes (after I modified it to know that a zero argument constructor throws and exception).
> 
> > Just technically, though, is it better to make a copy of the data initially so that it is accessible to JS, rather than making a copy on demand (which the accessor would end up doing, I think, right).
> 
> This seems to be the pattern we're using for SerializedScriptValues elsewhere.  This case wasn't handled because you had a constructible object.
> 
> > That may depend on usage. I would expect client code would very rarely access this field, since they are in control of setting it and presumably have better access to the data in its original form while they're preparing the Intent object.
> 
> Ok.  The way I would approach this problem is to put a FIXME into the code and worry about this optimization later.  My sense is that will let us move forward and get the rest of this features reviewed and landed.

Sounds good. Will you split off this codegen change for review? I'm happy to do it, but it's definitely your work; I understand what it's doing now, but could not have produced it. :-)

[Adam Barth]

> Sounds good. Will you split off this codegen change for review? I'm happy to do it, but it's definitely your work; I understand what it's doing now, but could not have produced it. :-)

Sure.  I'd be happy to.

[Greg Billock]

Created attachment 121343 [details]
Patch

[David Levin]

Comment on attachment 121343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121343&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1874
> +#    return if ($attribute->signature->type eq "SerializedScriptValue");

I don't think you meant to include this in your change.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2241
> +            # next;

Ditto.

[Adam Barth]

Comment on attachment 121343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121343&action=review

> Source/WebCore/Modules/intents/Intent.idl:29
> +      ConstructorParameters=3,

This isn't needed.

> Source/WebCore/Modules/intents/Intent.idl:35
> +        readonly attribute [InitializedByConstructor] DOMString action;
> +        readonly attribute [InitializedByConstructor] DOMString type;
> +        readonly attribute [InitializedByConstructor] SerializedScriptValue data;

[InitializedByConstructor] isn't needed.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1874
> -    return if ($attribute->signature->type eq "SerializedScriptValue");
> +#    return if ($attribute->signature->type eq "SerializedScriptValue");

This isn't needed.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2241
> -            next;
> +            # next;

Nor is this.

> Source/WebCore/page/DOMWindow.idl:612
> +        attribute [Conditional=WEB_INTENTS] IntentConstructor Intent; // Usable with the new operator

We should actually use the new supplemental machinery here (see DOMWindowWebAudio.idl), but lets do that in a follow-up patch.

[Greg Billock]

Comment on attachment 121343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121343&action=review

>> Source/WebCore/Modules/intents/Intent.idl:29
>> +      ConstructorParameters=3,
> 
> This isn't needed.

Done

>> Source/WebCore/Modules/intents/Intent.idl:35
>> +        readonly attribute [InitializedByConstructor] SerializedScriptValue data;
> 
> [InitializedByConstructor] isn't needed.

Done

>>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1874
>>> +#    return if ($attribute->signature->type eq "SerializedScriptValue");
>> 
>> I don't think you meant to include this in your change.
> 
> This isn't needed.

Oops! Killing this file from patch...

>> Source/WebCore/page/DOMWindow.idl:612
>> +        attribute [Conditional=WEB_INTENTS] IntentConstructor Intent; // Usable with the new operator
> 
> We should actually use the new supplemental machinery here (see DOMWindowWebAudio.idl), but lets do that in a follow-up patch.

That looks cool! Once this is in I'll change to that.

[Greg Billock]

Created attachment 121357 [details]
Patch

[Greg Billock]

Near-final form (doesn't build/test without 75641, so once that's in I need to double check).

[Adam Barth]

Comment on attachment 121357 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121357&action=review

> Source/WebCore/ChangeLog:5
> +        WebCore implementation of the Intent object
> +        See http://dvcs.w3.org/hg/web-intents/raw-file/tip/spec/Overview.html
> +        for draft spec.

Ideally, we'd include some more information in the ChangeLog, but I think this is ok for this patch.

> Source/WebCore/Modules/intents/Intent.cpp:76
> +void Intent::setData(SerializedScriptValue* data)

I would have just inlined this into the constructor.

> Source/WebCore/Modules/intents/Intent.h:55
> +    int identifier() const;
> +    void setIdentifier(int);

What is an identifier?  This doesn't seem to be needed as part of this patch.  Can we add this later when needed?

[Adam Barth]

Created attachment 121400 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 121400 [details]
Patch for landing

Rejecting attachment 121400 [details] from commit-queue.

New failing tests:
webintents/web-intents-api.html
Full output: http://queues.webkit.org/results/11143271

[Adam Barth]

We either need to enable the feature for testing or mark the tests as expected to fail on Chromium.

[Greg Billock]

Comment on attachment 121357 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121357&action=review

>> Source/WebCore/Modules/intents/Intent.cpp:76
>> +void Intent::setData(SerializedScriptValue* data)
> 
> I would have just inlined this into the constructor.

Agreed.

>> Source/WebCore/Modules/intents/Intent.h:55
>> +    void setIdentifier(int);
> 
> What is an identifier?  This doesn't seem to be needed as part of this patch.  Can we add this later when needed?

Sounds fine.

[Greg Billock]

For the layout tests, is this webkit/tools/layout_tests/test_expectations.txt that needs changing? Or will my '.../Skipped' changes suffice for chromium to make sure these tests don't run? (The trybots won't have the required build flag, so they should just skip these tests for now, I think; once we turn on the feature, we'll need to enable it for testing despite being skipped by default for now in webkit, right?)

I'm find with the Patch for landing you uploaded, or I can submit another that has setData merged into the constructor.

[Adam Barth]

LayoutTests/platform/chromium/test_expectations.txt is the file that needs to be updated.

[Greg Billock]

Created attachment 121451 [details]
Patch

[Greg Billock]

Created attachment 121453 [details]
Patch

[Greg Billock]

(In reply to comment #87)
> LayoutTests/platform/chromium/test_expectations.txt is the file that needs to be updated.

Thanks. Added. (Also updated the tests to track switch to codegen, as well as duplicate the changes to remove intent identifier and setData.)

[Adam Barth]

Comment on attachment 121453 [details]
Patch

Looks great.  Thanks for iterating on the patch.

[Greg Billock]

(In reply to comment #91)
> (From update of attachment 121453 [details])
> Looks great.  Thanks for iterating on the patch.

Definitely. Thanks for all your help and getting the codegen set up for SerializedScriptValue! I'll start getting follow-on CLs ready.

[Adam Barth]

Comment on attachment 121453 [details]
Patch

Clearing flags on attachment: 121453

Committed r104358: <http://trac.webkit.org/changeset/104358>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.