Bug 47078

Created attachment 69625 [details] Patch

Please cc darin on reviews that touch the WebKit layer. WIll review later.

Comment on attachment 69625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69625&action=review > WebKit/chromium/public/WebDeviceMotionData.h:42 > + WEBKIT_API static WebDeviceMotionData* create(bool canProvideXAcceleration, double xAcceleration, why heap allocated? normally, when we use WebPrivatePtr to implement a type, it means that we provide a constructor, destructor, initialize, reset, assign, etc. methods. please see WebNode for an example.

Comment on attachment 69625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69625&action=review looks pretty good minus comments > WebKit/chromium/public/WebDeviceMotionClient.h:38 > + virtual void setController(WebDeviceMotionController*) = 0; We no longer make webkit api's pure virtual. Use WEBKIT_ASSERT_NOT_IMPLEMENTED and return 0 when necessary. >> WebKit/chromium/public/WebDeviceMotionData.h:42 >> + WEBKIT_API static WebDeviceMotionData* create(bool canProvideXAcceleration, double xAcceleration, > > why heap allocated? > > normally, when we use WebPrivatePtr to implement a type, it means that we > provide a constructor, destructor, initialize, reset, assign, etc. methods. > please see WebNode for an example. Note that we use heap allocation only when we have virtual methods and code living in Chromium. > WebKit/chromium/src/DeviceMotionClientProxy.cpp:72 > + newline seems excessive...could maybe even combine the two if statements with an || > WebKit/chromium/src/DeviceMotionClientProxy.h:43 > + ~DeviceMotionClientProxy(); Virtual is probably good for documentation > WebKit/chromium/src/WebDeviceMotionController.cpp:43 > + PassRefPtr<WebCore::DeviceMotionData> motion(*deviceMotionData); RefPtr

Thank you both for the review. (In reply to comment #3) > (From update of attachment 69625 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69625&action=review > > > WebKit/chromium/public/WebDeviceMotionData.h:42 > > + WEBKIT_API static WebDeviceMotionData* create(bool canProvideXAcceleration, double xAcceleration, > > why heap allocated? Fixed that now. > normally, when we use WebPrivatePtr to implement a type, it means that we > provide a constructor, destructor, initialize, reset, assign, etc. methods. > please see WebNode for an example. Done. (Skipping assign because I don't think it's needed.) (In reply to comment #4) > (From update of attachment 69625 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69625&action=review > > looks pretty good minus comments > > > WebKit/chromium/public/WebDeviceMotionClient.h:38 > > + virtual void setController(WebDeviceMotionController*) = 0; > > We no longer make webkit api's pure virtual. Use WEBKIT_ASSERT_NOT_IMPLEMENTED and return 0 when necessary. Done using WEBKIT_ASSERT_NOT_REACHED (couldn't find WEBKIT_ASSERT_NOT_IMPLEMENTED.) > > >> WebKit/chromium/public/WebDeviceMotionData.h:42 > >> + WEBKIT_API static WebDeviceMotionData* create(bool canProvideXAcceleration, double xAcceleration, > > > > why heap allocated? > > > > normally, when we use WebPrivatePtr to implement a type, it means that we > > provide a constructor, destructor, initialize, reset, assign, etc. methods. > > please see WebNode for an example. > > Note that we use heap allocation only when we have virtual methods and code living in Chromium. > > > WebKit/chromium/src/DeviceMotionClientProxy.cpp:72 > > + > > newline seems excessive...could maybe even combine the two if statements with an || Done. Keeping them on separate lines as they kind of test for different things, and I hope to remove the former statement eventually. > > > WebKit/chromium/src/DeviceMotionClientProxy.h:43 > > + ~DeviceMotionClientProxy(); > > Virtual is probably good for documentation Done. > > > WebKit/chromium/src/WebDeviceMotionController.cpp:43 > > + PassRefPtr<WebCore::DeviceMotionData> motion(*deviceMotionData); > > RefPtr Done. It got slightly involved though :S (compiler doesn't see that it can use WebDeviceMotionData's conversion operator to PassRefPtr and use that to construct a RefPtr)

Created attachment 69795 [details] Patch

Comment on attachment 69795 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69795&action=review LGTM, but it'd be nice to have Darin sign off on it. > WebKit/chromium/public/WebDeviceMotionClient.h:43 > + virtual WebDeviceMotionData* currentDeviceMotion() const { WEBKIT_ASSERT_NOT_REACHED(); return 0; } anything over one statement needs to be on multiple lines. Unfortunately. > WebKit/chromium/public/WebDeviceMotionController.h:39 > + WebDeviceMotionController(WebCore::DeviceMotionController*); Needs WEBKIT_API > WebKit/chromium/src/WebDeviceMotionController.cpp:43 > + RefPtr<WebCore::DeviceMotionData> motion(static_cast<PassRefPtr<WebCore::DeviceMotionData> >(*deviceMotionData)); Ugh...ok, maybe what was there was better.

(In reply to comment #7) > (From update of attachment 69795 [details]) > > WebKit/chromium/public/WebDeviceMotionController.h:39 > > + WebDeviceMotionController(WebCore::DeviceMotionController*); > > Needs WEBKIT_API But it will never be called from outside WebKit (and can never be, because outside WebKit, the caller would not be able to pass in a WebCore::DeviceMotionController*).

(In reply to comment #7) > (From update of attachment 69795 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69795&action=review > > LGTM, but it'd be nice to have Darin sign off on it. > > > WebKit/chromium/public/WebDeviceMotionClient.h:43 > > + virtual WebDeviceMotionData* currentDeviceMotion() const { WEBKIT_ASSERT_NOT_REACHED(); return 0; } > > anything over one statement needs to be on multiple lines. Unfortunately. Done. > > WebKit/chromium/public/WebDeviceMotionController.h:39 > > + WebDeviceMotionController(WebCore::DeviceMotionController*); > > Needs WEBKIT_API As noted above, the intention is not to make this part of the API, as it is only called inside the WebKit layer. Is it ok to just ommit the WEBKIT_API like I did, or should I use #if WEBKIT_IMPLEMENTATION to hide it? (And then probably declare a dummy private default constructor to make ut unconstructable.) > > WebKit/chromium/src/WebDeviceMotionController.cpp:43 > > + RefPtr<WebCore::DeviceMotionData> motion(static_cast<PassRefPtr<WebCore::DeviceMotionData> >(*deviceMotionData)); > > Ugh...ok, maybe what was there was better. Putting it back the way it was. Darin: does this look ok to you?

Created attachment 69908 [details] Patch

Comment on attachment 69908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69908&action=review > WebKit/chromium/public/WebDeviceMotionClient.h:40 > + virtual void setController(WebDeviceMotionController*) { WEBKIT_ASSERT_NOT_REACHED(); } why is setController an explicit method here? couldn't you also pass the WebDeviceMotionController to the method on WebViewClient used to fetch the WebDeviceMotionClient? when does the WebDeviceMotionController pointer get freed? > WebKit/chromium/public/WebDeviceMotionClient.h:43 > + virtual WebDeviceMotionData* currentDeviceMotion() const nit: currentDeviceMotion -> currentDeviceMotionData why is currentDeviceMotion returned by pointer and not by value? the whole point of using WebPrivatePtr<...wrapping a RefCounted type...> is to support passing the container type by value. > WebKit/chromium/public/WebDeviceMotionController.h:39 > + WebDeviceMotionController(WebCore::DeviceMotionController*); this constructor should be wrapped with WEBKIT_IMPLEMENTATION, and i presume that the default constructor should be declared but marked private. i see that WebDeviceOrientationController has the same problem. > WebKit/chromium/public/WebDeviceMotionController.h:47 > + WebCore::DeviceMotionController* m_controller; i don't understand what the memory management is here? this looks like a crash waiting to happen. > WebKit/chromium/public/WebDeviceMotionData.h:39 > +class WebDeviceMotionData { Looking at the way this class is used, I'm not sure why you didn't just do the same thing as you did for WebDeviceOrientation. It seems like it does not need to be a wrapper around the WebCore type. It can just be a simple class with member variables for the various fields. > WebKit/chromium/public/WebDeviceMotionData.h:74 > + WEBKIT_API void init(); what is the reason for making these private? why aren't you providing a copy constructor and assignment operator? those are typically provided since WebDeviceMotionData is really just like a RefPtr<DeviceMotionData>. it can be copied around, and all of the memory management happens implicitly.

Comment on attachment 69908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69908&action=review > WebKit/chromium/public/WebDeviceMotionData.h:42 > + WebDeviceMotionData(bool canProvideXAcceleration, double xAcceleration, also, functions with large numbers of parameters are really unfortunate. it is very easy to make a mistake when calling such a function. a better solution would probably look like this: class WebFoo { public: WebFoo() : m_hasBar(false), m_bar(0.0) { } void setBar(double bar) { m_hasBar = true; m_bar = bar; } bool hasBar() const { return m_hasBar; } double bar() const { return m_bar; } // ditto for other fields private: bool m_hasBar; double m_bar; }; at the place where you construct and initialize such a class, you would just have a bunch of setBar calls, one for each of the properties you want to set. on the chromium side, this structure would be trivial to write ParamTraits for in order to support IPC serialization. see chrome/common/webkit_param_traits.h. if you applied this approach to device orientation as well, then you could delete ViewMsg_DeviceOrientationUpdated_Params.

Thank you very much for the review. > > WebKit/chromium/public/WebDeviceMotionClient.h:40 > > + virtual void setController(WebDeviceMotionController*) { WEBKIT_ASSERT_NOT_REACHED(); } > > why is setController an explicit method here? couldn't you also pass the WebDeviceMotionController > to the method on WebViewClient used to fetch the WebDeviceMotionClient? Hmm, the DeviceMotionController that WebDeviceMotionController wraps is created and owned by the Page, which is created *after* the WebDeviceMotionClient, so I don't see how we could make that work. The setController method here mirrors the setController method on DeviceMotionClient. I suppose we could find another way to push motion data to the controller, but I think we would end up needing it anyway when adding a mock client similar to DeviceOrientationClientMock, which I plan to do. > when does the WebDeviceMotionController pointer get freed? The client whose setController method is called is responsible for freeing it. I suppose I could add a comment about that, or do you think there is a way to show it more clearly through code? > > WebKit/chromium/public/WebDeviceMotionClient.h:43 > > + virtual WebDeviceMotionData* currentDeviceMotion() const > > nit: currentDeviceMotion -> currentDeviceMotionData Hmm, the method is called currentDeviceMotion in DeviceMotionClient. I'm happy to rename them both, but in that case I'd like to do them at the same time, and in a separate patch. > why is currentDeviceMotion returned by pointer and not by value? the whole > point of using WebPrivatePtr<...wrapping a RefCounted type...> is to support > passing the container type by value. It's returned by pointer to match the DeviceMotionClient method, and so that the method can return NULL (meaning it has no current motion data). I think this is better than adding a null state to WebDeviceMotionData like we did with WebDeviceOrientation, but I don't have any strong feelings here. > > WebKit/chromium/public/WebDeviceMotionController.h:39 > > + WebDeviceMotionController(WebCore::DeviceMotionController*); > > this constructor should be wrapped with WEBKIT_IMPLEMENTATION, and i presume > that the default constructor should be declared but marked private. i see > that WebDeviceOrientationController has the same problem. Done. I will apply this and the other improvements from this discussion to the device orientation code when this patch passes review. > > WebKit/chromium/public/WebDeviceMotionController.h:47 > > + WebCore::DeviceMotionController* m_controller; > > i don't understand what the memory management is here? this looks like a crash > waiting to happen. The memory management here is the same as for DeviceMotionClient::setController. The setController method takes a weak pointer to the controller. Since the controller is a WebCore class we can't just pass it to the WebDeviceMotionClient::setController method, but put it in this thin wrapper, and let the WebDeviceMotionClient take ownership of the wrapper. Thinking more about this, we should probably forward the DeviceMotionClient::deviceMotionControllerDestroyed() method too, so we can signal when the pointer to the controller becomes invalid. Doing that. > > WebKit/chromium/public/WebDeviceMotionData.h:39 > > +class WebDeviceMotionData { > > Looking at the way this class is used, I'm not sure why you didn't just > do the same thing as you did for WebDeviceOrientation. It seems like it > does not need to be a wrapper around the WebCore type. It can just be > a simple class with member variables for the various fields. Doing this. You're right, it is probably easier. > > WebKit/chromium/public/WebDeviceMotionData.h:74 > > + WEBKIT_API void init(); > > what is the reason for making these private? This does not apply to the new WebDeviceMotionData. > why aren't you providing a copy constructor and assignment operator? those > are typically provided since WebDeviceMotionData is really just like a > RefPtr<DeviceMotionData>. it can be copied around, and all of the memory > management happens implicitly. This does not exist in the new WebDeviceMotionData. (In reply to comment #12) > > WebKit/chromium/public/WebDeviceMotionData.h:42 > > + WebDeviceMotionData(bool canProvideXAcceleration, double xAcceleration, > > also, functions with large numbers of parameters are really unfortunate. it is > very easy to make a mistake when calling such a function. > > a better solution would probably look like this: > > class WebFoo { > public: > WebFoo() : m_hasBar(false), m_bar(0.0) { > } > > void setBar(double bar) { > m_hasBar = true; > m_bar = bar; > } > bool hasBar() const { return m_hasBar; } > double bar() const { return m_bar; } > > // ditto for other fields > > private: > bool m_hasBar; > double m_bar; > }; > > at the place where you construct and initialize such a class, you would just > have a bunch of setBar calls, one for each of the properties you want to set. Doing this. Will update the device orientation code to do the same after this patch. > on the chromium side, this structure would be trivial to write ParamTraits for > in order to support IPC serialization. see chrome/common/webkit_param_traits.h. Will do. > if you applied this approach to device orientation as well, then you could > delete ViewMsg_DeviceOrientationUpdated_Params. Looking forward to it :)

Created attachment 70089 [details] Patch

Darin, are you happy with this?

I'm now blocked on this. Any input would be most welcome.

Comment on attachment 70089 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70089&action=review these are my only concerns. Are you ok Darin? > WebKit/chromium/src/WebDeviceMotionController.cpp:41 > +void WebDeviceMotionController::didChangeDeviceMotion(const WebDeviceMotionData* deviceMotionData) In cases like this, we should use & rather than *. > WebKit/chromium/src/WebDeviceMotionController.cpp:43 > + PassRefPtr<WebCore::DeviceMotionData> motion(*deviceMotionData); I'm still surprised this can't be done any more cleanly...but I know sometimes this stuff can be a mess.

Is this still pending? Does this patch still apply? It's been sitting here a long time.

Comment on attachment 70089 [details] Patch This is not currently pending (and the patch most likely doesn't apply anymore). I expect to get back to this eventually, but taking it out of the review queue for now.

For anyone interested, this work has moved to Bug 89197.