Bug 75756

Created attachment 121515 [details] Patch

Comment on attachment 121515 [details] Patch This patch seems to have a bunch of stuff from your earlier CL in it.

Some high-level comments: You should be using the [Supplemental] attribute to keep as much WebIntent-specific code in Modules/intents as possible. You shouldn't be modifying Navigator.idl or any of the other "core" IDL files in these patches. The [Supplemental] mechanism is somewhat new, but you can see how Gamepad uses it for inspiration.

This is the follow-up to 73051 containing mostly the callback, startActivity, and IntentsController. There is still some noise from 73051 that I will merge out as that lands. I've moved the IntentsController to be owned by the ScriptExecutionContext. After looking at the deserializer code, I think it is copying all the data out of the SerializedScriptValue input, so I've also changed the callback to just take a pointer, which required a change to the CodeGeneratorV8. Take a look and see if you think it looks like something we want (if so, it should probably get split off again to get the right tests along with it). I was looking at the Supplemental examples to try and put the navigator change in a supplemental IDL file. The examples show a supplemental constructor and supplemental fields, but not supplemental methods. Does it support that? If not, that's a candidate for another side patch.

Comment on attachment 121515 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121515&action=review > Source/WebCore/Modules/intents/IntentsController.cpp:57 > +int IntentsController::assignIntentId(PassRefPtr<IntentResultCallback> successCallback, PassRefPtr<IntentResultCallback> errorCallback) Why > Source/WebCore/page/Navigator.cpp:328 > +void Navigator::startActivity(ScriptExecutionContext* context, > + Intent* intent, PassRefPtr<IntentResultCallback> successCallback, PassRefPtr<IntentResultCallback> errorCallback, ExceptionCode& ec) This code should all be inside Module/intents. Also, I don't see any tests fo this API in this patch. > Source/WebCore/page/Navigator.cpp:348 > + int id = context->intentsController()->assignIntentId(successCallback, errorCallback); > + intent->setIdentifier(id); > + activeFrame->loader()->client()->dispatchIntent(*intent); I don't understand why we have this complicated ID mechanism. Can we call startActivity more than once per Intent? It looks like we can only have one ID per intent. Maybe we should store the callbacks on the intent object itself instead of using using a hashmap for indirection. Is there something the hashmap buys us?

> This is the follow-up to 73051 containing mostly the callback, startActivity, and IntentsController. There is still some noise from 73051 that I will merge out as that lands. Please post the patch without the code from the other patch. It's too hard to review with all this extra stuff in it. > I was looking at the Supplemental examples to try and put the navigator change in a supplemental IDL file. The examples show a supplemental constructor and supplemental fields, but not supplemental methods. Does it support that? If not, that's a candidate for another side patch. It does. Look at Gamepad.

Replies: [gamepad, supplemental IDL] Got it. I guess it has propagated to the web FEs, but I see how NavigatorGamepad.idl is doing this and will copy. [tests] There's a very simple existence and invocation test. I'm going to add more. [crap from earlier change] Yes. I was hoping to get exactly these kinds of high-level comments earlier. :-) Please don't review in depth until I get them stripped out, though. (Should be soon.) [ID mechanism in IntentsController] I don't think startActivity should be called more than once per intent (although there is nothing to forbid it at present). But it could be called more than once per client document. That is, there can be situations (at least right now) when there are multiple startActivity async calls in flight at once. I'm not totally convinced that's how it should remain, and if it doesn't, then all we need is just a spot to write down the callbacks and the controller is very simple (and could even be collapsed with another object). At the present time, the spec doesn't disallow that kind of use, and we want to do more experimentation with it like this before being persuaded it ought to be forbidden. (There are use cases in discussion that want to use web intents as a control invocation mechanism on discovered objects, i.e. home stereo equipment. It isn't clear yet how this will shake out, but being able to experiment with the code allowing that pattern will be useful.)

I'd recommend creating an IntentActivity object to hold the Intent and the callbacks. We'd the pass ownership of that object to the Client and when the Client wants to call one of the callbacks, it hands the object back to us. That avoids the need for a long-lived controller object and the need for this ID scheme.

The IntentActivity object should probably be an ActiveDOMObject so it can get notifications about the ScriptExecutionContext stopping and getting destroyed. That will prevent problems where the callbacks are called after the ScriptExecutionContext is inactive.

Maybe ActivityRequest ? I get the sense that this object isn't quite the activity itself (i.e., that we're starting), but a request to start an activity. You probably have a better sense for what the names ought to be.

