Navigation transitions: Added createStyledMarkupForNavigationTransition

Navigation transitions: Added createStyledMarkupForNavigationTransition

This will be used to serialize markup for transition elements, which is then sent to the transition layer.

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

[oystein (OOO til 10th of July), 2014-05-21 21:26:47]

eseidel: Any suggestions on who else should be added here?

[oystein (OOO til 10th of July), 2014-05-22 18:23:56]

+esprehn

[esprehn, 2014-05-22 18:29:06]

I really don't think this is what you want, why do you need to serialize html?

https://codereview.chromium.org/281383006/diff/1/Source/core/editing/markup.cpp
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/281383006/diff/1/Source/core/editing/markup.c...
Source/core/editing/markup.cpp:340:
m_wrappingStyle->style()->removeProperty(CSSPropertyBackground);
Huh?

https://codereview.chromium.org/281383006/diff/1/Source/core/editing/markup.c...
Source/core/editing/markup.cpp:1084: accumulator.serializeNodes(node,
NodeTraversal::nextSkippingChildren(*node));
This code doesn't make sense, why are you going to the next sibling or parent
node? nextSkippingChildren doesn't look correct.

[oystein (OOO til 10th of July), 2014-05-22 18:41:05]

On 2014/05/22 18:29:06, esprehn wrote:
> I really don't think this is what you want, why do you need to serialize html?
>

The linked bug has docs with API and implementation details, but this is for the
purposes of popping up designated transition elements from one page to another
(a transition page), which lives in a separate renderer and will be animated by
a stylesheet fetched from the destination page we're in the middle of navigating
to.
 
>
https://codereview.chromium.org/281383006/diff/1/Source/core/editing/markup.cpp
> File Source/core/editing/markup.cpp (right):
> 
>
https://codereview.chromium.org/281383006/diff/1/Source/core/editing/markup.c...
> Source/core/editing/markup.cpp:340:
> m_wrappingStyle->style()->removeProperty(CSSPropertyBackground);
> Huh?

Transition elements shouldn't inline the background color from any wrapping
styles. Imagine an image (like the Google logo), on a page with a blue
background. If we're transitioning to another page with a red background, we
don't want a blue rectangle behind the logo as we animate it to its new
position. We just want whatever background color we have otherwise to show
through. 
 
>
https://codereview.chromium.org/281383006/diff/1/Source/core/editing/markup.c...
> Source/core/editing/markup.cpp:1084: accumulator.serializeNodes(node,
> NodeTraversal::nextSkippingChildren(*node));
> This code doesn't make sense, why are you going to the next sibling or parent
> node? nextSkippingChildren doesn't look correct.

We want to serialize the supplied node and any children it might have; the
second parameter is for designating the "end" node, i.e. when to stop.

[oystein (OOO til 10th of July), 2014-05-28 17:57:52]

Ping :)

[eseidel, 2014-05-28 22:09:28]

I'd much rather yoshi (or one of the editing folks) reviewed this.  dcheng was
also mr. clipboard for a while.

[yosin_UTC9, 2014-05-29 03:58:52]

Could you add layout test for your change?
Thanks in advance.

https://codereview.chromium.org/281383006/diff/1/Source/core/editing/markup.cpp
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/281383006/diff/1/Source/core/editing/markup.c...
Source/core/editing/markup.cpp:308: if (&element == m_highestNodeToBeSerialized
&& m_shouldAnnotate == AnnotateForNavigationTransition)
nit: You don't need |&| for |element|. |Node| has
bool operator==(const Node&, const Node*)

[oystein (OOO til 10th of July), 2014-05-29 18:26:49]

Thanks yosa!

On 2014/05/29 03:58:52, yosi wrote:
> Could you add layout test for your change?
> Thanks in advance.

I'd love to, but I'm not sure how: this is just a piece of a bigger puzzle
(check the linked bug). Since this code path is only ever exercised from the
Chrome-side (as part of a navigation transition), I'm not sure how to add a
layout test for it without adding some C++ code which would only be used for
that specific test. What's the usual procedure in these situations?

