Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2223223002:
[ParseHTMLOnMainThread] Force plaintext synchronously on lookahead parser (Closed)
|
Created:
4 years, 4 months ago by Charlie Harrison Modified:
4 years, 4 months ago Reviewers:
kouhei (in TOK) CC:
chromium-reviews, blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org, kinuko+watch, loading-reviews+parser_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ParseHTMLOnMainThread] Force plaintext synchronously on lookahead parser
BUG=635634
Committed: https://crrev.com/5a5b7beccee1f245d245eed92f4b5f591b841b4f
Cr-Commit-Position: refs/heads/master@{#411900}
Patch Set 1 #Patch Set 2 : Add layout test #
Total comments: 3
Patch Set 3 : iframe onload works and isnt flaky in either direction #
Depends on Patchset: Messages
Total messages: 27 (20 generated)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
csharrison@chromium.org changed reviewers: + kouhei@chromium.org
Kouhei, PTAL at this change. Feel free to punt this to another reviewer if you don't have time.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2016/08/09 19:04:44, csharrison wrote: > Kouhei, PTAL at this change. Feel free to punt this to another reviewer if you > don't have time. I think win_chromium_rel_ng is just being flaky. I'll spin up a new job tonight if things calm down.
lgtm https://codereview.chromium.org/2223223002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/parser/force-plaintext.html (right): https://codereview.chromium.org/2223223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/parser/force-plaintext.html:10: window.addEventListener('load', function() { I'm a bit confused here. Don't we need to add 'load' event listener to the iframe instead of window? https://codereview.chromium.org/2223223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/2223223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:577: // tokenizing can happen before plaintext is forced. Would you note this in CL description too?
One question https://codereview.chromium.org/2223223002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/parser/force-plaintext.html (right): https://codereview.chromium.org/2223223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/parser/force-plaintext.html:10: window.addEventListener('load', function() { On 2016/08/09 23:54:02, kouhei wrote: > I'm a bit confused here. Don't we need to add 'load' event listener to the > iframe instead of window? Doesn't the main frame load event occur after subframes? I guess I am still a bit concerned there is a race here. Locally I confirmed that this fails without the patch that the 'message' event is called, but is that guaranteed?
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/2223223002/#ps40001 (title: "iframe onload works and isnt flaky in either direction")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [ParseHTMLOnMainThread] Force plaintext synchronously on lookahead parser BUG=635634 ========== to ========== [ParseHTMLOnMainThread] Force plaintext synchronously on lookahead parser BUG=635634 Committed: https://crrev.com/5a5b7beccee1f245d245eed92f4b5f591b841b4f Cr-Commit-Position: refs/heads/master@{#411900} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5a5b7beccee1f245d245eed92f4b5f591b841b4f Cr-Commit-Position: refs/heads/master@{#411900} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
