[Webkit-unassigned] [Bug 73051] [Web Intents] Flagged-off WebCore implementation of navigator.startActivity

Mon Nov 28 14:45:04 PST 2011

https://bugs.webkit.org/show_bug.cgi?id=73051

--- Comment #6 from Greg Billock <gbillock at google.com>  2011-11-28 14:45:04 PST ---
(From update of attachment 116428)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.