Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: fix chromium-style warnings in linux code #12949

Merged
merged 1 commit into from May 16, 2018

Conversation

Projects
None yet
3 participants
@nornagon
Copy link
Contributor

nornagon commented May 15, 2018

No description provided.

@nornagon nornagon requested review from as code owners May 15, 2018

@nornagon nornagon changed the title fix: chromium-style warnings in linux code refactor: fix chromium-style warnings in linux code May 15, 2018

@ckerr

ckerr approved these changes May 15, 2018

Copy link
Member

ckerr left a comment

👍

Thank you for working on the Linux builds 🙌

@ckerr
Copy link
Member

ckerr left a comment

oh, hmm. CI is seems to be unhappy for cause for a change

In file included from ../../atom/browser/api/atom_api_web_contents.cc:33:
../../atom/browser/osr/osr_render_widget_host_view.h:202:36: error: only virtual member functions can be marked 'override'
  bool IsAutoResizeEnabled() const override;
                                   ^~~~~~~~

@nornagon nornagon force-pushed the nornagon:chromium-style branch from 37e588e to 905923a May 15, 2018

@nornagon

This comment has been minimized.

Copy link
Contributor Author

nornagon commented May 15, 2018

@ckerr the new results look spurious (something to do with vendor/pdf_viewer?)

@nornagon nornagon force-pushed the nornagon:chromium-style branch from 44d5e59 to 905923a May 16, 2018

@ckerr
Copy link
Member

ckerr left a comment

Mostly LGTM, minor questions inline.

I love it that the clang is finding all these microoptimizations e.g. weeding out unnecessary virtuals

void SetAutoResizeFlags(uint8_t flags) override {
auto_resize_flags_ = flags;
}
void SetAutoResizeFlags(uint8_t flags) override;

This comment has been minimized.

@ckerr

ckerr May 16, 2018

Member

not a blocker, but what was the issue with not inlining this? size-would-be-larger warning?

This comment has been minimized.

@nornagon
@@ -184,6 +184,7 @@ class OffScreenRenderWidgetHostView
std::unique_ptr<ui::CompositorLock> GetCompositorLock(
ui::CompositorLockClient* client) override;
void CompositorResizeLockEnded() override;
bool IsAutoResizeEnabled() const override;

This comment has been minimized.

@ckerr

ckerr May 16, 2018

Member

Was this not needed on non-macOS platforms?

This comment has been minimized.

@nornagon

nornagon May 16, 2018

Author Contributor

It's only needed on non-macOS platforms, as far as I can tell. On macOS, it doesn't override anything in the parent class and we never call it.

@@ -26,7 +26,7 @@ const SkColor kDefaultColor = SkColorSetARGB(255, 233, 233, 233);
} // namespace

MenuBar::MenuBar(views::View* window)
: background_color_(kDefaultColor), menu_model_(NULL), window_(window) {
: background_color_(kDefaultColor), window_(window), menu_model_(NULL) {

This comment has been minimized.

@ckerr

ckerr May 16, 2018

Member

genuinely surprised that some of these, like order-of-initialization, weren't already warnings

@nornagon

This comment has been minimized.

Copy link
Contributor Author

nornagon commented May 16, 2018

I don't think removing virtual is an optimization at all; override implies virtual and the chromium-style checker just requires that you don't specify both.

@ckerr

ckerr approved these changes May 16, 2018

@poiru

poiru approved these changes May 16, 2018

@ckerr ckerr merged commit da0fd10 into electron:master May 16, 2018

10 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nornagon nornagon deleted the nornagon:chromium-style branch May 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.