Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(214)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 23972003: Update containtingIsolate to go back all the way to top isolate from current (Closed)

Created:
7 years, 3 months ago by jww
Modified:
7 years, 3 months ago
CC:
blink-reviews, dglazkov+blink, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, eseidel, Mike West
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Update containtingIsolate to go back all the way to top isolate from current root, rather than stopping at the first isolate it finds. This works because the current root is always updated with each isolate run. BUG=279277 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157268

Patch Set 1 #

Total comments: 12

Patch Set 2 : Fixed nits from leviw and igor #

Patch Set 3 : Reverted assert to normal ASSERT, rather than ASSERT_WITH_SECURITY_IMPLICATION #

Messages

Total messages: 10 (0 generated)
jww
7 years, 3 months ago (2013-09-04 21:53:08 UTC) #1
leviw_travelin_and_unemployed
Great looking patch! https://codereview.chromium.org/23972003/diff/1/LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html File LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html (right): https://codereview.chromium.org/23972003/diff/1/LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html#newcode1 LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html:1: <!-- This tests for regression of ...
7 years, 3 months ago (2013-09-04 22:00:24 UTC) #2
igoroliveira
Looks awesome! https://codereview.chromium.org/23972003/diff/1/Source/core/rendering/RenderBlockLineLayout.cpp File Source/core/rendering/RenderBlockLineLayout.cpp (right): https://codereview.chromium.org/23972003/diff/1/Source/core/rendering/RenderBlockLineLayout.cpp#newcode1321 Source/core/rendering/RenderBlockLineLayout.cpp:1321: ASSERT(isolatedInline); ASSERT_WITH_SECURITY_IMPLICATION maybe?
7 years, 3 months ago (2013-09-04 22:04:56 UTC) #3
jww
https://codereview.chromium.org/23972003/diff/1/LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html File LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html (right): https://codereview.chromium.org/23972003/diff/1/LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html#newcode1 LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html:1: <!-- This tests for regression of https://crbug.com/279277 where non-adjacent, ...
7 years, 3 months ago (2013-09-04 22:37:25 UTC) #4
leviw_travelin_and_unemployed
lgtm with those nits https://codereview.chromium.org/23972003/diff/1/LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html File LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html (right): https://codereview.chromium.org/23972003/diff/1/LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html#newcode1 LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html:1: <!-- This tests for regression ...
7 years, 3 months ago (2013-09-04 22:46:25 UTC) #5
jww
https://codereview.chromium.org/23972003/diff/1/LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html File LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html (right): https://codereview.chromium.org/23972003/diff/1/LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html#newcode1 LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html:1: <!-- This tests for regression of https://crbug.com/279277 where non-adjacent, ...
7 years, 3 months ago (2013-09-04 23:12:15 UTC) #6
jww
On 2013/09/04 23:12:15, jww wrote: > https://codereview.chromium.org/23972003/diff/1/LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html > File > LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html > (right): > > ...
7 years, 3 months ago (2013-09-04 23:15:08 UTC) #7
igoroliveira
On 2013/09/04 23:15:08, jww wrote: > On 2013/09/04 23:12:15, jww wrote: > > > https://codereview.chromium.org/23972003/diff/1/LayoutTests/fast/text/international/unicode-bidi-isolate-nested-with-removes-not-adjacent.html ...
7 years, 3 months ago (2013-09-04 23:16:52 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/23972003/14001
7 years, 3 months ago (2013-09-04 23:24:13 UTC) #9
commit-bot: I haz the power
7 years, 3 months ago (2013-09-05 03:54:12 UTC) #10
Message was sent while issue was closed. 
Change committed as 157268

Powered by Google App Engine
This is Rietveld 408576698