| Summary: | Use OptionSet for ActivityState::Flags | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||||||
| Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | benjamin, berto, bfulgham, cdumez, cgarcia, cmarcelo, commit-queue, dbates, dino, esprehn+autocc, ews-watchlist, graouts, gustavo, gyuyoung.kim, kondapallykalyan, mcatanzaro, simon.fraser, webkit-bug-importer, zalan | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Antti Koivisto
2018-08-14 07:56:22 PDT
Created attachment 347081 [details]
patch
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. > 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.
Created attachment 347153 [details]
patch
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 Created attachment 347157 [details]
patch
/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.
Created attachment 347204 [details]
patch
Comment on attachment 347204 [details] patch Clearing flags on attachment: 347204 Committed r234920: <https://trac.webkit.org/changeset/234920> All reviewed patches have been landed. Closing bug. |