[Webkit-unassigned] [Bug 188554] Use OptionSet for ActivityState::Flags

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=188554

Brent Fulgham <bfulgham at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #347081|review?                     |review+
              Flags|                            |

--- Comment #2 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 347081
  --> https://bugs.webkit.org/attachment.cgi?id=347081
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347081&action=review

Looks good! r=me

> Source/WebCore/page/ActivityState.cpp:54
> +    appendIf(ActivityState::IsCapturingMedia, "capturing media");

Nice cleanup!

> Source/WebCore/page/FocusController.cpp:356
> +    m_page.setActivityState(focused ? m_activityState | ActivityState::IsFocused : m_activityState - ActivityState::IsFocused);

It's too bad we still have to use all of this bitwise math for an OptionSet. It would be much clearer to say something like "m_activityState.remove(ActivityState::IsFocused)

> Source/WebCore/page/Page.cpp:1652
> +        state -= { ActivityState::IsVisible, ActivityState::IsVisibleOrOccluded };

Again, these bitwise operators are hard to reason about when reading the code. Seems like extending OptionSet with helpful methods would make these kinds of statements easier to understand.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180814/3a4b318c/attachment.html>