Bug 188554

Summary: Use OptionSet for ActivityState::Flags
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: Layout and RenderingAssignee: 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 Flags
patch
bfulgham: review+
patch
none
patch
none
patch none

Antti Koivisto
Reported 2018-08-14 07:56:22 PDT
Make it safer.
Attachments
patch (53.82 KB, patch)
2018-08-14 09:15 PDT, Antti Koivisto 		bfulgham: review+
DetailsFormatted DiffDiff
patch (64.70 KB, patch)
2018-08-15 03:05 PDT, Antti Koivisto 		no flags
DetailsFormatted DiffDiff
patch (65.88 KB, patch)
2018-08-15 04:29 PDT, Antti Koivisto 		no flags
DetailsFormatted DiffDiff
patch (66.40 KB, patch)
2018-08-15 13:36 PDT, Antti Koivisto 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Antti Koivisto
Comment 1 2018-08-14 09:15:43 PDT
Created attachment 347081 [details] patch
Brent Fulgham
Comment 2 2018-08-14 09:30:51 PDT
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
Comment 3 2018-08-14 09:43:15 PDT
> 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
Comment 4 2018-08-15 03:05:20 PDT
Created attachment 347153 [details] patch
EWS Watchlist
Comment 5 2018-08-15 03:57:47 PDT
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
Comment 6 2018-08-15 04:29:58 PDT
Created attachment 347157 [details] patch
Brent Fulgham
Comment 7 2018-08-15 08:02:05 PDT
/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
Comment 8 2018-08-15 13:36:35 PDT
Created attachment 347204 [details] patch
WebKit Commit Bot
Comment 9 2018-08-16 05:49:08 PDT
Comment on attachment 347204 [details] patch Clearing flags on attachment: 347204 Committed r234920: <https://trac.webkit.org/changeset/234920>
WebKit Commit Bot
Comment 10 2018-08-16 05:49:10 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 11 2018-08-16 05:51:30 PDT
<rdar://problem/43378163>
Note You need to log in before you can comment on or make changes to this bug.