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

Issue 2269043002: Reland of Remove EventSender from HTMLStyleElement (Closed)

Created:
4 years, 4 months ago by hiroshige
Modified:
4 years, 1 month ago
CC:
chromium-reviews, blink-reviews-html_chromium.org, blink-reviews-style_chromium.org, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews, rwlbuis, loading-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Reland of Remove EventSender from HTMLStyleElement This CL replaces EventSender in HTMLLinkElement with postTask(): Instead of calling HTMLStyleElement::dispatchPendingLoadEvents() in Document::implicitClose() to enforce <style>'s onload events to be executed before the document's onload event, this CL blocks document's onload until <style>'s onload event using IncrementLoadEventDelayCount. This CL changes the timing of body onload, particularly causes some of the following events that were previously called AFTER document onload to be called BEFORE document onload: [A] The first layout. [B] Latter half of FrameLoader::finishedParsing(). [C] JavaScript function |f| scheduled as |setTimeout(f, 0)| in the <script> at the end of <body>. Notable test fixes (marked with the causes of failures): fast/text/international/inline-plaintext-relayout-with-leading-neutrals.html: [A] Marked as flaky, because it depends on whether body onload is called before (Pass) or after (Failure) the first layout (crbug/651343). fast/scrolling/scrollbar-tickmarks-styled.html: [A] Fixed by https://codereview.chromium.org/2450793002. fast/scrolling/scrollbar-tickmarks-hittest.html: [A] Fixed by https://codereview.chromium.org/2484613003. fast/forms/text/text-set-selection-crash.html: [A] There is race between "Blink Test Plugin: initializing" console log (which is asynchronously triggered by the first layout) and document onload (=test finish). Because the test should pass if not crashes, this CL make the test to ignore the log, by converting the test to use testharness.js and removing the -expected.txt. virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html: [B] Fixed by https://codereview.chromium.org/2467433002. svg/animations/animate-end-attribute-numeric-precision.html: [C] After this CL, calling executeTest() in svg/animations/script-tests/animate-end-attribute-numeric-precision.js causes sampleAnimation() to be called before document onload and test to fail. We set |animationStartsImmediately| true instead, in order to ensure the test is started in document onload. fast/harness/error-in-async-test.html: [C] Whether "Uncaught Exception" console log is shown depends on whether buggyAsyncCode() is executed before or after document onload. This CL deflakes the test by ensuring it is executed after document onload. accessibility/inline-text-changes.html: [Race between the accessibility notification and document onload] Ignore the second call of the notification handler. Even before this CL, the notification is called multiple times, but the first notification terminated the test immediately by finishJSTest() because it is called after document onload. This CL makes the notification calls before document onload, and thus the test continues until document onload and we have to ignore the second call explicitly. fast/block/float/marquee-shrink-to-avoid-floats.html: The layout tree changes slightly, but it looks like acceptable because the image expectation matches. BUG=624697, 651343 Committed: https://crrev.com/52d00fd56184deadce99ff4fe599156ea1da3543 Committed: https://crrev.com/b8cb16f79a95c8bc639a9c58785a8484c197aca9 Cr-Original-Commit-Position: refs/heads/master@{#430348} Cr-Commit-Position: refs/heads/master@{#430572}

Patch Set 1 #

Patch Set 2 : Block onload event #

Patch Set 3 : Blocking #

Patch Set 4 : not Blocking #

Patch Set 5 : Update test expectations #

Patch Set 6 : Rebase #

Patch Set 7 : Use DOMManipulation task runner #

Patch Set 8 : Rebase #

Patch Set 9 : Test fixes #

Patch Set 10 : Fix more tests. #

Patch Set 11 : Call restoreScrollPositionAndViewState() in finishedParsing() #

Patch Set 12 : Rebase #

Patch Set 13 : Fix #

Patch Set 14 : Fixes. #

Patch Set 15 : Rebase. #

Patch Set 16 : Rebase #

Patch Set 17 : minor refinement #

Patch Set 18 : Run test in onload() #

Patch Set 19 : Rebase #

Patch Set 20 : Rebase (First Commit) #

Patch Set 21 : Deflake #

Total comments: 2

Patch Set 22 : remove t.step() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+41 lines, -46 lines) Patch
M third_party/WebKit/LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/accessibility/inline-text-changes.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +10 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/animations/animation-duration-infinite.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +5 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/block/float/marquee-shrink-to-avoid-floats-expected.txt View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/dom/HTMLDialogElement/inert-node-is-unfocusable-expected.txt View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/forms/text/text-set-selection-crash.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +5 lines, -5 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/forms/text/text-set-selection-crash-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +0 lines, -10 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/harness/error-in-async-test.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/loading/preload-img-test-expected.txt View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/svg/animations/script-tests/animate-end-attribute-numeric-precision.js View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +0 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLStyleElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +3 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLStyleElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +9 lines, -14 lines 0 comments Download

