Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2272043002:
Remove unnecessary includes from HTMLDocumentParser.h (Closed)
|
Created:
4 years, 4 months ago by pasko Modified:
4 years, 3 months ago Reviewers:
Charlie Harrison CC:
chromium-reviews, blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org, kinuko+watch, loading-reviews+parser_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove unnecessary includes from HTMLDocumentParser.h
BUG=none
Committed: https://crrev.com/02b1a521c549f3fbde3cc7eabcfec8a216d55163
Cr-Commit-Position: refs/heads/master@{#414774}
Patch Set 1 #
Total comments: 4
Patch Set 2 : rebase #
Messages
Total messages: 28 (14 generated)
The CQ bit was checked by pasko@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...
pasko@chromium.org changed reviewers: + csharrison@chromium.org
csharrison: PTaL not sure what the Blink style mandates .. was looking at this file and saw a few includes that surprised me. "Less surprises - more readable" - I thought.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2016/08/24 16:53:49, pasko wrote: > csharrison: PTaL > > not sure what the Blink style mandates .. was looking at this file and saw a few > includes that surprised me. "Less surprises - more readable" - I thought. friendly ping :)
Thanks for the ping! I had comments all ready and somehow forgot to publish. Changes lgtm % comments. Thank you for the clean up! https://codereview.chromium.org/2272043002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h (left): https://codereview.chromium.org/2272043002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h:35: #include "core/html/parser/CompactHTMLToken.h" Looks like we need this one for void constructTreeFromCompactHTMLToken(const CompactHTMLToken&); https://codereview.chromium.org/2272043002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h:49: #include "platform/text/SegmentedString.h" Looks like need this declaration for "void insert(const SegmentedString&) final"
hm, before landing let's figure why we are at disagreement https://codereview.chromium.org/2272043002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h (left): https://codereview.chromium.org/2272043002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h:35: #include "core/html/parser/CompactHTMLToken.h" On 2016/08/25 12:55:52, Charlie Harrison wrote: > Looks like we need this one for > void constructTreeFromCompactHTMLToken(const CompactHTMLToken&); A forward declaration of this class is sufficient, and should be preferred because all declarations in this file are not using any information about the internal organization of CompactHTMLToken. https://codereview.chromium.org/2272043002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h:49: #include "platform/text/SegmentedString.h" On 2016/08/25 12:55:51, Charlie Harrison wrote: > Looks like need this declaration for "void insert(const SegmentedString&) final" Same as above
Ah just re-read the Chromium style guide and you are right, fwd declarations are preferred here. still lgtm.
The CQ bit was checked by pasko@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by pasko@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2272043002/#ps20001 (title: "rebase")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by pasko@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.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Remove unnecessary includes from HTMLDocumentParser.h BUG=none ========== to ========== Remove unnecessary includes from HTMLDocumentParser.h BUG=none Committed: https://crrev.com/02b1a521c549f3fbde3cc7eabcfec8a216d55163 Cr-Commit-Position: refs/heads/master@{#414774} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/02b1a521c549f3fbde3cc7eabcfec8a216d55163 Cr-Commit-Position: refs/heads/master@{#414774} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
