New issue
Advanced search Search tips

Issue 895555 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Unify the redirect blocking + pop-up blocking code

Project Member Reported by csharrison@chromium.org, Oct 15

Issue description

Right now, Chrome has a pop-up blocker and a redirect blocker.

These surfaces are almost identical, and should share most of the same similarity. However, this similarity is _not_ reflected in the code.

I think we can reasonably merge the framebust_block_tab_helper and the popup_blocker_tab_helper into a single blocked_navigation_tab_helper, with two separate instances (and slightly different configuration options).

 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/804659abdf89d2528234201efd66d87681503551

commit 804659abdf89d2528234201efd66d87681503551
Author: Charlie Harrison <csharrison@chromium.org>
Date: Tue Oct 16 16:43:38 2018

Refactor popup policy away from blocked popup management

The popup_blocker_tab_helper is designed to manage blocked popups, which
backs the UI on desktop. Currently it also contains logic for _which_
popups should be blocked.

This CL separates these two functions. The tab helper continues to
manage the list of blocked popups, but the policy decisions are moved
to flat functions in a new file: popup_blocker.{cc,h}.

Additionally, the tab helper stops maintaining ownership of the
safe browsing triggered popup blocker (it still creates it though).
The safe browsing triggered blocker is changed to be a
WebContentsUserData.

Bug: 895555
Change-Id: Ib62d2e2b3e7f80fc8bf9f78158ba5e6106ab0f56
Reviewed-on: https://chromium-review.googlesource.com/c/1281725
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600016}
[modify] https://crrev.com/804659abdf89d2528234201efd66d87681503551/chrome/browser/android/tab_web_contents_delegate_android.cc
[modify] https://crrev.com/804659abdf89d2528234201efd66d87681503551/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/804659abdf89d2528234201efd66d87681503551/chrome/browser/subresource_filter/subresource_filter_abusive_unittest.cc
[modify] https://crrev.com/804659abdf89d2528234201efd66d87681503551/chrome/browser/ui/BUILD.gn
[add] https://crrev.com/804659abdf89d2528234201efd66d87681503551/chrome/browser/ui/blocked_content/popup_blocker.cc
[add] https://crrev.com/804659abdf89d2528234201efd66d87681503551/chrome/browser/ui/blocked_content/popup_blocker.h
[modify] https://crrev.com/804659abdf89d2528234201efd66d87681503551/chrome/browser/ui/blocked_content/popup_blocker_tab_helper.cc
[modify] https://crrev.com/804659abdf89d2528234201efd66d87681503551/chrome/browser/ui/blocked_content/popup_blocker_tab_helper.h
[modify] https://crrev.com/804659abdf89d2528234201efd66d87681503551/chrome/browser/ui/blocked_content/safe_browsing_triggered_popup_blocker.cc
[modify] https://crrev.com/804659abdf89d2528234201efd66d87681503551/chrome/browser/ui/blocked_content/safe_browsing_triggered_popup_blocker.h
[modify] https://crrev.com/804659abdf89d2528234201efd66d87681503551/chrome/browser/ui/blocked_content/safe_browsing_triggered_popup_blocker_unittest.cc
[modify] https://crrev.com/804659abdf89d2528234201efd66d87681503551/chrome/browser/ui/browser.cc
[modify] https://crrev.com/804659abdf89d2528234201efd66d87681503551/chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Oct 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b718554b61b113f78c4fe5d9531931dc98f0a63f

commit b718554b61b113f78c4fe5d9531931dc98f0a63f
Author: Charlie Harrison <csharrison@chromium.org>
Date: Wed Oct 31 15:04:11 2018

Remove Framebust logic from TabSpecificContentSettings

We aren't actually using anything intrinsic to the class, so there is
no point.

Note: This CL also removes a TODO to add _more_ framebust logic
to TabSpecificContentSettings. Since there are multiple UIs that
interact with the POPUP setting, it seems best to have a separate class
deal with that state, and eventually move popup stuff out of there.

Bug: 895555
Change-Id: Ifc1de176525c30972a4a00ac2c90b2c44f339f0b
Reviewed-on: https://chromium-review.googlesource.com/c/1283411
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604254}
[modify] https://crrev.com/b718554b61b113f78c4fe5d9531931dc98f0a63f/chrome/browser/content_settings/tab_specific_content_settings.cc
[modify] https://crrev.com/b718554b61b113f78c4fe5d9531931dc98f0a63f/chrome/browser/content_settings/tab_specific_content_settings.h
[modify] https://crrev.com/b718554b61b113f78c4fe5d9531931dc98f0a63f/chrome/browser/ui/blocked_content/framebust_block_tab_helper.cc
[modify] https://crrev.com/b718554b61b113f78c4fe5d9531931dc98f0a63f/chrome/browser/ui/blocked_content/tab_under_navigation_throttle.cc
[modify] https://crrev.com/b718554b61b113f78c4fe5d9531931dc98f0a63f/chrome/browser/ui/browser.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bb1de6287e02c927e028bb05a34822daafb33cea

commit bb1de6287e02c927e028bb05a34822daafb33cea
Author: Charlie Harrison <csharrison@chromium.org>
Date: Wed Nov 21 13:44:06 2018

Introduce the UrlListManager

This CL introduces the skeleton of a class which can drive the UI
of "blocked URL" lists (i.e. the popup and redirect setting bubbles).

Currently, it only exposes an observer interface to unify how popup
and redirect UI surfaces receive updates about new URLs that were
blocked.

Follow-up CLs can add functionality including storing the actual
blocked URLs within the manager.

Bug: 895555
Change-Id: I6e95cd4ca3c8d6109cbfdb770516dc6d18c98554
Reviewed-on: https://chromium-review.googlesource.com/c/1334420
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610035}
[modify] https://crrev.com/bb1de6287e02c927e028bb05a34822daafb33cea/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/bb1de6287e02c927e028bb05a34822daafb33cea/chrome/browser/ui/blocked_content/framebust_block_tab_helper.cc
[modify] https://crrev.com/bb1de6287e02c927e028bb05a34822daafb33cea/chrome/browser/ui/blocked_content/framebust_block_tab_helper.h
[modify] https://crrev.com/bb1de6287e02c927e028bb05a34822daafb33cea/chrome/browser/ui/blocked_content/popup_blocker_tab_helper.cc
[modify] https://crrev.com/bb1de6287e02c927e028bb05a34822daafb33cea/chrome/browser/ui/blocked_content/popup_blocker_tab_helper.h
[add] https://crrev.com/bb1de6287e02c927e028bb05a34822daafb33cea/chrome/browser/ui/blocked_content/url_list_manager.cc
[add] https://crrev.com/bb1de6287e02c927e028bb05a34822daafb33cea/chrome/browser/ui/blocked_content/url_list_manager.h
[modify] https://crrev.com/bb1de6287e02c927e028bb05a34822daafb33cea/chrome/browser/ui/content_settings/content_setting_bubble_model.cc
[modify] https://crrev.com/bb1de6287e02c927e028bb05a34822daafb33cea/chrome/browser/ui/content_settings/content_setting_bubble_model.h
[modify] https://crrev.com/bb1de6287e02c927e028bb05a34822daafb33cea/chrome/browser/ui/content_settings/framebust_block_browsertest.cc

Status: Assigned (was: Untriaged)
This issue has an owner, a component and a priority, but is still listed as untriaged or unconfirmed. By definition, this bug is triaged. Changing status to "assigned". Please reach out to me if you disagree with how I've done this.

Sign in to add a comment