https://codereview.chromium.org/281383006/diff/1/Source/core/editing/markup.cpp
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/281383006/diff/1/Source/core/editing/markup.c...
Source/core/editing/markup.cpp:308: if (&element == m_highestNodeToBeSerialized
&& m_shouldAnnotate == AnnotateForNavigationTransition)
On 2014/05/29 03:58:52, yosi wrote:
> nit: You don't need |&| for |element|. |Node| has
> bool operator==(const Node&, const Node*)

Done.

[oystein (OOO til 10th of July), 2014-05-29 18:27:17]

On 2014/05/29 18:26:49, Oystein wrote:
> Thanks yosa!
> 

s/yosa/yosi/

[esprehn, 2014-05-29 21:32:09]

Serializing the markup like this doesn't seem right. What about CSS animations?
Also serializing the width/height and positioning like that isn't right.

Can you explain what this is trying to do? I think you want to just put real
<style> elements into the page. Trying to inline the CSS like this isn't right,
especially not reusing this editing stuff. This is for copy/paste into email and
such.

https://codereview.chromium.org/281383006/diff/20001/Source/core/editing/Edit...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/281383006/diff/20001/Source/core/editing/Edit...
Source/core/editing/EditingStyle.cpp:1243:
m_mutableStyle->setProperty(CSSPropertyHeight,
cssValuePool().createValue(rect->bottom() - rect->top(),
CSSPrimitiveValue::CSS_PX));
What about padding/margins? This approach doesn't seem right.

https://codereview.chromium.org/281383006/diff/20001/Source/core/editing/Edit...
File Source/core/editing/EditingStyle.h (right):

https://codereview.chromium.org/281383006/diff/20001/Source/core/editing/Edit...
Source/core/editing/EditingStyle.h:135: void
addAbsolutePositioningFromElement(Element*);
Should take a reference.

[esprehn, 2014-05-29 21:33:45]

This needs a unit test too. I'd really rather you didn't depend on this
clipboard related code though. Navigation transitions doesn't want all those
quirks.

[oystein (OOO til 10th of July), 2014-05-29 21:47:42]

On 2014/05/29 21:32:09, esprehn wrote:
> Serializing the markup like this doesn't seem right. What about CSS
animations?
> Also serializing the width/height and positioning like that isn't right.
> 
> Can you explain what this is trying to do? I think you want to just put real
> <style> elements into the page. Trying to inline the CSS like this isn't
right,
> especially not reusing this editing stuff. This is for copy/paste into email
and
> such.

Docs here:

API:
https://docs.google.com/a/chromium.org/document/d/17jg1RRL3RI969cLwbKBIcoGDsP...
Architecture:
https://docs.google.com/a/chromium.org/document/d/1kREPtFJaeLoDKwrfmrYTD7DHCd...

Essentially, quirks are fine. Stuff like CSS animations not working is fine,
too. Content authors designate which page elements should be transition
elements, and will have to keep them simple and "compatible" with the editing
code.

What would the alternative be? The elements need to be recreated in a wholly
separate renderer (could even be a separate process), so some sort of
serialization is needed.

>This needs a unit test too. I'd really rather you didn't depend on this
>clipboard related code though. Navigation transitions doesn't want all those
>quirks.

See reply to yosi above, unsure how to do this without adding test-specific
codepaths to Blink as there'll be no way to run this code without the whole
system in place.

[oystein (OOO til 10th of July), 2014-06-03 18:02:32]

esprehn/yosi: I added a test and a new binding to Internals.cpp to be able to
exercise this.

To be able to do this I also included a function in Document which gathers in
the transition data from any meta tags in the document; please let me know if
there's any better locations for it :).

I'm now also accounting for padding/margins/borders in the code which sets the
absolute positioning.

PTAL!

[esprehn, 2014-06-05 01:08:19]

I'll look this over asap. Thanks for updating it.

[oystein (OOO til 10th of July), 2014-06-05 01:10:07]

On 2014/06/05 01:08:19, esprehn wrote:
> I'll look this over asap. Thanks for updating it.

Np, thanks!

There's still cases where this'll break (when a transition element wasn't using
absolute positioning before, but one of its children is for example), but
hopefully that's something we can iterate on.

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

