Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2667803002:
Picker takes a vector of pairs, and passes back the user chosen target. (Closed)
|
Created:
3 years, 10 months ago by constantina Modified:
3 years, 10 months ago CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPicker takes a vector of pairs, and passes back the user chosen target.
Instead of vector of string16s, input is a vector of pairs of string16
and GURL where the first element is the app name, and the second is the
manifest URL of the app.
The manifest URL of the chosen target is passed back via a callback as
a string. The passed URL is Optional, so it can be null if the share is
cancelled.
BUG=668389
Review-Url: https://codereview.chromium.org/2667803002
Cr-Commit-Position: refs/heads/master@{#448512}
Committed: https://chromium.googlesource.com/chromium/src/+/e4c513e7ae3fd3a6c7752f8add47b93fb1c66444
Patch Set 1 #Patch Set 2 : Change tests to check chosen target #Patch Set 3 : Rebase mgiuca@ patch #Patch Set 4 : Simplify test #Patch Set 5 : Move replacing of placeholders to after picker #
Total comments: 11
Patch Set 6 : Rebase #Patch Set 7 : Fix upstream #Patch Set 8 : Fix things that slipped through rebase cracks #
Total comments: 2
Patch Set 9 : Fixed, as per feedback #Patch Set 10 : Fix another reference from enum to optional #Patch Set 11 : Check a target is selected #Patch Set 12 : Enable share button, only if target selected #
Total comments: 21
Patch Set 13 : Fixed, as per feedback. #
Total comments: 2
Patch Set 14 : Fixed nits #Patch Set 15 : Pass vector of pairs of name and manifest URLs to UI #Patch Set 16 : Update documentation #Patch Set 17 : Pass vector of string16 and gurl, and return optional string #
Total comments: 14
Patch Set 18 : Rebase-update #Patch Set 19 : Fixed, as per feedback #
Total comments: 4
Patch Set 20 : Dynamically enable/disable button #Messages
Total messages: 53 (21 generated)
The CQ bit was checked by constantina@google.com 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...
constantina@google.com changed reviewers: + mgiuca@chromium.org, sky@chromium.org
mgiuca@chromium.org: Please review changes in chrome/browser/webshare sky@chromium.org: Please review changes in chrome/browser (browser_dialogs.h and views code).
The CQ bit was checked by constantina@google.com 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_dialogs.h:157: // titles that will be shown in a list. Calls |callback| with SHARE if an app Update description https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:85: close_callback_.Run(GetText(table_->FirstSelectedRow(), 0)); What if there is no selected row? https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/webshare... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:156: callback.Run(base::Optional<std::string>("Share was cancelled")); Shouldn't this run the callback with no value and let the callback decide how to interpret it? https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/webshare... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.h:39: const base::Callback<void(base::Optional<base::string16>)>& callback); Document |callback|
https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/webshare... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:156: callback.Run(base::Optional<std::string>("Share was cancelled")); On 2017/01/31 05:22:32, sky wrote: > Shouldn't this run the callback with no value and let the callback decide how to > interpret it? There are two different callbacks at play here with different semantics: the incoming one (which is given an Optional<string16>; previously got a SharePickerResult) gets nullopt if cancelled, or else the URL template. The outgoing one, |callback|, gets nullopt if successful or else an error message string. (That outgoing callback is defined by the mojom long ago.) So it makes sense here to be converting a nullopt (cancellation) into a non-null string (error message). https://codereview.chromium.org/2667803002/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2667803002/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:13: #include "base/optional.h" Need base/strings/string16 as well.
Shouldn't the outgoing one get an enum for the error and have it up to the site using the api to map to a string if appropriate? On Mon, Jan 30, 2017 at 11:28 PM, <mgiuca@chromium.org> wrote: > > https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/webshare... > File chrome/browser/webshare/share_service_impl.cc (right): > > https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/webshare... > chrome/browser/webshare/share_service_impl.cc:156: > callback.Run(base::Optional<std::string>("Share was cancelled")); > On 2017/01/31 05:22:32, sky wrote: >> Shouldn't this run the callback with no value and let the callback > decide how to >> interpret it? > > There are two different callbacks at play here with different semantics: > the incoming one (which is given an Optional<string16>; previously got a > SharePickerResult) gets nullopt if cancelled, or else the URL template. > The outgoing one, |callback|, gets nullopt if successful or else an > error message string. (That outgoing callback is defined by the mojom > long ago.) > > So it makes sense here to be converting a nullopt (cancellation) into a > non-null string (error message). > > https://codereview.chromium.org/2667803002/diff/140001/chrome/browser/ui/brow... > File chrome/browser/ui/browser_dialogs.h (right): > > https://codereview.chromium.org/2667803002/diff/140001/chrome/browser/ui/brow... > chrome/browser/ui/browser_dialogs.h:13: #include "base/optional.h" > Need base/strings/string16 as well. > > https://codereview.chromium.org/2667803002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
That is a valid point that has been raised before, but as Matt said, the optional string approach is part of the mojom for this service, and the Android implementation of this API already uses it. I made a bug for this (crbug.com/687376), but there will be a separate CL for fixing it. On 2017/01/31 18:12:16, sky wrote: > Shouldn't the outgoing one get an enum for the error and have it up to > the site using the api to map to a string if appropriate? > > On Mon, Jan 30, 2017 at 11:28 PM, <mailto:mgiuca@chromium.org> wrote: > > > > > https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/webshare... > > File chrome/browser/webshare/share_service_impl.cc (right): > > > > > https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/webshare... > > chrome/browser/webshare/share_service_impl.cc:156: > > callback.Run(base::Optional<std::string>("Share was cancelled")); > > On 2017/01/31 05:22:32, sky wrote: > >> Shouldn't this run the callback with no value and let the callback > > decide how to > >> interpret it? > > > > There are two different callbacks at play here with different semantics: > > the incoming one (which is given an Optional<string16>; previously got a > > SharePickerResult) gets nullopt if cancelled, or else the URL template. > > The outgoing one, |callback|, gets nullopt if successful or else an > > error message string. (That outgoing callback is defined by the mojom > > long ago.) > > > > So it makes sense here to be converting a nullopt (cancellation) into a > > non-null string (error message). > > > > > https://codereview.chromium.org/2667803002/diff/140001/chrome/browser/ui/brow... > > File chrome/browser/ui/browser_dialogs.h (right): > > > > > https://codereview.chromium.org/2667803002/diff/140001/chrome/browser/ui/brow... > > chrome/browser/ui/browser_dialogs.h:13: #include "base/optional.h" > > Need base/strings/string16 as well. > > > > https://codereview.chromium.org/2667803002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_dialogs.h:157: // titles that will be shown in a list. Calls |callback| with SHARE if an app On 2017/01/31 05:22:32, sky wrote: > Update description Done. https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:85: close_callback_.Run(GetText(table_->FirstSelectedRow(), 0)); On 2017/01/31 05:22:32, sky wrote: > What if there is no selected row? If there are no targets in the list, there will be no selected row, but for now, there is always a selected row, as there is always at least one target. (The first row is selected by default.) https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/webshare... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.h:39: const base::Callback<void(base::Optional<base::string16>)>& callback); On 2017/01/31 05:22:32, sky wrote: > Document |callback| Done. https://codereview.chromium.org/2667803002/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2667803002/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:13: #include "base/optional.h" On 2017/01/31 07:28:02, Matt Giuca wrote: > Need base/strings/string16 as well. Done.
https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:85: close_callback_.Run(GetText(table_->FirstSelectedRow(), 0)); On 2017/01/31 23:29:42, constantina wrote: > On 2017/01/31 05:22:32, sky wrote: > > What if there is no selected row? > > If there are no targets in the list, there will be no selected row, but for now, > there is always a selected row, as there is always at least one target. (The > first row is selected by default.) This is true now, but won't be true very soon (and this file shouldn't assume there will always be at least one row, because it isn't enforcing it). Please add a check for this case.
https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:85: close_callback_.Run(GetText(table_->FirstSelectedRow(), 0)); On 2017/01/31 23:48:27, Matt Giuca wrote: > On 2017/01/31 23:29:42, constantina wrote: > > On 2017/01/31 05:22:32, sky wrote: > > > What if there is no selected row? > > > > If there are no targets in the list, there will be no selected row, but for > now, > > there is always a selected row, as there is always at least one target. (The > > first row is selected by default.) > > This is true now, but won't be true very soon (and this file shouldn't assume > there will always be at least one row, because it isn't enforcing it). Please > add a check for this case. Not just that, but there is nothing stopping the user from clearing the selection.
https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:85: close_callback_.Run(GetText(table_->FirstSelectedRow(), 0)); On 2017/02/01 00:51:33, sky wrote: > On 2017/01/31 23:48:27, Matt Giuca wrote: > > On 2017/01/31 23:29:42, constantina wrote: > > > On 2017/01/31 05:22:32, sky wrote: > > > > What if there is no selected row? > > > > > > If there are no targets in the list, there will be no selected row, but for > > now, > > > there is always a selected row, as there is always at least one target. (The > > > first row is selected by default.) > > > > This is true now, but won't be true very soon (and this file shouldn't assume > > there will always be at least one row, because it isn't enforcing it). Please > > add a check for this case. > > Not just that, but there is nothing stopping the user from clearing the > selection. Done. If nothing is selected and user accepts, UI behaves as though share was cancelled.
The CQ bit was checked by constantina@google.com 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.
That's confusing. The accept key should only be enabled if there is a selected item, once the selection changes disable it. On Tue, Jan 31, 2017 at 5:06 PM, <constantina@google.com> wrote: > > https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc > (right): > > https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:85: > close_callback_.Run(GetText(table_->FirstSelectedRow(), 0)); > On 2017/02/01 00:51:33, sky wrote: >> On 2017/01/31 23:48:27, Matt Giuca wrote: >> > On 2017/01/31 23:29:42, constantina wrote: >> > > On 2017/01/31 05:22:32, sky wrote: >> > > > What if there is no selected row? >> > > >> > > If there are no targets in the list, there will be no selected > row, but for >> > now, >> > > there is always a selected row, as there is always at least one > target. (The >> > > first row is selected by default.) >> > >> > This is true now, but won't be true very soon (and this file > shouldn't assume >> > there will always be at least one row, because it isn't enforcing > it). Please >> > add a check for this case. >> >> Not just that, but there is nothing stopping the user from clearing > the >> selection. > > Done. If nothing is selected and user accepts, UI behaves as though > share was cancelled. > > https://codereview.chromium.org/2667803002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Done. On 2017/02/01 03:20:31, sky wrote: > That's confusing. The accept key should only be enabled if there is a > selected item, once the selection changes disable it. > > On Tue, Jan 31, 2017 at 5:06 PM, <mailto:constantina@google.com> wrote: > > > > > https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/views... > > File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc > > (right): > > > > > https://codereview.chromium.org/2667803002/diff/80001/chrome/browser/ui/views... > > chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:85: > > close_callback_.Run(GetText(table_->FirstSelectedRow(), 0)); > > On 2017/02/01 00:51:33, sky wrote: > >> On 2017/01/31 23:48:27, Matt Giuca wrote: > >> > On 2017/01/31 23:29:42, constantina wrote: > >> > > On 2017/01/31 05:22:32, sky wrote: > >> > > > What if there is no selected row? > >> > > > >> > > If there are no targets in the list, there will be no selected > > row, but for > >> > now, > >> > > there is always a selected row, as there is always at least one > > target. (The > >> > > first row is selected by default.) > >> > > >> > This is true now, but won't be true very soon (and this file > > shouldn't assume > >> > there will always be at least one row, because it isn't enforcing > > it). Please > >> > add a check for this case. > >> > >> Not just that, but there is nothing stopping the user from clearing > > the > >> selection. > > > > Done. If nothing is selected and user accepts, UI behaves as though > > share was cancelled. > > > > https://codereview.chromium.org/2667803002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
The CQ bit was checked by constantina@google.com 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/2667803002/diff/220001/chrome/browser/ui/brow... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:164: // |callback| with the manifest URL of the chosen target, and returns null if s/and/or. https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:83: bool WebShareTargetPickerView::Accept() { DCHECK(!table_->selection_model().empty()); to state explicitly that you expect the table to be non-empty (due to the Accept button being greyed out whenever it is empty). https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:100: if (button == ui::DIALOG_BUTTON_OK) Small comment here to explain why. https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/webshare/webshare_target_picker_view.h (right): https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/webshare/webshare_target_picker_view.h:30: // target, and returns null if the user cancelled the share. s/and/or https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:142: constexpr char kUrlBase[] = "https://wicg.github.io/web-share-target/demos/"; This makes it ugly: you are no longer displaying the title of the app, but rather the URL template. You will need either another CL (or just do it here, up to you) where you pass in BOTH a display name for use in the table, and a URL string to be returned. If you want to do it in a follow-up CL, add a TODO here (replace the above TODO since it's a bit out of date). https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:143: std::vector<base::string16> targets{base::ASCIIToUTF16(kUrlBase)}; Also I'm confused what the actual URL is that you're passing into (and getting back from) the dialog. The documentation (in 3 places) says "manifest URL", but this is the base URL. I think previously we discussed this being the full template with placeholders -- NOT the manifest URL or the base URL. (See my comment below.) Update the documentation to be consistent. https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:161: "sharetarget.html?title={title}&text={text}&url={url}"; This template part of the URL is still hard-coded here. That should be going away with this CL: you will keep the hard-coded string in Share(), for the time being, because we do not yet have data to populate the dialog with. But you should not have anything hard-coded in here because you should be getting back whatever site the user chose. In other words, if you need to hard-code anything here, it suggests the data model is wrong, because it won't be able to vary with the user's choice. I think the template should be concatenated onto the base URL before putting it into the vector, and then when the user makes a choice, you get that absolute template URL back as a result. https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:37: // target, and is null if the user cancelled the share. Virtual for testing. s/and/or :)
https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:171: std::string url_base = base::UTF16ToASCII(result.value()); UTF16ToUTF8 Otherwise this will fail if it contains non-ASCII characters. OK, it's a URL so "shouldn't" contain non-ASCII characters but I think they can do.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:143: std::vector<base::string16> targets{base::ASCIIToUTF16(kUrlBase)}; On 2017/02/01 07:04:33, Matt Giuca wrote: > Also I'm confused what the actual URL is that you're passing into (and getting > back from) the dialog. The documentation (in 3 places) says "manifest URL", but > this is the base URL. I think previously we discussed this being the full > template with placeholders -- NOT the manifest URL or the base URL. > > (See my comment below.) > > Update the documentation to be consistent. Had an offline chat: the summary is that this will all be sorted out by https://codereview.chromium.org/2664033002/ (where it *will* use the manifest URL to identify targets everywhere, which I think is nice). In the meantime, just change this CL to be consistent with that eventual plan, even though it does mean hard-coding the manifest URL here, and then (temporarily) ignoring the result of the dialog in OnPickerClosed and hard-coding the URL template.
https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/brow... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:164: // |callback| with the manifest URL of the chosen target, and returns null if On 2017/02/01 07:04:33, Matt Giuca wrote: > s/and/or. Done. https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:83: bool WebShareTargetPickerView::Accept() { On 2017/02/01 07:04:33, Matt Giuca wrote: > DCHECK(!table_->selection_model().empty()); > > to state explicitly that you expect the table to be non-empty (due to the Accept > button being greyed out whenever it is empty). Done. https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:100: if (button == ui::DIALOG_BUTTON_OK) On 2017/02/01 07:04:33, Matt Giuca wrote: > Small comment here to explain why. Done. https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/webshare/webshare_target_picker_view.h (right): https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/webshare/webshare_target_picker_view.h:30: // target, and returns null if the user cancelled the share. On 2017/02/01 07:04:33, Matt Giuca wrote: > s/and/or Done. https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:142: constexpr char kUrlBase[] = "https://wicg.github.io/web-share-target/demos/"; On 2017/02/01 07:04:33, Matt Giuca wrote: > This makes it ugly: you are no longer displaying the title of the app, but > rather the URL template. > > You will need either another CL (or just do it here, up to you) where you pass > in BOTH a display name for use in the table, and a URL string to be returned. If > you want to do it in a follow-up CL, add a TODO here (replace the above TODO > since it's a bit out of date). Done. https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:143: std::vector<base::string16> targets{base::ASCIIToUTF16(kUrlBase)}; On 2017/02/01 07:25:23, Matt Giuca wrote: > On 2017/02/01 07:04:33, Matt Giuca wrote: > > Also I'm confused what the actual URL is that you're passing into (and getting > > back from) the dialog. The documentation (in 3 places) says "manifest URL", > but > > this is the base URL. I think previously we discussed this being the full > > template with placeholders -- NOT the manifest URL or the base URL. > > > > (See my comment below.) > > > > Update the documentation to be consistent. > > Had an offline chat: the summary is that this will all be sorted out by > https://codereview.chromium.org/2664033002/ (where it *will* use the manifest > URL to identify targets everywhere, which I think is nice). > > In the meantime, just change this CL to be consistent with that eventual plan, > even though it does mean hard-coding the manifest URL here, and then > (temporarily) ignoring the result of the dialog in OnPickerClosed and > hard-coding the URL template. Done. https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:161: "sharetarget.html?title={title}&text={text}&url={url}"; On 2017/02/01 07:04:33, Matt Giuca wrote: > This template part of the URL is still hard-coded here. That should be going > away with this CL: you will keep the hard-coded string in Share(), for the time > being, because we do not yet have data to populate the dialog with. But you > should not have anything hard-coded in here because you should be getting back > whatever site the user chose. > > In other words, if you need to hard-code anything here, it suggests the data > model is wrong, because it won't be able to vary with the user's choice. > > I think the template should be concatenated onto the base URL before putting it > into the vector, and then when the user makes a choice, you get that absolute > template URL back as a result. Done as per your last comment. https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:171: std::string url_base = base::UTF16ToASCII(result.value()); On 2017/02/01 07:17:23, Matt Giuca wrote: > UTF16ToUTF8 > > Otherwise this will fail if it contains non-ASCII characters. OK, it's a URL so > "shouldn't" contain non-ASCII characters but I think they can do. Done. https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:37: // target, and is null if the user cancelled the share. Virtual for testing. On 2017/02/01 07:04:33, Matt Giuca wrote: > s/and/or :) Done :)
LGTM with a couple of nits. https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:100: if (button == ui::DIALOG_BUTTON_OK) On 2017/02/01 23:24:21, constantina wrote: > On 2017/02/01 07:04:33, Matt Giuca wrote: > > Small comment here to explain why. > > Done. how about "if there are no targets available". (Since the check is based on the size of the list, not whether there are selections.) https://codereview.chromium.org/2667803002/diff/240001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2667803002/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:145: // TODO(constantina): Pass vector of pairs of target names and manifest URls nit: "URL" not "URl".
https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:100: if (button == ui::DIALOG_BUTTON_OK) On 2017/02/02 00:07:40, Matt Giuca wrote: > On 2017/02/01 23:24:21, constantina wrote: > > On 2017/02/01 07:04:33, Matt Giuca wrote: > > > Small comment here to explain why. > > > > Done. > > how about "if there are no targets available". (Since the check is based on the > size of the list, not whether there are selections.) "!table_->selection_model().empty()" is for whether there is a selection. We discussed on hangouts to do one or the other based on whether it was dynamic or not, but since we can't work out how to deselect a target, I couldn't test it. This condition still works as we want it to for empty or non-empty lists though. https://codereview.chromium.org/2667803002/diff/240001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2667803002/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:145: // TODO(constantina): Pass vector of pairs of target names and manifest URls On 2017/02/02 00:07:40, Matt Giuca wrote: > nit: "URL" not "URl". Done.
On 2017/02/02 00:54:17, constantina wrote: > https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/view... > File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): > > https://codereview.chromium.org/2667803002/diff/220001/chrome/browser/ui/view... > chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:100: if (button > == ui::DIALOG_BUTTON_OK) > On 2017/02/02 00:07:40, Matt Giuca wrote: > > On 2017/02/01 23:24:21, constantina wrote: > > > On 2017/02/01 07:04:33, Matt Giuca wrote: > > > > Small comment here to explain why. > > > > > > Done. > > > > how about "if there are no targets available". (Since the check is based on > the > > size of the list, not whether there are selections.) > > "!table_->selection_model().empty()" is for whether there is a selection. We > discussed on hangouts to do one or the other based on whether it was dynamic or > not, but since we can't work out how to deselect a target, I couldn't test it. > This condition still works as we want it to for empty or non-empty lists though. > > https://codereview.chromium.org/2667803002/diff/240001/chrome/browser/webshar... > File chrome/browser/webshare/share_service_impl.cc (right): > > https://codereview.chromium.org/2667803002/diff/240001/chrome/browser/webshar... > chrome/browser/webshare/share_service_impl.cc:145: // TODO(constantina): Pass > vector of pairs of target names and manifest URls > On 2017/02/02 00:07:40, Matt Giuca wrote: > > nit: "URL" not "URl". > > Done. sky: Friendly ping?
Description was changed from ========== Picker UI passes the user chosen target to OnPickerClosed callback. Chosen target is Optional, so it can be null if the share is cancelled and the user doesn't pick anything. Also passes the default share target to the picker, which can then be chosen by the user passed to the callback. BUG=668389 ========== to ========== Picker takes a vector of pairs, and passes back the user chosen target. Instead of vector of string16s, input is a vector of pairs of string16s where the first element is the app name, and the second is the manifest URL of the app. The manifest URL of the chosen target is passed back via a callback. The passed URL is Optional, so it can be null if the share is cancelled. BUG=668389 ==========
Description was changed from ========== Picker takes a vector of pairs, and passes back the user chosen target. Instead of vector of string16s, input is a vector of pairs of string16s where the first element is the app name, and the second is the manifest URL of the app. The manifest URL of the chosen target is passed back via a callback. The passed URL is Optional, so it can be null if the share is cancelled. BUG=668389 ========== to ========== Picker takes a vector of pairs, and passes back the user chosen target. Instead of vector of string16s, input is a vector of pairs of string16 and GURL where the first element is the app name, and the second is the manifest URL of the app. The manifest URL of the chosen target is passed back via a callback as a string. The passed URL is Optional, so it can be null if the share is cancelled. BUG=668389 ==========
Changed some parameter types. See updated description for details, and in-code documentation.
slgtm with a few extra nits. https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/ui/brow... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:163: // title and manifest URL pairss that will be shown in a list. If the user picks nit: double s https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:87: close_callback_.Run(targets_[table_->FirstSelectedRow()].second.spec()); Good; it's much better to directly return the data that we got from the client rather than looking it up in the view. https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:115: return targets_[row].first + // Show "title (origin)", to disambiguate titles that are the same, and as a security measure. https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:141: // TODO(constantina): Replace hard-coded manifest URL with the list of name and manifest URL https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:143: constexpr char kTargetName[] = "Web Share Target Demo App"; s/Demo/Test (no point changing this line from what we had originally, and "Test" is the title of the page at https://wicg.github.io/web-share-target/demos/sharetarget.html). https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:43: base::Optional<std::string> picker_result_ = base::nullopt; base::nullopt isn't necessary here (it's the default constructor). https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:116: share_service_helper_->set_picker_result(base::nullopt); This isn't necessary any more as the dialog is cancelled by default. (Keep a comment around to that effect, because it is not obvious.)
Good: with the change to pass a vector of pairs into the UI, we are no longer degrading the experience pending a follow-up CL. (And there are no more CLs required on this UI in the immediate future!)
https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/ui/brow... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:163: // title and manifest URL pairss that will be shown in a list. If the user picks On 2017/02/03 01:44:45, Matt Giuca wrote: > nit: double s Done. https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:87: close_callback_.Run(targets_[table_->FirstSelectedRow()].second.spec()); On 2017/02/03 01:44:45, Matt Giuca wrote: > Good; it's much better to directly return the data that we got from the client > rather than looking it up in the view. Yee. Acknowledged. https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:115: return targets_[row].first + On 2017/02/03 01:44:45, Matt Giuca wrote: > // Show "title (origin)", to disambiguate titles that are the same, and as a > security measure. Done. https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:141: // TODO(constantina): Replace hard-coded manifest URL with the list of On 2017/02/03 01:44:45, Matt Giuca wrote: > name and manifest URL Done. https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:143: constexpr char kTargetName[] = "Web Share Target Demo App"; On 2017/02/03 01:44:45, Matt Giuca wrote: > s/Demo/Test (no point changing this line from what we had originally, and "Test" > is the title of the page at > https://wicg.github.io/web-share-target/demos/sharetarget.html). Done. https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:43: base::Optional<std::string> picker_result_ = base::nullopt; On 2017/02/03 01:44:45, Matt Giuca wrote: > base::nullopt isn't necessary here (it's the default constructor). Done. https://codereview.chromium.org/2667803002/diff/320001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:116: share_service_helper_->set_picker_result(base::nullopt); On 2017/02/03 01:44:45, Matt Giuca wrote: > This isn't necessary any more as the dialog is cancelled by default. > > (Keep a comment around to that effect, because it is not obvious.) Done.
https://codereview.chromium.org/2667803002/diff/360001/chrome/browser/ui/brow... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2667803002/diff/360001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:165: // or returns null if the user cancelled the share. By 'returns null' I think you mean supplies null? https://codereview.chromium.org/2667803002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2667803002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:105: return !table_->selection_model().empty(); You need respond to OnSelectionChanged() and trigger updating the button.
Also, please add tests of the view class.
Matt is doing the tests in this other CL: https://codereview.chromium.org/2679533002 https://codereview.chromium.org/2667803002/diff/360001/chrome/browser/ui/brow... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2667803002/diff/360001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:165: // or returns null if the user cancelled the share. On 2017/02/03 18:48:42, sky wrote: > By 'returns null' I think you mean supplies null? Done. https://codereview.chromium.org/2667803002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2667803002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:105: return !table_->selection_model().empty(); On 2017/02/03 18:48:42, sky wrote: > You need respond to OnSelectionChanged() and trigger updating the button. Done.
LGTM
On 2017/02/06 17:08:39, sky wrote: > LGTM Please include test coverage of selection as well.
Hi Scott, My plan yesterday was to have Connie rebase this CL onto my tests, but it looks like this CL is otherwise ready to go, and my test CL has a few issues (crashing on Windows). In the interest of time, can we just land this one in its current form (without tests) and I rebase my test CL onto it, and add tests for the selection changes? (Note that it is Connie's last week and I want to make sure all her outstanding CLs get landed.)
Land away. On Mon, Feb 6, 2017 at 4:10 PM, <mgiuca@chromium.org> wrote: > Hi Scott, > > My plan yesterday was to have Connie rebase this CL onto my tests, but it > looks > like this CL is otherwise ready to go, and my test CL has a few issues > (crashing > on Windows). > > In the interest of time, can we just land this one in its current form > (without > tests) and I rebase my test CL onto it, and add tests for the selection > changes? > (Note that it is Connie's last week and I want to make sure all her > outstanding > CLs get landed.) > > https://codereview.chromium.org/2667803002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/07 00:28:29, sky wrote: > Land away. Cheers! > > On Mon, Feb 6, 2017 at 4:10 PM, <mailto:mgiuca@chromium.org> wrote: > > Hi Scott, > > > > My plan yesterday was to have Connie rebase this CL onto my tests, but it > > looks > > like this CL is otherwise ready to go, and my test CL has a few issues > > (crashing > > on Windows). > > > > In the interest of time, can we just land this one in its current form > > (without > > tests) and I rebase my test CL onto it, and add tests for the selection > > changes? > > (Note that it is Connie's last week and I want to make sure all her > > outstanding > > CLs get landed.) > > > > https://codereview.chromium.org/2667803002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
The CQ bit was checked by constantina@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2667803002/#ps380001 (title: "Dynamically enable/disable button")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 380001, "attempt_start_ts": 1486428340736560,
"parent_rev": "3014682836a4792d50abfa27456fce6a2dceaa0d", "commit_rev":
"e4c513e7ae3fd3a6c7752f8add47b93fb1c66444"}
Message was sent while issue was closed.
Description was changed from ========== Picker takes a vector of pairs, and passes back the user chosen target. Instead of vector of string16s, input is a vector of pairs of string16 and GURL where the first element is the app name, and the second is the manifest URL of the app. The manifest URL of the chosen target is passed back via a callback as a string. The passed URL is Optional, so it can be null if the share is cancelled. BUG=668389 ========== to ========== Picker takes a vector of pairs, and passes back the user chosen target. Instead of vector of string16s, input is a vector of pairs of string16 and GURL where the first element is the app name, and the second is the manifest URL of the app. The manifest URL of the chosen target is passed back via a callback as a string. The passed URL is Optional, so it can be null if the share is cancelled. BUG=668389 Review-Url: https://codereview.chromium.org/2667803002 Cr-Commit-Position: refs/heads/master@{#448512} Committed: https://chromium.googlesource.com/chromium/src/+/e4c513e7ae3fd3a6c7752f8add47... ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001) as https://chromium.googlesource.com/chromium/src/+/e4c513e7ae3fd3a6c7752f8add47... |
