Navigation transitions: Parse out transition-entering-stylesheet link headers.

Navigation transitions: Parse out transition-entering-stylesheet link headers.

transition-entering-stylesheet is parsed out from the response header of the incoming page and passed to the Java side.

Design doc: https://docs.google.com/a/chromium.org/document/d/17jg1RRL3RI969cLwbKBIcoGDsPwqaEdBxafGNYGwiY4/edit#
Implementation details: https://docs.google.com/a/chromium.org/document/d/1kREPtFJaeLoDKwrfmrYTD7DHCdxX1RzFBga2gNY8lyE/edit#heading=h.bng2kpmyvxq5

BUG=370696
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285391

[Zhen Wang, 2014-07-15 00:25:25]

Hi guys, please take a look.

[no sievers, 2014-07-15 21:06:36]

On 2014/07/15 00:25:25, Zhen Wang wrote:
> Hi guys, please take a look.

Deferring review of Android bits to simonhatch@

[Zhen Wang, 2014-07-15 21:36:03]

Simon is on maternity leave starting from this week. Shall I ask Oystein to
review that part?

On 2014/07/15 21:06:36, sievers wrote:
> On 2014/07/15 00:25:25, Zhen Wang wrote:
> > Hi guys, please take a look.
> 
> Deferring review of Android bits to simonhatch@

[jam, 2014-07-16 05:50:39]

redirecting to jochen as I'm overloaded

[jochen (gone - plz use gerrit), 2014-07-16 13:25:59]

I'll have to wrap my heard around this a bit more, so only a few nits for now.
Also, please add unit tests

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

https://codereview.chromium.org/387703004/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.cc:22: bool EnumerateLinkHeaders(
should be in an anon namespace

https://codereview.chromium.org/387703004/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.cc:40: if (attributes) {
nit. no { } for single line bodies

https://codereview.chromium.org/387703004/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.cc:95: }
is this file clang formatted?

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

https://codereview.chromium.org/387703004/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.h:21: class HttpResponseHeaders;
don't indent

https://codereview.chromium.org/387703004/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.h:27: TransitionLayerData();
please inline ctor/dtor here.

[jochen (gone - plz use gerrit), 2014-07-16 13:26:17]

(for the C++ parts that is)

[jochen (gone - plz use gerrit), 2014-07-17 13:09:52]

ok, essentially, I'm wondering why you pass the transition layer data around
instead of passing the headers around and parse the link header right where you
need it?

[Zhen Wang, 2014-07-18 00:56:35]

I have updated it and added the unit tests. 

The transition layer data will include more things and send them to the Java
side, such as markup, transition color, and transition url. Those will be added
in the next few CLs. 

ptal

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

https://codereview.chromium.org/387703004/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.cc:22: bool EnumerateLinkHeaders(
On 2014/07/16 13:25:59, jochen wrote:
> should be in an anon namespace

Done.

https://codereview.chromium.org/387703004/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.cc:40: if (attributes) {
On 2014/07/16 13:25:58, jochen wrote:
> nit. no { } for single line bodies

Done.

https://codereview.chromium.org/387703004/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.cc:95: }
On 2014/07/16 13:25:58, jochen wrote:
> is this file clang formatted?

Done.

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

https://codereview.chromium.org/387703004/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.h:21: class HttpResponseHeaders;
On 2014/07/16 13:25:59, jochen wrote:
> don't indent

Done.

https://codereview.chromium.org/387703004/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.h:27: TransitionLayerData();
If I inline them, the compiler will complain with error message:

"error: [chromium-style] Complex constructor has an inlined body."


On 2014/07/16 13:25:59, jochen wrote:
> please inline ctor/dtor here.

[jochen (gone - plz use gerrit), 2014-07-18 09:15:16]

On 2014/07/18 at 00:56:35, zhenw wrote:
> I have updated it and added the unit tests. 

can you add some more cases (also including cases that should trigger the parser
to bail out)


> The transition layer data will include more things and send them to the Java
side, such as markup, transition color, and transition url. Those will be added
in the next few CLs. 

still, why pass the data around with a request-related callback instead of the
headers?

[Zhen Wang, 2014-07-18 17:56:24]

More tests are added. ptal.

The full version of transition_layer_data will be:

struct TransitionLayerData {
  TransitionLayerData();
  ~TransitionLayerData();

  GURL transition_url;
  std::string markup;
  std::vector<GURL> transition_stylesheets;
  std::string transition_color;
};