On 2014/06/05 01:10:07, Oystein wrote:
> On 2014/06/05 01:08:19, esprehn wrote:
> > I'll look this over asap. Thanks for updating it.
> 
> Np, thanks!
> 
> There's still cases where this'll break (when a transition element wasn't
using
> absolute positioning before, but one of its children is for example), but
> hopefully that's something we can iterate on.

Esprehn: ping :)

[esprehn, 2014-06-13 08:47:06]

Thanks for adding stuff to write tests.

https://codereview.chromium.org/281383006/diff/80001/LayoutTests/fast/html/na...
File LayoutTests/fast/html/navigation-transition.html (right):

https://codereview.chromium.org/281383006/diff/80001/LayoutTests/fast/html/na...
LayoutTests/fast/html/navigation-transition.html:24: <body bgcolor="#ffffff"
marginheight="0" marginwidth="0">
Using presentational markup is really weird. Can we use css instead?

https://codereview.chromium.org/281383006/diff/80001/LayoutTests/fast/html/na...
LayoutTests/fast/html/navigation-transition.html:39: <iframe id="testiframe"
frameBorder="0" style="width:100%;height:600px;background:gray">
border: none; lets not use presentational markup unless you explicitly are
trying to test that.

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

https://codereview.chromium.org/281383006/diff/80001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5677: void
Document::getTransitionElementData(Vector<TransitionElementData>* elementData,
ExceptionState& exceptionState)
Since this is only used in tests, lets put it in Internals.

https://codereview.chromium.org/281383006/diff/80001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5681: RefPtr<HTMLCollection> metaElements =
getElementsByTagName(HTMLNames::metaTag.localName());
We don't normally allow code like this in core/, except in Internals.

https://codereview.chromium.org/281383006/diff/80001/Source/core/editing/Edit...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/281383006/diff/80001/Source/core/editing/Edit...
Source/core/editing/EditingStyle.cpp:1254: height -= renderBox->paddingTop() +
renderBox->paddingBottom() + renderBox->borderHeight();
This doesn't work if you set box-sizing, I really think you want to just set
box-sizing to border-box here and then copy the padding and margins over.

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

https://codereview.chromium.org/281383006/diff/80001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2381:
frame()->document()->getTransitionElementData(&elementData, exceptionState);
pass vector by reference.

https://codereview.chromium.org/281383006/diff/80001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2387: }
no braces.

[oystein (OOO til 10th of July), 2014-06-17 21:07:06]

New version up; ptal :).

https://codereview.chromium.org/281383006/diff/80001/LayoutTests/fast/html/na...
File LayoutTests/fast/html/navigation-transition.html (right):

https://codereview.chromium.org/281383006/diff/80001/LayoutTests/fast/html/na...
LayoutTests/fast/html/navigation-transition.html:24: <body bgcolor="#ffffff"
marginheight="0" marginwidth="0">
On 2014/06/13 08:47:05, esprehn wrote:
> Using presentational markup is really weird. Can we use css instead?

Done.

https://codereview.chromium.org/281383006/diff/80001/LayoutTests/fast/html/na...
LayoutTests/fast/html/navigation-transition.html:39: <iframe id="testiframe"
frameBorder="0" style="width:100%;height:600px;background:gray">
On 2014/06/13 08:47:05, esprehn wrote:
> border: none; lets not use presentational markup unless you explicitly are
> trying to test that.

Done.

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

https://codereview.chromium.org/281383006/diff/80001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5681: RefPtr<HTMLCollection> metaElements =
getElementsByTagName(HTMLNames::metaTag.localName());
On 2014/06/13 08:47:06, esprehn wrote:
> We don't normally allow code like this in core/, except in Internals.

Replaced getElementsByTagName with Traversal<HTMLMetaElement>.

https://codereview.chromium.org/281383006/diff/80001/Source/core/editing/Edit...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/281383006/diff/80001/Source/core/editing/Edit...
Source/core/editing/EditingStyle.cpp:1254: height -= renderBox->paddingTop() +
renderBox->paddingBottom() + renderBox->borderHeight();
On 2014/06/13 08:47:06, esprehn wrote:
> This doesn't work if you set box-sizing, I really think you want to just set
> box-sizing to border-box here and then copy the padding and margins over.

