Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2696653004:
[Script Spec Annotation] Annotate and refactor script element's flags (Closed)
|
Created:
3 years, 10 months ago by hiroshige Modified:
3 years, 10 months ago Reviewers:
kouhei (in TOK) CC:
chromium-reviews, blink-reviews-html_chromium.org, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Script Spec Annotation] Annotate and refactor script element's flags
This CL annotates some flags of ScriptLoader that correspond directly
to the spec (https://html.spec.whatwg.org/#script-processing-model), and
refactors related code to improve the correspondence to the spec.
This CL also
- Modifies ScriptLoader constructor to match its flag setting code with
the spec,
- Reorders the flags to match the order in the spec,
- Renames |m_forceAsnyc| to |m_nonBlocking| according to the spec, and
- Turn bitfields into normal bools to allow in-class initializers.
BUG=686281
Review-Url: https://codereview.chromium.org/2696653004
Cr-Commit-Position: refs/heads/master@{#450203}
Committed: https://chromium.googlesource.com/chromium/src/+/f4d659e887283a9cf544f917cf82a5abde22aa7b
Patch Set 1 #
Total comments: 9
Patch Set 2 : More refactor #
Dependent Patchsets: Messages
Total messages: 22 (15 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...
Description was changed from ========== [Script Spec Annotation] Annotate script element's flags BUG= ========== to ========== [Script Spec Annotation] Annotate script element's flags This CL annotates some flags of ScriptLoader that correspond directly to the spec. This CL also - Reorders the flags to match the order in the spec, - Renames |m_forceAsnyc| to |m_nonBlocking| according to the spec, and - Turn bitfields into normal bools to allow in-class initializers. BUG= ==========
hiroshige@chromium.org changed reviewers: + kouhei@chromium.org
Description was changed from ========== [Script Spec Annotation] Annotate script element's flags This CL annotates some flags of ScriptLoader that correspond directly to the spec. This CL also - Reorders the flags to match the order in the spec, - Renames |m_forceAsnyc| to |m_nonBlocking| according to the spec, and - Turn bitfields into normal bools to allow in-class initializers. BUG= ========== to ========== [Script Spec Annotation] Annotate script element's flags This CL annotates some flags of ScriptLoader that correspond directly to the spec. This CL also - Reorders the flags to match the order in the spec, - Renames |m_forceAsnyc| to |m_nonBlocking| according to the spec, and - Turn bitfields into normal bools to allow in-class initializers. BUG=686281 ==========
PTAL. Level-setting question: In this CL I tried to add comments as much as possible, e.g. in ScriptLoader::ScriptLoader(), to show the code has corresponding spec statements. Do you think this is preferable, or is this too much? https://codereview.chromium.org/2696653004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ScriptLoader.h (right): https://codereview.chromium.org/2696653004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.h:155: bool m_nonBlocking; Renamed from m_forceAsync. https://codereview.chromium.org/2696653004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.h:162: // TODO(hiroshige): Implement "script's type". I'm not sure whether we'll implement the script type here, but I put this comment just as a placeholder/reminder.
> Do you think this is preferable, or is this too much? I'm supportive of this change https://codereview.chromium.org/2696653004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/2696653004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:73: // flag on the copy if it is set on the element being cloned." My understanding is that cloning steps are implemented in {HTML,SVG}ScriptElement::cloneElementWithoutAttributesAndChildren (yes. duped logic) https://codereview.chromium.org/2696653004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ScriptLoader.h (right): https://codereview.chromium.org/2696653004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.h:148: // https://html.spec.whatwg.org/#already-started Should we // "Initially, script elements must have this flag unset" here, and = false? https://codereview.chromium.org/2696653004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.h:162: // TODO(hiroshige): Implement "script's type". On 2017/02/13 21:17:25, hiroshige wrote: > I'm not sure whether we'll implement the script type here, but I put this > comment just as a placeholder/reminder. sgtm https://codereview.chromium.org/2696653004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLScriptElement.cpp (right): https://codereview.chromium.org/2696653004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLScriptElement.cpp:129: return fastHasAttribute(asyncAttr) || (m_loader->isNonBlocking()); remove extra parens on (m_loader->isNonBlocking())?
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...
https://codereview.chromium.org/2696653004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/2696653004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:73: // flag on the copy if it is set on the element being cloned." On 2017/02/13 21:31:08, kouhei-atCAM wrote: > My understanding is that cloning steps are implemented in > {HTML,SVG}ScriptElement::cloneElementWithoutAttributesAndChildren (yes. duped > logic) Agree. Added TODO. https://codereview.chromium.org/2696653004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ScriptLoader.h (right): https://codereview.chromium.org/2696653004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.h:148: // https://html.spec.whatwg.org/#already-started On 2017/02/13 21:31:09, kouhei-atCAM wrote: > Should we // "Initially, script elements must have this flag unset" here, and = > false? Done. Applied the same to other flags and modifies the constructor. Please confirm that the modification preserves the behavior. https://codereview.chromium.org/2696653004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLScriptElement.cpp (right): https://codereview.chromium.org/2696653004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLScriptElement.cpp:129: return fastHasAttribute(asyncAttr) || (m_loader->isNonBlocking()); On 2017/02/13 21:31:09, kouhei-atCAM wrote: > remove extra parens on (m_loader->isNonBlocking())? Done.
lgtm
The CQ bit was unchecked by hiroshige@chromium.org
Description was changed from ========== [Script Spec Annotation] Annotate script element's flags This CL annotates some flags of ScriptLoader that correspond directly to the spec. This CL also - Reorders the flags to match the order in the spec, - Renames |m_forceAsnyc| to |m_nonBlocking| according to the spec, and - Turn bitfields into normal bools to allow in-class initializers. BUG=686281 ========== to ========== [Script Spec Annotation] Annotate script element's flags This CL annotates some flags of ScriptLoader that correspond directly to the spec (https://html.spec.whatwg.org/#script-processing-model). This CL also - Modifies ScriptLoader constructor to match its flag setting code with the spec, - Reorders the flags to match the order in the spec, - Renames |m_forceAsnyc| to |m_nonBlocking| according to the spec, and - Turn bitfields into normal bools to allow in-class initializers. BUG=686281 ==========
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
Description was changed from ========== [Script Spec Annotation] Annotate script element's flags This CL annotates some flags of ScriptLoader that correspond directly to the spec (https://html.spec.whatwg.org/#script-processing-model). This CL also - Modifies ScriptLoader constructor to match its flag setting code with the spec, - Reorders the flags to match the order in the spec, - Renames |m_forceAsnyc| to |m_nonBlocking| according to the spec, and - Turn bitfields into normal bools to allow in-class initializers. BUG=686281 ========== to ========== [Script Spec Annotation] Annotate and refactor script element's flags This CL annotates some flags of ScriptLoader that correspond directly to the spec (https://html.spec.whatwg.org/#script-processing-model), and refactors related code to improve the correspondence to the spec. This CL also - Modifies ScriptLoader constructor to match its flag setting code with the spec, - Reorders the flags to match the order in the spec, - Renames |m_forceAsnyc| to |m_nonBlocking| according to the spec, and - Turn bitfields into normal bools to allow in-class initializers. BUG=686281 ==========
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": 20001, "attempt_start_ts": 1487037875418750,
"parent_rev": "30aef3be9b857563cfab70278e011dba9d1e8a14", "commit_rev":
"f4d659e887283a9cf544f917cf82a5abde22aa7b"}
Message was sent while issue was closed.
Description was changed from ========== [Script Spec Annotation] Annotate and refactor script element's flags This CL annotates some flags of ScriptLoader that correspond directly to the spec (https://html.spec.whatwg.org/#script-processing-model), and refactors related code to improve the correspondence to the spec. This CL also - Modifies ScriptLoader constructor to match its flag setting code with the spec, - Reorders the flags to match the order in the spec, - Renames |m_forceAsnyc| to |m_nonBlocking| according to the spec, and - Turn bitfields into normal bools to allow in-class initializers. BUG=686281 ========== to ========== [Script Spec Annotation] Annotate and refactor script element's flags This CL annotates some flags of ScriptLoader that correspond directly to the spec (https://html.spec.whatwg.org/#script-processing-model), and refactors related code to improve the correspondence to the spec. This CL also - Modifies ScriptLoader constructor to match its flag setting code with the spec, - Reorders the flags to match the order in the spec, - Renames |m_forceAsnyc| to |m_nonBlocking| according to the spec, and - Turn bitfields into normal bools to allow in-class initializers. BUG=686281 Review-Url: https://codereview.chromium.org/2696653004 Cr-Commit-Position: refs/heads/master@{#450203} Committed: https://chromium.googlesource.com/chromium/src/+/f4d659e887283a9cf544f917cf82... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f4d659e887283a9cf544f917cf82... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
