Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
439433004:
Browser goes to the previous page when exiting fullscreen via escape(esc) key of BT keyboard (Closed)
|
Created:
6 years, 4 months ago by divya.bansal Modified:
6 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionBrowser goes to the previous page when exiting fullscreen via escape(esc) key of BT keyboard
Browser should go back to video page(having inline video) through
escape key of BT keyboard instead of goig back to main page.
BUG=396996
Committed: https://crrev.com/594ca7929784afb94457e57d1656a4ae8b920495
Cr-Commit-Position: refs/heads/master@{#293181}
Patch Set 1 #Patch Set 2 : Added name in Authors file #Patch Set 3 : KeyEvent amended #
Total comments: 2
Patch Set 4 : Changes made according to review comments #Patch Set 5 : adding name in authors file #Patch Set 6 : Review changes #
Total comments: 3
Patch Set 7 : Adding indentation changes #Messages
Total messages: 26 (1 generated)
PTAL and give suggestions for this
I'm not familiar with this code. Adding a more appropriate reviewer.
On 2014/08/01 14:30:08, jdduke wrote: > I'm not familiar with this code. Adding a more appropriate reviewer. trchen@: Could you take a look?
On 2014/08/01 14:30:19, jdduke wrote: > On 2014/08/01 14:30:08, jdduke wrote: > > I'm not familiar with this code. Adding a more appropriate reviewer. > > trchen@: Could you take a look? PTAL and suggest if any changes are required.
On 2014/08/04 17:09:27, divya.bansal wrote: > On 2014/08/01 14:30:19, jdduke wrote: > > On 2014/08/01 14:30:08, jdduke wrote: > > > I'm not familiar with this code. Adding a more appropriate reviewer. > > > > trchen@: Could you take a look? > > PTAL and suggest if any changes are required. PTAL
On 2014/08/05 16:39:10, divya.bansal wrote: > On 2014/08/04 17:09:27, divya.bansal wrote: > > On 2014/08/01 14:30:19, jdduke wrote: > > > On 2014/08/01 14:30:08, jdduke wrote: > > > > I'm not familiar with this code. Adding a more appropriate reviewer. > > > > > > trchen@: Could you take a look? > > > > PTAL and suggest if any changes are required. > > PTAL PTAL
https://codereview.chromium.org/439433004/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java (right): https://codereview.chromium.org/439433004/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:130: } else if (keyCode == KeyEvent.KEYCODE_BACK && Can you combine the ESCAPE keyCode check with this one (using ||) ?
On 2014/08/22 18:01:44, jdduke wrote: > https://codereview.chromium.org/439433004/diff/40001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java > (right): > > https://codereview.chromium.org/439433004/diff/40001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:130: > } else if (keyCode == KeyEvent.KEYCODE_BACK && > Can you combine the ESCAPE keyCode check with this one (using ||) ? @jdduke Thanks for reviewing the changes. I will make the necessary changes.
https://chromiumcodereview.appspot.com/439433004/diff/40001/content/public/an... File content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java (right): https://chromiumcodereview.appspot.com/439433004/diff/40001/content/public/an... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:130: } else if (keyCode == KeyEvent.KEYCODE_BACK && On 2014/08/22 18:01:43, jdduke wrote: > Can you combine the ESCAPE keyCode check with this one (using ||) ? Done.
On 2014/08/25 06:53:03, divya.bansal wrote: > https://chromiumcodereview.appspot.com/439433004/diff/40001/content/public/an... > File > content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java > (right): > > https://chromiumcodereview.appspot.com/439433004/diff/40001/content/public/an... > content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:130: > } else if (keyCode == KeyEvent.KEYCODE_BACK && > On 2014/08/22 18:01:43, jdduke wrote: > > Can you combine the ESCAPE keyCode check with this one (using ||) ? > > Done. @jdduke Please review the changes and give your suggestions
On 2014/08/25 06:53:44, divya.bansal wrote: > On 2014/08/25 06:53:03, divya.bansal wrote: > > > https://chromiumcodereview.appspot.com/439433004/diff/40001/content/public/an... > > File > > > content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java > > (right): > > > > > https://chromiumcodereview.appspot.com/439433004/diff/40001/content/public/an... > > > content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:130: > > } else if (keyCode == KeyEvent.KEYCODE_BACK && > > On 2014/08/22 18:01:43, jdduke wrote: > > > Can you combine the ESCAPE keyCode check with this one (using ||) ? > > > > Done. > > @jdduke Please review the changes and give your suggestions PTAL
Removing myself as a reviewer, trchen@ please either take a look or close this as appropriate.
jdduke@chromium.org changed reviewers: - jdduke@chromium.org
On 2014/08/26 15:26:48, jdduke wrote: > mailto:jdduke@chromium.org changed reviewers: > - mailto:jdduke@chromium.org @trchen please review the changes and give your suggestions.
On 2014/08/28 05:28:16, divya.bansal wrote: > On 2014/08/26 15:26:48, jdduke wrote: > > mailto:jdduke@chromium.org changed reviewers: > > - mailto:jdduke@chromium.org > > @trchen please review the changes and give your suggestions. PTAL
On 2014/08/31 12:49:36, divya.bansal wrote: > On 2014/08/28 05:28:16, divya.bansal wrote: > > On 2014/08/26 15:26:48, jdduke wrote: > > > mailto:jdduke@chromium.org changed reviewers: > > > - mailto:jdduke@chromium.org > > > > @trchen please review the changes and give your suggestions. > > PTAL PTAL
this seems reasonable to to me. lgtm w/ nit https://chromiumcodereview.appspot.com/439433004/diff/100001/AUTHORS File AUTHORS (right): https://chromiumcodereview.appspot.com/439433004/diff/100001/AUTHORS#newcode105 AUTHORS:105: Divya Bansal <divya.bansal@samsung.com> And this should be reverted as you already were added in a separate CL. https://chromiumcodereview.appspot.com/439433004/diff/100001/content/public/a... File content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java (right): https://chromiumcodereview.appspot.com/439433004/diff/100001/content/public/a... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:131: && event.getAction() == KeyEvent.ACTION_UP) { +4 indent
On 2014/09/02 16:28:56, Ted C wrote: > this seems reasonable to to me. > > lgtm w/ nit > > https://chromiumcodereview.appspot.com/439433004/diff/100001/AUTHORS > File AUTHORS (right): > > https://chromiumcodereview.appspot.com/439433004/diff/100001/AUTHORS#newcode105 > AUTHORS:105: Divya Bansal <mailto:divya.bansal@samsung.com> > And this should be reverted as you already were added in a separate CL. > > https://chromiumcodereview.appspot.com/439433004/diff/100001/content/public/a... > File > content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java > (right): > > https://chromiumcodereview.appspot.com/439433004/diff/100001/content/public/a... > content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:131: > && event.getAction() == KeyEvent.ACTION_UP) { > +4 indent @Ted thanks for the review. I will do the necessary changes and commit the change.
https://chromiumcodereview.appspot.com/439433004/diff/100001/content/public/a... File content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java (right): https://chromiumcodereview.appspot.com/439433004/diff/100001/content/public/a... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:131: && event.getAction() == KeyEvent.ACTION_UP) { On 2014/09/02 16:28:56, Ted C wrote: > +4 indent Done.
On 2014/09/03 04:42:23, divya.bansal wrote: > https://chromiumcodereview.appspot.com/439433004/diff/100001/content/public/a... > File > content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java > (right): > > https://chromiumcodereview.appspot.com/439433004/diff/100001/content/public/a... > content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:131: > && event.getAction() == KeyEvent.ACTION_UP) { > On 2014/09/02 16:28:56, Ted C wrote: > > +4 indent > > Done. @Ted Please reviewthe changes.
The CQ bit was checked by tedchoc@chromium.org
On 2014/09/03 04:42:46, divya.bansal wrote: > On 2014/09/03 04:42:23, divya.bansal wrote: > > > https://chromiumcodereview.appspot.com/439433004/diff/100001/content/public/a... > > File > > > content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java > > (right): > > > > > https://chromiumcodereview.appspot.com/439433004/diff/100001/content/public/a... > > > content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:131: > > && event.getAction() == KeyEvent.ACTION_UP) { > > On 2014/09/02 16:28:56, Ted C wrote: > > > +4 indent > > > > Done. > > @Ted Please reviewthe changes. Still looks good for me, but for changes where I give an lgtm with comments it is fine to just address the comments and land the patch without asking for a re-review.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/divya.bansal@samsung.com/439433004/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as 624afdc303290b55524772ecf265f7fde0f053f9
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/594ca7929784afb94457e57d1656a4ae8b920495 Cr-Commit-Position: refs/heads/master@{#293181} |
