[Webkit-unassigned] [Bug 106133] document.body.scrollTop & document.documentElement.scrollTop differ cross-browser

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=106133


gur.trio at gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |gur.trio at gmail.com




--- Comment #8 from gur.trio at gmail.com  2013-08-20 06:09:15 PST ---
(In reply to comment #4)
> (From update of attachment 209083 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209083&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +        No new tests (OOPS!).
> 
> WebKit changes that fix bugs must come with tests that cover the new behavior. Code change looks OK, but need test cases demonstrating both quirks mode and strict mode behavior.
> 
> > Source/WebCore/dom/Element.cpp:780
> >      document()->updateLayoutIgnorePendingStylesheets();
> > +    bool inQuirksMode = document()->inQuirksMode();     
> > +
> > +    if (document()->documentElement() == this) {
> > +        if (!inQuirksMode) {
> > +            if (FrameView* view = document()->view()) {
> > +                if (RenderView* renderView = document()->renderView())
> > +                    return adjustForAbsoluteZoom(view->scrollX(), renderView);
> > +            }
> > +        } else
> > +            return 0;
> > +    }
> >      if (RenderBox* rend = renderBox())
> >          return adjustForAbsoluteZoom(rend->scrollLeft(), rend);
> >      return 0;
> 
> A few small improvements are possible here; I suggest doing one or more of these:
> 
> First, if we are in quirks mode and this is the document element, it would be nice to not call updateLayoutIgnorePendingStylesheets at all, since there's no need to do that just to return 0.
> Second, We normally prefer "early return", so it would be more like this:
> 
>     if (inQuirksMode)
>         return 0;
>     // continue with strict mode case
> 
> Third, I think it's slightly more elegant to get the FrameView* from the RenderView* rather than getting both from the Document. Sort of “move over to the view system” first, and then get the relevant objects within the view system rather than going from model to view twice in a row.
> 
> All of these improvements could apply to scrollTop as well.
> 
> > Source/WebCore/html/HTMLBodyElement.cpp:270
> >      Document* document = this->document();
> >      document->updateLayoutIgnorePendingStylesheets();
> >      FrameView* view = document->view();
> > -    return view ? adjustForZoom(view->scrollX(), document) : 0;
> > +    if (document->inQuirksMode())
> > +        return view ? adjustForZoom(view->scrollX(), document) : 0;
> > +    return 0;
> 
> We should restructure the code so it does not do updateLayoutIgnorePendingStylesheets if it is just going to return 0. Same in scrollTop.

Thanks for the review. I have done the changes as per your suggestion and also test cases have been added.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.