fix for Text is not displaying in search box after pressing the ctrl+z key in Bookmark Manager if d…

fix for Text is not displaying in search box after pressing the ctrl+z key
in Bookmark Manager if deletion is performed before search completion.
ctrl+z is handled at JS level in the page itself no matter where it has
been pressed, made the changes such that for pressing ctrl+z in search
box will not be handled in js and will be handled normally.

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

[chhajer.m, 2014-07-15 10:08:11]

I have added a patch for the issue,
please take a look.

[Pam (message me for reviews), 2014-07-17 17:28:00]

It looks like yosin@ is familiar with the bookmark manager and can review it
properly. I'm happy to give resources/ owner approval once that's done.

- Pam

[yosin_UTC9, 2014-07-18 01:34:24]

LGTM

Thanks for fixing bug!
Add arv@ for OWNERS review for landing.

https://codereview.chromium.org/390983009/diff/1/chrome/browser/resources/boo...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/390983009/diff/1/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/main.js:465: var input = $('term');
nit: Since |input| is used once, we don't need to have |input|.
nit: This is simple boolean expression, we could write:

e.canExecurte = e.currentTarget.activeElement !== $('term');

Note: we would like to use strict non-equality '!=='

[chhajer.m, 2014-07-18 07:49:46]

Thanks Yosi.
I've incorporated review comments and patch set#2 is uploaded.

@Arv,
Please have a look.

https://codereview.chromium.org/390983009/diff/1/chrome/browser/resources/boo...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/390983009/diff/1/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/main.js:465: var input = $('term');
On 2014/07/18 01:34:24, Yosi_UTC9 wrote:
> nit: Since |input| is used once, we don't need to have |input|.
> nit: This is simple boolean expression, we could write:
> 
> e.canExecurte = e.currentTarget.activeElement !== $('term');
> 
> Note: we would like to use strict non-equality '!==' 

Done.

[Pam (message me for reviews), 2014-07-18 21:31:51]

https://codereview.chromium.org/390983009/diff/20001/chrome/browser/resources...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/390983009/diff/20001/chrome/browser/resources...
chrome/browser/resources/bookmark_manager/js/main.js:464: // Just to serprate
undo on search text box to be handled
Sorry, but I don't understand this comment, and it appears to contradict itself.
Does this describe what the code now does?

// If the search box is active, pass the undo command through (fixes
http://crbug.com/278112).
// Otherwise, because the global undo command has no visible UI, always enable
it, and just make it
// a no-op if undo is not possible.

[chhajer.m, 2014-07-23 04:46:24]

On 2014/07/18 21:31:51, Pam (also PM for reviews) wrote:
>
https://codereview.chromium.org/390983009/diff/20001/chrome/browser/resources...
> File chrome/browser/resources/bookmark_manager/js/main.js (right):
> 
>
https://codereview.chromium.org/390983009/diff/20001/chrome/browser/resources...
> chrome/browser/resources/bookmark_manager/js/main.js:464: // Just to serprate
> undo on search text box to be handled
> Sorry, but I don't understand this comment, and it appears to contradict
itself.
> Does this describe what the code now does?
> 
> // If the search box is active, pass the undo command through (fixes
> http://crbug.com/278112).
> // Otherwise, because the global undo command has no visible UI, always enable
> it, and just make it
> // a no-op if undo is not possible.

Thanks Pam.
I've incorporated review comments and patch set#3 is uploaded.

[chhajer.m, 2014-07-23 05:23:35]

Thanks Pam.
I've incorporated review comments and patch set#3 is uploaded.

https://codereview.chromium.org/390983009/diff/20001/chrome/browser/resources...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/390983009/diff/20001/chrome/browser/resources...
chrome/browser/resources/bookmark_manager/js/main.js:464: // Just to serprate
undo on search text box to be handled
On 2014/07/18 21:31:50, Pam (also PM for reviews) wrote:
> Sorry, but I don't understand this comment, and it appears to contradict
itself.
> Does this describe what the code now does?
> 
> // If the search box is active, pass the undo command through (fixes
> http://crbug.com/278112).
> // Otherwise, because the global undo command has no visible UI, always enable
> it, and just make it
> // a no-op if undo is not possible.

Done.

[Pam (message me for reviews), 2014-07-23 17:51:59]

LGTM with a tiny nit, described below.

https://codereview.chromium.org/390983009/diff/40001/chrome/browser/resources...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/390983009/diff/40001/chrome/browser/resources...
chrome/browser/resources/bookmark_manager/js/main.js:464: // the global undo
command has no visible UI, so always enable it, and
nit: remove "so" (it's redundant with "because" now)

[chhajer.m, 2014-07-24 04:37:16]

Thanks Pam.
I've incorporated review comments and patch set#3 is uploaded.

https://codereview.chromium.org/390983009/diff/40001/chrome/browser/resources...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/390983009/diff/40001/chrome/browser/resources...
chrome/browser/resources/bookmark_manager/js/main.js:464: // the global undo
command has no visible UI, so always enable it, and
On 2014/07/23 17:51:58, Pam (also PM for reviews) wrote:
> nit: remove "so" (it's redundant with "because" now)

Done.

[chhajer.m, 2014-07-24 04:38:07]

The CQ bit was checked by chhajer.m@samsung.com

[commit-bot: I haz the power, 2014-07-24 04:39:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chhajer.m@samsung.com/390983009/60001

[commit-bot: I haz the power, 2014-07-24 06:34:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[commit-bot: I haz the power, 2014-07-24 14:04:28]

Change committed as 285241