We should use `static NeverDestroyed<AtomString>`.
Created attachment 398107 [details] Patch
Created attachment 398108 [details] Patch
Comment on attachment 398108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398108&action=review > Source/WebCore/ChangeLog:10 > + `static NeverDestroyed<const AtomString> ...("...", AtomString::ConstructFromLiteral)`. Since HTML is in the main thread, we can just use `static NeverDestroyed<>`. can you add asserts everywhere you did this that we're on the main thread?
I spoke to Yusuke and this is done everywhere. My observation is we should have asserts in all those places. But this patch doesn't need to be the thing adding them.
(In reply to Saam Barati from comment #4) > I spoke to Yusuke and this is done everywhere. > > My observation is we should have asserts in all those places. > > But this patch doesn't need to be the thing adding them. I think we should introduce `MainThreadNeverDestroyed<const AtomString>` and insert `ASSERT(isMainThread());` in get accessor of that class. And replace existing `NeverDestroyed<const AtomString>` with that. But I think this should be done separately given that we already have so many things like that and this patch's goal is not fixing / finding an existing bug with a new abstraction.
(In reply to Yusuke Suzuki from comment #5) > (In reply to Saam Barati from comment #4) > > I spoke to Yusuke and this is done everywhere. > > > > My observation is we should have asserts in all those places. > > > > But this patch doesn't need to be the thing adding them. > > I think we should introduce `MainThreadNeverDestroyed<const AtomString>` and > insert `ASSERT(isMainThread());` in get accessor of that class. And replace > existing `NeverDestroyed<const AtomString>` with that. But I think this > should be done separately given that we already have so many things like > that and this patch's goal is not fixing / finding an existing bug with a > new abstraction. https://bugs.webkit.org/show_bug.cgi?id=211264
Inserted `ASSERT(isMainThread())` for newly added NeverDestroyed<>. I postpone `isMainThread()` assertion check in the existing places in https://bugs.webkit.org/show_bug.cgi?id=211264.
Created attachment 398119 [details] Patch Patch for landing
Comment on attachment 398119 [details] Patch OK, EWS is green.
Committed r260977: <https://trac.webkit.org/changeset/260977> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398119 [details].
<rdar://problem/62691779>
Comment on attachment 398108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398108&action=review >> Source/WebCore/ChangeLog:10 >> + `static NeverDestroyed<const AtomString> ...("...", AtomString::ConstructFromLiteral)`. Since HTML is in the main thread, we can just use `static NeverDestroyed<>`. > > can you add asserts everywhere you did this that we're on the main thread? If so, please don’t write each one out. Make a class that does it! Even one specific to AtomString would be OK with me.
Comment on attachment 398119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398119&action=review > Source/WebCore/html/SearchInputType.cpp:70 > + ASSERT(isMainThread()); Adding all these asserts like this doesn’t seem good. Tons of DOM code all assumes it’s on the main thread. Adding these assertions everywhere we use an AtomString like this is inelegant and inconsistent with WebKit coding style. The DOM code could be peppered with ASSERT(isMainThread()); that assertion is true/needed throughout. If we really want these, I suggest we make a class MainThreadAtomStringLiteral: static MainThreadAtomStringLiteral webkitSearchResultsButtonName("-webkit-search-results-button"); The assertions would go in the "get" function as well as the construction code.
Comment on attachment 398119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398119&action=review >> Source/WebCore/html/SearchInputType.cpp:70 >> + ASSERT(isMainThread()); > > Adding all these asserts like this doesn’t seem good. Tons of DOM code all assumes it’s on the main thread. Adding these assertions everywhere we use an AtomString like this is inelegant and inconsistent with WebKit coding style. The DOM code could be peppered with ASSERT(isMainThread()); that assertion is true/needed throughout. > > If we really want these, I suggest we make a class MainThreadAtomStringLiteral: > > static MainThreadAtomStringLiteral webkitSearchResultsButtonName("-webkit-search-results-button"); > > The assertions would go in the "get" function as well as the construction code. I already landed MainThreadNeverDestroyed :) https://trac.webkit.org/changeset/261013/webkit See https://bugs.webkit.org/show_bug.cgi?id=211223#c7 and https://bugs.webkit.org/show_bug.cgi?id=211264
Comment on attachment 398119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398119&action=review >>> Source/WebCore/html/SearchInputType.cpp:70 >>> + ASSERT(isMainThread()); >> >> Adding all these asserts like this doesn’t seem good. Tons of DOM code all assumes it’s on the main thread. Adding these assertions everywhere we use an AtomString like this is inelegant and inconsistent with WebKit coding style. The DOM code could be peppered with ASSERT(isMainThread()); that assertion is true/needed throughout. >> >> If we really want these, I suggest we make a class MainThreadAtomStringLiteral: >> >> static MainThreadAtomStringLiteral webkitSearchResultsButtonName("-webkit-search-results-button"); >> >> The assertions would go in the "get" function as well as the construction code. > > I already landed MainThreadNeverDestroyed :) https://trac.webkit.org/changeset/261013/webkit > > See https://bugs.webkit.org/show_bug.cgi?id=211223#c7 and https://bugs.webkit.org/show_bug.cgi?id=211264 And that patch actually found one tricky usage in GTK/WPE ports https://trac.webkit.org/changeset/261075/webkit
(In reply to Yusuke Suzuki from comment #14) > I already landed MainThreadNeverDestroyed :) > https://trac.webkit.org/changeset/261013/webkit Could you use that here instead of adding all these assertions?
(In reply to Darin Adler from comment #16) > (In reply to Yusuke Suzuki from comment #14) > > I already landed MainThreadNeverDestroyed :) > > https://trac.webkit.org/changeset/261013/webkit > > Could you use that here instead of adding all these assertions? The patch here is already landed. And https://trac.webkit.org/changeset/261013/webkit is introduced after that, and it replaces inserted `ASSERT(isMainThread())` with `MainThreadNeverDestroyed<>`. So, it is already done :)
(In reply to Yusuke Suzuki from comment #17) > it replaces inserted `ASSERT(isMainThread())` with > `MainThreadNeverDestroyed<>` Great! Sorry I didn’t understand. I still think that one specifically for AtomString would be nice to make the code less wordy.