Navigation transitions (web to native app): Get names and rects of transition elements (Chrome side)

Navigation transitions (web to native app): Get names and rects of transition elements (Chrome side)

Web to native app navigation transition uses Activity Transitions APIs in Android L. It requires the names and rects of transition elements. This CL gets the names and rects and pass them to TransitionRequestManager.

This is the Chrome side of the CL. Blink side will be done after this CL is landed. https://codereview.chromium.org/654953002/

Design doc: https://docs.google.com/a/chromium.org/document/d/17jg1RRL3RI969cLwbKBIcoGDsPwqaEdBxafGNYGwiY4/edit#
Demo video: https://drive.google.com/a/google.com/file/d/0B3hetueIc91Gd01DU25uT2hWU2M/view?usp=sharing
Activity Transitions in Android L: https://developer.android.com/preview/material/animations.html#transitions

BUG=370696
Committed: https://crrev.com/70986bd4cc0712a132247adcd17ad5fca9cb9bd8
Cr-Commit-Position: refs/heads/master@{#300304}

[Zhen Wang, 2014-10-14 22:37:56]

zhenw@chromium.org changed reviewers:
+ nasko@chromium.org

[Zhen Wang, 2014-10-14 22:37:57]

ptal

[nasko, 2014-10-14 23:09:52]

Let's start by addressing the main issue with the IPC change. I'll look through
the rest of the code tomorrow.

https://codereview.chromium.org/652283002/diff/40001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/652283002/diff/40001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2443:
data.push_back(allowed_destination_host_pattern.utf8());
Do not collapse top-level parameters into a vector. They should remain separate
parameters. If you are exceeding the number of parameters in the IPC macros, use
a struct and describe that struct. There are plenty of examples to follow.

[Zhen Wang, 2014-10-15 18:22:12]

Use struct as IPC params now. ptal.

https://codereview.chromium.org/652283002/diff/40001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/652283002/diff/40001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2443:
data.push_back(allowed_destination_host_pattern.utf8());
On 2014/10/14 23:09:51, nasko wrote:
> Do not collapse top-level parameters into a vector. They should remain
separate
> parameters. If you are exceeding the number of parameters in the IPC macros,
use
> a struct and describe that struct. There are plenty of examples to follow.

Done.

[Zhen Wang, 2014-10-17 18:22:51]

Ping

[nasko, 2014-10-17 20:04:19]

Few comments. Can you also point me to where the allowed hosts pattern is
actually enforced/checked?

https://codereview.chromium.org/652283002/diff/60001/content/browser/transiti...
File content/browser/transition_request_manager.cc (right):

https://codereview.chromium.org/652283002/diff/60001/content/browser/transiti...
content/browser/transition_request_manager.cc:201: std::vector<gfx::Rect>()); /*
rects */
It seems strange to me to hardcode those in this method instead of passing it
from the test.

https://codereview.chromium.org/652283002/diff/60001/content/browser/transiti...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/652283002/diff/60001/content/browser/transiti...
content/browser/transition_request_manager.h:114: ~AllowedEntry();
If there is a destructor, this is no longer a simple struct. Since the
destructor you've provided is empty, why is it even needed?

https://codereview.chromium.org/652283002/diff/60001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/652283002/diff/60001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2449: Send(new
FrameHostMsg_AddNavigationTransitionData(params));
There shouldn't be two methods that send IPCs, just call the other method with
the default params.

[Zhen Wang, 2014-10-17 21:34:29]

> Few comments. Can you also point me to where the allowed hosts pattern is
> actually enforced/checked?

As I remember, this is not enforced yet. oysteine@ plans to fix it.

https://codereview.chromium.org/652283002/diff/60001/content/browser/transiti...
File content/browser/transition_request_manager.cc (right):

https://codereview.chromium.org/652283002/diff/60001/content/browser/transiti...
content/browser/transition_request_manager.cc:201: std::vector<gfx::Rect>()); /*
rects */
This is because it is used in
WebContentsAndroid::SetHasPendingNavigationTransitionForTesting

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...

