Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2889523003:
Add kApplicationIdKey for ShellSurface::GetApplicationId and SetApplicationId (Closed)
DescriptionAdd kApplicationIdKey for ShellSurface::GetApplicationId and SetApplicationId
BUG=722765
TEST=none
Review-Url: https://codereview.chromium.org/2889523003
Cr-Commit-Position: refs/heads/master@{#475273}
Committed: https://chromium.googlesource.com/chromium/src/+/a57b15774cb1599333fa6e3c9cb57095d847b02a
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add kExoAppIdKey. #Patch Set 3 : Use DEFINE_OWNED_UI_CLASS_PROPERTY_KEY. #
Total comments: 6
Patch Set 4 : Address comments. #
Total comments: 4
Patch Set 5 : Fix nits. #
Total comments: 2
Patch Set 6 : Rebase. #Patch Set 7 : Fix shell_surface_unittest. #
Messages
Total messages: 48 (23 generated)
yawano@chromium.org changed reviewers: + reveman@chromium.org, sky@chromium.org
https://codereview.chromium.org/2860503002 has changed ShellSurface::Get/SetApplicationId to use app_id of ash::ShelfID. But I believe app_id of ash::ShelfID is a different ID from an application id in ShellSurface. An application id in ShellSurface is expected to be like "org.chromium.arc.%d" for Android window. I'm going to simply revert the part to use kAppIdKey as before. Let me know if we cannot simply revert it. PTAL. reveman@: files under components/exo sky@: files under ui/aura Thank you!
The CQ bit was checked by yawano@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...
Why is ash::ShelfID not working as expected? Afaict we're setting and getting the app_id just like before. Are we setting the launch_id instead? Not clear as the ctor as 2 arguments with default values..
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
msw@chromium.org changed reviewers: + msw@chromium.org
On 2017/05/16 10:34:27, reveman wrote: > Why is ash::ShelfID not working as expected? Afaict we're setting and getting > the app_id just like before. Are we setting the launch_id instead? Not clear as > the ctor as 2 arguments with default values.. Perhaps some other code is assigning a ShelfId using the 32-character crx id format to the same windows? Sorry for the inconvenience here... https://codereview.chromium.org/2889523003/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2889523003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:558: window->SetProperty(aura::client::kAppIdKey, new std::string(id)); As this is the only place using kAppIdKey, it should probably be defined locally, in this file, not in aura::client.
https://codereview.chromium.org/2889523003/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2889523003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:558: window->SetProperty(aura::client::kAppIdKey, new std::string(id)); On 2017/05/16 17:00:06, msw wrote: > As this is the only place using kAppIdKey, it should probably be defined > locally, in this file, not in aura::client. And perhaps renaming the property to disambiguate it from other app ids would be helpful. Maybe kShellSurfaceIdKey or kExoIdKey?
On 2017/05/16 17:03:27, msw wrote: > https://codereview.chromium.org/2889523003/diff/1/components/exo/shell_surfac... > File components/exo/shell_surface.cc (right): > > https://codereview.chromium.org/2889523003/diff/1/components/exo/shell_surfac... > components/exo/shell_surface.cc:558: > window->SetProperty(aura::client::kAppIdKey, new std::string(id)); > On 2017/05/16 17:00:06, msw wrote: > > As this is the only place using kAppIdKey, it should probably be defined > > locally, in this file, not in aura::client. > > And perhaps renaming the property to disambiguate it from other app ids would be > helpful. Maybe kShellSurfaceIdKey or kExoIdKey? App id in launcher, and ApplicationId here are different things, but they sounded the same due to naming. Yes, it can and should be defined locally. Not sure about naming as it's called "app_id" in xdg interface. Maybe kExoAppIdKey with documentation is good enough? https://cs.chromium.org/chromium/src/components/exo/wayland/server.cc?rcl=5c6...
PTAL again. Thank you! https://codereview.chromium.org/2889523003/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2889523003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:558: window->SetProperty(aura::client::kAppIdKey, new std::string(id)); On 2017/05/16 17:03:27, msw wrote: > On 2017/05/16 17:00:06, msw wrote: > > As this is the only place using kAppIdKey, it should probably be defined > > locally, in this file, not in aura::client. > > And perhaps renaming the property to disambiguate it from other app ids would be > helpful. Maybe kShellSurfaceIdKey or kExoIdKey? Done. Renamed to kExoAppIdKey as oshima has suggested, and move the property to this file.
Description was changed from ========== Use kAppIdKey for ShellSurface::GetApplicationId and SetApplicationId BUG=722765 TEST=none ========== to ========== Add kExoAppIdKey for ShellSurface::GetApplicationId and SetApplicationId BUG=722765 TEST=none ==========
Updated subject and description.
https://codereview.chromium.org/2889523003/diff/40001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2889523003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:316: namespace { nit: for consistency, please remove this namespace here or move to unnamed namespace at the top of the file along with the kMainSurfaceKey https://codereview.chromium.org/2889523003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:318: // An application Id which is set by a wayland server. This property is not This code should not be referring to wayland. How about "Application Id set by the client." ? https://codereview.chromium.org/2889523003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:570: const std::string ShellSurface::GetApplicationId(aura::Window* window) { can this return a pointer instead so we don't have to make a copy of the string?
Thanks! FWIW, lgtm % reveman's comments.
yawano@chromium.org changed reviewers: + cylee@chromium.org, dtseng@chromium.org, stevenjb@chromium.org - sky@chromium.org
PTAL. reveman@: Files under components/exo dtseng@: c/b/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc cylee@: c/b/memory/tab_manager_delegate_chromeos.cc stevenjb@: c/b/ui/ash/launcher/arc_app_window_launcher_controller.cc Moved sky@ from reviewer to cc as we no longer touch aura_constants.h/cc Thank you! https://codereview.chromium.org/2889523003/diff/40001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2889523003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:316: namespace { On 2017/05/17 15:31:06, reveman wrote: > nit: for consistency, please remove this namespace here or move to unnamed > namespace at the top of the file along with the kMainSurfaceKey Done. Moved it to the anonymous namespace at the top as I want to put DEFINE_OWNED_UI_CLASS_PROPERTY_KEY in anonymous namespace. Unlike DEFINE_LOCAL_UI_CLASS_PROPERTY_KEY, it doesn't put it in anonymous namespace. https://codereview.chromium.org/2889523003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:318: // An application Id which is set by a wayland server. This property is not On 2017/05/17 15:31:06, reveman wrote: > This code should not be referring to wayland. How about "Application Id set by > the client." ? Done. https://codereview.chromium.org/2889523003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:570: const std::string ShellSurface::GetApplicationId(aura::Window* window) { On 2017/05/17 15:31:06, reveman wrote: > can this return a pointer instead so we don't have to make a copy of the string? Done.
lgtm with nits https://codereview.chromium.org/2889523003/diff/60001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2889523003/diff/60001/components/exo/shell_su... components/exo/shell_surface.cc:57: DEFINE_OWNED_UI_CLASS_PROPERTY_KEY(std::string, kExoAppIdKey, nullptr); nit: s/kExoAppIdKey/kApplicationIdKey/ as exo prefix is not needed as this is private to this file and it's preferred to not use abbreviations https://codereview.chromium.org/2889523003/diff/60001/components/exo/shell_su... components/exo/shell_surface.cc:720: value->SetString("application_id", nit: empty application_id value is not worth much so please avoid setting this when it's null. this way you can also move the application_id variable into the "GetWidget() && GetWidget()->GetNativeWindow()" scope
Thank you! https://codereview.chromium.org/2889523003/diff/60001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2889523003/diff/60001/components/exo/shell_su... components/exo/shell_surface.cc:57: DEFINE_OWNED_UI_CLASS_PROPERTY_KEY(std::string, kExoAppIdKey, nullptr); On 2017/05/18 03:53:12, reveman wrote: > nit: s/kExoAppIdKey/kApplicationIdKey/ as exo prefix is not needed as this is > private to this file and it's preferred to not use abbreviations Done. https://codereview.chromium.org/2889523003/diff/60001/components/exo/shell_su... components/exo/shell_surface.cc:720: value->SetString("application_id", On 2017/05/18 03:53:12, reveman wrote: > nit: empty application_id value is not worth much so please avoid setting this > when it's null. this way you can also move the application_id variable into the > "GetWidget() && GetWidget()->GetNativeWindow()" scope Done.
yusukes@chromium.org changed reviewers: + yusukes@chromium.org
https://codereview.chromium.org/2889523003/diff/80001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (left): https://codereview.chromium.org/2889523003/diff/80001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:78: std::string application_id = exo::ShellSurface::GetApplicationId(window); FYI the code has been removed: https://codereview.chromium.org/2877883002/
yusukes@chromium.org changed reviewers: - yusukes@chromium.org
chrome/browser/ui/ash/launcher/ lgtm
yawano@chromium.org changed reviewers: - cylee@chromium.org
Moved cylee@ from reviewer to CC as we no longer touch tab_manager_delegate_chromeos.cc. dtseng@: ping. https://codereview.chromium.org/2889523003/diff/80001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (left): https://codereview.chromium.org/2889523003/diff/80001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:78: std::string application_id = exo::ShellSurface::GetApplicationId(window); On 2017/05/19 16:55:28, Yusuke Sato wrote: > FYI the code has been removed: > https://codereview.chromium.org/2877883002/ Thank you for letting me know about this! Rebased this CL.
Description was changed from ========== Add kExoAppIdKey for ShellSurface::GetApplicationId and SetApplicationId BUG=722765 TEST=none ========== to ========== Add kApplicationIdKey for ShellSurface::GetApplicationId and SetApplicationId BUG=722765 TEST=none ==========
Updated subject and description as we have changed key name.
lgtm arc accessibility
On 2017/05/25 00:09:07, David Tseng wrote: > lgtm arc accessibility Thank you!
The CQ bit was checked by yawano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, reveman@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2889523003/#ps100001 (title: "Rebase.")
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yawano@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.
reveman@: Fixed shell_surface_unittest. PTAL again. Thank you!
still lgtm On May 25, 2017 11:20 PM, <yawano@chromium.org> wrote: > reveman@: Fixed shell_surface_unittest. PTAL again. Thank you! > > https://codereview.chromium.org/2889523003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/05/26 13:48:29, reveman wrote: > still lgtm > > On May 25, 2017 11:20 PM, <mailto:yawano@chromium.org> wrote: > > > reveman@: Fixed shell_surface_unittest. PTAL again. Thank you! > > > > https://codereview.chromium.org/2889523003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Thank you!
The CQ bit was checked by yawano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, stevenjb@chromium.org, dtseng@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2889523003/#ps120001 (title: "Fix shell_surface_unittest.")
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": 120001, "attempt_start_ts": 1496020876810380,
"parent_rev": "c2cf640fde6976075f3b8876ed68eec9d0e97a61", "commit_rev":
"a57b15774cb1599333fa6e3c9cb57095d847b02a"}
Message was sent while issue was closed.
Description was changed from ========== Add kApplicationIdKey for ShellSurface::GetApplicationId and SetApplicationId BUG=722765 TEST=none ========== to ========== Add kApplicationIdKey for ShellSurface::GetApplicationId and SetApplicationId BUG=722765 TEST=none Review-Url: https://codereview.chromium.org/2889523003 Cr-Commit-Position: refs/heads/master@{#475273} Committed: https://chromium.googlesource.com/chromium/src/+/a57b15774cb1599333fa6e3c9cb5... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/a57b15774cb1599333fa6e3c9cb5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
