Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2688483003:
exo: Refactor ShellSurface and WaylandRemoteShell (Closed)
|
Created:
3 years, 10 months ago by Dominik Laskowski Modified:
3 years, 10 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionexo: Refactor ShellSurface and WaylandRemoteShell
The gfx::Size part of ShellSurface::initial_bounds_ was abused to tell
shell surface types apart, i.e. their behavior implicitly depended on
otherwise unused initial state, e.g. gfx::Size(1, 1). This CL replaces
initial_bounds_ with an origin_ and explicit state that determines how
shell surface bounds are controlled.
This CL also includes miscellaneous style cleanups.
BUG=642894
TEST=None
Review-Url: https://codereview.chromium.org/2688483003
Cr-Commit-Position: refs/heads/master@{#450533}
Committed: https://chromium.googlesource.com/chromium/src/+/886e1c8ecc4e7cfdf003f9dddbc0f0f05b8b9180
Patch Set 1 #
Total comments: 22
Patch Set 2 : Address nits #
Total comments: 22
Patch Set 3 : Address more nits #Patch Set 4 : Revert unnecessary changes #
Dependent Patchsets: Messages
Total messages: 26 (17 generated)
The CQ bit was checked by domlaskowski@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...
domlaskowski@chromium.org changed reviewers: + oshima@chromium.org, reveman@chromium.org
PTAL. Split from https://codereview.chromium.org/2645663004/
https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:579: NOTREACHED(); How are we protected from a malicious client causing this code to be executed? https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:581: } nit: please add return statements in the switch cases above so you can add a NOTREACHED() here to detect if bounds_mode_ has entered an invalid state at runtime https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:590: switch (bounds_mode_) { same comments here https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1130: const bool client_controls_bounds = bounds_mode_ == BoundsMode::CLIENT; nit: remove 'const'. doesn't hurt but typically not something we use for local variables in exo code https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1133: if (!client_controls_bounds) can we add the observer unconditionally and instead adjust the observer callback code? https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1150: const bool movement_disabled = bounds_mode_ != BoundsMode::SHELL; nit: remove 'const' https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1185: const gfx::Vector2d origin_offset = pending_origin_offset_accumulator_; nit: remove 'const' https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1328: NOTREACHED(); Can a malicious client cause this? https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1330: } return above and NOTREACHED() here? https://codereview.chromium.org/2688483003/diff/1/components/exo/wayland/serv... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2688483003/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:29: #include <string> I don't understand what the changes to this file are for. Please explain.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:579: NOTREACHED(); On 2017/02/09 04:50:15, reveman wrote: > How are we protected from a malicious client causing this code to be executed? NOTREACHED only logs an error in release builds. https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:581: } On 2017/02/09 04:50:16, reveman wrote: > nit: please add return statements in the switch cases above so you can add a > NOTREACHED() here to detect if bounds_mode_ has entered an invalid state at > runtime Done. https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:590: switch (bounds_mode_) { On 2017/02/09 04:50:15, reveman wrote: > same comments here Done. https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1130: const bool client_controls_bounds = bounds_mode_ == BoundsMode::CLIENT; On 2017/02/09 04:50:15, reveman wrote: > nit: remove 'const'. doesn't hurt but typically not something we use for local > variables in exo code Removed variable along with check below. https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1133: if (!client_controls_bounds) On 2017/02/09 04:50:15, reveman wrote: > can we add the observer unconditionally and instead adjust the observer callback > code? Done. https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1150: const bool movement_disabled = bounds_mode_ != BoundsMode::SHELL; On 2017/02/09 04:50:15, reveman wrote: > nit: remove 'const' Done. https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1185: const gfx::Vector2d origin_offset = pending_origin_offset_accumulator_; On 2017/02/09 04:50:15, reveman wrote: > nit: remove 'const' Done. https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1328: NOTREACHED(); On 2017/02/09 04:50:15, reveman wrote: > Can a malicious client cause this? Only debug builds would crash. https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1330: } On 2017/02/09 04:50:15, reveman wrote: > return above and NOTREACHED() here? Done. https://codereview.chromium.org/2688483003/diff/1/components/exo/wayland/serv... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2688483003/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:29: #include <string> On 2017/02/09 04:50:16, reveman wrote: > I don't understand what the changes to this file are for. Please explain. The code for delayed sending of display metrics was generalized to multiplex display configuration events, i.e. multiple calls to ScheduleSendDisplayMetrics result in one call to SendDisplayMetrics. This is used by the dependent CL to coalesce DisplayObserver (and MaximizeModeObserver) callbacks.
lgtm % nits and last set of comments https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:579: NOTREACHED(); On 2017/02/09 at 23:35:28, Dominik Laskowski wrote: > On 2017/02/09 04:50:15, reveman wrote: > > How are we protected from a malicious client causing this code to be executed? > > NOTREACHED only logs an error in release builds. A malicious client being able to crash a debug build is not OK. NOTREACHED means it will not be reached unless there's a bug. https://codereview.chromium.org/2688483003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1328: NOTREACHED(); On 2017/02/09 at 23:35:28, Dominik Laskowski wrote: > On 2017/02/09 04:50:15, reveman wrote: > > Can a malicious client cause this? > > Only debug builds would crash. Debug builds crashing is not ok. https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:579: break; return; https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:597: break; return; https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:954: if (bounds_mode_ == BoundsMode::CLIENT || !widget_ || !surface_ || why is this needed? https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:1307: switch (bounds_mode_) { is this switch statement needed? what happens if you remove it? https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:1394: if (bounds_mode_ == BoundsMode::SHELL) { why is this check needed? should we return early instead if it's actually needed? https://codereview.chromium.org/2688483003/diff/20001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2688483003/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:2100: void ScheduleSendDisplayMetrics(int delay_ms = 0) { nit: remove default argument and explicitly pass 0 https://codereview.chromium.org/2688483003/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:2114: const display::Display primary = screen->GetPrimaryDisplay(); nit: s/primary/primary_display/ https://codereview.chromium.org/2688483003/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:2116: gfx::Size size = primary.size(); where is this used? is the temporary variable needed? maybe primary_size in that case
https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:579: break; On 2017/02/10 02:10:53, reveman wrote: > return; Done. https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:597: break; On 2017/02/10 02:10:53, reveman wrote: > return; Done. https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:954: if (bounds_mode_ == BoundsMode::CLIENT || !widget_ || !surface_ || On 2017/02/10 02:10:53, reveman wrote: > why is this needed? Oops, this belongs to the dependent CL. Removed. https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:1307: switch (bounds_mode_) { On 2017/02/10 02:10:53, reveman wrote: > is this switch statement needed? what happens if you remove it? Not necessary but simplifies the diff of the dependent CL, which fills out the CLIENT case. https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:1394: if (bounds_mode_ == BoundsMode::SHELL) { On 2017/02/10 02:10:53, reveman wrote: > why is this check needed? should we return early instead if it's actually > needed? It replaces the allow_set_bounds_in_maximized check, and IsResizing can only be true in the SHELL case. https://codereview.chromium.org/2688483003/diff/20001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2688483003/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:2100: void ScheduleSendDisplayMetrics(int delay_ms = 0) { On 2017/02/10 02:10:53, reveman wrote: > nit: remove default argument and explicitly pass 0 Done. https://codereview.chromium.org/2688483003/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:2114: const display::Display primary = screen->GetPrimaryDisplay(); On 2017/02/10 02:10:54, reveman wrote: > nit: s/primary/primary_display/ Done. https://codereview.chromium.org/2688483003/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:2116: gfx::Size size = primary.size(); On 2017/02/10 02:10:53, reveman wrote: > where is this used? is the temporary variable needed? maybe primary_size in that > case In the dependent CL, where it might be the size of the primary or virtual display.
The CQ bit was checked by domlaskowski@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:1307: switch (bounds_mode_) { On 2017/02/10 at 03:05:18, Dominik Laskowski wrote: > On 2017/02/10 02:10:53, reveman wrote: > > is this switch statement needed? what happens if you remove it? > > Not necessary but simplifies the diff of the dependent CL, which fills out the CLIENT case. Let's save switch statement until needed. That makes changes and history of changes easier to understand. https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:1394: if (bounds_mode_ == BoundsMode::SHELL) { On 2017/02/10 at 03:05:18, Dominik Laskowski wrote: > On 2017/02/10 02:10:53, reveman wrote: > > why is this check needed? should we return early instead if it's actually > > needed? > > It replaces the allow_set_bounds_in_maximized check, and IsResizing can only be true in the SHELL case. Let's not make the assumption that IsResizing can only be true in the SHELL case. I think it's better to avoid that if possible and it will also make this patch a bit easier to understand here. https://codereview.chromium.org/2688483003/diff/20001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2688483003/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:2116: gfx::Size size = primary.size(); On 2017/02/10 at 03:05:19, Dominik Laskowski wrote: > On 2017/02/10 02:10:53, reveman wrote: > > where is this used? is the temporary variable needed? maybe primary_size in that > > case > > In the dependent CL, where it might be the size of the primary or virtual display. Save that for the follow up CL. Changes are hard to understand when they include things like this that will only make sense in a follow up patch.
The CQ bit was checked by domlaskowski@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...
https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:1307: switch (bounds_mode_) { On 2017/02/10 05:51:54, reveman wrote: > On 2017/02/10 at 03:05:18, Dominik Laskowski wrote: > > On 2017/02/10 02:10:53, reveman wrote: > > > is this switch statement needed? what happens if you remove it? > > > > Not necessary but simplifies the diff of the dependent CL, which fills out the > CLIENT case. > > Let's save switch statement until needed. That makes changes and history of > changes easier to understand. Done. https://codereview.chromium.org/2688483003/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:1394: if (bounds_mode_ == BoundsMode::SHELL) { On 2017/02/10 05:51:53, reveman wrote: > On 2017/02/10 at 03:05:18, Dominik Laskowski wrote: > > On 2017/02/10 02:10:53, reveman wrote: > > > why is this check needed? should we return early instead if it's actually > > > needed? > > > > It replaces the allow_set_bounds_in_maximized check, and IsResizing can only > be true in the SHELL case. > > Let's not make the assumption that IsResizing can only be true in the SHELL > case. I think it's better to avoid that if possible and it will also make this > patch a bit easier to understand here. Reverted. https://codereview.chromium.org/2688483003/diff/20001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2688483003/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:2116: gfx::Size size = primary.size(); On 2017/02/10 05:51:54, reveman wrote: > On 2017/02/10 at 03:05:19, Dominik Laskowski wrote: > > On 2017/02/10 02:10:53, reveman wrote: > > > where is this used? is the temporary variable needed? maybe primary_size in > that > > > case > > > > In the dependent CL, where it might be the size of the primary or virtual > display. > > Save that for the follow up CL. Changes are hard to understand when they include > things like this that will only make sense in a follow up patch. Done.
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 domlaskowski@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2688483003/#ps60001 (title: "Revert unnecessary changes")
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": 1487114572551130,
"parent_rev": "9db26fbab2a06135a5519f21324d1a01851cef3d", "commit_rev":
"886e1c8ecc4e7cfdf003f9dddbc0f0f05b8b9180"}
Message was sent while issue was closed.
Description was changed from ========== exo: Refactor ShellSurface and WaylandRemoteShell The gfx::Size part of ShellSurface::initial_bounds_ was abused to tell shell surface types apart, i.e. their behavior implicitly depended on otherwise unused initial state, e.g. gfx::Size(1, 1). This CL replaces initial_bounds_ with an origin_ and explicit state that determines how shell surface bounds are controlled. This CL also includes miscellaneous style cleanups. BUG=642894 TEST=None ========== to ========== exo: Refactor ShellSurface and WaylandRemoteShell The gfx::Size part of ShellSurface::initial_bounds_ was abused to tell shell surface types apart, i.e. their behavior implicitly depended on otherwise unused initial state, e.g. gfx::Size(1, 1). This CL replaces initial_bounds_ with an origin_ and explicit state that determines how shell surface bounds are controlled. This CL also includes miscellaneous style cleanups. BUG=642894 TEST=None Review-Url: https://codereview.chromium.org/2688483003 Cr-Commit-Position: refs/heads/master@{#450533} Committed: https://chromium.googlesource.com/chromium/src/+/886e1c8ecc4e7cfdf003f9dddbc0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/886e1c8ecc4e7cfdf003f9dddbc0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