Cool, setting just border-box seems to work well (padding and margins should be
carried over already).

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

https://codereview.chromium.org/281383006/diff/80001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2381:
frame()->document()->getTransitionElementData(&elementData, exceptionState);
On 2014/06/13 08:47:06, esprehn wrote:
> pass vector by reference.

Done.

https://codereview.chromium.org/281383006/diff/80001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2387: }
On 2014/06/13 08:47:06, esprehn wrote:
> no braces.

Done.

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

Slight update based on https://codereview.chromium.org/316053007/ :
createStyledMarkupForNavigationTransition() now includes some HTML header stuff
in the generated markup string.

[oystein (OOO til 10th of July), 2014-06-19 17:15:56]

esprehn: ping :)

[oystein (OOO til 10th of July), 2014-06-23 19:13:46]

On 2014/06/19 17:15:56, Oystein wrote:
> esprehn: ping :)

re-ping!

[esprehn, 2014-06-26 09:32:56]

This approach of trying to blacklist CSS properties and mess with positioning is
super brittle, I don't think it's going to result in something you can spec
well.

https://codereview.chromium.org/281383006/diff/140001/LayoutTests/fast/html/n...
File LayoutTests/fast/html/navigation-transition.html (right):

https://codereview.chromium.org/281383006/diff/140001/LayoutTests/fast/html/n...
LayoutTests/fast/html/navigation-transition.html:10: function onLoadedImage() {
onload = function(),

you don't need to wait for the image, just wait for the whole page.

https://codereview.chromium.org/281383006/diff/140001/LayoutTests/fast/html/n...
LayoutTests/fast/html/navigation-transition.html:37: <div
id="boxTransitionElement"
style="background:red;padding-left:18px;padding-top:8px;padding-bottom:50x;padding-right:50px;
Can we use a <style> block instead of inline style?

https://codereview.chromium.org/281383006/diff/140001/LayoutTests/fast/html/n...
LayoutTests/fast/html/navigation-transition.html:46: <div>This should NOT be
displayed.</div>
iframe will never show it's children, I'm not sure what you're testing here?

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

https://codereview.chromium.org/281383006/diff/140001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5721: for (HTMLMetaElement* metaElement = head() ?
Traversal<HTMLMetaElement>::firstChild(*head()) : 0; metaElement; metaElement =
Traversal<HTMLMetaElement>::nextSibling(*metaElement)) {
Lets early return instead.

if (!head())
  return;

https://codereview.chromium.org/281383006/diff/140001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5726: metaElement->content().string().split(';',
tokens);
";" is valid in CSS selectors, I don't think you can do this. ex.
element[attr=";"]. Instead you'll need to get the first index of ";" and then
split take everything before and everything after.

https://codereview.chromium.org/281383006/diff/140001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5730: RefPtr<NodeList> nodeList =
querySelectorAll(AtomicString(tokens[0]), exceptionState);
What happens if the elements you select are nested? Your code is going to make
them into siblings below which is pretty crazy. Your spec is going to need to be
very specific about what you do in here and the CSS handling below.

