Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2639463002:
Add a pref name for share targets, and store their manifest data. (Closed)
|
Created:
3 years, 11 months ago by constantina Modified:
3 years, 10 months ago CC:
chrome-apps-syd-reviews_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, extensions-reviews_chromium.org, jam, mlamouri+watch-content_chromium.org, mlamouri+watch-manifest_chromium.org, mlamouri (slow - plz ping) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a pref name for share targets, and store their manifest data.
Adds a dictionary pref name, to store sites with manifests that
contain the share_url_template field; the key is the site origin, and
value is the value of share_url_template in the site's manifest.
Reads manifests after they have been parsed, for the share_url_template,
and stores the site and value under the pref name added.
Follow-up CL will read the pref store for the pref name added, and read
share targets in chrome/browser/webshare/share_service_impl.cc.
NO_DEPENDENCY_CHECKS=true
BUG=668389
Review-Url: https://codereview.chromium.org/2639463002
Cr-Commit-Position: refs/heads/master@{#447721}
Committed: https://chromium.googlesource.com/chromium/src/+/8bad70ff3383c7c71a96451d32aa93fc4a87b018
Patch Set 1 #Patch Set 2 : Update after pull. #Patch Set 3 : Uploaded and fixing cl #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : Remove manifest related changes. #Patch Set 7 : Read dictionary with url_template key, instead of reading string #Patch Set 8 : Added tests. #
Total comments: 54
Patch Set 9 : Separate tests, add .h, as per feedback #
Total comments: 49
Patch Set 10 : Fixed includes #Patch Set 11 : Fixed, as per feedback #
Total comments: 12
Patch Set 12 : Fixed, as per feedback #
Total comments: 3
Patch Set 13 : Declare vars closer to use, plus rebase #Patch Set 14 : Rename AddShareTargetsToPrefs to UpdateShareTargetInPrefs #Patch Set 15 : Rebase upstream #Patch Set 16 : Remove dependency #Patch Set 17 : Remove upstream dependency #
Messages
Total messages: 59 (32 generated)
Description was changed from ========== BUG= ========== to ========== Adds a pref name for share targets, and their related manifest data. Adds a dictionary pref name, to store sites with manifests that contain the share_url_template field; the key is the site origin, and value is the value of share_url_template in the site's manifest. Reads manifests after they have been parsed, for the share_url_template, and stores the site and value under the pref name added. Dependant on: codereview.chromium.org/2637003002/ Follow-up CL will read the pref store for the pref name added, and read share targets in chrome/browser/webshare/share_service_impl.cc. BUG=668389 ==========
Description was changed from ========== Adds a pref name for share targets, and their related manifest data. Adds a dictionary pref name, to store sites with manifests that contain the share_url_template field; the key is the site origin, and value is the value of share_url_template in the site's manifest. Reads manifests after they have been parsed, for the share_url_template, and stores the site and value under the pref name added. Dependant on: codereview.chromium.org/2637003002/ Follow-up CL will read the pref store for the pref name added, and read share targets in chrome/browser/webshare/share_service_impl.cc. BUG=668389 ========== to ========== Add a pref name for share targets, and their related manifest data. Adds a dictionary pref name, to store sites with manifests that contain the share_url_template field; the key is the site origin, and value is the value of share_url_template in the site's manifest. Reads manifests after they have been parsed, for the share_url_template, and stores the site and value under the pref name added. Dependant on: codereview.chromium.org/2637003002/ Follow-up CL will read the pref store for the pref name added, and read share targets in chrome/browser/webshare/share_service_impl.cc. BUG=668389 ==========
constantina@google.com changed reviewers: + mgiuca@chromium.org
PTAL for preliminary review.
Added tests; ready for review.
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/bookmark_app_helper.cc (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:590: manifest.share_target.url_template, Following up from my comment on the manifest's CL, maybe this should be: ``` if (!ChromeOriginTrialPolicy().IsFeatureDisabled("WebShare") && manifest.share_target.has_value()) { AddShareTargetToPrefs(manifest_url.GetOrigin().spec(), manifest.share_target.value().url_template, profile_->GetPrefs()); } ```
Great work figuring out all this stuff! Despite a lot of comments, you nailed the overall structure and wrote some thorough tests. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/bookmark_app_helper.cc (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:31: #include "chrome/browser/webshare/share_target_pref_helper.cc" You can't include a .cc file. Need a header file. (Did you make a header file but forgot to add it, or is there only a .cc file?) https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:590: manifest.share_target.url_template, Needs formatting (some alignment issue). https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/profile... File chrome/browser/profiles/profile.cc (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile.cc:180: #if defined(OS_WIN) || defined(OS_LINUX) I don't think this OS check is appropriate. If we want to add a new OS to WST later it will be hard to track down all these conditions. We *maybe* should add a new flag for WST, but I've had a lot of trouble with those in the past (if needed through a lot of the codebase). I'd rather just leave this open -- it doesn't matter if it's registered on other platforms (it just won't be used). https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper.cc (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. Copyright 2017. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:4: You need a .h file for this file. Also you need to add both this and the header file to the BUILD file for chrome/browser. And #include the header file from its .cc file. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:14: namespace { This can't be in an anonymous namespace, since it will be used by other modules. (It just happens to work because you are directly #including the .cc file, but you aren't allowed do that.) https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:18: // this function does nothing. Obsolete comment? It removes the target from the pref. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:19: void AddShareTargetToPrefs(std::string share_target_origin, StringPiece? https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:20: base::NullableString16 share_target_url_template, Hmm, I think it would be more natural to accept a std::string (or rather a base::StringPiece) here. I think the NullableString16 is something sammc@ is trying to remove from Manifest -- it is a weird anomaly which we're trying to get rid of AFAIK. It's better for this interface to distance itself from the implementation details of Manifest, and it's best for interfaces to take string, not string16, if they need a string internally. Do the 16-to-8-bit conversion in the calling code. But you do care about optionality. Can this be a base::Optional<base::StringPiece>? https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:25: if (share_target_url_template.is_null()) { nit: && rather than two if statements. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:38: ss >> url_template; What's with the stringstream? Are you just doing this to convert the string16 to a string? Use UTF16ToUTF8 in utf_string_conversions.h. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper_unittest.cc (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. Copyright 2017. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:23: TEST_F(ShareTargetPrefHelperUnittest, ShareTargetInfoFromManifestToPrefs) { I think this could be broken up into multiple individual tests. This would help to distinguish which of these are actually intended to be run in sequence and which are independent of one another. For example, adding sharetarget2.com should be in the same test as sharetarget.com, since you are testing having multiple share targets. Adding sharetarget2.com with a null should be in the same test as sharetarget2.com, since you are testing the behaviour of adding it once, then again. But split those up into two separate tests (this will involve duplicating the initial "insert", which you should not feel bad about). The test of "don't add a site that has a null template" can be a test by itself. Etc. This means putting constants into static members of the class (for reuse). And setting up TestingPrefServiceSimple in a SetUp method, rather than at the top of this test -- which is good practice anyway. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:26: TestingPrefServiceSimple* pref_service = new TestingPrefServiceSimple(); This will leak memory. Can this be a non-pointer so it cleans itself up at the end? (After taking my above advice into account, this will need to be a class member, so probably makes sense for it to be a unique_ptr which is initialized in the SetUp method.) https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:50: std::string url_template_in_dict; I think you can drop "_in_dict" since there is no other variable called "url_template". https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:51: ASSERT_TRUE(share_target_info_dict->GetString(kUrlTemplateKey, Does not have to be an ASSERT, since if this fails it won't result in a bad state; just an empty string. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:101: base::NullableString16(url_string16, true), I don't think it's worth testing a *null* NullableString16 with a *non-empty* string in it -- that's just a bad input. So keep this test but just call base::NullableString16() which creates an empty, null NullableString16. https://codereview.chromium.org/2639463002/diff/140001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/common/pref_nam... chrome/common/pref_names.cc:2364: #if defined(OS_WIN) || defined(OS_LINUX) Again, don't qualify this for specific OSes. https://codereview.chromium.org/2639463002/diff/140001/chrome/common/pref_nam... chrome/common/pref_names.cc:2365: // A dictionary of domains of Web Share Targets to their share_url_template Not really their url template... it maps to a dict containing a copy of the "share_target" section of the manifest. https://codereview.chromium.org/2639463002/diff/140001/chrome/common/pref_nam... chrome/common/pref_names.cc:2367: const char kWebShareVisitedTargets[] = "profile.web_share.visited_targets"; Can we call this "registered_targets" instead of visited? Because if we change it in the future to require explicit user interaction, I imagine we won't store things here until the user asks for it, right? https://codereview.chromium.org/2639463002/diff/140001/chrome/common/pref_nam... File chrome/common/pref_names.h (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/common/pref_nam... chrome/common/pref_names.h:872: #if defined(OS_WIN) || defined(OS_LINUX) Again, don't qualify this for specific OSes.
https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/bookmark_app_helper.cc (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. Comment about the CL description: - The first line should describe briefly the WHOLE CL. "Add a pref name" doesn't capture that you are actually recording things into the pref with this same CL. Make a brief summary, then go into details. - Remove "Dependant on" line. That will not be true once this lands, it's only true now because that CL hasn't landed yet. Reviewers can see that it's dependent from the dependency information below the patch set.
Description was changed from ========== Add a pref name for share targets, and their related manifest data. Adds a dictionary pref name, to store sites with manifests that contain the share_url_template field; the key is the site origin, and value is the value of share_url_template in the site's manifest. Reads manifests after they have been parsed, for the share_url_template, and stores the site and value under the pref name added. Dependant on: codereview.chromium.org/2637003002/ Follow-up CL will read the pref store for the pref name added, and read share targets in chrome/browser/webshare/share_service_impl.cc. BUG=668389 ========== to ========== Add a pref name for share targets, and store their manifest data. Adds a dictionary pref name, to store sites with manifests that contain the share_url_template field; the key is the site origin, and value is the value of share_url_template in the site's manifest. Reads manifests after they have been parsed, for the share_url_template, and stores the site and value under the pref name added. Follow-up CL will read the pref store for the pref name added, and read share targets in chrome/browser/webshare/share_service_impl.cc. BUG=668389 ==========
https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper.cc (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:41: share_target_dict->SetWithoutPathExpansion(share_target_origin, As discussed offline, this should actually be keyed to the full (absolute) manifest URL, not just the origin (because there could be two separate manifests with target apps on the same domain).
https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/bookmark_app_helper.cc (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2017/01/23 03:04:12, Matt Giuca wrote: > Comment about the CL description: > > - The first line should describe briefly the WHOLE CL. "Add a pref name" doesn't > capture that you are actually recording things into the pref with this same CL. > Make a brief summary, then go into details. > > - Remove "Dependant on" line. That will not be true once this lands, it's only > true now because that CL hasn't landed yet. Reviewers can see that it's > dependent from the dependency information below the patch set. Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:31: #include "chrome/browser/webshare/share_target_pref_helper.cc" On 2017/01/23 03:00:57, Matt Giuca wrote: > You can't include a .cc file. Need a header file. > > (Did you make a header file but forgot to add it, or is there only a .cc file?) Ah yeah, I just wanted to get it to work. Added header file and included it everywhere necessary. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:590: manifest.share_target.url_template, On 2017/01/23 03:00:57, Matt Giuca wrote: > Needs formatting (some alignment issue). Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:590: manifest.share_target.url_template, On 2017/01/23 00:24:06, mlamouri wrote: > Following up from my comment on the manifest's CL, maybe this should be: > ``` > if (!ChromeOriginTrialPolicy().IsFeatureDisabled("WebShare") && > manifest.share_target.has_value()) { > AddShareTargetToPrefs(manifest_url.GetOrigin().spec(), > manifest.share_target.value().url_template, > profile_->GetPrefs()); > } > ``` Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/profile... File chrome/browser/profiles/profile.cc (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile.cc:180: #if defined(OS_WIN) || defined(OS_LINUX) On 2017/01/23 03:00:57, Matt Giuca wrote: > I don't think this OS check is appropriate. If we want to add a new OS to WST > later it will be hard to track down all these conditions. We *maybe* should add > a new flag for WST, but I've had a lot of trouble with those in the past (if > needed through a lot of the codebase). I'd rather just leave this open -- it > doesn't matter if it's registered on other platforms (it just won't be used). Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper.cc (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/23 03:00:57, Matt Giuca wrote: > Copyright 2017. OMG. Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:4: On 2017/01/23 03:00:57, Matt Giuca wrote: > You need a .h file for this file. > > Also you need to add both this and the header file to the BUILD file for > chrome/browser. And #include the header file from its .cc file. Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:14: namespace { On 2017/01/23 03:00:57, Matt Giuca wrote: > This can't be in an anonymous namespace, since it will be used by other modules. > > (It just happens to work because you are directly #including the .cc file, but > you aren't allowed do that.) Done. Used 'webshare'. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:18: // this function does nothing. On 2017/01/23 03:00:58, Matt Giuca wrote: > Obsolete comment? It removes the target from the pref. Yeah, obsolete. Removed. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:19: void AddShareTargetToPrefs(std::string share_target_origin, On 2017/01/23 03:00:57, Matt Giuca wrote: > StringPiece? Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:20: base::NullableString16 share_target_url_template, On 2017/01/23 03:00:57, Matt Giuca wrote: > Hmm, I think it would be more natural to accept a std::string (or rather a > base::StringPiece) here. > > I think the NullableString16 is something sammc@ is trying to remove from > Manifest -- it is a weird anomaly which we're trying to get rid of AFAIK. It's > better for this interface to distance itself from the implementation details of > Manifest, and it's best for interfaces to take string, not string16, if they > need a string internally. > > Do the 16-to-8-bit conversion in the calling code. > > But you do care about optionality. Can this be a > base::Optional<base::StringPiece>? I do care about optionality, because we want to allow empty url templates, and want to distinguish between empty and non-existent. Changed to Optional. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:25: if (share_target_url_template.is_null()) { On 2017/01/23 03:00:57, Matt Giuca wrote: > nit: && rather than two if statements. There are supposed to be two distinct cases here, and the return occurs in both; do you want me to unnest them? ''' if (share_target_url_template.is_null() && share_target_dict->HasKey(share_target_origin)) { share_target_dict->RemoveWithoutPathExpansion(share_target_origin, NULL); return; } if (share_target_url_template.is_null()) return; ''' https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:38: ss >> url_template; On 2017/01/23 03:00:57, Matt Giuca wrote: > What's with the stringstream? Are you just doing this to convert the string16 to > a string? > > Use UTF16ToUTF8 in utf_string_conversions.h. Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:41: share_target_dict->SetWithoutPathExpansion(share_target_origin, On 2017/01/23 03:58:56, Matt Giuca wrote: > As discussed offline, this should actually be keyed to the full (absolute) > manifest URL, not just the origin (because there could be two separate manifests > with target apps on the same domain). Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper_unittest.cc (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/23 03:00:58, Matt Giuca wrote: > Copyright 2017. Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:23: TEST_F(ShareTargetPrefHelperUnittest, ShareTargetInfoFromManifestToPrefs) { On 2017/01/23 03:00:58, Matt Giuca wrote: > I think this could be broken up into multiple individual tests. This would help > to distinguish which of these are actually intended to be run in sequence and > which are independent of one another. > > For example, adding http://sharetarget2.com should be in the same test as > http://sharetarget.com, since you are testing having multiple share targets. Adding > http://sharetarget2.com with a null should be in the same test as http://sharetarget2.com, > since you are testing the behaviour of adding it once, then again. But split > those up into two separate tests (this will involve duplicating the initial > "insert", which you should not feel bad about). > > The test of "don't add a site that has a null template" can be a test by itself. > Etc. > > This means putting constants into static members of the class (for reuse). And > setting up TestingPrefServiceSimple in a SetUp method, rather than at the top of > this test -- which is good practice anyway. Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:26: TestingPrefServiceSimple* pref_service = new TestingPrefServiceSimple(); On 2017/01/23 03:00:58, Matt Giuca wrote: > This will leak memory. Can this be a non-pointer so it cleans itself up at the > end? > > (After taking my above advice into account, this will need to be a class member, > so probably makes sense for it to be a unique_ptr which is initialized in the > SetUp method.) Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:50: std::string url_template_in_dict; On 2017/01/23 03:00:58, Matt Giuca wrote: > I think you can drop "_in_dict" since there is no other variable called > "url_template". Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:51: ASSERT_TRUE(share_target_info_dict->GetString(kUrlTemplateKey, On 2017/01/23 03:00:58, Matt Giuca wrote: > Does not have to be an ASSERT, since if this fails it won't result in a bad > state; just an empty string. Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:101: base::NullableString16(url_string16, true), On 2017/01/23 03:00:58, Matt Giuca wrote: > I don't think it's worth testing a *null* NullableString16 with a *non-empty* > string in it -- that's just a bad input. > > So keep this test but just call base::NullableString16() which creates an empty, > null NullableString16. Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/common/pref_nam... chrome/common/pref_names.cc:2364: #if defined(OS_WIN) || defined(OS_LINUX) On 2017/01/23 03:01:00, Matt Giuca wrote: > Again, don't qualify this for specific OSes. Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/common/pref_nam... chrome/common/pref_names.cc:2365: // A dictionary of domains of Web Share Targets to their share_url_template On 2017/01/23 03:00:59, Matt Giuca wrote: > Not really their url template... it maps to a dict containing a copy of the > "share_target" section of the manifest. Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/common/pref_nam... chrome/common/pref_names.cc:2367: const char kWebShareVisitedTargets[] = "profile.web_share.visited_targets"; On 2017/01/23 03:00:58, Matt Giuca wrote: > Can we call this "registered_targets" instead of visited? Because if we change > it in the future to require explicit user interaction, I imagine we won't store > things here until the user asks for it, right? If I change the pref name without changing the functionality, I fear that someone reading the code as it is will be confused/make assumption about the functionality. Added a TODO instead. https://codereview.chromium.org/2639463002/diff/140001/chrome/common/pref_nam... File chrome/common/pref_names.h (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/common/pref_nam... chrome/common/pref_names.h:872: #if defined(OS_WIN) || defined(OS_LINUX) On 2017/01/23 03:01:00, Matt Giuca wrote: > Again, don't qualify this for specific OSes. Done.
https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper.cc (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:14: namespace { On 2017/01/24 02:03:43, constantina wrote: > On 2017/01/23 03:00:57, Matt Giuca wrote: > > This can't be in an anonymous namespace, since it will be used by other > modules. > > > > (It just happens to work because you are directly #including the .cc file, but > > you aren't allowed do that.) > > Done. Used 'webshare'. We haven't used the "webshare" namespace anywhere else. I think just leave this bare for consistency. (i.e., ShareServiceImpl is not in any namespace.) https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:25: if (share_target_url_template.is_null()) { On 2017/01/24 02:03:42, constantina wrote: > On 2017/01/23 03:00:57, Matt Giuca wrote: > > nit: && rather than two if statements. > > There are supposed to be two distinct cases here, and the return occurs in both; > do you want me to unnest them? > ''' > if (share_target_url_template.is_null() && > share_target_dict->HasKey(share_target_origin)) { > share_target_dict->RemoveWithoutPathExpansion(share_target_origin, NULL); > return; > } > > if (share_target_url_template.is_null()) return; > ''' I see, I misread it. No, don't do the same check twice. Instead, just add a blank line between that inner if statement and the return, so it's a bit easier to follow. https://codereview.chromium.org/2639463002/diff/140001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/common/pref_nam... chrome/common/pref_names.cc:2367: const char kWebShareVisitedTargets[] = "profile.web_share.visited_targets"; On 2017/01/24 02:03:43, constantina wrote: > On 2017/01/23 03:00:58, Matt Giuca wrote: > > Can we call this "registered_targets" instead of visited? Because if we change > > it in the future to require explicit user interaction, I imagine we won't > store > > things here until the user asks for it, right? > > If I change the pref name without changing the functionality, I fear that > someone reading the code as it is will be confused/make assumption about the > functionality. Added a TODO instead. The problem is that it's really hard to change a pref name (without deleting all the data). I guess it's a question of whether *if* we change to explicit registration if we want to clear out all the existing visited ones. And... I guess we do? OK in that case just remove the TODO. We don't have a solid plan for this and it currently does what it says it does. That's sufficient. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/bookmark_app_helper.cc (right): https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:590: base::StringPiece share_target( This is a confusing name since it's the manifest URL, not the share target. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:592: manifest_url_spec.size() - manifest_url.ExtractFileName().size()); This is getting the path to the directory the manifest is in? But chopping off its filename? Why chop off the filename? There could be two manifests in the same directory. We should use the full URL including the filename as the key. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:594: base::Optional<std::string> url_template = base::nullopt; I don't think you need "= base::nullopt" (default constructor should be this). https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:601: webshare::AddShareTargetToPrefs(share_target, url_template, std::move(url_template) to avoid copying. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper.cc (right): https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:7: #include <sstream> No more sstream. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:24: share_target_dict->RemoveWithoutPathExpansion(share_target_origin, NULL); Does this need a check at all? If the key is missing, and you call RemoveWithoutPathExpansion, will it just do nothing (which is what you want)? If so, remove the check as it is redundant. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper.h (right): https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.h:10: #include "base/values.h" This isn't needed here -- move it into the .cc file. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.h:11: #include "components/prefs/pref_service.h" This can be forward-declared, since you only refer to it from a function and it is just an ordinary class (unlike StringPiece which is a typedef, and Optional which is templated). Add "class PrefService;" and move this #include into the .cc file. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.h:12: #include "components/prefs/scoped_user_pref_update.h" This isn't needed. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.h:13: #include "content/public/common/manifest.h" Nor is this. Only include headers you need to refer to in the header (not the .cc file). And don't include the same header from both the .h and .cc file. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.h:21: void AddShareTargetToPrefs(base::StringPiece share_target_origin, This is no longer an origin, but a full URL to the manifest. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper_unittest.cc (right): https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:16: I think this entire file (except for the forward declares) should be in an anonymous namespace. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:23: pref_service.registry()->RegisterDictionaryPref( This will need a corresponding TearDown which resets it back to a default state. If that's not possible, make it a unique_ptr and construct a new TestingPrefServiceSimple on SetUp. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:30: TestingPrefServiceSimple pref_service; These should follow standard style rules: fields should be private and names should end with underscores. Make a pref_service() getter which is protected. This has the advantage that pref_service() can return a PrefService*, abstracting away the fact that this is a TestingPrefServiceSimple from the rest of the test. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:31: std::string share_target_origin; These four (share_target_origin .. share_target_info_dict) don't really belong here. They don't represent pre-setup state or constant data used by all tests; rather they are local variables that happen to be used in all tests. I think move them back down into the individual tests. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:39: TEST_F(ShareTargetPrefHelperUnittest, AddMultipleShareTargets) { Good, the tests feel much better split up. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:41: { I don't think these braces are really warranted. They aren't being used for scoping (all your variables are being declared outside these blocks anyway), just for logical separation. You already have the visual separation from the blank lines and comments. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:43: expected_url_template = base::Optional<std::string>("share/?title={title}"); This shouldn't be called "expected_url_template" because it's also used as the *input* to the function-under-test. Just "url_template". (The fact that it is also used as the expected value is fine; it just doesn't justify it being called "expected".) https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:81: TEST_F(ShareTargetPrefHelperUnittest, AddShareTargetTwice) { I think this test can be removed ... it is essentially testing the same thing as UpdateShareTarget. Isn't it? https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:121: TEST_F(ShareTargetPrefHelperUnittest, DontAddNonShareTarget) { Move this down right before RemoveShareTarget.... https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:123: { Don't need the extra braces for a test that only has one block. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:182: TEST_F(ShareTargetPrefHelperUnittest, RemoveShareTargetThatIsNoLongerTarget) { Maybe just rename this to "RemoveShareTarget". https://codereview.chromium.org/2639463002/diff/160001/chrome/common/pref_nam... File chrome/common/pref_names.h (right): https://codereview.chromium.org/2639463002/diff/160001/chrome/common/pref_nam... chrome/common/pref_names.h:42: extern const char kNewTabPageLocationOverride[]; You rebased and made local changes in the same Patch Set. A reminder: if you rebase, please upload a patch set containing JUST the rebase called "Rebase", and then upload your local changes. This makes it much easier to review (fortunately it's fine in this case because none of the important files got affected by the rebase).
https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper.cc (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:14: namespace { On 2017/01/24 03:26:09, Matt Giuca wrote: > On 2017/01/24 02:03:43, constantina wrote: > > On 2017/01/23 03:00:57, Matt Giuca wrote: > > > This can't be in an anonymous namespace, since it will be used by other > > modules. > > > > > > (It just happens to work because you are directly #including the .cc file, > but > > > you aren't allowed do that.) > > > > Done. Used 'webshare'. > > We haven't used the "webshare" namespace anywhere else. I think just leave this > bare for consistency. (i.e., ShareServiceImpl is not in any namespace.) Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:25: if (share_target_url_template.is_null()) { On 2017/01/24 03:26:09, Matt Giuca wrote: > On 2017/01/24 02:03:42, constantina wrote: > > On 2017/01/23 03:00:57, Matt Giuca wrote: > > > nit: && rather than two if statements. > > > > There are supposed to be two distinct cases here, and the return occurs in > both; > > do you want me to unnest them? > > ''' > > if (share_target_url_template.is_null() && > > share_target_dict->HasKey(share_target_origin)) { > > share_target_dict->RemoveWithoutPathExpansion(share_target_origin, NULL); > > return; > > } > > > > if (share_target_url_template.is_null()) return; > > ''' > > I see, I misread it. > > No, don't do the same check twice. Instead, just add a blank line between that > inner if statement and the return, so it's a bit easier to follow. Done. https://codereview.chromium.org/2639463002/diff/140001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2639463002/diff/140001/chrome/common/pref_nam... chrome/common/pref_names.cc:2367: const char kWebShareVisitedTargets[] = "profile.web_share.visited_targets"; On 2017/01/24 03:26:09, Matt Giuca wrote: > On 2017/01/24 02:03:43, constantina wrote: > > On 2017/01/23 03:00:58, Matt Giuca wrote: > > > Can we call this "registered_targets" instead of visited? Because if we > change > > > it in the future to require explicit user interaction, I imagine we won't > > store > > > things here until the user asks for it, right? > > > > If I change the pref name without changing the functionality, I fear that > > someone reading the code as it is will be confused/make assumption about the > > functionality. Added a TODO instead. > > The problem is that it's really hard to change a pref name (without deleting all > the data). I guess it's a question of whether *if* we change to explicit > registration if we want to clear out all the existing visited ones. And... I > guess we do? > > OK in that case just remove the TODO. We don't have a solid plan for this and it > currently does what it says it does. That's sufficient. Done. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/bookmark_app_helper.cc (right): https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:590: base::StringPiece share_target( On 2017/01/24 03:26:09, Matt Giuca (OOO til Jan 30) wrote: > This is a confusing name since it's the manifest URL, not the share target. Done. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:592: manifest_url_spec.size() - manifest_url.ExtractFileName().size()); On 2017/01/24 03:26:09, Matt Giuca (OOO til Jan 30) wrote: > This is getting the path to the directory the manifest is in? But chopping off > its filename? > > Why chop off the filename? There could be two manifests in the same directory. > We should use the full URL including the filename as the key. My bad, I misinterpretted manifest URL, as this. Changed all instances. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:594: base::Optional<std::string> url_template = base::nullopt; On 2017/01/24 03:26:09, Matt Giuca (OOO til Jan 30) wrote: > I don't think you need "= base::nullopt" (default constructor should be this). Done. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:601: webshare::AddShareTargetToPrefs(share_target, url_template, On 2017/01/24 03:26:09, Matt Giuca (OOO til Jan 30) wrote: > std::move(url_template) to avoid copying. Done. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper.cc (right): https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:7: #include <sstream> On 2017/01/24 03:26:09, Matt Giuca wrote: > No more sstream. Done. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.cc:24: share_target_dict->RemoveWithoutPathExpansion(share_target_origin, NULL); On 2017/01/24 03:26:09, Matt Giuca (OOO til Jan 30) wrote: > Does this need a check at all? If the key is missing, and you call > RemoveWithoutPathExpansion, will it just do nothing (which is what you want)? If > so, remove the check as it is redundant. Yeah, it does nothing. Deleted. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper.h (right): https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.h:10: #include "base/values.h" On 2017/01/24 03:26:09, Matt Giuca wrote: > This isn't needed here -- move it into the .cc file. Done. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.h:11: #include "components/prefs/pref_service.h" On 2017/01/24 03:26:09, Matt Giuca wrote: > This can be forward-declared, since you only refer to it from a function and it > is just an ordinary class (unlike StringPiece which is a typedef, and Optional > which is templated). > > Add "class PrefService;" and move this #include into the .cc file. Done. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.h:12: #include "components/prefs/scoped_user_pref_update.h" On 2017/01/24 03:26:09, Matt Giuca wrote: > This isn't needed. Done. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.h:13: #include "content/public/common/manifest.h" On 2017/01/24 03:26:09, Matt Giuca wrote: > Nor is this. > > Only include headers you need to refer to in the header (not the .cc file). And > don't include the same header from both the .h and .cc file. Done. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper.h:21: void AddShareTargetToPrefs(base::StringPiece share_target_origin, On 2017/01/24 03:26:09, Matt Giuca (OOO til Jan 30) wrote: > This is no longer an origin, but a full URL to the manifest. Done. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper_unittest.cc (right): https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:16: On 2017/01/24 03:26:09, Matt Giuca wrote: > I think this entire file (except for the forward declares) should be in an > anonymous namespace. Done. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:23: pref_service.registry()->RegisterDictionaryPref( As in, an UnregisterPref? I can't find anything that exists like that, so made it a unique_ptr. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:30: TestingPrefServiceSimple pref_service; On 2017/01/24 03:26:10, Matt Giuca (OOO til Jan 30) wrote: > These should follow standard style rules: fields should be private and names > should end with underscores. Make a pref_service() getter which is protected. > This has the advantage that pref_service() can return a PrefService*, > abstracting away the fact that this is a TestingPrefServiceSimple from the rest > of the test. Done. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:31: std::string share_target_origin; On 2017/01/24 03:26:10, Matt Giuca wrote: > These four (share_target_origin .. share_target_info_dict) don't really belong > here. They don't represent pre-setup state or constant data used by all tests; > rather they are local variables that happen to be used in all tests. > > I think move them back down into the individual tests. Done. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:39: TEST_F(ShareTargetPrefHelperUnittest, AddMultipleShareTargets) { On 2017/01/24 03:26:10, Matt Giuca wrote: > Good, the tests feel much better split up. :D https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:41: { On 2017/01/24 03:26:10, Matt Giuca wrote: > I don't think these braces are really warranted. They aren't being used for > scoping (all your variables are being declared outside these blocks anyway), > just for logical separation. You already have the visual separation from the > blank lines and comments. Done. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:43: expected_url_template = base::Optional<std::string>("share/?title={title}"); On 2017/01/24 03:26:10, Matt Giuca (OOO til Jan 30) wrote: > This shouldn't be called "expected_url_template" because it's also used as the > *input* to the function-under-test. Just "url_template". > > (The fact that it is also used as the expected value is fine; it just doesn't > justify it being called "expected".) There's already a url_template, which you said to change from url_template_in_dict, because there was no other url_template. I could change that back and change the expected one to just url_template? https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:81: TEST_F(ShareTargetPrefHelperUnittest, AddShareTargetTwice) { On 2017/01/24 03:26:10, Matt Giuca (OOO til Jan 30) wrote: > I think this test can be removed ... it is essentially testing the same thing as > UpdateShareTarget. Isn't it? One adds the same share target with the same url_template, and the other adds the same share target with a different url_template. I would count those as different cases, but can delete if you disagree. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:121: TEST_F(ShareTargetPrefHelperUnittest, DontAddNonShareTarget) { On 2017/01/24 03:26:10, Matt Giuca (OOO til Jan 30) wrote: > Move this down right before RemoveShareTarget.... Done. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:123: { On 2017/01/24 03:26:10, Matt Giuca (OOO til Jan 30) wrote: > Don't need the extra braces for a test that only has one block. Done. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:182: TEST_F(ShareTargetPrefHelperUnittest, RemoveShareTargetThatIsNoLongerTarget) { On 2017/01/24 03:26:10, Matt Giuca (OOO til Jan 30) wrote: > Maybe just rename this to "RemoveShareTarget". Done. https://codereview.chromium.org/2639463002/diff/160001/chrome/common/pref_nam... File chrome/common/pref_names.h (right): https://codereview.chromium.org/2639463002/diff/160001/chrome/common/pref_nam... chrome/common/pref_names.h:42: extern const char kNewTabPageLocationOverride[]; On 2017/01/24 03:26:10, Matt Giuca wrote: > You rebased and made local changes in the same Patch Set. A reminder: if you > rebase, please upload a patch set containing JUST the rebase called "Rebase", > and then upload your local changes. This makes it much easier to review > (fortunately it's fine in this case because none of the important files got > affected by the rebase). Acknowledged.
lgtm with a few more nits requested. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper_unittest.cc (right): https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:23: pref_service.registry()->RegisterDictionaryPref( On 2017/01/24 23:39:05, constantina wrote: > As in, an UnregisterPref? I can't find anything that exists like that, so made > it a unique_ptr. Acknowledged. https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:43: expected_url_template = base::Optional<std::string>("share/?title={title}"); On 2017/01/24 23:39:05, constantina wrote: > On 2017/01/24 03:26:10, Matt Giuca (OOO til Jan 30) wrote: > > This shouldn't be called "expected_url_template" because it's also used as the > > *input* to the function-under-test. Just "url_template". > > > > (The fact that it is also used as the expected value is fine; it just doesn't > > justify it being called "expected".) > > There's already a url_template, which you said to change from > url_template_in_dict, because there was no other url_template. I could change > that back and change the expected one to just url_template? Oh I see what I did there. Yeah I think it makes more sense to change it back: url_template_from_dict (or in_dict) and url_template. https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/bookmark_app_helper.cc (right): https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:589: std::string manifest_url_string = manifest_url.spec(); This can be a const std::string& (since GURL::spec() returns a reference). https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper_unittest.cc (right): https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:25: pref_service_ = std::unique_ptr<TestingPrefServiceSimple>( pref_service_.reset(new TPSS()); Prefer reset over construction of a new object and assignment. Just less verbose. (Also *potentially* more efficient but likely to be the same either way; that's not the main reason.) https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:40: std::string manifest_url; nit: I *think* these should be declared along side their first assignment (no strong feeling though). https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:82: std::string manifest_url; Again, just declare these as you need them. (Avoids needing to assign nullptr just to assign it again later.) In this one, you aren't changing manifest_url and expected_url_template between the two adds, so they can be constants. manifest_url should be const char []. And rename them to kXXX. https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:166: std::string manifest_url; As with AddShareTarget, these can be constants because they are never changed.
https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper_unittest.cc (right): https://codereview.chromium.org/2639463002/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:43: expected_url_template = base::Optional<std::string>("share/?title={title}"); On 2017/01/25 00:25:53, Matt Giuca (OOO til Jan 30) wrote: > On 2017/01/24 23:39:05, constantina wrote: > > On 2017/01/24 03:26:10, Matt Giuca (OOO til Jan 30) wrote: > > > This shouldn't be called "expected_url_template" because it's also used as > the > > > *input* to the function-under-test. Just "url_template". > > > > > > (The fact that it is also used as the expected value is fine; it just > doesn't > > > justify it being called "expected".) > > > > There's already a url_template, which you said to change from > > url_template_in_dict, because there was no other url_template. I could change > > that back and change the expected one to just url_template? > > Oh I see what I did there. > > Yeah I think it makes more sense to change it back: url_template_from_dict (or > in_dict) and url_template. Done. https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/bookmark_app_helper.cc (right): https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:589: std::string manifest_url_string = manifest_url.spec(); On 2017/01/25 00:25:53, Matt Giuca (OOO til Jan 30) wrote: > This can be a const std::string& (since GURL::spec() returns a reference). Done. https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper_unittest.cc (right): https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:25: pref_service_ = std::unique_ptr<TestingPrefServiceSimple>( On 2017/01/25 00:25:54, Matt Giuca (OOO til Jan 30) wrote: > pref_service_.reset(new TPSS()); > > Prefer reset over construction of a new object and assignment. Just less > verbose. (Also *potentially* more efficient but likely to be the same either > way; that's not the main reason.) Done. https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:40: std::string manifest_url; On 2017/01/25 00:25:53, Matt Giuca (OOO til Jan 30) wrote: > nit: I *think* these should be declared along side their first assignment (no > strong feeling though). Acknowledged. I think it makes the subtests look inconsistent and messy. But, I could add scoping braces again, and declare the variable in both subtest scopes? Also, url_template and the DictionaryValues need to be declared anyway, to pass them as output parameters. https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:82: std::string manifest_url; On 2017/01/25 00:25:54, Matt Giuca (OOO til Jan 30) wrote: > Again, just declare these as you need them. (Avoids needing to assign nullptr > just to assign it again later.) > > In this one, you aren't changing manifest_url and expected_url_template between > the two adds, so they can be constants. manifest_url should be const char []. > And rename them to kXXX. As I said in last comment, I have to declare them anyway. I initially had to assign nullptr to suppress a warning. But it doesn't appear now when I remove nullptr. Constants are done though. https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:166: std::string manifest_url; On 2017/01/25 00:25:54, Matt Giuca (OOO til Jan 30) wrote: > As with AddShareTarget, these can be constants because they are never changed. Done.
constantina@google.com changed reviewers: + anthonyvd@chromium.org, benwells@chromium.org
anthonyvd@ for chrome/browser/profiles/ benwells@ for extensions/
mlamouri@chromium.org changed reviewers: - mlamouri@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
c/b/profiles/ lgtm
Hi, you wrote "done" in a lot of places but didn't upload a new patch set. Did you forget git cl upload?
On 2017/01/30 03:03:07, Matt Giuca (OOO til Jan 30) wrote: > Hi, you wrote "done" in a lot of places but didn't upload a new patch set. Did > you forget git cl upload? Oops, yeah, I did forget
https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper_unittest.cc (right): https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:40: std::string manifest_url; On 2017/01/25 03:18:27, constantina wrote: > On 2017/01/25 00:25:53, Matt Giuca (OOO til Jan 30) wrote: > > nit: I *think* these should be declared along side their first assignment (no > > strong feeling though). > > Acknowledged. I think it makes the subtests look inconsistent and messy. But, I > could add scoping braces again, and declare the variable in both subtest scopes? > > Also, url_template and the DictionaryValues need to be declared anyway, to pass > them as output parameters. I think the thing is you shouldn't be thinking of them as "subtests" but all part of one test. (If you are testing multiple independent things then they should be split up, and you did that.) I get what you mean by "inconsistent" (if the top one declares and the bottom one re-uses) but it doesn't have to be consistent since they aren't independent anyway. This recommendation comes from the style guide: https://google.github.io/styleguide/cppguide.html#Local_Variables "We encourage you to declare [local variables] ... as close to the first use as possible." The things that get assigned first (manifest_url, expected_url_template, share_target_dict) should be declared at their first assignment. The things that are first used as output parameters (url_template, share_target_info_dict) need to be declared on their own line, but should be done immediately before use.
lgtm, thanks! https://codereview.chromium.org/2639463002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/bookmark_app_helper.cc (right): https://codereview.chromium.org/2639463002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:598: AddShareTargetToPrefs(manifest_url_string, std::move(url_template), Nit: maybe add a comment about why this is called when url_template is blank.
https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/webshar... File chrome/browser/webshare/share_target_pref_helper_unittest.cc (right): https://codereview.chromium.org/2639463002/diff/200001/chrome/browser/webshar... chrome/browser/webshare/share_target_pref_helper_unittest.cc:40: std::string manifest_url; On 2017/01/30 03:32:39, Matt Giuca wrote: > On 2017/01/25 03:18:27, constantina wrote: > > On 2017/01/25 00:25:53, Matt Giuca (OOO til Jan 30) wrote: > > > nit: I *think* these should be declared along side their first assignment > (no > > > strong feeling though). > > > > Acknowledged. I think it makes the subtests look inconsistent and messy. But, > I > > could add scoping braces again, and declare the variable in both subtest > scopes? > > > > Also, url_template and the DictionaryValues need to be declared anyway, to > pass > > them as output parameters. > > I think the thing is you shouldn't be thinking of them as "subtests" but all > part of one test. (If you are testing multiple independent things then they > should be split up, and you did that.) I get what you mean by "inconsistent" (if > the top one declares and the bottom one re-uses) but it doesn't have to be > consistent since they aren't independent anyway. > > This recommendation comes from the style guide: > https://google.github.io/styleguide/cppguide.html#Local_Variables > "We encourage you to declare [local variables] ... as close to the first use as > possible." > > The things that get assigned first (manifest_url, expected_url_template, > share_target_dict) should be declared at their first assignment. The things that > are first used as output parameters (url_template, share_target_info_dict) need > to be declared on their own line, but should be done immediately before use. That makes sense. Done. https://codereview.chromium.org/2639463002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/bookmark_app_helper.cc (right): https://codereview.chromium.org/2639463002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:598: AddShareTargetToPrefs(manifest_url_string, std::move(url_template), On 2017/01/31 00:50:31, benwells wrote: > Nit: maybe add a comment about why this is called when url_template is blank. I realise the confusion probably lies in the fact the function has prefix "Add". Should I rename to "UpdateShareTargetsInPrefs"?
https://codereview.chromium.org/2639463002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/bookmark_app_helper.cc (right): https://codereview.chromium.org/2639463002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/bookmark_app_helper.cc:598: AddShareTargetToPrefs(manifest_url_string, std::move(url_template), On 2017/01/31 23:37:06, constantina wrote: > On 2017/01/31 00:50:31, benwells wrote: > > Nit: maybe add a comment about why this is called when url_template is blank. > > I realise the confusion probably lies in the fact the function has prefix "Add". > Should I rename to "UpdateShareTargetsInPrefs"? Yes that's a good choice. Also add a brief comment explaining that this will remove or not add it if it's empty (despite that already being in the documentation, I think it helps to briefly state that here too). (Since Ben is likely out today, just do this and submit.)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by constantina@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from anthonyvd@chromium.org, mgiuca@chromium.org, benwells@chromium.org Link to the patchset: https://codereview.chromium.org/2639463002/#ps280001 (title: "Rebase upstream")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2667803002 Patch 200001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
Description was changed from ========== Add a pref name for share targets, and store their manifest data. Adds a dictionary pref name, to store sites with manifests that contain the share_url_template field; the key is the site origin, and value is the value of share_url_template in the site's manifest. Reads manifests after they have been parsed, for the share_url_template, and stores the site and value under the pref name added. Follow-up CL will read the pref store for the pref name added, and read share targets in chrome/browser/webshare/share_service_impl.cc. BUG=668389 ========== to ========== Add a pref name for share targets, and store their manifest data. Adds a dictionary pref name, to store sites with manifests that contain the share_url_template field; the key is the site origin, and value is the value of share_url_template in the site's manifest. Reads manifests after they have been parsed, for the share_url_template, and stores the site and value under the pref name added. Follow-up CL will read the pref store for the pref name added, and read share targets in chrome/browser/webshare/share_service_impl.cc. NO_DEPENDENCY_CHECKS=true BUG=668389 ==========
The CQ bit was checked by constantina@google.com
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by constantina@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from benwells@chromium.org, anthonyvd@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2639463002/#ps300001 (title: "Remove dependency")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by constantina@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from benwells@chromium.org, anthonyvd@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2639463002/#ps320001 (title: "Remove upstream dependency")
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": 320001, "attempt_start_ts": 1486015506331010,
"parent_rev": "952ec0e36758c881a1e7a388895eb61d0d840817", "commit_rev":
"8bad70ff3383c7c71a96451d32aa93fc4a87b018"}
Message was sent while issue was closed.
Description was changed from ========== Add a pref name for share targets, and store their manifest data. Adds a dictionary pref name, to store sites with manifests that contain the share_url_template field; the key is the site origin, and value is the value of share_url_template in the site's manifest. Reads manifests after they have been parsed, for the share_url_template, and stores the site and value under the pref name added. Follow-up CL will read the pref store for the pref name added, and read share targets in chrome/browser/webshare/share_service_impl.cc. NO_DEPENDENCY_CHECKS=true BUG=668389 ========== to ========== Add a pref name for share targets, and store their manifest data. Adds a dictionary pref name, to store sites with manifests that contain the share_url_template field; the key is the site origin, and value is the value of share_url_template in the site's manifest. Reads manifests after they have been parsed, for the share_url_template, and stores the site and value under the pref name added. Follow-up CL will read the pref store for the pref name added, and read share targets in chrome/browser/webshare/share_service_impl.cc. NO_DEPENDENCY_CHECKS=true BUG=668389 Review-Url: https://codereview.chromium.org/2639463002 Cr-Commit-Position: refs/heads/master@{#447721} Committed: https://chromium.googlesource.com/chromium/src/+/8bad70ff3383c7c71a96451d32aa... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/8bad70ff3383c7c71a96451d32aa... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
