Bug 85325 - Remove uneeded min/max pref width assignment from RenderView
: Remove uneeded min/max pref width assignment from RenderView
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Eric Seidel
:
Depends on:
Blocks: 45950
  Show dependency treegraph
 
Reported: 2012-05-01 16:47 PDT by Eric Seidel
Modified: 2012-05-02 17:48 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel 2012-05-01 16:47:29 PDT
Remove uneeded min/max pref width assignment from RenderView
Comment 1 Eric Seidel 2012-05-01 16:51:34 PDT
Created attachment 139712 [details]
Patch
Comment 2 Eric Seidel 2012-05-01 17:20:17 PDT
With cr-linux (thus the layout tests) passing, this should just be a rubber-stamp for anyone willing.
Comment 3 Emil A Eklund 2012-05-01 17:23:10 PDT
Comment on attachment 139712 [details]
Patch

Seems reasonable, would be good to eventually get rid of it though!
Comment 4 Levi Weintraub 2012-05-01 17:23:33 PDT
Comment on attachment 139712 [details]
Patch

Great description and I agree with your conclusion. LGTM.
Comment 5 Eric Seidel 2012-05-01 17:27:20 PDT
Of course we need an actual reviewer to pull the trigger...
Comment 6 Julien Chaffraix 2012-05-01 17:28:51 PDT
Comment on attachment 139712 [details]
Patch

It sounds like we should annotate the function to say that we should remove it and have people not relying on it.
Comment 7 Eric Seidel 2012-05-01 17:33:22 PDT
Created attachment 139717 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-05-01 19:08:51 PDT
Comment on attachment 139717 [details]
Patch for landing

Clearing flags on attachment: 139717

Committed r115779: <http://trac.webkit.org/changeset/115779>
Comment 9 WebKit Review Bot 2012-05-01 19:08:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Darin Adler 2012-05-02 12:02:09 PDT
Comment on attachment 139717 [details]
Patch for landing

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 Eric Seidel 2012-05-02 17:48:33 PDT
(In reply to comment #10)
> (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.

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.