https://codereview.chromium.org/281383006/diff/140001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5736: for (unsigned nodeIdx = 0; nodeIdx <
nodeListLength; ++nodeIdx) {
nodeIndex, don't abbreviate.

https://codereview.chromium.org/281383006/diff/140001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5738: markup = markup +
createStyledMarkupForNavigationTransition(node);
I think there's a string builder for this kind of thing.

https://codereview.chromium.org/281383006/diff/140001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5741: if (!markup.isEmpty()) {
How can markup be empty if nodeList was non empty? I suspect you can early
return above instead.

https://codereview.chromium.org/281383006/diff/140001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5743: newElements.scope = tokens[1];
You'll want to trim() this.

https://codereview.chromium.org/281383006/diff/140001/Source/core/editing/Edi...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/281383006/diff/140001/Source/core/editing/Edi...
Source/core/editing/EditingStyle.cpp:1253:
m_mutableStyle->setProperty(CSSPropertyBoxSizing, CSSValueBorderBox);
The spec is going to have to be really clear about all the stuff you're doing
down here for implementations to be interoperable.

https://codereview.chromium.org/281383006/diff/140001/Source/core/editing/mar...
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/281383006/diff/140001/Source/core/editing/mar...
Source/core/editing/markup.cpp:320: if (&element == m_highestNodeToBeSerialized
&& m_shouldAnnotate == AnnotateForNavigationTransition)
You don't need the &

https://codereview.chromium.org/281383006/diff/140001/Source/core/editing/mar...
Source/core/editing/markup.cpp:352:
m_wrappingStyle->style()->removeProperty(CSSPropertyBackground);
This doesn't work, background is a shorthand so you're not really doing anything
here. If you set only background-color or background-image this will miss that.

https://codereview.chromium.org/281383006/diff/140001/Source/core/editing/mar...
Source/core/editing/markup.cpp:1098: static const char* documentMarkup =
"<!DOCTYPE html><meta name=\"viewport\" content=\"width=device-width,
user-scalable=0\">";
Why not use the meta viewport from the original document? You don't have to in
this patch, but I suspect what you have here isn't going to be correct for a
bunch of pages.

https://codereview.chromium.org/281383006/diff/140001/Source/core/editing/mar...
File Source/core/editing/markup.h (right):

https://codereview.chromium.org/281383006/diff/140001/Source/core/editing/mar...
Source/core/editing/markup.h:69: String
createStyledMarkupForNavigationTransition(Node*);
const Node* ? Why do you need to mutate it?

[oystein (OOO til 10th of July), 2014-06-26 23:13:43]

New version up; ptal.

On 2014/06/26 09:32:56, esprehn wrote:
> This approach of trying to blacklist CSS properties and mess with positioning
is
> super brittle, I don't think it's going to result in something you can spec
> well.

Sure. No better approaches have been suggested so far, so it's what we have to
deal with.

https://codereview.chromium.org/281383006/diff/140001/LayoutTests/fast/html/n...
File LayoutTests/fast/html/navigation-transition.html (right):

https://codereview.chromium.org/281383006/diff/140001/LayoutTests/fast/html/n...
LayoutTests/fast/html/navigation-transition.html:10: function onLoadedImage() {
On 2014/06/26 09:32:55, esprehn wrote:
> onload = function(),
> 
> you don't need to wait for the image, just wait for the whole page.

Done.

https://codereview.chromium.org/281383006/diff/140001/LayoutTests/fast/html/n...
LayoutTests/fast/html/navigation-transition.html:37: <div
id="boxTransitionElement"
style="background:red;padding-left:18px;padding-top:8px;padding-bottom:50x;padding-right:50px;
On 2014/06/26 09:32:55, esprehn wrote:
> Can we use a <style> block instead of inline style?

Done.

https://codereview.chromium.org/281383006/diff/140001/LayoutTests/fast/html/n...
LayoutTests/fast/html/navigation-transition.html:46: <div>This should NOT be
displayed.</div>
On 2014/06/26 09:32:55, esprehn wrote:
> iframe will never show it's children, I'm not sure what you're testing here?

Done.

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

https://codereview.chromium.org/281383006/diff/140001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5721: for (HTMLMetaElement* metaElement = head() ?
Traversal<HTMLMetaElement>::firstChild(*head()) : 0; metaElement; metaElement =
Traversal<HTMLMetaElement>::nextSibling(*metaElement)) {
On 2014/06/26 09:32:55, esprehn wrote:
> Lets early return instead.
> 
> if (!head())
>   return;

Done.

https://codereview.chromium.org/281383006/diff/140001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5726: metaElement->content().string().split(';',
tokens);
On 2014/06/26 09:32:55, esprehn wrote:
> ";" is valid in CSS selectors, I don't think you can do this. ex.
> element[attr=";"]. Instead you'll need to get the first index of ";" and then
> split take everything before and everything after.

Done.

https://codereview.chromium.org/281383006/diff/140001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5730: RefPtr<NodeList> nodeList =
querySelectorAll(AtomicString(tokens[0]), exceptionState);
On 2014/06/26 09:32:55, esprehn wrote:
> What happens if the elements you select are nested? Your code is going to make
> them into siblings below which is pretty crazy. Your spec is going to need to
be
> very specific about what you do in here and the CSS handling below.

Sure, there'll be a few "doing this is undefined behavior, please don't" caveats
here.

https://codereview.chromium.org/281383006/diff/140001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5736: for (unsigned nodeIdx = 0; nodeIdx <
nodeListLength; ++nodeIdx) {
On 2014/06/26 09:32:55, esprehn wrote:
> nodeIndex, don't abbreviate.

Done.

https://codereview.chromium.org/281383006/diff/140001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5738: markup = markup +
createStyledMarkupForNavigationTransition(node);
On 2014/06/26 09:32:55, esprehn wrote:
> I think there's a string builder for this kind of thing.

Done.

https://codereview.chromium.org/281383006/diff/140001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5741: if (!markup.isEmpty()) {
On 2014/06/26 09:32:55, esprehn wrote:
> How can markup be empty if nodeList was non empty? I suspect you can early
> return above instead.

Done.

https://codereview.chromium.org/281383006/diff/140001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5743: newElements.scope = tokens[1];
On 2014/06/26 09:32:55, esprehn wrote:
> You'll want to trim() this.

Done.

https://codereview.chromium.org/281383006/diff/140001/Source/core/editing/mar...
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/281383006/diff/140001/Source/core/editing/mar...
Source/core/editing/markup.cpp:320: if (&element == m_highestNodeToBeSerialized
&& m_shouldAnnotate == AnnotateForNavigationTransition)
On 2014/06/26 09:32:55, esprehn wrote:
> You don't need the &

I do, m_highestNodeToBeSerialized is a RawPtrWillBeMember<Node> not a Dode*.

https://codereview.chromium.org/281383006/diff/140001/Source/core/editing/mar...
Source/core/editing/markup.cpp:352:
m_wrappingStyle->style()->removeProperty(CSSPropertyBackground);
On 2014/06/26 09:32:55, esprehn wrote:
> This doesn't work, background is a shorthand so you're not really doing
anything
> here. If you set only background-color or background-image this will miss
that.

Done.

https://codereview.chromium.org/281383006/diff/140001/Source/core/editing/mar...
Source/core/editing/markup.cpp:1098: static const char* documentMarkup =
"<!DOCTYPE html><meta name=\"viewport\" content=\"width=device-width,
user-scalable=0\">";
On 2014/06/26 09:32:55, esprehn wrote:
> Why not use the meta viewport from the original document? You don't have to in
> this patch, but I suspect what you have here isn't going to be correct for a
> bunch of pages.

The idea is that we'll enforce the whole navigation transitions functionality to
only be enabled when the viewport is width=device-width.

https://codereview.chromium.org/281383006/diff/140001/Source/core/editing/mar...
File Source/core/editing/markup.h (right):

https://codereview.chromium.org/281383006/diff/140001/Source/core/editing/mar...
Source/core/editing/markup.h:69: String
createStyledMarkupForNavigationTransition(Node*);
On 2014/06/26 09:32:55, esprehn wrote:
> const Node* ? Why do you need to mutate it?

StyledMarkupAccumulator::serializeNodes takes a non-const node; I'd rather not
do a const-correctness-yak-shave here.

[oystein (OOO til 10th of July), 2014-07-01 12:09:38]

esprehn: ping

[oystein (OOO til 10th of July), 2014-07-14 19:14:38]

esprehn: I'm back from vacation, would awesome if we could move forward on this
again :).

