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

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

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.

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

================

Originally this was a 3-way patch since it involves API change in WebFrameClient.h. After the chrome side impl was committed (step 1), we found that it is better to pass struct instead of long list of params. So this turns out to be a 5-way patch.

Here are the 5 steps:
1. Chrome side - save the data (https://codereview.chromium.org/652283002/).
2. Blink side - getting the data and introduce struct WebTransitionElementData, but still using long list of params (this CL).
3. Chrome side - implement the API with WebTransitionElementData (https://codereview.chromium.org/679813003/).
4. Blink side - remove old APIs that use long list of params and start to use new API with WebTransitionElementData.
5. Chrome side - remove redundent implmentations of the old APIs.


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

[Zhen Wang, 2014-10-21 20:41:25]

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

[Zhen Wang, 2014-10-21 20:41:27]

ptal

[esprehn, 2014-10-21 20:50:49]

https://codereview.chromium.org/654953002/diff/40001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/654953002/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5655:
newElements.names.append(element->getIdAttribute().string());
These elements don't have to have ids, you just select them with querySelector.

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

https://codereview.chromium.org/654953002/diff/40001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1241:
client()->dispatchAddNavigationTransitionData(iter->scope, iter->selector,
iter->markup, iter->names, iter->rects);
Seems like you should be passing a struct, passing a super long list of stuff
from the iter seems strange. Why not just pass the TransitionElementData?

https://codereview.chromium.org/654953002/diff/40001/Source/core/testing/Inte...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/654953002/diff/40001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2282: PassRefPtrWillBeRawPtr<ClientRectList>
Internals::getTransitionElementRects(ExceptionState&)
You should remove the ExceptionState argument and the throws thing from the idl.

https://codereview.chromium.org/654953002/diff/40001/Source/core/testing/Inte...
File Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/654953002/diff/40001/Source/core/testing/Inte...
Source/core/testing/Internals.idl:282: [TypeChecking=Interface] ClientRect
boundsInRootViewSpace(Element element);
What's with the TypeChecking attr? I don't think you need that.

https://codereview.chromium.org/654953002/diff/40001/Source/core/testing/Inte...
Source/core/testing/Internals.idl:285: [RaisesException, TypeChecking=Interface]
ClientRectList getTransitionElementRects();
Remove RaisesException, it doesn't use that argument. Also why you do have this
TypeChecking attr?

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

https://codereview.chromium.org/654953002/diff/40001/Source/web/FrameLoaderCl...
Source/web/FrameLoaderClientImpl.cpp:483: const String& markup, const
Vector<String>& names,
pass a struct instead of a long list of args.

[Zhen Wang, 2014-10-21 21:36:48]

https://codereview.chromium.org/654953002/diff/40001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/654953002/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5655:
newElements.names.append(element->getIdAttribute().string());
These elements are from the elementList, which are from querySelectorAll(). See
line 5641. So they have ids.

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

https://codereview.chromium.org/654953002/diff/40001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1241:
client()->dispatchAddNavigationTransitionData(iter->scope, iter->selector,
iter->markup, iter->names, iter->rects);
ditto. See FrameLoaderClientImpl.cpp

https://codereview.chromium.org/654953002/diff/40001/Source/core/testing/Inte...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/654953002/diff/40001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2282: PassRefPtrWillBeRawPtr<ClientRectList>
Internals::getTransitionElementRects(ExceptionState&)
On 2014/10/21 20:50:48, esprehn wrote:
> You should remove the ExceptionState argument and the throws thing from the
idl.

Done.

https://codereview.chromium.org/654953002/diff/40001/Source/core/testing/Inte...
File Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/654953002/diff/40001/Source/core/testing/Inte...
Source/core/testing/Internals.idl:282: [TypeChecking=Interface] ClientRect
boundsInRootViewSpace(Element element);
ditto

https://codereview.chromium.org/654953002/diff/40001/Source/core/testing/Inte...
Source/core/testing/Internals.idl:285: [RaisesException, TypeChecking=Interface]
ClientRectList getTransitionElementRects();
I see other places using ClientRectList have RaisesException and TypeChecking
and thought it is required.

They are removed now.

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

https://codereview.chromium.org/654953002/diff/40001/Source/web/FrameLoaderCl...
Source/web/FrameLoaderClientImpl.cpp:483: const String& markup, const
Vector<String>& names,
Currently the implementation uses this interface. See:
https://codereview.chromium.org/652283002/diff/60001/content/renderer/render_...
(committed yesterday. not yet in online code search.)


Do you mean I should create something like WebTransitionElementData and pass the
data to content side?

[sof, 2014-10-22 05:22:47]

https://codereview.chromium.org/654953002/diff/40001/Source/core/testing/Inte...
File Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/654953002/diff/40001/Source/core/testing/Inte...
Source/core/testing/Internals.idl:282: [TypeChecking=Interface] ClientRect
boundsInRootViewSpace(Element element);
On 2014/10/21 21:36:48, Zhen Wang wrote:
> ditto

Please keep using [TypeChecking=Inteface].

boundsInRootViewSpace() will now crash if you pass it {} as argument (or some
other non-Element object.)

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

https://codereview.chromium.org/654953002/diff/40001/Source/core/testing/Inte...
File Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/654953002/diff/40001/Source/core/testing/Inte...
Source/core/testing/Internals.idl:282: [TypeChecking=Interface] ClientRect
boundsInRootViewSpace(Element element);
I see. Thanks for pointing this out.

[Zhen Wang, 2014-10-24 14:12:09]

ping

[sof, 2014-10-24 15:17:15]

https://codereview.chromium.org/654953002/diff/80001/Source/core/loader/Frame...
File Source/core/loader/FrameLoaderClient.h (right):

https://codereview.chromium.org/654953002/diff/80001/Source/core/loader/Frame...
Source/core/loader/FrameLoaderClient.h:105: const String& origin, const String&
selector, const String& markup,
Let's keep this on one line also?

https://codereview.chromium.org/654953002/diff/80001/Source/core/testing/Inte...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/654953002/diff/80001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2272:
Vector<Document::TransitionElementData>::iterator iter = elementData.begin();
Shorter as (the untested)

  for (const auto& element : elementData)
      names.appendVector(element.names);

https://codereview.chromium.org/654953002/diff/80001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2288:
Vector<Document::TransitionElementData>::iterator iter = elementData.begin();
Can also be shortened.

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

https://codereview.chromium.org/654953002/diff/80001/Source/web/FrameLoaderCl...
Source/web/FrameLoaderClientImpl.cpp:483: const String& markup, const
Vector<String>& names,
Ditto

https://codereview.chromium.org/654953002/diff/80001/Source/web/FrameLoaderCl...
Source/web/FrameLoaderClientImpl.cpp:488: allowedDestinationOrigin, selector,
markup, names, rects);
Keep line length style as-was?

https://codereview.chromium.org/654953002/diff/80001/Source/web/FrameLoaderCl...
File Source/web/FrameLoaderClientImpl.h (right):

https://codereview.chromium.org/654953002/diff/80001/Source/web/FrameLoaderCl...
Source/web/FrameLoaderClientImpl.h:99: virtual void
dispatchAddNavigationTransitionData(
Stick with the Blink coding style of keeping it all one line?

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

https://codereview.chromium.org/654953002/diff/80001/public/web/WebFrameClien...
public/web/WebFrameClient.h:317: const WebString& allowedDestinationOrigin,
const WebString& selector,
Ditto

[esprehn, 2014-10-24 21:59:11]

Yes please do add some kind of struct to pass the data. This patch can't work as
designed though, random elements selected with querySelector don't need to have
ids.

https://codereview.chromium.org/654953002/diff/100001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/654953002/diff/100001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5626:
newElements.names.append(element->getIdAttribute().string());
Using querySelector doesn't mean that these have ids. I don't understand what
you're saying. I can querySelector for a thing like "div + div", and select an
element with no attributes at all. I can similarly select .foo and get something
with class names and no ids. This can't work.

[Zhen Wang, 2014-10-27 14:14:45]

Using struct and checking the ID now. This CL now is part of the 5-way patch.
Please see the updated CL description.

https://codereview.chromium.org/654953002/diff/80001/Source/core/loader/Frame...
File Source/core/loader/FrameLoaderClient.h (right):

https://codereview.chromium.org/654953002/diff/80001/Source/core/loader/Frame...
Source/core/loader/FrameLoaderClient.h:105: const String& origin, const String&
selector, const String& markup,
Using struct now.

https://codereview.chromium.org/654953002/diff/80001/Source/core/testing/Inte...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/654953002/diff/80001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2272:
Vector<Document::TransitionElementData>::iterator iter = elementData.begin();
I see other for loops using the traditional version. Any strong reason for the
shorter version?

https://codereview.chromium.org/654953002/diff/80001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2288:
Vector<Document::TransitionElementData>::iterator iter = elementData.begin();
ditto

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

https://codereview.chromium.org/654953002/diff/80001/Source/web/FrameLoaderCl...
Source/web/FrameLoaderClientImpl.cpp:483: const String& markup, const
Vector<String>& names,
Using struct now.

https://codereview.chromium.org/654953002/diff/80001/Source/web/FrameLoaderCl...
Source/web/FrameLoaderClientImpl.cpp:488: allowedDestinationOrigin, selector,
markup, names, rects);
This will be changed into using struct in later CLs.

By the way, I see other places also using multiple line style. Are we guided to
use single line statements only?

https://codereview.chromium.org/654953002/diff/80001/Source/web/FrameLoaderCl...
File Source/web/FrameLoaderClientImpl.h (right):

https://codereview.chromium.org/654953002/diff/80001/Source/web/FrameLoaderCl...
Source/web/FrameLoaderClientImpl.h:99: virtual void
dispatchAddNavigationTransitionData(
Using struct now.

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

https://codereview.chromium.org/654953002/diff/80001/public/web/WebFrameClien...
public/web/WebFrameClient.h:317: const WebString& allowedDestinationOrigin,
const WebString& selector,
Will change to use struct in later CLs.

https://codereview.chromium.org/654953002/diff/100001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/654953002/diff/100001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5626:
newElements.names.append(element->getIdAttribute().string());
Checking if the element has ID now. Using empty string if it does not have ID.

[Zhen Wang, 2014-10-29 13:57:19]

ping

[Zhen Wang, 2014-10-31 14:43:36]

ping

[Zhen Wang, 2014-11-03 19:35:52]

zhenw@chromium.org changed reviewers:
+ japhet@chromium.org
- esprehn@chromium.org

[Zhen Wang, 2014-11-03 19:35:53]

Hi Nate,

It seems that Elliott is too busy to continue to review this. Can you take a
look?

Best
-Zhen

[Nate Chapin, 2014-11-05 18:29:43]

https://codereview.chromium.org/654953002/diff/160001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/654953002/diff/160001/Source/core/dom/Documen...
Source/core/dom/Document.h:389: String name;
Given that this is set based on the elment's id, should this parameter be called
id as well?

https://codereview.chromium.org/654953002/diff/160001/Source/core/loader/Fram...
File Source/core/loader/FrameLoaderClient.h (right):

https://codereview.chromium.org/654953002/diff/160001/Source/core/loader/Fram...
Source/core/loader/FrameLoaderClient.h:38: #include
"platform/geometry/IntRect.h"
Is this actually needed?

https://codereview.chromium.org/654953002/diff/160001/Source/core/loader/Fram...
Source/core/loader/FrameLoaderClient.h:105: virtual void
dispatchAddNavigationTransitionData(const Document::TransitionElementData&) { }
I'm not thrilled with this struct being part of Document, but I don't have a
better idea.

https://codereview.chromium.org/654953002/diff/160001/Source/web/FrameLoaderC...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/654953002/diff/160001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:483: struct TransitionElementData {
I don't see any uses of this struct.

https://codereview.chromium.org/654953002/diff/160001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:493: if (m_webFrame->client()) {
Early exit if m_webFrame->client() is null, rather than wrapping this whole
function in an if()?

https://codereview.chromium.org/654953002/diff/160001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:494: // TODO(zhenw): change to use
WebTransitionElementData after the chrome side is done.
blink uses FIXME: instead of TODO(name):

https://codereview.chromium.org/654953002/diff/160001/public/web/WebFrameClie...
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/654953002/diff/160001/public/web/WebFrameClie...
public/web/WebFrameClient.h:319: // TODO(zhenw): remove the following two
addNavigationTransitionData() functions after the chrome side is done.
TODO->FIXME

https://codereview.chromium.org/654953002/diff/160001/public/web/WebFrameClie...
public/web/WebFrameClient.h:321: virtual void addNavigationTransitionData(const
WebString& allowedDestinationOrigin, const WebString& selector, const WebString&
markup, const WebVector<WebString>& names, const WebVector<WebRect>& rects) { }
Is this intermediate api actually necessary?

https://codereview.chromium.org/654953002/diff/160001/public/web/WebTransitio...
File public/web/WebTransitionElementData.h (right):

https://codereview.chromium.org/654953002/diff/160001/public/web/WebTransitio...
public/web/WebTransitionElementData.h:45: struct WebTransitionElementData {
You should probably add compile asserts to ensure that this continues matching
the internal struct in Source/web/AssertMatchingEnums.cpp.

[Zhen Wang, 2014-11-06 17:59:26]

https://codereview.chromium.org/654953002/diff/160001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/654953002/diff/160001/Source/core/dom/Documen...
Source/core/dom/Document.h:389: String name;
I see your point. 

I use "name" here because the transition element will be eventually passed to
Chrome for Android Java side and be used as the name of the transition element
with Activity Transitions API in Android 5.0. So I guess it is natural to use
"name" as the variable name.

https://codereview.chromium.org/654953002/diff/160001/Source/core/loader/Fram...
File Source/core/loader/FrameLoaderClient.h (right):

https://codereview.chromium.org/654953002/diff/160001/Source/core/loader/Fram...
Source/core/loader/FrameLoaderClient.h:38: #include
"platform/geometry/IntRect.h"
Removed

https://codereview.chromium.org/654953002/diff/160001/Source/web/FrameLoaderC...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/654953002/diff/160001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:483: struct TransitionElementData {
See the changes in Document.cpp.

https://codereview.chromium.org/654953002/diff/160001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:493: if (m_webFrame->client()) {
On 2014/11/05 18:29:43, Nate Chapin wrote:
> Early exit if m_webFrame->client() is null, rather than wrapping this whole
> function in an if()?

Done.

https://codereview.chromium.org/654953002/diff/160001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:494: // TODO(zhenw): change to use
WebTransitionElementData after the chrome side is done.
On 2014/11/05 18:29:43, Nate Chapin wrote:
> blink uses FIXME: instead of TODO(name):

Done.

https://codereview.chromium.org/654953002/diff/160001/public/web/WebFrameClie...
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/654953002/diff/160001/public/web/WebFrameClie...
public/web/WebFrameClient.h:319: // TODO(zhenw): remove the following two
addNavigationTransitionData() functions after the chrome side is done.
On 2014/11/05 18:29:43, Nate Chapin wrote:
> TODO->FIXME

Done.

https://codereview.chromium.org/654953002/diff/160001/public/web/WebFrameClie...
public/web/WebFrameClient.h:321: virtual void addNavigationTransitionData(const
WebString& allowedDestinationOrigin, const WebString& selector, const WebString&
markup, const WebVector<WebString>& names, const WebVector<WebRect>& rects) { }
In theory, not necessary. But in reality, we need it to pass compilation.

Currently in render_frame_impl, we have two implementations:

1. addNavigationTransitionData(WebString&, WebString&, WebString&)
2. addNavigationTransitionData(WebString&, WebString&, WebString&, WebVector&,
WebVector&)

If I remove this intermediate api and leave 2 function declarations here, the
compiler will complain that there is no matching member function. It cannot map
the declarations with WebTransitionElementData struct to the implementation.

If I keep this intermediate api, the compiler will understand that 2 functions
are implemented, and the one with WebTransitionElementData is not implemented.
And then the compilation can pass.

https://codereview.chromium.org/654953002/diff/160001/public/web/WebTransitio...
File public/web/WebTransitionElementData.h (right):

https://codereview.chromium.org/654953002/diff/160001/public/web/WebTransitio...
public/web/WebTransitionElementData.h:45: struct WebTransitionElementData {
I don't really understand. No enum is used in this struct.

[Nate Chapin, 2014-11-11 00:28:18]

Almost there, just a couple nits and some lingering confusion about the struct
in FraeLoaderClinetImpl.cpp

https://codereview.chromium.org/654953002/diff/160001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/654953002/diff/160001/Source/core/dom/Documen...
Source/core/dom/Document.h:389: String name;
On 2014/11/06 17:59:26, Zhen Wang wrote:
> I see your point. 
> 
> I use "name" here because the transition element will be eventually passed to
> Chrome for Android Java side and be used as the name of the transition element
> with Activity Transitions API in Android 5.0. So I guess it is natural to use
> "name" as the variable name.

Hrm. I guess consistency trumps having a sane name internally, but I'm still
just a little grumpy. :)

https://codereview.chromium.org/654953002/diff/160001/Source/web/FrameLoaderC...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/654953002/diff/160001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:483: struct TransitionElementData {
On 2014/11/06 17:59:26, Zhen Wang wrote:
> See the changes in Document.cpp.

It's defined in Document.h, why the redefinition here?

https://codereview.chromium.org/654953002/diff/160001/public/web/WebFrameClie...
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/654953002/diff/160001/public/web/WebFrameClie...
public/web/WebFrameClient.h:321: virtual void addNavigationTransitionData(const
WebString& allowedDestinationOrigin, const WebString& selector, const WebString&
markup, const WebVector<WebString>& names, const WebVector<WebRect>& rects) { }
On 2014/11/06 17:59:26, Zhen Wang wrote:
> In theory, not necessary. But in reality, we need it to pass compilation.
> 
> Currently in render_frame_impl, we have two implementations:
> 
> 1. addNavigationTransitionData(WebString&, WebString&, WebString&)
> 2. addNavigationTransitionData(WebString&, WebString&, WebString&, WebVector&,
> WebVector&)
> 
> If I remove this intermediate api and leave 2 function declarations here, the
> compiler will complain that there is no matching member function. It cannot
map
> the declarations with WebTransitionElementData struct to the implementation.
> 
> If I keep this intermediate api, the compiler will understand that 2 functions
> are implemented, and the one with WebTransitionElementData is not implemented.
> And then the compilation can pass.

Huh, that seems strange, but I guess it's OK. So long as we get rid of it as
quickly as possible.

https://codereview.chromium.org/654953002/diff/160001/public/web/WebTransitio...
File public/web/WebTransitionElementData.h (right):

https://codereview.chromium.org/654953002/diff/160001/public/web/WebTransitio...
public/web/WebTransitionElementData.h:45: struct WebTransitionElementData {
On 2014/11/06 17:59:26, Zhen Wang wrote:
> I don't really understand. No enum is used in this struct.

Doh. I mixed-and-matched concepts, don't mind me.

https://codereview.chromium.org/654953002/diff/200001/Source/web/FrameLoaderC...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/654953002/diff/200001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:496: // FIXME(zhenw): change to use
WebTransitionElementData after the chrome side is done.
Nit: typically we don't include a specific name on a FIXME, for *historical
reasons*.

https://codereview.chromium.org/654953002/diff/200001/public/web/WebFrameClie...
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/654953002/diff/200001/public/web/WebFrameClie...
public/web/WebFrameClient.h:318: // TODO(zhenw): remove the following two
addNavigationTransitionData() functions after the chrome side is done.
TODO(zhew)->FIXME

[Zhen Wang, 2014-11-11 15:12:20]

https://codereview.chromium.org/654953002/diff/160001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/654953002/diff/160001/Source/core/dom/Documen...
Source/core/dom/Document.h:389: String name;
I see. Using "id" now.

https://codereview.chromium.org/654953002/diff/160001/Source/web/FrameLoaderC...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/654953002/diff/160001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:483: struct TransitionElementData {
Oh. My mistake. Fixed.

https://codereview.chromium.org/654953002/diff/200001/Source/web/FrameLoaderC...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/654953002/diff/200001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:496: // FIXME(zhenw): change to use
WebTransitionElementData after the chrome side is done.
On 2014/11/11 00:28:17, Nate Chapin wrote:
> Nit: typically we don't include a specific name on a FIXME, for *historical
> reasons*.

Done.

https://codereview.chromium.org/654953002/diff/200001/public/web/WebFrameClie...
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/654953002/diff/200001/public/web/WebFrameClie...
public/web/WebFrameClient.h:318: // TODO(zhenw): remove the following two
addNavigationTransitionData() functions after the chrome side is done.
On 2014/11/11 00:28:17, Nate Chapin wrote:
> TODO(zhew)->FIXME

Done.

[Nate Chapin, 2014-11-11 19:18:55]

LGTM, thanks!

[Zhen Wang, 2014-11-11 19:23:45]

The CQ bit was checked by zhenw@chromium.org

[commit-bot: I haz the power, 2014-11-11 19:24:41]

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

[commit-bot: I haz the power, 2014-11-11 19:27:04]

Committed patchset #13 (id:240001) as 185140