Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2041013004:
HTMLDocumentParser: Dedupe member inits by moving them into in-class init (Closed)
|
Created:
4 years, 6 months ago by kouhei (in TOK) Modified:
4 years, 5 months ago CC:
chromium-reviews, blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org, kinuko+watch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHTMLDocumentParser: Dedupe member inits by moving them into in-class init
BUG=None
Committed: https://crrev.com/630d84f64ee2f1d54d2627828f1ee593e85e2327
Cr-Commit-Position: refs/heads/master@{#403876}
Patch Set 1 #
Total comments: 4
Patch Set 2 : use delegating ctor #
Total comments: 5
Patch Set 3 : no scriptrunner in fragment parser #
Messages
Total messages: 29 (10 generated)
kouhei@chromium.org changed reviewers: + tzik@chromium.org, yoav@yoav.ws
kouhei@chromium.org changed required reviewers: + tzik@chromium.org
The CQ bit was checked by kouhei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2041013004/1
https://codereview.chromium.org/2041013004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h (right): https://codereview.chromium.org/2041013004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h:195: OwnPtr<WebTaskRunner> m_loadingTaskRunner = adoptPtr(document()->loadingTaskRunner()->clone()); Is having non-const, dependent on field init order, initializers such a good idea?
https://codereview.chromium.org/2041013004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h (right): https://codereview.chromium.org/2041013004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h:195: OwnPtr<WebTaskRunner> m_loadingTaskRunner = adoptPtr(document()->loadingTaskRunner()->clone()); On 2016/06/08 06:02:04, sof wrote: > Is having non-const, dependent on field init order, initializers such a good > idea? The background is that we found m_isParsingAtLineNumber uninitialized in one of the ctors. I'm ok as long as we are able to avoid that kind of mistakes in the future. I think the options are: - This CL. - Const trivially initialized members use in-class init. non-const members init are duped. - Make a private delegating constructor and initialize all members there. No in-class init.
Description was changed from ========== HTMLDocumentParser: Dedupe member inits by moving them into in-class init BUG=None ========== to ========== HTMLDocumentParser: Dedupe member inits by moving them into in-class init BUG=None ==========
kouhei@chromium.org changed reviewers: + yutak@chromium.org
+yutak for more discussion
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/2041013004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h (right): https://codereview.chromium.org/2041013004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h:195: OwnPtr<WebTaskRunner> m_loadingTaskRunner = adoptPtr(document()->loadingTaskRunner()->clone()); On 2016/06/08 06:07:17, kouhei wrote: > On 2016/06/08 06:02:04, sof wrote: > > Is having non-const, dependent on field init order, initializers such a good > > idea? > > The background is that we found m_isParsingAtLineNumber uninitialized in one of > the ctors. > I'm ok as long as we are able to avoid that kind of mistakes in the future. > > I think the options are: > - This CL. > - Const trivially initialized members use in-class init. non-const members init > are duped. > - Make a private delegating constructor and initialize all members there. No > in-class init. Thanks for the context (and menu :) ), initializations needs to be commoned-up. Would a delegating constructor be feasible & readable, i.e., not have too many arguments?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2041013004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h (right): https://codereview.chromium.org/2041013004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h:195: OwnPtr<WebTaskRunner> m_loadingTaskRunner = adoptPtr(document()->loadingTaskRunner()->clone()); On 2016/06/08 06:40:24, sof wrote: > On 2016/06/08 06:07:17, kouhei wrote: > > On 2016/06/08 06:02:04, sof wrote: > > > Is having non-const, dependent on field init order, initializers such a good > > > idea? > > > > The background is that we found m_isParsingAtLineNumber uninitialized in one > of > > the ctors. > > I'm ok as long as we are able to avoid that kind of mistakes in the future. > > > > I think the options are: > > - This CL. > > - Const trivially initialized members use in-class init. non-const members > init > > are duped. > > - Make a private delegating constructor and initialize all members there. No > > in-class init. > > Thanks for the context (and menu :) ), initializations needs to be commoned-up. > Would a delegating constructor be feasible & readable, i.e., not have too many > arguments? Just to make sure that I understand the problems sof sees in this CL, is it related to the fact that document() is initialized first, and then used to initialize the other members?
ptal
https://codereview.chromium.org/2041013004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/2041013004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:112: , m_scriptRunner(scriptingContentIsAllowed(contentPolicy) ? HTMLScriptRunner::create(&document, this) : nullptr) # As we chatted offline. On old code, m_scriptRunner was not created on DocumentFragment* mode and it is created on new code. Is it intended?
On 2016/07/05 07:59:09, tzik wrote: > https://codereview.chromium.org/2041013004/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right): > > https://codereview.chromium.org/2041013004/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:112: , > m_scriptRunner(scriptingContentIsAllowed(contentPolicy) ? > HTMLScriptRunner::create(&document, this) : nullptr) > # As we chatted offline. > On old code, m_scriptRunner was not created on DocumentFragment* mode and it is > created on new code. Is it intended? No. Fixed.
https://codereview.chromium.org/2041013004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/2041013004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:94: m_treeBuilder = HTMLTreeBuilder::create(this, &document, AllowScriptingContent, m_options); Doesn't this mean that we're creating m_treeBuilder twice? https://codereview.chromium.org/2041013004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:97: HTMLDocumentParser::HTMLDocumentParser(DocumentFragment* fragment, Element* contextElement, ParserContentPolicy parserContentPolicy) Not critical, but It would've been easier to review if you'd have kept this constructor in its old location.
lgtm
https://codereview.chromium.org/2041013004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/2041013004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:94: m_treeBuilder = HTMLTreeBuilder::create(this, &document, AllowScriptingContent, m_options); On 2016/07/05 08:20:14, Yoav Weiss wrote: > Doesn't this mean that we're creating m_treeBuilder twice? Yes. This does mean that m_treeBuilder Member smartptr is created twice, but this should be cheap. (HTMLTreeBuilder itself is only created once.) We plan to improve the code further in separate refactor CLs. https://codereview.chromium.org/2041013004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:97: HTMLDocumentParser::HTMLDocumentParser(DocumentFragment* fragment, Element* contextElement, ParserContentPolicy parserContentPolicy) On 2016/07/05 08:20:14, Yoav Weiss wrote: > Not critical, but It would've been easier to review if you'd have kept this > constructor in its old location. I think we should have delegated ctor def either at the top or the bottom. I put this bottom in this case, as this was declared last.
On 2016/07/06 00:49:54, kouhei wrote: > https://codereview.chromium.org/2041013004/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right): > > https://codereview.chromium.org/2041013004/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:94: > m_treeBuilder = HTMLTreeBuilder::create(this, &document, AllowScriptingContent, > m_options); > On 2016/07/05 08:20:14, Yoav Weiss wrote: > > Doesn't this mean that we're creating m_treeBuilder twice? > > Yes. This does mean that m_treeBuilder Member smartptr is created twice, but > this should be cheap. (HTMLTreeBuilder itself is only created once.) OK. I thought the constructors creating the treebuilder are delegating from to the other, but looking again, that's not the case. > We plan to improve the code further in separate refactor CLs. > > https://codereview.chromium.org/2041013004/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:97: > HTMLDocumentParser::HTMLDocumentParser(DocumentFragment* fragment, Element* > contextElement, ParserContentPolicy parserContentPolicy) > On 2016/07/05 08:20:14, Yoav Weiss wrote: > > Not critical, but It would've been easier to review if you'd have kept this > > constructor in its old location. > > I think we should have delegated ctor def either at the top or the bottom. I put > this bottom in this case, as this was declared last. LGTM
The CQ bit was checked by kouhei@chromium.org
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.
Description was changed from ========== HTMLDocumentParser: Dedupe member inits by moving them into in-class init BUG=None ========== to ========== HTMLDocumentParser: Dedupe member inits by moving them into in-class init BUG=None ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== HTMLDocumentParser: Dedupe member inits by moving them into in-class init BUG=None ========== to ========== HTMLDocumentParser: Dedupe member inits by moving them into in-class init BUG=None Committed: https://crrev.com/630d84f64ee2f1d54d2627828f1ee593e85e2327 Cr-Commit-Position: refs/heads/master@{#403876} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/630d84f64ee2f1d54d2627828f1ee593e85e2327 Cr-Commit-Position: refs/heads/master@{#403876} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