https://codereview.chromium.org/652283002/diff/60001/content/browser/transiti...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/652283002/diff/60001/content/browser/transiti...
content/browser/transition_request_manager.h:114: ~AllowedEntry();
It will complain without the destructor.

http://www.chromium.org/developers/coding-style/chromium-style-checker-errors...

https://codereview.chromium.org/652283002/diff/60001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/652283002/diff/60001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2449: Send(new
FrameHostMsg_AddNavigationTransitionData(params));
Are we ok to use default params? I heard somewhere else that we prefer not to
use default params.

Besides, this is a three-way patch involving both Chrome side and Blink side
changes as indicated in the CL description. At current stage, only this function
will Send IPC. After the blink side is done, only the below function will Send
IPC. And I will make a final CL to remove this function after both Chrome side
and Blink side are done. So at any time point, only one method will send IPC in
reality.

[nasko, 2014-10-20 14:14:18]

nasko@chromium.org changed reviewers:
+ oysteine@chromium.org

[nasko, 2014-10-20 14:14:19]

I'm really concerned that we are shipping code to users that exposes them to
potential attacks. I know it is not part of this CL, but adding oysteine@ to
ensure this happens very soon or we disable completely the code behind a command
line switch.

As far as this CL, the only thing that stands out is hardcoding values for
testing, which are for the host pattern matching. If oysteine@ has a plan on how
to change those, I'm ok with it.

https://codereview.chromium.org/652283002/diff/60001/content/browser/transiti...
File content/browser/transition_request_manager.cc (right):

https://codereview.chromium.org/652283002/diff/60001/content/browser/transiti...
content/browser/transition_request_manager.cc:201: std::vector<gfx::Rect>()); /*
rects */
On 2014/10/17 21:34:29, Zhen Wang wrote:
> This is because it is used in
> WebContentsAndroid::SetHasPendingNavigationTransitionForTesting
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...

How would  you test non "*" pattern then?

https://codereview.chromium.org/652283002/diff/60001/content/browser/transiti...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/652283002/diff/60001/content/browser/transiti...
content/browser/transition_request_manager.h:114: ~AllowedEntry();
On 2014/10/17 21:34:29, Zhen Wang wrote:
> It will complain without the destructor.
> 
>
http://www.chromium.org/developers/coding-style/chromium-style-checker-errors...

Acknowledged.

https://codereview.chromium.org/652283002/diff/60001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/652283002/diff/60001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2449: Send(new
FrameHostMsg_AddNavigationTransitionData(params));
On 2014/10/17 21:34:29, Zhen Wang wrote:
> Are we ok to use default params? I heard somewhere else that we prefer not to
> use default params.
> 
> Besides, this is a three-way patch involving both Chrome side and Blink side
> changes as indicated in the CL description. At current stage, only this
function
> will Send IPC. After the blink side is done, only the below function will Send
> IPC. And I will make a final CL to remove this function after both Chrome side
> and Blink side are done. So at any time point, only one method will send IPC
in
> reality.

The pattern is to have one method that sends the IPC and all other methods call
it, supplying whatever values make sense for the extra parameters. In any case,
if you are removing the method immediately after, it is fine to leave it as is.

[oystein (OOO til 10th of July), 2014-10-20 14:54:12]

On 2014/10/20 14:14:19, nasko wrote:
> I'm really concerned that we are shipping code to users that exposes them to
> potential attacks. I know it is not part of this CL, but adding oysteine@ to
> ensure this happens very soon or we disable completely the code behind a
command
> line switch.
> 
> As far as this CL, the only thing that stands out is hardcoding values for
> testing, which are for the host pattern matching. If oysteine@ has a plan on
how
> to change those, I'm ok with it.

Right now all of Navigation Transitions should be behind the
experimental-web-platform-features switch. Zhen, can you doublecheck that this
is also enforced for web-to-native?

The plan is to add the host name checking in this quarter. Maybe it would be
safer to hardcode more narrow-scope hostnames, for the tests? i.e. "localhost".


