Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2432013002:
Improve intent: URI handling (Closed)
|
Created:
4 years, 2 months ago by Yusuke Sato Modified:
4 years, 1 month ago CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), davemoore+watch_chromium.org, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yusukes+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove intent: URI handling
This CL adds |fallback_url| field to the IntentHandlerInfo struct so
that RequestUrlHandlerList() can return such a URL when it is called
with intent:. This CL also modifies our external protocol handler to
let it handle the new field properly.
For example, if RequestUrlHandlerList() is called with
"intent://xxx/#Intent;package=com.xyz;S.browser_fallback_url=http://abc.org;end"
and the package com.xyz is not installed, the API returns
"http://abc.org", and the new external protocol handler code opens the
URL in a Chrome tab. Or, if the URL is without a browser fallback URL,
"intent://xxx/#Intent;package=com.xyz;end", and the package com.xyz
is not installed, the API returns "market://details?id=com.xyz" so the
user can open the app installation page if they want.
BUG=654944
TEST=try
Committed: https://crrev.com/b7ee04cc50b5f176d9033814e5d81565c4cf4d50
Cr-Commit-Position: refs/heads/master@{#427409}
Patch Set 1 #Patch Set 2 : tests added #Patch Set 3 : review #
Total comments: 8
Patch Set 4 : address comments #
Total comments: 2
Patch Set 5 : comment fix #Patch Set 6 : rebase, fix one TODO #Patch Set 7 : rebase onto Luis' directory reorganize CL #
Messages
Total messages: 46 (33 generated)
The CQ bit was checked by yusukes@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yusukes@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yusukes@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...
Improve intent: URI handling This CL adds |fallback_url| field to the IntentHandlerInfo struct so that RequestUrlHandlerList() can return such a URL when it is called with intent:. This CL also modifies our external protocol handler to let it handle the new field properly. For example, if RequestUrlHandlerList() is called with "intent://xxx/#Intent;package=com.xyz;S.browser_fallback_url=http://abc.org;end" and the package com.xyz is not installed, the API returns "http://abc.org", and the new external protocol handler code opens the URL in a Chrome tab. Or, if the URL is without a browser fallback URL, "intent://xxx/#Intent;package=com.xyz;end", and the package com.xyz is not installed, the API returns "market://details?id=com.xyz" so the user can open the app installation page if they want. BUG=654944 TEST=try
Description was changed from ========== wip BUG=654944 TEST=(adding unit tests) ========== to ========== Improve intent: URI handling This CL adds |fallback_url| field to the IntentHandlerInfo struct so that RequestUrlHandlerList() can return such a URL when it is called with intent:. This CL also modifies our external protocol handler to let it handle the new field properly. For example, if RequestUrlHandlerList() is called with "intent://xxx/#Intent;package=com.xyz;S.browser_fallback_url=http://abc.org;end" and the package com.xyz is not installed, the API returns "http://abc.org", and the new external protocol handler code opens the URL in a Chrome tab. Or, if the URL is without a browser fallback URL, "intent://xxx/#Intent;package=com.xyz;end", and the package com.xyz is not installed, the API returns "market://details?id=com.xyz" so the user can open the app installation page if they want. BUG=654944 TEST=try ==========
yusukes@chromium.org changed reviewers: + djacobo@google.com, kinaba@chromium.org, rickyz@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Only a question and a couple of nits, thanks for the extensive amount of test cases. https://codereview.chromium.org/2432013002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2432013002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc:86: url, content::Referrer(), WindowOpenDisposition::CURRENT_TAB, qq: does using content::Referrer() in here means we are discarding part of the navigation story within this |web_contents|, are we OK with this? https://codereview.chromium.org/2432013002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc:109: // |package_name| matches a valid option and return the index. nit: kind of duplicated comment? I would keep the one at lines [104 -105] :) https://codereview.chromium.org/2432013002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_external_protocol_dialog_unittest.cc (right): https://codereview.chromium.org/2432013002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_external_protocol_dialog_unittest.cc:289: const std::string package_name = "com.other.browser"; awesome name :P https://codereview.chromium.org/2432013002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_external_protocol_dialog_unittest.cc:386: // Tests the same, but make the first handler a preferred one. Isn't that the second? You are talking about the order within the handlers structure right?
The CQ bit was checked by yusukes@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...
The CQ bit was checked by yusukes@chromium.org to run a CQ dry run
Please take another look. https://codereview.chromium.org/2432013002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2432013002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc:86: url, content::Referrer(), WindowOpenDisposition::CURRENT_TAB, On 2016/10/24 18:17:36, djacobo wrote: > qq: does using content::Referrer() in here means we are discarding part of the > navigation story within this |web_contents|, are we OK with this? Let me make this a TODO. Sending a non-empty referrer following the privacy policy requires a fair amount of work, I think. https://codereview.chromium.org/2432013002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc:109: // |package_name| matches a valid option and return the index. On 2016/10/24 18:17:36, djacobo wrote: > nit: kind of duplicated comment? I would keep the one at lines [104 -105] :) Thanks, this is a copy-and-paste mistake. Moved the comment back to OnIntentPickerClosed. https://codereview.chromium.org/2432013002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_external_protocol_dialog_unittest.cc (right): https://codereview.chromium.org/2432013002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_external_protocol_dialog_unittest.cc:386: // Tests the same, but make the first handler a preferred one. On 2016/10/24 18:17:36, djacobo wrote: > Isn't that the second? You are talking about the order within the handlers > structure right? Done. https://codereview.chromium.org/2432013002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2432013002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc:236: // TODO(yusukes): Call ArcNavigationThrottle::GetAppIndex once it's ready. Removed the test for GetAppIndex() and instead added this TODO. I'll remove GetAppIndex() in this function once https://codereview.chromium.org/2443313002/ is submitted.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2432013002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2432013002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc:86: url, content::Referrer(), WindowOpenDisposition::CURRENT_TAB, On 2016/10/24 19:30:11, Yusuke Sato wrote: > On 2016/10/24 18:17:36, djacobo wrote: > > qq: does using content::Referrer() in here means we are discarding part of the > > navigation story within this |web_contents|, are we OK with this? > > Let me make this a TODO. Sending a non-empty referrer following the privacy > policy requires a fair amount of work, I think. Sounds fine to me, thanks. https://codereview.chromium.org/2432013002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2432013002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc:236: // TODO(yusukes): Call ArcNavigationThrottle::GetAppIndex once it's ready. On 2016/10/24 19:30:11, Yusuke Sato wrote: > Removed the test for GetAppIndex() and instead added this TODO. I'll remove > GetAppIndex() in this function once https://codereview.chromium.org/2443313002/ > is submitted. Acknowledged.
Thanks David. Ricky, kinaba, PTAL
mojom lgtm - thanks for the extensive tests!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yusukes@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm! sorry for taking long for the review
The CQ bit was checked by yusukes@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...
yusukes@chromium.org changed reviewers: + xiyuan@chromium.org
Thanks all! +xiyuan, c/b/chromeos/ owner. Please have a look at the directory. I'm adding unit tests to arc/. The diff between PS#6 and PS#7 is just rebase (no code change.) Thanks.
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 yusukes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rickyz@chromium.org, djacobo@google.com, kinaba@chromium.org Link to the patchset: https://codereview.chromium.org/2432013002/#ps120001 (title: "rebase onto Luis' directory reorganize CL")
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 ========== Improve intent: URI handling This CL adds |fallback_url| field to the IntentHandlerInfo struct so that RequestUrlHandlerList() can return such a URL when it is called with intent:. This CL also modifies our external protocol handler to let it handle the new field properly. For example, if RequestUrlHandlerList() is called with "intent://xxx/#Intent;package=com.xyz;S.browser_fallback_url=http://abc.org;end" and the package com.xyz is not installed, the API returns "http://abc.org", and the new external protocol handler code opens the URL in a Chrome tab. Or, if the URL is without a browser fallback URL, "intent://xxx/#Intent;package=com.xyz;end", and the package com.xyz is not installed, the API returns "market://details?id=com.xyz" so the user can open the app installation page if they want. BUG=654944 TEST=try ========== to ========== Improve intent: URI handling This CL adds |fallback_url| field to the IntentHandlerInfo struct so that RequestUrlHandlerList() can return such a URL when it is called with intent:. This CL also modifies our external protocol handler to let it handle the new field properly. For example, if RequestUrlHandlerList() is called with "intent://xxx/#Intent;package=com.xyz;S.browser_fallback_url=http://abc.org;end" and the package com.xyz is not installed, the API returns "http://abc.org", and the new external protocol handler code opens the URL in a Chrome tab. Or, if the URL is without a browser fallback URL, "intent://xxx/#Intent;package=com.xyz;end", and the package com.xyz is not installed, the API returns "market://details?id=com.xyz" so the user can open the app installation page if they want. BUG=654944 TEST=try ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Improve intent: URI handling This CL adds |fallback_url| field to the IntentHandlerInfo struct so that RequestUrlHandlerList() can return such a URL when it is called with intent:. This CL also modifies our external protocol handler to let it handle the new field properly. For example, if RequestUrlHandlerList() is called with "intent://xxx/#Intent;package=com.xyz;S.browser_fallback_url=http://abc.org;end" and the package com.xyz is not installed, the API returns "http://abc.org", and the new external protocol handler code opens the URL in a Chrome tab. Or, if the URL is without a browser fallback URL, "intent://xxx/#Intent;package=com.xyz;end", and the package com.xyz is not installed, the API returns "market://details?id=com.xyz" so the user can open the app installation page if they want. BUG=654944 TEST=try ========== to ========== Improve intent: URI handling This CL adds |fallback_url| field to the IntentHandlerInfo struct so that RequestUrlHandlerList() can return such a URL when it is called with intent:. This CL also modifies our external protocol handler to let it handle the new field properly. For example, if RequestUrlHandlerList() is called with "intent://xxx/#Intent;package=com.xyz;S.browser_fallback_url=http://abc.org;end" and the package com.xyz is not installed, the API returns "http://abc.org", and the new external protocol handler code opens the URL in a Chrome tab. Or, if the URL is without a browser fallback URL, "intent://xxx/#Intent;package=com.xyz;end", and the package com.xyz is not installed, the API returns "market://details?id=com.xyz" so the user can open the app installation page if they want. BUG=654944 TEST=try Committed: https://crrev.com/b7ee04cc50b5f176d9033814e5d81565c4cf4d50 Cr-Commit-Position: refs/heads/master@{#427409} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b7ee04cc50b5f176d9033814e5d81565c4cf4d50 Cr-Commit-Position: refs/heads/master@{#427409} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
