Summary: | Random Gamepad cleanup | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||
Component: | WebCore Misc. | Assignee: | Brady Eidson <beidson> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | sam | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 134076 | ||||||
Attachments: |
|
Description
Brady Eidson
2014-08-22 21:36:40 PDT
Created attachment 237025 [details]
Patch v1
Comment on attachment 237025 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=237025&action=review > Source/WebCore/Modules/gamepad/NavigatorGamepad.cpp:79 > + if (index >= m_gamepads.size() || !m_gamepads[index]) > + return adoptRef(*Gamepad::create(platformGamepad).leakRef()); This looks suspect. Why can't you just return what Gamepad::create() returns (you may have to chance Gamepad::create() to return a PassRef<> I guess)? > Source/WebCore/page/DOMWindow.h:451 > +#if ENABLE(GAMEPAD) > + WeakPtr<DOMWindow> createWeakPtr() { return m_weakPtrFactory.createWeakPtr(); } > +#endif While Gamepad may be the only one using this right now, I don't think it makes sense to put this under #if ENABLE(GAMEPAD). (In reply to comment #2) > (From update of attachment 237025 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=237025&action=review > > > Source/WebCore/Modules/gamepad/NavigatorGamepad.cpp:79 > > + if (index >= m_gamepads.size() || !m_gamepads[index]) > > + return adoptRef(*Gamepad::create(platformGamepad).leakRef()); > > This looks suspect. Why can't you just return what Gamepad::create() returns (you may have to chance Gamepad::create() to return a PassRef<> I guess)? Right, and since we have to maintain a sparse Vector of Gamepads and we can't have null Ref<>s in that Vector, I think it makes more sense to keep Gamepad::create() returning PassRefPtr<> and play this trick here. > > > Source/WebCore/page/DOMWindow.h:451 > > +#if ENABLE(GAMEPAD) > > + WeakPtr<DOMWindow> createWeakPtr() { return m_weakPtrFactory.createWeakPtr(); } > > +#endif > > While Gamepad may be the only one using this right now, I don't think it makes sense to put this under #if ENABLE(GAMEPAD). Sure thing. |