|
|
Created:
3 years, 7 months ago by James Cook Modified:
3 years, 7 months ago CC:
chromium-reviews, kalyank, rsesek+watch_chromium.org, sadrul Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Don't send duplicate UserSession mojo messages to ash
After the profile is loaded the SessionControllerClient in the browser
sends an updated ash::mojom::UserSession to ash. Most of the time the
message is the same as the one just sent when the session was started.
BUG=714689, 712799
TEST=added to SessionControllerClientTest in unit_tests
Review-Url: https://codereview.chromium.org/2842693002
Cr-Commit-Position: refs/heads/master@{#467172}
Committed: https://chromium.googlesource.com/chromium/src/+/714f3da6005d557c7dff987fb3ee097201cb31fd
Patch Set 1 #
Total comments: 6
Patch Set 2 : rebase #Patch Set 3 : use EqualsTraits #
Total comments: 1
Patch Set 4 : review comment #Patch Set 5 : rebase #
Messages
Total messages: 22 (11 generated)
The CQ bit was checked by jamescook@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...
jamescook@chromium.org changed reviewers: + xiyuan@chromium.org
xiyuan, please take a look. https://codereview.chromium.org/2842693002/diff/1/ui/gfx/image/image_skia.h File ui/gfx/image/image_skia.h (right): https://codereview.chromium.org/2842693002/diff/1/ui/gfx/image/image_skia.h#n... ui/gfx/image/image_skia.h:100: bool Equals(const gfx::ImageSkia& other) const; I think this is the right way to handle comparisons for mojo. Mojo has EqualsTraits<>, but they are not like StructTraits<>. I don't think you're supposed to define your own EqualsTraits. I could define my own mojo-only comparison like this: namespace mojo { namespace internals { // uh oh, using internals template <> bool Equals(const ImageSkia& a, const ImageSkia& b) { ... } } } However, then it's weird that ui::mojom::ImageSkia has an Equals() but the underlying gfx::ImageSkia doesn't. That could also lead to a situation where someone else adds gfx::ImageSkia::operator==() that doesn't match ui::mojom::ImageSkia's concept of equals.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm You still need a gfx owner to stamp on the CL though. https://codereview.chromium.org/2842693002/diff/1/chrome/browser/ui/ash/sessi... File chrome/browser/ui/ash/session_controller_client_unittest.cc (right): https://codereview.chromium.org/2842693002/diff/1/chrome/browser/ui/ash/sessi... chrome/browser/ui/ash/session_controller_client_unittest.cc:99: update_user_session_count_++; nit: pre-increment, i.e. ++update_user_session_count_ https://codereview.chromium.org/2842693002/diff/1/ui/gfx/image/image_skia.h File ui/gfx/image/image_skia.h (right): https://codereview.chromium.org/2842693002/diff/1/ui/gfx/image/image_skia.h#n... ui/gfx/image/image_skia.h:100: bool Equals(const gfx::ImageSkia& other) const; On 2017/04/25 00:58:28, James Cook wrote: > I think this is the right way to handle comparisons for mojo. > > Mojo has EqualsTraits<>, but they are not like StructTraits<>. I don't think > you're supposed to define your own EqualsTraits. > > I could define my own mojo-only comparison like this: > > namespace mojo { > namespace internals { // uh oh, using internals > > template <> > bool Equals(const ImageSkia& a, const ImageSkia& b) { ... } > > } > } > > However, then it's weird that ui::mojom::ImageSkia has an Equals() but the > underlying gfx::ImageSkia doesn't. That could also lead to a situation where > someone else adds gfx::ImageSkia::operator==() that doesn't match > ui::mojom::ImageSkia's concept of equals. If gfx::ImageSkia needs an operation== in the future, it should probably be BackedBySameObjectAs() as well. So I think this is fine.
jamescook@chromium.org changed reviewers: + oshima@chromium.org
oshima, can I get OWNERS for ui/gfx/image/image_skia.cc?
https://codereview.chromium.org/2842693002/diff/1/ui/gfx/image/image_skia.h File ui/gfx/image/image_skia.h (right): https://codereview.chromium.org/2842693002/diff/1/ui/gfx/image/image_skia.h#n... ui/gfx/image/image_skia.h:100: bool Equals(const gfx::ImageSkia& other) const; On 2017/04/25 16:04:53, xiyuan wrote: > On 2017/04/25 00:58:28, James Cook wrote: > > I think this is the right way to handle comparisons for mojo. > > > > Mojo has EqualsTraits<>, but they are not like StructTraits<>. I don't think > > you're supposed to define your own EqualsTraits. > > > > I could define my own mojo-only comparison like this: > > > > namespace mojo { > > namespace internals { // uh oh, using internals > > > > template <> > > bool Equals(const ImageSkia& a, const ImageSkia& b) { ... } > > > > } > > } > > > > However, then it's weird that ui::mojom::ImageSkia has an Equals() but the > > underlying gfx::ImageSkia doesn't. That could also lead to a situation where > > someone else adds gfx::ImageSkia::operator==() that doesn't match > > ui::mojom::ImageSkia's concept of equals. > > If gfx::ImageSkia needs an operation== in the future, it should probably be > BackedBySameObjectAs() as well. So I think this is fine. Does this have to be named "Equals"? "equals" and "same" has subtle difference in meaning so I want to use Same if possible.
https://codereview.chromium.org/2842693002/diff/1/ui/gfx/image/image_skia.h File ui/gfx/image/image_skia.h (right): https://codereview.chromium.org/2842693002/diff/1/ui/gfx/image/image_skia.h#n... ui/gfx/image/image_skia.h:100: bool Equals(const gfx::ImageSkia& other) const; On 2017/04/25 17:28:04, oshima wrote: > On 2017/04/25 16:04:53, xiyuan wrote: > > On 2017/04/25 00:58:28, James Cook wrote: > > > I think this is the right way to handle comparisons for mojo. > > > > > > Mojo has EqualsTraits<>, but they are not like StructTraits<>. I don't think > > > you're supposed to define your own EqualsTraits. > > > > > > I could define my own mojo-only comparison like this: > > > > > > namespace mojo { > > > namespace internals { // uh oh, using internals > > > > > > template <> > > > bool Equals(const ImageSkia& a, const ImageSkia& b) { ... } > > > > > > } > > > } > > > > > > However, then it's weird that ui::mojom::ImageSkia has an Equals() but the > > > underlying gfx::ImageSkia doesn't. That could also lead to a situation where > > > someone else adds gfx::ImageSkia::operator==() that doesn't match > > > ui::mojom::ImageSkia's concept of equals. > > > > If gfx::ImageSkia needs an operation== in the future, it should probably be > > BackedBySameObjectAs() as well. So I think this is fine. > > Does this have to be named "Equals"? "equals" and "same" has subtle difference > in meaning so I want to use Same if possible. It must either be called Equals() or be operator==(). See https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/lib/equals_trai... I need this in order to compare two mojo structs, UserSessionPtrs. Other options: * Write my own comparison for the two UserSession objects, which means I have to manually keep track of all the other fields to compare so I can do a special comparison for the image field * Do the hacky thing with mojo::internals mentioned above.
On 2017/04/25 17:32:47, James Cook wrote: > https://codereview.chromium.org/2842693002/diff/1/ui/gfx/image/image_skia.h > File ui/gfx/image/image_skia.h (right): > > https://codereview.chromium.org/2842693002/diff/1/ui/gfx/image/image_skia.h#n... > ui/gfx/image/image_skia.h:100: bool Equals(const gfx::ImageSkia& other) const; > On 2017/04/25 17:28:04, oshima wrote: > > On 2017/04/25 16:04:53, xiyuan wrote: > > > On 2017/04/25 00:58:28, James Cook wrote: > > > > I think this is the right way to handle comparisons for mojo. > > > > > > > > Mojo has EqualsTraits<>, but they are not like StructTraits<>. I don't > think > > > > you're supposed to define your own EqualsTraits. > > > > > > > > I could define my own mojo-only comparison like this: > > > > > > > > namespace mojo { > > > > namespace internals { // uh oh, using internals > > > > > > > > template <> > > > > bool Equals(const ImageSkia& a, const ImageSkia& b) { ... } > > > > > > > > } > > > > } > > > > > > > > However, then it's weird that ui::mojom::ImageSkia has an Equals() but the > > > > underlying gfx::ImageSkia doesn't. That could also lead to a situation > where > > > > someone else adds gfx::ImageSkia::operator==() that doesn't match > > > > ui::mojom::ImageSkia's concept of equals. > > > > > > If gfx::ImageSkia needs an operation== in the future, it should probably be > > > BackedBySameObjectAs() as well. So I think this is fine. > > > > Does this have to be named "Equals"? "equals" and "same" has subtle difference > > in meaning so I want to use Same if possible. > > It must either be called Equals() or be operator==(). See > https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/lib/equals_trai... > > I need this in order to compare two mojo structs, UserSessionPtrs. > > Other options: > > * Write my own comparison for the two UserSession objects, which means I have to > manually keep track of all the other fields to compare so I can do a special > comparison for the image field > * Do the hacky thing with mojo::internals mentioned above. I see. I prefer Equals then. lgtm
https://codereview.chromium.org/2842693002/diff/1/ui/gfx/image/image_skia.h File ui/gfx/image/image_skia.h (right): https://codereview.chromium.org/2842693002/diff/1/ui/gfx/image/image_skia.h#n... ui/gfx/image/image_skia.h:100: bool Equals(const gfx::ImageSkia& other) const; On 2017/04/25 17:32:47, James Cook wrote: > On 2017/04/25 17:28:04, oshima wrote: > > On 2017/04/25 16:04:53, xiyuan wrote: > > > On 2017/04/25 00:58:28, James Cook wrote: > > > > I think this is the right way to handle comparisons for mojo. > > > > > > > > Mojo has EqualsTraits<>, but they are not like StructTraits<>. I don't > think > > > > you're supposed to define your own EqualsTraits. > > > > > > > > I could define my own mojo-only comparison like this: > > > > > > > > namespace mojo { > > > > namespace internals { // uh oh, using internals > > > > > > > > template <> > > > > bool Equals(const ImageSkia& a, const ImageSkia& b) { ... } > > > > > > > > } > > > > } > > > > > > > > However, then it's weird that ui::mojom::ImageSkia has an Equals() but the > > > > underlying gfx::ImageSkia doesn't. That could also lead to a situation > where > > > > someone else adds gfx::ImageSkia::operator==() that doesn't match > > > > ui::mojom::ImageSkia's concept of equals. > > > > > > If gfx::ImageSkia needs an operation== in the future, it should probably be > > > BackedBySameObjectAs() as well. So I think this is fine. > > > > Does this have to be named "Equals"? "equals" and "same" has subtle difference > > in meaning so I want to use Same if possible. > > It must either be called Equals() or be operator==(). See > https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/lib/equals_trai... > > I need this in order to compare two mojo structs, UserSessionPtrs. > > Other options: > > * Write my own comparison for the two UserSession objects, which means I have to > manually keep track of all the other fields to compare so I can do a special > comparison for the image field > * Do the hacky thing with mojo::internals mentioned above. Oshima, please wait on looking at this. Yuzhu had a different suggestion on how to handles Equals(). I need to move some code in mojo.
jamescook@chromium.org changed reviewers: + yzshen@chromium.org
yzshen, please take a look at session_controller_client.cc -- is this the right place to put the EqualsTraits<>?
LGTM for EqualsTraits usage, with one nit. https://codereview.chromium.org/2842693002/diff/40001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/session_controller_client.cc (right): https://codereview.chromium.org/2842693002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/session_controller_client.cc:112: struct EqualsTraits<gfx::ImageSkia, false> { Do you need to explicitly put "false" here? I thought it has got a default value which depends on whether gfx::ImageSkia has Equals or not.
The CQ bit was checked by jamescook@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, oshima@chromium.org, yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/2842693002/#ps80001 (title: "rebase")
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": 80001, "attempt_start_ts": 1493163194448070, "parent_rev": "bf25b17bd99b5dbeff92e62f8dddd289c335dc0a", "commit_rev": "714f3da6005d557c7dff987fb3ee097201cb31fd"}
Message was sent while issue was closed.
Description was changed from ========== cros: Don't send duplicate UserSession mojo messages to ash After the profile is loaded the SessionControllerClient in the browser sends an updated ash::mojom::UserSession to ash. Most of the time the message is the same as the one just sent when the session was started. BUG=714689,712799 TEST=added to SessionControllerClientTest in unit_tests ========== to ========== cros: Don't send duplicate UserSession mojo messages to ash After the profile is loaded the SessionControllerClient in the browser sends an updated ash::mojom::UserSession to ash. Most of the time the message is the same as the one just sent when the session was started. BUG=714689,712799 TEST=added to SessionControllerClientTest in unit_tests Review-Url: https://codereview.chromium.org/2842693002 Cr-Commit-Position: refs/heads/master@{#467172} Committed: https://chromium.googlesource.com/chromium/src/+/714f3da6005d557c7dff987fb3ee... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/714f3da6005d557c7dff987fb3ee... |