[ash-md] Adds support for Z-order iteration in views::View

[ash-md] Adds support for Z-order iteration in views::View

This CL is a demonstration of an alternative to
https://codereview.chromium.org/2557333003/ . This alternative shows how
we could add a custom iterator over children to views::View.

Benefits:
 - Avoids need to override all of those:
   View::GetTooltipHandlerForPoint
   View::PaintChildren
   View::ReorderChildLayers
   ViewTargeterDelegate::TargetForRect
 just for the sake of implementing Z-order. We may still need to override
 them to add custom painting and such but not to mess with the Z-order.
 - Keeps the iterations in views::View base class implementation

BUG=664244
Committed: https://crrev.com/321f8fbf1e90199a9d8bbbe14f5a99db92e85246
Cr-Commit-Position: refs/heads/master@{#441247}

[varkha, 2016-12-09 00:48:32]

varkha@chromium.org changed reviewers:
+ sadrul@chromium.org

[varkha, 2016-12-09 00:48:33]

sadrul@, can you comment on this first, I can send it to sky then for more
insights. Thanks!

[varkha, 2016-12-09 18:57:26]

varkha@chromium.org changed reviewers:
+ sky@chromium.org

[varkha, 2016-12-09 18:57:26]

+sky for feedback on the idea.

[sky, 2016-12-09 20:48:00]

I would prefer not to add more api to View that complicates an already confusing
class. Could we instead have the classes that are customizing paint order
reorder the views? This may be more involved, but that would simplify view API.

[varkha, 2016-12-10 00:20:55]

On 2016/12/09 20:48:00, sky wrote:
> I would prefer not to add more api to View that complicates an already
confusing
> class. Could we instead have the classes that are customizing paint order
> reorder the views? This may be more involved, but that would simplify view
API.

The problem with that approach is that it would modify things like tab order
focus iteration. It can be fixed of course but I'm not sure if that is any
better.

[sky, 2016-12-12 02:02:44]

Good point. What are the set of views that customize paint order of
children? I know TabStrip does, but none of TabStrips views are
focusable, so it wouldn't impact the tab order.

  -Scott

On Fri, Dec 9, 2016 at 4:20 PM,  <varkha@chromium.org> wrote:
> On 2016/12/09 20:48:00, sky wrote:
>> I would prefer not to add more api to View that complicates an already
> confusing
>> class. Could we instead have the classes that are customizing paint order
>> reorder the views? This may be more involved, but that would simplify view
> API.
>
> The problem with that approach is that it would modify things like tab order
> focus iteration. It can be fixed of course but I'm not sure if that is any
> better.
>
> https://codereview.chromium.org/2561253002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[varkha, 2016-12-12 04:00:53]

On 2016/12/12 02:02:44, sky wrote:
> Good point. What are the set of views that customize paint order of
> children? I know TabStrip does, but none of TabStrips views are
> focusable, so it wouldn't impact the tab order.
> 
>   -Scott
> 
> On Fri, Dec 9, 2016 at 4:20 PM,  <mailto:varkha@chromium.org> wrote:
> > On 2016/12/09 20:48:00, sky wrote:
> >> I would prefer not to add more api to View that complicates an already
> > confusing
> >> class. Could we instead have the classes that are customizing paint order
> >> reorder the views? This may be more involved, but that would simplify view
> > API.
> >
> > The problem with that approach is that it would modify things like tab order
> > focus iteration. It can be fixed of course but I'm not sure if that is any
> > better.
> >
> > https://codereview.chromium.org/2561253002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

I was going to use it instead of what's used now for sticky scrollable section
headers such as for network types in system menu's network page.

[sadrul, 2016-12-12 15:51:04]

I think the list under 'improvements' are more of a 'requirement', as in, once
we have the new iteration order, we need to make it impossible to iterate over
the list of children incorrectly. Otherwise, custom views will use the wrong
iterator, and introduce very hard-to-find bugs.

[sky, 2016-12-12 16:12:38]

On Sun, Dec 11, 2016 at 8:00 PM,  <varkha@chromium.org> wrote:
> On 2016/12/12 02:02:44, sky wrote:
>> Good point. What are the set of views that customize paint order of
>> children? I know TabStrip does, but none of TabStrips views are
>> focusable, so it wouldn't impact the tab order.
>>
>> -Scott
>>
>> On Fri, Dec 9, 2016 at 4:20 PM, <mailto:varkha@chromium.org> wrote:
>> > On 2016/12/09 20:48:00, sky wrote:
>> >> I would prefer not to add more api to View that complicates an already
>> > confusing
>> >> class. Could we instead have the classes that are customizing paint
>> >> order
>> >> reorder the views? This may be more involved, but that would simplify
>> >> view
>> > API.
>> >
>> > The problem with that approach is that it would modify things like tab
>> > order
>> > focus iteration. It can be fixed of course but I'm not sure if that is
>> > any
>> > better.
>> >
>> > https://codereview.chromium.org/2561253002/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
> email
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> I was going to use it instead of what's used now for sticky scrollable
> section
> headers such as for network types in system menu's network page.

Are any of those items focusable?

>
> https://codereview.chromium.org/2561253002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[varkha, 2016-12-12 17:18:20]

On 2016/12/12 16:12:38, sky wrote:
> On Sun, Dec 11, 2016 at 8:00 PM,  <mailto:varkha@chromium.org> wrote:
> > On 2016/12/12 02:02:44, sky wrote:
> >> Good point. What are the set of views that customize paint order of
> >> children? I know TabStrip does, but none of TabStrips views are
> >> focusable, so it wouldn't impact the tab order.
> >>
> >> -Scott
> >>
> >> On Fri, Dec 9, 2016 at 4:20 PM, <mailto:varkha@chromium.org> wrote:
> >> > On 2016/12/09 20:48:00, sky wrote:
> >> >> I would prefer not to add more api to View that complicates an already
> >> > confusing
> >> >> class. Could we instead have the classes that are customizing paint
> >> >> order
> >> >> reorder the views? This may be more involved, but that would simplify
> >> >> view
> >> > API.
> >> >
> >> > The problem with that approach is that it would modify things like tab
> >> > order
> >> > focus iteration. It can be fixed of course but I'm not sure if that is
> >> > any
> >> > better.
> >> >
> >> > https://codereview.chromium.org/2561253002/
> >>
> >> --
> >> You received this message because you are subscribed to the Google Groups
> >> "Chromium-reviews" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an
> > email
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> > I was going to use it instead of what's used now for sticky scrollable
> > section
> > headers such as for network types in system menu's network page.
> 
> Are any of those items focusable?
> 
> >
> > https://codereview.chromium.org/2561253002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

Yes, the rows in the system menu (or their children) are focusable with TAB.

[varkha, 2016-12-12 17:19:08]

On 2016/12/12 15:51:04, sadrul wrote:
> I think the list under 'improvements' are more of a 'requirement', as in, once
> we have the new iteration order, we need to make it impossible to iterate over
> the list of children incorrectly. Otherwise, custom views will use the wrong
> iterator, and introduce very hard-to-find bugs.

Yes, I agree. I've posted this to have something to discuss the general approach
with.

[sky, 2016-12-12 17:57:51]

On 2016/12/12 17:18:20, varkha wrote:
> On 2016/12/12 16:12:38, sky wrote:
> > On Sun, Dec 11, 2016 at 8:00 PM,  <mailto:varkha@chromium.org> wrote:
> > > On 2016/12/12 02:02:44, sky wrote:
> > >> Good point. What are the set of views that customize paint order of
> > >> children? I know TabStrip does, but none of TabStrips views are
> > >> focusable, so it wouldn't impact the tab order.
> > >>
> > >> -Scott
> > >>
> > >> On Fri, Dec 9, 2016 at 4:20 PM, <mailto:varkha@chromium.org> wrote:
> > >> > On 2016/12/09 20:48:00, sky wrote:
> > >> >> I would prefer not to add more api to View that complicates an already
> > >> > confusing
> > >> >> class. Could we instead have the classes that are customizing paint
> > >> >> order
> > >> >> reorder the views? This may be more involved, but that would simplify
> > >> >> view
> > >> > API.
> > >> >
> > >> > The problem with that approach is that it would modify things like tab
> > >> > order
> > >> > focus iteration. It can be fixed of course but I'm not sure if that is
> > >> > any
> > >> > better.
> > >> >
> > >> > https://codereview.chromium.org/2561253002/
> > >>
> > >> --
> > >> You received this message because you are subscribed to the Google Groups
> > >> "Chromium-reviews" group.
> > >> To unsubscribe from this group and stop receiving emails from it, send an
> > > email
> > >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> > >>
> > >
> > > I was going to use it instead of what's used now for sticky scrollable
> > > section
> > > headers such as for network types in system menu's network page.
> > 
> > Are any of those items focusable?
> > 
> > >
> > > https://codereview.chromium.org/2561253002/
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> 
> Yes, the rows in the system menu (or their children) are focusable with TAB.

Bummer.

I don't think you can make the new one an iterator as it's not necessarily easy
to compute. In particular I think the View function should be something like:

std::vector<View*> GetChildrenOrderedByVisualOrder();

But even this isn't enough for layer ordering as this function alone doesn't
convey when the z-order changes so that layers can be reordered. I tend to favor
a more focused fix for scrollview without introducing additional complexity in
View.

[sadrul, 2016-12-12 18:38:38]

What about a focus-focused API, e.g. setting something equivalent to tab-index
in views.

[varkha, 2016-12-12 19:03:15]

> Bummer.
> 
> I don't think you can make the new one an iterator as it's not necessarily
easy
> to compute. In particular I think the View function should be something like:
> 
> std::vector<View*> GetChildrenOrderedByVisualOrder();
> 
> But even this isn't enough for layer ordering as this function alone doesn't
> convey when the z-order changes so that layers can be reordered. I tend to
favor
> a more focused fix for scrollview without introducing additional complexity in
> View.

A system menu bubbles fix for scrollview with sticky headers is here -
https://codereview.chromium.org/2557333003/

[varkha, 2016-12-12 22:39:20]

On 2016/12/12 18:38:38, sadrul wrote:
> What about a focus-focused API, e.g. setting something equivalent to tab-index
> in views.

There's View::SetNextFocusableView() that could be used but I think this would
considerably complicate headers imlementation because the headers would need to
be placed after their group in parent's |children_|. This was the initial
approach in [1] but what we have now is simpler (and would have been simpler yet
with the z-order following an iterator.
But yes, the cost of creating an iterator in general case could be higher so
the general case this may not work.

[1] - https://codereview.chromium.org/2453133002

> On 2016/12/12 17:57:51, sky wrote:
> In particular I think the View function should be something like:
>
> std::vector<View*> GetChildrenOrderedByVisualOrder();
>
> But even this isn't enough for layer ordering as this function alone doesn't
> convey when the z-order changes so that layers can be reordered.

What if in view.cc the GetChildrenOrderedByVisualOrder() would just return
|children_| and we could override it? Then we could call that in
   View::GetTooltipHandlerForPoint
   View::PaintChildren
   View::ReorderChildLayers
   ViewTargeterDelegate::TargetForRect
and iterate over the returned array instead of iterating over |children_|.
A child derived from View could update its internal |children_in_visual_order_|
member in its override of ReorderChildLayers() *before* calling base
implementation of View::ReorderChildLayers().

A more explicit would be if View::ReorderChildLayers() would call a virtual
this->UpdateChildrenVisualOrder() to get the order updated.

I can try putting together a patch to see if either of those approaches works.
WDYT?

[varkha, 2016-12-12 23:15:13]

Posted an exploration using View::GetChildrenOrderedByVisualOrder().

[sky, 2016-12-13 01:49:36]

This sounds messy, but I could certainly be missing something. You
should make sure what you are proposing works with TabStrip.

  -Scott

On Mon, Dec 12, 2016 at 2:39 PM,  <varkha@chromium.org> wrote:
> On 2016/12/12 18:38:38, sadrul wrote:
>> What about a focus-focused API, e.g. setting something equivalent to
>> tab-index
>> in views.
>
> There's View::SetNextFocusableView() that could be used but I think this
> would
> considerably complicate headers imlementation because the headers would need
> to
> be placed after their group in parent's |children_|. This was the initial
> approach in [1] but what we have now is simpler (and would have been simpler
> yet
> with the z-order following an iterator.
> But yes, the cost of creating an iterator in general case could be higher so
> the general case this may not work.
>
> [1] - https://codereview.chromium.org/2453133002
>
>> On 2016/12/12 17:57:51, sky wrote:
>> In particular I think the View function should be something like:
>>
>> std::vector<View*> GetChildrenOrderedByVisualOrder();
>>
>> But even this isn't enough for layer ordering as this function alone
>> doesn't
>> convey when the z-order changes so that layers can be reordered.
>
> What if in view.cc the GetChildrenOrderedByVisualOrder() would just return
> |children_| and we could override it? Then we could call that in
> View::GetTooltipHandlerForPoint
> View::PaintChildren
> View::ReorderChildLayers
> ViewTargeterDelegate::TargetForRect
> and iterate over the returned array instead of iterating over |children_|.
> A child derived from View could update its internal
> |children_in_visual_order_|
> member in its override of ReorderChildLayers() *before* calling base
> implementation of View::ReorderChildLayers().
>
> A more explicit would be if View::ReorderChildLayers() would call a virtual
> this->UpdateChildrenVisualOrder() to get the order updated.
>
> I can try putting together a patch to see if either of those approaches
> works.
> WDYT?
>
> https://codereview.chromium.org/2561253002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[varkha, 2016-12-13 16:53:15]

On 2016/12/13 01:49:36, sky wrote:
> This sounds messy, but I could certainly be missing something. You
> should make sure what you are proposing works with TabStrip.
> 
>   -Scott

Well what I have in the last patch should not change anything unless
you actually override GetChildrenOrderedByVisualOrder and nothing
currently does other than the scroll contents in CrOS system UI. So
I don't expect anything to change for the TabStrip. It also should
not be regressing performance-wise because there are no additional
iterations added (thanks to your suggestion).

[sky, 2016-12-13 18:06:42]

Do you have a patch that fixes only scrollview (doesn't change view)?

  -Scott

On Tue, Dec 13, 2016 at 8:53 AM,  <varkha@chromium.org> wrote:
> On 2016/12/13 01:49:36, sky wrote:
>> This sounds messy, but I could certainly be missing something. You
>> should make sure what you are proposing works with TabStrip.
>>
>> -Scott
>
> Well what I have in the last patch should not change anything unless
> you actually override GetChildrenOrderedByVisualOrder and nothing
> currently does other than the scroll contents in CrOS system UI. So
> I don't expect anything to change for the TabStrip. It also should
> not be regressing performance-wise because there are no additional
> iterations added (thanks to your suggestion).
>
> https://codereview.chromium.org/2561253002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[varkha, 2016-12-13 22:22:29]

On 2016/12/13 18:06:42, sky wrote:
> Do you have a patch that fixes only scrollview (doesn't change view)?
> 
>   -Scott

Yes, that's the link at the top of the description:
https://codereview.chromium.org/2557333003/

It only affects the system menu (ScrollContentsView in TrayDetailsView).

[sky, 2016-12-13 23:40:56]

I would prefer not to complicate view with additional api, but I agree it's far
simpler in this case. Please add test coverage of changes to view.

https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:1448: for (int i = 0, count = children.size(); i < count; ++i)
{
Use range based for-loop?

https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:1603: for (int i = children.size() - 1; i >= 0; --i)
I think a reverse iterator is safer here.

https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.h#newcode193
ui/views/view.h:193: int child_count() const { return
static_cast<int>(children_.size()); }
Please add a comment here about GetChildrenOrderedByVisualOrder().

https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.h#newcode310
ui/views/view.h:310: // Get a container of |children_| arranged according to the
stacking order.
How about:
Returns the child views ordered in reverse z-order. That is, views later in the
returned vector have a higher z-order (are painted later) than those early in
the vector. The returned vector has exactly the same number of Views as
children(). The default implementation returns children(), subclass if the paint
order should differ from that of children().

I'm also wondering if it should be named GetChildrenOrderedByZOrder()? But I'm
ok with what you have.

Can this be protected?

[varkha, 2016-12-14 17:51:38]

https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.h#newcode310
ui/views/view.h:310: // Get a container of |children_| arranged according to the
stacking order.
On 2016/12/13 23:40:56, sky wrote:
> How about:
> Returns the child views ordered in reverse z-order. That is, views later in
the
> returned vector have a higher z-order (are painted later) than those early in
> the vector. The returned vector has exactly the same number of Views as
> children(). The default implementation returns children(), subclass if the
paint
> order should differ from that of children().
> 
> I'm also wondering if it should be named GetChildrenOrderedByZOrder()? But I'm
> ok with what you have.
> 
> Can this be protected?

I will look into the rest of the comments but making it protected would require
friending ViewTargeterDelegate::TargetForRect, no?

[sky, 2016-12-14 18:33:24]

*SIGH* I guess there are reasons to keep the function public.

On Wed, Dec 14, 2016 at 9:51 AM,  <varkha@chromium.org> wrote:
>
> https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.h
> File ui/views/view.h (right):
>
>
https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.h#newcode310
> ui/views/view.h:310: // Get a container of |children_| arranged
> according to the stacking order.
> On 2016/12/13 23:40:56, sky wrote:
>> How about:
>> Returns the child views ordered in reverse z-order. That is, views
> later in the
>> returned vector have a higher z-order (are painted later) than those
> early in
>> the vector. The returned vector has exactly the same number of Views
> as
>> children(). The default implementation returns children(), subclass if
> the paint
>> order should differ from that of children().
>>
>> I'm also wondering if it should be named GetChildrenOrderedByZOrder()?
> But I'm
>> ok with what you have.
>>
>> Can this be protected?
>
> I will look into the rest of the comments but making it protected would
> require friending ViewTargeterDelegate::TargetForRect, no?
>
> https://codereview.chromium.org/2561253002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[varkha, 2016-12-19 20:35:44]

Added a test. PTAL.

https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:1448: for (int i = 0, count = children.size(); i < count; ++i)
{
On 2016/12/13 23:40:56, sky (OOO) wrote:
> Use range based for-loop?

Done. I will try to get the rest of the similar iterations in view.cc and
view_targeter_delegate.cc to be like this as well (in a separate CL).

https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:1603: for (int i = children.size() - 1; i >= 0; --i)
On 2016/12/13 23:40:56, sky (OOO) wrote:
> I think a reverse iterator is safer here.

Done.

https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.h#newcode193
ui/views/view.h:193: int child_count() const { return
static_cast<int>(children_.size()); }
On 2016/12/13 23:40:56, sky (OOO) wrote:
> Please add a comment here about GetChildrenOrderedByVisualOrder().

Done.

https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.h#newcode310
ui/views/view.h:310: // Get a container of |children_| arranged according to the
stacking order.
On 2016/12/13 23:40:56, sky (OOO) wrote:
> How about:
> Returns the child views ordered in reverse z-order. That is, views later in
the
> returned vector have a higher z-order (are painted later) than those early in
> the vector. The returned vector has exactly the same number of Views as
> children(). The default implementation returns children(), subclass if the
paint
> order should differ from that of children().
> 

Done (edited the comment).

[varkha, 2016-12-19 20:36:56]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 20:37:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[varkha, 2016-12-19 20:56:16]

https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:1448: for (int i = 0, count = children.size(); i < count; ++i)
{
On 2016/12/13 23:40:56, sky (OOO) wrote:
> Use range based for-loop?

Posted https://codereview.chromium.org/2583343003 for the rest of the similar
iterations in this file.

[commit-bot: I haz the power, 2016-12-19 21:43:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-19 21:43:15]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[varkha, 2016-12-19 22:04:06]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 22:04:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 23:26:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-19 23:26:22]

Dry run: This issue passed the CQ dry run.

[varkha, 2016-12-23 00:43:33]

Patchset #8 (id:140001) has been deleted

[varkha, 2016-12-23 00:46:25]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 00:46:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[varkha, 2016-12-23 00:47:30]

Description was changed from

==========
[ash-md] Adds support for Z-order iteration in views::View

This CL is a demonstration of an alternative to
https://codereview.chromium.org/2557333003/ . This alternative shows how
we could add a custom iterator over children to views::View.

Benefits:
 - Avoids need to override all of those:
   View::GetTooltipHandlerForPoint
   View::PaintChildren
   View::ReorderChildLayers
   ViewTargeterDelegate::TargetForRect
 just for the sake of implementing Z-order. We may still need to override
 them to add custom painting and such but not to mess with the Z-order.

 - Keeps the iterations in views::View base class implementation

Improvements:
 - Turn that into a real iterator, this would have better performance
   because it would be only constructed once for any iteration sequence.
 - Avoid possibility of using a wrong iterator by doing something like
   this (pseudocode):
     virtual iterator* View::GetChildIterator(enum Order {FOCUS | STACKING})
     default implementation returns children::iterator.
     A subclass could create a different ordering and return an iterator
     over that ordering, thus changing FOCUS ordering or STACKING ordering.
 - Change all call sites where we use child_at(i) to use the new iterator.
 - retire child_at().

BUG=664244
==========

to

==========
[ash-md] Adds support for Z-order iteration in views::View

This CL is a demonstration of an alternative to
https://codereview.chromium.org/2557333003/ . This alternative shows how
we could add a custom iterator over children to views::View.

Benefits:
 - Avoids need to override all of those:
   View::GetTooltipHandlerForPoint
   View::PaintChildren
   View::ReorderChildLayers
   ViewTargeterDelegate::TargetForRect
 just for the sake of implementing Z-order. We may still need to override
 them to add custom painting and such but not to mess with the Z-order.
 - Keeps the iterations in views::View base class implementation

BUG=664244
==========

[varkha, 2016-12-23 00:48:23]

Now that the refactoring in the View iterators has landed this CL is a bit
simplified. PTAL when you have a moment. Thanks!

[commit-bot: I haz the power, 2016-12-23 02:35:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-23 02:35:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[varkha, 2017-01-03 21:12:51]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 21:13:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-01-03 21:46:36]

LGTM

https://codereview.chromium.org/2561253002/diff/180001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/2561253002/diff/180001/ui/views/view_unittest...
ui/views/view_unittest.cc:4675: static const int VIEW_ID_RAISED = 1000;
constexpr and a description.

[varkha, 2017-01-03 22:21:21]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[varkha, 2017-01-03 22:21:35]

https://codereview.chromium.org/2561253002/diff/180001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/2561253002/diff/180001/ui/views/view_unittest...
ui/views/view_unittest.cc:4675: static const int VIEW_ID_RAISED = 1000;
On 2017/01/03 21:46:36, sky (OOO) wrote:
> constexpr and a description.

Done.

[commit-bot: I haz the power, 2017-01-03 22:21:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-03 23:15:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-03 23:15:28]

Dry run: This issue passed the CQ dry run.

[varkha, 2017-01-03 23:16:03]

The CQ bit was checked by varkha@chromium.org

[varkha, 2017-01-03 23:16:04]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2561253002/#ps200001
(title: "[ash-md] Adds support for Z-order iteration in views::View (nit)")

[commit-bot: I haz the power, 2017-01-03 23:16:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-03 23:21:52]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1483485363946140,
"parent_rev": "12671b192c4e22ddeddc840dfe5f066bd5b01525", "commit_rev":
"7228b9b376db00c737122608bac631be2ab4fd65"}

[commit-bot: I haz the power, 2017-01-03 23:22:23]

Description was changed from

==========
[ash-md] Adds support for Z-order iteration in views::View

This CL is a demonstration of an alternative to
https://codereview.chromium.org/2557333003/ . This alternative shows how
we could add a custom iterator over children to views::View.

Benefits:
 - Avoids need to override all of those:
   View::GetTooltipHandlerForPoint
   View::PaintChildren
   View::ReorderChildLayers
   ViewTargeterDelegate::TargetForRect
 just for the sake of implementing Z-order. We may still need to override
 them to add custom painting and such but not to mess with the Z-order.
 - Keeps the iterations in views::View base class implementation

BUG=664244
==========

to

==========
[ash-md] Adds support for Z-order iteration in views::View

This CL is a demonstration of an alternative to
https://codereview.chromium.org/2557333003/ . This alternative shows how
we could add a custom iterator over children to views::View.

Benefits:
 - Avoids need to override all of those:
   View::GetTooltipHandlerForPoint
   View::PaintChildren
   View::ReorderChildLayers
   ViewTargeterDelegate::TargetForRect
 just for the sake of implementing Z-order. We may still need to override
 them to add custom painting and such but not to mess with the Z-order.
 - Keeps the iterations in views::View base class implementation

BUG=664244

Review-Url: https://codereview.chromium.org/2561253002
==========

[commit-bot: I haz the power, 2017-01-03 23:22:25]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2017-01-03 23:25:26]

Description was changed from

==========
[ash-md] Adds support for Z-order iteration in views::View

This CL is a demonstration of an alternative to
https://codereview.chromium.org/2557333003/ . This alternative shows how
we could add a custom iterator over children to views::View.

Benefits:
 - Avoids need to override all of those:
   View::GetTooltipHandlerForPoint
   View::PaintChildren
   View::ReorderChildLayers
   ViewTargeterDelegate::TargetForRect
 just for the sake of implementing Z-order. We may still need to override
 them to add custom painting and such but not to mess with the Z-order.
 - Keeps the iterations in views::View base class implementation

BUG=664244

Review-Url: https://codereview.chromium.org/2561253002
==========

to

==========
[ash-md] Adds support for Z-order iteration in views::View

This CL is a demonstration of an alternative to
https://codereview.chromium.org/2557333003/ . This alternative shows how
we could add a custom iterator over children to views::View.

Benefits:
 - Avoids need to override all of those:
   View::GetTooltipHandlerForPoint
   View::PaintChildren
   View::ReorderChildLayers
   ViewTargeterDelegate::TargetForRect
 just for the sake of implementing Z-order. We may still need to override
 them to add custom painting and such but not to mess with the Z-order.
 - Keeps the iterations in views::View base class implementation

BUG=664244

Committed: https://crrev.com/321f8fbf1e90199a9d8bbbe14f5a99db92e85246
Cr-Commit-Position: refs/heads/master@{#441247}
==========

[commit-bot: I haz the power, 2017-01-03 23:25:27]

Patchset 10 (id:??) landed as
https://crrev.com/321f8fbf1e90199a9d8bbbe14f5a99db92e85246
Cr-Commit-Position: refs/heads/master@{#441247}