Bug 85325 - Remove uneeded min/max pref width assignment from RenderView
: Remove uneeded min/max pref width assignment from RenderView
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 45950
  Show dependency treegraph
 
Reported: 2012-05-01 16:47 PST by
Modified: 2012-05-02 17:48 PST (History)


Attachments
Patch (2.39 KB, patch)
2012-05-01 16:51 PST, Eric Seidel
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (3.88 KB, patch)
2012-05-01 17:33 PST, Eric Seidel
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-01 16:47:29 PST
Remove uneeded min/max pref width assignment from RenderView
------- Comment #1 From 2012-05-01 16:51:34 PST -------
Created an attachment (id=139712) [details]
Patch
------- Comment #2 From 2012-05-01 17:20:17 PST -------
With cr-linux (thus the layout tests) passing, this should just be a rubber-stamp for anyone willing.
------- Comment #3 From 2012-05-01 17:23:10 PST -------
(From update of attachment 139712 [details])
Seems reasonable, would be good to eventually get rid of it though!
------- Comment #4 From 2012-05-01 17:23:33 PST -------
(From update of attachment 139712 [details])
Great description and I agree with your conclusion. LGTM.
------- Comment #5 From 2012-05-01 17:27:20 PST -------
Of course we need an actual reviewer to pull the trigger...
------- Comment #6 From 2012-05-01 17:28:51 PST -------
(From update of attachment 139712 [details])
It sounds like we should annotate the function to say that we should remove it and have people not relying on it.
------- Comment #7 From 2012-05-01 17:33:22 PST -------
Created an attachment (id=139717) [details]
Patch for landing
------- Comment #8 From 2012-05-01 19:08:51 PST -------
(From update of attachment 139717 [details])
Clearing flags on attachment: 139717

Committed r115779: <http://trac.webkit.org/changeset/115779>
------- Comment #9 From 2012-05-01 19:08:56 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #10 From 2012-05-02 12:02:09 PST -------
(From update of attachment 139717 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=139717&action=review

> Source/WebCore/rendering/RenderView.h:60
> +    // FIXME: This override is not needed and should be removed
> +    // it only exists to make computePreferredLogicalWidths public.
> +    virtual void computePreferredLogicalWidths() OVERRIDE;

The way to do that is:

    using RenderBlock::computePreferredLogicalWidths;

That makes computePreferredLogicalWidths public in RenderView without adding an override.
------- Comment #11 From 2012-05-02 17:48:33 PST -------
(In reply to comment #10)
> (From update of attachment 139717 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139717&action=review
> 
> > Source/WebCore/rendering/RenderView.h:60
> > +    // FIXME: This override is not needed and should be removed
> > +    // it only exists to make computePreferredLogicalWidths public.
> > +    virtual void computePreferredLogicalWidths() OVERRIDE;
> 
> The way to do that is:
> 
>     using RenderBlock::computePreferredLogicalWidths;
> 
> That makes computePreferredLogicalWidths public in RenderView without adding an override.

Cute.  I wasn't aware of that aspect of c++.

It's unclear that this function even needs to exist.  I believe removing it has a small chance of breaking some (possibly untested) frame flattening code paths, so I've left it for now.  I would like to go back and see about removing this function entirely (as I had on my branch), once I'm done landing seamless.