Issue 316053007: Navigation transitions: Added notifyTransitionsShown and setupTransitionsView to WebLocalFrame

Issue 316053007: Navigation transitions: Added notifyTransitionsShown and setupTransitionsView to WebLocalFrame (Closed)

Created:
6 years, 6 months ago by oystein (OOO til 10th of July)

Modified:
6 years, 4 months ago

Reviewers:
abarth-chromium, esprehn, dcheng, eseidel

CC:
abarth-chromium, blink-reviews, dglazkov+blink, jamesr, shatch, site-isolation-dev_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Visibility:
Public.

More Reviews

Description

Navigation transitions: Added notifyTransitionsShown and setupTransitionsView to WebLocalFrame The former is called on the outgoing page, and will hide any transition elements and enable any exit transition animations. The latter is called on the transition layer, and will replace the document (about:blank) with the serialized markup, enforce sandboxing, and turn off script execution. R=eseidel,esprehn BUG=370696 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179431

Patch Set 1 : #

Total comments: 23

Patch Set 2 : Review fixes #

Total comments: 14

Patch Set 3 : #

Patch Set 4 : #

Total comments: 7

Patch Set 5 : Review fixes #

Total comments: 4

Patch Set 6 : Review fix #

Created: 6 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+118 lines, -0 lines)			Patch
M	Source/core/dom/Document.h	View	1 2 3 4 5	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/dom/Document.cpp	View	1 2 3 4 5	1 chunk	+14 lines, -0 lines	0 comments	Download
M	Source/core/loader/DocumentLoader.cpp	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/web/WebDocument.cpp	View	1 2 3 4 5	1 chunk	+13 lines, -0 lines	0 comments	Download
M	Source/web/WebLocalFrameImpl.h	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/web/WebLocalFrameImpl.cpp	View	1 2 3 4 5	1 chunk	+7 lines, -0 lines	0 comments	Download
M	Source/web/tests/WebDocumentTest.cpp	View	1 2 3 4	2 chunks	+46 lines, -0 lines	0 comments	Download
M	Source/web/tests/WebFrameTest.cpp	View	1 2 3 4 5	2 chunks	+22 lines, -0 lines	0 comments	Download
A	Source/web/tests/data/transition_exit.css	View	1 2 3 4	1 chunk	+3 lines, -0 lines	0 comments	Download
A	Source/web/tests/data/transition_exit.html	View	1 2 3 4	1 chunk	+6 lines, -0 lines	0 comments	Download
M	public/web/WebDocument.h	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
M	public/web/WebLocalFrame.h	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 22 (0 generated)

Expand Messages | Collapse Messages

oystein (OOO til 10th of July)

Two more pieces of the puzzle; Blink endpoints which are called on the outgoing document ...

6 years, 6 months ago (2014-06-05 01:14:04 UTC) #1

esprehn

https://codereview.chromium.org/316053007/diff/20001/Source/web/WebLocalFrameImpl.cpp File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/316053007/diff/20001/Source/web/WebLocalFrameImpl.cpp#newcode1300 Source/web/WebLocalFrameImpl.cpp:1300: RefPtr<HTMLCollection> metaElements = frame()->document()->getElementsByTagName(HTMLNames::metaTag.localName()); We don't use node lists ...

6 years, 6 months ago (2014-06-13 08:55:20 UTC) #2

esprehn

You should talk to abarth@ about how this integrates, I don't think the FrameLoader logic ...

6 years, 6 months ago (2014-06-13 08:55:42 UTC) #3

abarth-chromium

https://codereview.chromium.org/316053007/diff/20001/Source/web/WebLocalFrameImpl.cpp File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/316053007/diff/20001/Source/web/WebLocalFrameImpl.cpp#newcode1298 Source/web/WebLocalFrameImpl.cpp:1298: void WebLocalFrameImpl::notifyTransitionShown(int elementsToHide) This is way too much logic ...

6 years, 6 months ago (2014-06-13 17:29:01 UTC) #4

dcheng

https://codereview.chromium.org/316053007/diff/20001/public/web/WebFrame.h File public/web/WebFrame.h (right): https://codereview.chromium.org/316053007/diff/20001/public/web/WebFrame.h#newcode475 public/web/WebFrame.h:475: virtual void setupTransitionView(const WebString& markup) = 0; I don't ...

6 years, 6 months ago (2014-06-18 18:12:18 UTC) #5

