[Contextual Search] Fixing ViewResourceInflater layout invalidation.

[Contextual Search] Fixing ViewResourceInflater layout invalidation.

This CL changes the ViewResourceInflater class to account for
changes in the layout.

This change is necessary to fix OverlayPanelInflater instances
not being properly updates upon rotation, which will be
addressed in a followup CL.

BUG=596978
Committed: https://crrev.com/ce771b386aa324fee29fd2534487e331ff421ff4
Cr-Commit-Position: refs/heads/master@{#383540}

[pedro (no code reviews), 2016-03-22 18:52:34]

pedrosimonetti@chromium.org changed reviewers:
+ dtrainor@chromium.org

[pedro (no code reviews), 2016-03-22 18:52:35]

David, please take a look at this change.

[pedro (no code reviews), 2016-03-23 01:14:53]

Description was changed from

==========
[Contextual Search] Fixing ViewResourceInflater layout invalidation.

This CL changes the ViewResourceInflater class to account for
changes in the layout.

This change is necessary to fix OverlayPanelInflater instances
not being properly updates upon rotation, which will be
addressed in a followup CL.

BUG=596978
==========

to

==========
[Contextual Search] Fixing ViewResourceInflater layout invalidation.

This CL changes the ViewResourceInflater class to account for
changes in the layout.

This change is necessary to fix OverlayPanelInflater instances
not being properly updates upon rotation, which will be
addressed in a followup CL.

BUG=596978
==========

[pedro (no code reviews), 2016-03-23 01:14:53]

pedrosimonetti@chromium.org changed reviewers:
+ jaekyun@chromium.org
- dtrainor@chromium.org

[pedro (no code reviews), 2016-03-23 01:15:44]

Just noticed dtrainor@ will OOO for some time, so I'm delegating this to
jaekyun@.

Jaekyun: please take a look at this change.

[Jaekyun Seok (inactive), 2016-03-23 01:38:35]

jaekyun@chromium.org changed reviewers:
+ changwan@chromium.org
- jaekyun@chromium.org

[Jaekyun Seok (inactive), 2016-03-23 01:40:40]

On 2016/03/23 01:15:44, pedrosimonetti wrote:
> Just noticed dtrainor@ will OOO for some time, so I'm delegating this to
> jaekyun@.
> 
> Jaekyun: please take a look at this change.

I'm not working on the project anymore.

Changwan, could you please take a look at this?

[pedro (no code reviews), 2016-03-23 18:58:01]

pedrosimonetti@chromium.org changed reviewers:
+ newt@chromium.org
- changwan@chromium.org

[pedro (no code reviews), 2016-03-23 19:01:13]

It turns out Changwan is not an owner of /ui/chrome, so I'm sending this to
newt@ instead.

Newton: Please take a look at this change.

Changwan: I'm moving you to cc list, as an optional reviewer.

Matt: I'm also adding you to the cc list, as a FYI.

[newt (away), 2016-03-23 22:14:52]

The control flow with mDidRequestLayout is rather confusing: requestLayout()
sets mDidRequestLayout and then calls invalidate(), which checks
mDidRequestLayout and then calls layout(), which resets mDidRequestLayout and
then calls View.requestLayout().

How about removing the magical mDidRequestLayout variable, and replacing it with
a local variable inside the invalidate() method:

    public void invalidate() {
        invalidate(false);
    }

    public void requestLayout() {
        invalidate(true);
    }

    private void invalidate(boolean requestLayout) {
        ...

        if (requestLayout) {
            layout();
        }

        ...
    }

https://codereview.chromium.org/1823203002/diff/1/ui/android/java/src/org/chr...
File
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java
(right):

https://codereview.chromium.org/1823203002/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:300:
* Update the View layout.
How about: "Lays out the view according to the current width and height measure
specs."

https://codereview.chromium.org/1823203002/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:308:
if (mIsAttached) {
Could you call layout() before the view is attached? Layout should work fine
when the view isn't attached to the hierarchy and that would simplify the code
paths here.

https://codereview.chromium.org/1823203002/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:310:
if (View.MeasureSpec.getMode(widthMeasureSpec) == View.MeasureSpec.EXACTLY) {
If getWidthMeasureSpec() returns EXACTLY one time, and then UNSPECIFIED another
time, this code won't correctly reset mView.getLayoutParams().width. That should
be fixed.

https://codereview.chromium.org/1823203002/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:319:
mView.requestLayout();
How do you currently know when the layout has been performed?

[pedro (no code reviews), 2016-03-24 22:21:13]

Newton, thanks for the suggestions. Please take another look.

https://codereview.chromium.org/1823203002/diff/1/ui/android/java/src/org/chr...
File
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java
(right):

https://codereview.chromium.org/1823203002/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:300:
* Update the View layout.
On 2016/03/23 22:14:52, newt wrote:
> How about: "Lays out the view according to the current width and height
measure
> specs."

Done.

https://codereview.chromium.org/1823203002/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:308:
if (mIsAttached) {
On 2016/03/23 22:14:52, newt wrote:
> Could you call layout() before the view is attached? Layout should work fine
> when the view isn't attached to the hierarchy and that would simplify the code
> paths here.

Take another look. I simplified things a little bit, and now we are laying out
the view before attaching to the hierarchy.

https://codereview.chromium.org/1823203002/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:310:
if (View.MeasureSpec.getMode(widthMeasureSpec) == View.MeasureSpec.EXACTLY) {
On 2016/03/23 22:14:52, newt wrote:
> If getWidthMeasureSpec() returns EXACTLY one time, and then UNSPECIFIED
another
> time, this code won't correctly reset mView.getLayoutParams().width. That
should
> be fixed.

Good point. Done.

https://codereview.chromium.org/1823203002/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:319:
mView.requestLayout();
On 2016/03/23 22:14:52, newt wrote:
> How do you currently know when the layout has been performed?

As discussed offline, we use the onDraw() method to track view changes.

[newt (away), 2016-03-24 22:31:36]

Thanks. Looks better :)  One last comment

https://codereview.chromium.org/1823203002/diff/20001/ui/android/java/src/org...
File
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java
(right):

https://codereview.chromium.org/1823203002/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:329:
mView.measure(getWidthMeasureSpec(), getHeightMeasureSpec());
If you're going to attach this view to the view hierarchy, there's no point in
measuring and laying it out in advance; it'll just get measured and laid out
again once it's attached to the hierarchy. So, I'd skip these lines in that
case.

[pedro (no code reviews), 2016-03-25 00:37:47]

Newton, please take another look.

https://codereview.chromium.org/1823203002/diff/20001/ui/android/java/src/org...
File
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java
(right):

https://codereview.chromium.org/1823203002/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:329:
mView.measure(getWidthMeasureSpec(), getHeightMeasureSpec());
On 2016/03/24 22:31:36, newt wrote:
> If you're going to attach this view to the view hierarchy, there's no point in
> measuring and laying it out in advance; it'll just get measured and laid out
> again once it's attached to the hierarchy. So, I'd skip these lines in that
> case.

Made changes based on our offline discussion.

[newt (away), 2016-03-25 00:49:20]

lgtm after comments. Thanks :)

https://codereview.chromium.org/1823203002/diff/40001/ui/android/java/src/org...
File
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java
(right):

https://codereview.chromium.org/1823203002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:274:
private void invalidate(boolean shouldUpdateLayoutParams) {
actually, I'd call this parameter didSizeChange as well, since updating the
layout params is mostly an implementation details as you mentioned.

https://codereview.chromium.org/1823203002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:291:
// Layout the View if necessary, updating its size.
"Update the View's layout params, which will trigger a re-layout."

[pedro (no code reviews), 2016-03-25 18:07:57]

Newton, please take another look.

I did some more testes and noticed updating layout params is only necessary when
attached to the view, so I made a small change to not update the params when not
attached.

Also, I decided to expose the invalidate(boolean didViewSizeChange) method,
removing the need for the onSizeChanged method. I this makes the API simpler to
use.

https://codereview.chromium.org/1823203002/diff/40001/ui/android/java/src/org...
File
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java
(right):

https://codereview.chromium.org/1823203002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:274:
private void invalidate(boolean shouldUpdateLayoutParams) {
On 2016/03/25 00:49:20, newt wrote:
> actually, I'd call this parameter didSizeChange as well, since updating the
> layout params is mostly an implementation details as you mentioned.

Thanks for pointing this out. I'm calling it now didViewSizeChange, which is
slightly more specific.

https://codereview.chromium.org/1823203002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:291:
// Layout the View if necessary, updating its size.
On 2016/03/25 00:49:20, newt wrote:
> "Update the View's layout params, which will trigger a re-layout."

Done.

[newt (away), 2016-03-25 18:27:00]

Sorry -- one more round of comments :)

https://codereview.chromium.org/1823203002/diff/60001/ui/android/java/src/org...
File
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java
(right):

https://codereview.chromium.org/1823203002/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:137:
* @param didViewSizeChange Whether the View's size has changed..
"Whether the container's size has changed", and maybe call the param
"didContainerSizeChange"

https://codereview.chromium.org/1823203002/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:156:
if (didViewSizeChange) {
If the view is already attached (mIsAttached is true), then we still want to
update the layout params, right? Also, the first time the view is attached, we
should update the layout params since they won't be correct to begin with,
right?

In that case, this should be:

    if (!mIsAttached && shouldAttachView()) {
        attachView();
        didViewSizeChange = true;
    }

    if (mIsAttached && didViewSizeChange) {
        updateLayoutParams();
    }

[pedro (no code reviews), 2016-03-26 00:11:25]

Thanks for catching a bug! Please take another look and feel free to post more
comments if you so desire.

https://codereview.chromium.org/1823203002/diff/60001/ui/android/java/src/org...
File
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java
(right):

https://codereview.chromium.org/1823203002/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:137:
* @param didViewSizeChange Whether the View's size has changed..
On 2016/03/25 18:27:00, newt wrote:
> "Whether the container's size has changed", and maybe call the param
> "didContainerSizeChange"

This class handles the use of a particular Android View as a Dynamic Resource.
In this sense, I think it makes more sense to call it didViewSizeChange, because
the View is what this class care about. Also, a View could change its size
independently from its container. Similarly, a change in container size not
necessarily will imply in changing the size of the View.

https://codereview.chromium.org/1823203002/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceInflater.java:156:
if (didViewSizeChange) {
On 2016/03/25 18:27:00, newt wrote:
> If the view is already attached (mIsAttached is true), then we still want to
> update the layout params, right? Also, the first time the view is attached, we
> should update the layout params since they won't be correct to begin with,
> right?
> 
> In that case, this should be:
> 
>     if (!mIsAttached && shouldAttachView()) {
>         attachView();
>         didViewSizeChange = true;
>     }
> 
>     if (mIsAttached && didViewSizeChange) {
>         updateLayoutParams();
>     }

Good catch. Yes, if the view is already attached, we need to updated the layout
params too.

Regarding the first time the view is attached, I don't think we need to update
the layout params. The subclasses need to manually inflate and invalidate the
View before rendering it, which means they should be able to tell whether to
override the layout params there, or to let the default values specified in the
xml file be used.

[newt (away), 2016-03-26 00:16:14]

Sounds good. lgtm :)

[pedro (no code reviews), 2016-03-28 18:18:59]

The CQ bit was checked by pedrosimonetti@chromium.org

[commit-bot: I haz the power, 2016-03-28 18:19:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1823203002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1823203002/80001

[commit-bot: I haz the power, 2016-03-28 19:32:46]

Description was changed from

==========
[Contextual Search] Fixing ViewResourceInflater layout invalidation.

This CL changes the ViewResourceInflater class to account for
changes in the layout.

This change is necessary to fix OverlayPanelInflater instances
not being properly updates upon rotation, which will be
addressed in a followup CL.

BUG=596978
==========

to

==========
[Contextual Search] Fixing ViewResourceInflater layout invalidation.

This CL changes the ViewResourceInflater class to account for
changes in the layout.

This change is necessary to fix OverlayPanelInflater instances
not being properly updates upon rotation, which will be
addressed in a followup CL.

BUG=596978
==========

[commit-bot: I haz the power, 2016-03-28 19:32:47]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-28 19:34:33]

Description was changed from

==========
[Contextual Search] Fixing ViewResourceInflater layout invalidation.

This CL changes the ViewResourceInflater class to account for
changes in the layout.

This change is necessary to fix OverlayPanelInflater instances
not being properly updates upon rotation, which will be
addressed in a followup CL.

BUG=596978
==========

to

==========
[Contextual Search] Fixing ViewResourceInflater layout invalidation.

This CL changes the ViewResourceInflater class to account for
changes in the layout.

This change is necessary to fix OverlayPanelInflater instances
not being properly updates upon rotation, which will be
addressed in a followup CL.

BUG=596978

Committed: https://crrev.com/ce771b386aa324fee29fd2534487e331ff421ff4
Cr-Commit-Position: refs/heads/master@{#383540}
==========

[commit-bot: I haz the power, 2016-03-28 19:34:34]

Patchset 5 (id:??) landed as
https://crrev.com/ce771b386aa324fee29fd2534487e331ff421ff4
Cr-Commit-Position: refs/heads/master@{#383540}