Bug 109750

Created attachment 188186 [details] Patch

Comment on attachment 188186 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188186&action=review > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:109 > + if (m_tagId == UnknownTagId) > return; I think this is very clever. It's not clear to me that the HTMLTagIdentifier stuff needs to leak out of this class at all. It could just be private to StartTagScanner? I assume it's part of a larger plan you have for world domination, hence being separate? > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:112 > AtomicString attributeName(iter->name); I believe this will crash on the background thread, no? It looks like you don't even use it as Atomic, but making it Atomic does prevent a copy in teh common case on the main thread.

(In reply to comment #2) > (From update of attachment 188186 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=188186&action=review > > > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:109 > > + if (m_tagId == UnknownTagId) > > return; > > I think this is very clever. It's not clear to me that the HTMLTagIdentifier stuff needs to leak out of this class at all. It could just be private to StartTagScanner? I assume it's part of a larger plan you have for world domination, hence being separate? It's going to leak out of the class, but hopefully not out of the file. For example, I plan to use it in processPossibleStyleTag as well. > > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:112 > > AtomicString attributeName(iter->name); > > I believe this will crash on the background thread, no? It looks like you don't even use it as Atomic, but making it Atomic does prevent a copy in teh common case on the main thread. Yeah, processAttributes(const HTMLToken::AttributeList& attributes) is going to be main thread only. I plan to add a processAttributes that takes a CompactHTMLToken::AttributeList or whatever the actual type is called. I can add an ASSERT(isMainThread()) to this function if that would be helpful.

Review ping. This patch is blocking this stack of patches. :)

Comment on attachment 188186 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188186&action=review LGTM. >>> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:112 >>> AtomicString attributeName(iter->name); >> >> I believe this will crash on the background thread, no? It looks like you don't even use it as Atomic, but making it Atomic does prevent a copy in teh common case on the main thread. > > Yeah, processAttributes(const HTMLToken::AttributeList& attributes) is going to be main thread only. I plan to add a processAttributes that takes a CompactHTMLToken::AttributeList or whatever the actual type is called. I can add an ASSERT(isMainThread()) to this function if that would be helpful. I think ASSERTing when we should be on the main thread is helpful, yes.

Created attachment 188243 [details] Patch for landing

Comment on attachment 188243 [details] Patch for landing Clearing flags on attachment: 188243 Committed r142840: <http://trac.webkit.org/changeset/142840>

All reviewed patches have been landed. Closing bug.