Navigation transitions (web to native app): Pass data after starting provisional load (Chrome side)

Navigation transitions (web to native app): Pass data after starting provisional load (Chrome side)

Pass transitional elements' CSS selector, names and rects to TransitionPageHelper after starting provisional load. Those data are needed when overriding the URL navigation to jump to a native Android app.

The is the Chrome side of the CL. The Clank side is here: https://chrome-internal-review.googlesource.com/#/c/180668/

BUG=370696
Committed: https://crrev.com/5b846f579d3dd1a0f4d0b8c26ff274117bfba0bc
Cr-Commit-Position: refs/heads/master@{#302279}

[Zhen Wang, 2014-10-22 13:55:38]

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

[nasko, 2014-10-22 21:10:04]

Please have someone review the java code, as I'm not familiar with it.

https://codereview.chromium.org/652953008/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/652953008/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:263: url,
This should be validated_url, not the input parameter.

https://codereview.chromium.org/652953008/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_android.h (right):

https://codereview.chromium.org/652953008/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_android.h:64: int64 frame_id, const
TransitionLayerData& transition_data);
nit: params should be each on separate line

[Zhen Wang, 2014-10-22 23:04:39]

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

[Zhen Wang, 2014-10-22 23:04:40]

Hi Daniel, can you take a look at the Java part? Thanks!

https://codereview.chromium.org/652953008/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/652953008/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:263: url,
On 2014/10/22 21:10:04, nasko wrote:
> This should be validated_url, not the input parameter.

Done.

https://codereview.chromium.org/652953008/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_android.h (right):

https://codereview.chromium.org/652953008/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_android.h:64: int64 frame_id, const
TransitionLayerData& transition_data);
On 2014/10/22 21:10:04, nasko wrote:
> nit: params should be each on separate line

Done.

[no sievers, 2014-10-23 22:13:54]

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

[no sievers, 2014-10-23 22:14:13]

+oysteine for review, I can stamp

https://codereview.chromium.org/652953008/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content_public/browser/NavigationTransitionDelegate.java
(right):

https://codereview.chromium.org/652953008/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content_public/browser/NavigationTransitionDelegate.java:41:
*                    or applied to transition layer's makrup later.
nit: makrup/markup...
" The CSS selector, which is to be used by Activity Transitions
  or applied to the transition layer's markup later."

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

https://codereview.chromium.org/652953008/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content_public/browser/NavigationTransitionDelegate.java
(right):

https://codereview.chromium.org/652953008/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content_public/browser/NavigationTransitionDelegate.java:41:
*                    or applied to transition layer's makrup later.
On 2014/10/23 22:14:13, sievers wrote:
> nit: makrup/markup...
> " The CSS selector, which is to be used by Activity Transitions
>   or applied to the transition layer's markup later."

Done.

[oystein (OOO til 10th of July), 2014-10-27 18:45:12]

https://codereview.chromium.org/652953008/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/652953008/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.cc:340: for (; it !=
transition_data.names.end() && jt != transition_data.rects.end();
Could the names+rects be stored together in a vector of structs, rather than two
vectors? It should be cheaper and more readable.

[Zhen Wang, 2014-10-27 23:47:19]

https://codereview.chromium.org/652953008/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/652953008/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.cc:340: for (; it !=
transition_data.names.end() && jt != transition_data.rects.end();
Done.

But I didn't find whether customized struct is available over jni from C++ side
to Java side (jobject seems to be used from Java side to C++ side only).

[oystein (OOO til 10th of July), 2014-10-29 21:37:31]

lgtm

[nasko, 2014-10-29 22:14:25]

LGTM with one nit.

