DevTools: [Console] Separate height caching from willHide event

DevTools: [Console] Separate height caching from willHide event

This patch introduces ViewportElement.cacheHeight() method which is
called by viewport control in appropriate time to let viewportElement
cache its height. Currently, ViewportElement.willHide() method is
responsible for height caching.

ViewportElement.cacheHeight() must not invalidate DOM. This is not
the case for willHide() method due to the console.table implementation
which removes inner DataGrid views. The bug happens because of the
"scroller securing" (see ViewportElement.refresh method) being done after
the sequence of willHide() calls.

BUG=385685
R=aandrey, pfeldman

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

[lushnikov, 2014-06-17 17:10:02]

PTAL

[aandrey, 2014-06-17 19:21:35]

https://codereview.chromium.org/340483003/diff/1/Source/devtools/front_end/ui...
File Source/devtools/front_end/ui/ViewportControl.js (right):

https://codereview.chromium.org/340483003/diff/1/Source/devtools/front_end/ui...
Source/devtools/front_end/ui/ViewportControl.js:91: cacheHeight: function() { },
maybe willReLayout()?
one may want to cache something else than height, e.g. clientWidth or scrollTop.

or is it only the height that needs to be cached?

is there any contract between this method and
ViewportControl.Provider.fastHeight?
if so, maybe call it cacheFastHeight?
moreover, maybe it should be moved to ViewportControl.Provider interface next to
fastHeight, or move the fastHeight into this interface?

[lushnikov, 2014-06-18 08:27:21]

On 2014/06/17 19:21:35, aandrey wrote:
>
https://codereview.chromium.org/340483003/diff/1/Source/devtools/front_end/ui...
> File Source/devtools/front_end/ui/ViewportControl.js (right):
> 
>
https://codereview.chromium.org/340483003/diff/1/Source/devtools/front_end/ui...
> Source/devtools/front_end/ui/ViewportControl.js:91: cacheHeight: function() {
},
> maybe willReLayout()?
> one may want to cache something else than height, e.g. clientWidth or
scrollTop.
> 
> or is it only the height that needs to be cached?
This method is used only by ViewportControl, and it requires it to cache only
height. Nothing else should be done there.

> 
> is there any contract between this method and
> ViewportControl.Provider.fastHeight?
There is a contract; fastHeight returns the cached height of the viewported
element.

> if so, maybe call it cacheFastHeight?
I'm ok with this name if it makes things clearer.

> moreover, maybe it should be moved to ViewportControl.Provider interface next
to
> fastHeight, or move the fastHeight into this interface?

Here are a few bullets driving this particular implementation:
1. ViewportControl is a factory method that might be creating ViewportElement
instances on demand via itemElement() method. On the other hand,
height of the viewport element might be determined without creating every
ViewportElement by its provider -- this is the case of
FilteredItemSelectionDialog.
Given that ViewportControl queries heights of all viewported elements of
provider, we avoid a lot of pointless allocations by having fastHeight() method
in provider rather then a part ViewportElement
class. See crbug.com/377749

2. On the other side, cacheHeight() should happen for the particular
ViewportElements that have been rendered on screen. There's no way to move
cacheHeight into provider as provider's order of items
might have already changed (consider refreshing viewport as a result of
invalidate() method).

[aandrey, 2014-06-18 09:54:43]

> > if so, maybe call it cacheFastHeight?
> I'm ok with this name if it makes things clearer.

Yeah, it would give at least some hint that this method is related to the
fastHeight method of *another* interface(!)

lgtm

[lushnikov, 2014-06-18 14:54:00]

Done, thanks!

[lushnikov, 2014-06-18 14:54:03]

The CQ bit was checked by lushnikov@chromium.org

[commit-bot: I haz the power, 2014-06-18 14:55:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lushnikov@chromium.org/340483003/20001

[commit-bot: I haz the power, 2014-06-18 16:21:43]

Change committed as 176431