- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.7k
Regression(270725@main) Many Web extensions API tests are crashing #20548
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| EWS run on previous version of this PR (hash 9d7547a) | 
| EWS run on previous version of this PR (hash b428bfd) | 
        
          
                Source/WTF/wtf/OptionSet.h
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks risky. It is setting all the bits, including the ones that don't have a valid enum value.
I could see how this function might return incorrect result as a result:
    constexpr bool containsAll(OptionSet optionSet) const
    {
        return (*this & optionSet) == optionSet;
    }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fromRaw makes the input to only valid options. return OptionSet(static_cast<E>(maskRawValue<E>(rawValue)), FromRawValue);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I had missed that 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| EWS run on current version of this PR (hash bd4eb2d) 
 | 
https://webkit.org/b/264885 rdar://problem/118464397 Reviewed by Chris Dumez. Remove the None and All values from Web Extension OptionSet enums, and use new OptionSet::all() instead to get a valid OptionSet for the given enum. Add back the EnumTraits for the OptionSets, since GeneratedSerializers does not generate them, and they are needed by OptionSet to check valid values. * Source/WTF/wtf/OptionSet.h: (WTF::OptionSet::all): Added. * Source/WebKit/Shared/Extensions/WebExtensionWindow.serialization.in: * Source/WebKit/UIProcess/API/Cocoa/_WKWebExtensionContext.mm: (toImpl): * Source/WebKit/UIProcess/Extensions/WebExtensionTab.h: * Source/WebKit/UIProcess/Extensions/WebExtensionWindow.h: * Source/WebKit/WebProcess/Extensions/API/Cocoa/WebExtensionAPIWindowsCocoa.mm: (WebKit::WebExtensionAPIWindows::parseWindowTypesFilter): (WebKit::toWindowTypeFilter): (WebKit::WebExtensionContextProxy::dispatchWindowsEvent): * Source/WebKit/WebProcess/Extensions/API/Cocoa/WebExtensionAPIWindowsEventCocoa.mm: (WebKit::WebExtensionAPIWindowsEvent::invokeListenersWithArgument): * Source/WebKit/WebProcess/Extensions/API/WebExtensionAPIWindowsEvent.h: Canonical link: https://commits.webkit.org/270782@main
bd4eb2d    to
    5effaab      
    Compare
  
    | Committed 270782@main (5effaab): https://commits.webkit.org/270782@main Reviewed commits have been landed. Closing PR #20548 and removing active labels. | 
5effaab
bd4eb2d
🛠 ios🛠 mac🛠 wpe🛠 wincairo🛠 ios-sim🛠 mac-AS-debug🧪 wpe-wk2🧪 ios-wk2🧪 api-mac🛠 gtk🧪 ios-wk2-wpt🧪 mac-wk1🧪 gtk-wk2🛠 🧪 jsc🧪 api-ios🧪 mac-wk2🧪 api-gtk🛠 🧪 jsc-arm64🛠 tv🧪 mac-AS-debug-wk2🛠 jsc-armv7🛠 tv-sim🧪 mac-wk2-stress🧪 jsc-armv7-tests🛠 watch🛠 watch-sim