Use UNUSED_PARAM instead of the void casting.
Created attachment 250780 [details] Patch
Created attachment 250781 [details] Patch
Comment on attachment 250781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250781&action=review > Source/WebKit2/WebProcess/Notifications/NotificationPermissionRequestManager.cpp:65 > + UNUSED_PARAM(page); Using UNUSED_PARAM(page) unconditionally is incorrect, it should be in the #else case of ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
Created attachment 250791 [details] Patch
Thank you for pointing that out. I updated the patch. Please take a look. Thanks!
Comment on attachment 250791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250791&action=review > Source/WebKit2/WebProcess/Notifications/NotificationPermissionRequestManager.cpp:65 > +#if !ENABLE(NOTIFICATIONS) && !ENABLE(LEGACY_NOTIFICATIONS) I don't like to use unnecessary macro. Probably it looks Ossy wants to use a thing similar to below style. NotificationPermissionRequestManager::NotificationPermissionRequestManager(WebPage* page) { #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS) m_page = page; #else UNUSED_PARAM(page); #endif }
Comment on attachment 250791 [details] Patch Ouch, it's so ugly. :-/ Unfortunately we can't put UNUSED_PARAM to the same place as init list. I think this one would be the least ugly solution NotificationPermissionRequestManager::NotificationPermissionRequestManager(WebPage* page) { #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS) m_page = page #else UNUSED_PARAM(page); #endif }
(In reply to comment #6) > Comment on attachment 250791 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=250791&action=review > > > Source/WebKit2/WebProcess/Notifications/NotificationPermissionRequestManager.cpp:65 > > +#if !ENABLE(NOTIFICATIONS) && !ENABLE(LEGACY_NOTIFICATIONS) > > I don't like to use unnecessary macro. Probably it looks Ossy wants to use a > thing similar to below style. > > NotificationPermissionRequestManager:: > NotificationPermissionRequestManager(WebPage* page) > { > #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS) > m_page = page; > #else > UNUSED_PARAM(page); > #endif > } haha, you must be a thought-reader :)
(In reply to comment #8) > (In reply to comment #6) > > Comment on attachment 250791 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=250791&action=review > > > > > Source/WebKit2/WebProcess/Notifications/NotificationPermissionRequestManager.cpp:65 > > > +#if !ENABLE(NOTIFICATIONS) && !ENABLE(LEGACY_NOTIFICATIONS) > > > > I don't like to use unnecessary macro. Probably it looks Ossy wants to use a > > thing similar to below style. > > > > NotificationPermissionRequestManager:: > > NotificationPermissionRequestManager(WebPage* page) > > { > > #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS) > > m_page = page; > > #else > > UNUSED_PARAM(page); > > #endif > > } > > haha, you must be a thought-reader :) Because you and I have dealt with similar issues many times. :)
(In reply to comment #7) > Comment on attachment 250791 [details] > Patch > > Ouch, it's so ugly. :-/ Unfortunately we can't put UNUSED_PARAM to the same > place as init list. > > I think this one would be the least ugly solution > > NotificationPermissionRequestManager:: > NotificationPermissionRequestManager(WebPage* page) > { > #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS) > m_page = page > #else > UNUSED_PARAM(page); > #endif > } Agreed. But the member initialization is slightly different from the assignment. Any thoughts on this?
Created attachment 250795 [details] Patch
(In reply to comment #10) > (In reply to comment #7) > > Comment on attachment 250791 [details] > > Patch > > > > Ouch, it's so ugly. :-/ Unfortunately we can't put UNUSED_PARAM to the same > > place as init list. > > > > I think this one would be the least ugly solution > > > > NotificationPermissionRequestManager:: > > NotificationPermissionRequestManager(WebPage* page) > > { > > #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS) > > m_page = page > > #else > > UNUSED_PARAM(page); > > #endif > > } > > Agreed. But the member initialization is slightly different from the > assignment. > Any thoughts on this? I think biggest difference is generally that copy constructor of *WebPage* class is called or not. In this case, however, m_page is a pointer type of *WebPage*. Thus it seems to me that there is no big difference between both.
Addressed your comments.(In reply to comment #12) > (In reply to comment #10) > > (In reply to comment #7) > > > Comment on attachment 250791 [details] > > > Patch > > > > > > Ouch, it's so ugly. :-/ Unfortunately we can't put UNUSED_PARAM to the same > > > place as init list. > > > > > > I think this one would be the least ugly solution > > > > > > NotificationPermissionRequestManager:: > > > NotificationPermissionRequestManager(WebPage* page) > > > { > > > #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS) > > > m_page = page > > > #else > > > UNUSED_PARAM(page); > > > #endif > > > } > > > > Agreed. But the member initialization is slightly different from the > > assignment. > > Any thoughts on this? > > I think biggest difference is generally that copy constructor of *WebPage* > class is called or not. In this case, however, m_page is a pointer type of > *WebPage*. Thus it seems to me that there is no big difference between both. (In reply to comment #12) > (In reply to comment #10) > > (In reply to comment #7) > > > Comment on attachment 250791 [details] > > > Patch > > > > > > Ouch, it's so ugly. :-/ Unfortunately we can't put UNUSED_PARAM to the same > > > place as init list. > > > > > > I think this one would be the least ugly solution > > > > > > NotificationPermissionRequestManager:: > > > NotificationPermissionRequestManager(WebPage* page) > > > { > > > #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS) > > > m_page = page > > > #else > > > UNUSED_PARAM(page); > > > #endif > > > } > > > > Agreed. But the member initialization is slightly different from the > > assignment. > > Any thoughts on this? > > I think biggest difference is generally that copy constructor of *WebPage* > class is called or not. In this case, however, m_page is a pointer type of > *WebPage*. Thus it seems to me that there is no big difference between both. I addressed your comments. Thanks!
Comment on attachment 250795 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250795&action=review I’m OK with this as is, but it doesn’t really seem like much of an improvement. > Source/WebKit2/WebProcess/Notifications/NotificationPermissionRequestManager.cpp:67 > NotificationPermissionRequestManager::NotificationPermissionRequestManager(WebPage* page) > +{ > #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS) > - : m_page(page) > + m_page = page; > +#else > + UNUSED_PARAM(page); > #endif > -{ > - (void)page; > } I don’t really like the way this changes from initialization syntax to assignment syntax. I’d prefer to avoid both UNUSED_PARAM and (void) myself by just repeating the function twice: #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS) NotificationPermissionRequestManager::NotificationPermissionRequestManager(WebPage* page) : m_page(page) { } #else NotificationPermissionRequestManager::NotificationPermissionRequestManager(WebPage*) { } #endif One nice thing about that is that it clears the way for further refinement, for example, we could make the empty constructor inline by putting it in the header and marking it inline without making the non-empty constructor be inlined.
Created attachment 250982 [details] Patch
Comment on attachment 250982 [details] Patch Address Darin's comments
Comment on attachment 250982 [details] Patch Clearing flags on attachment: 250982 Committed r182936: <http://trac.webkit.org/changeset/182936>
All reviewed patches have been landed. Closing bug.