Bug 101380 – Autoresize should work even if turned on while the page is loading.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Fady Samuel]

Complete AutoSize after loading

[Fady Samuel]

Created attachment 172624 [details]
Patch

[David Levin]

Looking at this.

[David Levin]

The problem isn't that autoresize really needs to be run after loadcomplete.

The bug is in the case of turning on autoresize after the load has started but before it completed when the initial size is huge.

We have some code in the autoresize function that prevents the size from getting smaller during the load due to janky-ness that happens when we allow this. However, in this case when autoresize is turned on or set to a smaller size, we should make the size smaller.

Here's my suggested fix:

Look for the code in autoSizeIfEnabled that looks like this:
        // Avoid doing resizing to a smaller size while the frame is loading to avoid switching to a small size
        // during an intermediate state (and then changing back to a bigger size as the load progresses).
        if (!frame()->loader()->isComplete() && (newSize.height() < size.height() || newSize.width() < size.width())) {

Change the if to be:

        if (m_didRunAutosize && size.height() <= m_maxAutoSize.height() && size.width() <= m_maxAutoSize.width()
            && (!frame()->loader()->isComplete() && (newSize.height() < size.height() || newSize.width() < size.width())))

And adjust the comment and braces appropriately.


Let me know if this doesn't make sense or you need help with the comment, etc.


What should happen is that

[Fady Samuel]

Created attachment 172705 [details]
Patch

[Fady Samuel]

(In reply to comment #3)
> The problem isn't that autoresize really needs to be run after loadcomplete.
> 
> The bug is in the case of turning on autoresize after the load has started but before it completed when the initial size is huge.
> 
> We have some code in the autoresize function that prevents the size from getting smaller during the load due to janky-ness that happens when we allow this. However, in this case when autoresize is turned on or set to a smaller size, we should make the size smaller.
> 
> Here's my suggested fix:
> 
> Look for the code in autoSizeIfEnabled that looks like this:
>         // Avoid doing resizing to a smaller size while the frame is loading to avoid switching to a small size
>         // during an intermediate state (and then changing back to a bigger size as the load progresses).
>         if (!frame()->loader()->isComplete() && (newSize.height() < size.height() || newSize.width() < size.width())) {
> 
> Change the if to be:
> 
>         if (m_didRunAutosize && size.height() <= m_maxAutoSize.height() && size.width() <= m_maxAutoSize.width()
>             && (!frame()->loader()->isComplete() && (newSize.height() < size.height() || newSize.width() < size.width())))
> 
> And adjust the comment and braces appropriately.
> 
> 
> Let me know if this doesn't make sense or you need help with the comment, etc.
> 
> 
> What should happen is that

I've applied your change locally, and I've confirmed that it solves my problem! Thanks! I'm not too familiar with this algorithm and how it works so I don't really know how to best update this comment. Suggestions would be most helpful. :-)

[David Levin]

(In reply to comment #5)
> 
> I've applied your change locally, and I've confirmed that it solves my problem! Thanks! I'm not too familiar with this algorithm and how it works so I don't really know how to best update this comment. Suggestions would be most helpful. :-)


Here's my suggested rewrite:

// While loading only allow the size to increase (to avoid twitching during intermediate smaller states)
// unless autoresize has just been turned on or the maximum size is smaller than the current size.

I hope this makes sense. The idea would be that this helps explain the code to someone who may not know the code as well so you are an ideal candidate to judge that!

[David Levin]

And I just noticed that I put in too many parenthesis.

This is just as clear:

        if (m_didRunAutosize && size.height() <= m_maxAutoSize.height() && size.width() <= m_maxAutoSize.width()
           && !frame()->loader()->isComplete() && (newSize.height() < size.height() || newSize.width() < size.width()))

[David Levin]

Comment on attachment 172705 [details]
Patch

Please update the comment and fix the parens.

Then I'd be happy to r+.

[David Levin]

Comment on attachment 172705 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172705&action=review

> Source/WebCore/ChangeLog:3
> +        Complete AutoSize after loading

This comment no longer makes sense. (Perhaps the bug should be titled that autoresize should work even if turned on while the page is loading.)

[Fady Samuel]

(In reply to comment #9)
> (From update of attachment 172705 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172705&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Complete AutoSize after loading
> 
> This comment no longer makes sense. (Perhaps the bug should be titled that autoresize should work even if turned on while the page is loading.)

Doh! I take that back. While this works with manual testing, this broke one of my browser_tests. I'm investigating further.

[David Levin]

I am starting to believe that it may be good to do your original change *and* the change I gave you.

The issue is that it does allow the size to get smaller while loading but after load complete, it should likely be given the chance to adjust the size one last time in case the size got smaller.

It is a little odd to the name of the function.

Recommendation:
Do your original change and my addition.

Also, change the name of the function to handleLoadCompleted from checkFlushDeferredRepaintsAfterLoadComplete and add a comment about why the auto size is called there. (See what I wrote above about "the issue".

[Fady Samuel]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 172705 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=172705&action=review
> > 
> > > Source/WebCore/ChangeLog:3
> > > +        Complete AutoSize after loading
> > 
> > This comment no longer makes sense. (Perhaps the bug should be titled that autoresize should work even if turned on while the page is loading.)
> 
> Doh! I take that back. While this works with manual testing, this broke one of my browser_tests. I'm investigating further.

It looks like these two lines, immediately after the break, are the culprit:

       if (document->processingLoadEvent())
            newSize = newSize.expandedTo(size);

Why are we doing this? Does this make sense? If so, I would say this part needs a comment too, because I can't understand why it's there :-)

[Fady Samuel]

Created attachment 172836 [details]
Patch

[David Levin]

Comment on attachment 172836 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172836&action=review

> Source/WebCore/ChangeLog:11
> +

Typically in WebKit, the changelog has some small per function comments as well to explain why a change was done there.

I'll add some below as an example. I would suggest adding them or something of this sort to your change log.

> Source/WebCore/ChangeLog:13
> +        (WebCore::FrameLoader::checkCompleted):

Adjust to function rename.

> Source/WebCore/ChangeLog:15
> +        (WebCore::FrameView::handleLoadCompleted):

Rename function to encompass its expanded responsibilities (which include doing the final auto size after the load is completed).

> Source/WebCore/ChangeLog:16
> +        (WebCore::FrameView::autoSizeIfEnabled):

Allow the size to shrink if autosize is adjusted while the page is loading.

> Source/WebCore/ChangeLog:18
> +        (FrameView):

Function rename.

[Fady Samuel]

Created attachment 172845 [details]
Patch

[WebKit Review Bot]

Comment on attachment 172845 [details]
Patch

Clearing flags on attachment: 172845

Committed r133790: <http://trac.webkit.org/changeset/133790>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.