transition_layer_data's fields are populated in
CrossSiteResourceHandler::OnNavigationTransitionResponseStarted.
transition_stylesheets and transition_color come from the response header.
transition_url and markup come from other places. They will all be sent to the
Java side. So we use transition_layer_data instead of the header to pass the
data.

The other fields will be added in the next few patches.

[shatch, 2014-07-22 20:06:29]

Per sievers' request, looked at content/browser/android/*

https://codereview.chromium.org/387703004/diff/140001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/387703004/diff/140001/content/browser/android...
content/browser/android/content_view_core_impl.cc:1623: for (; iter !=
transition_layer_data.transition_stylesheets.end(); ++iter) {
Since there's 2 sets of stylesheets in the feature, a generic "stylesheets" name
isn't very descriptive. The "entering" stylesheets are the ones parsed from the
response headers, might be more clear if instead of "transition_stylesheets" you
renamed these to "entering_stylesheets". Same with the function
addStylesheetToTransition.

lg2m with those changes.

[Zhen Wang, 2014-07-22 23:02:06]

updated according to shatch's comment. ptal.

On 2014/07/22 20:06:29, shatch wrote:
> Per sievers' request, looked at content/browser/android/*
> 
>
https://codereview.chromium.org/387703004/diff/140001/content/browser/android...
> File content/browser/android/content_view_core_impl.cc (right):
> 
>
https://codereview.chromium.org/387703004/diff/140001/content/browser/android...
> content/browser/android/content_view_core_impl.cc:1623: for (; iter !=
> transition_layer_data.transition_stylesheets.end(); ++iter) {
> Since there's 2 sets of stylesheets in the feature, a generic "stylesheets"
name
> isn't very descriptive. The "entering" stylesheets are the ones parsed from
the
> response headers, might be more clear if instead of "transition_stylesheets"
you
> renamed these to "entering_stylesheets". Same with the function
> addStylesheetToTransition.
> 
> lg2m with those changes.

[jochen (gone - plz use gerrit), 2014-07-23 07:04:19]

somehow the codereview tool swallowed my comments :(

I understand that you will add more data to TransitionLayerData and that seems
very reasonable.

However, the resource loader logic should not be concerned with stylesheets and
colors. The CrossSiteResourceHandler should pass the headers in the callback,
and the ContentViewCoreImpl should be the one that does the parsing and
collecting of the data for the TransitionLayerData (which you can then still
pass to Java).

[Zhen Wang, 2014-07-23 19:08:29]

I see. 

I have removed TransitionLayerData. Header parsing is now in
ContentViewCoreImpl. ptal.


On 2014/07/23 07:04:19, jochen wrote:
> somehow the codereview tool swallowed my comments :(
> 
> I understand that you will add more data to TransitionLayerData and that seems
> very reasonable.
> 
> However, the resource loader logic should not be concerned with stylesheets
and
> colors. The CrossSiteResourceHandler should pass the headers in the callback,
> and the ContentViewCoreImpl should be the one that does the parsing and
> collecting of the data for the TransitionLayerData (which you can then still
> pass to Java).

[jochen (gone - plz use gerrit), 2014-07-24 09:05:27]

thank you, lgtm

https://codereview.chromium.org/387703004/diff/220001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/387703004/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_delegate.h:25: struct
TransitionLayerData;
nit. not required anymore

https://codereview.chromium.org/387703004/diff/220001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/387703004/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:50: struct
TransitionLayerData;
not required

[Zhen Wang, 2014-07-24 15:29:56]

nit fix

https://codereview.chromium.org/387703004/diff/220001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/387703004/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_delegate.h:25: struct
TransitionLayerData;
On 2014/07/24 09:05:27, jochen wrote:
> nit. not required anymore

Done.

https://codereview.chromium.org/387703004/diff/220001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/387703004/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:50: struct
TransitionLayerData;
On 2014/07/24 09:05:27, jochen wrote:
> not required

Done.

[Zhen Wang, 2014-07-24 16:52:06]

The CQ bit was checked by zhenw@chromium.org

[commit-bot: I haz the power, 2014-07-24 16:53:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zhenw@chromium.org/387703004/240001

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

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-24 18:43:31]

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

[commit-bot: I haz the power, 2014-07-24 18:43:32]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)

[Zhen Wang, 2014-07-24 21:49:13]

The CQ bit was checked by zhenw@chromium.org

[commit-bot: I haz the power, 2014-07-24 21:51:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zhenw@chromium.org/387703004/240001

[commit-bot: I haz the power, 2014-07-24 22:22:11]

Change committed as 285391