[esprehn, 2014-07-14 22:04:31]

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

https://codereview.chromium.org/281383006/diff/180001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5710: Vector<String> tokens;
Dead code?

https://codereview.chromium.org/281383006/diff/180001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5732: newElements.scope =
metaElementContents.substring(firstSemicolon + 1).stripWhiteSpace();
What happens if there's nothing after the ;, isn't this out of bounds?

https://codereview.chromium.org/281383006/diff/180001/Source/core/editing/mar...
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/281383006/diff/180001/Source/core/editing/mar...
Source/core/editing/markup.cpp:1109: StyledMarkupAccumulator accumulator(0,
ResolveAllURLs, AnnotateForNavigationTransition, nullptr, 0);
What updates the style of the page before you get here?

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

Thanks!

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

https://codereview.chromium.org/281383006/diff/180001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5710: Vector<String> tokens;
On 2014/07/14 22:04:31, esprehn wrote:
> Dead code?

Done.

https://codereview.chromium.org/281383006/diff/180001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5732: newElements.scope =
metaElementContents.substring(firstSemicolon + 1).stripWhiteSpace();
On 2014/07/14 22:04:31, esprehn wrote:
> What happens if there's nothing after the ;, isn't this out of bounds?

StringImpl::substring does a bounds check and returns an empty() string if OOB.