oystein (OOO til 10th of July)

Second version up; ptal :) https://codereview.chromium.org/316053007/diff/20001/Source/web/WebLocalFrameImpl.cpp File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/316053007/diff/20001/Source/web/WebLocalFrameImpl.cpp#newcode1300 Source/web/WebLocalFrameImpl.cpp:1300: RefPtr<HTMLCollection> metaElements = frame()->document()->getElementsByTagName(HTMLNames::metaTag.localName()); ...

6 years, 6 months ago (2014-06-18 23:37:42 UTC) #6

Second version up; ptal :)

https://codereview.chromium.org/316053007/diff/20001/Source/web/WebLocalFrame...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/316053007/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1300: RefPtr<HTMLCollection> metaElements =
frame()->document()->getElementsByTagName(HTMLNames::metaTag.localName());
On 2014/06/13 08:55:20, esprehn wrote:
> We don't use node lists of querySelector inside Source/ like this. For example
> your code below makes us walk the entire document twice looking for <meta>
> elements. You need to keep track the meta elements for transitions in a Vector
> in document using ::insertedInto and attributeChanged notifications in
> HTMLMetaElement.

Somewhat improved now. Is it worth using ::insertedInto and attributeChanged to
keep track a runtime list of the meta elements though, when we at most do this
twice per document (the ones which have navigation transitions defined?

https://codereview.chromium.org/316053007/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1303: unsigned length = metaElements->length();
On 2014/06/13 08:55:20, esprehn wrote:
> Touching length() walks the entire document, then you walk it again below.

Done.

https://codereview.chromium.org/316053007/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1311:
String(metaElement->content().impl()).split(';', tokens);
On 2014/06/13 08:55:20, esprehn wrote:
> metaElement->content().string().split(...)

Done.

https://codereview.chromium.org/316053007/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1324:
toElement(node)->setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone);
On 2014/06/13 08:55:19, esprehn wrote:
> Modifying parts of the page like this doesn't seem right.

Is there a better way of hiding these elements?

https://codereview.chromium.org/316053007/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1332:
frame()->document()->styleEngine()->enableExitTransitionStylesheets();
On 2014/06/13 08:55:19, esprehn wrote:
> You can't do this, closeURL() shuts the FrameLoader down and dispatches unload
> events. I don't think you can then make us do recalcStyle and such later on
the
> same document.

I believe dispatching the unload events first was actually the idea here, so
that we wouldn't disrupt the normal page flow when we hide the transition
elements and apply any exit animations (from any transition-exiting
stylesheets). That still works, so I assume recalcStyle still works :).

https://codereview.chromium.org/316053007/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1341:
frame()->document()->enforceSandboxFlags(SandboxAll);
On 2014/06/13 17:29:01, abarth wrote:
> Can you assert that document()->securityOrigin()->isUnique() here?  I'm not
sure
> you're successfully gotten a unique security origin.

Done, though I'm not quite sure what I'm doing here. The page URL is basically
just about:blank, should we be using some other URL here with the domain we're
navigating to?

https://codereview.chromium.org/316053007/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1342:
frame()->page()->settings().setScriptEnabled(false);
On 2014/06/13 17:29:01, abarth wrote:
> This line is redundant with enforceSandboxFlags(SandboxAll);

Done.

https://codereview.chromium.org/316053007/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1346: String tempMarkup = String("<!DOCTYPE
html><meta name=\"viewport\" content=\"width=device-width, user-scalable=0\">")
+ String(markup);
On 2014/06/13 17:29:01, abarth wrote:
> This should be included in |markup|.  I'm not sure where |markup| is
generated,
> but whoever generates it should generate the complete string, including the
> doctype and the viewport.

Done.

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

https://codereview.chromium.org/316053007/diff/20001/public/web/WebFrame.h#ne...
public/web/WebFrame.h:475: virtual void setupTransitionView(const WebString&
markup) = 0;
On 2014/06/18 18:12:17, dcheng wrote:
> I don't think you'd ever want to call this on a remote frame? As such, I think
> it'd be more appropriate to only add this API to WebLocalFrame.h.

Done.

