Add HTTPSEverywhereService test #74

bbondy · 2018-03-27T20:45:49Z

This adds 2 browser tests to ensure HTTPS Everywhere is working:

Load a URL which has an HTTPSE rule and verify we rewrote it.
Load a URL which has no HTTPSE rule and verify we did not rewrite it.

In addition this also refactors HTTPSEverywhereService to be a singleton.

Fix brave/brave-browser#99

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).

Test Plan:

Reviewer Checklist:

New files have MPL-2.0 license header.
Adequate test coverage exists to prevent regressions

simonhong · 2018-03-28T01:56:45Z

components/brave_shields/browser/https_everywhere_service.h

@@ -38,12 +43,20 @@ class HTTPSEverywhereService : public BaseBraveShieldsService {
 public:
   HTTPSEverywhereService();
   ~HTTPSEverywhereService() override;
+
+  static HTTPSEverywhereService* GetInstance();
+


We can remove line 77 and its implementation.

Good catch, will do if we keep the singleton depending on your answer above.

simonhong · 2018-03-28T02:08:10Z

browser/brave_browser_process_impl.cc

-  https_everywhere_service_ =
-    brave_shields::HTTPSEverywhereServiceFactory();
-  return https_everywhere_service_.get();
+  return brave_shields::HTTPSEverywhereService::GetInstance();


Q) Why do you want to use HTTPSEverywhereService as singleton?
In general, singleton is hard to determine its lifetime.
If there is no reason about that, making BraveBrowserProcessImpl control its lifetime seems fine.

Ah.. I assume that the reason is you want to test its internal state right?
If so, how about using g_browser_process?
With that, you can get HTTPSEverywhereService pointer from it by BraveBrowserProcessImpl casting.

Is there a case where we need to destroy or control its lifetime? Mainly it was just because there was a TODO in the SafeBrowsingService to make it a singleton in the last version of Chrome when this was implemented. The comment seems removed now but it also seems to hand roll a singleton now for it. I thought this service was most close to that.

We have g_browser_process readily available for that btw. No it was really just to resolve a TODO comment that the SafeBrowsingService had and these were similar to. I have no strong feelings though so I can drop that commit.

Sorry I meant to say, we have g_brave_browser_process readily available for that.

simonhong · 2018-03-28T02:11:56Z

components/brave_shields/browser/https_everywhere_service.h

  bool GetHTTPSURL(const GURL* url, const uint64_t& request_id,
      std::string& new_url);
  bool GetHTTPSURLFromCacheOnly(const GURL* url,
      const uint64_t& request_id, std::string& cached_url);

+  // Useful for tests to customize
+  static GURL g_https_everywhere_url;


How about moving this to private section and add test class as a friend?
In general(in chromium), friend class is used to access/modify private member from test class.
For more clear, how about using SetHttpsEveryWhereURLForTest()?
Or, if we should use it in ctor, how about passing it as an argument?

Ah.. I see. you used static member because it's hard to change it during the test process initialization.
Ignore my previous comment about passing as an argument.

Good suggestions, will do.

simonhong

LGTM with nits

simonhong · 2018-03-28T04:09:18Z

components/brave_shields/browser/ad_block_service.cc

-  return base::Singleton<AdBlockService>::get();
+// The brave shields factory. Using the Brave Shields as a singleton
+// is the job of the browser process.
+// TODO(bbondy): consider making this a singleton.


nit: comment should be changed.

simonhong · 2018-03-28T04:09:50Z

components/brave_shields/browser/https_everywhere_service.cc

+  g_https_everywhere_url = url;
+}
+
+


nit: two blank line

simonhong · 2018-03-28T04:10:09Z

components/brave_shields/browser/https_everywhere_service.h

@@ -54,6 +56,9 @@ class HTTPSEverywhereService : public BaseBraveShieldsService {
  std::string CorrecttoRuleToRE2Engine(const std::string& to);

 private:
+  friend class ::HTTPSEverywhereServiceTest;
+  static GURL g_https_everywhere_url;
+  static void SetHttpsEveryWhereURLForTest(const GURL& url);


nit: need one blank line below.

Fix brave/brave-browser#99

This reverts commit 052c7a3.

bbondy · 2018-03-28T04:15:00Z

Pushed out the nits, thanks for the review.

Adds support for get all reports

Fix lint error

bbondy self-assigned this Mar 27, 2018

bbondy requested a review from garrettr March 27, 2018 20:45

bbondy force-pushed the httpse-tests branch from fc8accb to 86ff9c6 Compare March 28, 2018 01:46

bbondy requested a review from simonhong March 28, 2018 01:47

simonhong reviewed Mar 28, 2018

View reviewed changes

bbondy force-pushed the httpse-tests branch 4 times, most recently from 00dcd94 to 97646c4 Compare March 28, 2018 03:56

simonhong previously approved these changes Mar 28, 2018

View reviewed changes

bbondy added 2 commits March 28, 2018 00:13

Add HTTPS Everywhere service browser test

6acd8d3

Fix brave/brave-browser#99

Revert "Make AdBlockService into a singleton"

1eb5105

This reverts commit 052c7a3.

bbondy dismissed simonhong’s stale review via 1eb5105 March 28, 2018 04:14

bbondy force-pushed the httpse-tests branch from 97646c4 to 1eb5105 Compare March 28, 2018 04:14

bbondy merged commit f978d16 into master Mar 28, 2018

bsclifton deleted the httpse-tests branch June 18, 2018 06:29

NejcZdovc added a commit that referenced this pull request Dec 10, 2018

Merge pull request #74 from brave-intl/reports

8885b4c

Adds support for get all reports

bbondy pushed a commit that referenced this pull request Feb 18, 2019

Merge pull request #74 from brave/fix_lint

9e6c43e

Fix lint error

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HTTPSEverywhereService test #74

Add HTTPSEverywhereService test #74

bbondy commented Mar 27, 2018 •

edited

Loading

simonhong Mar 28, 2018

bbondy Mar 28, 2018

simonhong Mar 28, 2018

simonhong Mar 28, 2018

bbondy Mar 28, 2018

bbondy Mar 28, 2018

bbondy Mar 28, 2018

simonhong Mar 28, 2018 •

edited

Loading

simonhong Mar 28, 2018

bbondy Mar 28, 2018

simonhong left a comment

simonhong Mar 28, 2018

simonhong Mar 28, 2018

simonhong Mar 28, 2018

bbondy commented Mar 28, 2018

Add HTTPSEverywhereService test #74

Add HTTPSEverywhereService test #74

Conversation

bbondy commented Mar 27, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbondy commented Mar 28, 2018

bbondy commented Mar 27, 2018 •

edited

Loading

simonhong Mar 28, 2018 •

edited

Loading