Autosizing storage doesnot belong on FrameView

Autosizing storage doesnot belong on FrameView

    Removing AutoSize storage from Frameview and create seperate struct to handle autoSize

BUG=401254
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180663

[Shanmuga Pandi, 2014-08-09 12:39:18]

Please review the changes.

[ostap, 2014-08-10 05:01:51]

https://codereview.chromium.org/459633002/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/459633002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:60: struct FrameViewAutoSizeData {
Shouldn't it go into separate .h & .cpp ?

https://codereview.chromium.org/459633002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:445: FrameViewAutoSizeData& ensureAutoSizeData()
autoSizeData() ?

[Shanmuga Pandi, 2014-08-10 09:56:48]

https://codereview.chromium.org/459633002/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/459633002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:60: struct FrameViewAutoSizeData {
On 2014/08/10 05:01:51, ostap wrote:
> Shouldn't it go into separate .h & .cpp ?

I thought it is going to be used only in FrameView. I make it like that. Should
I move it to separate file ?

https://codereview.chromium.org/459633002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:445: FrameViewAutoSizeData& ensureAutoSizeData()
On 2014/08/10 05:01:51, ostap wrote:
> autoSizeData() ?

I have referred ensureRareData in RenderBox.h. 
Can I rename to autoSizeData() ?

[Shanmuga Pandi, 2014-08-11 08:58:00]

Please have a look on these changes.

https://codereview.chromium.org/459633002/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/459633002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:60: struct FrameViewAutoSizeData {
On 2014/08/10 05:01:51, ostap wrote:
> Shouldn't it go into separate .h & .cpp ?

Done.

https://codereview.chromium.org/459633002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:445: FrameViewAutoSizeData& ensureAutoSizeData()
On 2014/08/10 05:01:51, ostap wrote:
> autoSizeData() ?

Done.

[ostap, 2014-08-11 19:24:27]

https://codereview.chromium.org/459633002/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/459633002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:60: struct FrameViewAutoSizeData {
On 2014/08/10 09:56:48, Shanmuga Pandi wrote:
> On 2014/08/10 05:01:51, ostap wrote:
> > Shouldn't it go into separate .h & .cpp ?
> 
> I thought it is going to be used only in FrameView. I make it like that.
Should
> I move it to separate file ?

I see.
Ok.

https://codereview.chromium.org/459633002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:445: FrameViewAutoSizeData& ensureAutoSizeData()
On 2014/08/10 09:56:48, Shanmuga Pandi wrote:
> On 2014/08/10 05:01:51, ostap wrote:
> > autoSizeData() ?
> 
> I have referred ensureRareData in RenderBox.h. 
> Can I rename to autoSizeData() ?

I sees it both ways in code, so it should be fine as is.

[ostap, 2014-08-11 19:27:23]

On 2014/08/11 08:58:00, Shanmuga Pandi wrote:
> Please have a look on these changes.

I think it is Ok now.
Please add blink owners to reviewers.

[Shanmuga Pandi, 2014-08-12 05:09:21]

pdr@chromium.org: Please review the changes.

[pdr., 2014-08-12 05:19:15]

I think this looks good.

+cc eseidel

[Shanmuga Pandi, 2014-08-12 08:32:40]

Rebased the code, due to trybot failure. 
Please add trybot again.

[Shanmuga Pandi, 2014-08-13 04:25:21]

@esidel
  Gentle ping!!

[eseidel, 2014-08-19 16:31:18]

I think this is no worse than what we have now.  But we definitely could do
better here.

If you're willing to make further improvements to this code, please consider my
comments below.

rslgtm.

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:858: autoSizeInfo().autoSizeIfEnabled();
This caused us to allocate the AutosizeInfo.  I would keep autoSizeIfEnabled()
on FrameView, and use the existance of an Autosizer to indicate that it's
enabled or not.

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameViewAutoSizeInfo.h (right):

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameViewAutoSizeInfo.h:23: void enableAutoSizeMode(bool
enable, const IntSize& minSize, const IntSize& maxSize);
This doesn't need to take a bool, does it?

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameViewAutoSizeInfo.h:27: FrameView* m_frameView;
This can never be null, so might as well make it a reference.

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameViewAutoSizeInfo.h:30: bool m_shouldAutoSize;
This bool isnt' needed.  We could have the presence of this "autosizer" on
FrameView indicate that autosizing is enabled, no?  Or do we need to be able to
enable/disable autosizing while keeping the current size max/min autosize
settings around?

[eseidel, 2014-08-19 16:35:00]

In any case, I think we should land this as-is and consider a second patch to
make this logic more clear.

