Bug 161610

Created attachment 287988 [details] Patch

Comment on attachment 287988 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287988&action=review > Source/WebCore/dom/Element.cpp:712 > + RenderBox* renderer = renderBox(); > + if (!renderer) > + return; I would slightly prefer auto* here. But the code below does not use renderer, which seems peculiar. Maybe it’s supposed to be renderer->scrollLeft() and renderer->scrollTop() below? > Source/WebCore/dom/Element.cpp:717 > + scrollTo(x, y); I think this should just be all on one line: scrollTo(scrollLeft() + normalizeNonFiniteValue(x), scrollTop() + normalizeNonFiniteValue(y)); No need to keep the code that modifies x and y above on four separate lines of code above. > Source/WebCore/dom/Element.cpp:731 > + RenderBox* renderer = renderBox(); I slightly prefer auto* here. > Source/WebCore/dom/Element.cpp:750 > + ScrollToOptions options; > + options.left = x; > + options.top = y; > + scrollTo(options); I think you can instead just write: scrollTo({ x, y }); Although if I am write about the optional thing then it might need to be: scrollTo(ScrollToOptions{ x, y }); > Source/WebCore/dom/Element.h:141 > + Optional<double> left; > + Optional<double> top; This doesn’t seem right. These aren’t optional in the IDL file. > Source/WebCore/dom/Element.h:144 > + void scrollBy(const ScrollToOptions&); This doesn’t seem right. The IDL file says the options argument is optional, so shouldn’t this be Optional<ScrollToOptions>? Also, with a small structure like this it’s more efficient not to use "const X&". > Source/WebCore/dom/Element.h:146 > + virtual void scrollTo(const ScrollToOptions&); Ditto. > Source/WebCore/dom/Element.idl:75 > - attribute long scrollLeft; > - attribute long scrollTop; > + attribute long scrollLeft; // FIXME: should be unrestricted double > + attribute long scrollTop; // FIXME: should be unrestricted double Seems relatively straightforward to fix this. > Source/WebCore/html/HTMLBodyElement.cpp:304 > + // FIXME: scrolling an independently scrollable body is broken: webkit.org/b/161612 Should be sentence style with a capitalized first word "Scrolling". > Source/WebCore/html/HTMLBodyElement.cpp:305 > + DOMWindow* window = document().domWindow(); I slightly prefer auto* here. > Source/WebCore/html/HTMLBodyElement.cpp:313 > + DOMWindow::ScrollToOptions windowScrollOptions; > + windowScrollOptions.left = options.left; > + windowScrollOptions.top = options.top; > + window->scrollTo(windowScrollOptions); > + return; First, I think we can make DOMWindow::ScrollToOptions and Element::ScrollToOptions be the same struct. Can just use "using" to bring it into one or both classes as a member even if it’s defined in the other class or outside any class. Then we would not need to convert the type here. Second, if we do need to convert it I think we can write it like this: window->scrollTo({ options.left, options.top });

(In reply to comment #2) > Comment on attachment 287988 [details] > Patch > > > Source/WebCore/dom/Element.h:141 > > + Optional<double> left; > > + Optional<double> top; > > This doesn’t seem right. These aren’t optional in the IDL file. I was following what Chris did in bug 157666. > > Source/WebCore/dom/Element.h:144 > > + void scrollBy(const ScrollToOptions&); > > This doesn’t seem right. The IDL file says the options argument is optional, > so shouldn’t this be Optional<ScrollToOptions>? Was also following Chris's change. > > Source/WebCore/dom/Element.idl:75 > > - attribute long scrollLeft; > > - attribute long scrollTop; > > + attribute long scrollLeft; // FIXME: should be unrestricted double > > + attribute long scrollTop; // FIXME: should be unrestricted double > > Seems relatively straightforward to fix this. I didn't want to make a web-facing change here in the same patch. > > Source/WebCore/html/HTMLBodyElement.cpp:313 > > + DOMWindow::ScrollToOptions windowScrollOptions; > > + windowScrollOptions.left = options.left; > > + windowScrollOptions.top = options.top; > > + window->scrollTo(windowScrollOptions); > > + return; > > First, I think we can make DOMWindow::ScrollToOptions and > Element::ScrollToOptions be the same struct. Can just use "using" to bring > it into one or both classes as a member even if it’s defined in the other > class or outside any class. Then we would not need to convert the type here. The generated code references Element::ScrollToOptions and DOMWindow::ScrollToOptions, so I think we'd have to change the generator to fix this?

Comment on attachment 287988 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287988&action=review >>> Source/WebCore/dom/Element.h:141 >>> + Optional<double> top; >> >> This doesn’t seem right. These aren’t optional in the IDL file. > > I was following what Chris did in bug 157666. They are not marked as 'required' in the IDL so they are optional by default as per Web IDL dictionary members. Using Optional<> here seems OK to me.

Comment on attachment 287988 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287988&action=review >>> Source/WebCore/dom/Element.h:144 >>> + void scrollBy(const ScrollToOptions&); >> >> This doesn’t seem right. The IDL file says the options argument is optional, so shouldn’t this be Optional<ScrollToOptions>? Also, with a small structure like this it’s more efficient not to use "const X&". > > Was also following Chris's change. Again, this seems fine to me. If you look at the generated code, it will always call the implementation method with a Dictionary. If scrollBy() is called without parameter, the implementation will be called with scrollBy(ScrollToOptions()).

Comment on attachment 287988 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287988&action=review r=me > Source/WebCore/dom/Element.cpp:708 > + document().updateLayoutIgnorePendingStylesheets(); Calling scrollLeft() already takes care of this. > Source/WebCore/dom/Element.cpp:711 > + if (!renderer) This does not seem very useful since scrollTo() is a no-op if there is not renderer. >> Source/WebCore/dom/Element.cpp:717 >> + scrollTo(x, y); > > I think this should just be all on one line: > > scrollTo(scrollLeft() + normalizeNonFiniteValue(x), scrollTop() + normalizeNonFiniteValue(y)); > > No need to keep the code that modifies x and y above on four separate lines of code above. I agree with Darin here. > Source/WebCore/dom/Element.cpp:727 > + document().updateLayoutIgnorePendingStylesheets(); Seems like we wouldn't need this call if we called Element::setScrollLeft() / setScrollTop() below instead of going straight to the renderer. > Source/WebCore/dom/Element.cpp:742 > + scrollableArea->setScrolledProgrammatically(true); Looks like this wouldn't be needed if we called Element::setScrollLeft() / setScrollTop(). >> Source/WebCore/dom/Element.cpp:750 >> + scrollTo(options); > > I think you can instead just write: > > scrollTo({ x, y }); > > Although if I am write about the optional thing then it might need to be: > > scrollTo(ScrollToOptions{ x, y }); Yes, definitely. >>> Source/WebCore/html/HTMLBodyElement.cpp:313 >>> + return; >> >> First, I think we can make DOMWindow::ScrollToOptions and Element::ScrollToOptions be the same struct. Can just use "using" to bring it into one or both classes as a member even if it’s defined in the other class or outside any class. Then we would not need to convert the type here. >> >> Second, if we do need to convert it I think we can write it like this: >> >> window->scrollTo({ options.left, options.top }); > > The generated code references Element::ScrollToOptions and DOMWindow::ScrollToOptions, so I think we'd have to change the generator to fix this? This is why Darin suggests using "using ..." statements so that Element::ScrollToOptions and DOMWindow::ScrollToOptions point to the same type. No generator change should be needed. I think we should fix this before landing if not too much work. At the very least use window->scrollTo({ options.left, options.top }); like Darin suggests.