I landed the first patch for you in case that makes it easier to prepare this patch without the "crap".

(In reply to comment #8) > I'd recommend creating an IntentActivity object to hold the Intent and the callbacks. We'd the pass ownership of that object to the Client and when the Client wants to call one of the callbacks, it hands the object back to us. That avoids the need for a long-lived controller object and the need for this ID scheme. It pushes the up the requirement for this kind of memory, yes. My motivation for putting it at this level was to make it less work for client code to implement intent handling, following the examples of other features like geolocation. Do you think the state here is minimal enough that it's not a big burden? (Clients will have to track these IDS, so they may as well track the IntentActivity/IntentRequest object.) The biggest such burden, I think, is making sure the proper notification gets to the right callback and no other. Using IntentActivity/IntentRequest would help with that, but will it require a more complex interaction to make sure the right ScriptExecutionContext is still active? Or will we end up needing a layer of notifications so that the IntentActivity will know that it went away? My thinking so far is that the dispatch mechanism for Web Intents will require ID tracking at some point anyway, so it ought to be baked in early to minimize error-proneness.

Created attachment 121680 [details] Patch

The request objects should just be subclasses of ActiveDOMObject, so they'll get all the right notifications.

Comment on attachment 121680 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121680&action=review This generally looks fine, but you haven't addressed my comments from before. > Source/WebCore/page/Navigator.cpp:310 > +#if ENABLE(WEB_INTENTS) > +void Navigator::startActivity(ScriptExecutionContext* context, > + Intent* intent, PassRefPtr<IntentResultCallback> successCallback, PassRefPtr<IntentResultCallback> errorCallback, ExceptionCode& ec) This should be done with [Supplemental] so we don't need to dump more unrelated code into Navigator.cpp. > Source/WebCore/page/Navigator.cpp:312 > + Frame* activeFrame = static_cast<Document*>(context)->frame(); We should probably use the frame from Navigator rather than from the currently executing script. That's the normal thing when you call a method of an object. > Source/WebCore/page/Navigator.cpp:326 > + if (!activeFrame || !intent) { > + ec = INVALID_STATE_ERR; > + return; > + } > + > + if (intent->action().isEmpty() || intent->type().isEmpty()) { > + ec = VALIDATION_ERR; > + return; > + } > + > + if (!ScriptController::processingUserGesture()) { > + ec = INVALID_ACCESS_ERR; > + return; > + } Is the order of these error checks spelled out in the spec? Because you're throwing different exceptions, the order is detectable. > Source/WebCore/page/Navigator.idl:66 > + [CallWith=ScriptExecutionContext] void startActivity(in Intent intent, We don't need CallWith=ScriptExecutionContext. Navigator is already associated with a frame.

(In reply to comment #14) > The request objects should just be subclasses of ActiveDOMObject, so they'll get all the right notifications. That's cool! I hadn't seen those. I'll just build the controller in at the client layer and assign IDs there. The Supplemental stuff also came out nice. I'll upload a new patch when I convert to these ActiveDOMObject things.

Created attachment 121729 [details] Patch

Comment on attachment 121729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121729&action=review This is starting to look good. There's just a couple memory management issues to polish up. > Source/WebCore/ChangeLog:8 > + Implement navigator.startActivity; add IntentsController for managing web intents callbacks. > + https://bugs.webkit.org/show_bug.cgi?id=75756 > + > + Reviewed by NOBODY (OOPS!). > + > + Test: webintents/web-intents-api.html It would be nice to have more information in the ChangeLog. Folks like to read these to understand what's happening in the project. > Source/WebCore/Modules/intents/IntentRequest.cpp:63 > + ContextDestructionObserver::contextDestroyed(); > + m_successCallback.clear(); > + m_errorCallback.clear(); Rather than overriding these, you should just check whether scriptExecutionContext() is null when the callbacks are called. > Source/WebCore/Modules/intents/IntentRequest.cpp:85 > + unsetPendingActivity(this); Depending on the ownership model for IntentRequest, you might want to store a RefPtr to |this| on the stack so that we know that the last reference to this object doesn't get destroyed by the JavaScript that runs during handleEvent. > Source/WebCore/Modules/intents/IntentRequest.h:53 > + virtual void contextDestroyed(); Please add the OVERRIDE keyword. > Source/WebCore/Modules/intents/IntentRequest.h:55 > + // TODO(gbillock@google.com): support suspend/resume I'd just remove this comment. (If we were to keep it, we'd need it to be "FIXME:" rather than "TODO(gbillock@google.com)" ) > Source/WebCore/Modules/intents/IntentRequest.h:60 > + Intent* m_intent; Doesn't this need to be a RefPtr? What's keeping the Intent alive? > Source/WebCore/Modules/intents/IntentResultCallback.h:33 > +#include <wtf/RefCounted.h> > +#include <wtf/RefPtr.h> These should be inside the #if > Source/WebCore/Modules/intents/NavigatorIntents.h:44 > + static void startActivity(Navigator*, > + Intent*, PassRefPtr<IntentResultCallback> successCallback, PassRefPtr<IntentResultCallback> errorCallback, ExceptionCode&); One line pls.

Comment on attachment 121729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121729&action=review >> Source/WebCore/ChangeLog:8 >> + Test: webintents/web-intents-api.html > > It would be nice to have more information in the ChangeLog. Folks like to read these to understand what's happening in the project. Added more description about what's going on in the change. >> Source/WebCore/Modules/intents/IntentRequest.cpp:63 >> + m_errorCallback.clear(); > > Rather than overriding these, you should just check whether scriptExecutionContext() is null when the callbacks are called. Since only one method is allowed to be called, I was using the callback cleared state to do this so I can release references as early as possible. Since the callbacks are optional and can be null, this ended up all working out nicely in the postResult/Failure code, so I piggybacked on that here. Do I need to check scriptExecutionContext() though? That is, is the threading nature here such that I need to lock around getting contextDestroyed() and postResult/Failure? >> Source/WebCore/Modules/intents/IntentRequest.cpp:85 >> + unsetPendingActivity(this); > > Depending on the ownership model for IntentRequest, you might want to store a RefPtr to |this| on the stack so that we know that the last reference to this object doesn't get destroyed by the JavaScript that runs during handleEvent. The model I have in mind is to pass ownership up to client code (see the FrameLoaderClient change below). Then the client code can throw away the object if it gets information that it isn't needed, which will then remove held refs. Should I hold a RefPtr to Intent as well, based on that premise? Or is it OK to let that be a weak pointer? >> Source/WebCore/Modules/intents/IntentRequest.h:53 >> + virtual void contextDestroyed(); > > Please add the OVERRIDE keyword. Done >> Source/WebCore/Modules/intents/IntentRequest.h:55 >> + // TODO(gbillock@google.com): support suspend/resume > > I'd just remove this comment. (If we were to keep it, we'd need it to be "FIXME:" rather than "TODO(gbillock@google.com)" ) Done. >> Source/WebCore/Modules/intents/IntentRequest.h:60 >> + Intent* m_intent; > > Doesn't this need to be a RefPtr? What's keeping the Intent alive? Yeah. This means the Intent is only alive during original dispatch (the call stack within the original startActivity call), which might violate the principle of least surprise... The code currently doesn't use it outside that scope, and it might be a heavy object, but still, Keeping a RefPtr so we can purposefully kill it if wanted sounds better. >> Source/WebCore/Modules/intents/IntentResultCallback.h:33 >> +#include <wtf/RefPtr.h> > > These should be inside the #if Done. >> Source/WebCore/Modules/intents/NavigatorIntents.h:44 >> + Intent*, PassRefPtr<IntentResultCallback> successCallback, PassRefPtr<IntentResultCallback> errorCallback, ExceptionCode&); > > One line pls. Done

Created attachment 121757 [details] Patch

> That is, is the threading nature here such that I need to lock around getting contextDestroyed() and postResult/Failure? There is only one thread. No locking needed. > >> Source/WebCore/Modules/intents/IntentRequest.cpp:85 > >> + unsetPendingActivity(this); > > > > Depending on the ownership model for IntentRequest, you might want to store a RefPtr to |this| on the stack so that we know that the last reference to this object doesn't get destroyed by the JavaScript that runs during handleEvent. > > The model I have in mind is to pass ownership up to client code (see the FrameLoaderClient change below). Then the client code can throw away the object if it gets information that it isn't needed, which will then remove held refs. Should I hold a RefPtr to Intent as well, based on that premise? Or is it OK to let that be a weak pointer? There's no guarantee the client will keep this object alive after the JavaScript runs. For example, suppose the client drops its reference to this object when the Frame is destroyed. It's quite possible that the JavaScript we execute synchronously here will be able to destroy the Frame. > >> Source/WebCore/Modules/intents/IntentRequest.h:60 > >> + Intent* m_intent; > > > > Doesn't this need to be a RefPtr? What's keeping the Intent alive? > > Yeah. This means the Intent is only alive during original dispatch (the call stack within the original startActivity call), which might violate the principle of least surprise... The code currently doesn't use it outside that scope, and it might be a heavy object, but still, Keeping a RefPtr so we can purposefully kill it if wanted sounds better. If we want to drop the pointer to the Intent, we can zero out the reference. That's better than having a dangling pointer temping folks to write security bugs. :)

Comment on attachment 121757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121757&action=review Two minor issues below. > Source/WebCore/Modules/intents/IntentRequest.cpp:59 > +void IntentRequest::contextDestroyed() Sorry, I don't think that contextDestroyed is quite the right thing. I think you want to override stop(). contextDestroyed is when the context is finally deallocated (e.g., it's called in the destructor). This stuff is kind of obscure. I should write some documentation explaining the lifecycle of these objects. > Source/WebCore/Modules/intents/IntentRequest.cpp:70 > + Please add the following line here: RefPtr<Intent> protector(this); We don't control the lifetime of |this|, so we have to assume that handleEvent, which can do arbitrary things, can remove all existing references to us. That means we need to take a reference to ourselves because we assume that |this| is still alive after handleEvent returns. > Source/WebCore/Modules/intents/IntentRequest.cpp:82 > + ditto

Created attachment 121764 [details] Patch

Comment on attachment 121757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121757&action=review >> Source/WebCore/Modules/intents/IntentRequest.cpp:59 >> +void IntentRequest::contextDestroyed() > > Sorry, I don't think that contextDestroyed is quite the right thing. I think you want to override stop(). contextDestroyed is when the context is finally deallocated (e.g., it's called in the destructor). This stuff is kind of obscure. I should write some documentation explaining the lifecycle of these objects. Are you sure? The ActiveDomObject impl looks like contextDestroyed is what's hooked to the notification system. I'll just hook stop() up to the same code, though, right? >> Source/WebCore/Modules/intents/IntentRequest.cpp:70 >> + > > Please add the following line here: > > RefPtr<Intent> protector(this); > > We don't control the lifetime of |this|, so we have to assume that handleEvent, which can do arbitrary things, can remove all existing references to us. That means we need to take a reference to ourselves because we assume that |this| is still alive after handleEvent returns. Yeah, I was busy finding that idiom as you were typing this I guess. (Looked like some protect, some protector). >> Source/WebCore/Modules/intents/IntentRequest.cpp:82 >> + > > ditto Yep. Got it.

One more thing on this. It includes a change to the codegen for the SerializedScriptValue treatment in handleEvent. Shall I pull that out to a separate patch? It'll need some separate tests.

> One more thing on this. It includes a change to the codegen for the SerializedScriptValue treatment in handleEvent. Shall I pull that out to a separate patch? It'll need some separate tests. I think that's fine. It won't compile otherwise, so it's not in that much risk of regressing. :)