Messages

Total messages: 94 (74 generated)
hiroshige
PTAL. kojii@ for LayoutTests/TestExpectations haraken@ for other files as a core/ OWNER.
4 years, 1 month ago (2016-10-31 09:35:26 UTC) #32
haraken
On 2016/10/31 09:35:26, hiroshige wrote: > PTAL. > > kojii@ for LayoutTests/TestExpectations > haraken@ for ...
4 years, 1 month ago (2016-10-31 10:13:29 UTC) #33
hiroshige
+japhet@, yhirano@. Could you take a look? (although changes are not in core/fetch|loader, this CL ...
4 years, 1 month ago (2016-10-31 16:05:44 UTC) #39
yhirano
lgtm
4 years, 1 month ago (2016-11-01 07:08:55 UTC) #42
haraken
LGTM
4 years, 1 month ago (2016-11-01 07:22:03 UTC) #43
kojii
LayoutTests/TestExpectations lgtm
4 years, 1 month ago (2016-11-01 07:25:03 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2269043002/360001
4 years, 1 month ago (2016-11-07 09:23:25 UTC) #51
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/332185)
4 years, 1 month ago (2016-11-07 10:27:51 UTC) #53
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2269043002/380001
4 years, 1 month ago (2016-11-07 16:15:20 UTC) #61
commit-bot: I haz the power
Committed patchset #20 (id:380001)
4 years, 1 month ago (2016-11-07 19:37:23 UTC) #63
commit-bot: I haz the power
Patchset 20 (id:??) landed as https://crrev.com/52d00fd56184deadce99ff4fe599156ea1da3543 Cr-Commit-Position: refs/heads/master@{#430348}
4 years, 1 month ago (2016-11-07 19:46:44 UTC) #65
xlai (Olivia)
A revert of this CL (patchset #20 id:380001) has been created in https://codereview.chromium.org/2487433002/ by xlai@chromium.org. ...
4 years, 1 month ago (2016-11-07 20:41:00 UTC) #66
hiroshige
yhirano@, could you take a look again? Patch Set 21 deflakes the following tests: fast/forms/text/text-set-selection-crash.html ...
4 years, 1 month ago (2016-11-08 05:29:48 UTC) #80
yhirano
https://codereview.chromium.org/2269043002/diff/420001/third_party/WebKit/LayoutTests/fast/forms/text/text-set-selection-crash.html File third_party/WebKit/LayoutTests/fast/forms/text/text-set-selection-crash.html (right): https://codereview.chromium.org/2269043002/diff/420001/third_party/WebKit/LayoutTests/fast/forms/text/text-set-selection-crash.html#newcode8 third_party/WebKit/LayoutTests/fast/forms/text/text-set-selection-crash.html:8: t.step(function() { Can you tell me why this |t.step| ...
4 years, 1 month ago (2016-11-08 06:25:54 UTC) #81
hiroshige
https://codereview.chromium.org/2269043002/diff/420001/third_party/WebKit/LayoutTests/fast/forms/text/text-set-selection-crash.html File third_party/WebKit/LayoutTests/fast/forms/text/text-set-selection-crash.html (right): https://codereview.chromium.org/2269043002/diff/420001/third_party/WebKit/LayoutTests/fast/forms/text/text-set-selection-crash.html#newcode8 third_party/WebKit/LayoutTests/fast/forms/text/text-set-selection-crash.html:8: t.step(function() { On 2016/11/08 06:25:54, yhirano wrote: > Can ...
4 years, 1 month ago (2016-11-08 06:38:37 UTC) #83
yhirano
lgtm
4 years, 1 month ago (2016-11-08 06:53:35 UTC) #85
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2269043002/440001
4 years, 1 month ago (2016-11-08 07:49:35 UTC) #89
commit-bot: I haz the power
Committed patchset #22 (id:440001)
4 years, 1 month ago (2016-11-08 10:54:15 UTC) #91
commit-bot: I haz the power
Patchset 22 (id:??) landed as https://crrev.com/b8cb16f79a95c8bc639a9c58785a8484c197aca9 Cr-Commit-Position: refs/heads/master@{#430572}
4 years, 1 month ago (2016-11-08 10:56:08 UTC) #93
hiroshige
4 years, 1 month ago (2016-11-08 11:53:02 UTC) #94
Message was sent while issue was closed.
This CL exposes unfavorable timing dependencies to document onload() in some
tests and thus might make the tests flaky.
Please mark them (if any) in TestExpectations and refer to BUG=624697. I'll
investigate and fix the tests (or revert this CL if needed).

Powered by Google App Engine
This is Rietveld 408576698