Bug 188554 – Use OptionSet for ActivityState::Flags

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Antti Koivisto]

Make it safer.

[Antti Koivisto]

Created attachment 347081 [details]
patch

[Brent Fulgham]

Comment on attachment 347081 [details]
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.

[Antti Koivisto]

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

Yeah, at least add/remove would be good for readability.

[Antti Koivisto]

Created attachment 347153 [details]
patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Antti Koivisto]

Created attachment 347157 [details]
patch

[Brent Fulgham]

/Volumes/Data/EWS/WebKit/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2316:59: error: no member named 'AllFlags' in 'WebCore::ActivityState'
    _page->activityStateDidChange(WebCore::ActivityState::AllFlags);
                                  ~~~~~~~~~~~~~~~~~~~~~~~~^
1 error generated.

[Antti Koivisto]

Created attachment 347204 [details]
patch

[WebKit Commit Bot]

Comment on attachment 347204 [details]
patch

Clearing flags on attachment: 347204

Committed r234920: <https://trac.webkit.org/changeset/234920>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43378163>