(In reply to comment #24) > (From update of attachment 121757 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121757&action=review > > >> Source/WebCore/Modules/intents/IntentRequest.cpp:59 > >> +void IntentRequest::contextDestroyed() > > > > Sorry, I don't think that contextDestroyed is quite the right thing. I think you want to override stop(). contextDestroyed is when the context is finally deallocated (e.g., it's called in the destructor). This stuff is kind of obscure. I should write some documentation explaining the lifecycle of these objects. > > Are you sure? The ActiveDomObject impl looks like contextDestroyed is what's hooked to the notification system. I'll just hook stop() up to the same code, though, right? Pretty sure: http://trac.webkit.org/browser/trunk/Source/WebCore/dom/ScriptExecutionContext.cpp#L243 We can look at the callers of that function to double check.

Created attachment 121770 [details] Patch

Comment on attachment 121770 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121770&action=review > Source/WebCore/Modules/intents/IntentRequest.cpp:68 > + contextDestroyed(); This seems like crossing the streams, no? I would just remove the contextDestroyed override and clear the callbacks here. stop() will always be called before contextDestroyed() so there isn't a risk of them living too long.

(In reply to comment #27) > (In reply to comment #24) > > (From update of attachment 121757 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=121757&action=review > > > > >> Source/WebCore/Modules/intents/IntentRequest.cpp:59 > > >> +void IntentRequest::contextDestroyed() > > > > > > Sorry, I don't think that contextDestroyed is quite the right thing. I think you want to override stop(). contextDestroyed is when the context is finally deallocated (e.g., it's called in the destructor). This stuff is kind of obscure. I should write some documentation explaining the lifecycle of these objects. > > > > Are you sure? The ActiveDomObject impl looks like contextDestroyed is what's hooked to the notification system. I'll just hook stop() up to the same code, though, right? > > Pretty sure: http://trac.webkit.org/browser/trunk/Source/WebCore/dom/ScriptExecutionContext.cpp#L243 > > We can look at the callers of that function to double check. That looks pretty compelling. On the other hand, there are classes that purely implement the ContextDestructionObserver directly. See Source/WebCore/html/DOMURL.h for instance. I've hooked them up to the same code currently, which is fine internally for this object. It's a bit slapdash, though; if one evolves to call the other, we're screwed. :-) I connected stop() as having to do with the suspend/resume thing (presumably as a way to kill any suspend()ed actions), and that the destruction notification is what we really want to respond to to abort callback delivery. I'm guessing from looking at callers that that's incorrect. What I don't know is whether those callers have complete coverage. Is there any way for ~ScriptExecutionContext() to occur without stopActiveDOMObjects()? If so, or if this may occur in the future, we ought to have both impls (although cross invoking them is untidy...). (Also, codegen tests and layout tests pass here.)

Created attachment 121773 [details] Patch

I think stop is always called at least once before the context is destroyed. The DOMURL case is different because it just wants to free resources, rather than execute script.

Comment on attachment 121773 [details] Patch Clearing flags on attachment: 121773 Committed r104531: <http://trac.webkit.org/changeset/104531>

All reviewed patches have been landed. Closing bug.