Support "Report Issue" in case of no-browser windows in ChromeOS.

Support "Report Issue" in case of no-browser windows in ChromeOS.

"Report Issue" shortcut (Alt-Shift-I) should be available even if there are
no browser windows, in case that the user wants to report issues of ChromeOS
window systems.

Changes around ash:
add OPEN_FEEDBACK_PAGE accelerator command and assign it in the global shortcut key

Changes around feedback_ui:
allow 'tab_index' parameter is -1

BUG=131158
TEST=manually verified on lumpy, in case of no-windows and some windows

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=146530

[Yusuke Sato, 2012-07-09 17:49:59]

Ash accelerator part LGTM

http://codereview.chromium.org/10698115/diff/1/ash/accelerators/accelerator_t...
File ash/accelerators/accelerator_table.h (right):

http://codereview.chromium.org/10698115/diff/1/ash/accelerators/accelerator_t...
ash/accelerators/accelerator_table.h:70: OPEN_FEEDBACK_PAGE,
can you merge line 70-76 into 17-62 and sort them alphabetically? see the
comment above.

[rkc1, 2012-07-09 18:23:22]

http://codereview.chromium.org/10698115/diff/1/chrome/browser/ui/webui/feedba...
File chrome/browser/ui/webui/feedback_ui.cc (right):

http://codereview.chromium.org/10698115/diff/1/chrome/browser/ui/webui/feedba...
chrome/browser/ui/webui/feedback_ui.cc:392: #if !defined(OS_CHROMEOS)
We set the index in ShowFeedbackView for all platforms, so why would we want to
make the index check only non-cros?

[Jun Mukai, 2012-07-10 11:08:20]

http://codereview.chromium.org/10698115/diff/1/ash/accelerators/accelerator_t...
File ash/accelerators/accelerator_table.h (right):

http://codereview.chromium.org/10698115/diff/1/ash/accelerators/accelerator_t...
ash/accelerators/accelerator_table.h:70: OPEN_FEEDBACK_PAGE,
On 2012/07/09 17:49:59, Yusuke Sato wrote:
> can you merge line 70-76 into 17-62 and sort them alphabetically? see the
> comment above.

Done.

http://codereview.chromium.org/10698115/diff/1/chrome/browser/ui/webui/feedba...
File chrome/browser/ui/webui/feedback_ui.cc (right):

http://codereview.chromium.org/10698115/diff/1/chrome/browser/ui/webui/feedba...
chrome/browser/ui/webui/feedback_ui.cc:392: #if !defined(OS_CHROMEOS)
On 2012/07/09 18:23:22, g_rkc wrote:
> We set the index in ShowFeedbackView for all platforms, so why would we want
to
> make the index check only non-cros?

That's browser->active_index() in line 178, right?  I think most of platforms
expects that a tab is active for reporting issue.

However, in ChromeOS only, the user may want to open the feedback UI even though
there are no browser windows, for reporting the desktop itself.  In that case,
active_index() returns -1.  That's why I moved the check only for non-ChromeOS.

Can we just allow for all platforms?  I think either would be fine because the
index has to be set correctly in non-ChromeOS too.

[rkc1, 2012-07-10 17:20:10]

LGTM provided the check is removed.

http://codereview.chromium.org/10698115/diff/1/chrome/browser/ui/webui/feedba...
File chrome/browser/ui/webui/feedback_ui.cc (right):

http://codereview.chromium.org/10698115/diff/1/chrome/browser/ui/webui/feedba...
chrome/browser/ui/webui/feedback_ui.cc:392: #if !defined(OS_CHROMEOS)
On 2012/07/10 11:08:20, Jun Mukai wrote:
> On 2012/07/09 18:23:22, g_rkc wrote:
> > We set the index in ShowFeedbackView for all platforms, so why would we want
> to
> > make the index check only non-cros?
> 
> That's browser->active_index() in line 178, right?  I think most of platforms
> expects that a tab is active for reporting issue.
> 
> However, in ChromeOS only, the user may want to open the feedback UI even
though
> there are no browser windows, for reporting the desktop itself.  In that case,
> active_index() returns -1.  That's why I moved the check only for
non-ChromeOS.
> 
> Can we just allow for all platforms?  I think either would be fine because the
> index has to be set correctly in non-ChromeOS too.

Yes, let's remove it for all platforms. There are an increasing number of cases
where we may not actually have the tab id; in which case our code already
compensates by leaving the target tab url as empty.

