Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(746)

Issue 2523053004: Enhance chrome.app.window API for shelf integration with restore support (Closed)

Created:
4 years ago by Andra Paraschiv
Modified:
4 years ago
CC:
chromium-reviews, extensions-reviews_chromium.org, sadrul, Matt Giuca, tfarina, kalyank, chromium-apps-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Enhance chrome.app.window API for shelf integration with restore support Enhance the chrome.app.window API for shelf integration with the possibility of restoring showInShelf=true pinned windows. Based on https://codereview.chromium.org/2341643002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST = https://codereview.chromium.org/2297633002 Committed: https://crrev.com/4e4fb8bbebc3d8f3e38130021daec21b760c5840 Cr-Commit-Position: refs/heads/master@{#438803}

Patch Set 1 #

Total comments: 12

Patch Set 2 : Review v2 #

Total comments: 6

Patch Set 3 : Review v3 #

Patch Set 4 : Review v4 #

Total comments: 10

Patch Set 5 : Review v5 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+67 lines, -4 lines) Patch
M apps/launcher.h View 1 2 1 chunk +13 lines, -0 lines 0 comments Download
M apps/launcher.cc View 1 2 3 4 3 chunks +16 lines, -1 line 0 comments Download
M chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/ash/launcher/chrome_launcher_controller.h View 1 2 3 4 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc View 1 2 3 4 1 chunk +9 lines, -0 lines 0 comments Download
M chrome/browser/ui/ash/launcher/launcher_controller_helper.h View 1 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/ui/ash/launcher/launcher_controller_helper.cc View 1 2 3 2 chunks +9 lines, -0 lines 0 comments Download
M chrome/browser/ui/extensions/app_launch_params.h View 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/ui/extensions/application_launch.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 30 (8 generated)
Andra Paraschiv
Could you please take a look? Thank you.
4 years ago (2016-11-28 07:07:10 UTC) #3
Mr4D (OOO till 08-26)
Sorry for the delay. Please see comments! https://codereview.chromium.org/2523053004/diff/1/apps/launcher.h File apps/launcher.h (right): https://codereview.chromium.org/2523053004/diff/1/apps/launcher.h#newcode39 apps/launcher.h:39: // |launch_id| ...
4 years ago (2016-12-03 01:55:27 UTC) #4
stevenjb
https://codereview.chromium.org/2523053004/diff/1/apps/launcher.h File apps/launcher.h (right): https://codereview.chromium.org/2523053004/diff/1/apps/launcher.h#newcode50 apps/launcher.h:50: PLAY_STORE_STATUS_UNKNOWN); Given that we already have a bunch of ...
4 years ago (2016-12-05 16:47:31 UTC) #5
Andra Paraschiv
Thank you all for feedback, I uploaded a new patch set. https://codereview.chromium.org/2523053004/diff/1/apps/launcher.h File apps/launcher.h (right): ...
4 years ago (2016-12-06 13:19:16 UTC) #7
Andra Paraschiv
Thank you all for feedback, I uploaded a new patch set.
4 years ago (2016-12-06 13:19:18 UTC) #8
stevenjb
https://codereview.chromium.org/2523053004/diff/20001/apps/app_load_service.cc File apps/app_load_service.cc (right): https://codereview.chromium.org/2523053004/diff/20001/apps/app_load_service.cc#newcode120 apps/app_load_service.cc:120: extensions::SOURCE_LOAD_AND_LAUNCH); Please remove any formatting-only changes like this. https://codereview.chromium.org/2523053004/diff/20001/apps/launcher.cc ...
4 years ago (2016-12-06 16:42:07 UTC) #9
benwells
Is there something in particular you want me to look at?
4 years ago (2016-12-06 23:09:08 UTC) #10
Andra Paraschiv
On 2016/12/06 23:09:08, benwells wrote: > Is there something in particular you want me to ...
4 years ago (2016-12-07 08:13:55 UTC) #11
Andra Paraschiv
Thank you Steven for review, I updated the CL. https://codereview.chromium.org/2523053004/diff/20001/apps/app_load_service.cc File apps/app_load_service.cc (right): https://codereview.chromium.org/2523053004/diff/20001/apps/app_load_service.cc#newcode120 apps/app_load_service.cc:120: ...
4 years ago (2016-12-07 08:33:21 UTC) #12
benwells
On 2016/12/07 08:13:55, Andra Paraschiv wrote: > On 2016/12/06 23:09:08, benwells wrote: > > Is ...
4 years ago (2016-12-09 06:12:38 UTC) #13
benwells
the files you asked me to look at lgtm
4 years ago (2016-12-09 06:50:33 UTC) #14
Andra Paraschiv
On 2016/12/09 06:12:38, benwells wrote: > On 2016/12/07 08:13:55, Andra Paraschiv wrote: > > On ...
4 years ago (2016-12-09 09:25:33 UTC) #15
Mr4D (OOO till 08-26)
lgtm
4 years ago (2016-12-09 14:56:20 UTC) #16
stevenjb
https://codereview.chromium.org/2523053004/diff/60001/apps/launcher.cc File apps/launcher.cc (right): https://codereview.chromium.org/2523053004/diff/60001/apps/launcher.cc#newcode413 apps/launcher.cc:413: launch_data->id.reset(new std::string(launch_id)); Rather than potentially allocating an empty string ...
4 years ago (2016-12-09 17:00:57 UTC) #17
Andra Paraschiv
Thank you for feedback, Steven. I added the fixes. https://codereview.chromium.org/2523053004/diff/60001/apps/launcher.cc File apps/launcher.cc (right): https://codereview.chromium.org/2523053004/diff/60001/apps/launcher.cc#newcode413 apps/launcher.cc:413: ...
4 years ago (2016-12-12 14:28:23 UTC) #18
stevenjb
https://codereview.chromium.org/2523053004/diff/60001/chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc (right): https://codereview.chromium.org/2523053004/diff/60001/chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc#newcode113 chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:113: event_flags); On 2016/12/12 14:28:22, Andra Paraschiv wrote: > On ...
4 years ago (2016-12-13 19:31:14 UTC) #19
Andra Paraschiv
https://codereview.chromium.org/2523053004/diff/60001/chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc (right): https://codereview.chromium.org/2523053004/diff/60001/chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc#newcode113 chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:113: event_flags); On 2016/12/13 19:31:14, stevenjb wrote: > On 2016/12/12 ...
4 years ago (2016-12-14 14:59:38 UTC) #20
stevenjb
lgtm https://codereview.chromium.org/2523053004/diff/60001/chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc (right): https://codereview.chromium.org/2523053004/diff/60001/chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc#newcode113 chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:113: event_flags); On 2016/12/14 14:59:38, Andra Paraschiv wrote: > ...
4 years ago (2016-12-14 18:40:21 UTC) #21
Andra Paraschiv
https://codereview.chromium.org/2523053004/diff/60001/chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc (right): https://codereview.chromium.org/2523053004/diff/60001/chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc#newcode113 chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:113: event_flags); On 2016/12/14 18:40:21, stevenjb wrote: > On 2016/12/14 ...
4 years ago (2016-12-15 10:20:15 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2523053004/80001
4 years ago (2016-12-15 10:27:37 UTC) #25
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years ago (2016-12-15 11:14:27 UTC) #28
commit-bot: I haz the power
4 years ago (2016-12-15 11:16:34 UTC) #30
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/4e4fb8bbebc3d8f3e38130021daec21b760c5840
Cr-Commit-Position: refs/heads/master@{#438803}

Powered by Google App Engine
This is Rietveld 408576698