Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2668833003:
DialogBrowserTest implementation to invoke Content settings bubble dialogs. (Closed)
DescriptionDialogBrowserTest implementation to invoke Content settings bubble dialogs.
There needs to be a way to invoke all the various content settings
bubble dialog views. This wires up the BrowserDialogTest framework in
order to invoke/test them.
BUG=652024
Review-Url: https://codereview.chromium.org/2668833003
Cr-Commit-Position: refs/heads/master@{#450776}
Committed: https://chromium.googlesource.com/chromium/src/+/a5d37aea27ce6ff61d1b7c55c65fe4d7ecc662fe
Patch Set 1 #Patch Set 2 : Temporarily added --disable-gpu when launching subprocess. Filled out all bubble dialog invocation … #
Total comments: 2
Patch Set 3 : content_type() -> GetContentType() #Patch Set 4 : Fixed Cocoa build #
Total comments: 55
Patch Set 5 : Cleanup/changed due to feedback #Patch Set 6 : Merge fix from https://codereview.chromium.org/2663163004. This is temporary #
Total comments: 28
Patch Set 7 : Merged in suggestions from reviewers #
Total comments: 31
Patch Set 8 : Updates from feedback #
Total comments: 32
Patch Set 9 : Fixed nits and added enum element per recommendation. #
Total comments: 5
Patch Set 10 : Build fix for Windows & Linux #Patch Set 11 : Invoke bubble dialog on key release of the space bar #
Total comments: 3
Patch Set 12 : Removed the 'deceptive content' enum element in favor of the new 'subresource filter' element #
Total comments: 9
Patch Set 13 : More nits addressed #Messages
Total messages: 94 (53 generated)
kylixrd@chromium.org changed reviewers: + tapted@chromium.org
I've created this CL for the initial purpose of getting it to actually work. I think I've done enough where the test will operate programatically, however the bubble dialog will not appear when invoked interactively. It *thinks* the bubble is present, because when browser window is clicked, the bubble is canceled and the interactive test terminates. So *why* won't the bubble dialog actually appear? On Windows, when I trace down into the top-level Widget, the underlying HWND thinks it's visible. Thinking that maybe the bubble is behind the browser window, I event tried to force it to the top. All to no avail. I'm clearly missing something here.
On 2017/02/01 19:23:52, kylix_rd wrote: > I've created this CL for the initial purpose of getting it to actually work. I > think I've done enough where the test will operate programatically, however the > bubble dialog will not appear when invoked interactively. It *thinks* the bubble > is present, because when browser window is clicked, the bubble is canceled and > the interactive test terminates. > > So *why* won't the bubble dialog actually appear? On Windows, when I trace down > into the top-level Widget, the underlying HWND thinks it's visible. Thinking > that maybe the bubble is behind the browser window, I event tried to force it to > the top. All to no avail. I'm clearly missing something here. My dialog is invisible too. We figured out it appears if you pass in --disable-gpu. I filed a bug about it here: crbug.com/687387, in case it ends up being the same thing.
On 2017/02/01 20:58:54, Bret Sepulveda wrote: > On 2017/02/01 19:23:52, kylix_rd wrote: > > I've created this CL for the initial purpose of getting it to actually work. I > > think I've done enough where the test will operate programatically, however > the > > bubble dialog will not appear when invoked interactively. It *thinks* the > bubble > > is present, because when browser window is clicked, the bubble is canceled and > > the interactive test terminates. > > > > So *why* won't the bubble dialog actually appear? On Windows, when I trace > down > > into the top-level Widget, the underlying HWND thinks it's visible. Thinking > > that maybe the bubble is behind the browser window, I event tried to force it > to > > the top. All to no avail. I'm clearly missing something here. > > My dialog is invisible too. We figured out it appears if you pass in > --disable-gpu. I filed a bug about it here: crbug.com/687387, in case it ends up > being the same thing. Well, well, well... That makes a big difference! Thanks, Bret! It also solves another annoying issue where the LocationBar and OmniBox tend to jump around.
On 2017/02/01 21:15:36, kylix_rd wrote: > On 2017/02/01 20:58:54, Bret Sepulveda wrote: > > On 2017/02/01 19:23:52, kylix_rd wrote: > > > I've created this CL for the initial purpose of getting it to actually work. > I > > > think I've done enough where the test will operate programatically, however > > the > > > bubble dialog will not appear when invoked interactively. It *thinks* the > > bubble > > > is present, because when browser window is clicked, the bubble is canceled > and > > > the interactive test terminates. > > > > > > So *why* won't the bubble dialog actually appear? On Windows, when I trace > > down > > > into the top-level Widget, the underlying HWND thinks it's visible. Thinking > > > that maybe the bubble is behind the browser window, I event tried to force > it > > to > > > the top. All to no avail. I'm clearly missing something here. > > > > My dialog is invisible too. We figured out it appears if you pass in > > --disable-gpu. I filed a bug about it here: crbug.com/687387, in case it ends > up > > being the same thing. > > Well, well, well... That makes a big difference! Thanks, Bret! > > It also solves another annoying issue where the LocationBar and OmniBox tend to > jump around. Yeah I'm not sure what's going on here with the --disable-gpu thing - there may be a weird bug lurking :/ Also note that to get past tryjobs on desktop linux, some dialogs may also need a tweak to the part of the test harness that finds dialogs -- see http://crbug.com/654151#c14 . Hopefully I'll land a fix soon (needs some investigation why this is needed first). haven't looked at the code here, but ping me again if you want a review from me :)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== DialogBrowserTest implementation to invoke Content settins bubble dialogs. ========== to ========== DialogBrowserTest implementation to invoke Content settings bubble dialogs. ==========
kylixrd@chromium.org changed reviewers: + bauerb@chromium.org, pkasting@chromium.org
kylixrd@chromium.org changed reviewers: - tapted@chromium.org
This patch fleshes out the interactive dialog tests for the various content settings bubbles. tapted@ - Added temporary code under Windows to always add the --disable-gpu switch for the subprocess. bauerb@ - Minor change to the content settings hierarchy to enable easier testing and invocation of the bubble dialogs. pkasting@ - Similar to the content settings changes, changes to LocationBar are to enable easier testing and bubble dialog invocation.
https://codereview.chromium.org/2668833003/diff/40001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_image_model.h (right): https://codereview.chromium.org/2668833003/diff/40001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_image_model.h:69: virtual ContentSettingsType content_type(); Name this method GetContentType()? It seems non-trivial enough (at least with the hierarchy) to not get a unix_hacker_style accessor.
The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run
https://codereview.chromium.org/2668833003/diff/40001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_image_model.h (right): https://codereview.chromium.org/2668833003/diff/40001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_image_model.h:69: virtual ContentSettingsType content_type(); On 2017/02/02 17:33:19, Bernhard Bauer wrote: > Name this method GetContentType()? It seems non-trivial enough (at least with > the hierarchy) to not get a unix_hacker_style accessor. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
content_settings LGTM.
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_...)
kylixrd@chromium.org changed reviewers: + tapted@chromium.org
Patchset #4 (id:80001) has been deleted
tapted@ - It appears that you're on the list of OWNERS for the Cocoa bits... Needed to update those bits to fix the Mac build. PTAL.
The CQ bit was checked by kylixrd@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Can you add a BUG= and a CL description? https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:86: int ContentSettingImageModelCount() override; Just a general comment - not sure there's much to do about it until the Cocoa code is deleted - but this approach (inheriting from a testing interface in release code) is quite unusual. The preferred approach is to implement a `TestApi` which is a friend in release, but otherwise implemented (and compiled) only in test files. (grep the codebase for test_api.h files to see examples - or you can see a simple one I just made in https://codereview.chromium.org/2666523002/ ). There might not be a neat way to jump to this approach until we can assume we are using a particular implementation (i.e. views only). I guess LocationBarTesting has set a precedent already, but it feels quite bad. Just something to think about -- maybe you can scrutinise the places in this CL where test-only code is being added to the release build to see if there are ways to avoid it. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:341: DCHECK(index >= 0 && index < content_setting_decorations_.size()); optional: remove - .at(..) will throw an exception, which is as effective as the DCHECK otherwise (non-optional ;)): remove `index >= 0` since it is a tautology (index is unsigned) and do DCHECK_LT(index, content_setting_decorations_.size()) (we could also make |index| an int - that's closer to style guide recommendations, but it would make it inconsistent with the other methods on the interface) https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:158: GetContentType()); since this was claimed "for testing only" we shouldn't change it here. content_type_, or keep the protected content_type() accessor if a subclass still needs it https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:356: CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA; did `git cl format` do this? (looks different to how it normally format these, but the format config files have been messed up lately) https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:401: nit: remove blank line https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.h (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.h:69: virtual ContentSettingsType GetContentType(); This is adding a lot of plumbing. (and we generally shouldn't rely on a comment saying `ForTestingOnly`, instead we do GetContentTypeForTesting() -- there are presubmits that check for that being used outside of test code -- but it doesn't work for virtual methods. there is also a performance cost converting an accessor into a virtual method). I see that it's tricky with all the subclasses though. But this is cross-platform code, so there's no reason to avoid a TestApi. And I think at least we should try to remove the virtual method. I suggest namespace test { class ContentSettingImageModelTestApi; } class ContentSettingImageModel { .. protected: set_last_content_type_for_test(ContentSettingsType content_type) { last_content_type_for_test_ = content_type; } .. private: friend class test::ContentSettingImageModelTestApi; .. ContentSettingsType last_content_type_for_test_ = /*something invalid */ }; in content_setting_bubble_dialog_browsertest.cc (for now - we can move it later if other tests want it) namespace test { class ContentSettingImageModelTestApi { public: explicit ContentSettingImageModelTestApi(ContentSettingImageModel* model) : model_(model) {} Foo::ContentType last_content_type() { return model_->last_content_type_for_test_; } private: ContentSettingImageModel* model_; }; } // namespace test (then remove the ContentSettingMediaImageModel bits and have other subclasses set last_content_type_for_test_ in their constructor) https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/loca... File chrome/browser/ui/location_bar/location_bar.h (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/loca... chrome/browser/ui/location_bar/location_bar.h:19: class ContentSettingImageModel; nit: sort https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/test... File chrome/browser/ui/test/browser_dialog_browsertest.cc (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/test... chrome/browser/ui/test/browser_dialog_browsertest.cc:72: // http://crbug.com/687387. There was a chromium-dev thread lately about not just citing bugs, but ensuring there is enough context as well. E.g., the comment should mention something like // Under Windows, dialogs (but not the browser window) created in the // spawned browser_test process are invisible for an unknown reason. Pass // --disable-gpu to resolve this for now. See http://crbug.com/687387. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. It's unclear what file this is testing (which is understandable - there's a few involved). But usually it's good to make that link. This is mainly concerned with the bubble, which is content_setting_bubble_contents, so I'd call this file content_setting_bubble_contents_browsertest.cc https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:14: #include "chrome/browser/ui/views/content_setting_bubble_contents.h" Then move this include up to the top and put a blank line after https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:18: #include "chrome/browser/ui/views/location_bar/location_bar_view.h" hum - this is not cross-platform. So I think we can use a test api for LocationBarView. With this #include, it's possible to safely downcast from the LocationBarTesting* to a LocationBarView* and create a test api from it. We don't need to modify LocationBarTesting https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:42: void ShowDialogBubble(ContentSettingsType content_type) { This is too big for an inline method - even in a test. Perhaps a helper method in an anonymous namespace like // Comment. ShowContentSettingBubble(Browser* browser, ContentSettingsType type) (note that a review from pkasting will typically ask for *all* methods in tests to be declared out-of-line unless they are accessors that meet the style guide notes for release code) https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:57: case CONTENT_SETTINGS_TYPE_GEOLOCATION: nit: indent is off https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:62: content_settings->OnContentBlocked(content_type); comment about this? https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:66: LocationBarView* location_bar_view = browser_view->GetLocationBarView(); huh, actually we have a LocationBarView* here too -- definitely no need for `LocationBarTesting` - LocationBarViewTestApi is the way to access LocationBarView internals https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:70: location_bar_view->Update(web_contents); this needs more comments/commentary. E.g. is this the line that reveals the dialog? https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:78: image_view->OnActivate(ui::MouseEvent( Why's this necessary? We typically can't rely on activation to be reliable in browser_tests because there are many tests running in parallel on the test machine -- another test may pop a dialog and deactivate. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:88: const struct _content_settings_values { the name `_content_settings_values` can just be omitted I think https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:92: {std::string("cookies"), CONTENT_SETTINGS_TYPE_COOKIES}, string has an implicit constructor, so the `std::string(` shouldn't be needed https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:109: for (size_t i = 0; i < sizeof(content_settings_values) / there is arraysize in base/macros.h for this, but I think you can just do for (const auto& case : content_settings_values) { if (name == case.name) { ShowDialogBubble(case.content_type); return; } } https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:117: EXPECT_TRUE(false); NOTREACHED() https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_image_view.h (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_image_view.h:22: class ContentSettingBubbleDialogTest; nit: sort (but, also, since they share a namespace, I don't think this forward declare is necessary -- declaring the friend will implicitly forward-declare the class, so I think we can just remove this) - same for location_bar_view.h https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_image_view.h:92: views::BubbleDialogDelegateView* bubble_view() { This shouldn't be necessary here (a private accessor function is kinda odd) -- the test harness can just do image_view->bubble_view_ instead of image_view->bubble_view() https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1046: size_t result = 0; why this change? -- (style guide prefers int in general, but size_t is acceptable when it makes things cleaner) https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1238: LocationBarView::GetContentSettingImageViewFromImageModel( (move to TestApi class) https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1240: for (auto i = content_setting_views_.begin(); for (ContentSettingImageView* view : content_setting_views_) { if (view->content_setting_image_model() == image_model) return view;
https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:86: int ContentSettingImageModelCount() override; On 2017/02/03 00:35:39, tapted wrote: > Just a general comment - not sure there's much to do about it until the Cocoa > code is deleted - but this approach (inheriting from a testing interface in > release code) is quite unusual. The preferred approach is to implement a > `TestApi` which is a friend in release, but otherwise implemented (and compiled) > only in test files. (grep the codebase for test_api.h files to see examples - or > you can see a simple one I just made in > https://codereview.chromium.org/2666523002/ ). > > There might not be a neat way to jump to this approach until we can assume we > are using a particular implementation (i.e. views only). I guess > LocationBarTesting has set a precedent already, but it feels quite bad. > > Just something to think about -- maybe you can scrutinise the places in this CL > where test-only code is being added to the release build to see if there are > ways to avoid it. Note I kinda went back on this in later comments - I think we should be using a TestApi for the toolkit-views LocationBarView* . (The implementations of these methods on LocationBarViewMac seem to be unused in the test, so they shouldn't be added)
Description was changed from ========== DialogBrowserTest implementation to invoke Content settings bubble dialogs. ========== to ========== DialogBrowserTest implementation to invoke Content settings bubble dialogs. There needs to be a way to invoke all the various content settings bubble dialog views. This wires up the BrowserDialogTest framework in order to invoke/test them. BUG=652024 ==========
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
There is more work and discussion needed. This is a checkpoint for most of the undisputed items. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:86: int ContentSettingImageModelCount() override; On 2017/02/03 00:35:39, tapted wrote: > Just a general comment - not sure there's much to do about it until the Cocoa > code is deleted - but this approach (inheriting from a testing interface in > release code) is quite unusual. The preferred approach is to implement a > `TestApi` which is a friend in release, but otherwise implemented (and compiled) > only in test files. (grep the codebase for test_api.h files to see examples - or > you can see a simple one I just made in > https://codereview.chromium.org/2666523002/ ). > > There might not be a neat way to jump to this approach until we can assume we > are using a particular implementation (i.e. views only). I guess > LocationBarTesting has set a precedent already, but it feels quite bad. > > Just something to think about -- maybe you can scrutinise the places in this CL > where test-only code is being added to the release build to see if there are > ways to avoid it. I was just trying to do a minimum of damage while keeping the the existing model intact. If this isn't the right approach, I would suggest a different, more complete, pass is done. Since this is merely there to get the code to compile, this CL should focus on its primary purpose. Revamping the testing APIs seem a little out of scope here. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:341: DCHECK(index >= 0 && index < content_setting_decorations_.size()); On 2017/02/03 00:35:39, tapted wrote: > optional: remove - .at(..) will throw an exception, which is as effective as the > DCHECK Are exceptions OK in the Mac world? > otherwise (non-optional ;)): remove `index >= 0` since it is a tautology (index > is unsigned) and do DCHECK_LT(index, content_setting_decorations_.size()) Ah, right. It was an int originally and I changed it to match the other methods and neglected to update the DCHECK. Good catch. I also didn't know about DCHECK_LT. Thanks. > (we could also make |index| an int - that's closer to style guide > recommendations, but it would make it inconsistent with the other methods on the > interface) Agreed. I'd rather be consistent in this instance. 'size_t' makes more sense anyway. I think there were some discussions about making some changes WRT using size_t for indices. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:356: CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA; On 2017/02/03 00:35:39, tapted wrote: > did `git cl format` do this? (looks different to how it normally format these, > but the format config files have been messed up lately) Yes, 'git cl format' did this. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.h (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.h:69: virtual ContentSettingsType GetContentType(); On 2017/02/03 00:35:39, tapted wrote: > This is adding a lot of plumbing. (and we generally shouldn't rely on a comment > saying `ForTestingOnly`, instead we do GetContentTypeForTesting() -- there are > presubmits that check for that being used outside of test code -- but it doesn't > work for virtual methods. there is also a performance cost converting an > accessor into a virtual method). > > I see that it's tricky with all the subclasses though. But this is > cross-platform code, so there's no reason to avoid a TestApi. And I think at > least we should try to remove the virtual method. I suggest > > namespace test { > class ContentSettingImageModelTestApi; > } > > class ContentSettingImageModel { > .. > protected: > set_last_content_type_for_test(ContentSettingsType content_type) { > last_content_type_for_test_ = content_type; > } > .. > > private: > friend class test::ContentSettingImageModelTestApi; > .. > ContentSettingsType last_content_type_for_test_ = /*something invalid */ > }; > > > in content_setting_bubble_dialog_browsertest.cc (for now - we can move it later > if other tests want it) > > namespace test { > class ContentSettingImageModelTestApi { > public: > explicit ContentSettingImageModelTestApi(ContentSettingImageModel* model) : > model_(model) {} > > Foo::ContentType last_content_type() { return > model_->last_content_type_for_test_; } > > private: > ContentSettingImageModel* model_; > }; > } // namespace test > > > (then remove the ContentSettingMediaImageModel bits and have other subclasses > set last_content_type_for_test_ in their constructor) This seems a little overly complicated for merely getting one value. I would argue that where this code is exercised (in the UI), the performance hit for the added virtual method is negligible. I understand the sentiment here, but I don't think the overhead is enough to warrant all this complexity. In plenty of other cases, yes, but these classes are fairly simple. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/loca... File chrome/browser/ui/location_bar/location_bar.h (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/loca... chrome/browser/ui/location_bar/location_bar.h:19: class ContentSettingImageModel; On 2017/02/03 00:35:39, tapted wrote: > nit: sort Done. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/test... File chrome/browser/ui/test/browser_dialog_browsertest.cc (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/test... chrome/browser/ui/test/browser_dialog_browsertest.cc:72: // http://crbug.com/687387. On 2017/02/03 00:35:39, tapted wrote: > There was a chromium-dev thread lately about not just citing bugs, but ensuring > there is enough context as well. E.g., the comment should mention something like > > // Under Windows, dialogs (but not the browser window) created in the > // spawned browser_test process are invisible for an unknown reason. Pass > // --disable-gpu to resolve this for now. See http://crbug.com/687387. LOL! I just got nailed by my own pet-peeve... ;) Citing a bug reference only without any summary context. Silly me. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/test... chrome/browser/ui/test/browser_dialog_browsertest.cc:72: // http://crbug.com/687387. On 2017/02/03 00:35:39, tapted wrote: > There was a chromium-dev thread lately about not just citing bugs, but ensuring > there is enough context as well. E.g., the comment should mention something like > > // Under Windows, dialogs (but not the browser window) created in the > // spawned browser_test process are invisible for an unknown reason. Pass > // --disable-gpu to resolve this for now. See http://crbug.com/687387. Done. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/02/03 00:35:40, tapted wrote: > It's unclear what file this is testing (which is understandable - there's a few > involved). But usually it's good to make that link. This is mainly concerned > with the bubble, which is content_setting_bubble_contents, so I'd call this file > content_setting_bubble_contents_browsertest.cc You make a good point, however I think it is testing both the content and the invocation for that content. While they do all share the same content framework, each one will exercise different aspects of it. This also ensures that the pathway to actually getting that bubble dialog onto the screen is functional in all cases. I may be mildly redundant in the grand scheme of things, but it is a pathway that I didn't see getting coverage. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:57: case CONTENT_SETTINGS_TYPE_GEOLOCATION: On 2017/02/03 00:35:39, tapted wrote: > nit: indent is off It's odd that 'git cl format' didn't catch this one. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:66: LocationBarView* location_bar_view = browser_view->GetLocationBarView(); On 2017/02/03 00:35:40, tapted wrote: > huh, actually we have a LocationBarView* here too -- definitely no need for > `LocationBarTesting` - LocationBarViewTestApi is the way to access > LocationBarView internals Yes, I should have commented this code. This block is needed in order to work around some silly C++ visibility issues. While LocationBarView is descended from LocationBar, it moves overridden virtuals to private (there should be a compiler warning that indicates a descendant is lowering member visibility). Doing the up-cast will reveal those virtuals. This is a classic case where lowering visibility in a descendant is kind of silly because it doesn't *really* hide anything. This doesn't apply for pure-virtual classes used as 'interfaces', where you want to require the caller to only use the 'interface' to interact with the instance. In the end, another approach would be for the LocationBarView to befriend the ContentSettingBubbleDialogTest class which would then expose GetLocationBarForTesting(), obviating the need for this block. Both techniques work. Which to use is just a matter of preference and style. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:78: image_view->OnActivate(ui::MouseEvent( On 2017/02/03 00:35:40, tapted wrote: > Why's this necessary? > > We typically can't rely on activation to be reliable in browser_tests because > there are many tests running in parallel on the test machine -- another test may > pop a dialog and deactivate. This actually invokes the dialog... In, essentially, the same manner as if the user had clicked or otherwise activated the glyph/text in the omnibox. I'll add a comment to that effect. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:88: const struct _content_settings_values { On 2017/02/03 00:35:39, tapted wrote: > the name `_content_settings_values` can just be omitted I think My "C" roots are showing ;) https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:92: {std::string("cookies"), CONTENT_SETTINGS_TYPE_COOKIES}, On 2017/02/03 00:35:40, tapted wrote: > string has an implicit constructor, so the `std::string(` shouldn't be needed I thought so as well, C++11 complains without the cast. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:109: for (size_t i = 0; i < sizeof(content_settings_values) / On 2017/02/03 00:35:39, tapted wrote: > there is arraysize in base/macros.h for this, but I think you can just do > > > for (const auto& case : content_settings_values) { > if (name == case.name) { > ShowDialogBubble(case.content_type); > return; > } > } Right... Thanks for the reminder. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:117: EXPECT_TRUE(false); On 2017/02/03 00:35:40, tapted wrote: > NOTREACHED() Won't NOTREACHED() terminated the whole test process while this will only fail the current test? Although, if it does land here, there are probably larger issues afoot. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_image_view.h (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_image_view.h:22: class ContentSettingBubbleDialogTest; On 2017/02/03 00:35:40, tapted wrote: > nit: sort (but, also, since they share a namespace, I don't think this forward > declare is necessary -- declaring the friend will implicitly forward-declare the > class, so I think we can just remove this) > > - same for location_bar_view.h Done. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_image_view.h:92: views::BubbleDialogDelegateView* bubble_view() { On 2017/02/03 00:35:40, tapted wrote: > This shouldn't be necessary here (a private accessor function is kinda odd) -- > the test harness can just do image_view->bubble_view_ instead of > image_view->bubble_view() I figured that with the inline method, it would collapse to just 'image_view->bubble_view_' while having a place to explicitly place a comment that it's being accessed by a test. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1046: size_t result = 0; On 2017/02/03 00:35:41, tapted wrote: > why this change? -- (style guide prefers int in general, but size_t is > acceptable when it makes things cleaner) Done.
The CQ bit was checked by kylixrd@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.
Random drive-by https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:341: DCHECK(index >= 0 && index < content_setting_decorations_.size()); On 2017/02/03 18:55:03, kylix_rd wrote: > On 2017/02/03 00:35:39, tapted wrote: > > optional: remove - .at(..) will throw an exception, which is as effective as > the > > DCHECK > > Are exceptions OK in the Mac world? For reference, in .cc files I ask people to avoid .at(), because the difference between it and [] is unclear to most readers, especially in Chromium where exceptions are disabled. [] + DCHECK is more idiomatic. I don't know if Cocoa has a different pattern. > > (we could also make |index| an int - that's closer to style guide > > recommendations, but it would make it inconsistent with the other methods on > the > > interface) > > Agreed. I'd rather be consistent in this instance. 'size_t' makes more sense > anyway. I think there were some discussions about making some changes WRT using > size_t for indices. size_t is not just OK but encouraged per the Google and Chromium style guides. See e.g. https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... . Basically, if size_t is conceptually correct, don't use int. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:117: EXPECT_TRUE(false); On 2017/02/03 18:55:04, kylix_rd wrote: > On 2017/02/03 00:35:40, tapted wrote: > > NOTREACHED() > > Won't NOTREACHED() terminated the whole test process while this will only fail > the current test? Although, if it does land here, there are probably larger > issues afoot. Right, don't use NOTREACHED() in tests for the same reason you want to avoid DCHECK -- it kills the process and messes up cleanup. I believe the idiom you want here is ADD_FAILURE(). https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1046: size_t result = 0; On 2017/02/03 00:35:41, tapted wrote: > why this change? -- (style guide prefers int in general, but size_t is > acceptable when it makes things cleaner) See other comment on style guide not actually preferring size_t in general, but it's definitely weird to change to a size_t here but not in the signature of the function. Changing the function as a whole to return a size_t would be OK, though, and probably in keeping with the other APIs here that take size_ts.
I think I will need to play with this on Monday to see how it can be plumbed through on Mac. LocationBarView and BrowserView are not compiled on Mac, but the content settings bubbles are, so we should be able to display them with DialogBrowserTest. Maybe there is a way with LocationBarViewTestApi (e.g. by compiling a completely distinct implementation for Mac), or maybe we need to extend LocationBarTesting, or maybe we need to cut the LocationBar out of the picture, and invoke the bubble directly, without first making the location bar icon. I'm OK with getting it going on non-Mac first, and I can deal with the Mac weirdness in a follow-up, but there are other issues. The neatest way forward may be to fix these together. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:86: int ContentSettingImageModelCount() override; On 2017/02/03 18:55:03, kylix_rd wrote: > On 2017/02/03 00:35:39, tapted wrote: > > Just a general comment - not sure there's much to do about it until the Cocoa > > code is deleted - but this approach (inheriting from a testing interface in > > release code) is quite unusual. The preferred approach is to implement a > > `TestApi` which is a friend in release, but otherwise implemented (and > compiled) > > only in test files. (grep the codebase for test_api.h files to see examples - > or > > you can see a simple one I just made in > > https://codereview.chromium.org/2666523002/ ). > > > > There might not be a neat way to jump to this approach until we can assume we > > are using a particular implementation (i.e. views only). I guess > > LocationBarTesting has set a precedent already, but it feels quite bad. > > > > Just something to think about -- maybe you can scrutinise the places in this > CL > > where test-only code is being added to the release build to see if there are > > ways to avoid it. > > I was just trying to do a minimum of damage while keeping the the existing model > intact. If this isn't the right approach, I would suggest a different, more > complete, pass is done. Since this is merely there to get the code to compile, > this CL should focus on its primary purpose. Revamping the testing APIs seem a > little out of scope here. We tend not to worry about damage if there's an overall improvement to code health. But maintaining an existing broken model increases the code debt and makes it harder to fix in future. The current model is a violation of https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... . i.e. "Functions used only for testing should be restricted to test-only scenarios either by #ifdefing them appropriately (e.g. #if defined(UNIT_TEST)) or by naming them with a ForTesting suffix. The latter will be checked at presubmit time to ensure they're only called by test files." If, while improving code health, there's sufficient damage that it makes code review hard, then we try to split the CL, and land the cleanup first. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:341: DCHECK(index >= 0 && index < content_setting_decorations_.size()); On 2017/02/03 22:20:41, Peter Kasting wrote: > On 2017/02/03 18:55:03, kylix_rd wrote: > > On 2017/02/03 00:35:39, tapted wrote: > > > optional: remove - .at(..) will throw an exception, which is as effective as > > the > > > DCHECK > > > > Are exceptions OK in the Mac world? > > For reference, in .cc files I ask people to avoid .at(), because the difference > between it and [] is unclear to most readers, especially in Chromium where > exceptions are disabled. [] + DCHECK is more idiomatic. > > I don't know if Cocoa has a different pattern. > > > > (we could also make |index| an int - that's closer to style guide > > > recommendations, but it would make it inconsistent with the other methods on > > the > > > interface) > > > > Agreed. I'd rather be consistent in this instance. 'size_t' makes more sense > > anyway. I think there were some discussions about making some changes WRT > using > > size_t for indices. > > size_t is not just OK but encouraged per the Google and Chromium style guides. > See e.g. > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > . > > Basically, if size_t is conceptually correct, don't use int. Ooh nice! I'm glad we clarify this. go/cppguide and https://google.github.io/styleguide/cppguide.html still say "Of the C integer types, only int should be used. When appropriate, you are welcome to use standard types like size_t and ptrdiff_t.". which always bugged me - I read it as ~"prefer int unless you need size_t". https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:341: DCHECK(index >= 0 && index < content_setting_decorations_.size()); On 2017/02/03 18:55:03, kylix_rd wrote: > On 2017/02/03 00:35:39, tapted wrote: > > optional: remove - .at(..) will throw an exception, which is as effective as > the > > DCHECK > > Are exceptions OK in the Mac world? I think the C++ library is pre-built with exceptions enabled - they don't get turned off there with -fno-exceptions - the code that calls std::terminate() when the exception is unhandled will then kick in since there's no code in Chrome to handle it. (pretty sure this is true everywhere.. but I've seen some exceptions-disabled library blobs shipped for Windows things in the past). (And, annoyingly, on Mac we can't use AppKit without exceptions -- there are some library behaviours that can only be dealt with using exception handlers, so we can't compile with -fno-exceptions the way we do on linux). https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/02/03 18:55:04, kylix_rd wrote: > On 2017/02/03 00:35:40, tapted wrote: > > It's unclear what file this is testing (which is understandable - there's a > few > > involved). But usually it's good to make that link. This is mainly concerned > > with the bubble, which is content_setting_bubble_contents, so I'd call this > file > > content_setting_bubble_contents_browsertest.cc > > You make a good point, however I think it is testing both the content and the > invocation for that content. While they do all share the same content framework, > each one will exercise different aspects of it. This also ensures that the > pathway to actually getting that bubble dialog onto the screen is functional in > all cases. I may be mildly redundant in the grand scheme of things, but it is a > pathway that I didn't see getting coverage. Hm - I think you are right that this particular location icon has some missing coverage for invocation. But there is location_icon_view_interactive_uitest.cc and page_action_image_view_interactive_uitest.cc for testing the invocation of other location bar decorations (note they need to be "interactive" tests, otherwise another test can steal focus away and cause the dialog to dismiss itself). If we don't spin a RunLoop, we may be able to get away with a browser_test instead of an interactive_ui_test, but I don't think we can properly test invocation/dismissal in this file -- DialogBrowserTest isn't designed for that :/ https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:66: LocationBarView* location_bar_view = browser_view->GetLocationBarView(); On 2017/02/03 18:55:03, kylix_rd wrote: > On 2017/02/03 00:35:40, tapted wrote: > > huh, actually we have a LocationBarView* here too -- definitely no need for > > `LocationBarTesting` - LocationBarViewTestApi is the way to access > > LocationBarView internals > > Yes, I should have commented this code. > > This block is needed in order to work around some silly C++ visibility issues. > While LocationBarView is descended from LocationBar, it moves overridden > virtuals to private (there should be a compiler warning that indicates a > descendant is lowering member visibility). Doing the up-cast will reveal those > virtuals. This is a classic case where lowering visibility in a descendant is > kind of silly because it doesn't *really* hide anything. This doesn't apply for > pure-virtual classes used as 'interfaces', where you want to require the caller > to only use the 'interface' to interact with the instance. hehe - I agree with you on this point - I don't think overrides should ever have more restricted visibility, since there are always ways around it. But not everyone agrees on this, and there's no style guideline about it. We used to have implicit_cast<> in base/something.h that made it clear "I'm upcasting to get around visibility weirdness" but it got removed. > > In the end, another approach would be for the LocationBarView to befriend the > ContentSettingBubbleDialogTest class which would then expose > GetLocationBarForTesting(), obviating the need for this block. Generally, it's nicer to `friend` a TestApi class only. There are many `TODO(..): This class has too many friends` littered around the codebase, since they proliferate easily without a TestApi. > Both techniques > work. Which to use is just a matter of preference and style. If we are using LocationBarTesting, I think we should use browser()->window()->GetLocationBar()->GetLocationBarForTesting(); to jump straight to LocationBarTesting. Ideally... we don't #include location_bar_view.h -- that class isn't used on MAC. Then we ensure the LocationBarTesting API is sufficient for testing without access to the LocationBarView* internals (except I don't think we should be adding more to LocationBarTesting, since it's modifying release code). If we are testing LocationBarView* only, then we should use LocationBarViewTestApi (and not mix two different testing APIs). But to get this test running on Mac, we need to avoid LocationBarView* entirely. https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:92: {std::string("cookies"), CONTENT_SETTINGS_TYPE_COOKIES}, On 2017/02/03 18:55:04, kylix_rd wrote: > On 2017/02/03 00:35:40, tapted wrote: > > string has an implicit constructor, so the `std::string(` shouldn't be needed > > I thought so as well, C++11 complains without the cast. och boo ;). I'd suggest making the |name| member a const char* then. It will get implicitly converted when operator== is applied. I think then you can also declare this as `constexpr struct`, which is the preferred way of declaring consts (when it works). https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_image_view.h (right): https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_image_view.h:92: views::BubbleDialogDelegateView* bubble_view() { On 2017/02/03 18:55:04, kylix_rd wrote: > On 2017/02/03 00:35:40, tapted wrote: > > This shouldn't be necessary here (a private accessor function is kinda odd) -- > > the test harness can just do image_view->bubble_view_ instead of > > image_view->bubble_view() > > I figured that with the inline method, it would collapse to just > 'image_view->bubble_view_' while having a place to explicitly place a comment > that it's being accessed by a test. This violates https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right): https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. The CL description needs to follow the guidelines at https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.h (right): https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.h:69: virtual ContentSettingsType GetContentType(); This still violates https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md..., so we should try harder to avoid it. https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc (right): https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:15: #include "chrome/browser/ui/views/frame/browser_view.h" We need to drop this include -- it isn't compiled on Mac (the Browser window is still cocoa -- not views::BrowserView) https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:18: #include "chrome/browser/ui/views/location_bar/location_bar_view.h" I think all these location_bar includes won't work on Mac either -- only "content_setting_bubble_contents.h" will work. https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1238: for (auto view : content_setting_views_) { The guide says we can use auto for "cases where the type doesn't aid in clarity for the reader." - I don't think that's the case here when compared to ContentSettingImageView* (also seeing raw `auto` without `auto*` or `const auto&` also means we have to question whether we are invoking the copy constructor by accident, but that's orthogonal). See http://go/cppguide#auto https://codereview.chromium.org/2668833003/diff/180001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2668833003/diff/180001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:2144: "../browser/ui/views/frame/browser_non_client_frame_view_browsertest_win.cc", The browser test should be added up here -- toolkit-views content settings bubbles are already hooked up on Mac. (but we can't rely on BrowserView or LocationBarView then - see other comments). https://codereview.chromium.org/2668833003/diff/180001/ui/views/test/widget_t... File ui/views/test/widget_test_aura.cc (right): https://codereview.chromium.org/2668833003/diff/180001/ui/views/test/widget_t... ui/views/test/widget_test_aura.cc:169: Widget::GetAllChildWidgets(window->GetRootWindow(), &all_widgets); This shouldn't be landed with this CL -- it needs to be landed with an update to widget_test_unittest.cc as well (or we need to investigate why windows are being parented to the WindowTreeHost's "root" window rather than the "content" window)
https://codereview.chromium.org/2668833003/diff/180001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2668833003/diff/180001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:2144: "../browser/ui/views/frame/browser_non_client_frame_view_browsertest_win.cc", On 2017/02/03 23:07:03, tapted wrote: > The browser test should be added up here -- toolkit-views content settings > bubbles are already hooked up on Mac. (but we can't rely on BrowserView or > LocationBarView then - see other comments). I got it going on Mac with the CL at https://codereview.chromium.org/2675213002/ . You can diff to Patch Set 1 for a diff to the patchset here. A lot less changes in release code, but I did end up modifying LocationBarTesting in order to implement a cross platform `TestContentSettingImagePressed`. I still think that should be moved to a TestApi, but it doesn't have to reach far outside of the current scope of `LocationBarTesting` (I haven't tried it out on a toolkit-views browser yet - there might be something wrong with my LocationBarView::TestContentSettingImagePressed implementation). https://codereview.chromium.org/2668833003/diff/180001/ui/views/test/widget_t... File ui/views/test/widget_test_aura.cc (right): https://codereview.chromium.org/2668833003/diff/180001/ui/views/test/widget_t... ui/views/test/widget_test_aura.cc:169: Widget::GetAllChildWidgets(window->GetRootWindow(), &all_widgets); On 2017/02/03 23:07:03, tapted wrote: > This shouldn't be landed with this CL -- it needs to be landed with an update to > widget_test_unittest.cc as well (or we need to investigate why windows are being > parented to the WindowTreeHost's "root" window rather than the "content" window) (oops - I missed the "this is temporary" note you had. I've updated https://codereview.chromium.org/2663163004 with a test, but I didn't have time today to look into why the parent is unexpected for some dialogs :/)
LGTM https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc (right): https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:33: }; Nit: I think you still want DISALLOW_COPY_AND_ASSIGN even in test classes. https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:36: LocationBarTesting* location_bar) { Nit: All lines of args should be aligned (git cl format will probably solve this and other style issues automatically) https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:39: location_bar->GetContentSettingImageModel(i); Nit: Wrong indentation (should be 4) You're passing an int to a function expecting a size_t, which triggers a (disabled) compiler warning. This will automatically be fixed by using size_t consistently throughout here (see comment in ContentSettingImageModelCount()). https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:68: content_settings->OnContentBlocked(content_type); Nit: It would make me more comfortable to have a break; after this, just in case someone adds another case below in the future (I don't think the style guide mandates that "default" comes last) https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:71: BrowserView::GetBrowserViewForBrowser(browser()); Nit: Nit: Could just inline into next line https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:74: location_bar_view->GetLocationBarForTesting(); Nit: Declare temps as close to first use as possible (or inline below) https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:76: // the Omnibar. Nit: omnibox https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:80: EXPECT_NE(image_model, nullptr); Nit: ([un]expected, actual) for EQ/NE (several places) https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:87: ui::ET_MOUSE_RELEASED, gfx::Point(0, 0), gfx::Point(0, 0), Nit: Just gfx::Point() https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:91: views::Widget* widget = image_view->bubble_view()->GetWidget(); Nit: Could just inline into next line https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:97: std::string name; Nit: Use const char*. This also allows you to declare this as static constexpr. https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_image_view.h (right): https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_image_view.h:50: // For testing. Return this view's ContentSettingImageModel. Nit: Returns (2 places) "For testing" sounds like this must only be used in tests. Is that what you intended to imply? You might want "Useful in tests" at the end of the comment instead, if not, just to sound a bit milder. Whatever you do, be consistent here and on the accessor you add below. https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1101: return content_setting_views_.size(); This is a size_t, so the function should return a size_t (and callers of it should store the results in a size_t, etc.) https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1107: return content_setting_views_.at(index)->content_setting_image_model(); Nit: At least in location_bar code, don't use at(). Use []. (See my previous drive-by comments; I don't speak for what Mac wants to do, but I'd rather not use at() here.) https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1238: for (auto view : content_setting_views_) { On 2017/02/03 23:07:03, tapted wrote: > The guide says we can use auto for "cases where the type doesn't aid in clarity > for the reader." - I don't think that's the case here when compared to > ContentSettingImageView* > > (also seeing raw `auto` without `auto*` or `const auto&` also means we have to > question whether we are invoking the copy constructor by accident, but that's > orthogonal). > > See http://go/cppguide#auto I would use [const] auto* here, personally. I agree that auto* is better than auto, but I think auto* is sufficiently clear here and we don't really need ContentSettingImageView*. That said, I don't care much; I am OK with either route. https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.h:401: ContentSettingImageModel* image_model); Nit: Following convention in this file, place non-overrides above overrides.
This patch implements aspects from tapted@'s CL :https://codereview.chromium.org/2675213002/ It differs in content_setting_image_model.cc where the vector of ContentSettingImageModels and ContentSettingTypes indices are in sync within the same file. https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc (right): https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:33: }; On 2017/02/06 21:14:25, Peter Kasting wrote: > Nit: I think you still want DISALLOW_COPY_AND_ASSIGN even in test classes. Done. https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:68: content_settings->OnContentBlocked(content_type); On 2017/02/06 21:14:25, Peter Kasting wrote: > Nit: It would make me more comfortable to have a break; after this, just in case > someone adds another case below in the future (I don't think the style guide > mandates that "default" comes last) Done. https://codereview.chromium.org/2668833003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:97: std::string name; On 2017/02/06 21:14:24, Peter Kasting wrote: > Nit: Use const char*. This also allows you to declare this as static constexpr. Done.
neat! I like where this is headed :) . Thanks a ton for looping in the Mac fixes. (also nit: don't forget to re-wrap the CL description per https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... ) https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:525: static constexpr ContentSettingsType content_types[] = { nit: Might be your C roots showing again ;). It's less common to use `static` as a storage class specifier in Chromium (trivia: early C++ standards deprecated it, but then they undeprecated it - I think there are cases for templates where you can't have an anonymous namespace, but still want local linkage). this should go up in the anonymous namespace, below kImageDetails[] (and be named kContentTypeIconOrder or similar) Edit: but, see comment below https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:536: CONTENT_SETTINGS_TYPE_DEFAULT, This at least needs a comment, but.. although I used "DEFAULT" in my test code, I'm not sure we can do this here for release code. I didn't fully dive in, but I don't think there's a corresponding ContentSettingsType for ContentSettingSubresourceFilterImageModel (aka "Deceptive Content"). And there's a comment in content_settings_types.h "WARNING: This enum is going to be removed soon." - so I don't think we can just add a DECPETIVE_CONTENT enum value (it's also used for histograms/UMA, so there are other implications with doing that). I _think_ the new way to identify ContentSettingsTypes is "content_settings::ResourceIdentifier" (typedef to std::string), but that may confuse things here. So, my suggestion for this would be something like the following: // Deceptive content doesn't have a corresponding ContentSettingsType, so // provide a new, unique value to identify it in the icon order. constexpr int kDeceptiveContentModelId = CONTENT_SETTINGS_TYPE_DEFAULT - 1; constexpr int kContentTypeIconOrder[] = { CONTENT_SETTINGS_TYPE_COOKIES .. CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC, // Note: also does camera. kDeceptiveContentModelId, ... }; but we may want an OWNER (e.g. raymes@) to run an eye over this Using an existing enum value, such as _DEFAULT is an option too, so long as it's clearly commented (but I'm leaning towards kDeceptiveContentModelId) https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:546: for (auto content_type : content_types) { (with the above, my preference would be auto -> int to be clear we are losing some type safety) https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:566: result.push_back( Comment here, e.g. // All other content settings types use ContentSettingBlockedImageModel. https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:575: size_t ContentSettingImageModel::GetContentSettingImageModelIndexForTesting( I would actually just expose kContentTypeIconOrder in the .h -- (e.g. as a data member, and go back to static const -heh). Then I think this can be implmented in the test file (and doesn't need `ForTesting`). https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:586: return ARRAYSIZE(content_types); ARRAYSIZE -> arraysize (I'm not sure where ARRAYSIZE is defined (libusb? or maybe a windows header)), same above https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/loca... File chrome/browser/ui/location_bar/location_bar.h (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/loca... chrome/browser/ui/location_bar/location_bar.h:132: // Simulates a left mouse pressed on the content setting image at |index|. nit: (didn't get around to updating this in my experiment CL), I would probably say something like "Invokes the content setting image at |index|, displaying the bubble." (i.e. since there may be simpler code if we don't restrict ourselves to generating a fake mouse event) https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:15: #include "chrome/browser/ui/views/frame/browser_view.h" is it possible to drop this? (it isn't linked on Mac) you might just need to substitute #include "chrome/browser/ui/browser_window.h" which is cross-platform https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:30: private: nit: blank line before https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:67: static constexpr struct { nit: no need for static (I think that will reserve space in the data segment, making the binary slightly bigger .. or the compiler could just optimise it all away too since it is constexpr) https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:91: ADD_FAILURE(); nit: maybe comment here like " // Not reached."? I guess I haven't seen this pattern enough to immediately realise that's what it indicates here. https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_image_view.h (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_image_view.h:51: nit: remove blank line https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1231: nit: remove blank line https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.h:257: friend class ContentSettingBubbleDialogTest; (nit - remove?: not sure you still need this) https://codereview.chromium.org/2668833003/diff/200001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:2170: "../browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc", move up to the `if (toolkit_views) block on line 2140 https://codereview.chromium.org/2668833003/diff/200001/ui/views/test/widget_t... File ui/views/test/widget_test_aura.cc (right): https://codereview.chromium.org/2668833003/diff/200001/ui/views/test/widget_t... ui/views/test/widget_test_aura.cc:166: std::vector<aura::Window*> toplevels = GetAllTopLeveLWindows(); This just landed \o/ - r448452
https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:525: static constexpr ContentSettingsType content_types[] = { On 2017/02/07 01:23:31, tapted wrote: > nit: Might be your C roots showing again ;). It's less common to use `static` as > a storage class specifier in Chromium Don't generalize too hard -- see e.g. https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/pF7nAmUQ2uw . Maybe you meant to speak of not using "static" to affect linkage, which is true -- we try to use anonymous namespace for that. But "static" for e.g. static storage duration in a function is common. In this case, the struct is not at function-scope, so I'd agree with you and make it non-static in an anonymous namespace.
Description was changed from ========== DialogBrowserTest implementation to invoke Content settings bubble dialogs. There needs to be a way to invoke all the various content settings bubble dialog views. This wires up the BrowserDialogTest framework in order to invoke/test them. BUG=652024 ========== to ========== DialogBrowserTest implementation to invoke Content settings bubble dialogs. There needs to be a way to invoke all the various content settings bubble dialog views. This wires up the BrowserDialogTest framework in order to invoke/test them. BUG=652024 ==========
Patchset #8 (id:220001) has been deleted
This should address most of the feedback. Will likely need another cleanup pass. https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:536: CONTENT_SETTINGS_TYPE_DEFAULT, On 2017/02/07 01:23:31, tapted wrote: > This at least needs a comment, but.. although I used "DEFAULT" in my test code, > I'm not sure we can do this here for release code. I didn't fully dive in, but I > don't think there's a corresponding ContentSettingsType for > ContentSettingSubresourceFilterImageModel (aka "Deceptive Content"). > > And there's a comment in content_settings_types.h "WARNING: This enum is going > to be removed soon." - so I don't think we can just add a DECPETIVE_CONTENT enum > value (it's also used for histograms/UMA, so there are other implications with > doing that). > > I _think_ the new way to identify ContentSettingsTypes is > "content_settings::ResourceIdentifier" (typedef to std::string), but that may > confuse things here. > > > So, my suggestion for this would be something like the following: > > // Deceptive content doesn't have a corresponding ContentSettingsType, so > // provide a new, unique value to identify it in the icon order. > constexpr int kDeceptiveContentModelId = CONTENT_SETTINGS_TYPE_DEFAULT - 1; > constexpr int kContentTypeIconOrder[] = { > CONTENT_SETTINGS_TYPE_COOKIES > .. > CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC, // Note: also does camera. > kDeceptiveContentModelId, > ... > }; I started down this path, but it's going to require some ugly casting between ContentSettingType and int. At least the compiler isn't treating the enum as an int here. > but we may want an OWNER (e.g. raymes@) to run an eye over this > > Using an existing enum value, such as _DEFAULT is an option too, so long as it's > clearly commented (but I'm leaning towards kDeceptiveContentModelId) https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:566: result.push_back( On 2017/02/07 01:23:31, tapted wrote: > Comment here, e.g. > > // All other content settings types use ContentSettingBlockedImageModel. Done. https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:575: size_t ContentSettingImageModel::GetContentSettingImageModelIndexForTesting( On 2017/02/07 01:23:31, tapted wrote: > I would actually just expose kContentTypeIconOrder in the .h -- (e.g. as a data > member, and go back to static const -heh). Then I think this can be implmented > in the test file (and doesn't need `ForTesting`). Hmmm... I think keeping it internal and private is better here. This static method shouldn't get linked in except for the tests since it's not referenced in the release code. https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:586: return ARRAYSIZE(content_types); On 2017/02/07 01:23:31, tapted wrote: > ARRAYSIZE -> arraysize (I'm not sure where ARRAYSIZE is defined (libusb? or > maybe a windows header)), same above Done. https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:15: #include "chrome/browser/ui/views/frame/browser_view.h" On 2017/02/07 01:23:31, tapted wrote: > is it possible to drop this? (it isn't linked on Mac) > > you might just need to substitute #include "chrome/browser/ui/browser_window.h" > which is cross-platform Done. https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:30: private: On 2017/02/07 01:23:31, tapted wrote: > nit: blank line before Done. https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.h:257: friend class ContentSettingBubbleDialogTest; On 2017/02/07 01:23:32, tapted wrote: > (nit - remove?: not sure you still need this) GetLocationBarForTesting() is private, so this is needed.
The CQ bit was checked by kylixrd@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
lgtm with a few nits (assuming there's no fallout..), but please wait for a c/b/ui/ OWNER to lg as well. (also note it's often helpful to upload a distinct patchset whenever you need to rebase/merge, with just the changes for the rebase -- this helps when reviewing the `diff`. Howeve,r the additional changes in this case were pretty easy to ignore). Thanks! https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:575: size_t ContentSettingImageModel::GetContentSettingImageModelIndexForTesting( On 2017/02/07 18:25:21, kylix_rd wrote: > On 2017/02/07 01:23:31, tapted wrote: > > I would actually just expose kContentTypeIconOrder in the .h -- (e.g. as a > data > > member, and go back to static const -heh). Then I think this can be implmented > > in the test file (and doesn't need `ForTesting`). > > Hmmm... I think keeping it internal and private is better here. This static > method shouldn't get linked in except for the tests since it's not referenced in > the release code. Although this is likely true, I've never seen it used as justification for putting a method in the release build rather than test code. I think the "don't call ForTesting methods" is enforced via a python presubmit, so I've never really trusted it. I don't see much wrong with exposing kContentTypeIconOrder over exposing GetContentSettingImageModelIndexForTesting(). kContentTypeIconOrder is a constant, so it's hard to abuse. I won't push on this, but an OWNER might (and I am not an OWNER here ;) https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/loca... File chrome/browser/ui/location_bar/location_bar.h (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/loca... chrome/browser/ui/location_bar/location_bar.h:132: // Simulates a left mouse pressed on the content setting image at |index|. On 2017/02/07 01:23:31, tapted wrote: > nit: (didn't get around to updating this in my experiment CL), I would probably > say something like "Invokes the content setting image at |index|, displaying the > bubble." (i.e. since there may be simpler code if we don't restrict ourselves to > generating a fake mouse event) ping? https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.h:257: friend class ContentSettingBubbleDialogTest; On 2017/02/07 18:25:21, kylix_rd wrote: > On 2017/02/07 01:23:32, tapted wrote: > > (nit - remove?: not sure you still need this) > > GetLocationBarForTesting() is private, so this is needed. True, it's private in LocationBarView, but browser()->window()->GetLocationBar()->GetLocationBarForTesting(); should be calling the method on LocationBar, where the method is public :) https://codereview.chromium.org/2668833003/diff/200001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:2170: "../browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc", On 2017/02/07 01:23:32, tapted wrote: > move up to the `if (toolkit_views) block on line 2140 ping? https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:569: // All other content settins types use ContentSettingBlockedImageModel settins -> settings (and full stop at end) https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:571: base::MakeUnique<ContentSettingBlockedImageModel>(icon)); my preference would be a static cast here. Casting "out of range" values to an enum can result in undefined behavior (see item (8) of http://en.cppreference.com/w/cpp/language/static_cast) I would also suggest adding DCHECK(icon >= CONTENT_SETTINGS_TYPE_COOKIES && icon <= CONTENT_SETTINGS_TYPE_MIDI_SYSEX);
On 2017/02/08 00:31:50, tapted wrote: > lgtm with a few nits (assuming there's no fallout..), but please wait for a > c/b/ui/ OWNER to lg as well. I had LGTMed the location_bar changes. Do I need to review the other c/b/ui/ stuff?
On 2017/02/08 00:33:06, Peter Kasting wrote: > On 2017/02/08 00:31:50, tapted wrote: > > lgtm with a few nits (assuming there's no fallout..), but please wait for a > > c/b/ui/ OWNER to lg as well. > > I had LGTMed the location_bar changes. Do I need to review the other c/b/ui/ > stuff? There's some new stuff under chrome/browser/ui/content_settings/ which I'm not certain about. I think you or a content_settings OWNER should take a look
https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:336: if (index >= content_setting_decorations_.size()) Nit: Should this just DCHECK_LT? https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:344: content_setting_decorations_[index]->OnMousePressed(frame, NSZeroPoint); Nit: Can this just use |decoration|? https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (left): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:95: Nit: Don't remove this https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:133: static_cast<int>(CONTENT_SETTINGS_TYPE_DEFAULT) - 1); This seems dangerous, because it's easy to reorder the original enum so DEFAULT is not -1 or something, but rather has a valid value just before it. It would probably be better to have the enum explicitly declare this signal value as -2. It's weird to have this in the enum, but DEFAULT is already weird, and if clients are going to be passing values as ContentSettingsTypes that have meaning, they should really be explicitly listed in the original enum and updated there. https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:552: result.push_back( Nit: Might make sense to do: std::unique_ptr<ContentSettingImageModel> model; switch (icon) { case XXX: model = base::MakeUnique<...>; ... } result.push_back(std::move(model)); Due to better wrapping this might end up slightly shorter, and either way it duplicates the least logic. https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:571: base::MakeUnique<ContentSettingBlockedImageModel>(icon)); On 2017/02/08 00:31:49, tapted wrote: > my preference would be a static cast here. Casting what? |icon| is already of the type the ContentSettingBlockedImageModel expects. > I would also suggest adding > > DCHECK(icon >= CONTENT_SETTINGS_TYPE_COOKIES && icon <= > CONTENT_SETTINGS_TYPE_MIDI_SYSEX); Note: If you do this, do it as two DCHECK_GE/LEs https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:581: size_t result = 0; Why does this temp exist? Why not just return |i| directly in the loop below? https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.h (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.h:35: // ContentSettingsType. For Testing. Nit: testing https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/loca... File chrome/browser/ui/location_bar/location_bar.h (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/loca... chrome/browser/ui/location_bar/location_bar.h:132: // Simulates a left mouse pressed on the content setting image at |index|. Nit: pressed -> press https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/test... File chrome/browser/ui/test/browser_dialog_browsertest.cc (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/test... chrome/browser/ui/test/browser_dialog_browsertest.cc:74: // http://crbug.com/687387. Check my last comment on that bug to ensure this is still a problem before adding this. https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:29: void ShowDialog(const std::string& name) override; Nit: I'd place a blank line between non-overrides and overrides, and probably give a "// DialogBrowserTest:" comment above overrides, to keep the two blocks clear. https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:92: // Not reached. Nit: Comment unnecessary (if this needs explanation, then maybe instead do something like ADD_FAILURE() << "Unknown dialog type";). https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1083: views::View* image_view = content_setting_views_[index]; Nit: Could just inline into next line https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1085: ui::KeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_SPACE, ui::EF_NONE)); Nit: Wrong indentation (should be 4) (git cl format could probably do this automatically)
https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:571: base::MakeUnique<ContentSettingBlockedImageModel>(icon)); On 2017/02/08 01:00:03, Peter Kasting wrote: > On 2017/02/08 00:31:49, tapted wrote: > > my preference would be a static cast here. > > Casting what? |icon| is already of the type the ContentSettingBlockedImageModel > expects. An option would be to declare kContentTypeIconOrder[] as int[], that avoids the static_cast from a calculated int above, but would require a static cast here. (the ContentSettingsType enum is used for UMA and is on the chopping block according to comments in the header, so adding a new enum value to handle "deceptive content" may have other downsides.)
https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:571: base::MakeUnique<ContentSettingBlockedImageModel>(icon)); On 2017/02/08 01:20:19, tapted wrote: > On 2017/02/08 01:00:03, Peter Kasting wrote: > > On 2017/02/08 00:31:49, tapted wrote: > > > my preference would be a static cast here. > > > > Casting what? |icon| is already of the type the > ContentSettingBlockedImageModel > > expects. > > An option would be to declare kContentTypeIconOrder[] as int[], that avoids the > static_cast from a calculated int above, but would require a static cast here. > (the ContentSettingsType enum is used for UMA and is on the chopping block > according to comments in the header, so adding a new enum value to handle > "deceptive content" may have other downsides.) I think we're OK if the new value is -2 -- that won't need any histogram changes. I think adding the value to the enum is probably a better route than using an int.
Patchset #9 (id:260001) has been deleted
bauerb@ An element was added to the ContentSettingsType enum per pkasting@'s recommendation. Please verify & review. This should address all the nits and comments in one manner or another. https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:336: if (index >= content_setting_decorations_.size()) On 2017/02/08 01:00:03, Peter Kasting (OOO Feb. 8-9) wrote: > Nit: Should this just DCHECK_LT? I don't think so... since it's called by a test, the test will check the result and then fail just that test instead of the DCHECK killing the process https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:344: content_setting_decorations_[index]->OnMousePressed(frame, NSZeroPoint); On 2017/02/08 01:00:03, Peter Kasting (OOO Feb. 8-9) wrote: > Nit: Can this just use |decoration|? Seems like it should. I just lifted this code from tapted@. https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (left): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:95: On 2017/02/08 01:00:03, Peter Kasting (OOO Feb. 8-9) wrote: > Nit: Don't remove this Done. https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:552: result.push_back( On 2017/02/08 01:00:03, Peter Kasting (OOO Feb. 8-9) wrote: > Nit: Might make sense to do: > > std::unique_ptr<ContentSettingImageModel> model; > switch (icon) { > case XXX: > model = base::MakeUnique<...>; > ... > } > result.push_back(std::move(model)); > > Due to better wrapping this might end up slightly shorter, and either way it > duplicates the least logic. Done. https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:571: base::MakeUnique<ContentSettingBlockedImageModel>(icon)); On 2017/02/08 01:26:19, Peter Kasting (OOO Feb. 8-9) wrote: > On 2017/02/08 01:20:19, tapted wrote: > > On 2017/02/08 01:00:03, Peter Kasting wrote: > > > On 2017/02/08 00:31:49, tapted wrote: > > > > my preference would be a static cast here. > > > > > > Casting what? |icon| is already of the type the > > ContentSettingBlockedImageModel > > > expects. > > > > An option would be to declare kContentTypeIconOrder[] as int[], that avoids > the > > static_cast from a calculated int above, but would require a static cast here. > > (the ContentSettingsType enum is used for UMA and is on the chopping block > > according to comments in the header, so adding a new enum value to handle > > "deceptive content" may have other downsides.) > > I think we're OK if the new value is -2 -- that won't need any histogram > changes. > > I think adding the value to the enum is probably a better route than using an > int. Done. Added value to the enum. https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:581: size_t result = 0; On 2017/02/08 01:00:03, Peter Kasting (OOO Feb. 8-9) wrote: > Why does this temp exist? Why not just return |i| directly in the loop below? Done. https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.h (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.h:35: // ContentSettingsType. For Testing. On 2017/02/08 01:00:03, Peter Kasting (OOO Feb. 8-9) wrote: > Nit: testing Done. https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/loca... File chrome/browser/ui/location_bar/location_bar.h (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/loca... chrome/browser/ui/location_bar/location_bar.h:132: // Simulates a left mouse pressed on the content setting image at |index|. On 2017/02/08 01:00:03, Peter Kasting (OOO Feb. 8-9) wrote: > Nit: pressed -> press Done. https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/test... File chrome/browser/ui/test/browser_dialog_browsertest.cc (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/test... chrome/browser/ui/test/browser_dialog_browsertest.cc:74: // http://crbug.com/687387. On 2017/02/08 01:00:03, Peter Kasting (OOO Feb. 8-9) wrote: > Check my last comment on that bug to ensure this is still a problem before > adding this. There is another issue that bsep@ will also be addressing, regarding the other requirement of using --single-process. AFAICT, it's still happening. https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:29: void ShowDialog(const std::string& name) override; On 2017/02/08 01:00:03, Peter Kasting (OOO Feb. 8-9) wrote: > Nit: I'd place a blank line between non-overrides and overrides, and probably > give a "// DialogBrowserTest:" comment above overrides, to keep the two blocks > clear. Done. https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc:92: // Not reached. On 2017/02/08 01:00:03, Peter Kasting (OOO Feb. 8-9) wrote: > Nit: Comment unnecessary (if this needs explanation, then maybe instead do > something like ADD_FAILURE() << "Unknown dialog type";). Done. https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1083: views::View* image_view = content_setting_views_[index]; On 2017/02/08 01:00:03, Peter Kasting (OOO Feb. 8-9) wrote: > Nit: Could just inline into next line Done. https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1085: ui::KeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_SPACE, ui::EF_NONE)); On 2017/02/08 01:00:03, Peter Kasting (OOO Feb. 8-9) wrote: > Nit: Wrong indentation (should be 4) (git cl format could probably do this > automatically) Done.
The CQ bit was checked by kylixrd@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run
Another patch to fix a build problem on Windows & Linux. See comment about the reverted change. https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2668833003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1083: views::View* image_view = content_setting_views_[index]; On 2017/02/10 22:47:48, kylix_rd wrote: > On 2017/02/08 01:00:03, Peter Kasting (OOO Feb. 8-9) wrote: > > Nit: Could just inline into next line > > Done. Reverted. OnKeyPressed is moved to the protected section in the descendant. This implicit up-cast will expose the method.
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm
The CQ bit was checked by kylixrd@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2668833003/diff/280001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2668833003/diff/280001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:142: CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC, // Note: also does camera. Tiny nit: Since the actual icon this currently uses is a camera icon, it might be easier to understand how the UI maps to the code if this is _CAMERA, and it "also handles mic" (with matches changes below). https://codereview.chromium.org/2668833003/diff/280001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:580: for (size_t i = 0; i < arraysize(kContentTypeIconOrder); ++i) Nit: {} since body is more than one physical line https://codereview.chromium.org/2668833003/diff/320001/components/content_set... File components/content_settings/core/common/content_settings_types.h (right): https://codereview.chromium.org/2668833003/diff/320001/components/content_set... components/content_settings/core/common/content_settings_types.h:56: CONTENT_SETTINGS_TYPE_DECEPTIVE_CONTENT, I thought the plan was for this to have the value -2 so it was clear it wasn't actually a real content settings type and we didn't need histogram changes? Or should we just consider this a "real" content setting type and add a histogram entry for it? It seems like that's what other things are doing. Also, it's super confusing that there's a CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER already, but it's _not_ tied to ContentSettingSubresourceFilterImageModel. Maybe the latter should be s/SubresourceFilter/DeceptiveContent/? That could happen in this CL or elsewhere.
https://codereview.chromium.org/2668833003/diff/320001/components/content_set... File components/content_settings/core/common/content_settings_types.h (right): https://codereview.chromium.org/2668833003/diff/320001/components/content_set... components/content_settings/core/common/content_settings_types.h:56: CONTENT_SETTINGS_TYPE_DECEPTIVE_CONTENT, On 2017/02/13 23:10:50, Peter Kasting wrote: > I thought the plan was for this to have the value -2 so it was clear it wasn't > actually a real content settings type and we didn't need histogram changes? > > Or should we just consider this a "real" content setting type and add a > histogram entry for it? It seems like that's what other things are doing. > > Also, it's super confusing that there's a > CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER already, but it's _not_ tied to > ContentSettingSubresourceFilterImageModel. Maybe the latter should be > s/SubresourceFilter/DeceptiveContent/? That could happen in this CL or > elsewhere. CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER actually only appeared last week (http://crrev.com/448969). Conveniently, it's exactly what we need - I think we can use it for our purposes in this CL :)
Now using the CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER element. Removed CONTENT_SETTINGS_TYPE_DECEPTIVE_CONTENT element. Hopefully this should address everything. https://codereview.chromium.org/2668833003/diff/280001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2668833003/diff/280001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:142: CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC, // Note: also does camera. On 2017/02/13 23:10:50, Peter Kasting wrote: > Tiny nit: Since the actual icon this currently uses is a camera icon, it might > be easier to understand how the UI maps to the code if this is _CAMERA, and it > "also handles mic" (with matches changes below). Done. https://codereview.chromium.org/2668833003/diff/280001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:580: for (size_t i = 0; i < arraysize(kContentTypeIconOrder); ++i) On 2017/02/13 23:10:50, Peter Kasting wrote: > Nit: {} since body is more than one physical line Done. https://codereview.chromium.org/2668833003/diff/320001/components/content_set... File components/content_settings/core/common/content_settings_types.h (right): https://codereview.chromium.org/2668833003/diff/320001/components/content_set... components/content_settings/core/common/content_settings_types.h:56: CONTENT_SETTINGS_TYPE_DECEPTIVE_CONTENT, On 2017/02/13 23:49:46, tapted wrote: > On 2017/02/13 23:10:50, Peter Kasting wrote: > > I thought the plan was for this to have the value -2 so it was clear it wasn't > > actually a real content settings type and we didn't need histogram changes? > > > > Or should we just consider this a "real" content setting type and add a > > histogram entry for it? It seems like that's what other things are doing. > > > > Also, it's super confusing that there's a > > CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER already, but it's _not_ tied to > > ContentSettingSubresourceFilterImageModel. Maybe the latter should be > > s/SubresourceFilter/DeceptiveContent/? That could happen in this CL or > > elsewhere. > > CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER actually only appeared last week > (http://crrev.com/448969). Conveniently, it's exactly what we need - I think we > can use it for our purposes in this CL :) Done.
The CQ bit was checked by kylixrd@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.
LGTM https://codereview.chromium.org/2668833003/diff/280001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/2668833003/diff/280001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_image_model.cc:95: Nit: Don't remove this https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1082: // to the protected section. My instinct is that we should just make that public again and then go with the inlined, no-comment version. This is OK too.
lgtm - just a few minor things from earlier comments. I've copied on to the latest patchset so they're easier to find. https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/loca... File chrome/browser/ui/location_bar/location_bar.h (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/loca... chrome/browser/ui/location_bar/location_bar.h:132: // Simulates a left mouse pressed on the content setting image at |index|. On 2017/02/08 00:31:49, tapted wrote: > On 2017/02/07 01:23:31, tapted wrote: > > nit: (didn't get around to updating this in my experiment CL), I would > probably > > say something like "Invokes the content setting image at |index|, displaying > the > > bubble." (i.e. since there may be simpler code if we don't restrict ourselves > to > > generating a fake mouse event) > > ping? ping? https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.h:257: friend class ContentSettingBubbleDialogTest; On 2017/02/08 00:31:49, tapted wrote: > On 2017/02/07 18:25:21, kylix_rd wrote: > > On 2017/02/07 01:23:32, tapted wrote: > > > (nit - remove?: not sure you still need this) > > > > GetLocationBarForTesting() is private, so this is needed. > > True, it's private in LocationBarView, but > > browser()->window()->GetLocationBar()->GetLocationBarForTesting(); > > should be calling the method on LocationBar, where the method is public :) ping? https://codereview.chromium.org/2668833003/diff/200001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2668833003/diff/200001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:2170: "../browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc", On 2017/02/08 00:31:49, tapted wrote: > On 2017/02/07 01:23:32, tapted wrote: > > move up to the `if (toolkit_views) block on line 2140 > > ping? ping? https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/loca... File chrome/browser/ui/location_bar/location_bar.h (right): https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/loca... chrome/browser/ui/location_bar/location_bar.h:132: // Simulates a left mouse press on the content setting image at |index|. (update comment) https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.h:249: friend class ContentSettingBubbleDialogTest; (still fairly certain this isn't necessary) https://codereview.chromium.org/2668833003/diff/340001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2668833003/diff/340001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:2167: "../browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc", (it would be nice to move this up into the sources list in `if (toolkit_views)` so there is bot coverage on Mac - I'm happy to do that in a follow-up if issues arise).
The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run
Once all the try-bots finish, I'll commit this. https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/loca... File chrome/browser/ui/location_bar/location_bar.h (right): https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/loca... chrome/browser/ui/location_bar/location_bar.h:132: // Simulates a left mouse press on the content setting image at |index|. On 2017/02/15 01:44:25, tapted wrote: > (update comment) Done. https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1082: // to the protected section. On 2017/02/15 01:14:01, Peter Kasting wrote: > My instinct is that we should just make that public again and then go with the > inlined, no-comment version. Yes, true. However, moving overrides to private seem to be all the rage in many places I've looked. IMO, if anything is done about that, it should be a separate refactoring pass. > This is OK too. https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.h:249: friend class ContentSettingBubbleDialogTest; On 2017/02/15 01:44:25, tapted wrote: > (still fairly certain this isn't necessary) Done. https://codereview.chromium.org/2668833003/diff/340001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2668833003/diff/340001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:2167: "../browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc", On 2017/02/15 01:44:25, tapted wrote: > (it would be nice to move this up into the sources list in `if (toolkit_views)` > so there is bot coverage on Mac - I'm happy to do that in a follow-up if issues > arise). Done.
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 kylixrd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, tapted@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2668833003/#ps360001 (title: "More nits addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1082: // to the protected section. On 2017/02/15 16:42:59, kylix_rd wrote: > On 2017/02/15 01:14:01, Peter Kasting wrote: > > My instinct is that we should just make that public again and then go with the > > inlined, no-comment version. > > Yes, true. However, moving overrides to private seem to be all the rage in many > places I've looked. IMO, if anything is done about that, it should be a separate > refactoring pass. I'm usually responsible for pushing for that -- my default policy is "make overrides as private as possible". I wasn't thinking so much of reversing that on a policy level, as simply saying "for this class, someone external needs to access this" and having that be reasonable justification for making it public.
On 2017/02/15 19:11:40, Peter Kasting wrote: > https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/view... > File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): > > https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/location_bar_view.cc:1082: // to the > protected section. > On 2017/02/15 16:42:59, kylix_rd wrote: > > On 2017/02/15 01:14:01, Peter Kasting wrote: > > > My instinct is that we should just make that public again and then go with > the > > > inlined, no-comment version. > > > > Yes, true. However, moving overrides to private seem to be all the rage in > many > > places I've looked. IMO, if anything is done about that, it should be a > separate > > refactoring pass. > > I'm usually responsible for pushing for that -- my default policy is "make > overrides as private as possible". I wasn't thinking so much of reversing that > on a policy level, as simply saying "for this class, someone external needs to > access this" and having that be reasonable justification for making it public. I guess this is where my framework philosophy would kick in... I don't like having a lot of *public* virtual methods. Virtual methods should, in general, be protected and overidden as protected (or private if the class is then "sealed"). Even if the public, non-virtual method is merely a simple call to the virtual method (and is inlined as a result), it still helps with the evolvability of the framework by keeping a more stable public facing API without injecting heavy contracts on the callers. By having a lot of public virtual methods that are overridden frequently, this is what you end up with. Note this does not apply to pure-virtual classes. In those cases the pure-virtuals are actually interfaces. You *do* want to ensure that the public facing API only interacts with the interface and not directly on the implementing class. Making the pure-virtual overrides private is the right thing to do.
On 2017/02/15 19:20:52, kylix_rd wrote: > On 2017/02/15 19:11:40, Peter Kasting wrote: > > > https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/view... > > File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): > > > > > https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/view... > > chrome/browser/ui/views/location_bar/location_bar_view.cc:1082: // to the > > protected section. > > On 2017/02/15 16:42:59, kylix_rd wrote: > > > On 2017/02/15 01:14:01, Peter Kasting wrote: > > > > My instinct is that we should just make that public again and then go with > > the > > > > inlined, no-comment version. > > > > > > Yes, true. However, moving overrides to private seem to be all the rage in > > many > > > places I've looked. IMO, if anything is done about that, it should be a > > separate > > > refactoring pass. > > > > I'm usually responsible for pushing for that -- my default policy is "make > > overrides as private as possible". I wasn't thinking so much of reversing > that > > on a policy level, as simply saying "for this class, someone external needs to > > access this" and having that be reasonable justification for making it public. > > I guess this is where my framework philosophy would kick in... I don't like > having a lot of *public* virtual methods. Virtual methods should, in general, be > protected and overidden as protected (or private if the class is then "sealed"). > Even if the public, non-virtual method is merely a simple call to the virtual > method (and is inlined as a result), it still helps with the evolvability of the > framework by keeping a more stable public facing API without injecting heavy > contracts on the callers. > > By having a lot of public virtual methods that are overridden frequently, this > is what you end up with. Note this does not apply to pure-virtual classes. In > those cases the pure-virtuals are actually interfaces. You *do* want to ensure > that the public facing API only interacts with the interface and not directly on > the implementing class. Making the pure-virtual overrides private is the right > thing to do. I don't know whether I follow that last paragraph. If you have an abstract base with public virtuals, and a concrete child with private implementation overrides, someone with a Child* can't call the interface methods without first casting to a Base*. That's basically the problem we have here, just with a non-abstract base. Perhaps what you're saying is that people shouldn't have Child*s to begin with, but I'm not sure how that's any more realistic for pure abstract bases than non-pure ones. Anyway, this is all tangential -- all courses of action here are OK.
On 2017/02/15 19:29:25, Peter Kasting wrote: > On 2017/02/15 19:20:52, kylix_rd wrote: > > On 2017/02/15 19:11:40, Peter Kasting wrote: > > > > > > https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/view... > > > File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): > > > > > > > > > https://codereview.chromium.org/2668833003/diff/340001/chrome/browser/ui/view... > > > chrome/browser/ui/views/location_bar/location_bar_view.cc:1082: // to the > > > protected section. > > > On 2017/02/15 16:42:59, kylix_rd wrote: > > > > On 2017/02/15 01:14:01, Peter Kasting wrote: > > > > > My instinct is that we should just make that public again and then go > with > > > the > > > > > inlined, no-comment version. > > > > > > > > Yes, true. However, moving overrides to private seem to be all the rage in > > > many > > > > places I've looked. IMO, if anything is done about that, it should be a > > > separate > > > > refactoring pass. > > > > > > I'm usually responsible for pushing for that -- my default policy is "make > > > overrides as private as possible". I wasn't thinking so much of reversing > > that > > > on a policy level, as simply saying "for this class, someone external needs > to > > > access this" and having that be reasonable justification for making it > public. > > > > I guess this is where my framework philosophy would kick in... I don't like > > having a lot of *public* virtual methods. Virtual methods should, in general, > be > > protected and overidden as protected (or private if the class is then > "sealed"). > > Even if the public, non-virtual method is merely a simple call to the virtual > > method (and is inlined as a result), it still helps with the evolvability of > the > > framework by keeping a more stable public facing API without injecting heavy > > contracts on the callers. > > > > By having a lot of public virtual methods that are overridden frequently, this > > is what you end up with. Note this does not apply to pure-virtual classes. In > > those cases the pure-virtuals are actually interfaces. You *do* want to ensure > > that the public facing API only interacts with the interface and not directly > on > > the implementing class. Making the pure-virtual overrides private is the right > > thing to do. > > I don't know whether I follow that last paragraph. If you have an abstract base > with public virtuals, and a concrete child with private implementation > overrides, someone with a Child* can't call the interface methods without first > casting to a Base*. That's basically the problem we have here, just with a > non-abstract base. Perhaps what you're saying is that people shouldn't have > Child*s to begin with, but I'm not sure how that's any more realistic for pure > abstract bases than non-pure ones. I'm saying that except for pure-virtual abstract bases, Child* should be the same as Base*.
CQ is committing da patch.
Bot data: {"patchset_id": 360001, "attempt_start_ts": 1487181430559950,
"parent_rev": "5c71eb349c1b9f3e0624ad6f27ba0b09c666337f", "commit_rev":
"a5d37aea27ce6ff61d1b7c55c65fe4d7ecc662fe"}
Message was sent while issue was closed.
Description was changed from ========== DialogBrowserTest implementation to invoke Content settings bubble dialogs. There needs to be a way to invoke all the various content settings bubble dialog views. This wires up the BrowserDialogTest framework in order to invoke/test them. BUG=652024 ========== to ========== DialogBrowserTest implementation to invoke Content settings bubble dialogs. There needs to be a way to invoke all the various content settings bubble dialog views. This wires up the BrowserDialogTest framework in order to invoke/test them. BUG=652024 Review-Url: https://codereview.chromium.org/2668833003 Cr-Commit-Position: refs/heads/master@{#450776} Committed: https://chromium.googlesource.com/chromium/src/+/a5d37aea27ce6ff61d1b7c55c65f... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:360001) as https://chromium.googlesource.com/chromium/src/+/a5d37aea27ce6ff61d1b7c55c65f... |
