Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2693423002:
Do not re-initialize PendingScript in HTMLParserScriptRunner (Closed)
|
Created:
3 years, 10 months ago by hiroshige Modified:
3 years, 10 months ago CC:
chromium-reviews, blink-reviews-html_chromium.org, sof, eae+blinkwatch, loading-reviews+parser_chromium.org, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews, kinuko+watch, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not re-initialize PendingScript in HTMLParserScriptRunner
Previously, |HTMLParserScriptRunner::m_parserBlockingScript| was
re-initialized every time it processes a new script tag
(i.e. a new ScriptLoader), and therefore one PendingScript can be
reused for multiple ScriptLoaders.
This CL makes |m_parserBlockingScript| to be newly created rather than
re-initialized with the same object.
This clarifies that a PendingScript corresponds to a ScriptLoader,
and also enables creating ModulePendingScript in a cleaner way in
https://codereview.chromium.org/2653923008/.
BUG=686281
Review-Url: https://codereview.chromium.org/2693423002
Cr-Commit-Position: refs/heads/master@{#451911}
Committed: https://chromium.googlesource.com/chromium/src/+/85a1f107f95bc55f75992f52d61343cd51f1aa53
Patch Set 1 #Patch Set 2 : fix test #
Total comments: 19
Patch Set 3 : Reflect comments #
Total comments: 2
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 48 (28 generated)
The CQ bit was checked by hiroshige@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.
Description was changed from ========== Do not re-initialize PendingScript in HTMLParserScriptRunner BUG= ========== to ========== Do not re-initialize PendingScript in HTMLParserScriptRunner Previously, |HTMLParserScriptRunner::m_parserBlockingScript| was re-initialized every time it processes a new script tag (i.e. a new ScriptLoader), and therefore one PendingScript can be reused for multiple ScriptLoaders. This CL makes |m_parserBlockingScript| to be newly created rather than re-initialized with the same object. This clarifies that a PendingScript corresponds to a ScriptLoader, and also enables creating ModulePendingScript in a cleaner way in https://codereview.chromium.org/2653923008/. BUG=686281 ==========
hiroshige@chromium.org changed reviewers: + kouhei@chromium.org
hiroshige@chromium.org changed reviewers: + sigbjornf@opera.com
PTAL.
https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp (right): https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:585: if (!resource) { Do we ever get here?
https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp (right): https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:585: if (!resource) { On 2017/02/16 00:55:09, kouhei-atCAM wrote: > Do we ever get here? Probably no, because fetchScript() should have returned false. I'll create a separate CL to replace this if statement with CHECK().
lgtm
I think this'll make ownership easier to reason about. Added mostly style-related feedback. https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/PendingScript.cpp (right): https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/PendingScript.cpp:105: CHECK(m_element); Could you xref the element() declaration's comment, to clarify why the CHECK() is used? https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/PendingScript.cpp:144: if (m_element) { Not part of this CL at all, but moving this SRI-specific special case out into a separate method, would read better. https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/PendingScript.h (right): https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/PendingScript.h:137: // In unit tests |m_element| can be null. nit: null => false https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:820: m_pendingScript->dispose(); Call detach() instead? Actually, I think we should rename it to detachPendingScript() (and made private.) https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp (right): https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:160: m_parserBlockingScript(nullptr) { I don't mind terribly, but null-initializing Member<>s isn't needed. https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:323: TextPosition startingPosition; nit: could you move these decls down to where they're initially bound? https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:329: if (!parserBlockingScript()) I think it makes sense to have this check first. https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:452: executePendingScriptAndDispatchEvent(m_parserBlockingScript, parserBlockingScript() for consistency.
The CQ bit was checked by hiroshige@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by hiroshige@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.
https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/PendingScript.cpp (right): https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/PendingScript.cpp:105: CHECK(m_element); On 2017/02/16 08:15:15, sof wrote: > Could you xref the element() declaration's comment, to clarify why the CHECK() > is used? Done. https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/PendingScript.cpp:144: if (m_element) { On 2017/02/16 08:15:15, sof wrote: > Not part of this CL at all, but moving this SRI-specific special case out into a > separate method, would read better. Created https://codereview.chromium.org/2698613007/. https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/PendingScript.h (right): https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/PendingScript.h:137: // In unit tests |m_element| can be null. On 2017/02/16 08:15:15, sof wrote: > nit: null => false |m_element| is Member<Element> so it is |null|. Rephrased. https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:820: m_pendingScript->dispose(); On 2017/02/16 08:15:15, sof wrote: > Call detach() instead? Actually, I think we should rename it to > detachPendingScript() (and made private.) Done. https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp (right): https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:160: m_parserBlockingScript(nullptr) { On 2017/02/16 08:15:16, sof wrote: > I don't mind terribly, but null-initializing Member<>s isn't needed. I'd like to leave this to make it more explicitly that now |m_parserBlockingScript| is initialized to nullptr. https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:323: TextPosition startingPosition; On 2017/02/16 08:15:16, sof wrote: > nit: could you move these decls down to where they're initially bound? Done. https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:329: if (!parserBlockingScript()) On 2017/02/16 08:15:16, sof wrote: > I think it makes sense to have this check first. Done. https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:452: executePendingScriptAndDispatchEvent(m_parserBlockingScript, On 2017/02/16 08:15:16, sof wrote: > parserBlockingScript() for consistency. I used parserBlockingScript() for const ref and m_parserBlockingScript only for non-const ref. Here non-const ref is needed and thus I used m_parserBlockingScript. https://codereview.chromium.org/2693423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:585: if (!resource) { On 2017/02/16 01:13:15, hiroshige wrote: > On 2017/02/16 00:55:09, kouhei-atCAM wrote: > > Do we ever get here? > > Probably no, because fetchScript() should have returned false. > I'll create a separate CL to replace this if statement with CHECK(). Created https://codereview.chromium.org/2692863013.
lgtm
The CQ bit was checked by hiroshige@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/2693423002/#ps40001 (title: "Reflect comments")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by hiroshige@chromium.org
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
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...)
hiroshige@chromium.org changed reviewers: + haraken@chromium.org
haraken@, could you take a look as bindings/ OWNER?
LGTM https://codereview.chromium.org/2693423002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/PendingScript.h (right): https://codereview.chromium.org/2693423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/PendingScript.h:139: const bool m_isForTesting; Or would there be an option to create a dummy element in the test so that we can avoid adding a special case for testing code?
https://codereview.chromium.org/2693423002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/PendingScript.h (right): https://codereview.chromium.org/2693423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/PendingScript.h:139: const bool m_isForTesting; On 2017/02/22 00:30:04, haraken wrote: > > Or would there be an option to create a dummy element in the test so that we can > avoid adding a special case for testing code? Possibly, but it should be done separately not to complicate (the changes to) PendingScript state transitions. (And I hope we'll no longer need such changes after we refactor PendingScript largely later in this year)
The CQ bit was checked by hiroshige@chromium.org
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 hiroshige@chromium.org
The CQ bit was checked by hiroshige@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by hiroshige@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487747943757600,
"parent_rev": "4f7222a63ccfd557e7ef1657fee3b4b7ab1467c1", "commit_rev":
"85a1f107f95bc55f75992f52d61343cd51f1aa53"}
Message was sent while issue was closed.
Description was changed from ========== Do not re-initialize PendingScript in HTMLParserScriptRunner Previously, |HTMLParserScriptRunner::m_parserBlockingScript| was re-initialized every time it processes a new script tag (i.e. a new ScriptLoader), and therefore one PendingScript can be reused for multiple ScriptLoaders. This CL makes |m_parserBlockingScript| to be newly created rather than re-initialized with the same object. This clarifies that a PendingScript corresponds to a ScriptLoader, and also enables creating ModulePendingScript in a cleaner way in https://codereview.chromium.org/2653923008/. BUG=686281 ========== to ========== Do not re-initialize PendingScript in HTMLParserScriptRunner Previously, |HTMLParserScriptRunner::m_parserBlockingScript| was re-initialized every time it processes a new script tag (i.e. a new ScriptLoader), and therefore one PendingScript can be reused for multiple ScriptLoaders. This CL makes |m_parserBlockingScript| to be newly created rather than re-initialized with the same object. This clarifies that a PendingScript corresponds to a ScriptLoader, and also enables creating ModulePendingScript in a cleaner way in https://codereview.chromium.org/2653923008/. BUG=686281 Review-Url: https://codereview.chromium.org/2693423002 Cr-Commit-Position: refs/heads/master@{#451911} Committed: https://chromium.googlesource.com/chromium/src/+/85a1f107f95bc55f75992f52d613... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/85a1f107f95bc55f75992f52d613... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
