Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2165193002:
[arc-intents] Adjust link-click detection code. (Closed)
|
Created:
4 years, 5 months ago by Ben Kwa Modified:
4 years, 5 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[arc-intents] Adjust link-click detection code.
Use the page transition, which seems more reliable, rather than the
HasUserGesture method, to detect whether a given navigation event was
spawned by the user clicking on a link.
BUG=630072
Committed: https://crrev.com/ef206ad2f2bd182205b4adb5580c00e7967c4fd8
Cr-Commit-Position: refs/heads/master@{#407256}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix typo #
Total comments: 2
Patch Set 3 : Properly mask page transition; add braces. #Patch Set 4 : Use page transition API #
Total comments: 2
Patch Set 5 : Exclude qualified page transitions. #
Total comments: 4
Patch Set 6 : Minor cleanup. #Messages
Total messages: 34 (20 generated)
Description was changed from ========== [arc-intents] Adjust link-click detection code. Use the page transition, which seems more reliable, rather than the HasUserGesture method, to detect whether a given navigation event was spawned by the user clicking on a link. BUG=6300723 ========== to ========== [arc-intents] Adjust link-click detection code. Use the page transition, which seems more reliable, rather than the HasUserGesture method, to detect whether a given navigation event was spawned by the user clicking on a link. BUG=6300723 ==========
kenobi@chromium.org changed reviewers: + djacobo@google.com, yusukes@chromium.org
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
yusukes@ is ooo, so i'll take over for him. https://codereview.chromium.org/2165193002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2165193002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:70: This seems to be missing a closing brace. Can you run trybots?
The CQ bit was checked by kenobi@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...
https://codereview.chromium.org/2165193002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2165193002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:70: On 2016/07/21 15:51:09, Luis Héctor Chávez wrote: > This seems to be missing a closing brace. Can you run trybots? What th... >.< Sorry about that. Fixed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can you update the bug? It doesn't seem to be a valid crbug.com bug. https://codereview.chromium.org/2165193002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2165193002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:67: if (navigation_handle()->GetPageTransition() != ui::PAGE_TRANSITION_LINK) Brace elision is only valid if there is just one simple statement. This does need braces due to the added comment. Also, doesn't this need to be something like (navigation_handle()->GetPageTransition() & ui::PAGE_TRANSITION_CORE_MASK) != ui::PAGE_TRANSITION_LINK ?
Description was changed from ========== [arc-intents] Adjust link-click detection code. Use the page transition, which seems more reliable, rather than the HasUserGesture method, to detect whether a given navigation event was spawned by the user clicking on a link. BUG=6300723 ========== to ========== [arc-intents] Adjust link-click detection code. Use the page transition, which seems more reliable, rather than the HasUserGesture method, to detect whether a given navigation event was spawned by the user clicking on a link. BUG=630072 ==========
The CQ bit was checked by kenobi@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...
https://codereview.chromium.org/2165193002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2165193002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:67: if (navigation_handle()->GetPageTransition() != ui::PAGE_TRANSITION_LINK) On 2016/07/21 18:08:36, Luis Héctor Chávez wrote: > Brace elision is only valid if there is just one simple statement. This does > need braces due to the added comment. > > Also, doesn't this need to be something like > > (navigation_handle()->GetPageTransition() & ui::PAGE_TRANSITION_CORE_MASK) != > ui::PAGE_TRANSITION_LINK > > ? Done. Thanks for the catch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by kenobi@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...
Changed again, this time to use the page transition API which I just found. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2165193002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2165193002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:69: // If this navigation event wasn't spawned by the user clicking on a link. nit: was spawned? From the PAGE_TRANSITION_LINK comment: "User got to this page by clicking a link on another page."
Ok, third time lucky (maybe). TLDR is that we need to look at both the core transition type as well as the transition qualifiers to completely determine whether this was a link click. Details in the linked bug. https://codereview.chromium.org/2165193002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2165193002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:69: // If this navigation event wasn't spawned by the user clicking on a link. On 2016/07/22 15:35:34, Luis Héctor Chávez wrote: > nit: was spawned? > > From the PAGE_TRANSITION_LINK comment: "User got to this page by clicking a link > on another page." Egads. Sorry, this page transition code is confusing. Fixed now.
oh this looks much better :) lgtm https://codereview.chromium.org/2165193002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2165193002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:80: // b/29250054). Can you put all the accumulated knowledge in crbug.com/630072 (and change this link to the public bug)? https://codereview.chromium.org/2165193002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:85: return content::NavigationThrottle::PROCEED; nit: remove the braces to see if the diff becomes prettier.
Thanks! https://codereview.chromium.org/2165193002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2165193002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:80: // b/29250054). On 2016/07/22 19:08:18, Luis Héctor Chávez wrote: > Can you put all the accumulated knowledge in crbug.com/630072 (and change this > link to the public bug)? Done. https://codereview.chromium.org/2165193002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:85: return content::NavigationThrottle::PROCEED; On 2016/07/22 19:08:18, Luis Héctor Chávez wrote: > nit: remove the braces to see if the diff becomes prettier. Done.
The CQ bit was checked by kenobi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from djacobo@google.com, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2165193002/#ps100001 (title: "Minor cleanup.")
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-intents] Adjust link-click detection code. Use the page transition, which seems more reliable, rather than the HasUserGesture method, to detect whether a given navigation event was spawned by the user clicking on a link. BUG=630072 ========== to ========== [arc-intents] Adjust link-click detection code. Use the page transition, which seems more reliable, rather than the HasUserGesture method, to detect whether a given navigation event was spawned by the user clicking on a link. BUG=630072 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [arc-intents] Adjust link-click detection code. Use the page transition, which seems more reliable, rather than the HasUserGesture method, to detect whether a given navigation event was spawned by the user clicking on a link. BUG=630072 ========== to ========== [arc-intents] Adjust link-click detection code. Use the page transition, which seems more reliable, rather than the HasUserGesture method, to detect whether a given navigation event was spawned by the user clicking on a link. BUG=630072 Committed: https://crrev.com/ef206ad2f2bd182205b4adb5580c00e7967c4fd8 Cr-Commit-Position: refs/heads/master@{#407256} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ef206ad2f2bd182205b4adb5580c00e7967c4fd8 Cr-Commit-Position: refs/heads/master@{#407256} |