[rkc, 2012-07-10 18:44:04]

LGTM from the right account.

On 2012/07/10 17:20:10, g_rkc wrote:
> LGTM provided the check is removed.
> 
>
http://codereview.chromium.org/10698115/diff/1/chrome/browser/ui/webui/feedba...
> File chrome/browser/ui/webui/feedback_ui.cc (right):
> 
>
http://codereview.chromium.org/10698115/diff/1/chrome/browser/ui/webui/feedba...
> chrome/browser/ui/webui/feedback_ui.cc:392: #if !defined(OS_CHROMEOS)
> On 2012/07/10 11:08:20, Jun Mukai wrote:
> > On 2012/07/09 18:23:22, g_rkc wrote:
> > > We set the index in ShowFeedbackView for all platforms, so why would we
want
> > to
> > > make the index check only non-cros?
> > 
> > That's browser->active_index() in line 178, right?  I think most of
platforms
> > expects that a tab is active for reporting issue.
> > 
> > However, in ChromeOS only, the user may want to open the feedback UI even
> though
> > there are no browser windows, for reporting the desktop itself.  In that
case,
> > active_index() returns -1.  That's why I moved the check only for
> non-ChromeOS.
> > 
> > Can we just allow for all platforms?  I think either would be fine because
the
> > index has to be set correctly in non-ChromeOS too.
> 
> Yes, let's remove it for all platforms. There are an increasing number of
cases
> where we may not actually have the tab id; in which case our code already
> compensates by leaving the target tab url as empty.

[Jun Mukai, 2012-07-11 12:42:39]

http://codereview.chromium.org/10698115/diff/1/chrome/browser/ui/webui/feedba...
File chrome/browser/ui/webui/feedback_ui.cc (right):

http://codereview.chromium.org/10698115/diff/1/chrome/browser/ui/webui/feedba...
chrome/browser/ui/webui/feedback_ui.cc:392: #if !defined(OS_CHROMEOS)
Sure, then removed the check itself.  now index==-1 is valid here.

On 2012/07/10 17:20:10, g_rkc wrote:
> On 2012/07/10 11:08:20, Jun Mukai wrote:
> > On 2012/07/09 18:23:22, g_rkc wrote:
> > > We set the index in ShowFeedbackView for all platforms, so why would we
want
> > to
> > > make the index check only non-cros?
> > 
> > That's browser->active_index() in line 178, right?  I think most of
platforms
> > expects that a tab is active for reporting issue.
> > 
> > However, in ChromeOS only, the user may want to open the feedback UI even
> though
> > there are no browser windows, for reporting the desktop itself.  In that
case,
> > active_index() returns -1.  That's why I moved the check only for
> non-ChromeOS.
> > 
> > Can we just allow for all platforms?  I think either would be fine because
the
> > index has to be set correctly in non-ChromeOS too.
> 
> Yes, let's remove it for all platforms. There are an increasing number of
cases
> where we may not actually have the tab id; in which case our code already
> compensates by leaving the target tab url as empty.

[Jun Mukai, 2012-07-11 12:43:03]

sky, can you check the code as the owner of ash and c/b/ui?

[Jun Mukai, 2012-07-12 11:04:14]

I heard sky is on vacation. ben, can you take a look at this?

[Ben Goodger (Google), 2012-07-12 15:50:40]

lgtm

http://codereview.chromium.org/10698115/diff/9001/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/chrome_shell_delegate.cc (right):

http://codereview.chromium.org/10698115/diff/9001/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:329:
chrome::OpenFeedbackDialog(browser);
chrome::OpenFeedbackDialog(GetTargetBrowser());

[Jun Mukai, 2012-07-13 02:31:24]

http://codereview.chromium.org/10698115/diff/9001/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/chrome_shell_delegate.cc (right):

http://codereview.chromium.org/10698115/diff/9001/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:329:
chrome::OpenFeedbackDialog(browser);
On 2012/07/12 15:50:40, Ben Goodger (Google) wrote:
> chrome::OpenFeedbackDialog(GetTargetBrowser());

Done.

[commit-bot: I haz the power, 2012-07-13 02:31:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/10698115/11002

[commit-bot: I haz the power, 2012-07-13 04:48:38]

Try job failure for 10698115-11002 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-07-13 05:23:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/10698115/11002

[commit-bot: I haz the power, 2012-07-13 06:35:46]

Try job failure for 10698115-11002 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...