https://codereview.chromium.org/652953008/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/652953008/diff/60001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:260:
TransitionRequestManager::GetInstance()->HasPendingTransitionRequest(
This method name looks very weird. A Has* method should return a boolean and not
the actual data. Renaming to Get* fits more of how it is actually used here.

[nasko, 2014-10-29 22:18:43]

On 2014/10/29 22:14:25, nasko wrote:
> LGTM with one nit.
> 
>
https://codereview.chromium.org/652953008/diff/60001/content/browser/frame_ho...
> File content/browser/frame_host/navigator_impl.cc (right):
> 
>
https://codereview.chromium.org/652953008/diff/60001/content/browser/frame_ho...
> content/browser/frame_host/navigator_impl.cc:260:
> TransitionRequestManager::GetInstance()->HasPendingTransitionRequest(
> This method name looks very weird. A Has* method should return a boolean and
not
> the actual data. Renaming to Get* fits more of how it is actually used here.

Actually, one thing I missed. There should be a hard limit on the size of the
vector coming in as a parameter in render_message_filter.cc. Please add this
before committing.

[Zhen Wang, 2014-10-29 23:11:06]

> Actually, one thing I missed. There should be a hard limit on the size of the
> vector coming in as a parameter in render_message_filter.cc. Please add this
> before committing.

What does the "hard limit" mean and why do we need it? I didn't see any other
example in frame_message.h. Do you mean something like:

IPC_STRUCT_BEGIN(FrameHostMsg_AddNavigationTransitionData_Params)
  IPC_STRUCT_MEMBER(int, render_frame_id)
  IPC_STRUCT_MEMBER(std::string, allowed_destination_host_pattern)
  IPC_STRUCT_MEMBER(std::string, selector)
  IPC_STRUCT_MEMBER(std::string, markup)
  IPC_STRUCT_MEMBER(std::vector<content::TransitionElement>, elements)
  IPC_STRUCT_MEMBER(int, number_of_transition_elements)
IPC_STRUCT_END()

Then put the size of the vector to number_of_transition_elements and send the
IPC. In render_message_filer.cc we check if the size of the vector equals
number_of_transition_elements?

https://codereview.chromium.org/652953008/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/652953008/diff/60001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:260:
TransitionRequestManager::GetInstance()->HasPendingTransitionRequest(
On 2014/10/29 22:14:24, nasko wrote:
> This method name looks very weird. A Has* method should return a boolean and
not
> the actual data. Renaming to Get* fits more of how it is actually used here.

Done.

[nasko, 2014-10-29 23:17:19]

On 2014/10/29 23:11:06, Zhen Wang wrote:
> > Actually, one thing I missed. There should be a hard limit on the size of
the
> > vector coming in as a parameter in render_message_filter.cc. Please add this
> > before committing.
> 
> What does the "hard limit" mean and why do we need it? I didn't see any other
> example in frame_message.h. Do you mean something like:
> 
> IPC_STRUCT_BEGIN(FrameHostMsg_AddNavigationTransitionData_Params)
>   IPC_STRUCT_MEMBER(int, render_frame_id)
>   IPC_STRUCT_MEMBER(std::string, allowed_destination_host_pattern)
>   IPC_STRUCT_MEMBER(std::string, selector)
>   IPC_STRUCT_MEMBER(std::string, markup)
>   IPC_STRUCT_MEMBER(std::vector<content::TransitionElement>, elements)
>   IPC_STRUCT_MEMBER(int, number_of_transition_elements)
> IPC_STRUCT_END()
> 
> Then put the size of the vector to number_of_transition_elements and send the
> IPC. In render_message_filer.cc we check if the size of the vector equals
> number_of_transition_elements?

No, just "if (elements.size() > 2048) just return". We try to avoid passing
arbitrarily big objects across our IPC boundary and have some sane limits on
vector sizes. It doesn't make sense to have 2^32 sized vector of elements, does
it? I picked the 2048 out of thin air, but something that makes sense to you
should be used.


>
https://codereview.chromium.org/652953008/diff/60001/content/browser/frame_ho...
> File content/browser/frame_host/navigator_impl.cc (right):
> 
>
https://codereview.chromium.org/652953008/diff/60001/content/browser/frame_ho...
> content/browser/frame_host/navigator_impl.cc:260:
> TransitionRequestManager::GetInstance()->HasPendingTransitionRequest(
> On 2014/10/29 22:14:24, nasko wrote:
> > This method name looks very weird. A Has* method should return a boolean and
> not
> > the actual data. Renaming to Get* fits more of how it is actually used here.
> 
> Done.

[Zhen Wang, 2014-10-29 23:45:51]

I see. Hard limit added.

[nasko, 2014-10-29 23:48:40]

LGTM

[Zhen Wang, 2014-10-31 15:37:32]

The CQ bit was checked by zhenw@chromium.org

[commit-bot: I haz the power, 2014-10-31 15:38:55]

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

[Zhen Wang, 2014-10-31 15:44:46]

The CQ bit was unchecked by zhenw@chromium.org

[Zhen Wang, 2014-10-31 16:31:25]

The CQ bit was checked by zhenw@chromium.org

[commit-bot: I haz the power, 2014-10-31 16:32:20]

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

[commit-bot: I haz the power, 2014-10-31 16:33:27]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-10-31 16:34:01]

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

[Hajime Morrita, 2014-10-31 18:24:07]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/689123003/ by morrita@chromium.org.

The reason for reverting is: Crashing a LayoutTest

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#master=Ch...

Hitting an assertion:
10:15:12.016 3607  
[10283:10283:1031/101511:5678803780814:FATAL:transition_request_manager.cc(161)]
Check failed: ::content::BrowserThread::CurrentlyOn(BrowserThread::IO). Must be
called on Chrome_IOThread; actually called on CrBrowserMain.
10:15:12.016 3607   #0 0x7f66666664ce base::debug::StackTrace::StackTrace()
10:15:12.016 3607   #1 0x7f6666700bf5 logging::LogMessage::~LogMessage()
10:15:12.016 3607   #2 0x7f66612dcafa
content::TransitionRequestManager::GetPendingTransitionRequest()
10:15:12.016 3607   #3 0x7f6660bccdb1
content::NavigatorImpl::DidStartProvisionalLoad()
10:15:12.016 3607   #4 0x7f6660bdb821
content::RenderFrameHostImpl::OnDidStartProvisionalLoadForFrame()
10:15:12.016 3607   #5 0x7f6660bf3554 DispatchToMethod<>()
10:15:12.016 3607   #6 0x7f6660be38b7
FrameHostMsg_DidStartProvisionalLoadForFrame::Dispatch<>()
10:15:12.016 3607   #7 0x7f6660bda57e
content::RenderFrameHostImpl::OnMessageReceived()
10:15:12.016 3607   #8 0x7f666103ad0e
content::RenderProcessHostImpl::OnMessageReceived()
10:15:12.016 3607   #9 0x7f666103b11f
content::RenderProcessHostImpl::OnMessageReceived()
10:15:12.016 3607   #10 0x7f6664fe5e9d
IPC::ChannelProxy::Context::OnDispatchMessage()
10:15:12.016 3607   #11 0x7f6664feea5a base::internal::RunnableAdapter<>::Run()
10:15:12.016 3607   #12 0x7f6664fee9b1
base::internal::InvokeHelper<>::MakeItSo()
10:15:12.016 3607   #13 0x7f6664fee94c base::internal::Invoker<>::Run()
10:15:12.016 3607   #14 0x7f666664eb3e base::Callback<>::Run()
10:15:12.016 3607   #15 0x7f666666cb89 base::debug::TaskAnnotator::RunTask()
10:15:12.016 3607   #16 0x7f6666729968 base::MessageLoop::RunTask()
10:15:12.016 3607   #17 0x7f6666729acb
base::MessageLoop::DeferOrRunPendingTask()
10:15:12.016 3607   #18 0x7f6666729d05 base::MessageLoop::DoWork()
10:15:12.016 3607   #19 0x7f66666227bc base::MessagePumpGlib::HandleDispatch()
10:15:12.016 3607   #20 0x7f6666622fd1 base::(anonymous
namespace)::WorkSourceDispatch()
10:15:12.016 3607   #21 0x7f6658588d13 g_main_context_dispatch
10:15:12.016 3607   #22 0x7f6658589060 <unknown>
10:15:12.016 3607   #23 0x7f6658589124 g_main_context_iteration
10:15:12.016 3607   #24 0x7f66666228c5 base::MessagePumpGlib::Run()
10:15:12.016 3607   #25 0x7f6666729432 base::MessageLoop::RunHandler()
10:15:12.016 3607   #26 0x7f66667920d4 base::RunLoop::Run()
10:15:12.016 3607   #27 0x7f66609ca164
content::BrowserMainLoop::MainMessageLoopRun()
10:15:12.016 3607   #28 0x7f66609c9fc1
content::BrowserMainLoop::RunMainMessageLoopParts()
10:15:12.016 3607   #29 0x7f66609d3075 content::BrowserMainRunnerImpl::Run()
10:15:12.016 3607   #30 0x000000452ef4 (anonymous namespace)::RunOneTest()
10:15:12.016 3607   #31 0x000000452b53 (anonymous namespace)::RunTests()
10:15:12.016 3607   #32 0x0000004529af LayoutTestBrowserMain()
10:15:12.016 3607   #33 0x00000044fcda content::ShellMainDelegate::RunProcess()
10:15:12.017 3607   #34 0x7f6660849f01 content::RunNamedProcessTypeMain()
10:15:12.017 3607   #35 0x7f666084e232 content::ContentMainRunnerImpl::Run()
10:15:12.017 3607   #36 0x7f66608493f5 content::ContentMain()
10:15:12.017 3607   #37 0x00000044ee5c main
10:15:12.017 3607   #38 0x7f665785676d __libc_start_main
10:15:12.017 3607   #39 0x00000044ed49 <unknown>
.