| Summary: | Delete Node::boundingBox() | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, dino, jonlee, rniwa, simon.fraser, thorton, zalan | ||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||
|
Description
Myles C. Maxfield
2014-12-05 20:17:32 PST
Created attachment 242698 [details]
Patch
Comment on attachment 242698 [details] Patch Attachment 242698 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6135943379550208 New failing tests: fast/spatial-navigation/snav-imagemap-overlapped-areas.html fast/spatial-navigation/snav-container-white-space.html Created attachment 242705 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 242698 [details] Patch Attachment 242698 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5371345248452608 New failing tests: fast/spatial-navigation/snav-imagemap-overlapped-areas.html fast/spatial-navigation/snav-container-white-space.html Created attachment 242706 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 242723 [details]
Patch
Created attachment 242730 [details]
Patch
Looks like EFL compilation was killed Comment on attachment 242730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242730&action=review It would also be better to figure out whether you could push this functionality down to RenderBoxModelObject. (1. not bloat RenderObject 2. no need to put that highly specific function on RenderText) > Source/WebCore/html/ColorInputType.cpp:210 > - return element().document().view()->contentsToRootView(element().pixelSnappedBoundingBox()); > + LayoutRect contentsRect; > + if (RenderObject* renderer = element().renderer()) > + contentsRect = renderer->absoluteBoundingBoxRect(); > + return element().document().view()->contentsToRootView(snappedIntRect(contentsRect)); Please come up with a helper function so that the callers don't need to be this verbose. > Source/WebCore/rendering/RenderObject.cpp:1110 > + const RenderObject* o = this; > + if (!o->isInline() || o->isReplaced()) { > + point = o->localToAbsolute(FloatPoint(), UseTransforms); > + return true; > + } Could you make this copy-paste code look a bit better in this new rendering context? if (!isInline() ||isReplaced()) { and alikes. Comment on attachment 242730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242730&action=review >> Source/WebCore/html/ColorInputType.cpp:210 >> + return element().document().view()->contentsToRootView(snappedIntRect(contentsRect)); > > Please come up with a helper function so that the callers don't need to be this verbose. Done. >> Source/WebCore/rendering/RenderObject.cpp:1110 >> + } > > Could you make this copy-paste code look a bit better in this new rendering context? if (!isInline() ||isReplaced()) { and alikes. Done. (In reply to comment #10) > Comment on attachment 242730 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=242730&action=review > > It would also be better to figure out whether you could push this > functionality down to RenderBoxModelObject. (1. not bloat RenderObject 2. no > need to put that highly specific function on RenderText) It looks like we can't: https://developer.mozilla.org/en-US/docs/Web/API/Element.scrollIntoView is on Element and is API that we support. :( > > > Source/WebCore/html/ColorInputType.cpp:210 > > - return element().document().view()->contentsToRootView(element().pixelSnappedBoundingBox()); > > + LayoutRect contentsRect; > > + if (RenderObject* renderer = element().renderer()) > > + contentsRect = renderer->absoluteBoundingBoxRect(); > > + return element().document().view()->contentsToRootView(snappedIntRect(contentsRect)); > > Please come up with a helper function so that the callers don't need to be > this verbose. > > > Source/WebCore/rendering/RenderObject.cpp:1110 > > + const RenderObject* o = this; > > + if (!o->isInline() || o->isReplaced()) { > > + point = o->localToAbsolute(FloatPoint(), UseTransforms); > > + return true; > > + } > > Could you make this copy-paste code look a bit better in this new rendering > context? if (!isInline() ||isReplaced()) { and alikes. Created attachment 242850 [details]
Patch
Created attachment 242870 [details]
Patch
Comment on attachment 242870 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242870&action=review Looks good. > Source/WebCore/ChangeLog:6 > + Conceptually, boundingBox() should be on RenderObject. In addition, "RenderObject". leftover for the previous patch. > Source/WebCore/accessibility/AccessibilitySlider.cpp:162 > + return downcast<HTMLInputElement>(sliderRenderer->node())->sliderThumbElement()->renderer()->absoluteBoundingBoxRect(); SliderThumbElement::setPositionFromValue() checks for valid renderer(), maybe we should do that too. (Can't tell whether it can or cannot be null) > Source/WebCore/dom/ContainerNode.cpp:913 > +LayoutRect possiblyEmptyRendererAnchorRect(const ContainerNode& n) any reason why you don't use the name 'node' like we use it everywhere else? I am not too sure about the function name, but I am not good at naming either. Some else should check this out too. I don't think the callers care whether the anchor rect is empty. (at least they don't care now) > Source/WebCore/dom/Node.cpp:2262 > +IntRect possiblyEmptyRendererBoundingBox(const Node& n) Same as above. > Source/WebCore/rendering/RenderText.h:167 > + LayoutUnit topOfFirstText() const; Maybe now that the getLeadingCorner() is on RenderElment, we can just call that firstTextBox()->root().lineTop(). RenderElement already deals with RenderInlines. I don't feel too strongly about it though. (I just think this is an overly specific function.) Comment on attachment 242870 [details] Patch Attachment 242870 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4668703429885952 New failing tests: fast/spatial-navigation/snav-imagemap-overlapped-areas.html Created attachment 242872 [details]
Archive of layout-test-results from ews101 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 242870 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242870&action=review >> Source/WebCore/ChangeLog:6 >> + Conceptually, boundingBox() should be on RenderObject. In addition, > > "RenderObject". leftover for the previous patch. Done. >> Source/WebCore/accessibility/AccessibilitySlider.cpp:162 >> + return downcast<HTMLInputElement>(sliderRenderer->node())->sliderThumbElement()->renderer()->absoluteBoundingBoxRect(); > > SliderThumbElement::setPositionFromValue() checks for valid renderer(), maybe we should do that too. (Can't tell whether it can or cannot be null) Done. >> Source/WebCore/dom/ContainerNode.cpp:913 >> +LayoutRect possiblyEmptyRendererAnchorRect(const ContainerNode& n) > > any reason why you don't use the name 'node' like we use it everywhere else? > I am not too sure about the function name, but I am not good at naming either. Some else should check this out too. > I don't think the callers care whether the anchor rect is empty. (at least they don't care now) Done. I think you're right. Done. >> Source/WebCore/dom/Node.cpp:2262 >> +IntRect possiblyEmptyRendererBoundingBox(const Node& n) > > Same as above. Done. >> Source/WebCore/rendering/RenderText.h:167 >> + LayoutUnit topOfFirstText() const; > > Maybe now that the getLeadingCorner() is on RenderElment, we can just call that firstTextBox()->root().lineTop(). RenderElement already deals with RenderInlines. I don't feel too strongly about it though. (I just think this is an overly specific function.) The problem here is that RenderElement.cpp only has a forward declaration of InlineTextBox, so it can't call root() on one because it doesn't know that root() is a member function. Looks like I've got to keep topOfFirstText() :( Created attachment 242883 [details]
Patch
Comment on attachment 242883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242883&action=review > Source/WebCore/ChangeLog:20 > + this false bounding box into RenderObject, using the name RenderElement. > Source/WebCore/dom/ContainerNode.cpp:918 > +LayoutRect rendererAnchorRect(const ContainerNode& node) > +{ > + if (RenderElement* renderer = node.renderer()) > + return renderer->anchorRect(); > + return LayoutRect(); > +} This could go to the header as inline helper. > Source/WebCore/dom/Node.cpp:2268 > +IntRect rendererBoundingBox(const Node& node) > +{ > + if (RenderObject* renderer = node.renderer()) > + return renderer->absoluteBoundingBoxRect(); > + return IntRect(); > +} > + same as above. > Source/WebCore/html/ColorInputType.cpp:207 > - return element().document().view()->contentsToRootView(element().pixelSnappedBoundingBox()); > + return element().document().view()->contentsToRootView(snappedIntRect(rendererBoundingBox(element()))); renderBoundingBox() already returns IntRect(), no need to snap it. > Source/WebCore/page/FrameView.cpp:2790 > + if (anchorNode != frame().document() && anchorNode->renderer()) > + rect = anchorNode->renderer()->anchorRect(); use rendererAnchorRect() helper instead. Comment on attachment 242883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242883&action=review >> Source/WebCore/ChangeLog:20 >> + this false bounding box into RenderObject, using the name > > RenderElement. Done. >> Source/WebCore/dom/ContainerNode.cpp:918 >> +} > > This could go to the header as inline helper. Actually it can't; the header only has a forward declaration of RenderElement, so the ->anchorRect() wouldn't work. >> Source/WebCore/dom/Node.cpp:2268 >> + > > same as above. Same response. >> Source/WebCore/html/ColorInputType.cpp:207 >> + return element().document().view()->contentsToRootView(snappedIntRect(rendererBoundingBox(element()))); > > renderBoundingBox() already returns IntRect(), no need to snap it. Done. >> Source/WebCore/page/FrameView.cpp:2790 >> + rect = anchorNode->renderer()->anchorRect(); > > use rendererAnchorRect() helper instead. Done. Created attachment 242888 [details]
Patch
> >> Source/WebCore/html/ColorInputType.cpp:207
> >> + return element().document().view()->contentsToRootView(snappedIntRect(rendererBoundingBox(element())));
> >
> > renderBoundingBox() already returns IntRect(), no need to snap it.
>
> Done.
I should have pointed out that there are multiple rendererBoundingBox() calls in this patch with redundant snapping. Please remove them all.
Created attachment 242952 [details]
Patch
Created attachment 242955 [details]
Patch
Created attachment 242956 [details]
Patch
Comment on attachment 242956 [details] Patch Clearing flags on attachment: 242956 Committed r177035: <http://trac.webkit.org/changeset/177035> All reviewed patches have been landed. Closing bug. Comment on attachment 242956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242956&action=review Great to clean this up, but I found some details that did not make me entirely happy. > Source/WebCore/ChangeLog:26 > + No new tests because there should be no behavior change. Given the discussion above this seems really surprising. It talks about how some functions depend on “bad behavior”. The “bad behavior” was always safely guaranteed in every case where we wanted it, and never accidentally gotten in any case where we didn’t? Because if there was any case where the code did the wrong thing, then there should be a way to show it with a test. > Source/WebKit/mac/ChangeLog:10 > + * WebView/WebActionMenuController.mm: > + (elementBoundingBoxInWindowCoordinatesFromNode): Use > + RenderObject::absoluteBoundingBoxRect(). This change log and many others describes a change we did not make. Instead, all these call sites were changed to use a helper function in Node.h. I’m not sure that changing from a member to a non-member function was all that helpful; we could simply have made the function non-virtual. Changing all these call sites from node.boundingBox() to rendererBoundingBox(node) seems like a minor refactor that didn’t improve things much. The other change was to always use IntRect rather than having one version that returns a LayoutRect. Was that a good change? I really don’t like this disconnect between what the change log says we are doing and what the code change is. > Source/WebCore/dom/ContainerNode.h:307 > +LayoutRect rendererAnchorRect(const ContainerNode&); Does this really need to be in ContainerNode.h? I don’t think we should have forwarding functions for renderer functions in the DOM code unless they are required to support web-exposed DOM APIs in the IDL file. Otherwise, callers can work with the renderers and it can be more explicit that the result depends on layout and style updating. If we really need this helper function, lets put it in FrameView.cpp, since that’s the only file we are using it in. > Source/WebCore/dom/Node.h:740 > +WEBCORE_EXPORT IntRect rendererBoundingBox(const Node&); It’s irritating and out of place to have this sitting here in the Node.h header. Can we find some clean way of keeping this rendering function out of the DOM? Maybe we need a new header for helper functions to get rendering information from DOM nodes. It would be nice to deprecate this and encourage people to instead use renderer functions to get information about geometry. It seems if we want to leave this in Node.h we should have left it as a member function, albeit a non-virtual one, but I prefer getting it out of Node.h entirely. > Source/WebCore/page/FrameView.cpp:2790 > + rect = rendererAnchorRect(*anchorNode.get()); No need for ".get()" here. The scrollElementToRect function calls updateLayoutIgnorePendingStylesheets, but this does not. Any reason that is OK, or is that a bug we should fix? > Source/WebCore/rendering/RenderElement.h:154 > + // anchorRect() is conceptually similar to absoluteBoundingBoxRect(), but is intended for scrolling to an anchor. > + // It works differently than absoluteBoundingBoxRect() for inline renderers. This comment is frustratingly oblique. It works differently, but how exactly is it different? I think that if we were more specific we could write a smaller comment that would be easier to understand. > Source/WebCore/rendering/RenderText.cpp:1011 > + return firstTextBox()->root().lineTop(); What guarantees firstTextBox() is non-null? Comment on attachment 242956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242956&action=review >> Source/WebCore/ChangeLog:26 >> + No new tests because there should be no behavior change. > > Given the discussion above this seems really surprising. It talks about how some functions depend on “bad behavior”. The “bad behavior” was always safely guaranteed in every case where we wanted it, and never accidentally gotten in any case where we didn’t? Because if there was any case where the code did the wrong thing, then there should be a way to show it with a test. The calls that depend on the bad behavior still use anchorRect(), which preserves the bad behavior. The call in AccessibilitySlider.cpp is on the slider thumb, which is always a block element in the shadow root (so there's no behavior change). The element in the call in ColorInputType.cpp is always forced to inline-block (so there's no behavior change). The call in HTMLInputElement.cpp is inside a !PLATFORM(IOS) block and an ENABLE(DATE_AND_TIME_INPUT_TYPES) block, so both iOS and mac are out. https://bugs.webkit.org/show_bug.cgi?id=139551 The call in ValidationMessage.cpp is only triggered if settings().interactiveFormValidationEnabled() is true, and we don't have internals settings to turn this on. https://bugs.webkit.org/show_bug.cgi?id=139552 The call in SpatialNavigation.cpp did change, and I updated the tests for it. WebPageCoordinatedGraphics.cpp is not compiled on either of our platforms. https://bugs.webkit.org/show_bug.cgi?id=139553 The call in WebActionMenuController.mm is for data detectors, and, according to thorton, is not testable in WebKit 1. https://bugs.webkit.org/show_bug.cgi?id=139554 The call in WebHitTestResult.cpp is used for WebHitTestResult. We only use the result of this call in one place, and according to thorton, that place (_createPreviewPopover) cannot be tested. The call is also exposed in clients of Page, but WebKitTestRunner doesn't enable that information. https://bugs.webkit.org/show_bug.cgi?id=139555 >> Source/WebKit/mac/ChangeLog:10 >> + RenderObject::absoluteBoundingBoxRect(). > > This change log and many others describes a change we did not make. Instead, all these call sites were changed to use a helper function in Node.h. I’m not sure that changing from a member to a non-member function was all that helpful; we could simply have made the function non-virtual. Changing all these call sites from node.boundingBox() to rendererBoundingBox(node) seems like a minor refactor that didn’t improve things much. > > The other change was to always use IntRect rather than having one version that returns a LayoutRect. Was that a good change? > > I really don’t like this disconnect between what the change log says we are doing and what the code change is. Well, the patch migrates callers from Node::boundingBox(), which returns a LayoutRect, to RenderObject::absoluteBoundingBoxRect(), which returns an IntRect. Philosophically, it seems that RenderObject::absoluteBoundingBoxRect() should returns a LayoutRect, but that refactor is for another patch. Do you think I should make a patch to update the existing ChangeLog entry to more accurately reflect the code change? Or is it too late? >> Source/WebCore/dom/ContainerNode.h:307 >> +LayoutRect rendererAnchorRect(const ContainerNode&); > > Does this really need to be in ContainerNode.h? I don’t think we should have forwarding functions for renderer functions in the DOM code unless they are required to support web-exposed DOM APIs in the IDL file. Otherwise, callers can work with the renderers and it can be more explicit that the result depends on layout and style updating. > > If we really need this helper function, lets put it in FrameView.cpp, since that’s the only file we are using it in. This is originally what I had; however, Zalan asked me in comment 10 [1] to make a helper function. I'll post a patch which migrates callers to using renderers directly, and I'll ask you to take a look at it to see if it is better or worse. That is a good idea. Done. [1] https://bugs.webkit.org/show_bug.cgi?id=139333#c10 >> Source/WebCore/page/FrameView.cpp:2790 >> + rect = rendererAnchorRect(*anchorNode.get()); > > No need for ".get()" here. > > The scrollElementToRect function calls updateLayoutIgnorePendingStylesheets, but this does not. Any reason that is OK, or is that a bug we should fix? I'm not sure about updateLayoutIgnorePendingStylesheets. https://bugs.webkit.org/show_bug.cgi?id=139512 >> Source/WebCore/rendering/RenderElement.h:154 >> + // It works differently than absoluteBoundingBoxRect() for inline renderers. > > This comment is frustratingly oblique. It works differently, but how exactly is it different? I think that if we were more specific we could write a smaller comment that would be easier to understand. Done. >> Source/WebCore/rendering/RenderText.cpp:1011 >> + return firstTextBox()->root().lineTop(); > > What guarantees firstTextBox() is non-null? The code that this is replacing already has this assumption in it. |