Followup to r225438.
Created attachment 328209 [details] Patch
Created attachment 328216 [details] Merge in dbates' suggestions as well
Comment on attachment 328216 [details] Merge in dbates' suggestions as well View in context: https://bugs.webkit.org/attachment.cgi?id=328216&action=review r=me > Source/WebKit/ChangeLog:37 > + (WebKit::operator==): Heh, prepareChangeLog does the best it can do here. You could move this up above your comment on line 34-35 though, it is the TouchBarMenuItemData's operator== > Source/WebKit/Shared/TouchBarMenuItemData.cpp:36 > +static ItemType getItemType(const String&) The parameter is unused right now, but we plan on using it? Otherwise we could just inline this and avoid the attributeWithotuSynchronization for the `typeAttr` below. > Source/WebKit/Shared/TouchBarMenuItemData.h:54 > bool validTouchBarDisplay { true }; You can put the boolean at the bottom of the list of members to potentially avoid unnecessary padding bytes in the struct.
Comment on attachment 328216 [details] Merge in dbates' suggestions as well View in context: https://bugs.webkit.org/attachment.cgi?id=328216&action=review Thanks for the review! >> Source/WebKit/ChangeLog:37 >> + (WebKit::operator==): > > Heh, prepareChangeLog does the best it can do here. You could move this up above your comment on line 34-35 though, it is the TouchBarMenuItemData's operator== Done! >> Source/WebKit/Shared/TouchBarMenuItemData.cpp:36 >> +static ItemType getItemType(const String&) > > The parameter is unused right now, but we plan on using it? Otherwise we could just inline this and avoid the attributeWithotuSynchronization for the `typeAttr` below. Yes, we do plan on using it when we introduce more item types (the intent is that we can grow this function as we add richer support). >> Source/WebKit/Shared/TouchBarMenuItemData.h:54 >> bool validTouchBarDisplay { true }; > > You can put the boolean at the bottom of the list of members to potentially avoid unnecessary padding bytes in the struct. Good catch!
Created attachment 328220 [details] Patch for landing
Comment on attachment 328220 [details] Patch for landing Clearing flags on attachment: 328220 Committed r225446: <https://trac.webkit.org/changeset/225446>
Comment on attachment 328216 [details] Merge in dbates' suggestions as well View in context: https://bugs.webkit.org/attachment.cgi?id=328216&action=review >>> Source/WebKit/Shared/TouchBarMenuItemData.cpp:36 >>> +static ItemType getItemType(const String&) >> >> The parameter is unused right now, but we plan on using it? Otherwise we could just inline this and avoid the attributeWithotuSynchronization for the `typeAttr` below. > > Yes, we do plan on using it when we introduce more item types (the intent is that we can grow this function as we add richer support). WebKit coding style says this function should not have the word "get" in its name.
(In reply to Darin Adler from comment #7) > Comment on attachment 328216 [details] > Merge in dbates' suggestions as well > > View in context: > https://bugs.webkit.org/attachment.cgi?id=328216&action=review > > >>> Source/WebKit/Shared/TouchBarMenuItemData.cpp:36 > >>> +static ItemType getItemType(const String&) > >> > >> The parameter is unused right now, but we plan on using it? Otherwise we could just inline this and avoid the attributeWithotuSynchronization for the `typeAttr` below. > > > > Yes, we do plan on using it when we introduce more item types (the intent is that we can grow this function as we add richer support). > > WebKit coding style says this function should not have the word "get" in its > name. Good point! Patch forthcoming.
Created attachment 328309 [details] Minor additional tweak
Comment on attachment 328309 [details] Minor additional tweak Clearing flags on attachment: 328309 Committed r225505: <https://trac.webkit.org/changeset/225505>