Navigation transitions: Initiate navigation transition and send serialized markup to Chrome

Navigation transitions: Initiate navigation transition and send serialized markup to Chrome

If any transition-elements metatags are present, we signal Chrome that this is a transition navigation and serialize the HTML elements designated by the selectors in the transitions-elements metatag.

The isTransitionNavigation flag needs to be passed through both WebFrameClient::decidePolicyForNavigation and WebFrameClient::dispatchDidStartProvisionalLoad to cover both same-origin and different-origin navigation paths.

Requires https://codereview.chromium.org/281383006/

R=esprehn,eseidel
BUG=281383006
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178413

[oystein (OOO til 10th of July), 2014-06-04 23:49:25]

ptal; mainly plumbing but sends some bools up to Chrome which will require some
3-way patch magic.

[esprehn, 2014-06-13 08:50:18]

abarth@ might have comments about how this interacts with the public/ API.

https://codereview.chromium.org/319573008/diff/20001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/319573008/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1289:
frame()->document()->getTransitionElementData(&elementData, IGNORE_EXCEPTION);
We don't allow NodeList and the other things you used in
getTransitionElementData in the other patch. We also don't use exceptions on
APIs not intended for script. I think you need to rethink what this API is
doing, we can't use getElementsByTagName or querySelector here.

https://codereview.chromium.org/319573008/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1290: if (elementData.isEmpty() ||
!validateTransitionNavigationMode())
The validateTransitionNavigationMode() check should be first, otherwise you do a
lot of work inside the get*() method above and then throw it away.

https://codereview.chromium.org/319573008/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1332: isTransitionNavigation =
dispatchAnyNavigationTransitionData();
Remove "Any".

[abarth-chromium, 2014-06-13 17:18:32]

https://codereview.chromium.org/319573008/diff/20001/Source/web/FrameLoaderCl...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/319573008/diff/20001/Source/web/FrameLoaderCl...
Source/web/FrameLoaderClientImpl.cpp:470: void
FrameLoaderClientImpl::dispatchAddNavigationTransitionData(const String& origin,
const String& markup)
I didn't track down where |origin| came from, but the type here most likely
should be SecurityOrigin rather than String.

https://codereview.chromium.org/319573008/diff/20001/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/319573008/diff/20001/public/web/WebFrameClien...
public/web/WebFrameClient.h:192: WebNavigationPolicy defaultPolicy, bool
isRedirect) { return defaultPolicy; }
This sort of change is begging for a struct instead of a long list of changing
parameters.  Using a struct will also make it easier to add and remove
parameters in the future.

https://codereview.chromium.org/319573008/diff/20001/public/web/WebViewClient.h
File public/web/WebViewClient.h (right):

https://codereview.chromium.org/319573008/diff/20001/public/web/WebViewClient...
public/web/WebViewClient.h:322: virtual void addNavigationTransitionData(const
WebString& origin, const WebString& markup) { }
|origin| should likely be a WebSecurityOrigin.

https://codereview.chromium.org/319573008/diff/20001/public/web/WebViewClient...
public/web/WebViewClient.h:323: 
Two blank lines between sections.

[oystein (OOO til 10th of July), 2014-06-13 17:52:27]

Thanks guys!

https://codereview.chromium.org/319573008/diff/20001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/319573008/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1289:
frame()->document()->getTransitionElementData(&elementData, IGNORE_EXCEPTION);
On 2014/06/13 08:50:18, esprehn wrote:
> We don't allow NodeList and the other things you used in
> getTransitionElementData in the other patch. We also don't use exceptions on
> APIs not intended for script. I think you need to rethink what this API is
> doing, we can't use getElementsByTagName or querySelector here.

Avoiding getElementsByTagName seems easy enough, but what's the alternative I
should use instead of querySelector to resolve the selectors the metatag
specifies, into nodes? Is the issue that these specific functions should only be
used by script bindings, or is it that it's resolved on-demand?

[esprehn, 2014-06-13 18:01:03]

