Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2243573002:
ARC: Show window title in launcher menu instead of app title (Closed)
DescriptionARC: Show window title in launcher menu instead of app title
BUG=627590
TEST=manual test
Committed: https://crrev.com/6ecae883342f04ab73135958cd9067fea0fa27d0
Cr-Commit-Position: refs/heads/master@{#411407}
Patch Set 1 #
Total comments: 6
Patch Set 2 : . #
Messages
Total messages: 24 (13 generated)
The CQ bit was checked by yoshiki@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...
Description was changed from ========== . BUG= ========== to ========== Show window title in launcher menu instead of app title BUG=627590 TEST=manual test ==========
yoshiki@chromium.org changed reviewers: + skuhne@google.com
Stefan, PTAL. Thanks.
Description was changed from ========== Show window title in launcher menu instead of app title BUG=627590 TEST=manual test ========== to ========== ARC: Show window title in launcher menu instead of app title BUG=627590 TEST=manual test ==========
skuhne@chromium.org changed reviewers: + skuhne@chromium.org
lgtm - but please have a look at the comment https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:56: (window ? window->title() : GetTitle()), &image, app_id(), Would it make sense to use GetTitle() if the window title is empty?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:56: (window ? window->title() : GetTitle()), &image, app_id(), On 2016/08/11 18:46:57, Mr4D wrote: > Would it make sense to use GetTitle() if the window title is empty? Yes. I think showing empty title is weird and should be fallen back to the app title.
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== ARC: Show window title in launcher menu instead of app title BUG=627590 TEST=manual test ========== to ========== ARC: Show window title in launcher menu instead of app title BUG=627590 TEST=manual test ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== ARC: Show window title in launcher menu instead of app title BUG=627590 TEST=manual test ========== to ========== ARC: Show window title in launcher menu instead of app title BUG=627590 TEST=manual test Committed: https://crrev.com/6ecae883342f04ab73135958cd9067fea0fa27d0 Cr-Commit-Position: refs/heads/master@{#411407} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/6ecae883342f04ab73135958cd9067fea0fa27d0 Cr-Commit-Position: refs/heads/master@{#411407}
Message was sent while issue was closed.
oshima@chromium.org changed reviewers: + oshima@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:56: (window ? window->title() : GetTitle()), &image, app_id(), On 2016/08/11 19:41:57, yoshiki wrote: > On 2016/08/11 18:46:57, Mr4D wrote: > > Would it make sense to use GetTitle() if the window title is empty? > > Yes. I think showing empty title is weird and should be fallen back to the app > title. Did you implement this?
Message was sent while issue was closed.
jamescook@chromium.org changed reviewers: + jamescook@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:51: for (auto it = windows().begin(); it != windows().end(); ++it, ++i) { Dumb question: windows() always has an even number of items? So it is safe to do ++it, ++it without testing against end()?
Message was sent while issue was closed.
On 2016/08/16 17:57:18, oshima wrote: > https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... > File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc > (right): > > https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... > chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:56: > (window ? window->title() : GetTitle()), &image, app_id(), > On 2016/08/11 19:41:57, yoshiki wrote: > > On 2016/08/11 18:46:57, Mr4D wrote: > > > Would it make sense to use GetTitle() if the window title is empty? > > > > Yes. I think showing empty title is weird and should be fallen back to the app > > title. > > Did you implement this? I forgot to do it. Let me do it in the separate issue: http://crbug.com/640091.
Message was sent while issue was closed.
https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:51: for (auto it = windows().begin(); it != windows().end(); ++it, ++i) { On 2016/08/17 21:41:33, James Cook (slow reviews) wrote: > Dumb question: windows() always has an even number of items? So it is safe to do > ++it, ++it without testing against end()? On 2016/08/17 21:41:34, James Cook (slow reviews) wrote: > https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... > File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc > (right): > > https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... > chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:51: > for (auto it = windows().begin(); it != windows().end(); ++it, ++i) { > Dumb question: windows() always has an even number of items? So it is safe to do > ++it, ++it without testing against end()? The second incremented variable is not "it" but "i".
Message was sent while issue was closed.
https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:51: for (auto it = windows().begin(); it != windows().end(); ++it, ++i) { On 2016/08/23 08:23:56, yoshiki wrote: > On 2016/08/17 21:41:33, James Cook (slow reviews) wrote: > > Dumb question: windows() always has an even number of items? So it is safe to > do > > ++it, ++it without testing against end()? > > On 2016/08/17 21:41:34, James Cook (slow reviews) wrote: > > > https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... > > File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc > > (right): > > > > > https://codereview.chromium.org/2243573002/diff/1/chrome/browser/ui/ash/launc... > > chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:51: > > for (auto it = windows().begin(); it != windows().end(); ++it, ++i) { > > Dumb question: windows() always has an even number of items? So it is safe to > do > > ++it, ++it without testing against end()? > > The second incremented variable is not "it" but "i". FYI: you can get the index by "itr - windows().begin()". |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