https://codereview.chromium.org/281383006/diff/180001/Source/core/editing/mar...
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/281383006/diff/180001/Source/core/editing/mar...
Source/core/editing/markup.cpp:1109: StyledMarkupAccumulator accumulator(0,
ResolveAllURLs, AnnotateForNavigationTransition, nullptr, 0);
On 2014/07/14 22:04:31, esprehn wrote:
> What updates the style of the page before you get here?

Not sure what you're asking here, exactly. If there's anything calling
recalcStyle if needed? The other places the markup serialization code is used
(like Element::innerHTML/outerHTML) doesn't seem to have anything like this.

[esprehn, 2014-07-14 22:47:20]

On 2014/07/14 at 22:44:57, oysteine wrote:
> Thanks!
> ...
> 
>
https://codereview.chromium.org/281383006/diff/180001/Source/core/editing/mar...
> Source/core/editing/markup.cpp:1109: StyledMarkupAccumulator accumulator(0,
ResolveAllURLs, AnnotateForNavigationTransition, nullptr, 0);
> On 2014/07/14 22:04:31, esprehn wrote:
> > What updates the style of the page before you get here?
> 
> Not sure what you're asking here, exactly. If there's anything calling
recalcStyle if needed? The other places the markup serialization code is used
(like Element::innerHTML/outerHTML) doesn't seem to have anything like this.

But you're reading out style information, which means you need to update the
style first.

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

On 2014/07/14 22:47:20, esprehn wrote:
> On 2014/07/14 at 22:44:57, oysteine wrote:
> > Thanks!
> > ...
> > 
> >
>
https://codereview.chromium.org/281383006/diff/180001/Source/core/editing/mar...
> > Source/core/editing/markup.cpp:1109: StyledMarkupAccumulator accumulator(0,
> ResolveAllURLs, AnnotateForNavigationTransition, nullptr, 0);
> > On 2014/07/14 22:04:31, esprehn wrote:
> > > What updates the style of the page before you get here?
> > 
> > Not sure what you're asking here, exactly. If there's anything calling
> recalcStyle if needed? The other places the markup serialization code is used
> (like Element::innerHTML/outerHTML) doesn't seem to have anything like this.
> 
> But you're reading out style information, which means you need to update the
> style first.

Good point; added a Document::updateLayoutIgnorePendingStylesheets call (found
another call site which uses this before setting up the
StyledMarkupAccumulator).

[oystein (OOO til 10th of July), 2014-07-16 18:49:40]

On 2014/07/14 22:57:40, Oystein wrote:
> On 2014/07/14 22:47:20, esprehn wrote:
> > On 2014/07/14 at 22:44:57, oysteine wrote:
> > > Thanks!
> > > ...
> > > 
> > >
> >
>
https://codereview.chromium.org/281383006/diff/180001/Source/core/editing/mar...
> > > Source/core/editing/markup.cpp:1109: StyledMarkupAccumulator
accumulator(0,
> > ResolveAllURLs, AnnotateForNavigationTransition, nullptr, 0);
> > > On 2014/07/14 22:04:31, esprehn wrote:
> > > > What updates the style of the page before you get here?
> > > 
> > > Not sure what you're asking here, exactly. If there's anything calling
> > recalcStyle if needed? The other places the markup serialization code is
used
> > (like Element::innerHTML/outerHTML) doesn't seem to have anything like this.
> > 
> > But you're reading out style information, which means you need to update the
> > style first.
> 
> Good point; added a Document::updateLayoutIgnorePendingStylesheets call (found
> another call site which uses this before setting up the
> StyledMarkupAccumulator).