> 
>
https://codereview.chromium.org/652283002/diff/60001/content/browser/transiti...
> File content/browser/transition_request_manager.cc (right):
> 
>
https://codereview.chromium.org/652283002/diff/60001/content/browser/transiti...
> content/browser/transition_request_manager.cc:201: std::vector<gfx::Rect>());
/*
> rects */
> On 2014/10/17 21:34:29, Zhen Wang wrote:
> > This is because it is used in
> > WebContentsAndroid::SetHasPendingNavigationTransitionForTesting
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...
> 
> How would  you test non "*" pattern then?
> 
>
https://codereview.chromium.org/652283002/diff/60001/content/browser/transiti...
> File content/browser/transition_request_manager.h (right):
> 
>
https://codereview.chromium.org/652283002/diff/60001/content/browser/transiti...
> content/browser/transition_request_manager.h:114: ~AllowedEntry();
> On 2014/10/17 21:34:29, Zhen Wang wrote:
> > It will complain without the destructor.
> > 
> >
>
http://www.chromium.org/developers/coding-style/chromium-style-checker-errors...
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/652283002/diff/60001/content/renderer/render_...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/652283002/diff/60001/content/renderer/render_...
> content/renderer/render_frame_impl.cc:2449: Send(new
> FrameHostMsg_AddNavigationTransitionData(params));
> On 2014/10/17 21:34:29, Zhen Wang wrote:
> > Are we ok to use default params? I heard somewhere else that we prefer not
to
> > use default params.
> > 
> > Besides, this is a three-way patch involving both Chrome side and Blink side
> > changes as indicated in the CL description. At current stage, only this
> function
> > will Send IPC. After the blink side is done, only the below function will
Send
> > IPC. And I will make a final CL to remove this function after both Chrome
side
> > and Blink side are done. So at any time point, only one method will send IPC
> in
> > reality.
> 
> The pattern is to have one method that sends the IPC and all other methods
call
> it, supplying whatever values make sense for the extra parameters. In any
case,
> if you are removing the method immediately after, it is fine to leave it as
is.

[nasko, 2014-10-20 15:44:47]

On Mon, Oct 20, 2014 at 7:54 AM, <oysteine@chromium.org> wrote:

> On 2014/10/20 14:14:19, nasko wrote:
>
>> I'm really concerned that we are shipping code to users that exposes them
>> to
>> potential attacks. I know it is not part of this CL, but adding oysteine@
>> to
>> ensure this happens very soon or we disable completely the code behind a
>>
> command
>
>> line switch.
>>
>
>  As far as this CL, the only thing that stands out is hardcoding values for
>> testing, which are for the host pattern matching. If oysteine@ has a
>> plan on
>>
> how
>
>> to change those, I'm ok with it.
>>
>
> Right now all of Navigation Transitions should be behind the
> experimental-web-platform-features switch. Zhen, can you doublecheck that
> this
> is also enforced for web-to-native?
>
> The plan is to add the host name checking in this quarter. Maybe it would
> be
> safer to hardcode more narrow-scope hostnames, for the tests? i.e.
> "localhost".


I don't see how putting "localhost" will make any difference at all if it
is not even looked at. We've discussed in the past not shipping this to
users before implementing the pattern matching and I see this is now not
the case :(. Yes, it is behind the experimental features switch, but there
are lots of devs that run with that flag in order to play around with our
work. We shouldn't put them at risk by delaying adding checks.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nasko, 2014-10-20 15:47:45]

This CL isn't affected by the discussion that started. LGTM, though let's move
to email discussion on when we will add pattern matching.

[Zhen Wang, 2014-10-20 18:18:34]

The CQ bit was checked by zhenw@chromium.org

[commit-bot: I haz the power, 2014-10-20 18:20:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/652283002/60001

[commit-bot: I haz the power, 2014-10-20 18:50:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-20 18:51:20]

Patchset 4 (id:??) landed as
https://crrev.com/70986bd4cc0712a132247adcd17ad5fca9cb9bd8
Cr-Commit-Position: refs/heads/master@{#300304}