Bug 180305 – Make some minor adjustments to TouchBarMenuData and TouchBarMenuItemData

• Component: WebKit Misc.
• Status: NEW

[Wenson Hsieh]

Followup to r225438.

[Wenson Hsieh]

Created attachment 328209 [details]
Patch

[Wenson Hsieh]

Created attachment 328216 [details]
Merge in dbates' suggestions as well

[Joseph Pecoraro]

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.

[Wenson Hsieh]

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!

[Wenson Hsieh]

Created attachment 328220 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 328220 [details]
Patch for landing

Clearing flags on attachment: 328220

Committed r225446: <https://trac.webkit.org/changeset/225446>

[Darin Adler]

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.

[Wenson Hsieh]

(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.

[Wenson Hsieh]

Created attachment 328309 [details]
Minor additional tweak

[WebKit Commit Bot]

Comment on attachment 328309 [details]
Minor additional tweak

Clearing flags on attachment: 328309

Committed r225505: <https://trac.webkit.org/changeset/225505>