ping!

[esprehn, 2014-07-17 19:10:43]

You don't want that ExceptionState argument, I would remove it and make one on
the stack inside the loop. Right now if any of the <meta>'s causes an exception
you skip all the other ones which is not how web things work in general. We also
don't generally use ExceptionState on internally exposed APIs.

Please fix that and the nits, then lgtm

https://codereview.chromium.org/281383006/diff/220001/LayoutTests/fast/html/n...
File LayoutTests/fast/html/navigation-transition.html (right):

https://codereview.chromium.org/281383006/diff/220001/LayoutTests/fast/html/n...
LayoutTests/fast/html/navigation-transition.html:43: padding-left: 30px;
You can combine all these as padding: 13px 20px 8px 30px;

https://codereview.chromium.org/281383006/diff/220001/LayoutTests/fast/html/n...
LayoutTests/fast/html/navigation-transition.html:50: margin-left: 30px;
ditto

https://codereview.chromium.org/281383006/diff/220001/LayoutTests/fast/html/n...
LayoutTests/fast/html/navigation-transition.html:68: margin-left: 135px;
ditto

https://codereview.chromium.org/281383006/diff/220001/LayoutTests/fast/html/n...
LayoutTests/fast/html/navigation-transition.html:75: padding-left: 100px;
ditto

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

https://codereview.chromium.org/281383006/diff/220001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5719: if (!nodeList ||
exceptionState.hadException())
You're reusing the same exceptionState repeatedly here, so once any of the
<meta>'s causes an exception you're going to keep doing continue so you might as
well "return". That said, I don't think that's the right behavior, you probably
want to remove the exceptionState argument, make one on the stack, and skip ones
that are invalid with the hadException() continue logic you have now.

https://codereview.chromium.org/281383006/diff/220001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/281383006/diff/220001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:2371: String markup;
StringBuilder?

[oystein (OOO til 10th of July), 2014-07-17 21:15:06]

Awesome; thanks esprehn!

https://codereview.chromium.org/281383006/diff/220001/LayoutTests/fast/html/n...
File LayoutTests/fast/html/navigation-transition.html (right):

https://codereview.chromium.org/281383006/diff/220001/LayoutTests/fast/html/n...
LayoutTests/fast/html/navigation-transition.html:43: padding-left: 30px;
On 2014/07/17 19:10:43, esprehn wrote:
> You can combine all these as padding: 13px 20px 8px 30px;

Done.

https://codereview.chromium.org/281383006/diff/220001/LayoutTests/fast/html/n...
LayoutTests/fast/html/navigation-transition.html:50: margin-left: 30px;
On 2014/07/17 19:10:43, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/281383006/diff/220001/LayoutTests/fast/html/n...
LayoutTests/fast/html/navigation-transition.html:68: margin-left: 135px;
On 2014/07/17 19:10:43, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/281383006/diff/220001/LayoutTests/fast/html/n...
LayoutTests/fast/html/navigation-transition.html:75: padding-left: 100px;
On 2014/07/17 19:10:43, esprehn wrote:
> ditto

Done.

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

https://codereview.chromium.org/281383006/diff/220001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5719: if (!nodeList ||
exceptionState.hadException())
On 2014/07/17 19:10:43, esprehn wrote:
> You're reusing the same exceptionState repeatedly here, so once any of the
> <meta>'s causes an exception you're going to keep doing continue so you might
as
> well "return". That said, I don't think that's the right behavior, you
probably
> want to remove the exceptionState argument, make one on the stack, and skip
ones
> that are invalid with the hadException() continue logic you have now.

Done.

https://codereview.chromium.org/281383006/diff/220001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/281383006/diff/220001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:2371: String markup;
On 2014/07/17 19:10:43, esprehn wrote:
> StringBuilder?

Done.

[oystein (OOO til 10th of July), 2014-07-17 21:16:36]

The CQ bit was checked by oysteine@chromium.org

[commit-bot: I haz the power, 2014-07-17 21:16:52]

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

[commit-bot: I haz the power, 2014-07-17 22:57:03]

Change committed as 178403