Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2171713002:
Safe browsing subresource filter bubble UI skeleton. (Closed)
|
Created:
4 years, 5 months ago by melandory Modified:
4 years, 4 months ago CC:
chromium-reviews, markusheintz_, raymes+watch_chromium.org, oshima+watch_chromium.org, msramek+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSafe browsing subresource filter bubble UI skeleton.
After user proceed through Safe Browsing warning interstitials that are
displayed when the site ahead contains deceptive embedded content, we
need to give the user an ability to reload the page with the content
we've blocked previously.
This CL implements the skeleton of the bubble, but doesn't have logic to
triggering the bubble.
BUG=609747
Committed: https://crrev.com/58e4873ad4a9e3072967d0b2b0382e6b20d6791d
Cr-Commit-Position: refs/heads/master@{#408970}
Patch Set 1 #Patch Set 2 : fix mac? && tests #Patch Set 3 : cl format #Patch Set 4 : self review #Patch Set 5 : self review #
Total comments: 12
Patch Set 6 : xib #Patch Set 7 : xib with osx 10.9 #
Total comments: 24
Patch Set 8 : sort #Patch Set 9 : msramek@ comments #
Total comments: 8
Patch Set 10 : msramek@ comments #Patch Set 11 : remove empty line #
Total comments: 12
Patch Set 12 : comments #
Total comments: 2
Patch Set 13 : remove png #Patch Set 14 : Verified xib file #Messages
Total messages: 106 (78 generated)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
melandory@chromium.org changed reviewers: + markusheintz@chromium.org
Markus, please take a look at code in: chrome/browser/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)
raymes@chromium.org changed reviewers: + raymes@chromium.org
Hey melandory - markusheintz probably isn't the right reviewer anymore. Has this had UX input/approval? Will the users decision be stored e.g. if they visit the site again, will they have to reload with the blocked subresources?
raymes@chromium.org changed reviewers: + msramek@chromium.org - markusheintz@chromium.org
A few high level comments/questions. Thanks! https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/content_... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.h:191: void SetSubresourceFilterActivationBeenIndicated() { nit: SetSubresourceBlockageIndicated()? https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1346: // bubble model. nit: I think we can avoid propagating these comments more :) https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1353: ContentSettingSubresourceFilterBubbleModel(Delegate* delegate, Based on the mocks, this bubble seems really similar to the mixed script one. Should we follow the same pattern as that for consistency or are there important differences needed? https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_image_model.cc:533: } Hmm, since we're planning to add a new content setting for this anyway, I wonder if we should just add the content setting now. That would reduce all the code needed here I think? If it doesn't simplify things this is ok.
https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1353: ContentSettingSubresourceFilterBubbleModel(Delegate* delegate, On 2016/07/26 07:19:35, raymes wrote: > Based on the mocks, this bubble seems really similar to the mixed script one. > Should we follow the same pattern as that for consistency or are there important > differences needed? Talked to our UX. Despite the two bubble definitely looks similar, there is the difference which makes them harder to unify. Mixed script bubble has "Learn More" link in the left bottom corner: ____________________________________ Mixed content something, something --------------- |Load full site| --------------- -- Learn More |OK| -- ____________________________________ Out bubble doesn't need this link, but then it will look weird if we have empty space under "Load full site".
https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/content_... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.h:191: void SetSubresourceFilterActivationBeenIndicated() { On 2016/07/26 07:19:35, raymes wrote: > nit: SetSubresourceBlockageIndicated()? Done. https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1346: // bubble model. On 2016/07/26 07:19:35, raymes wrote: > nit: I think we can avoid propagating these comments more :) Done. https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_image_model.cc:533: } On 2016/07/26 07:19:36, raymes wrote: > Hmm, since we're planning to add a new content setting for this anyway, I wonder > if we should just add the content setting now. That would reduce all the code > needed here I think? If it doesn't simplify things this is ok. Main motivation for not having a content setting is "not to persist any data related to the sub-resource filtering". msramek@ agrees here that since currently we need only bubble, it doesn't make sense to create a content setting.
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #7 (id:120001) has been deleted
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I left some comments, mostly about making the code cleaner. The code that you added to TabSpecificContentSettings semantically belongs there, because it has to mirror changes in ImageModel and BubbleModel. But unlike in ImageModel and BubbleModel, TabSpecificContentSettings have not been refactored to naturally support indicators for things that are not content settings. But I'm fine with it for now. Either you add a content setting later and this problem will disappear, or I'll eventually get to refactor TabSpecificContentSettings to better support your use case. https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_image_model.cc:533: } On 2016/07/26 11:58:11, melandory wrote: > On 2016/07/26 07:19:36, raymes wrote: > > Hmm, since we're planning to add a new content setting for this anyway, I > wonder > > if we should just add the content setting now. That would reduce all the code > > needed here I think? If it doesn't simplify things this is ok. > Main motivation for not having a content setting is "not to persist any data > related to the sub-resource filtering". > > msramek@ agrees here that since currently we need only bubble, it doesn't make > sense to create a content setting. Just to add context - the refactoring that I made last year made it possible to create a bubble without a content setting (specifically, it was the media bubble in my case). One of the reasons for that refactoring was exactly the fact that people were adding new content setting types even if they didn't actually need the content setting, only the bubble. It is now possible to have a bubble without a content setting, and so we should encourage that (maybe even move this code out of content_settings/ eventually!). https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/content... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:191: void SetSubresourceBlockageIndicated() { Please define in this in the .cc file like above. https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:247: bool is_subresource_filter_enabled() const { Please make it more obvious that this supplements IsContentBlocked(). https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:251: void set_is_subresource_filter_enabled(bool enabled) { Please make this consistent with SetDownloadsBlocked(), SetPopupsBlocked() etc. https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:483: bool is_subresource_filter_enabled_; nit: the "is" seems superfluous https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:38: #include "components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h" nit: Unused (you just left a TODO) https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1345: return nullptr; nit: Also copy-paste the comment? :) https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1348: // ContentSettingSubresourceFilterBubbleModel ---------------------------------- Please move this higher up. The most general section of this file, describing the base class ContentSettingBubbleModel, was supposed to be the last. Please keep the same ordering as in the .h file. https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_bubble_model.h:48: // ContentSettingSubresourceFilterBubbleModel - deceptive content nit: alignment of the dash https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:87: class ContentSettingSubresourceFilterImageModel Please update the hierarchy comment at the top of this file. https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:447: // TODO(msramek): For bubbles that are not tied to a single content setting, This TODO now applies to ContentSettingSubresourceFilterImageModel as well. Can you please move it to TabSpecificContentSetting, explain that we're abusing it for media *and* subresources now, and that we should be ashamed of that? https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:522: return content_settings && content_settings->is_subresource_filter_enabled(); This should check if the blockage has been *indicated*, not if the content is blocked. https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.h (right): https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.h:69: CreateSubresourceFilterImageModelForTesting(); I think this fundamentally doesn't belong here. We should probably pull out the class declaration here to the .cc file, same way as we did for ContentSettingSimpleImageModel. Basically, we could get away with hiding most class declarations in the .cc file because ContentSettingSimpleImageModel knows how to construct them from ContentSettingsType, but that's not possible here. So I believe your class declaration should be public in the .h file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/content... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:191: void SetSubresourceBlockageIndicated() { On 2016/07/27 09:46:21, msramek wrote: > Please define in this in the .cc file like above. Done. https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:247: bool is_subresource_filter_enabled() const { On 2016/07/27 09:46:21, msramek wrote: > Please make it more obvious that this supplements IsContentBlocked(). Done. https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:251: void set_is_subresource_filter_enabled(bool enabled) { On 2016/07/27 09:46:21, msramek wrote: > Please make this consistent with SetDownloadsBlocked(), SetPopupsBlocked() etc. Done. https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:483: bool is_subresource_filter_enabled_; On 2016/07/27 09:46:21, msramek wrote: > nit: the "is" seems superfluous Done. https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:38: #include "components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h" On 2016/07/27 09:46:21, msramek wrote: > nit: Unused (you just left a TODO) Done. https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1345: return nullptr; On 2016/07/27 09:46:21, msramek wrote: > nit: Also copy-paste the comment? :) https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... I was there :) https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1348: // ContentSettingSubresourceFilterBubbleModel ---------------------------------- On 2016/07/27 09:46:21, msramek wrote: > Please move this higher up. The most general section of this file, describing > the base class ContentSettingBubbleModel, was supposed to be the last. Please > keep the same ordering as in the .h file. Done. https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_bubble_model.h:48: // ContentSettingSubresourceFilterBubbleModel - deceptive content On 2016/07/27 09:46:21, msramek wrote: > nit: alignment of the dash Done. https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:87: class ContentSettingSubresourceFilterImageModel On 2016/07/27 09:46:21, msramek wrote: > Please update the hierarchy comment at the top of this file. Done. https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:447: // TODO(msramek): For bubbles that are not tied to a single content setting, On 2016/07/27 09:46:21, msramek wrote: > This TODO now applies to ContentSettingSubresourceFilterImageModel as well. Can > you please move it to TabSpecificContentSetting, explain that we're abusing it > for media *and* subresources now, and that we should be ashamed of that? Hope that "this is not ideal" expresses relevant level of being ashamed :) https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:522: return content_settings && content_settings->is_subresource_filter_enabled(); On 2016/07/27 09:46:21, msramek wrote: > This should check if the blockage has been *indicated*, not if the content is > blocked. Done. https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.h (right): https://codereview.chromium.org/2171713002/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.h:69: CreateSubresourceFilterImageModelForTesting(); On 2016/07/27 09:46:22, msramek wrote: > I think this fundamentally doesn't belong here. We should probably pull out the > class declaration here to the .cc file, same way as we did for > ContentSettingSimpleImageModel. > > Basically, we could get away with hiding most class declarations in the .cc file > because ContentSettingSimpleImageModel knows how to construct them from > ContentSettingsType, but that's not possible here. So I believe your class > declaration should be public in the .h file. Done.
Thanks. The ordering in BubbleModel is still not consistent, but it seems that it wasn't before either, so I'm not going to ask you to fix that now. TabSpecificContentSettings needs more tidying up. I think what I propose illustrates well that subresource filter is almost-a-content-setting-but-not-really, and it will also make it easier for you to convert it to a content setting if you decide to do so in the future. https://codereview.chromium.org/2171713002/diff/180001/chrome/browser/content... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/2171713002/diff/180001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:42: // |subresource_filter_enabled_| without being tight either to a single content nit: tied https://codereview.chromium.org/2171713002/diff/180001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:198: void SetSubresourceFilteringActivationIndicated(); Why the renaming to "FilteringActivation"? Since the filtering really blocks content (just like the oldest content settings do), I would prefer the original naming with "blocked", "blockage", etc. https://codereview.chromium.org/2171713002/diff/180001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:254: bool IsSubresourceBlocked() const { return subresource_filter_enabled_; } AFAIU, in general a method can be inlined in .cc file only if it's exactly of the form: lower_case_identifier() { return lower_case_identifier_; } But in any case, I'd like to have these defined in the .cc file because the standard methods for content settings are also defined there. Basically, it should be clear that subresource filter behaves the same way as content settings do here, it's just not a content setting. So please follow this ordering and naming conventions. bool SetPopupsBlocked(); bool SetDownloadsBlocked(); bool SetSubresourceBlocked(); <---- bool IsContentBlocked(); bool IsSubresourceBlocked(); <---- bool IsBlockageIndicated(); bool IsSubresourceBlockageIndicated(); <---- bool SetBlockageHasBeenIndicated(); bool SetSubresourceBlockageIndicated(); <---- https://codereview.chromium.org/2171713002/diff/180001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:256: void SetSubresourceFilterEnabledd(bool enabled) { typo: Enabled (single "d")
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)
On 2016/07/27 13:28:27, msramek wrote: > Thanks. > > The ordering in BubbleModel is still not consistent, but it seems that it wasn't > before either, so I'm not going to ask you to fix that now. It's has local consistency now =) > > TabSpecificContentSettings needs more tidying up. I think what I propose > illustrates well that subresource filter is > almost-a-content-setting-but-not-really, and it will also make it easier for you > to convert it to a content setting if you decide to do so in the future. > > https://codereview.chromium.org/2171713002/diff/180001/chrome/browser/content... > File chrome/browser/content_settings/tab_specific_content_settings.h (right): > > https://codereview.chromium.org/2171713002/diff/180001/chrome/browser/content... > chrome/browser/content_settings/tab_specific_content_settings.h:42: // > |subresource_filter_enabled_| without being tight either to a single content > nit: tied > > https://codereview.chromium.org/2171713002/diff/180001/chrome/browser/content... > chrome/browser/content_settings/tab_specific_content_settings.h:198: void > SetSubresourceFilteringActivationIndicated(); > Why the renaming to "FilteringActivation"? Since the filtering really blocks > content (just like the oldest content settings do), I would prefer the original > naming with "blocked", "blockage", etc. > > https://codereview.chromium.org/2171713002/diff/180001/chrome/browser/content... > chrome/browser/content_settings/tab_specific_content_settings.h:254: bool > IsSubresourceBlocked() const { return subresource_filter_enabled_; } > AFAIU, in general a method can be inlined in .cc file only if it's exactly of > the form: > > lower_case_identifier() { return lower_case_identifier_; } > > But in any case, I'd like to have these defined in the .cc file because the > standard methods for content settings are also defined there. > > Basically, it should be clear that subresource filter behaves the same way as > content settings do here, it's just not a content setting. So please follow this > ordering and naming conventions. > > bool SetPopupsBlocked(); > bool SetDownloadsBlocked(); > bool SetSubresourceBlocked(); <---- > > bool IsContentBlocked(); > bool IsSubresourceBlocked(); <---- > > bool IsBlockageIndicated(); > bool IsSubresourceBlockageIndicated(); <---- > > bool SetBlockageHasBeenIndicated(); > bool SetSubresourceBlockageIndicated(); <---- > > https://codereview.chromium.org/2171713002/diff/180001/chrome/browser/content... > chrome/browser/content_settings/tab_specific_content_settings.h:256: void > SetSubresourceFilterEnabledd(bool enabled) { > typo: Enabled (single "d")
Patchset #10 (id:200001) has been deleted
https://codereview.chromium.org/2171713002/diff/180001/chrome/browser/content... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/2171713002/diff/180001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:42: // |subresource_filter_enabled_| without being tight either to a single content On 2016/07/27 13:28:26, msramek wrote: > nit: tied Done. https://codereview.chromium.org/2171713002/diff/180001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:198: void SetSubresourceFilteringActivationIndicated(); On 2016/07/27 13:28:27, msramek wrote: > Why the renaming to "FilteringActivation"? Since the filtering really blocks > content (just like the oldest content settings do), I would prefer the original > naming with "blocked", "blockage", etc. Done. https://codereview.chromium.org/2171713002/diff/180001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:254: bool IsSubresourceBlocked() const { return subresource_filter_enabled_; } On 2016/07/27 13:28:26, msramek wrote: > AFAIU, in general a method can be inlined in .cc file only if it's exactly of > the form: > > lower_case_identifier() { return lower_case_identifier_; } > > But in any case, I'd like to have these defined in the .cc file because the > standard methods for content settings are also defined there. > > Basically, it should be clear that subresource filter behaves the same way as > content settings do here, it's just not a content setting. So please follow this > ordering and naming conventions. > > bool SetPopupsBlocked(); > bool SetDownloadsBlocked(); > bool SetSubresourceBlocked(); <---- Done > > bool IsContentBlocked(); > bool IsSubresourceBlocked(); <---- Done > bool IsBlockageIndicated(); > bool IsSubresourceBlockageIndicated(); <---- Done > > bool SetBlockageHasBeenIndicated(); > bool SetSubresourceBlockageIndicated(); <---- Done https://codereview.chromium.org/2171713002/diff/180001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:256: void SetSubresourceFilterEnabledd(bool enabled) { On 2016/07/27 13:28:27, msramek wrote: > typo: Enabled (single "d") Done.
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #11 (id:240001) has been deleted
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)
c/b/content_settings LGTM c/b/ui/content_settings LGTM (in the sense that I'm not an owner, but I know that code) https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/content... File chrome/browser/content_settings/tab_specific_content_settings.cc (right): https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.cc:275: const { nit: fits on the previous line https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/content... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:189: void SetSubresourceBlocked(bool enabled); nit: Comments for public methods, here and below.
melandory@chromium.org changed reviewers: + bauerb@chromium.org
bauerb@chromium.org: Please review changes in can you please take a look at chrome/browser/ui/*?
LGTM https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/content... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:44: // solution is to use a map<WebContents*, bool> in the Instead of using a map, could you attach this information via content::WebContentsUserData? https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm (right): https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:288: if (model->AsSubresourceFilterBubbleModel()) Doing all of these checks in sequence feels a bit weird. Could you extract this into a method and return a nib path? https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_bubble_model.h:48: // ContentSettingSubresourceFilterBubbleModel - deceptive content Nit: Should this be "filtered subresources"? Here might not be the best place to explain why we're choosing to filter this.
I defer to the others since they have reviewed it already, but I've left my thoughts below. https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1353: ContentSettingSubresourceFilterBubbleModel(Delegate* delegate, On 2016/07/26 09:37:30, melandory wrote: > On 2016/07/26 07:19:35, raymes wrote: > > Based on the mocks, this bubble seems really similar to the mixed script one. > > Should we follow the same pattern as that for consistency or are there > important > > differences needed? > > Talked to our UX. Despite the two bubble definitely looks similar, there is the > difference which makes them harder to unify. Mixed script bubble has "Learn > More" link in the left bottom corner: > ____________________________________ > > Mixed content something, something > > --------------- > |Load full site| > --------------- > -- > Learn More |OK| > -- > ____________________________________ > > Out bubble doesn't need this link, but then it will look weird if we have empty > space under "Load full site". Would we not want to add "learn more" for this too? https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_image_model.cc:533: } On 2016/07/27 09:46:21, msramek wrote: > On 2016/07/26 11:58:11, melandory wrote: > > On 2016/07/26 07:19:36, raymes wrote: > > > Hmm, since we're planning to add a new content setting for this anyway, I > > wonder > > > if we should just add the content setting now. That would reduce all the > code > > > needed here I think? If it doesn't simplify things this is ok. > > Main motivation for not having a content setting is "not to persist any data > > related to the sub-resource filtering". > > > > msramek@ agrees here that since currently we need only bubble, it doesn't make > > sense to create a content setting. > > Just to add context - the refactoring that I made last year made it possible to > create a bubble without a content setting (specifically, it was the media bubble > in my case). One of the reasons for that refactoring was exactly the fact that > people were adding new content setting types even if they didn't actually need > the content setting, only the bubble. It is now possible to have a bubble > without a content setting, and so we should encourage that (maybe even move this > code out of content_settings/ eventually!). The reason I made the suggestion above was because (if I understand correctly) we are planning to add a content setting for this anyway. I'm ok if we don't want to change this for now, but when we add the new content setting in we might be able to simplify this by inheriting from the ContentSettingSimpleImageModel. https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/content... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:196: bool IsSubresourceBlocked() const; Again, we could make several simplifications here if we add the content setting. If we are planning to store the setting eventually, I think that using a content setting will result in less churn. Otherwise we should revisit this when we add the setting.
https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2171713002/diff/80001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1353: ContentSettingSubresourceFilterBubbleModel(Delegate* delegate, I guess not all the page actions have learn more - so maybe it's not important for consistency :)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)
mac_chromium_gyp_rel
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
melandory@chromium.org changed reviewers: + grt@chromium.org
grt@chromium.org: Please review changes in chrome/app https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/content... File chrome/browser/content_settings/tab_specific_content_settings.cc (right): https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.cc:275: const { On 2016/07/27 15:47:40, msramek wrote: > nit: fits on the previous line Done. https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/content... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:44: // solution is to use a map<WebContents*, bool> in the On 2016/07/27 19:27:44, Bernhard Bauer wrote: > Instead of using a map, could you attach this information via > content::WebContentsUserData? Done. https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:189: void SetSubresourceBlocked(bool enabled); On 2016/07/27 15:47:40, msramek wrote: > nit: Comments for public methods, here and below. Done. https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/content... chrome/browser/content_settings/tab_specific_content_settings.h:196: bool IsSubresourceBlocked() const; On 2016/07/28 05:10:58, raymes wrote: > Again, we could make several simplifications here if we add the content setting. > If we are planning to store the setting eventually, I think that using a content > setting will result in less churn. Otherwise we should revisit this when we add > the setting. Added TODO. https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm (right): https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:288: if (model->AsSubresourceFilterBubbleModel()) On 2016/07/27 19:27:44, Bernhard Bauer wrote: > Doing all of these checks in sequence feels a bit weird. Could you extract this > into a method and return a nib path? Done. https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/2171713002/diff/260001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_bubble_model.h:48: // ContentSettingSubresourceFilterBubbleModel - deceptive content On 2016/07/27 19:27:44, Bernhard Bauer wrote: > Nit: Should this be "filtered subresources"? Here might not be the best place to > explain why we're choosing to filter this. Done.
Patchset #12 (id:280001) has been deleted
Patchset #12 (id:300001) has been deleted
melandory@chromium.org changed reviewers: + estade@chromium.org
estade@chromium.org: Please review changes in ui/gfx/
Patchset #12 (id:320001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)
On 2016/07/28 14:07:27, melandory wrote: > mailto:estade@chromium.org: Please review changes in > > ui/gfx/ estade@, grt@ friendly ping
https://codereview.chromium.org/2171713002/diff/340001/chrome/app/theme/theme... File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/2171713002/diff/340001/chrome/app/theme/theme... chrome/app/theme/theme_resources.grd:408: <structure type="chrome_scaled_image" name="IDR_SUBRESOURCE_FILTER_ACTIVE" file="common/subresource_filter_active.png" /> you shouldn't need to add the pngs, as MD is on by default everywhere.
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2171713002/diff/340001/chrome/app/theme/theme... File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/2171713002/diff/340001/chrome/app/theme/theme... chrome/app/theme/theme_resources.grd:408: <structure type="chrome_scaled_image" name="IDR_SUBRESOURCE_FILTER_ACTIVE" file="common/subresource_filter_active.png" /> On 2016/07/29 16:19:55, Evan Stade wrote: > you shouldn't need to add the pngs, as MD is on by default everywhere. Done.
cool, lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)
chrome/app rubberstamp lgtm
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2171713002/#ps360001 (title: "remove png")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by melandory@chromium.org
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org, bauerb@chromium.org, grt@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2171713002/#ps380001 (title: "Verified xib file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #14 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Safe browsing subresource filter bubble UI skeleton. After user proceed through Safe Browsing warning interstitials that are displayed when the site ahead contains deceptive embedded content, we need to give the user an ability to reload the page with the content we've blocked previously. This CL implements the skeleton of the bubble, but doesn't have logic to triggering the bubble. BUG=609747 ========== to ========== Safe browsing subresource filter bubble UI skeleton. After user proceed through Safe Browsing warning interstitials that are displayed when the site ahead contains deceptive embedded content, we need to give the user an ability to reload the page with the content we've blocked previously. This CL implements the skeleton of the bubble, but doesn't have logic to triggering the bubble. BUG=609747 Committed: https://crrev.com/58e4873ad4a9e3072967d0b2b0382e6b20d6791d Cr-Commit-Position: refs/heads/master@{#408970} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/58e4873ad4a9e3072967d0b2b0382e6b20d6791d Cr-Commit-Position: refs/heads/master@{#408970} |
