Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1140773003:
[1.5/2][Ozone-Drm] Keep track of display origin (Closed)
|
Created:
5 years, 7 months ago by dnicoara Modified:
5 years, 7 months ago Reviewers:
spang CC:
chromium-reviews, kalyank, piman+watch_chromium.org, ozone-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@handle-display-init3 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Ozone-Drm] Keep track of display origin
Otherwise we lose track of display layout. In particular this is a
problem when dealing with software mirror mode since the state goes out
of sync and the second monitor will have the same origin as the primary
even though it's supposed to be right-of primary.
Follow-up to https://codereview.chromium.org/1129923004/ to fix software
mirror mode.
BUG=484294
TEST=Test software mirror mode and make sure it works
Committed: https://crrev.com/9d137bfcbb5420949b90dedd42dafb763c827e4a
Cr-Commit-Position: refs/heads/master@{#329717}
Patch Set 1 #
Total comments: 2
Patch Set 2 : . #
Messages
Total messages: 30 (6 generated)
dnicoara@chromium.org changed reviewers: + spang@chromium.org
Opsie, looks like I went too crazy with the simplifications and I removed too much.
The CQ bit was checked by dnicoara@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/1140773003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc (right): https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:88: old_displays.begin(), old_displays.end(), I'm not a fan of this. Can you update displays_ first, then convert displays_ to std::vector<DisplaySnapshot_Params>()?
https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc (right): https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:88: old_displays.begin(), old_displays.end(), On 2015/05/13 19:01:20, spang wrote: > I'm not a fan of this. > > Can you update displays_ first, then convert displays_ to > std::vector<DisplaySnapshot_Params>()? Hmm, it is simpler to create the std::vector<DisplaySnapshot_Params>, then update displays_ since the display ID is needed from DisplaySnapshot_Params. I don't think this has a huge mental overhead given that the important operations are just push_back(). Though I'm happy to change if you think that's cleaner :)
On 2015/05/13 19:10:36, dnicoara wrote: > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc (right): > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:88: old_displays.begin(), > old_displays.end(), > On 2015/05/13 19:01:20, spang wrote: > > I'm not a fan of this. > > > > Can you update displays_ first, then convert displays_ to > > std::vector<DisplaySnapshot_Params>()? > > Hmm, it is simpler to create the std::vector<DisplaySnapshot_Params>, then > update displays_ since the display ID is needed from DisplaySnapshot_Params. I > don't think this has a huge mental overhead given that the important operations > are just push_back(). Though I'm happy to change if you think that's cleaner :) I think it is helpful to break things apart into higher level operations. Even if writing the composition of update & get in one go is smaller that writing them separately, the factored version is easier to understand (which is my definition of "simpler"). It was pretty readable before this patch but now you need to integrate bits from the old state too. Do we actually need to need to build an new DrmDisplay for each (crtc_id, connector_id)? Why not update it (then you preserve origin & any future state for free)?
On 2015/05/13 19:28:23, spang wrote: > On 2015/05/13 19:10:36, dnicoara wrote: > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc (right): > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:88: old_displays.begin(), > > old_displays.end(), > > On 2015/05/13 19:01:20, spang wrote: > > > I'm not a fan of this. > > > > > > Can you update displays_ first, then convert displays_ to > > > std::vector<DisplaySnapshot_Params>()? > > > > Hmm, it is simpler to create the std::vector<DisplaySnapshot_Params>, then > > update displays_ since the display ID is needed from DisplaySnapshot_Params. I > > don't think this has a huge mental overhead given that the important > operations > > are just push_back(). Though I'm happy to change if you think that's cleaner > :) > > I think it is helpful to break things apart into higher level operations. Even > if writing the composition of update & get in one go is smaller that writing > them separately, the factored version is easier to understand (which is my > definition of "simpler"). It was pretty readable before this patch but now you > need to integrate bits from the old state too. > > Do we actually need to need to build an new DrmDisplay for each (crtc_id, > connector_id)? Why not update it (then you preserve origin & any future state > for free)? One idea would be to put ParseDrmDisplay into DrmDisplay as UpdateDrmDisplay.
On 2015/05/13 19:35:15, spang wrote: > On 2015/05/13 19:28:23, spang wrote: > > On 2015/05/13 19:10:36, dnicoara wrote: > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > > File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc (right): > > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:88: > old_displays.begin(), > > > old_displays.end(), > > > On 2015/05/13 19:01:20, spang wrote: > > > > I'm not a fan of this. > > > > > > > > Can you update displays_ first, then convert displays_ to > > > > std::vector<DisplaySnapshot_Params>()? > > > > > > Hmm, it is simpler to create the std::vector<DisplaySnapshot_Params>, then > > > update displays_ since the display ID is needed from DisplaySnapshot_Params. > I > > > don't think this has a huge mental overhead given that the important > > operations > > > are just push_back(). Though I'm happy to change if you think that's cleaner > > :) > > > > I think it is helpful to break things apart into higher level operations. Even > > if writing the composition of update & get in one go is smaller that writing > > them separately, the factored version is easier to understand (which is my > > definition of "simpler"). It was pretty readable before this patch but now you > > need to integrate bits from the old state too. > > > > Do we actually need to need to build an new DrmDisplay for each (crtc_id, > > connector_id)? Why not update it (then you preserve origin & any future state > > for free)? > > One idea would be to put ParseDrmDisplay into DrmDisplay as UpdateDrmDisplay. Something like patch set 2?
On 2015/05/13 20:03:01, dnicoara wrote: > On 2015/05/13 19:35:15, spang wrote: > > On 2015/05/13 19:28:23, spang wrote: > > > On 2015/05/13 19:10:36, dnicoara wrote: > > > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > > > File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > > > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:88: > > old_displays.begin(), > > > > old_displays.end(), > > > > On 2015/05/13 19:01:20, spang wrote: > > > > > I'm not a fan of this. > > > > > > > > > > Can you update displays_ first, then convert displays_ to > > > > > std::vector<DisplaySnapshot_Params>()? > > > > > > > > Hmm, it is simpler to create the std::vector<DisplaySnapshot_Params>, then > > > > update displays_ since the display ID is needed from > DisplaySnapshot_Params. > > I > > > > don't think this has a huge mental overhead given that the important > > > operations > > > > are just push_back(). Though I'm happy to change if you think that's > cleaner > > > :) > > > > > > I think it is helpful to break things apart into higher level operations. > Even > > > if writing the composition of update & get in one go is smaller that writing > > > them separately, the factored version is easier to understand (which is my > > > definition of "simpler"). It was pretty readable before this patch but now > you > > > need to integrate bits from the old state too. > > > > > > Do we actually need to need to build an new DrmDisplay for each (crtc_id, > > > connector_id)? Why not update it (then you preserve origin & any future > state > > > for free)? > > > > One idea would be to put ParseDrmDisplay into DrmDisplay as UpdateDrmDisplay. > > Something like patch set 2? I was hoping for a more substantial separation. std::vector<DisplaySnapshot_Params> GetDisplays() { std::vector<DisplaySnapshot_Params> snapshots; UpdateDisplays(); for (const auto& display : displays_) snapshots.push_back(display.Snapshot()); return snapshots; }
On 2015/05/13 20:16:46, spang wrote: > On 2015/05/13 20:03:01, dnicoara wrote: > > On 2015/05/13 19:35:15, spang wrote: > > > On 2015/05/13 19:28:23, spang wrote: > > > > On 2015/05/13 19:10:36, dnicoara wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > > > > File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > > > > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:88: > > > old_displays.begin(), > > > > > old_displays.end(), > > > > > On 2015/05/13 19:01:20, spang wrote: > > > > > > I'm not a fan of this. > > > > > > > > > > > > Can you update displays_ first, then convert displays_ to > > > > > > std::vector<DisplaySnapshot_Params>()? > > > > > > > > > > Hmm, it is simpler to create the std::vector<DisplaySnapshot_Params>, > then > > > > > update displays_ since the display ID is needed from > > DisplaySnapshot_Params. > > > I > > > > > don't think this has a huge mental overhead given that the important > > > > operations > > > > > are just push_back(). Though I'm happy to change if you think that's > > cleaner > > > > :) > > > > > > > > I think it is helpful to break things apart into higher level operations. > > Even > > > > if writing the composition of update & get in one go is smaller that > writing > > > > them separately, the factored version is easier to understand (which is my > > > > definition of "simpler"). It was pretty readable before this patch but now > > you > > > > need to integrate bits from the old state too. > > > > > > > > Do we actually need to need to build an new DrmDisplay for each (crtc_id, > > > > connector_id)? Why not update it (then you preserve origin & any future > > state > > > > for free)? > > > > > > One idea would be to put ParseDrmDisplay into DrmDisplay as > UpdateDrmDisplay. > > > > Something like patch set 2? > > I was hoping for a more substantial separation. > > std::vector<DisplaySnapshot_Params> GetDisplays() { > std::vector<DisplaySnapshot_Params> snapshots; > > UpdateDisplays(); > > for (const auto& display : displays_) > snapshots.push_back(display.Snapshot()); > > return snapshots; > } I think I see why you want to CreateDisplaySnapshotParams in its current form, though. You want share with the new (not yet added) browser code. And you can't put DrmDisplay in common/ because it uses ScreenManager. Maybe we should make DrmDisplay not need to call ScreenManager so that it's usable as a general utility. Anyway, this is enough change for now - lgtm.
The CQ bit was checked by spang@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/1140773003/20001
On 2015/05/13 20:21:40, spang wrote: > On 2015/05/13 20:16:46, spang wrote: > > On 2015/05/13 20:03:01, dnicoara wrote: > > > On 2015/05/13 19:35:15, spang wrote: > > > > On 2015/05/13 19:28:23, spang wrote: > > > > > On 2015/05/13 19:10:36, dnicoara wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > > > > > File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > > > > > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:88: > > > > old_displays.begin(), > > > > > > old_displays.end(), > > > > > > On 2015/05/13 19:01:20, spang wrote: > > > > > > > I'm not a fan of this. > > > > > > > > > > > > > > Can you update displays_ first, then convert displays_ to > > > > > > > std::vector<DisplaySnapshot_Params>()? > > > > > > > > > > > > Hmm, it is simpler to create the std::vector<DisplaySnapshot_Params>, > > then > > > > > > update displays_ since the display ID is needed from > > > DisplaySnapshot_Params. > > > > I > > > > > > don't think this has a huge mental overhead given that the important > > > > > operations > > > > > > are just push_back(). Though I'm happy to change if you think that's > > > cleaner > > > > > :) > > > > > > > > > > I think it is helpful to break things apart into higher level > operations. > > > Even > > > > > if writing the composition of update & get in one go is smaller that > > writing > > > > > them separately, the factored version is easier to understand (which is > my > > > > > definition of "simpler"). It was pretty readable before this patch but > now > > > you > > > > > need to integrate bits from the old state too. > > > > > > > > > > Do we actually need to need to build an new DrmDisplay for each > (crtc_id, > > > > > connector_id)? Why not update it (then you preserve origin & any future > > > state > > > > > for free)? > > > > > > > > One idea would be to put ParseDrmDisplay into DrmDisplay as > > UpdateDrmDisplay. > > > > > > Something like patch set 2? > > > > I was hoping for a more substantial separation. > > > > std::vector<DisplaySnapshot_Params> GetDisplays() { > > std::vector<DisplaySnapshot_Params> snapshots; > > > > UpdateDisplays(); > > > > for (const auto& display : displays_) > > snapshots.push_back(display.Snapshot()); > > > > return snapshots; > > } > > I think I see why you want to CreateDisplaySnapshotParams in its current form, > though. You want share with the new (not yet added) browser code. And you can't > put DrmDisplay in common/ because it uses ScreenManager. > > Maybe we should make DrmDisplay not need to call ScreenManager so that it's > usable as a general utility. Anyway, this is enough change for now - lgtm. Yep, I want the CreateDisplay*Params to be shared between the 2. At the same time I want to restrict the APIs used to configure the displays to the GPU code since we couldn't be doing any such operation on the browser side. The other reason I don't want to push much of this logic into DrmDisplay is because DisplaySnapshot_Params is meant for IPC-ing only, so I want to keep the snapshot cached in DrmDisplay. Similarly I don't want the HardwareDisplayControllerInfo cached since it also represents a snapshot of the CRTC and Connector state. I think this is going to become simpler after a few more changes. Specifically, I'd like to redo GetAvailableDisplayControllerInfos() such that it can be simpler to use with DrmDisplay and with the browser side code. Though perhaps you are right and there needs to be a shared DrmDisplayFoo base class that deals with my concerns.
On 2015/05/13 20:31:52, dnicoara wrote: > On 2015/05/13 20:21:40, spang wrote: > > On 2015/05/13 20:16:46, spang wrote: > > > On 2015/05/13 20:03:01, dnicoara wrote: > > > > On 2015/05/13 19:35:15, spang wrote: > > > > > On 2015/05/13 19:28:23, spang wrote: > > > > > > On 2015/05/13 19:10:36, dnicoara wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > > > > > > File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > > > > > > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:88: > > > > > old_displays.begin(), > > > > > > > old_displays.end(), > > > > > > > On 2015/05/13 19:01:20, spang wrote: > > > > > > > > I'm not a fan of this. > > > > > > > > > > > > > > > > Can you update displays_ first, then convert displays_ to > > > > > > > > std::vector<DisplaySnapshot_Params>()? > > > > > > > > > > > > > > Hmm, it is simpler to create the > std::vector<DisplaySnapshot_Params>, > > > then > > > > > > > update displays_ since the display ID is needed from > > > > DisplaySnapshot_Params. > > > > > I > > > > > > > don't think this has a huge mental overhead given that the important > > > > > > operations > > > > > > > are just push_back(). Though I'm happy to change if you think that's > > > > cleaner > > > > > > :) > > > > > > > > > > > > I think it is helpful to break things apart into higher level > > operations. > > > > Even > > > > > > if writing the composition of update & get in one go is smaller that > > > writing > > > > > > them separately, the factored version is easier to understand (which > is > > my > > > > > > definition of "simpler"). It was pretty readable before this patch but > > now > > > > you > > > > > > need to integrate bits from the old state too. > > > > > > > > > > > > Do we actually need to need to build an new DrmDisplay for each > > (crtc_id, > > > > > > connector_id)? Why not update it (then you preserve origin & any > future > > > > state > > > > > > for free)? > > > > > > > > > > One idea would be to put ParseDrmDisplay into DrmDisplay as > > > UpdateDrmDisplay. > > > > > > > > Something like patch set 2? > > > > > > I was hoping for a more substantial separation. > > > > > > std::vector<DisplaySnapshot_Params> GetDisplays() { > > > std::vector<DisplaySnapshot_Params> snapshots; > > > > > > UpdateDisplays(); > > > > > > for (const auto& display : displays_) > > > snapshots.push_back(display.Snapshot()); > > > > > > return snapshots; > > > } > > > > I think I see why you want to CreateDisplaySnapshotParams in its current form, > > though. You want share with the new (not yet added) browser code. And you > can't > > put DrmDisplay in common/ because it uses ScreenManager. > > > > Maybe we should make DrmDisplay not need to call ScreenManager so that it's > > usable as a general utility. Anyway, this is enough change for now - lgtm. > > Yep, I want the CreateDisplay*Params to be shared between the 2. At the same > time I want to restrict the APIs used to configure the displays to the GPU code > since we couldn't be doing any such operation on the browser side. > > The other reason I don't want to push much of this logic into DrmDisplay is > because DisplaySnapshot_Params is meant for IPC-ing only, so I want to keep the > snapshot cached in DrmDisplay. I don't follow, you do want to keep it cached or don't? I agree with not using DisplaySnapshot_Params outside of IPC. In fact I don't like that we're reusing it other places for setup (in say CreateDisplaySnapshotParams). > Similarly I don't want the > HardwareDisplayControllerInfo cached since it also represents a snapshot of the > CRTC and Connector state. > > I think this is going to become simpler after a few more changes. Specifically, > I'd like to redo GetAvailableDisplayControllerInfos() such that it can be > simpler to use with DrmDisplay and with the browser side code. Though perhaps > you are right and there needs to be a shared DrmDisplayFoo base class that deals > with my concerns. IMO there is no reason to withhold Configure() from the browser side object. The syscall is there, the fd is available, and we have info about the displays. The fact that we have to be careful to to cause races by actually using it from two processes at the same time is nothing new.
On 2015/05/13 20:43:48, spang wrote: > On 2015/05/13 20:31:52, dnicoara wrote: > > On 2015/05/13 20:21:40, spang wrote: > > > On 2015/05/13 20:16:46, spang wrote: > > > > On 2015/05/13 20:03:01, dnicoara wrote: > > > > > On 2015/05/13 19:35:15, spang wrote: > > > > > > On 2015/05/13 19:28:23, spang wrote: > > > > > > > On 2015/05/13 19:10:36, dnicoara wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > > > > > > > File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > > > > > > > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:88: > > > > > > old_displays.begin(), > > > > > > > > old_displays.end(), > > > > > > > > On 2015/05/13 19:01:20, spang wrote: > > > > > > > > > I'm not a fan of this. > > > > > > > > > > > > > > > > > > Can you update displays_ first, then convert displays_ to > > > > > > > > > std::vector<DisplaySnapshot_Params>()? > > > > > > > > > > > > > > > > Hmm, it is simpler to create the > > std::vector<DisplaySnapshot_Params>, > > > > then > > > > > > > > update displays_ since the display ID is needed from > > > > > DisplaySnapshot_Params. > > > > > > I > > > > > > > > don't think this has a huge mental overhead given that the > important > > > > > > > operations > > > > > > > > are just push_back(). Though I'm happy to change if you think > that's > > > > > cleaner > > > > > > > :) > > > > > > > > > > > > > > I think it is helpful to break things apart into higher level > > > operations. > > > > > Even > > > > > > > if writing the composition of update & get in one go is smaller that > > > > writing > > > > > > > them separately, the factored version is easier to understand (which > > is > > > my > > > > > > > definition of "simpler"). It was pretty readable before this patch > but > > > now > > > > > you > > > > > > > need to integrate bits from the old state too. > > > > > > > > > > > > > > Do we actually need to need to build an new DrmDisplay for each > > > (crtc_id, > > > > > > > connector_id)? Why not update it (then you preserve origin & any > > future > > > > > state > > > > > > > for free)? > > > > > > > > > > > > One idea would be to put ParseDrmDisplay into DrmDisplay as > > > > UpdateDrmDisplay. > > > > > > > > > > Something like patch set 2? > > > > > > > > I was hoping for a more substantial separation. > > > > > > > > std::vector<DisplaySnapshot_Params> GetDisplays() { > > > > std::vector<DisplaySnapshot_Params> snapshots; > > > > > > > > UpdateDisplays(); > > > > > > > > for (const auto& display : displays_) > > > > snapshots.push_back(display.Snapshot()); > > > > > > > > return snapshots; > > > > } > > > > > > I think I see why you want to CreateDisplaySnapshotParams in its current > form, > > > though. You want share with the new (not yet added) browser code. And you > > can't > > > put DrmDisplay in common/ because it uses ScreenManager. > > > > > > Maybe we should make DrmDisplay not need to call ScreenManager so that it's > > > usable as a general utility. Anyway, this is enough change for now - lgtm. > > > > Yep, I want the CreateDisplay*Params to be shared between the 2. At the same > > time I want to restrict the APIs used to configure the displays to the GPU > code > > since we couldn't be doing any such operation on the browser side. > > > > The other reason I don't want to push much of this logic into DrmDisplay is > > because DisplaySnapshot_Params is meant for IPC-ing only, so I want to keep > the > > snapshot cached in DrmDisplay. > > I don't follow, you do want to keep it cached or don't? Sorry, that was supposed to be a "don't", I don't want it cached. > > I agree with not using DisplaySnapshot_Params outside of IPC. In fact I don't > like that we're reusing it other places for setup (in say > CreateDisplaySnapshotParams). > > > Similarly I don't want the > > HardwareDisplayControllerInfo cached since it also represents a snapshot of > the > > CRTC and Connector state. > > > > I think this is going to become simpler after a few more changes. > Specifically, > > I'd like to redo GetAvailableDisplayControllerInfos() such that it can be > > simpler to use with DrmDisplay and with the browser side code. Though perhaps > > you are right and there needs to be a shared DrmDisplayFoo base class that > deals > > with my concerns. > > IMO there is no reason to withhold Configure() from the browser side object. The > syscall is there, the fd is available, and we have info about the displays. The > fact that we have to be careful to to cause races by actually using it from two > processes at the same time is nothing new.
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 dnicoara@chromium.org
On 2015/05/13 20:50:16, dnicoara wrote: > On 2015/05/13 20:43:48, spang wrote: > > On 2015/05/13 20:31:52, dnicoara wrote: > > > On 2015/05/13 20:21:40, spang wrote: > > > > On 2015/05/13 20:16:46, spang wrote: > > > > > On 2015/05/13 20:03:01, dnicoara wrote: > > > > > > On 2015/05/13 19:35:15, spang wrote: > > > > > > > On 2015/05/13 19:28:23, spang wrote: > > > > > > > > On 2015/05/13 19:10:36, dnicoara wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > > > > > > > > File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > > > > > > > > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:88: > > > > > > > old_displays.begin(), > > > > > > > > > old_displays.end(), > > > > > > > > > On 2015/05/13 19:01:20, spang wrote: > > > > > > > > > > I'm not a fan of this. > > > > > > > > > > > > > > > > > > > > Can you update displays_ first, then convert displays_ to > > > > > > > > > > std::vector<DisplaySnapshot_Params>()? > > > > > > > > > > > > > > > > > > Hmm, it is simpler to create the > > > std::vector<DisplaySnapshot_Params>, > > > > > then > > > > > > > > > update displays_ since the display ID is needed from > > > > > > DisplaySnapshot_Params. > > > > > > > I > > > > > > > > > don't think this has a huge mental overhead given that the > > important > > > > > > > > operations > > > > > > > > > are just push_back(). Though I'm happy to change if you think > > that's > > > > > > cleaner > > > > > > > > :) > > > > > > > > > > > > > > > > I think it is helpful to break things apart into higher level > > > > operations. > > > > > > Even > > > > > > > > if writing the composition of update & get in one go is smaller > that > > > > > writing > > > > > > > > them separately, the factored version is easier to understand > (which > > > is > > > > my > > > > > > > > definition of "simpler"). It was pretty readable before this patch > > but > > > > now > > > > > > you > > > > > > > > need to integrate bits from the old state too. > > > > > > > > > > > > > > > > Do we actually need to need to build an new DrmDisplay for each > > > > (crtc_id, > > > > > > > > connector_id)? Why not update it (then you preserve origin & any > > > future > > > > > > state > > > > > > > > for free)? > > > > > > > > > > > > > > One idea would be to put ParseDrmDisplay into DrmDisplay as > > > > > UpdateDrmDisplay. > > > > > > > > > > > > Something like patch set 2? > > > > > > > > > > I was hoping for a more substantial separation. > > > > > > > > > > std::vector<DisplaySnapshot_Params> GetDisplays() { > > > > > std::vector<DisplaySnapshot_Params> snapshots; > > > > > > > > > > UpdateDisplays(); > > > > > > > > > > for (const auto& display : displays_) > > > > > snapshots.push_back(display.Snapshot()); > > > > > > > > > > return snapshots; > > > > > } > > > > > > > > I think I see why you want to CreateDisplaySnapshotParams in its current > > form, > > > > though. You want share with the new (not yet added) browser code. And you > > > can't > > > > put DrmDisplay in common/ because it uses ScreenManager. > > > > > > > > Maybe we should make DrmDisplay not need to call ScreenManager so that > it's > > > > usable as a general utility. Anyway, this is enough change for now - lgtm. > > > > > > Yep, I want the CreateDisplay*Params to be shared between the 2. At the same > > > time I want to restrict the APIs used to configure the displays to the GPU > > code > > > since we couldn't be doing any such operation on the browser side. > > > > > > The other reason I don't want to push much of this logic into DrmDisplay is > > > because DisplaySnapshot_Params is meant for IPC-ing only, so I want to keep > > the > > > snapshot cached in DrmDisplay. > > > > I don't follow, you do want to keep it cached or don't? > > Sorry, that was supposed to be a "don't", I don't want it cached. You can build one from a DrmDisplay, though, right? I didn't mean to cache a copy, just essentially implementing DisplaySnapshot_Params SnapshotDrmDisplay(const DrmDisplay* display); > > > > > I agree with not using DisplaySnapshot_Params outside of IPC. In fact I don't > > like that we're reusing it other places for setup (in say > > CreateDisplaySnapshotParams). > > > > > Similarly I don't want the > > > HardwareDisplayControllerInfo cached since it also represents a snapshot of > > the > > > CRTC and Connector state. > > > > > > I think this is going to become simpler after a few more changes. > > Specifically, > > > I'd like to redo GetAvailableDisplayControllerInfos() such that it can be > > > simpler to use with DrmDisplay and with the browser side code. Though > perhaps > > > you are right and there needs to be a shared DrmDisplayFoo base class that > > deals > > > with my concerns. > > > > IMO there is no reason to withhold Configure() from the browser side object. > The > > syscall is there, the fd is available, and we have info about the displays. > The > > fact that we have to be careful to to cause races by actually using it from > two > > processes at the same time is nothing new.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1140773003/20001
On 2015/05/13 20:59:49, spang wrote: > On 2015/05/13 20:50:16, dnicoara wrote: > > On 2015/05/13 20:43:48, spang wrote: > > > On 2015/05/13 20:31:52, dnicoara wrote: > > > > On 2015/05/13 20:21:40, spang wrote: > > > > > On 2015/05/13 20:16:46, spang wrote: > > > > > > On 2015/05/13 20:03:01, dnicoara wrote: > > > > > > > On 2015/05/13 19:35:15, spang wrote: > > > > > > > > On 2015/05/13 19:28:23, spang wrote: > > > > > > > > > On 2015/05/13 19:10:36, dnicoara wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > > > > > > > > > File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > > > > > > > > > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:88: > > > > > > > > old_displays.begin(), > > > > > > > > > > old_displays.end(), > > > > > > > > > > On 2015/05/13 19:01:20, spang wrote: > > > > > > > > > > > I'm not a fan of this. > > > > > > > > > > > > > > > > > > > > > > Can you update displays_ first, then convert displays_ to > > > > > > > > > > > std::vector<DisplaySnapshot_Params>()? > > > > > > > > > > > > > > > > > > > > Hmm, it is simpler to create the > > > > std::vector<DisplaySnapshot_Params>, > > > > > > then > > > > > > > > > > update displays_ since the display ID is needed from > > > > > > > DisplaySnapshot_Params. > > > > > > > > I > > > > > > > > > > don't think this has a huge mental overhead given that the > > > important > > > > > > > > > operations > > > > > > > > > > are just push_back(). Though I'm happy to change if you think > > > that's > > > > > > > cleaner > > > > > > > > > :) > > > > > > > > > > > > > > > > > > I think it is helpful to break things apart into higher level > > > > > operations. > > > > > > > Even > > > > > > > > > if writing the composition of update & get in one go is smaller > > that > > > > > > writing > > > > > > > > > them separately, the factored version is easier to understand > > (which > > > > is > > > > > my > > > > > > > > > definition of "simpler"). It was pretty readable before this > patch > > > but > > > > > now > > > > > > > you > > > > > > > > > need to integrate bits from the old state too. > > > > > > > > > > > > > > > > > > Do we actually need to need to build an new DrmDisplay for each > > > > > (crtc_id, > > > > > > > > > connector_id)? Why not update it (then you preserve origin & any > > > > future > > > > > > > state > > > > > > > > > for free)? > > > > > > > > > > > > > > > > One idea would be to put ParseDrmDisplay into DrmDisplay as > > > > > > UpdateDrmDisplay. > > > > > > > > > > > > > > Something like patch set 2? > > > > > > > > > > > > I was hoping for a more substantial separation. > > > > > > > > > > > > std::vector<DisplaySnapshot_Params> GetDisplays() { > > > > > > std::vector<DisplaySnapshot_Params> snapshots; > > > > > > > > > > > > UpdateDisplays(); > > > > > > > > > > > > for (const auto& display : displays_) > > > > > > snapshots.push_back(display.Snapshot()); > > > > > > > > > > > > return snapshots; > > > > > > } > > > > > > > > > > I think I see why you want to CreateDisplaySnapshotParams in its current > > > form, > > > > > though. You want share with the new (not yet added) browser code. And > you > > > > can't > > > > > put DrmDisplay in common/ because it uses ScreenManager. > > > > > > > > > > Maybe we should make DrmDisplay not need to call ScreenManager so that > > it's > > > > > usable as a general utility. Anyway, this is enough change for now - > lgtm. > > > > > > > > Yep, I want the CreateDisplay*Params to be shared between the 2. At the > same > > > > time I want to restrict the APIs used to configure the displays to the GPU > > > code > > > > since we couldn't be doing any such operation on the browser side. > > > > > > > > The other reason I don't want to push much of this logic into DrmDisplay > is > > > > because DisplaySnapshot_Params is meant for IPC-ing only, so I want to > keep > > > the > > > > snapshot cached in DrmDisplay. > > > > > > I don't follow, you do want to keep it cached or don't? > > > > Sorry, that was supposed to be a "don't", I don't want it cached. > > You can build one from a DrmDisplay, though, right? I didn't mean to cache a > copy, just essentially implementing > > DisplaySnapshot_Params SnapshotDrmDisplay(const DrmDisplay* display); > Then the HardwareDisplayControllerInfo would need to be cached. I was hoping I wouldn't need to cache that. The reason I don't want to cache the HDCI is because it keeps a snapshot of the hardware configuration. That changes once a DrmDisplay::Configure() call is done (since the mode changes). > > > > > > > > I agree with not using DisplaySnapshot_Params outside of IPC. In fact I > don't > > > like that we're reusing it other places for setup (in say > > > CreateDisplaySnapshotParams). > > > > > > > Similarly I don't want the > > > > HardwareDisplayControllerInfo cached since it also represents a snapshot > of > > > the > > > > CRTC and Connector state. > > > > > > > > I think this is going to become simpler after a few more changes. > > > Specifically, > > > > I'd like to redo GetAvailableDisplayControllerInfos() such that it can be > > > > simpler to use with DrmDisplay and with the browser side code. Though > > perhaps > > > > you are right and there needs to be a shared DrmDisplayFoo base class that > > > deals > > > > with my concerns. > > > > > > IMO there is no reason to withhold Configure() from the browser side object. > > The > > > syscall is there, the fd is available, and we have info about the displays. > > The > > > fact that we have to be careful to to cause races by actually using it from > > two > > > processes at the same time is nothing new.
On Wed, May 13, 2015 at 5:03 PM, <dnicoara@chromium.org> wrote: > On 2015/05/13 20:59:49, spang wrote: >> >> On 2015/05/13 20:50:16, dnicoara wrote: >> > On 2015/05/13 20:43:48, spang wrote: >> > > On 2015/05/13 20:31:52, dnicoara wrote: >> > > > On 2015/05/13 20:21:40, spang wrote: >> > > > > On 2015/05/13 20:16:46, spang wrote: >> > > > > > On 2015/05/13 20:03:01, dnicoara wrote: >> > > > > > > On 2015/05/13 19:35:15, spang wrote: >> > > > > > > > On 2015/05/13 19:28:23, spang wrote: >> > > > > > > > > On 2015/05/13 19:10:36, dnicoara wrote: >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... >> >> > > > > > > > > > File >> > > > > > > > > > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc >> > (right): >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... >> >> > > > > > > > > > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:88: >> > > > > > > > old_displays.begin(), >> > > > > > > > > > old_displays.end(), >> > > > > > > > > > On 2015/05/13 19:01:20, spang wrote: >> > > > > > > > > > > I'm not a fan of this. >> > > > > > > > > > > >> > > > > > > > > > > Can you update displays_ first, then convert displays_ >> > > > > > > > > > > to >> > > > > > > > > > > std::vector<DisplaySnapshot_Params>()? >> > > > > > > > > > >> > > > > > > > > > Hmm, it is simpler to create the >> > > > std::vector<DisplaySnapshot_Params>, >> > > > > > then >> > > > > > > > > > update displays_ since the display ID is needed from >> > > > > > > DisplaySnapshot_Params. >> > > > > > > > I >> > > > > > > > > > don't think this has a huge mental overhead given that >> > > > > > > > > > the >> > > important >> > > > > > > > > operations >> > > > > > > > > > are just push_back(). Though I'm happy to change if you > > think >> >> > > that's >> > > > > > > cleaner >> > > > > > > > > :) >> > > > > > > > > >> > > > > > > > > I think it is helpful to break things apart into higher >> > > > > > > > > level >> > > > > operations. >> > > > > > > Even >> > > > > > > > > if writing the composition of update & get in one go is > > smaller >> >> > that >> > > > > > writing >> > > > > > > > > them separately, the factored version is easier to >> > > > > > > > > understand >> > (which >> > > > is >> > > > > my >> > > > > > > > > definition of "simpler"). It was pretty readable before >> > > > > > > > > this >> patch >> > > but >> > > > > now >> > > > > > > you >> > > > > > > > > need to integrate bits from the old state too. >> > > > > > > > > >> > > > > > > > > Do we actually need to need to build an new DrmDisplay for > > each >> >> > > > > (crtc_id, >> > > > > > > > > connector_id)? Why not update it (then you preserve origin >> > > > > > > > > & > > any >> >> > > > future >> > > > > > > state >> > > > > > > > > for free)? >> > > > > > > > >> > > > > > > > One idea would be to put ParseDrmDisplay into DrmDisplay as >> > > > > > UpdateDrmDisplay. >> > > > > > > >> > > > > > > Something like patch set 2? >> > > > > > >> > > > > > I was hoping for a more substantial separation. >> > > > > > >> > > > > > std::vector<DisplaySnapshot_Params> GetDisplays() { >> > > > > > std::vector<DisplaySnapshot_Params> snapshots; >> > > > > > >> > > > > > UpdateDisplays(); >> > > > > > >> > > > > > for (const auto& display : displays_) >> > > > > > snapshots.push_back(display.Snapshot()); >> > > > > > >> > > > > > return snapshots; >> > > > > > } >> > > > > >> > > > > I think I see why you want to CreateDisplaySnapshotParams in its > > current >> >> > > form, >> > > > > though. You want share with the new (not yet added) browser code. >> > > > > And >> you >> > > > can't >> > > > > put DrmDisplay in common/ because it uses ScreenManager. >> > > > > >> > > > > Maybe we should make DrmDisplay not need to call ScreenManager so >> > > > > that >> > it's >> > > > > usable as a general utility. Anyway, this is enough change for now >> > > > > - >> lgtm. >> > > > >> > > > Yep, I want the CreateDisplay*Params to be shared between the 2. At >> > > > the >> same >> > > > time I want to restrict the APIs used to configure the displays to >> > > > the > > GPU >> >> > > code >> > > > since we couldn't be doing any such operation on the browser side. >> > > > >> > > > The other reason I don't want to push much of this logic into >> > > > DrmDisplay >> is >> > > > because DisplaySnapshot_Params is meant for IPC-ing only, so I want >> > > > to >> keep >> > > the >> > > > snapshot cached in DrmDisplay. >> > > >> > > I don't follow, you do want to keep it cached or don't? >> > >> > Sorry, that was supposed to be a "don't", I don't want it cached. > > >> You can build one from a DrmDisplay, though, right? I didn't mean to cache >> a >> copy, just essentially implementing > > >> DisplaySnapshot_Params SnapshotDrmDisplay(const DrmDisplay* display); > > > > Then the HardwareDisplayControllerInfo would need to be cached. I was hoping > I > wouldn't need to cache that. The reason I don't want to cache the HDCI is > because it keeps a snapshot of the hardware configuration. That changes once > a > DrmDisplay::Configure() call is done (since the mode changes). I disagree with this premise. There's nothing to be gained by not storing the last known state. The state is old the moment you get it - throwing it away moments later doesn't solve any races. But it doesn't matter, all we need is to make sure we're eventually consistent by processing the change events. > > >> > >> > > >> > > I agree with not using DisplaySnapshot_Params outside of IPC. In fact >> > > I >> don't >> > > like that we're reusing it other places for setup (in say >> > > CreateDisplaySnapshotParams). >> > > >> > > > Similarly I don't want the >> > > > HardwareDisplayControllerInfo cached since it also represents a >> > > > snapshot >> of >> > > the >> > > > CRTC and Connector state. >> > > > >> > > > I think this is going to become simpler after a few more changes. >> > > Specifically, >> > > > I'd like to redo GetAvailableDisplayControllerInfos() such that it >> > > > can > > be >> >> > > > simpler to use with DrmDisplay and with the browser side code. >> > > > Though >> > perhaps >> > > > you are right and there needs to be a shared DrmDisplayFoo base >> > > > class > > that >> >> > > deals >> > > > with my concerns. >> > > >> > > IMO there is no reason to withhold Configure() from the browser side > > object. >> >> > The >> > > syscall is there, the fd is available, and we have info about the > > displays. >> >> > The >> > > fact that we have to be careful to to cause races by actually using it > > from >> >> > two >> > > processes at the same time is nothing new. > > > > > https://codereview.chromium.org/1140773003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9d137bfcbb5420949b90dedd42dafb763c827e4a Cr-Commit-Position: refs/heads/master@{#329717}
Message was sent while issue was closed.
On Wed, May 13, 2015 at 5:08 PM, Michael Spang <spang@chromium.org> wrote: > On Wed, May 13, 2015 at 5:03 PM, <dnicoara@chromium.org> wrote: > > On 2015/05/13 20:59:49, spang wrote: > >> > >> On 2015/05/13 20:50:16, dnicoara wrote: > >> > On 2015/05/13 20:43:48, spang wrote: > >> > > On 2015/05/13 20:31:52, dnicoara wrote: > >> > > > On 2015/05/13 20:21:40, spang wrote: > >> > > > > On 2015/05/13 20:16:46, spang wrote: > >> > > > > > On 2015/05/13 20:03:01, dnicoara wrote: > >> > > > > > > On 2015/05/13 19:35:15, spang wrote: > >> > > > > > > > On 2015/05/13 19:28:23, spang wrote: > >> > > > > > > > > On 2015/05/13 19:10:36, dnicoara wrote: > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > >> > >> > > > > > > > > > File > >> > > > > > > > > > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc > >> > (right): > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > >> > >> > > > > > > > > > > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:88: > >> > > > > > > > old_displays.begin(), > >> > > > > > > > > > old_displays.end(), > >> > > > > > > > > > On 2015/05/13 19:01:20, spang wrote: > >> > > > > > > > > > > I'm not a fan of this. > >> > > > > > > > > > > > >> > > > > > > > > > > Can you update displays_ first, then convert > displays_ > >> > > > > > > > > > > to > >> > > > > > > > > > > std::vector<DisplaySnapshot_Params>()? > >> > > > > > > > > > > >> > > > > > > > > > Hmm, it is simpler to create the > >> > > > std::vector<DisplaySnapshot_Params>, > >> > > > > > then > >> > > > > > > > > > update displays_ since the display ID is needed from > >> > > > > > > DisplaySnapshot_Params. > >> > > > > > > > I > >> > > > > > > > > > don't think this has a huge mental overhead given that > >> > > > > > > > > > the > >> > > important > >> > > > > > > > > operations > >> > > > > > > > > > are just push_back(). Though I'm happy to change if > you > > > > think > >> > >> > > that's > >> > > > > > > cleaner > >> > > > > > > > > :) > >> > > > > > > > > > >> > > > > > > > > I think it is helpful to break things apart into higher > >> > > > > > > > > level > >> > > > > operations. > >> > > > > > > Even > >> > > > > > > > > if writing the composition of update & get in one go is > > > > smaller > >> > >> > that > >> > > > > > writing > >> > > > > > > > > them separately, the factored version is easier to > >> > > > > > > > > understand > >> > (which > >> > > > is > >> > > > > my > >> > > > > > > > > definition of "simpler"). It was pretty readable before > >> > > > > > > > > this > >> patch > >> > > but > >> > > > > now > >> > > > > > > you > >> > > > > > > > > need to integrate bits from the old state too. > >> > > > > > > > > > >> > > > > > > > > Do we actually need to need to build an new DrmDisplay > for > > > > each > >> > >> > > > > (crtc_id, > >> > > > > > > > > connector_id)? Why not update it (then you preserve > origin > >> > > > > > > > > & > > > > any > >> > >> > > > future > >> > > > > > > state > >> > > > > > > > > for free)? > >> > > > > > > > > >> > > > > > > > One idea would be to put ParseDrmDisplay into DrmDisplay > as > >> > > > > > UpdateDrmDisplay. > >> > > > > > > > >> > > > > > > Something like patch set 2? > >> > > > > > > >> > > > > > I was hoping for a more substantial separation. > >> > > > > > > >> > > > > > std::vector<DisplaySnapshot_Params> GetDisplays() { > >> > > > > > std::vector<DisplaySnapshot_Params> snapshots; > >> > > > > > > >> > > > > > UpdateDisplays(); > >> > > > > > > >> > > > > > for (const auto& display : displays_) > >> > > > > > snapshots.push_back(display.Snapshot()); > >> > > > > > > >> > > > > > return snapshots; > >> > > > > > } > >> > > > > > >> > > > > I think I see why you want to CreateDisplaySnapshotParams in its > > > > current > >> > >> > > form, > >> > > > > though. You want share with the new (not yet added) browser > code. > >> > > > > And > >> you > >> > > > can't > >> > > > > put DrmDisplay in common/ because it uses ScreenManager. > >> > > > > > >> > > > > Maybe we should make DrmDisplay not need to call ScreenManager > so > >> > > > > that > >> > it's > >> > > > > usable as a general utility. Anyway, this is enough change for > now > >> > > > > - > >> lgtm. > >> > > > > >> > > > Yep, I want the CreateDisplay*Params to be shared between the 2. > At > >> > > > the > >> same > >> > > > time I want to restrict the APIs used to configure the displays to > >> > > > the > > > > GPU > >> > >> > > code > >> > > > since we couldn't be doing any such operation on the browser side. > >> > > > > >> > > > The other reason I don't want to push much of this logic into > >> > > > DrmDisplay > >> is > >> > > > because DisplaySnapshot_Params is meant for IPC-ing only, so I > want > >> > > > to > >> keep > >> > > the > >> > > > snapshot cached in DrmDisplay. > >> > > > >> > > I don't follow, you do want to keep it cached or don't? > >> > > >> > Sorry, that was supposed to be a "don't", I don't want it cached. > > > > > >> You can build one from a DrmDisplay, though, right? I didn't mean to > cache > >> a > >> copy, just essentially implementing > > > > > >> DisplaySnapshot_Params SnapshotDrmDisplay(const DrmDisplay* display); > > > > > > > > Then the HardwareDisplayControllerInfo would need to be cached. I was > hoping > > I > > wouldn't need to cache that. The reason I don't want to cache the HDCI is > > because it keeps a snapshot of the hardware configuration. That changes > once > > a > > DrmDisplay::Configure() call is done (since the mode changes). > > I disagree with this premise. There's nothing to be gained by not > storing the last known state. > > The state is old the moment you get it - throwing it away moments > later doesn't solve any races. But it doesn't matter, all we need is > to make sure we're eventually consistent by processing the change > events. > > It isn't meant to solve races, it is meant to force a specific order in operations. DrmDisplay::Update() IPC(DrmDisplay::GetSnapshot()) DrmDisplay::Configure() IPC(DrmDisplay::GetSnapshot()) In the above example, either Configure() updates the snapshot or it shouldn't be possible to get the snapshot after. I'm leaning towards: snapshot = DrmDisplay::GetUpdatedSnapshot() IPC(snapshot) DrmDisplay::Configure() snapshot = DrmDisplay::GetUpdatedSnapshot() IPC(snapshot) Since this makes it explicit that if one wants the snapshot one needs to query for it rather than just reuse. > > > > > >> > > >> > > > >> > > I agree with not using DisplaySnapshot_Params outside of IPC. In > fact > >> > > I > >> don't > >> > > like that we're reusing it other places for setup (in say > >> > > CreateDisplaySnapshotParams). > >> > > > >> > > > Similarly I don't want the > >> > > > HardwareDisplayControllerInfo cached since it also represents a > >> > > > snapshot > >> of > >> > > the > >> > > > CRTC and Connector state. > >> > > > > >> > > > I think this is going to become simpler after a few more changes. > >> > > Specifically, > >> > > > I'd like to redo GetAvailableDisplayControllerInfos() such that it > >> > > > can > > > > be > >> > >> > > > simpler to use with DrmDisplay and with the browser side code. > >> > > > Though > >> > perhaps > >> > > > you are right and there needs to be a shared DrmDisplayFoo base > >> > > > class > > > > that > >> > >> > > deals > >> > > > with my concerns. > >> > > > >> > > IMO there is no reason to withhold Configure() from the browser side > > > > object. > >> > >> > The > >> > > syscall is there, the fd is available, and we have info about the > > > > displays. > >> > >> > The > >> > > fact that we have to be careful to to cause races by actually using > it > > > > from > >> > >> > two > >> > > processes at the same time is nothing new. > > > > > > > > > > https://codereview.chromium.org/1140773003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2015/05/13 21:41:10, dnicoara wrote: > On Wed, May 13, 2015 at 5:08 PM, Michael Spang <mailto:spang@chromium.org> wrote: > > > On Wed, May 13, 2015 at 5:03 PM, <mailto:dnicoara@chromium.org> wrote: > > > On 2015/05/13 20:59:49, spang wrote: > > >> > > >> On 2015/05/13 20:50:16, dnicoara wrote: > > >> > On 2015/05/13 20:43:48, spang wrote: > > >> > > On 2015/05/13 20:31:52, dnicoara wrote: > > >> > > > On 2015/05/13 20:21:40, spang wrote: > > >> > > > > On 2015/05/13 20:16:46, spang wrote: > > >> > > > > > On 2015/05/13 20:03:01, dnicoara wrote: > > >> > > > > > > On 2015/05/13 19:35:15, spang wrote: > > >> > > > > > > > On 2015/05/13 19:28:23, spang wrote: > > >> > > > > > > > > On 2015/05/13 19:10:36, dnicoara wrote: > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > >> > > >> > > > > > > > > > File > > >> > > > > > > > > > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc > > >> > (right): > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > > > > > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... > > >> > > >> > > > > > > > > > > > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:88: > > >> > > > > > > > old_displays.begin(), > > >> > > > > > > > > > old_displays.end(), > > >> > > > > > > > > > On 2015/05/13 19:01:20, spang wrote: > > >> > > > > > > > > > > I'm not a fan of this. > > >> > > > > > > > > > > > > >> > > > > > > > > > > Can you update displays_ first, then convert > > displays_ > > >> > > > > > > > > > > to > > >> > > > > > > > > > > std::vector<DisplaySnapshot_Params>()? > > >> > > > > > > > > > > > >> > > > > > > > > > Hmm, it is simpler to create the > > >> > > > std::vector<DisplaySnapshot_Params>, > > >> > > > > > then > > >> > > > > > > > > > update displays_ since the display ID is needed from > > >> > > > > > > DisplaySnapshot_Params. > > >> > > > > > > > I > > >> > > > > > > > > > don't think this has a huge mental overhead given that > > >> > > > > > > > > > the > > >> > > important > > >> > > > > > > > > operations > > >> > > > > > > > > > are just push_back(). Though I'm happy to change if > > you > > > > > > think > > >> > > >> > > that's > > >> > > > > > > cleaner > > >> > > > > > > > > :) > > >> > > > > > > > > > > >> > > > > > > > > I think it is helpful to break things apart into higher > > >> > > > > > > > > level > > >> > > > > operations. > > >> > > > > > > Even > > >> > > > > > > > > if writing the composition of update & get in one go is > > > > > > smaller > > >> > > >> > that > > >> > > > > > writing > > >> > > > > > > > > them separately, the factored version is easier to > > >> > > > > > > > > understand > > >> > (which > > >> > > > is > > >> > > > > my > > >> > > > > > > > > definition of "simpler"). It was pretty readable before > > >> > > > > > > > > this > > >> patch > > >> > > but > > >> > > > > now > > >> > > > > > > you > > >> > > > > > > > > need to integrate bits from the old state too. > > >> > > > > > > > > > > >> > > > > > > > > Do we actually need to need to build an new DrmDisplay > > for > > > > > > each > > >> > > >> > > > > (crtc_id, > > >> > > > > > > > > connector_id)? Why not update it (then you preserve > > origin > > >> > > > > > > > > & > > > > > > any > > >> > > >> > > > future > > >> > > > > > > state > > >> > > > > > > > > for free)? > > >> > > > > > > > > > >> > > > > > > > One idea would be to put ParseDrmDisplay into DrmDisplay > > as > > >> > > > > > UpdateDrmDisplay. > > >> > > > > > > > > >> > > > > > > Something like patch set 2? > > >> > > > > > > > >> > > > > > I was hoping for a more substantial separation. > > >> > > > > > > > >> > > > > > std::vector<DisplaySnapshot_Params> GetDisplays() { > > >> > > > > > std::vector<DisplaySnapshot_Params> snapshots; > > >> > > > > > > > >> > > > > > UpdateDisplays(); > > >> > > > > > > > >> > > > > > for (const auto& display : displays_) > > >> > > > > > snapshots.push_back(display.Snapshot()); > > >> > > > > > > > >> > > > > > return snapshots; > > >> > > > > > } > > >> > > > > > > >> > > > > I think I see why you want to CreateDisplaySnapshotParams in its > > > > > > current > > >> > > >> > > form, > > >> > > > > though. You want share with the new (not yet added) browser > > code. > > >> > > > > And > > >> you > > >> > > > can't > > >> > > > > put DrmDisplay in common/ because it uses ScreenManager. > > >> > > > > > > >> > > > > Maybe we should make DrmDisplay not need to call ScreenManager > > so > > >> > > > > that > > >> > it's > > >> > > > > usable as a general utility. Anyway, this is enough change for > > now > > >> > > > > - > > >> lgtm. > > >> > > > > > >> > > > Yep, I want the CreateDisplay*Params to be shared between the 2. > > At > > >> > > > the > > >> same > > >> > > > time I want to restrict the APIs used to configure the displays to > > >> > > > the > > > > > > GPU > > >> > > >> > > code > > >> > > > since we couldn't be doing any such operation on the browser side. > > >> > > > > > >> > > > The other reason I don't want to push much of this logic into > > >> > > > DrmDisplay > > >> is > > >> > > > because DisplaySnapshot_Params is meant for IPC-ing only, so I > > want > > >> > > > to > > >> keep > > >> > > the > > >> > > > snapshot cached in DrmDisplay. > > >> > > > > >> > > I don't follow, you do want to keep it cached or don't? > > >> > > > >> > Sorry, that was supposed to be a "don't", I don't want it cached. > > > > > > > > >> You can build one from a DrmDisplay, though, right? I didn't mean to > > cache > > >> a > > >> copy, just essentially implementing > > > > > > > > >> DisplaySnapshot_Params SnapshotDrmDisplay(const DrmDisplay* display); > > > > > > > > > > > > Then the HardwareDisplayControllerInfo would need to be cached. I was > > hoping > > > I > > > wouldn't need to cache that. The reason I don't want to cache the HDCI is > > > because it keeps a snapshot of the hardware configuration. That changes > > once > > > a > > > DrmDisplay::Configure() call is done (since the mode changes). > > > > I disagree with this premise. There's nothing to be gained by not > > storing the last known state. > > > > The state is old the moment you get it - throwing it away moments > > later doesn't solve any races. But it doesn't matter, all we need is > > to make sure we're eventually consistent by processing the change > > events. > > > > > It isn't meant to solve races, it is meant to force a specific order in > operations. > > DrmDisplay::Update() > IPC(DrmDisplay::GetSnapshot()) > DrmDisplay::Configure() > IPC(DrmDisplay::GetSnapshot()) > > In the above example, either Configure() updates the snapshot or it > shouldn't be possible to get the snapshot after. Right, you could either update in Configure or mark the state dirty and update it lazily in GetSnapshot(). > > I'm leaning towards: > snapshot = DrmDisplay::GetUpdatedSnapshot() > IPC(snapshot) > DrmDisplay::Configure() > snapshot = DrmDisplay::GetUpdatedSnapshot() > IPC(snapshot) I think this way is worse. A get doesn't dirty the state, so calling get twice shouldn't ioctl twice. This way makes the caller either worry about dirtiness or cause redundant work, but it's easy to avoid that with the above way. > Since this makes it explicit that if one wants the snapshot one needs to > query for it rather than just reuse. > > > > > > > > > > >> > > > >> > > > > >> > > I agree with not using DisplaySnapshot_Params outside of IPC. In > > fact > > >> > > I > > >> don't > > >> > > like that we're reusing it other places for setup (in say > > >> > > CreateDisplaySnapshotParams). > > >> > > > > >> > > > Similarly I don't want the > > >> > > > HardwareDisplayControllerInfo cached since it also represents a > > >> > > > snapshot > > >> of > > >> > > the > > >> > > > CRTC and Connector state. > > >> > > > > > >> > > > I think this is going to become simpler after a few more changes. > > >> > > Specifically, > > >> > > > I'd like to redo GetAvailableDisplayControllerInfos() such that it > > >> > > > can > > > > > > be > > >> > > >> > > > simpler to use with DrmDisplay and with the browser side code. > > >> > > > Though > > >> > perhaps > > >> > > > you are right and there needs to be a shared DrmDisplayFoo base > > >> > > > class > > > > > > that > > >> > > >> > > deals > > >> > > > with my concerns. > > >> > > > > >> > > IMO there is no reason to withhold Configure() from the browser side > > > > > > object. > > >> > > >> > The > > >> > > syscall is there, the fd is available, and we have info about the > > > > > > displays. > > >> > > >> > The > > >> > > fact that we have to be careful to to cause races by actually using > > it > > > > > > from > > >> > > >> > two > > >> > > processes at the same time is nothing new. > > > > > > > > > > > > > > > https://codereview.chromium.org/1140773003/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On Wed, May 13, 2015 at 6:13 PM, <spang@chromium.org> wrote: > On 2015/05/13 21:41:10, dnicoara wrote: >> >> On Wed, May 13, 2015 at 5:08 PM, Michael Spang <mailto:spang@chromium.org> > > wrote: > > >> > On Wed, May 13, 2015 at 5:03 PM, <mailto:dnicoara@chromium.org> wrote: >> > > On 2015/05/13 20:59:49, spang wrote: >> > >> >> > >> On 2015/05/13 20:50:16, dnicoara wrote: >> > >> > On 2015/05/13 20:43:48, spang wrote: >> > >> > > On 2015/05/13 20:31:52, dnicoara wrote: >> > >> > > > On 2015/05/13 20:21:40, spang wrote: >> > >> > > > > On 2015/05/13 20:16:46, spang wrote: >> > >> > > > > > On 2015/05/13 20:03:01, dnicoara wrote: >> > >> > > > > > > On 2015/05/13 19:35:15, spang wrote: >> > >> > > > > > > > On 2015/05/13 19:28:23, spang wrote: >> > >> > > > > > > > > On 2015/05/13 19:10:36, dnicoara wrote: >> > >> > > > > > > > > > >> > >> > > > > > > > > >> > >> > > > > > > > >> > >> > > > > > > >> > >> > > > > > >> > >> > > > > >> > >> > > > >> > >> > > >> > >> > >> > > >> > > >> > > >> > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... >> >> > >> >> > >> > > > > > > > > > File >> > >> > > > > > > > > > >> > >> > > > > > > > > > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc >> > >> > (right): >> > >> > > > > > > > > > >> > >> > > > > > > > > > >> > >> > > > > > > > > >> > >> > > > > > > > >> > >> > > > > > > >> > >> > > > > > >> > >> > > > > >> > >> > > > >> > >> > > >> > >> > >> > > >> > > >> > > >> > > > > https://codereview.chromium.org/1140773003/diff/1/ui/ozone/platform/drm/gpu/d... >> >> > >> >> > >> > > > > > > > > > >> > ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:88: >> > >> > > > > > > > old_displays.begin(), >> > >> > > > > > > > > > old_displays.end(), >> > >> > > > > > > > > > On 2015/05/13 19:01:20, spang wrote: >> > >> > > > > > > > > > > I'm not a fan of this. >> > >> > > > > > > > > > > >> > >> > > > > > > > > > > Can you update displays_ first, then convert >> > displays_ >> > >> > > > > > > > > > > to >> > >> > > > > > > > > > > std::vector<DisplaySnapshot_Params>()? >> > >> > > > > > > > > > >> > >> > > > > > > > > > Hmm, it is simpler to create the >> > >> > > > std::vector<DisplaySnapshot_Params>, >> > >> > > > > > then >> > >> > > > > > > > > > update displays_ since the display ID is needed >> > >> > > > > > > > > > from >> > >> > > > > > > DisplaySnapshot_Params. >> > >> > > > > > > > I >> > >> > > > > > > > > > don't think this has a huge mental overhead given >> > >> > > > > > > > > > that >> > >> > > > > > > > > > the >> > >> > > important >> > >> > > > > > > > > operations >> > >> > > > > > > > > > are just push_back(). Though I'm happy to change if >> > you >> > > >> > > think >> > >> >> > >> > > that's >> > >> > > > > > > cleaner >> > >> > > > > > > > > :) >> > >> > > > > > > > > >> > >> > > > > > > > > I think it is helpful to break things apart into >> > >> > > > > > > > > higher >> > >> > > > > > > > > level >> > >> > > > > operations. >> > >> > > > > > > Even >> > >> > > > > > > > > if writing the composition of update & get in one go >> > >> > > > > > > > > is >> > > >> > > smaller >> > >> >> > >> > that >> > >> > > > > > writing >> > >> > > > > > > > > them separately, the factored version is easier to >> > >> > > > > > > > > understand >> > >> > (which >> > >> > > > is >> > >> > > > > my >> > >> > > > > > > > > definition of "simpler"). It was pretty readable >> > >> > > > > > > > > before >> > >> > > > > > > > > this >> > >> patch >> > >> > > but >> > >> > > > > now >> > >> > > > > > > you >> > >> > > > > > > > > need to integrate bits from the old state too. >> > >> > > > > > > > > >> > >> > > > > > > > > Do we actually need to need to build an new >> > >> > > > > > > > > DrmDisplay >> > for >> > > >> > > each >> > >> >> > >> > > > > (crtc_id, >> > >> > > > > > > > > connector_id)? Why not update it (then you preserve >> > origin >> > >> > > > > > > > > & >> > > >> > > any >> > >> >> > >> > > > future >> > >> > > > > > > state >> > >> > > > > > > > > for free)? >> > >> > > > > > > > >> > >> > > > > > > > One idea would be to put ParseDrmDisplay into >> > >> > > > > > > > DrmDisplay >> > as >> > >> > > > > > UpdateDrmDisplay. >> > >> > > > > > > >> > >> > > > > > > Something like patch set 2? >> > >> > > > > > >> > >> > > > > > I was hoping for a more substantial separation. >> > >> > > > > > >> > >> > > > > > std::vector<DisplaySnapshot_Params> GetDisplays() { >> > >> > > > > > std::vector<DisplaySnapshot_Params> snapshots; >> > >> > > > > > >> > >> > > > > > UpdateDisplays(); >> > >> > > > > > >> > >> > > > > > for (const auto& display : displays_) >> > >> > > > > > snapshots.push_back(display.Snapshot()); >> > >> > > > > > >> > >> > > > > > return snapshots; >> > >> > > > > > } >> > >> > > > > >> > >> > > > > I think I see why you want to CreateDisplaySnapshotParams in >> > >> > > > > its >> > > >> > > current >> > >> >> > >> > > form, >> > >> > > > > though. You want share with the new (not yet added) browser >> > code. >> > >> > > > > And >> > >> you >> > >> > > > can't >> > >> > > > > put DrmDisplay in common/ because it uses ScreenManager. >> > >> > > > > >> > >> > > > > Maybe we should make DrmDisplay not need to call >> > >> > > > > ScreenManager >> > so >> > >> > > > > that >> > >> > it's >> > >> > > > > usable as a general utility. Anyway, this is enough change >> > >> > > > > for >> > now >> > >> > > > > - >> > >> lgtm. >> > >> > > > >> > >> > > > Yep, I want the CreateDisplay*Params to be shared between the >> > >> > > > 2. >> > At >> > >> > > > the >> > >> same >> > >> > > > time I want to restrict the APIs used to configure the displays >> > >> > > > to >> > >> > > > the >> > > >> > > GPU >> > >> >> > >> > > code >> > >> > > > since we couldn't be doing any such operation on the browser >> > >> > > > side. >> > >> > > > >> > >> > > > The other reason I don't want to push much of this logic into >> > >> > > > DrmDisplay >> > >> is >> > >> > > > because DisplaySnapshot_Params is meant for IPC-ing only, so I >> > want >> > >> > > > to >> > >> keep >> > >> > > the >> > >> > > > snapshot cached in DrmDisplay. >> > >> > > >> > >> > > I don't follow, you do want to keep it cached or don't? >> > >> > >> > >> > Sorry, that was supposed to be a "don't", I don't want it cached. >> > > >> > > >> > >> You can build one from a DrmDisplay, though, right? I didn't mean to >> > cache >> > >> a >> > >> copy, just essentially implementing >> > > >> > > >> > >> DisplaySnapshot_Params SnapshotDrmDisplay(const DrmDisplay* display); >> > > >> > > >> > > >> > > Then the HardwareDisplayControllerInfo would need to be cached. I was >> > hoping >> > > I >> > > wouldn't need to cache that. The reason I don't want to cache the HDCI >> > > is >> > > because it keeps a snapshot of the hardware configuration. That >> > > changes >> > once >> > > a >> > > DrmDisplay::Configure() call is done (since the mode changes). >> > >> > I disagree with this premise. There's nothing to be gained by not >> > storing the last known state. >> > >> > The state is old the moment you get it - throwing it away moments >> > later doesn't solve any races. But it doesn't matter, all we need is >> > to make sure we're eventually consistent by processing the change >> > events. >> > >> > >> It isn't meant to solve races, it is meant to force a specific order in >> operations. > > >> DrmDisplay::Update() >> IPC(DrmDisplay::GetSnapshot()) >> DrmDisplay::Configure() >> IPC(DrmDisplay::GetSnapshot()) > > >> In the above example, either Configure() updates the snapshot or it >> shouldn't be possible to get the snapshot after. > > > Right, you could either update in Configure or mark the state dirty and > update > it lazily in GetSnapshot(). > > >> I'm leaning towards: >> snapshot = DrmDisplay::GetUpdatedSnapshot() >> IPC(snapshot) >> DrmDisplay::Configure() >> snapshot = DrmDisplay::GetUpdatedSnapshot() >> IPC(snapshot) > > > I think this way is worse. A get doesn't dirty the state, so calling get > twice > shouldn't ioctl twice. This way makes the caller either worry about > dirtiness or > cause redundant work, but it's easy to avoid that with the above way. > > > >> Since this makes it explicit that if one wants the snapshot one needs to >> query for it rather than just reuse. Note that tracking dirtiness doesn't preclude the current strategy of assuming all display state is dirty in GetDisplays(). I don't remember why we do that but I remember it is intentional. We'd just have to make those dirties/updates explicit. > > > >> > > >> > > >> > >> > >> > >> > > >> > >> > > I agree with not using DisplaySnapshot_Params outside of IPC. In >> > fact >> > >> > > I >> > >> don't >> > >> > > like that we're reusing it other places for setup (in say >> > >> > > CreateDisplaySnapshotParams). >> > >> > > >> > >> > > > Similarly I don't want the >> > >> > > > HardwareDisplayControllerInfo cached since it also represents a >> > >> > > > snapshot >> > >> of >> > >> > > the >> > >> > > > CRTC and Connector state. >> > >> > > > >> > >> > > > I think this is going to become simpler after a few more >> > >> > > > changes. >> > >> > > Specifically, >> > >> > > > I'd like to redo GetAvailableDisplayControllerInfos() such that >> > >> > > > it >> > >> > > > can >> > > >> > > be >> > >> >> > >> > > > simpler to use with DrmDisplay and with the browser side code. >> > >> > > > Though >> > >> > perhaps >> > >> > > > you are right and there needs to be a shared DrmDisplayFoo base >> > >> > > > class >> > > >> > > that >> > >> >> > >> > > deals >> > >> > > > with my concerns. >> > >> > > >> > >> > > IMO there is no reason to withhold Configure() from the browser >> > >> > > side >> > > >> > > object. >> > >> >> > >> > The >> > >> > > syscall is there, the fd is available, and we have info about the >> > > >> > > displays. >> > >> >> > >> > The >> > >> > > fact that we have to be careful to to cause races by actually >> > >> > > using >> > it >> > > >> > > from >> > >> >> > >> > two >> > >> > > processes at the same time is nothing new. >> > > >> > > >> > > >> > > >> > > https://codereview.chromium.org/1140773003/ >> > > > >> To unsubscribe from this group and stop receiving emails from it, send an > > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > https://codereview.chromium.org/1140773003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
