.
Created attachment 446944 [details] Patch
Created attachment 446964 [details] Slightly simpler approach
Comment on attachment 446964 [details] Slightly simpler approach The webrtc and XHR test failures do not appear to be related.
Created attachment 447020 [details] Minor style tweaks
Comment on attachment 447020 [details] Minor style tweaks If this is just about fixed position, please just use the term "fixed". "Viewport constrained" means "fixed or sticky" in the rest of the code.
(In reply to Simon Fraser (smfr) from comment #5) > Comment on attachment 447020 [details] > Minor style tweaks > > If this is just about fixed position, please just use the term "fixed". > "Viewport constrained" means "fixed or sticky" in the rest of the code. Will do!
Created attachment 447058 [details] Fix names
Comment on attachment 447058 [details] Fix names View in context: https://bugs.webkit.org/attachment.cgi?id=447058&action=review I don't think this code should be in style resolution. Tracking the fixed position ancestor could be more naturally tracked during render tree building (in RenderTreeUpdater) where the rest of the related code is going too. It should be really simple too, a single RenderTreeUpdater scope variable I think. Looking at the other patches the mechanism seems to be doing content scanning synchronously and then triggering another style update. Is the idea to make some portion of this asynchronous? > Source/WebCore/style/StyleUpdate.cpp:143 > +Element* Update::fixedPositionRoot(Element& element, const RenderStyle& style, Element* parentFixedPositionRoot) > +{ > + if (!m_document->shouldObserveFixedPositionRoots()) > + return parentFixedPositionRoot; > + > + if (!composedTreeAncestors(element).first() || is<HTMLBodyElement>(element)) > + return parentFixedPositionRoot; > + > + if (!isFixedPosition(style) || parentFixedPositionRoot) > + return parentFixedPositionRoot; > + > + return &element; > +} This non-static Style::Update member function doesn't access the object at all (besides m_document). It could just be a static function in the caller. The code here is bit silly in general. If you pass a non-null parentFixedPositionRoot the function just returns it, after various tests.
Comment on attachment 447058 [details] Fix names (In reply to Antti Koivisto from comment #8) > Comment on attachment 447058 [details] > Fix names > > View in context: > https://bugs.webkit.org/attachment.cgi?id=447058&action=review > > I don't think this code should be in style resolution. Tracking the fixed > position ancestor could be more naturally tracked during render tree > building (in RenderTreeUpdater) where the rest of the related code is going > too. It should be really simple too, a single RenderTreeUpdater scope > variable I think. > 👍🏻 Changing this so that it runs as a post-layout task, as we discussed!
Note that FrameView already has: std::unique_ptr<WeakHashSet<RenderLayerModelObject>> m_viewportConstrainedObjects;
(In reply to Simon Fraser (smfr) from comment #10) > Note that FrameView already has: > std::unique_ptr<WeakHashSet<RenderLayerModelObject>> > m_viewportConstrainedObjects; Thank you, that works great!
Created attachment 447385 [details] Patch
Created attachment 447386 [details] Patch
Created attachment 447387 [details] Remove unneeded #include
Comment on attachment 447387 [details] Remove unneeded #include Thanks for the review!
Comment on attachment 447387 [details] Remove unneeded #include View in context: https://bugs.webkit.org/attachment.cgi?id=447387&action=review > Source/WebCore/page/ModalContainerObserver.cpp:125 > + RefPtr element = object.element(); > + if (!element || is<HTMLBodyElement>(*element) || element->isDocumentNode() || !m_elementsToIgnoreWhenSearching.add(*element).isNewEntry) > + continue; > + > + for (auto& textNode : descendantsOfType<Text>(*element)) { > + if (!matchesSearchTerm(textNode, searchTerm)) > + continue; > + > + m_container = element.get(); Any particular reason why you traverse the DOM tree and not the render tree? (the renderer tree in general is "closer" to what ends up on the screens)
Comment on attachment 447387 [details] Remove unneeded #include View in context: https://bugs.webkit.org/attachment.cgi?id=447387&action=review r=me as well :) > Source/WebCore/page/ModalContainerObserver.cpp:49 > + Style: I'd remove this newline > Source/WebCore/page/ModalContainerObserver.cpp:57 > + // FIXME: Need to properly support subframes. Is there a bug for this? > Source/WebCore/page/ModalContainerObserver.cpp:64 > + RefPtr frame = document.frame(); NIT: my understanding is that since `frame` is only used for simple getters (i.e. no complicated logic) it doesn't need to be ref'd > Source/WebCore/page/ModalContainerObserver.cpp:81 > + bool shouldSearchNode = ([&] { NIT: Does this really need to be a lambda? > Source/WebCore/page/ModalContainerObserver.cpp:113 > + if (searchTerm.isNull()) NIT: you could `!searchTerm` > Source/WebCore/page/ModalContainerObserver.cpp:118 > + if (!element || is<HTMLBodyElement>(*element) || element->isDocumentNode() || !m_elementsToIgnoreWhenSearching.add(*element).isNewEntry) Style: I'd split this into separate lines so that the `WeakHashSet` operation is more obvious
Comment on attachment 447387 [details] Remove unneeded #include View in context: https://bugs.webkit.org/attachment.cgi?id=447387&action=review Thanks for taking a look! >> Source/WebCore/page/ModalContainerObserver.cpp:49 >> + > > Style: I'd remove this newline Fixed, good catch. >> Source/WebCore/page/ModalContainerObserver.cpp:57 >> + // FIXME: Need to properly support subframes. > > Is there a bug for this? Filed: https://bugs.webkit.org/show_bug.cgi?id=234446. I'll add a FIXME referencing it, too. >> Source/WebCore/page/ModalContainerObserver.cpp:64 >> + RefPtr frame = document.frame(); > > NIT: my understanding is that since `frame` is only used for simple getters (i.e. no complicated logic) it doesn't need to be ref'd Sure. >> Source/WebCore/page/ModalContainerObserver.cpp:81 >> + bool shouldSearchNode = ([&] { > > NIT: Does this really need to be a lambda? I guess it doesn't. Will remove. >> Source/WebCore/page/ModalContainerObserver.cpp:113 >> + if (searchTerm.isNull()) > > NIT: you could `!searchTerm` AtomString doesn't have an `operator bool()`. >> Source/WebCore/page/ModalContainerObserver.cpp:118 >> + if (!element || is<HTMLBodyElement>(*element) || element->isDocumentNode() || !m_elementsToIgnoreWhenSearching.add(*element).isNewEntry) > > Style: I'd split this into separate lines so that the `WeakHashSet` operation is more obvious Sure. >> Source/WebCore/page/ModalContainerObserver.cpp:125 >> + m_container = element.get(); > > Any particular reason why you traverse the DOM tree and not the render tree? (the renderer tree in general is "closer" to what ends up on the screens) That's a good point! I suppose I can traverse `RenderText` instead of `Text` nodes here — will give that a try.
>NIT: Does this really need to be a lambda? my answer would be yes, always.
Comment on attachment 447387 [details] Remove unneeded #include View in context: https://bugs.webkit.org/attachment.cgi?id=447387&action=review > Source/WebCore/page/ModalContainerObserver.cpp:116 > + for (auto& object : *view.viewportConstrainedObjects()) { btw, there are renderers. let's call them that.
(In reply to zalan from comment #20) > Comment on attachment 447387 [details] > Remove unneeded #include > > View in context: > https://bugs.webkit.org/attachment.cgi?id=447387&action=review > > > Source/WebCore/page/ModalContainerObserver.cpp:116 > > + for (auto& object : *view.viewportConstrainedObjects()) { > > btw, there are renderers. let's call them that. these are even
Comment on attachment 447387 [details] Remove unneeded #include View in context: https://bugs.webkit.org/attachment.cgi?id=447387&action=review >>> Source/WebCore/page/ModalContainerObserver.cpp:116 >>> + for (auto& object : *view.viewportConstrainedObjects()) { >> >> btw, there are renderers. let's call them that. > > these are even True — renamed to `renderer`!
(In reply to zalan from comment #19) > >NIT: Does this really need to be a lambda? > my answer would be yes, always. I'm also a fan of "inline lambdas w/ early returns"! I suppose I'll keep this one here after all.
(In reply to Wenson Hsieh from comment #23) > (In reply to zalan from comment #19) > > >NIT: Does this really need to be a lambda? > > my answer would be yes, always. > > I'm also a fan of "inline lambdas w/ early returns"! I suppose I'll keep > this one here after all. Yeah + great for scoping + replaces comments like // Check if we should search in this node
Created attachment 447493 [details] For EWS
Committed r287217 (245380@main): <https://commits.webkit.org/245380@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 447493 [details].
<rdar://problem/86659823>