Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2580903002:
Clean up ContentSubresourceFilterDriverFactory. (Closed)
|
Created:
4 years ago by pkalinnikov Modified:
4 years ago CC:
chromium-reviews, jam, darin-cc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClean up ContentSubresourceFilterDriverFactory.
- Remove a fulfilled TODO.
- Remove unused methods.
- Move implementation details to private / to .cc file.
- Avoid unnecessary std::string/GURL allocations.
- Push URL to the navigation chain only for main frame.
BUG=609747
Committed: https://crrev.com/f5c2fb3e7f0c1c5c8442f476df8f8243f94c292c
Cr-Commit-Position: refs/heads/master@{#439085}
Patch Set 1 #
Total comments: 4
Messages
Total messages: 18 (10 generated)
Description was changed from ========== Minor ContentSubresourceFilterDriverFactory refactoring. BUG=609747 ========== to ========== Minor ContentSubresourceFilterDriverFactory refactoring. - Remove unused methods. - Move implementation details to private / to .cc file. - Avoid unnecessary std::string/GURL allocations. - Push URL to the navigation chain only for main frame. BUG=609747 ==========
Description was changed from ========== Minor ContentSubresourceFilterDriverFactory refactoring. - Remove unused methods. - Move implementation details to private / to .cc file. - Avoid unnecessary std::string/GURL allocations. - Push URL to the navigation chain only for main frame. BUG=609747 ========== to ========== Minor ContentSubresourceFilterDriverFactory refactoring. - Remove fulfilled TODO. - Remove unused methods. - Move implementation details to private / to .cc file. - Avoid unnecessary std::string/GURL allocations. - Push URL to the navigation chain only for main frame. BUG=609747 ==========
Description was changed from ========== Minor ContentSubresourceFilterDriverFactory refactoring. - Remove fulfilled TODO. - Remove unused methods. - Move implementation details to private / to .cc file. - Avoid unnecessary std::string/GURL allocations. - Push URL to the navigation chain only for main frame. BUG=609747 ========== to ========== Minor ContentSubresourceFilterDriverFactory refactoring. - Remove a fulfilled TODO. - Remove unused methods. - Move implementation details to private / to .cc file. - Avoid unnecessary std::string/GURL allocations. - Push URL to the navigation chain only for main frame. BUG=609747 ==========
pkalinnikov@chromium.org changed reviewers: + engedy@chromium.org, melandory@chromium.org
engedy@: Please review the entire CL. melandory@: Please confirm that the TODO is fulfilled. Fell free to review the rest. Thank you, Pavel https://codereview.chromium.org/2580903002/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2580903002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:60: void AddHostOfURLToWhitelistSet(const GURL& url); From the outside this function is used only in tests. Move this to private too?
Description was changed from ========== Minor ContentSubresourceFilterDriverFactory refactoring. - Remove a fulfilled TODO. - Remove unused methods. - Move implementation details to private / to .cc file. - Avoid unnecessary std::string/GURL allocations. - Push URL to the navigation chain only for main frame. BUG=609747 ========== to ========== Clean up ContentSubresourceFilterDriverFactory. - Removed a fulfilled TODO. - Removed unused methods. - Moved implementation details to private / to .cc file. - Avoid unnecessary std::string/GURL allocations. - Push URL to the navigation chain only for main frame. BUG=609747 ==========
LGTM, thanks! https://codereview.chromium.org/2580903002/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2580903002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:60: void AddHostOfURLToWhitelistSet(const GURL& url); On 2016/12/15 17:14:13, pkalinnikov wrote: > From the outside this function is used only in tests. Move this to private too? Sounds good to me.
Description was changed from ========== Clean up ContentSubresourceFilterDriverFactory. - Removed a fulfilled TODO. - Removed unused methods. - Moved implementation details to private / to .cc file. - Avoid unnecessary std::string/GURL allocations. - Push URL to the navigation chain only for main frame. BUG=609747 ========== to ========== Clean up ContentSubresourceFilterDriverFactory. - Remove a fulfilled TODO. - Remove unused methods. - Move implementation details to private / to .cc file. - Avoid unnecessary std::string/GURL allocations. - Push URL to the navigation chain only for main frame. BUG=609747 ==========
On 2016/12/15 18:23:15, engedy wrote: > LGTM, thanks! > > https://codereview.chromium.org/2580903002/diff/1/components/subresource_filt... > File > components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h > (right): > > https://codereview.chromium.org/2580903002/diff/1/components/subresource_filt... > components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:60: > void AddHostOfURLToWhitelistSet(const GURL& url); > On 2016/12/15 17:14:13, pkalinnikov wrote: > > From the outside this function is used only in tests. Move this to private > too? > > Sounds good to me. lgtm
Submitting. Thanks! https://codereview.chromium.org/2580903002/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2580903002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:60: void AddHostOfURLToWhitelistSet(const GURL& url); On 2016/12/15 18:23:15, engedy wrote: > On 2016/12/15 17:14:13, pkalinnikov wrote: > > From the outside this function is used only in tests. Move this to private > too? > > Sounds good to me. Well, I found that it would require to make at least 3 new friends, probably with the FRIEND_TEST macro. Looks not very nice. Let's then keep it as is.
The CQ bit was checked by pkalinnikov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2580903002/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2580903002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:60: void AddHostOfURLToWhitelistSet(const GURL& url); On 2016/12/16 09:46:36, pkalinnikov wrote: > On 2016/12/15 18:23:15, engedy wrote: > > On 2016/12/15 17:14:13, pkalinnikov wrote: > > > From the outside this function is used only in tests. Move this to private > > too? > > > > Sounds good to me. > > Well, I found that it would require to make at least 3 new friends, probably > with the FRIEND_TEST macro. Looks not very nice. Let's then keep it as is. Agreed.
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1481881600735630, "parent_rev":
"951ec3882e200037691b780c83f0b0bc7075ea1a", "commit_rev":
"118956aba09b67011c10fca88fb31810acb86643"}
Message was sent while issue was closed.
Description was changed from ========== Clean up ContentSubresourceFilterDriverFactory. - Remove a fulfilled TODO. - Remove unused methods. - Move implementation details to private / to .cc file. - Avoid unnecessary std::string/GURL allocations. - Push URL to the navigation chain only for main frame. BUG=609747 ========== to ========== Clean up ContentSubresourceFilterDriverFactory. - Remove a fulfilled TODO. - Remove unused methods. - Move implementation details to private / to .cc file. - Avoid unnecessary std::string/GURL allocations. - Push URL to the navigation chain only for main frame. BUG=609747 Review-Url: https://codereview.chromium.org/2580903002 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Clean up ContentSubresourceFilterDriverFactory. - Remove a fulfilled TODO. - Remove unused methods. - Move implementation details to private / to .cc file. - Avoid unnecessary std::string/GURL allocations. - Push URL to the navigation chain only for main frame. BUG=609747 Review-Url: https://codereview.chromium.org/2580903002 ========== to ========== Clean up ContentSubresourceFilterDriverFactory. - Remove a fulfilled TODO. - Remove unused methods. - Move implementation details to private / to .cc file. - Avoid unnecessary std::string/GURL allocations. - Push URL to the navigation chain only for main frame. BUG=609747 Committed: https://crrev.com/f5c2fb3e7f0c1c5c8442f476df8f8243f94c292c Cr-Commit-Position: refs/heads/master@{#439085} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/f5c2fb3e7f0c1c5c8442f476df8f8243f94c292c Cr-Commit-Position: refs/heads/master@{#439085} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