https://codereview.chromium.org/316053007/diff/20001/public/web/WebFrame.h#ne...
public/web/WebFrame.h:475: virtual void setupTransitionView(const WebString&
markup) = 0;
On 2014/06/13 17:29:01, abarth wrote:
> I'm not sure this is the right way to load this content into the frame.
> 
> Rather than replacing the content of the document (always dicey), you should
> trigger a navigation of the frame to a new document with replacement data
> consisting of your markup.  Specifically, I think you want to call something
> like WebFrame::loadHTMLString.  Perhaps a function like
> WebFrame::loadTransitionHTML that calls WebFrame::loadData under the hood,
like
> WebFrame::loadHTMLString calls WebFrame::loadData?  Notice that
> WebFrame::loadData loads the data via the FrameLoader so that the rest of the
> system works correctly.

Done! Right now I just call loadData() from setupTransitionView() though, should
it be moved to some intermediate function?

oystein (OOO til 10th of July)

You get a ping! And you get a ping! Everyone gets a ping!

6 years, 6 months ago (2014-06-20 22:57:47 UTC) #7

abarth-chromium

This CL has no tests. https://codereview.chromium.org/316053007/diff/60001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/316053007/diff/60001/Source/core/dom/Document.cpp#newcode5736 Source/core/dom/Document.cpp:5736: continue; What if the ...

6 years, 6 months ago (2014-06-21 05:38:37 UTC) #8

oystein (OOO til 10th of July)

Apologies for taking so long to get back to this one; had a few other ...

6 years, 5 months ago (2014-07-22 21:17:21 UTC) #9

dcheng

Out of curiosity, what happens today if you try to use navigation transitions during a ...

6 years, 5 months ago (2014-07-22 21:24:58 UTC) #10

oystein (OOO til 10th of July)

On 2014/07/22 21:24:58, dcheng (OOO) wrote: > Out of curiosity, what happens today if you ...

6 years, 5 months ago (2014-07-22 21:29:10 UTC) #11

oystein (OOO til 10th of July)

abarth: ping :). Does this look more sensible now, and does it make sense for ...

6 years, 4 months ago (2014-07-28 20:40:48 UTC) #12

abarth-chromium

https://codereview.chromium.org/316053007/diff/140001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/316053007/diff/140001/Source/core/dom/Document.cpp#newcode5740 Source/core/dom/Document.cpp:5740: RefPtr<StaticNodeList> nodeList = querySelectorAll(AtomicString(cssSelector), exceptionState); If we're going to ...

6 years, 4 months ago (2014-07-29 17:07:16 UTC) #13

oystein (OOO til 10th of July)

New version up, with unit tests added. PTAL :) https://codereview.chromium.org/316053007/diff/140001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/316053007/diff/140001/Source/core/dom/Document.cpp#newcode5740 Source/core/dom/Document.cpp:5740: ...

6 years, 4 months ago (2014-07-30 22:11:51 UTC) #14

oystein (OOO til 10th of July)

abarth: ping :) On 2014/07/30 22:11:51, Oystein wrote: > New version up, with unit tests ...

6 years, 4 months ago (2014-08-01 22:23:51 UTC) #15

abarth-chromium

LGTM! https://codereview.chromium.org/316053007/diff/180001/Source/web/WebDocument.cpp File Source/web/WebDocument.cpp (right): https://codereview.chromium.org/316053007/diff/180001/Source/web/WebDocument.cpp#newcode276 Source/web/WebDocument.cpp:276: Document* document = unwrap<Document>(); RefPtr<Document> I don't think ...

6 years, 4 months ago (2014-08-01 22:27:32 UTC) #16

oystein (OOO til 10th of July)

Great, thanks! :) https://codereview.chromium.org/316053007/diff/180001/Source/web/WebDocument.cpp File Source/web/WebDocument.cpp (right): https://codereview.chromium.org/316053007/diff/180001/Source/web/WebDocument.cpp#newcode276 Source/web/WebDocument.cpp:276: Document* document = unwrap<Document>(); On 2014/08/01 ...

6 years, 4 months ago (2014-08-01 22:57:06 UTC) #18

oystein (OOO til 10th of July)

The CQ bit was checked by oysteine@chromium.org

6 years, 4 months ago (2014-08-01 22:57:23 UTC) #19

commit-bot: I haz the power

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

6 years, 4 months ago (2014-08-01 22:58:15 UTC) #20

commit-bot: I haz the power

FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink ...

6 years, 4 months ago (2014-08-02 00:28:16 UTC) #21

Message was sent while issue was closed.

Change committed as 179431

Expand Messages | Collapse Messages