On 2014/06/13 17:52:27, Oystein wrote:
> Thanks guys!
> 
>
https://codereview.chromium.org/319573008/diff/20001/Source/core/loader/Frame...
> File Source/core/loader/FrameLoader.cpp (right):
> 
>
https://codereview.chromium.org/319573008/diff/20001/Source/core/loader/Frame...
> Source/core/loader/FrameLoader.cpp:1289:
> frame()->document()->getTransitionElementData(&elementData, IGNORE_EXCEPTION);
> On 2014/06/13 08:50:18, esprehn wrote:
> > We don't allow NodeList and the other things you used in
> > getTransitionElementData in the other patch. We also don't use exceptions on
> > APIs not intended for script. I think you need to rethink what this API is
> > doing, we can't use getElementsByTagName or querySelector here.
> 
> Avoiding getElementsByTagName seems easy enough, but what's the alternative I
> should use instead of querySelector to resolve the selectors the metatag
> specifies, into nodes? Is the issue that these specific functions should only
be
> used by script bindings, or is it that it's resolved on-demand?

querySelectorAll is probably fine for that since you're taking user supplied
selectors and querying them. Ideally we'd have a version of that which returned
a Vector instead of a NodeList, but we can fix that later.

[oystein (OOO til 10th of July), 2014-06-18 00:04:09]

New version up; ptal :)

https://codereview.chromium.org/319573008/diff/20001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/319573008/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1290: if (elementData.isEmpty() ||
!validateTransitionNavigationMode())
On 2014/06/13 08:50:18, esprehn wrote:
> The validateTransitionNavigationMode() check should be first, otherwise you do
a
> lot of work inside the get*() method above and then throw it away.

validateTransitionNavigationMode() also tosses some error messages to the
console to tell developers why their transitions aren't working. Is it worth
splitting this up?

https://codereview.chromium.org/319573008/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1332: isTransitionNavigation =
dispatchAnyNavigationTransitionData();
On 2014/06/13 08:50:18, esprehn wrote:
> Remove "Any".

Done.

https://codereview.chromium.org/319573008/diff/20001/Source/web/FrameLoaderCl...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/319573008/diff/20001/Source/web/FrameLoaderCl...
Source/web/FrameLoaderClientImpl.cpp:470: void
FrameLoaderClientImpl::dispatchAddNavigationTransitionData(const String& origin,
const String& markup)
On 2014/06/13 17:18:31, abarth wrote:
> I didn't track down where |origin| came from, but the type here most likely
> should be SecurityOrigin rather than String.

My naming is bad; renamed to "allowedDestinationOrigin". It'll be something
CSP-lite; "*" or "http://*.google.com" etc.

https://codereview.chromium.org/319573008/diff/20001/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/319573008/diff/20001/public/web/WebFrameClien...
public/web/WebFrameClient.h:192: WebNavigationPolicy defaultPolicy, bool
isRedirect) { return defaultPolicy; }
On 2014/06/13 17:18:31, abarth wrote:
> This sort of change is begging for a struct instead of a long list of changing
> parameters.  Using a struct will also make it easier to add and remove
> parameters in the future.

Done! I made the URL the only constructor parameter; seemed to make the most
semantic sense, but I can add all of them if need be.

https://codereview.chromium.org/319573008/diff/20001/public/web/WebViewClient.h
File public/web/WebViewClient.h (right):

https://codereview.chromium.org/319573008/diff/20001/public/web/WebViewClient...
public/web/WebViewClient.h:323: 
On 2014/06/13 17:18:31, abarth wrote:
> Two blank lines between sections.

Done.

[abarth-chromium, 2014-06-18 00:20:00]

lgtm

[esprehn, 2014-06-21 02:50:21]

lgtm

https://codereview.chromium.org/319573008/diff/80001/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/319573008/diff/80001/public/web/WebFrameClien...
public/web/WebFrameClient.h:194: NavigationPolicyInfo(const WebURLRequest&
urlRequest) : frame(0), extraData(0), urlRequest(urlRequest),
navigationType(WebNavigationTypeOther),
defaultPolicy(WebNavigationPolicyIgnore), isRedirect(false),
isTransitionNavigation(false) { }
This should have each member on it's own line, and the : one line down.

https://codereview.chromium.org/319573008/diff/80001/public/web/WebFrameClien...
public/web/WebFrameClient.h:196: virtual WebNavigationPolicy
decidePolicyForNavigation(const NavigationPolicyInfo& info)
add a new line before this

[oystein (OOO til 10th of July), 2014-07-17 22:57:42]

The CQ bit was checked by oysteine@chromium.org

[commit-bot: I haz the power, 2014-07-17 22:58:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oysteine@chromium.org/319573008/180001

[commit-bot: I haz the power, 2014-07-18 01:23:25]

Change committed as 178413