Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1093103003:
[cleanup, battery] Don't include unneeded headers.
|
Created:
5 years, 8 months ago by riju_ Modified:
3 years, 6 months ago Reviewers:
timvolodine CC:
blink-reviews, mlamouri+watch-blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemoving some unneeded headers and using forward
declarations where possible in the battery module.
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Don-t-include-unneeded-headers
BUG=none
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 5
Patch Set 3 : Fix tim's comments #
Messages
Total messages: 27 (19 generated)
rijubrata.bhaumik@intel.com changed reviewers: + timvolodine@chromium.org
tim@ : I was doing a similar work for battery as for motion|Orientation in https://codereview.chromium.org/1079983003/ I was not sure if you like to have the "move" part and the "cleanup" part separate. i can add the next part to this CL next patchset, or make new CL.
On 2015/04/24 13:33:45, riju_ wrote: > tim@ : I was doing a similar work for battery as for motion|Orientation in > https://codereview.chromium.org/1079983003/ > > I was not sure if you like to have the "move" part and the "cleanup" part > separate. i can add the next part to this CL next patchset, or make new CL. since the "clean up" is relatively small, it's probably fine to do this in one go.
The CQ bit was checked by rijubrata.bhaumik@intel.com 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: linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by rijubrata.bhaumik@intel.com 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.
Tim@ : Just saw that this was lying in my list. The move part is not valid any more after Onion soup and mojo efforts. Let me know if you want to land this small cleanup, else we can close this issue.
On 2017/06/02 12:23:40, riju_ wrote: > Tim@ : Just saw that this was lying in my list. The move part is not valid any > more after Onion soup and mojo efforts. Let me know if you want to land this > small cleanup, else we can close this issue. dunno, fine with me if it still applies )
On 2017/06/08 20:29:36, timvolodine wrote: > On 2017/06/02 12:23:40, riju_ wrote: > > Tim@ : Just saw that this was lying in my list. The move part is not valid any > > more after Onion soup and mojo efforts. Let me know if you want to land this > > small cleanup, else we can close this issue. > > dunno, fine with me if it still applies ) In that case, this CL needs your blessing :)
Two comments below. Also, maybe expand the description of the issue a bit to describe what is actually cleaned up ) https://codereview.chromium.org/1093103003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/battery/BatteryManager.h (left): https://codereview.chromium.org/1093103003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/battery/BatteryManager.h:8: #include "bindings/core/v8/ScriptPromise.h" Actually, ScriptPromise is used in definition of StartRequest() below? https://codereview.chromium.org/1093103003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/battery/NavigatorBattery.h (right): https://codereview.chromium.org/1093103003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/battery/NavigatorBattery.h:16: class ScriptState; Probably need to include ScriptPromise and ScriptState in NavigatorBattery.cc?
Description was changed from ========== clean up in battery module code BUG=none ========== to ========== Removing some unneeded headers and using forward declarations where possible in the battery module. https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Don-t-... BUG=none ==========
The CQ bit was checked by rijubrata.bhaumik@intel.com 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...
Thanks Tim, description updated now, other answers inline. https://codereview.chromium.org/1093103003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/battery/BatteryManager.h (left): https://codereview.chromium.org/1093103003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/battery/BatteryManager.h:8: #include "bindings/core/v8/ScriptPromise.h" On 2017/06/09 12:06:19, timvolodine wrote: > Actually, ScriptPromise is used in definition of StartRequest() below? ScriptPromiseProperty.h already includes ScriptPromise.h https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... https://codereview.chromium.org/1093103003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/battery/NavigatorBattery.h (right): https://codereview.chromium.org/1093103003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/battery/NavigatorBattery.h:16: class ScriptState; On 2017/06/09 12:06:19, timvolodine wrote: > Probably need to include ScriptPromise and ScriptState in NavigatorBattery.cc? NavigatorBattery.cpp is including BatteryManager.h, which includes ScriptPromiseProperty, which includes ScriptPromise. ScriptPromise includes ScriptValue.h, which includes ScriptState.h
https://codereview.chromium.org/1093103003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/battery/NavigatorBattery.h (right): https://codereview.chromium.org/1093103003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/battery/NavigatorBattery.h:16: class ScriptState; On 2017/06/09 13:36:11, riju_ wrote: > On 2017/06/09 12:06:19, timvolodine wrote: > > Probably need to include ScriptPromise and ScriptState in NavigatorBattery.cc? > > NavigatorBattery.cpp is including BatteryManager.h, which includes > ScriptPromiseProperty, which includes ScriptPromise. > ScriptPromise includes ScriptValue.h, which includes ScriptState.h I think we generally avoid relying on transitive includes (because implementations can change which will cause potential breakage in files that depend on their includes). https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rijubrata.bhaumik@intel.com 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. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
