Issue 653223002: Navigation transitions (web to native app): Revert exit transition (Blink side)

Issue 653223002: Navigation transitions (web to native app): Revert exit transition (Blink side) (Closed)

Created:
6 years, 2 months ago by Zhen Wang

Modified:
6 years ago

Reviewers:
Nate Chapin

CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, dglazkov+blink, eae+blinkwatch, ed+blinkwatch_opera.com, gavinp+prerender_chromium.org, mkwst+moarreviews_chromium.org, rwlbuis, rune+blink, sof, Yoav Weiss

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

Project:
blink

Visibility:
Public.

More Reviews

Description

Navigation transitions (web to native app): Revert exit transition (Blink side) We need a way to revert exit transition and show hidden transition elements when coming back from native app (when back button is pressed). This is the Blink side of the CL. The Chrome side CL will be done after this CL is committed. https://codereview.chromium.org/775843004/ BUG=370696 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186641

Patch Set 1 #

Patch Set 2 : add tests #

Total comments: 36

Patch Set 3 : address review comments #

Patch Set 4 : Remove unnecessary const #

Patch Set 5 : rebase #

Patch Set 6 : rebase #

Patch Set 7 : rebase #

Patch Set 8 : rebase #

Patch Set 9 : rebase #

Patch Set 10 : resetIsTransitionDocument #

Patch Set 11 : #

Total comments: 4

Patch Set 12 : rebase #

Patch Set 13 : review fix #

Created: 6 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+107 lines, -15 lines)			Patch
A	LayoutTests/fast/dom/HTMLLinkElement/transition-exiting-stylesheet-enable-disable.html	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+42 lines, -0 lines	0 comments	Download
A +	LayoutTests/fast/dom/HTMLLinkElement/transition-exiting-stylesheet-enable-disable-expected.txt	View	1	1 chunk	+3 lines, -1 line	0 comments	Download
M	Source/core/dom/Document.h	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/dom/StyleEngine.h	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/dom/StyleEngine.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/dom/TreeScopeStyleSheetCollection.h	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/dom/TreeScopeStyleSheetCollection.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLLinkElement.h	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/HTMLLinkElement.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/loader/DocumentLoader.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/testing/Internals.h	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/testing/Internals.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+5 lines, -0 lines	0 comments	Download
M	Source/core/testing/Internals.idl	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/web/WebDocument.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12	2 chunks	+17 lines, -2 lines	0 comments	Download
M	Source/web/tests/WebDocumentTest.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+24 lines, -0 lines	0 comments	Download
M	Source/web/tests/WebFrameTest.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+1 line, -1 line	0 comments	Download
M	public/web/WebDocument.h	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 18 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

esprehn

https://codereview.chromium.org/653223002/diff/20001/LayoutTests/fast/dom/HTMLLinkElement/transition-exiting-stylesheet-enable-disable.html File LayoutTests/fast/dom/HTMLLinkElement/transition-exiting-stylesheet-enable-disable.html (right): https://codereview.chromium.org/653223002/diff/20001/LayoutTests/fast/dom/HTMLLinkElement/transition-exiting-stylesheet-enable-disable.html#newcode3 LayoutTests/fast/dom/HTMLLinkElement/transition-exiting-stylesheet-enable-disable.html:3: <head> leave out html, head and body. https://codereview.chromium.org/653223002/diff/20001/LayoutTests/fast/dom/HTMLLinkElement/transition-exiting-stylesheet-enable-disable.html#newcode7 LayoutTests/fast/dom/HTMLLinkElement/transition-exiting-stylesheet-enable-disable.html:7: ...

6 years, 2 months ago (2014-10-15 18:01:26 UTC) #3

Zhen Wang

https://codereview.chromium.org/653223002/diff/20001/LayoutTests/fast/dom/HTMLLinkElement/transition-exiting-stylesheet-enable-disable.html File LayoutTests/fast/dom/HTMLLinkElement/transition-exiting-stylesheet-enable-disable.html (right): https://codereview.chromium.org/653223002/diff/20001/LayoutTests/fast/dom/HTMLLinkElement/transition-exiting-stylesheet-enable-disable.html#newcode3 LayoutTests/fast/dom/HTMLLinkElement/transition-exiting-stylesheet-enable-disable.html:3: <head> On 2014/10/15 18:01:26, esprehn wrote: > leave out ...

6 years, 2 months ago (2014-10-15 19:50:58 UTC) #4

esprehn

https://codereview.chromium.org/653223002/diff/20001/Source/core/css/CSSStyleSheet.cpp File Source/core/css/CSSStyleSheet.cpp (right): https://codereview.chromium.org/653223002/diff/20001/Source/core/css/CSSStyleSheet.cpp#newcode235 Source/core/css/CSSStyleSheet.cpp:235: didMutate(EntireStyleSheetUpdate); On 2014/10/15 at 19:50:58, Zhen Wang wrote: > ...

6 years, 2 months ago (2014-10-15 20:19:07 UTC) #5