We can think of this "autosizer" as an add-on to FrameView, that isn't itself
part of Frameview.  It's that someone has decided to add this "plugin" to
frameview to change its behavior.

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:2746: void FrameView::enableAutoSizeMode(bool
enable, const IntSize& minSize, const IntSize& maxSize)
I suspect we want to split enableAutoSizeMode out from disableAutoSizeMode, and
remove the bool.

This could do the actual allocation of the FrameAutosizer and then set the sizes
on it.

disable could just clear hte m_autosizer.  And checking if autosizing was
enabled would just be checkig bool( m_autosizer).

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:410: m_autoSizeInfo = adoptPtr(new
FrameViewAutoSizeInfo(this));
Note that the way this is designed, we're always going to allocate one of these
the first time we go through layout.

[Shanmuga Pandi, 2014-08-20 07:38:09]

On 2014/08/19 16:35:00, eseidel wrote:
> In any case, I think we should land this as-is and consider a second patch to
> make this logic more clear.
> 
> We can think of this "autosizer" as an add-on to FrameView, that isn't itself
> part of Frameview.  It's that someone has decided to add this "plugin" to
> frameview to change its behavior.
> 
>
https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
> File Source/core/frame/FrameView.cpp (left):
> 
>
https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
> Source/core/frame/FrameView.cpp:2746: void FrameView::enableAutoSizeMode(bool
> enable, const IntSize& minSize, const IntSize& maxSize)
> I suspect we want to split enableAutoSizeMode out from disableAutoSizeMode,
and
> remove the bool.
> 
> This could do the actual allocation of the FrameAutosizer and then set the
sizes
> on it.
> 
> disable could just clear hte m_autosizer.  And checking if autosizing was
> enabled would just be checkig bool( m_autosizer).
> 
>
https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
> File Source/core/frame/FrameView.h (right):
> 
>
https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
> Source/core/frame/FrameView.h:410: m_autoSizeInfo = adoptPtr(new
> FrameViewAutoSizeInfo(this));
> Note that the way this is designed, we're always going to allocate one of
these
> the first time we go through layout.

I got your point. I will work on those and will submit new patch

[Shanmuga Pandi, 2014-08-20 14:54:13]

@eseidel
  Thanks for reviewing the patch.
I have uploaded new patch based on your comments. Please review it.

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:2746: void FrameView::enableAutoSizeMode(bool
enable, const IntSize& minSize, const IntSize& maxSize)
On 2014/08/19 16:35:00, eseidel wrote:
> I suspect we want to split enableAutoSizeMode out from disableAutoSizeMode,
and
> remove the bool.
> 
> This could do the actual allocation of the FrameAutosizer and then set the
sizes
> on it.
> 
> disable could just clear hte m_autosizer.  And checking if autosizing was
> enabled would just be checkig bool( m_autosizer).

Done.

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:858: autoSizeInfo().autoSizeIfEnabled();
On 2014/08/19 16:31:18, eseidel wrote:
> This caused us to allocate the AutosizeInfo.  I would keep autoSizeIfEnabled()
> on FrameView, and use the existance of an Autosizer to indicate that it's
> enabled or not.

Done.

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:410: m_autoSizeInfo = adoptPtr(new
FrameViewAutoSizeInfo(this));
On 2014/08/19 16:35:00, eseidel wrote:
> Note that the way this is designed, we're always going to allocate one of
these
> the first time we go through layout.

Changed logic.

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameViewAutoSizeInfo.h (right):

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameViewAutoSizeInfo.h:23: void enableAutoSizeMode(bool
enable, const IntSize& minSize, const IntSize& maxSize);
On 2014/08/19 16:31:18, eseidel wrote:
> This doesn't need to take a bool, does it?

Done.

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameViewAutoSizeInfo.h:27: FrameView* m_frameView;
On 2014/08/19 16:31:18, eseidel wrote:
> This can never be null, so might as well make it a reference.

Done.

https://codereview.chromium.org/459633002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameViewAutoSizeInfo.h:30: bool m_shouldAutoSize;
On 2014/08/19 16:31:18, eseidel wrote:
> This bool isnt' needed.  We could have the presence of this "autosizer" on
> FrameView indicate that autosizing is enabled, no?  Or do we need to be able
to
> enable/disable autosizing while keeping the current size max/min autosize
> settings around?

Done.

[eseidel, 2014-08-20 16:03:33]

The CQ bit was checked by eseidel@chromium.org

[eseidel, 2014-08-20 16:03:33]

lgtm

[commit-bot: I haz the power, 2014-08-20 16:03:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shanmuga.m@samsung.com/459633002/80001

[commit-bot: I haz the power, 2014-08-20 17:03:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/23673)

[commit-bot: I haz the power, 2014-08-20 17:38:22]

Committed patchset #5 (80001) as 180663