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

Issue 375263002: Revert of HTMLParser should use current value of scriptEnabled flag (Closed)

Created:
6 years, 5 months ago by Yuta Kitamura
Modified:
6 years, 5 months ago
CC:
arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-html_chromium.org, dglazkov+blink
Base URL:
https://chromium.googlesource.com/chromium/blink.git@WebKit-ADD
Visibility:
Public.

Description

Revert of HTMLParser should use current value of scriptEnabled flag (https://codereview.chromium.org/313173012/) Reason for revert: This patch *appears* to break AwSettingsTest and AwContentsClientShouldOverrideUrlLoadingTest: http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20(dbg) This is my wild guess but it might be possible that this patch is the cause, since the tests in question call setJavaScriptEnabled(true). Apologies if my guess is wrong. I'll revert the revert if it turned out that I'm wrong. Original issue's description: > HTMLParser should use current value of scriptEnabled flag > > This CL adds caching of scriptEnabled setting to Document. > > When page is reloaded we create new parser throught DocumentLoader::ensureWriter > and set scriptEnabled flag. ContentSettingsObserver::allowScript method is > responsible for checking settings but it uses cache. Cache is cleared on > DidCommitProvisionalLoad but parser is already created before that. However we > can't clear cache earlier because unload events for old page might be fired and > it depends on old page settings (cached). > > BUG=232410 > > Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177664 NOTREECHECKS=true NOTRY=true BUG=232410 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177730

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+13 lines, -61 lines) Patch
M LayoutTests/fast/harness/override-preferences.html View 1 chunk +3 lines, -7 lines 0 comments Download
D LayoutTests/fast/harness/resources/override-preferences-window.html View 1 chunk +0 lines, -7 lines 0 comments Download
D LayoutTests/http/tests/xsl/resources/xslt-transform-with-javascript-disabled-mainframe.html View 1 chunk +0 lines, -17 lines 0 comments Download
M LayoutTests/http/tests/xsl/xslt-transform-with-javascript-disabled.html View 1 chunk +8 lines, -12 lines 0 comments Download
M Source/bindings/core/v8/ScriptController.cpp View 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/dom/Document.h View 2 chunks +0 lines, -9 lines 0 comments Download
M Source/core/dom/Document.cpp View 2 chunks +0 lines, -8 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
Yuta Kitamura
Created Revert of HTMLParser should use current value of scriptEnabled flag
6 years, 5 months ago (2014-07-09 09:20:44 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yutak@chromium.org/375263002/1
6 years, 5 months ago (2014-07-09 09:21:31 UTC) #2
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 5 months ago (2014-07-09 09:21:32 UTC) #3
commit-bot: I haz the power
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an ...
6 years, 5 months ago (2014-07-09 09:21:33 UTC) #4
haraken
LGTM
6 years, 5 months ago (2014-07-09 09:25:03 UTC) #5
haraken
The CQ bit was checked by haraken@chromium.org
6 years, 5 months ago (2014-07-09 09:25:08 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yutak@chromium.org/375263002/1
6 years, 5 months ago (2014-07-09 09:25:32 UTC) #7
Yuta Kitamura
It's weird the revert did not add TBR or NOTREECHECKS lines to the change description. ...
6 years, 5 months ago (2014-07-09 09:30:12 UTC) #8
Yuta Kitamura
The CQ bit was unchecked by yutak@chromium.org
6 years, 5 months ago (2014-07-09 09:30:16 UTC) #9
Yuta Kitamura
The CQ bit was checked by yutak@chromium.org
6 years, 5 months ago (2014-07-09 09:30:41 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yutak@chromium.org/375263002/1
6 years, 5 months ago (2014-07-09 09:31:34 UTC) #11
commit-bot: I haz the power
6 years, 5 months ago (2014-07-09 09:32:07 UTC) #12
Message was sent while issue was closed.
Change committed as 177730

Powered by Google App Engine
This is Rietveld 408576698