https://codereview.chromium.org/653223002/diff/20001/Source/core/css/CSSStyle...
File Source/core/css/CSSStyleSheet.cpp (right):

https://codereview.chromium.org/653223002/diff/20001/Source/core/css/CSSStyle...
Source/core/css/CSSStyleSheet.cpp:235: didMutate(EntireStyleSheetUpdate);
On 2014/10/15 at 19:50:58, Zhen Wang wrote:
> The default will lead to FullStyleUpdate. We need AnalyzedStyleUpdate. 
> 
> See the demo video in the CL description. I navigate from the page to a native
app and then press the back button to come back. If I don't treat transition
stylesheets specially here, the user interaction will not function properly
after coming back (cannot click to navigate any more.)
> 
> This is related to the comments in HTMLLinkElement.cpp.

This is not correct, I don't know what you're talking about. AnalyzedStyleUpdate
is just an attempt to do an optimization over the FullStyleUpdate. It's a subset
of the amount of work done, but they should have exactly the same end result. We
should be able to switch all AnalyzedStyleUpdate over to FullStyleUpdate and
have an engine that functions the same.

https://codereview.chromium.org/653223002/diff/20001/Source/core/html/HTMLLin...
File Source/core/html/HTMLLinkElement.cpp (right):

https://codereview.chromium.org/653223002/diff/20001/Source/core/html/HTMLLin...
Source/core/html/HTMLLinkElement.cpp:671:
m_owner->document().styleResolverChanged(AnalyzedStyleUpdate);
On 2014/10/15 at 19:50:58, Zhen Wang wrote:
> The default is FullStyleUpdate. We need AnalyzedStyleUpdate here. 
> 
> See the demo video in the CL description. I navigate from the page to a native
app and then press the back button to come back. If I don't treat transition
stylesheets specially here, the user interaction will not function properly
after coming back (cannot click to navigate any more.)
> 
> This is related to comments in CSSStyleSheet.cpp.

ditto.

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

https://codereview.chromium.org/653223002/diff/20001/public/web/WebDocument.h...
public/web/WebDocument.h:124: BLINK_EXPORT void beginExitTransition(const
WebString& cssSelector, const bool exitToActivity);
On 2014/10/15 at 19:50:58, Zhen Wang wrote:
> Why not const bool?
> 
> Are we encouraged to use default value in blink? I see somewhere else in
chrome side that people say try not to use default value.

Default values are fine in blink, and const bool and const double don't make any
sense, you're passing by value.

https://codereview.chromium.org/653223002/diff/20001/public/web/WebDocument.h...
public/web/WebDocument.h:126: BLINK_EXPORT void onTransitionElementOpacity(
On 2014/10/15 at 19:50:58, Zhen Wang wrote:
> The renderer side will call this function to hide/show the transition elements
(not together with beginExitTransition or revertExitTransition). So I think we
need to export it.

It's not an okay API to expose.

https://codereview.chromium.org/653223002/diff/20001/public/web/WebDocument.h...
public/web/WebDocument.h:127: const WebString& cssSelector, const double
opacity);
On 2014/10/15 at 19:50:58, Zhen Wang wrote:
> Why not?

You're passing by value, it doesn't make sense. What are you telling the caller
about the function by marking it const?

Zhen Wang

6 years, 2 months ago (2014-10-15 23:04:42 UTC) #6

esprehn

We don't do const for pass by value properties in blink, it's just line noise. ...

6 years, 2 months ago (2014-10-15 23:08:10 UTC) #7

Zhen Wang

I removed const and use the default value now. For the style hack, I see ...

6 years, 2 months ago (2014-10-16 00:03:54 UTC) #8

Zhen Wang

Hi Nate, can you take a look since you are already familiar with the context? ...

6 years ago (2014-12-03 17:54:29 UTC) #11

Nate Chapin

LGTM with a couple formatting questions https://codereview.chromium.org/653223002/diff/200001/Source/core/dom/Document.h File Source/core/dom/Document.h (right): https://codereview.chromium.org/653223002/diff/200001/Source/core/dom/Document.h#newcode388 Source/core/dom/Document.h:388: void setIsTransitionDocument() { ...

6 years ago (2014-12-05 23:11:37 UTC) #12

Zhen Wang

https://codereview.chromium.org/653223002/diff/200001/Source/core/dom/Document.h File Source/core/dom/Document.h (right): https://codereview.chromium.org/653223002/diff/200001/Source/core/dom/Document.h#newcode388 Source/core/dom/Document.h:388: void setIsTransitionDocument() { m_isTransitionDocument = true; } On 2014/12/05 ...

6 years ago (2014-12-06 00:57:37 UTC) #13

Zhen Wang

Hi Nate, I made many format changes. Can you take a look before I commit ...

6 years ago (2014-12-06 00:58:57 UTC) #14

commit-bot: I haz the power

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

6 years ago (2014-12-06 05:14:24 UTC) #17

commit-bot: I haz the power

6 years ago (2014-12-06 06:29:30 UTC) #18

Message was sent while issue was closed.

Committed patchset #13 (id:240001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186641

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages