Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add google analytics polyfill, fixes brave/brave-browser#4402 #2488

Merged
merged 1 commit into from
Jun 19, 2019
Merged

add google analytics polyfill, fixes brave/brave-browser#4402 #2488

merged 1 commit into from
Jun 19, 2019

Conversation

pes10k
Copy link
Contributor

@pes10k pes10k commented May 24, 2019 •
edited
Loading

Fixes: brave/brave-browser#4402

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@pes10k pes10k mentioned this pull request May 24, 2019
Closed
28 tasks
@pes10k pes10k requested review from bbondy, petemill and bridiver and removed request for bbondy May 24, 2019 01:08
bridiver
bridiver reviewed May 24, 2019
View reviewed changes
app/brave_generated_resources.grd
@@ -417,6 +417,7 @@ By installing this extension, you are agreeing to the Google Widevine Terms of U
</message>
</messages>
<includes>
<include name="IDR_BRAVE_GOOGLE_ANALYTICS_POLYFILL" file="resources/js/google_analytics_polyfill.js" type="BINDATA" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbondy do you think it's ok to put it in this directory or should we create a new one for things that aren't copied from chrome?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine

bridiver
bridiver previously approved these changes Jun 16, 2019
View reviewed changes
@pes10k pes10k force-pushed the inject-ubo-google-analytics-code branch from 28fc895 to 4c689be Compare June 16, 2019 23:22
@bridiver bridiver force-pushed the inject-ubo-google-analytics-code branch from 4c689be to 221012a Compare June 17, 2019 00:06
@pes10k pes10k force-pushed the inject-ubo-google-analytics-code branch from 221012a to 54ae442 Compare June 18, 2019 16:52
bbondy
bbondy reviewed Jun 18, 2019
View reviewed changes
browser/net/brave_ad_block_tp_network_delegate_helper.cc
std::string GetGoogleTagManagerPolyfillJS() {
static std::string base64_output;
if (base64_output.length() != 0) {
return base64_output;
}
std::string str = ui::ResourceBundle::GetSharedInstance().GetRawDataResource(
IDR_BRAVE_TAG_MANAGER_POLYFILL).as_string();
base64_output.reserve(180);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these were added as an optimization so the buffer doesn't need to reallocate itself multiple times btw. But it's a micro-optimization so it's ok to get rid of it.

bbondy
bbondy previously approved these changes Jun 18, 2019
View reviewed changes
Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as long as this passes CI this is good to merge. Please do uplift PRs as well after it lands on Nightly.

bsclifton
bsclifton reviewed Jun 18, 2019
View reviewed changes
browser/net/brave_ad_block_tp_network_delegate_helper_unittest.cc Outdated
// Shields up, allow ads, tag manager should NOT get polyfill
ASSERT_FALSE(GetPolyfillForAdBlock(true, true, tab_origin, tag_manager_url, &out_url_spec));
// Shields up, allow ads, tag services should NOT get polyfill
ASSERT_FALSE(GetPolyfillForAdBlock(true, true, tab_origin, tag_services_url, &out_url_spec));
// Shields up, allow ads, normal URL should NOT get polyfill
ASSERT_FALSE(GetPolyfillForAdBlock(true, true, tab_origin, normal_url, &out_url_spec));

// Shields down, allow ads, google analytics should get polyfill
ASSERT_FALSE(GetPolyfillForAdBlock(false, true, tab_origin, google_analytics_url, &out_url_spec));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this correct? shields down... we still want to polyfill? (or typo in comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, yuck, typo. Repushed and edd940b fixes

@pes10k pes10k force-pushed the inject-ubo-google-analytics-code branch from 54ae442 to edd940b Compare June 18, 2019 18:17
bsclifton
bsclifton previously approved these changes Jun 18, 2019
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving - fix was made to issue I saw (and it was already approved) 😄

Pending CI passing; please uplift, per @bbondy comment 😄

@bsclifton bsclifton changed the title add google analytics polyfil, fixes brave/brave-browser#4402 add google analytics polyfill, fixes brave/brave-browser#4402 Jun 18, 2019
@pes10k pes10k force-pushed the inject-ubo-google-analytics-code branch from edd940b to 5dfea22 Compare June 18, 2019 20:43
@pes10k pes10k merged commit a3b4cf5 into master Jun 19, 2019
@pes10k
Copy link
Contributor Author

pes10k commented Jun 19, 2019

From Slack conversation with @bsclifton, going to merge this in, since the issue seems to be in CI, not in the code (after several reviews). Merging and uplifting

brave-builds pushed a commit that referenced this pull request Jun 19, 2019
@brave-browser-releases
Uplift of #2488 (squashed) to dev
019cfd6
brave-builds pushed a commit that referenced this pull request Jun 19, 2019
@brave-browser-releases
Uplift of #2488 (squashed) to beta
c560b95
This was referenced Jun 19, 2019
Merged
Merged
@kjozwiak kjozwiak deleted the inject-ubo-google-analytics-code branch June 19, 2019 14:42
@kjozwiak
Copy link
Member

Reproduced the original issue by loading both theverge.com or overclockers.co.uk using 0.68.60 Chromium: 75.0.3770.90. Verification PASSED on macOS 10.14.5 x64 using the following build:

Brave 0.68.64 Chromium: 75.0.3770.100 (Official Build) nightly (64-bit)
Revision cd0b15c8b6a4e70c44e27f35c37a4029bad3e3b0-refs/branch-heads/3770@{#1033}
OS Mac OS X

You can definitely see a difference with the fix. Both websites mentioned above load a lot faster compared to builds without the fix.

@bsclifton bsclifton added this to the 0.67.x - Dev milestone Jun 23, 2019
@ryanbr ryanbr mentioned this pull request Sep 17, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsclifton bsclifton bsclifton approved these changes

@bbondy bbondy bbondy left review comments

@bridiver bridiver bridiver left review comments

@petemill petemill Awaiting requested review from petemill

Assignees

@pes10k pes10k

Labels
enhancement feature/shields
Projects
None yet
Milestone
0.67.x - Release
Development

Successfully merging this pull request may close these issues.

theverge.com (among other sites) hang/delay before rendering in Brave
5 participants
@pes10k @kjozwiak @bridiver @bbondy @bsclifton