Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2664033002:
Read share targets from prefstore, and filter by engagement. (Closed)
|
Created:
3 years, 10 months ago by constantina Modified:
3 years, 10 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, tfarina, extensions-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRead share targets from prefstore, and filter by engagement.
New function, GetTargetsWithSufficientEngagement, called when browser
receives Share from renderer. Function collates all share targets and
their templates, and keeps targets that have a high enough engagement
score with the user.
Calls picker UI with this filtered list of targets.
BUG=668389
Review-Url: https://codereview.chromium.org/2664033002
Cr-Commit-Position: refs/heads/master@{#449911}
Committed: https://chromium.googlesource.com/chromium/src/+/5558f3f32e3182a29ef236242e2c50c37a3f6a1a
Patch Set 1 #Patch Set 2 : Adding git branch dependency #Patch Set 3 : Replace placeholders after picker, and add tests #Patch Set 4 : Remove duplicate function #
Total comments: 33
Patch Set 5 : Fixed, as per feedback #Patch Set 6 : In tests, check targets shown in picker are what they should be. #Patch Set 7 : Add test for insufficiently engaged targets #Patch Set 8 : Test for some engaged targets and some not #Patch Set 9 : Removed const #
Total comments: 43
Patch Set 10 : Rebase #Patch Set 11 : Rebase again #Patch Set 12 : Read target name from prefs #Patch Set 13 : Fixed tests to conform with changes in last patch #Patch Set 14 : Fixed, as per feedback #
Total comments: 12
Patch Set 15 : Fixed, as per feedback #
Total comments: 5
Patch Set 16 : Pass share_targets around, instead of storing as member variable. #
Total comments: 2
Patch Set 17 : Pass object to func, not unique_ptr. #
Total comments: 2
Patch Set 18 : Remove space. #Patch Set 19 : Create explicit GURL #Patch Set 20 : Don't define Browser references in Android #
Total comments: 2
Patch Set 21 : Undeclared var on android now declared #Patch Set 22 : Don't build tests on Android #
Depends on Patchset: Messages
Total messages: 62 (26 generated)
constantina@google.com changed reviewers: + mgiuca@chromium.org, sammc@google.com
More extensive tests need to be done, I think, but PTAL.
Cool, here are some initial comments but I will look again when tests are done. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:138: chrome::AddTabAt(GetBrowser(), target_url, You should still assign this to a Browser* local variable, to avoid calling GetLastActive() twice. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:172: std::vector<std::string> most_engaged_targets; Not really the right name (it implies that you'll pick the top N targets, whereas you actually pick all targets with sufficient engagement. sufficiently_engaged_targets? https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:174: for (base::DictionaryValue::Iterator it(*dict); !it.IsAtEnd(); it.Advance()) { auto? Also, what a strange iterator protocol! https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:175: GURL manifest_url(it.key()); This can probably be a const GURL& yes? https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:177: blink::mojom::EngagementLevel::LOW) { Can this be a constant (e.g., kMinimumEngagementLevel) at the top of the file in an anonymous namespace? (This is kind of my opinion; others would probably tell you to keep it here, but I like for arbitrary tunable constants to be in a more visible spot.) https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:189: std::vector<std::string> most_engaged_targets = Same name as previous function. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:194: targets.push_back(base::ASCIIToUTF16(target)); UTF8ToUTF16 You should never use ASCIIToUTF16 unless it is a hard-coded ASCII literal. (And generally never use UTF16ToASCII, always UTF16ToUTF8.) This will fail (via DCHECK -- i.e., in release simply misbehave) if there are UTF-8 characters in it. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:212: std::string chosen_target = base::UTF16ToASCII(result.value()); UTF16ToUTF8 https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:214: std::string url_template = GetTargetTemplate(chosen_target); I'm a big fan of this approach versus what we discussed the other day: to use the manifest URL as the unique identifier of a share target everywhere (up to and including OnPickerClosed) and then doing a lookup here to get the URL template. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:219: base::Optional<std::string>("Error: placeholders in share target's url " Is there a reason to change this string? https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:224: base::StringPiece url_base( // The template is relative to the manifest URL (minus the filename). Concatenate to make an absolute URL. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.h:39: std::string GetTargetTemplate(std::string target_url); const std::string& (Generally, all non-primitive arguments in C++ should be const X& unless you have a good reason.) https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.h:43: virtual blink::mojom::EngagementLevel GetEngagementLevel(GURL url); const GURL& https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl_unittest.cc:84: // TODO check targets were what was expected. Should this be done?
https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:138: chrome::AddTabAt(GetBrowser(), target_url, On 2017/02/01 07:30:20, Matt Giuca wrote: > You should still assign this to a Browser* local variable, to avoid calling > GetLastActive() twice. Done. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:172: std::vector<std::string> most_engaged_targets; On 2017/02/01 07:30:20, Matt Giuca wrote: > Not really the right name (it implies that you'll pick the top N targets, > whereas you actually pick all targets with sufficient engagement. > > sufficiently_engaged_targets? Done. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:174: for (base::DictionaryValue::Iterator it(*dict); !it.IsAtEnd(); it.Advance()) { On 2017/02/01 07:30:20, Matt Giuca wrote: > auto? > > Also, what a strange iterator protocol! Compiler cries. Ikr! https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:175: GURL manifest_url(it.key()); On 2017/02/01 07:30:20, Matt Giuca wrote: > This can probably be a const GURL& yes? Done. Just const, no "&". The key is a string, not a GURL, so I think that's why "&" won't work. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:177: blink::mojom::EngagementLevel::LOW) { On 2017/02/01 07:30:20, Matt Giuca wrote: > Can this be a constant (e.g., kMinimumEngagementLevel) at the top of the file in > an anonymous namespace? > > (This is kind of my opinion; others would probably tell you to keep it here, but > I like for arbitrary tunable constants to be in a more visible spot.) I moved it to the top of the function. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:189: std::vector<std::string> most_engaged_targets = On 2017/02/01 07:30:20, Matt Giuca wrote: > Same name as previous function. Done. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:194: targets.push_back(base::ASCIIToUTF16(target)); On 2017/02/01 07:30:20, Matt Giuca wrote: > UTF8ToUTF16 > > You should never use ASCIIToUTF16 unless it is a hard-coded ASCII literal. (And > generally never use UTF16ToASCII, always UTF16ToUTF8.) This will fail (via > DCHECK -- i.e., in release simply misbehave) if there are UTF-8 characters in > it. Done. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:212: std::string chosen_target = base::UTF16ToASCII(result.value()); On 2017/02/01 07:30:20, Matt Giuca wrote: > UTF16ToUTF8 Done. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:214: std::string url_template = GetTargetTemplate(chosen_target); On 2017/02/01 07:30:20, Matt Giuca wrote: > I'm a big fan of this approach versus what we discussed the other day: to use > the manifest URL as the unique identifier of a share target everywhere (up to > and including OnPickerClosed) and then doing a lookup here to get the URL > template. :D It has turned out to be quite neat; I like that it doesn't give unnecessary info to the picker. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:219: base::Optional<std::string>("Error: placeholders in share target's url " On 2017/02/01 07:30:20, Matt Giuca wrote: > Is there a reason to change this string? No, I just lost it in a rebase, wrote something similar for temporary testing purposes, and forgot to check what it actually used to be. Changed back. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:224: base::StringPiece url_base( On 2017/02/01 07:30:20, Matt Giuca wrote: > // The template is relative to the manifest URL (minus the filename). > Concatenate to make an absolute URL. Done. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.h:39: std::string GetTargetTemplate(std::string target_url); On 2017/02/01 07:30:20, Matt Giuca wrote: > const std::string& > > (Generally, all non-primitive arguments in C++ should be const X& unless you > have a good reason.) Done. I know, but I keep forgetting to do it, because I'm not yet used to it :) https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.h:43: virtual blink::mojom::EngagementLevel GetEngagementLevel(GURL url); On 2017/02/01 07:30:20, Matt Giuca wrote: > const GURL& Done. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl_unittest.cc:84: // TODO check targets were what was expected. On 2017/02/01 07:30:20, Matt Giuca wrote: > Should this be done? Yep, will do in next patch.
https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:174: for (base::DictionaryValue::Iterator it(*dict); !it.IsAtEnd(); it.Advance()) { On 2017/02/02 00:43:48, constantina wrote: > On 2017/02/01 07:30:20, Matt Giuca wrote: > > auto? > > > > Also, what a strange iterator protocol! > > Compiler cries. > > Ikr! Oh, right, you can't call auto because you're actually calling the Iterator constructor, as opposed to some kind of begin() method on the dict itself. (And this is the only way.) That's... pretty junk. Acknowledged. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:175: GURL manifest_url(it.key()); On 2017/02/02 00:43:47, constantina wrote: > On 2017/02/01 07:30:20, Matt Giuca wrote: > > This can probably be a const GURL& yes? > > Done. Just const, no "&". The key is a string, not a GURL, so I think that's why > "&" won't work. Oh OK, then could just remove the const as well (I see much less value in having a const value rather than a const ref). But you can also keep const as well; up to you. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:177: blink::mojom::EngagementLevel::LOW) { On 2017/02/02 00:43:47, constantina wrote: > On 2017/02/01 07:30:20, Matt Giuca wrote: > > Can this be a constant (e.g., kMinimumEngagementLevel) at the top of the file > in > > an anonymous namespace? > > > > (This is kind of my opinion; others would probably tell you to keep it here, > but > > I like for arbitrary tunable constants to be in a more visible spot.) > > I moved it to the top of the function. Acknowledged. https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.h:39: std::string GetTargetTemplate(std::string target_url); On 2017/02/02 00:43:48, constantina wrote: > On 2017/02/01 07:30:20, Matt Giuca wrote: > > const std::string& > > > > (Generally, all non-primitive arguments in C++ should be const X& unless you > > have a good reason.) > > Done. I know, but I keep forgetting to do it, because I'm not yet used to it :) Yep, that's fine. It's quite unintuitive.
https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2664033002/diff/60001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl.cc:175: GURL manifest_url(it.key()); On 2017/02/02 02:57:10, Matt Giuca wrote: > On 2017/02/02 00:43:47, constantina wrote: > > On 2017/02/01 07:30:20, Matt Giuca wrote: > > > This can probably be a const GURL& yes? > > > > Done. Just const, no "&". The key is a string, not a GURL, so I think that's > why > > "&" won't work. > > Oh OK, then could just remove the const as well (I see much less value in having > a const value rather than a const ref). But you can also keep const as well; up > to you. Removed
Added more tests, too.
https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:197: for (const std::string& target : sufficiently_engaged_targets) { Can be const auto& https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:216: std::string chosen_target = base::UTF16ToUTF8(result.value()); As discussed, conversion from UTF-8 to UTF-16 (in Share), then back to UTF-8 again (here) is not ideal. In the previous CL we discussed changing to always keep the manifest URL as a GURL (and therefore UTF-8 inside), and returned as a GURL. Then you won't need these conversions. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:39: std::string GetTargetTemplate(const std::string& target_url); Document this. Separate out these functions by spaces. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:43: virtual blink::mojom::EngagementLevel GetEngagementLevel(const GURL& url); Properly document this. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:30: static ShareServiceTestImpl* Create( Just noticed this (not directly applicable to this CL but already landed code)... what is the lifetime of the ShareServiceTestImpl? It creates a strong binding but does it ever get cleaned up anywhere? I'm not really familiar with the lifetime of a strong binding (is it actually needed in the test?). Do you have to call Close() manually in the TearDown? https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:44: void SetUpPrefs() { This can just go in the constructor (since you call it immediately after calling the ctor anyway). https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:50: void AddShareTarget(std::string manifest_url, std::string url_template) { const std::string& https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:65: void AddEngagementForTarget(std::string manifest_url, How about SetEngagementForTarget (rather than Add). Also const std::string& https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:70: void ClearEngagements() { engagement_map.clear(); } This isn't needed (since the whole ShareServiceTestImpl is created on each SetUp, you don't need to clear this on TearDown). https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:79: base::Optional<base::string16> picker_result_ = base::nullopt; base::nullopt not needed (it's the default) ... sorry this is from another CL but a quick fix. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:82: std::map<std::string, blink::mojom::EngagementLevel> engagement_map; engagement_map_ https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:122: ChromeRenderViewHostTestHarness::TearDown(); Is this override needed at all? (Before this CL) it was just calling its base class TearDown(). https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:123: share_service_helper_->ClearEngagements(); As stated above, this isn't needed because the entire share_service_helper_ is destroyed and recreated on each SetUp. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:127: const std::vector<base::string16>& expected_targets_in_picker, Little nit pick: could this argument go first? Just because the logical order in my mind is: 1. The targets go into the picker (check that), 2. The user picks a target, 3. We get back a picked URL (check that), 4. We return a mojo result (param) (check that). Also do the checks in the body of this function in that same order (first check targets, then URL, then param). Also (carry-over from a previous CL)... why is this called param? Isn't it the response from the Mojo service? In the mojom, it's called "error" not "param". Could you rename this to expected_error and error, for clarity? https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:135: EXPECT_EQ(expected_targets_in_picker, targets_in_picker); Good test that the targets actually show up in the dialog! https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:152: "https://wicg.github.io/web-share-target/demos/manifest.json"; Now that we are (as of this CL) no longer hard-coding the github sites at all, I think it makes sense to change these tests to fictional URLs (since usually tests won't use real-world data). You can make these templates and strings a bit simpler then. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:161: share_service_helper_->AddShareTarget(manifest_url, url_template); Perhaps add a second target here (since none of the tests currently test >1 target actually showing up in the dialog). Just copy the cpyro target from ShareWithSomeInsuffientlyEngagedTargets, but change the target engagements: one should be LOW but the other should be HIGH as a quick test that highly-engaged targets also show up. (Hmm... if you consider all my advice in this file, I'm basically asking you to have the exact same two targets in 3 tests, which suggests maybe this should be done in the SetUp, except for that pesky ShareCancel test that doesn't want any targets. At least move some of the constants like manifest URLs into top-level constants.) https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:180: TEST_F(ShareServiceImplUnittest, ShareCancel) { Add a test where there are 2 sufficiently engaged targets but the user chooses to cancel. (Right now, the only cancellation tests are those where there are no targets in the dialog.) (So there should be two cancellation tests: one with 0 targets and one with 2 targets.) https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:201: TEST_F(ShareServiceImplUnittest, ShareWithInsuffientlyEngagedTargets) { I don't think it's worth having this test. It essentially covers the intersection of ShareCancel and ShareWithSomeInsuffientlyEngagedTargets; that logic is already covered by those two tests. (Especially since I asked you above to add yet another test of cancellation when there are actual targets to choose from.) https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:227: TEST_F(ShareServiceImplUnittest, ShareWithSomeInsuffientlyEngagedTargets) { Insufficiently
Description was changed from ========== Read share targets from prefstore, and filter by engagement. New function, GetTargetsWithSufficientEngagement, called when browser receives Share from renderer. Function collates all share targets and their templates, and keeps targets that have a high enough engagement score with the user. Then replaces placeholders in the templates, and removes targets that fail replacement, or result in invalid URLs. Calls picker UI with this filtered list of targets. BUG=668389 ========== to ========== Read share targets from prefstore, and filter by engagement. New function, GetTargetsWithSufficientEngagement, called when browser receives Share from renderer. Function collates all share targets and their templates, and keeps targets that have a high enough engagement score with the user. Calls picker UI with this filtered list of targets. BUG=668389 ==========
Rebased on a few landed CLs since last mail. PTAL. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:197: for (const std::string& target : sufficiently_engaged_targets) { On 2017/02/03 02:40:23, Matt Giuca wrote: > Can be const auto& Gone. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:216: std::string chosen_target = base::UTF16ToUTF8(result.value()); On 2017/02/03 02:40:23, Matt Giuca wrote: > As discussed, conversion from UTF-8 to UTF-16 (in Share), then back to UTF-8 > again (here) is not ideal. > > In the previous CL we discussed changing to always keep the manifest URL as a > GURL (and therefore UTF-8 inside), and returned as a GURL. Then you won't need > these conversions. Gone. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:39: std::string GetTargetTemplate(const std::string& target_url); On 2017/02/03 02:40:24, Matt Giuca wrote: > Document this. > > Separate out these functions by spaces. Done. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:43: virtual blink::mojom::EngagementLevel GetEngagementLevel(const GURL& url); On 2017/02/03 02:40:24, Matt Giuca wrote: > Properly document this. Done. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:30: static ShareServiceTestImpl* Create( On 2017/02/03 02:40:24, Matt Giuca wrote: > Just noticed this (not directly applicable to this CL but already landed > code)... what is the lifetime of the ShareServiceTestImpl? > > It creates a strong binding but does it ever get cleaned up anywhere? I'm not > really familiar with the lifetime of a strong binding (is it actually needed in > the test?). Do you have to call Close() manually in the TearDown? I am not sure. I will have to ask Sam or someone else. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:44: void SetUpPrefs() { On 2017/02/03 02:40:24, Matt Giuca wrote: > This can just go in the constructor (since you call it immediately after calling > the ctor anyway). Done. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:50: void AddShareTarget(std::string manifest_url, std::string url_template) { On 2017/02/03 02:40:24, Matt Giuca wrote: > const std::string& Done. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:65: void AddEngagementForTarget(std::string manifest_url, On 2017/02/03 02:40:24, Matt Giuca wrote: > How about SetEngagementForTarget (rather than Add). > > Also const std::string& Done. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:70: void ClearEngagements() { engagement_map.clear(); } On 2017/02/03 02:40:24, Matt Giuca wrote: > This isn't needed (since the whole ShareServiceTestImpl is created on each > SetUp, you don't need to clear this on TearDown). Done. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:79: base::Optional<base::string16> picker_result_ = base::nullopt; On 2017/02/03 02:40:24, Matt Giuca wrote: > base::nullopt not needed (it's the default) ... sorry this is from another CL > but a quick fix. Done. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:82: std::map<std::string, blink::mojom::EngagementLevel> engagement_map; On 2017/02/03 02:40:24, Matt Giuca wrote: > engagement_map_ Done. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:90: if (picker_result_ != base::nullopt) { Don't need this anymore; not testing ShareServiceImpl implementation. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:122: ChromeRenderViewHostTestHarness::TearDown(); On 2017/02/03 02:40:24, Matt Giuca wrote: > Is this override needed at all? (Before this CL) it was just calling its base > class TearDown(). I think we worked out when we first wrote the tests that we had to call the base class constructor ourselves, and it doesn't work otherwise. Can confirm now it doesn't work if I remove. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:123: share_service_helper_->ClearEngagements(); On 2017/02/03 02:40:24, Matt Giuca wrote: > As stated above, this isn't needed because the entire share_service_helper_ is > destroyed and recreated on each SetUp. Done. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:127: const std::vector<base::string16>& expected_targets_in_picker, On 2017/02/03 02:40:24, Matt Giuca wrote: > Little nit pick: could this argument go first? Just because the logical order in > my mind is: > 1. The targets go into the picker (check that), > 2. The user picks a target, > 3. We get back a picked URL (check that), > 4. We return a mojo result (param) (check that). > > Also do the checks in the body of this function in that same order (first check > targets, then URL, then param). > > Also (carry-over from a previous CL)... why is this called param? Isn't it the > response from the Mojo service? In the mojom, it's called "error" not "param". > Could you rename this to expected_error and error, for clarity? Done. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:135: EXPECT_EQ(expected_targets_in_picker, targets_in_picker); On 2017/02/03 02:40:24, Matt Giuca wrote: > Good test that the targets actually show up in the dialog! :D https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:152: "https://wicg.github.io/web-share-target/demos/manifest.json"; On 2017/02/03 02:40:24, Matt Giuca wrote: > Now that we are (as of this CL) no longer hard-coding the github sites at all, I > think it makes sense to change these tests to fictional URLs (since usually > tests won't use real-world data). > > You can make these templates and strings a bit simpler then. Done. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:161: share_service_helper_->AddShareTarget(manifest_url, url_template); On 2017/02/03 02:40:24, Matt Giuca wrote: > Perhaps add a second target here (since none of the tests currently test >1 > target actually showing up in the dialog). > > Just copy the cpyro target from ShareWithSomeInsuffientlyEngagedTargets, but > change the target engagements: one should be LOW but the other should be HIGH as > a quick test that highly-engaged targets also show up. > > (Hmm... if you consider all my advice in this file, I'm basically asking you to > have the exact same two targets in 3 tests, which suggests maybe this should be > done in the SetUp, except for that pesky ShareCancel test that doesn't want any > targets. At least move some of the constants like manifest URLs into top-level > constants.) Done. Did a bit of a refactor. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:180: TEST_F(ShareServiceImplUnittest, ShareCancel) { On 2017/02/03 02:40:24, Matt Giuca wrote: > Add a test where there are 2 sufficiently engaged targets but the user chooses > to cancel. (Right now, the only cancellation tests are those where there are no > targets in the dialog.) > > (So there should be two cancellation tests: one with 0 targets and one with 2 > targets.) Done. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:201: TEST_F(ShareServiceImplUnittest, ShareWithInsuffientlyEngagedTargets) { On 2017/02/03 02:40:24, Matt Giuca wrote: > I don't think it's worth having this test. It essentially covers the > intersection of ShareCancel and ShareWithSomeInsuffientlyEngagedTargets; that > logic is already covered by those two tests. > > (Especially since I asked you above to add yet another test of cancellation when > there are actual targets to choose from.) Removed. https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:227: TEST_F(ShareServiceImplUnittest, ShareWithSomeInsuffientlyEngagedTargets) { On 2017/02/03 02:40:24, Matt Giuca wrote: > Insufficiently Done.
sammc@chromium.org changed reviewers: + sammc@chromium.org - sammc@google.com
https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:30: static ShareServiceTestImpl* Create( On 2017/02/08 23:25:16, constantina wrote: > On 2017/02/03 02:40:24, Matt Giuca wrote: > > Just noticed this (not directly applicable to this CL but already landed > > code)... what is the lifetime of the ShareServiceTestImpl? > > > > It creates a strong binding but does it ever get cleaned up anywhere? I'm not > > really familiar with the lifetime of a strong binding (is it actually needed > in > > the test?). Do you have to call Close() manually in the TearDown? > > I am not sure. I will have to ask Sam or someone else. A StrongBinding ties the lifetime of the object to the mojo connection. In a same-thread case like this, it'll be deleted when the message loop runs after the InterfacePtr owning this closes. Using a mojo::Binding and owning the implementation directly from the test would probably be easier to understand.
Great! lgtm with a few nits. Sam did you want to do a deeper review or are we good? https://codereview.chromium.org/2664033002/diff/250001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2664033002/diff/250001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:59: void AddShareTarget(const std::string& manifest_url, nit: Maybe call this AddShareTargetToPrefs. https://codereview.chromium.org/2664033002/diff/250001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:242: TEST_F(ShareServiceImplUnittest, ShareWithSomeInsuffientlyEngagedTargets) { s/Insuffiently/Insufficiently !
https://codereview.chromium.org/2664033002/diff/250001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2664033002/diff/250001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:132: return BrowserList::GetInstance()->GetLastActive(); This could be the wrong Browser. Instead, ShareServiceImpl should be constructed with a WebContents and use that to look up the Browser and get the Profile/BrowserContext. https://codereview.chromium.org/2664033002/diff/250001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:149: share_target_dict->GetDictionaryWithoutPathExpansion(target_url, Can this fail? Could a share target be removed while the picker is shown? https://codereview.chromium.org/2664033002/diff/250001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:171: const blink::mojom::EngagementLevel kMinimumEngagementLevel = constexpr https://codereview.chromium.org/2664033002/diff/250001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2664033002/diff/250001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:97: void ShowPickerDialog( Methods before fields.
https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2664033002/diff/150001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:30: static ShareServiceTestImpl* Create( On 2017/02/08 23:34:46, Sam McNally wrote: > On 2017/02/08 23:25:16, constantina wrote: > > On 2017/02/03 02:40:24, Matt Giuca wrote: > > > Just noticed this (not directly applicable to this CL but already landed > > > code)... what is the lifetime of the ShareServiceTestImpl? > > > > > > It creates a strong binding but does it ever get cleaned up anywhere? I'm > not > > > really familiar with the lifetime of a strong binding (is it actually needed > > in > > > the test?). Do you have to call Close() manually in the TearDown? > > > > I am not sure. I will have to ask Sam or someone else. > > A StrongBinding ties the lifetime of the object to the mojo connection. In a > same-thread case like this, it'll be deleted when the message loop runs after > the InterfacePtr owning this closes. > > Using a mojo::Binding and owning the implementation directly from the test would > probably be easier to understand. Done. https://codereview.chromium.org/2664033002/diff/250001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2664033002/diff/250001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:132: return BrowserList::GetInstance()->GetLastActive(); On 2017/02/09 02:10:39, Sam McNally wrote: > This could be the wrong Browser. Instead, ShareServiceImpl should be constructed > with a WebContents and use that to look up the Browser and get the > Profile/BrowserContext. Okay, can I do this in another CL? This was already used before this CL, and I just moved it into this function. https://codereview.chromium.org/2664033002/diff/250001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:149: share_target_dict->GetDictionaryWithoutPathExpansion(target_url, On 2017/02/09 02:10:39, Sam McNally wrote: > Can this fail? Could a share target be removed while the picker is shown? Yes, it can, if the site's manifest is changed and redownloaded between pref reads. I've now created a copy of the target prefs when I first read them, and stored the copy as a member variable. The URL may not be handled by the target site, if it's no longer a target, but there is no guarantee on that if it is a target anyway (could declare template but not handle it). https://codereview.chromium.org/2664033002/diff/250001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:171: const blink::mojom::EngagementLevel kMinimumEngagementLevel = On 2017/02/09 02:10:39, Sam McNally wrote: > constexpr Done. https://codereview.chromium.org/2664033002/diff/250001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2664033002/diff/250001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:59: void AddShareTarget(const std::string& manifest_url, On 2017/02/09 00:24:27, Matt Giuca wrote: > nit: Maybe call this AddShareTargetToPrefs. Done. https://codereview.chromium.org/2664033002/diff/250001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:97: void ShowPickerDialog( On 2017/02/09 02:10:39, Sam McNally wrote: > Methods before fields. Done. https://codereview.chromium.org/2664033002/diff/250001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:242: TEST_F(ShareServiceImplUnittest, ShareWithSomeInsuffientlyEngagedTargets) { On 2017/02/09 00:24:27, Matt Giuca wrote: > s/Insuffiently/Insufficiently ! Oops! Done!
lgtm
https://codereview.chromium.org/2664033002/diff/270001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2664033002/diff/270001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:174: share_targets = GetPrefService() Move this line into ::Share (so it isn't a side-effect of GTWSE). https://codereview.chromium.org/2664033002/diff/270001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2664033002/diff/270001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:91: std::unique_ptr<base::DictionaryValue> share_targets; share_targets_
https://codereview.chromium.org/2664033002/diff/270001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2664033002/diff/270001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:91: std::unique_ptr<base::DictionaryValue> share_targets; On 2017/02/09 05:47:16, Matt Giuca wrote: > share_targets_ Actually, remove this and just make it a temporary value you pass around. (Create it in Share, pass it as an argument to GetTargetsWithSufficientEngagement and OnPickerClosed, binding it into the latter.)
https://codereview.chromium.org/2664033002/diff/270001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2664033002/diff/270001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:174: share_targets = GetPrefService() On 2017/02/09 05:47:16, Matt Giuca wrote: > Move this line into ::Share (so it isn't a side-effect of GTWSE). Done. https://codereview.chromium.org/2664033002/diff/270001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2664033002/diff/270001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:91: std::unique_ptr<base::DictionaryValue> share_targets; On 2017/02/09 23:21:09, Matt Giuca wrote: > On 2017/02/09 05:47:16, Matt Giuca wrote: > > share_targets_ > > Actually, remove this and just make it a temporary value you pass around. > > (Create it in Share, pass it as an argument to > GetTargetsWithSufficientEngagement and OnPickerClosed, binding it into the > latter.) Done.
https://codereview.chromium.org/2664033002/diff/290001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2664033002/diff/290001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:45: std::unique_ptr<base::DictionaryValue> share_targets); This doesn't need to take ownership. Just take a const base::DictionaryValue& (and let OnPickerClosed keep ownership of the dictionary).
https://codereview.chromium.org/2664033002/diff/290001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2664033002/diff/290001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:45: std::unique_ptr<base::DictionaryValue> share_targets); On 2017/02/10 00:56:32, Matt Giuca wrote: > This doesn't need to take ownership. Just take a const base::DictionaryValue& > (and let OnPickerClosed keep ownership of the dictionary). Done.
lgtm with git cl format applied. https://codereview.chromium.org/2664033002/diff/310001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2664033002/diff/310001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:152: &share_target_info_dict); nit: Remove one space from this line (clang format or just git cl format).
https://codereview.chromium.org/2664033002/diff/310001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2664033002/diff/310001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:152: &share_target_info_dict); On 2017/02/10 02:16:50, Matt Giuca wrote: > nit: Remove one space from this line (clang format or just git cl format). Done!
The CQ bit was checked by constantina@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2664033002/#ps330001 (title: "Remove space.")
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
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by constantina@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2664033002/#ps350001 (title: "Create explicit GURL")
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
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by constantina@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2664033002/#ps370001 (title: "Don't define Browser references in Android")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2664033002/diff/370001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2664033002/diff/370001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:223: #endif I think this will fail because share_targets isn't defined on Android. Declare the variable outside the if statement. Also #else return (so that this code won't simply crash if used on another platform).
The CQ bit was unchecked by constantina@google.com
https://codereview.chromium.org/2664033002/diff/370001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2664033002/diff/370001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:223: #endif On 2017/02/10 04:35:19, Matt Giuca wrote: > I think this will fail because share_targets isn't defined on Android. > > Declare the variable outside the if statement. > > Also #else return (so that this code won't simply crash if used on another > platform). Done.
The CQ bit was checked by constantina@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2664033002/#ps390001 (title: "Undeclared var on android now declared")
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by constantina@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2664033002/#ps410001 (title: "Don't build tests on Android")
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
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
On 2017/02/10 07:13:01, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) D'ohhhhhhhhhhhhh ../../chrome/browser/webshare/share_service_impl_unittest.cc:28:16: error: unused variable 'kTargetName' [-Werror,-Wunused-const-variable] constexpr char kTargetName[] = "Share Target"; ^ ../../chrome/browser/webshare/share_service_impl_unittest.cc:29:16: error: unused variable 'kUrlTemplate' [-Werror,-Wunused-const-variable] constexpr char kUrlTemplate[] = "share?title={title}&text={text}&url={url}"; ^ I'll fix and land on Monday :(
On 2017/02/10 10:04:48, Matt Giuca wrote: > On 2017/02/10 07:13:01, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > > D'ohhhhhhhhhhhhh > > ../../chrome/browser/webshare/share_service_impl_unittest.cc:28:16: error: > unused variable 'kTargetName' [-Werror,-Wunused-const-variable] > constexpr char kTargetName[] = "Share Target"; > ^ > ../../chrome/browser/webshare/share_service_impl_unittest.cc:29:16: error: > unused variable 'kUrlTemplate' [-Werror,-Wunused-const-variable] > constexpr char kUrlTemplate[] = "share?title={title}&text={text}&url={url}"; > ^ > > I'll fix and land on Monday :( I just landed https://crrev.com/449905 which disables these files on Android and Mac. This CL should be able to land now without changes.
The CQ bit was checked by mgiuca@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 mgiuca@chromium.org
The CQ bit was checked by mgiuca@chromium.org
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 mgiuca@chromium.org
The CQ bit was checked by mgiuca@chromium.org
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": 410001, "attempt_start_ts": 1486963656244490,
"parent_rev": "1f37ab2e5576e8ce57c0eb58e412c65f0b66021b", "commit_rev":
"5558f3f32e3182a29ef236242e2c50c37a3f6a1a"}
Message was sent while issue was closed.
Description was changed from ========== Read share targets from prefstore, and filter by engagement. New function, GetTargetsWithSufficientEngagement, called when browser receives Share from renderer. Function collates all share targets and their templates, and keeps targets that have a high enough engagement score with the user. Calls picker UI with this filtered list of targets. BUG=668389 ========== to ========== Read share targets from prefstore, and filter by engagement. New function, GetTargetsWithSufficientEngagement, called when browser receives Share from renderer. Function collates all share targets and their templates, and keeps targets that have a high enough engagement score with the user. Calls picker UI with this filtered list of targets. BUG=668389 Review-Url: https://codereview.chromium.org/2664033002 Cr-Commit-Position: refs/heads/master@{#449911} Committed: https://chromium.googlesource.com/chromium/src/+/5558f3f32e3182a29ef236242e2c... ==========
Message was sent while issue was closed.
Committed patchset #22 (id:410001) as https://chromium.googlesource.com/chromium/src/+/5558f3f32e3182a29ef236242e2c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
