Because there should not be a good midsummer without a good WebKit patch — and some sauna 🧖♂️️
Created attachment 402805 [details] Patch
Comment on attachment 402805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402805&action=review > Source/JavaScriptCore/ChangeLog:3 > + Non unified build fixes, midsummer 20202 edition You mean 2020, right?
(In reply to Mark Lam from comment #2) > Comment on attachment 402805 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=402805&action=review > > > Source/JavaScriptCore/ChangeLog:3 > > + Non unified build fixes, midsummer 20202 edition > > You mean 2020, right? Most certainly, yes O:-)
Created attachment 402814 [details] Patch for landing
Comment on attachment 402814 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=402814&action=review > Source/JavaScriptCore/b3/air/AirTmpWidth.h:31 > +#include "AirTmpInlines.h" We should never #include an Inlines.h in a .h. This is the wrong way to fix this issue.
(In reply to Mark Lam from comment #5) > Comment on attachment 402814 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=402814&action=review > > > Source/JavaScriptCore/b3/air/AirTmpWidth.h:31 > > +#include "AirTmpInlines.h" > > We should never #include an Inlines.h in a .h. This is the wrong way to fix > this issue. I'll see what I can do about this, thanks for commenting!
Created attachment 403379 [details] Patch
It would be nice if Mark (or anyone else with JSC-fu) could please take a look and tell if the approach of moving the function from AirTmpWidth.h which used the inlines into the .cpp file and including AirTmpInlines.h from there is correct — the rest are the usual boring fixes for non-unified builds :] Thanks in advance!
Comment on attachment 403379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403379&action=review > Source/JavaScriptCore/b3/air/AirTmpWidth.h:-116 > - Widths& widths(Tmp tmp) > - { > - if (tmp.isGP()) { > - unsigned index = AbsoluteTmpMapper<GP>::absoluteIndex(tmp); > - ASSERT(index < m_widthGP.size()); > - return m_widthGP[index]; > - } > - unsigned index = AbsoluteTmpMapper<FP>::absoluteIndex(tmp); > - ASSERT(index < m_widthFP.size()); > - return m_widthFP[index]; > - } Please keep it inline function :)
Comment on attachment 403379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403379&action=review > Source/WTF/wtf/text/StringParsingBuffer.h:31 > #include <wtf/Forward.h> > #include <wtf/text/LChar.h> > #include <wtf/text/StringCommon.h> > +#include <wtf/text/StringView.h> Adding StringView.h allows us to remove some or all of the other includes above. Could you do that please? > Source/WebCore/html/parser/ParsingUtilities.h:33 > -#include <wtf/text/StringCommon.h> > +#include <wtf/text/StringParsingBuffer.h> Is this truly needed? I would have expected that a forward declaration of StringParsingBuffer would suffice. And we could perhaps add that to Forward.h? > Source/WebCore/platform/graphics/ColorSerialization.cpp:29 > +#include "Color.h" Adding Color.h should allow us to remove some of the includes below. > Source/WebCore/platform/graphics/ColorSerialization.h:28 > +#include <wtf/text/WTFString.h> Should include Forward.h instead of WTFString.h.
(In reply to Darin Adler from comment #10) > Comment on attachment 403379 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403379&action=review > > > Source/WTF/wtf/text/StringParsingBuffer.h:31 > > #include <wtf/Forward.h> > > #include <wtf/text/LChar.h> > > #include <wtf/text/StringCommon.h> > > +#include <wtf/text/StringView.h> > > Adding StringView.h allows us to remove some or all of the other includes > above. Could you do that please? Sure thing. > > Source/WebCore/html/parser/ParsingUtilities.h:33 > > -#include <wtf/text/StringCommon.h> > > +#include <wtf/text/StringParsingBuffer.h> > > Is this truly needed? I would have expected that a forward declaration of > StringParsingBuffer would suffice. And we could perhaps add that to > Forward.h? The following seems to work—I'll look into moving the forward declaration into wtf/Forward.h, then: diff --git a/Source/WebCore/html/parser/ParsingUtilities.h b/Source/WebCore/html/parser/ParsingUtilities.h index e853ee7881f..a37a9171b7e 100644 --- a/Source/WebCore/html/parser/ParsingUtilities.h +++ b/Source/WebCore/html/parser/ParsingUtilities.h @@ -30,10 +30,18 @@ #pragma once -#include <wtf/text/StringParsingBuffer.h> +#include <wtf/text/StringCommon.h> + +namespace WTF { + +template <typename T> class StringParsingBuffer; + +}; // namespace WTF namespace WebCore { +using WTF::StringParsingBuffer; + template<typename CharacterType> inline bool isNotASCIISpace(CharacterType c) { return !isASCIISpace(c); > > Source/WebCore/platform/graphics/ColorSerialization.cpp:29 > > +#include "Color.h" > > Adding Color.h should allow us to remove some of the includes below. Aye, will try. > > Source/WebCore/platform/graphics/ColorSerialization.h:28 > > +#include <wtf/text/WTFString.h> > > Should include Forward.h instead of WTFString.h. Okay, I'll try this as well. Thanks for the comments!
(In reply to Yusuke Suzuki from comment #9) > Comment on attachment 403379 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403379&action=review > > > Source/JavaScriptCore/b3/air/AirTmpWidth.h:-116 > > - Widths& widths(Tmp tmp) > > - { > > - if (tmp.isGP()) { > > - unsigned index = AbsoluteTmpMapper<GP>::absoluteIndex(tmp); > > - ASSERT(index < m_widthGP.size()); > > - return m_widthGP[index]; > > - } > > - unsigned index = AbsoluteTmpMapper<FP>::absoluteIndex(tmp); > > - ASSERT(index < m_widthFP.size()); > > - return m_widthFP[index]; > > - } > > Please keep it inline function :) If we keep it inline in AirTmpWidth.h, then we need to include AirTmpInlines.h from there, and Mark advised against doing that. Is there some other way of avoiding including AirTmpInlines.h _and_ also keeping the definition there? Any ideas are welcome :)
Comment on attachment 403379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403379&action=review >>> Source/JavaScriptCore/b3/air/AirTmpWidth.h:-116 >>> - } >> >> Please keep it inline function :) > > If we keep it inline in AirTmpWidth.h, then we need to include > AirTmpInlines.h from there, and Mark advised against doing that. > Is there some other way of avoiding including AirTmpInlines.h _and_ > also keeping the definition there? Any ideas are welcome :) Adding AirTmpWidthInlines.h and move it to that with `inline`, and include it if this function is used :)
Created attachment 404753 [details] Patch
Comment on attachment 404753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404753&action=review > Source/JavaScriptCore/b3/air/AirTmpInlines.h:211 > +inline TmpWidth::Widths& TmpWidth::widths(Tmp tmp) > +{ > + if (tmp.isGP()) { > + unsigned index = AbsoluteTmpMapper<GP>::absoluteIndex(tmp); > + ASSERT(index < m_widthGP.size()); > + return m_widthGP[index]; > + } > + unsigned index = AbsoluteTmpMapper<FP>::absoluteIndex(tmp); > + ASSERT(index < m_widthFP.size()); > + return m_widthFP[index]; > +} > + Given that there's a AirTmpWidth.h and a AirTmpWidth.cpp, this function should be in a AirTmpWidthInlines.h, not AirTmpInlines.h.
(In reply to Mark Lam from comment #15) > Comment on attachment 404753 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=404753&action=review > > > Source/JavaScriptCore/b3/air/AirTmpInlines.h:211 > > +inline TmpWidth::Widths& TmpWidth::widths(Tmp tmp) > > +{ > > + if (tmp.isGP()) { > > + unsigned index = AbsoluteTmpMapper<GP>::absoluteIndex(tmp); > > + ASSERT(index < m_widthGP.size()); > > + return m_widthGP[index]; > > + } > > + unsigned index = AbsoluteTmpMapper<FP>::absoluteIndex(tmp); > > + ASSERT(index < m_widthFP.size()); > > + return m_widthFP[index]; > > +} > > + > > Given that there's a AirTmpWidth.h and a AirTmpWidth.cpp, this function > should be in a AirTmpWidthInlines.h, not AirTmpInlines.h. Aye, will do, thanks for the feedback!
Created attachment 404768 [details] Patch
Committed r264630: <https://trac.webkit.org/changeset/264630> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404768 [details].
<rdar://problem/65853702>