Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2699713002:
[Script Spec Annotation] Annotate HTMLParserScriptRunner (Closed)
Description[Script Spec Annotation] Annotate HTMLParserScriptRunner
https://html.spec.whatwg.org/#the-end
https://html.spec.whatwg.org/#scriptEndTag
BUG=686281
Review-Url: https://codereview.chromium.org/2699713002
Cr-Commit-Position: refs/heads/master@{#451803}
Committed: https://chromium.googlesource.com/chromium/src/+/a2920d6de4edbf12a55e81345b5911849a7cebb6
Patch Set 1 #Patch Set 2 : Fix some comments #
Total comments: 6
Patch Set 3 : reflect comments #Patch Set 4 : reflect comments #
Dependent Patchsets: Messages
Total messages: 20 (12 generated)
kouhei@chromium.org changed reviewers: + dominicc@chromium.org, kouhei@chromium.org
lgtm
Description was changed from ========== Annotate executePendingScriptAndDispatchEvent BUG= ========== to ========== [Script Spec Annotation] Annotate HTMLParserScriptRunner around end tag https://html.spec.whatwg.org/#scriptEndTag BUG=686281 ==========
Description was changed from ========== [Script Spec Annotation] Annotate HTMLParserScriptRunner around end tag https://html.spec.whatwg.org/#scriptEndTag BUG=686281 ========== to ========== [Script Spec Annotation] Annotate HTMLParserScriptRunner https://html.spec.whatwg.org/#the-end https://html.spec.whatwg.org/#scriptEndTag BUG=686281 ==========
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/2699713002/#ps20001 (title: "Fix some comments")
The CQ bit was unchecked by hiroshige@chromium.org
DBCs inline. https://codereview.chromium.org/2699713002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp (right): https://codereview.chromium.org/2699713002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:421: // Implements the "Otherwise" Clause of 'An end tag whose tag name is "script"' I would make this more succinct by dropping "implements"; I think that is clear implicitly. Just mention the part of the spec and the link. https://codereview.chromium.org/2699713002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:429: // These conditions correspond to isParserBlockingScriptReady() and Great comment; thank you so much for doing this. Don't be afraid of using blank lines in the comment with // alone to break commentary from spec prose to make it clearer. BTW, would this be clearer if the spec text did not have quotes, and the asides were quoted with [] or something? 3. " looks a bit messy; 3. is also part of the spec text. The indentation with quotes is not consistent cf line 246, and it also gets a bit ugly when the quoted text contains quotes as on line 394. https://codereview.chromium.org/2699713002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:447: // Implemented as scope out of InsertionPointRecord. "scope out of" is a bit awkward. Maybe say something including "InsertionPointRecord goes out of scope" or "~InsertionPointRecord" or something? Ditto above and below.
https://codereview.chromium.org/2699713002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp (right): https://codereview.chromium.org/2699713002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:421: // Implements the "Otherwise" Clause of 'An end tag whose tag name is "script"' On 2017/02/16 08:41:00, dominicc wrote: > I would make this more succinct by dropping "implements"; I think that is clear > implicitly. Just mention the part of the spec and the link. Done. https://codereview.chromium.org/2699713002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:429: // These conditions correspond to isParserBlockingScriptReady() and On 2017/02/16 08:41:00, dominicc wrote: > Great comment; thank you so much for doing this. > > Don't be afraid of using blank lines in the comment with // alone to break > commentary from spec prose to make it clearer. Done. > BTW, would this be clearer if the spec text did not have quotes, and the asides > were quoted with [] or something? 3. " looks a bit messy; 3. is also part of the > spec text. The indentation with quotes is not consistent cf line 246, and it > also gets a bit ugly when the quoted text contains quotes as on line 394. As for formatting of spec excerpts, I'd like to keep the format mostly as-is (because I've committed spec comments in the same style) and do the reformatting later as a separate issue, to prevent module implementation from being blocked. https://codereview.chromium.org/2699713002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:447: // Implemented as scope out of InsertionPointRecord. On 2017/02/16 08:41:00, dominicc wrote: > "scope out of" is a bit awkward. Maybe say something including > "InsertionPointRecord goes out of scope" or "~InsertionPointRecord" or > something? Ditto above and below. Done.
The CQ bit was checked by dominicc@chromium.org
lgtm
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/2699713002/#ps60001 (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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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": 60001, "attempt_start_ts": 1487697333233530,
"parent_rev": "e28e50ecc0bc4077c879abc2d06957de0918e75e", "commit_rev":
"a2920d6de4edbf12a55e81345b5911849a7cebb6"}
Message was sent while issue was closed.
Description was changed from ========== [Script Spec Annotation] Annotate HTMLParserScriptRunner https://html.spec.whatwg.org/#the-end https://html.spec.whatwg.org/#scriptEndTag BUG=686281 ========== to ========== [Script Spec Annotation] Annotate HTMLParserScriptRunner https://html.spec.whatwg.org/#the-end https://html.spec.whatwg.org/#scriptEndTag BUG=686281 Review-Url: https://codereview.chromium.org/2699713002 Cr-Commit-Position: refs/heads/master@{#451803} Committed: https://chromium.googlesource.com/chromium/src/+/a2920d6de4edbf12a55e81345b59... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/a2920d6de4edbf12a55e81345b59... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
