New issue
Advanced search Search tips

Issue 900645 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Remove "blockage indication" storage from TabSpecificContentSettings

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

Issue description

This data is being used to control ContentSettingImageModels. However, this state is not strictly necessary, since all image models that can animate, want to animate when they first appear.

This is much easier implemented without looking at TSCS, and using separate, per-tab state.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 6

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

commit c84a939222b279171ffc2cef7609686e4dfe5f2e
Author: Charlie Harrison <csharrison@chromium.org>
Date: Tue Nov 06 16:25:50 2018

[Android] simplify pop-up infobar creation

Right now popups have a strange relationship with
TabSpecificContentSettings.

Right now: the infobar is opened if:
 - A global NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED is received
 - !IsBlockageIndicated(POPUPS) in TSCS
 - There are blocked popups

At first glance this seems reasonable. However, popups do not abide
by the typical TSCS notion of blockage being indicated. On Android,
we create a new infobar for every new popup blocked. On Desktop, these
new popups do not trigger another animation of the blocked image.

So, TSCS has android specific logic which resets the "indicated" bit
every time popups are marked as blocked.

This CL simplifies this complex interaction by removing it completely.
The PopupBlockerTabHelper is now responsible for the straightforward
creation of the infobar.

This CL has no (%comment) intended behavior change.

Bug:  900645 
Change-Id: I440b9b3b6f8d160f1b4d34f8fb7f566dc7dfb72d
Reviewed-on: https://chromium-review.googlesource.com/c/1308635
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605704}
[modify] https://crrev.com/c84a939222b279171ffc2cef7609686e4dfe5f2e/chrome/browser/android/tab_android.cc
[modify] https://crrev.com/c84a939222b279171ffc2cef7609686e4dfe5f2e/chrome/browser/android/tab_android.h
[modify] https://crrev.com/c84a939222b279171ffc2cef7609686e4dfe5f2e/chrome/browser/content_settings/tab_specific_content_settings.cc
[modify] https://crrev.com/c84a939222b279171ffc2cef7609686e4dfe5f2e/chrome/browser/ui/blocked_content/popup_blocker_tab_helper.cc
[modify] https://crrev.com/c84a939222b279171ffc2cef7609686e4dfe5f2e/chrome/browser/ui/blocked_content/safe_browsing_triggered_popup_blocker_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Nov 12

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

commit 4cf113ad3a70c49a1323dde5462c8d1fae9535cb
Author: Charlie Harrison <csharrison@chromium.org>
Date: Mon Nov 12 21:12:33 2018

Remove "BlockageIndicated" from TabSpecificContentSettings

Image models are not 1:1 with content settings. This CL replaces
per-tab storage from TSCS to a new per-WebContents container,
ContentSettingImageModelState, which contains information that
is 1:1 with image models.

This allows us to remove custom logic for image models that aren't
backed by a single content setting.

The new behavior works like this:
 1. Per-tab state is stored in ContentSettingImageModelState
 2. ShouldRunAnimation / SetAnimationHasRun updates the state
 3. State is additionally updated when an image is hidden

(3) entails a bunch of changes to UpdateFromWebContents, namely,
making it return a boolean for whether the update triggered visibility.

The benefit of this CL is that all of the logic to control animation of
image models come from ContentSettingImageModel.

This CL has no intended user-facing behavior change.

Bug:  900645 
Change-Id: I99c36a8184a0e04e6356e58fbc93a5aed3f03127
Reviewed-on: https://chromium-review.googlesource.com/c/1310674
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607331}
[modify] https://crrev.com/4cf113ad3a70c49a1323dde5462c8d1fae9535cb/chrome/browser/content_settings/tab_specific_content_settings.cc
[modify] https://crrev.com/4cf113ad3a70c49a1323dde5462c8d1fae9535cb/chrome/browser/content_settings/tab_specific_content_settings.h
[modify] https://crrev.com/4cf113ad3a70c49a1323dde5462c8d1fae9535cb/chrome/browser/content_settings/tab_specific_content_settings_unittest.cc
[modify] https://crrev.com/4cf113ad3a70c49a1323dde5462c8d1fae9535cb/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/4cf113ad3a70c49a1323dde5462c8d1fae9535cb/chrome/browser/ui/blocked_content/framebust_block_tab_helper.cc
[modify] https://crrev.com/4cf113ad3a70c49a1323dde5462c8d1fae9535cb/chrome/browser/ui/blocked_content/framebust_block_tab_helper.h
[modify] https://crrev.com/4cf113ad3a70c49a1323dde5462c8d1fae9535cb/chrome/browser/ui/blocked_content/popup_blocker_tab_helper.cc
[modify] https://crrev.com/4cf113ad3a70c49a1323dde5462c8d1fae9535cb/chrome/browser/ui/blocked_content/popup_blocker_tab_helper.h
[modify] https://crrev.com/4cf113ad3a70c49a1323dde5462c8d1fae9535cb/chrome/browser/ui/content_settings/content_setting_image_model.cc
[modify] https://crrev.com/4cf113ad3a70c49a1323dde5462c8d1fae9535cb/chrome/browser/ui/content_settings/content_setting_image_model.h
[add] https://crrev.com/4cf113ad3a70c49a1323dde5462c8d1fae9535cb/chrome/browser/ui/content_settings/content_setting_image_model_states.cc
[add] https://crrev.com/4cf113ad3a70c49a1323dde5462c8d1fae9535cb/chrome/browser/ui/content_settings/content_setting_image_model_states.h
[modify] https://crrev.com/4cf113ad3a70c49a1323dde5462c8d1fae9535cb/chrome/browser/ui/content_settings/content_setting_image_model_unittest.cc
[modify] https://crrev.com/4cf113ad3a70c49a1323dde5462c8d1fae9535cb/chrome/browser/ui/views/location_bar/content_setting_image_view.cc

Status: Fixed (was: Untriaged)

Sign in to add a comment