Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
787033004:
Update geolocation permission tests for the permission bubble (Closed)
|
Created:
6 years ago by felt Modified:
5 years, 10 months ago CC:
chromium-reviews, markusheintz_, mlamouri+watch-geolocation_chromium.org, Michael van Ouwerkerk Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate geolocation permission tests for the permission bubble
This makes the geolocation permission tests run for both infobars and
permission bubbles. The tests now run as parameterized tests.
Supercedes parts of:
https://codereview.chromium.org/411503005/
https://codereview.chromium.org/341833004/
BUG=438758
Committed: https://crrev.com/f57c61952870c6027dbf220eff8b2d703bfed3c8
Cr-Commit-Position: refs/heads/master@{#314541}
Committed: https://crrev.com/d9f24fc092eba7accd05df923b1a1aa911a8ca38
Cr-Commit-Position: refs/heads/master@{#315883}
Committed: https://crrev.com/9740ee70258c4a4ce0f91a0fd9bc56c104caf990
Cr-Commit-Position: refs/heads/master@{#316261}
Patch Set 1 #Patch Set 2 : Unit tests passing locally now #Patch Set 3 : Browser tests working for most tests #Patch Set 4 : Browser tests done #
Total comments: 34
Patch Set 5 : Helper methods for unit tests; new MockPermissionBubbleView class #Patch Set 6 : Added missing comment #Patch Set 7 : uint -> size_t #Patch Set 8 : fix compile warning/error #
Total comments: 28
Patch Set 9 : Changes for timvolodine #
Total comments: 2
Patch Set 10 : Compile fix #Patch Set 11 : Rebased #Patch Set 12 : Fixed rebase mistake #Patch Set 13 : Fix Tim's nits #
Total comments: 4
Patch Set 14 : Tweaks for Markus #Patch Set 15 : Fixed compile bug #Patch Set 16 : Screwed up gypi copy/paste #Patch Set 17 : Android fix #Patch Set 18 : Missing Android ifdef #Patch Set 19 : Tweak to gather more info #Patch Set 20 : Fixed rebase error #Patch Set 21 : Exclude flaky browsertests #Messages
Total messages: 42 (13 generated)
felt@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
mvanouwerkerk@, PTAL.
https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... File chrome/browser/geolocation/geolocation_browsertest.cc (right): https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:129: class MockView : public PermissionBubbleView { MockView seems quite a generic name, maybe MockBubbleView? https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:129: class MockView : public PermissionBubbleView { This looks like a completely generic class that could be used in tests for other features as well. There is a very similar one in geolocation_permission_context_unittest.cc. Why not extract it to a separate file? https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:131: MockView() : shown_(false), can_accept_updates_(true), delegate_(NULL) {} We should use nullptr now. Here and below. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:132: virtual ~MockView() {} Overriding destructors use override now instead of virtual. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:143: virtual void SetDelegate(Delegate* delegate) override { When using override, omit virtual. Here and below. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:143: virtual void SetDelegate(Delegate* delegate) override { Does this take ownership of the delegate? Maybe this should be document in permission_bubble_view.h https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:148: const std::vector<PermissionBubbleRequest*>& requests, From permission_bubble_view.h it seems this does not take ownership of the raw pointers. Is that correct? If so: ok. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:162: return shown_; Maybe rename shown_ to visible_ for consistency? https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:195: explicit GeolocationNotificationObserver( No need for explicit anymore. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:196: bool wait_for_prompt, MockView* view); Does this take ownership of the view? https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:297: (new content::MessageLoopRunner())->Run(); I think this runner is leaking, as no scoped_refptr is used. Anyway, I think you could replace this line with: base::RunLoop().Run(); or content::RunMessageLoop(); https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:300: } No need for curly braces for oneliners. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:364: // For |requesting_url| if |allowed| is true accept the ptompt. Otherwise prompt https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:447: current_browser_->tab_strip_model()->GetActiveWebContents(); There's some pretty verbose duplication here. It would be nice to do one of these: [1] Define a GetWebContents() method on GeolocationBrowserTest or [2] Extract something like SetMockBubbleView() https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:625: content::RunAllPendingInMessageLoop(); Could this be part of SetPromptResponse? https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (right): https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:305: PermissionBubbleManager* mgr) { s/mgr/manager/ - http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin... Here and below. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:522: if (BubbleEnabled()) { These kinds of branches make the code harder to read, and it seems fragile as well. How about some helper methods like GetNumberOfPrompts that would internally be something like: if (BubbleEnabled()) return GetBubblesQueueSize(mgr) return infobar_service()->infobar_count()
https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... File chrome/browser/geolocation/geolocation_browsertest.cc (right): https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:129: class MockView : public PermissionBubbleView { On 2014/12/17 17:20:18, Michael - OOO until 2 January wrote: > MockView seems quite a generic name, maybe MockBubbleView? Done. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:129: class MockView : public PermissionBubbleView { On 2014/12/17 17:20:19, Michael - OOO until 2 January wrote: > This looks like a completely generic class that could be used in tests for other > features as well. There is a very similar one in > geolocation_permission_context_unittest.cc. Why not extract it to a separate > file? Done. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:131: MockView() : shown_(false), can_accept_updates_(true), delegate_(NULL) {} On 2014/12/17 17:20:19, Michael - OOO until 2 January wrote: > We should use nullptr now. Here and below. Done. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:132: virtual ~MockView() {} On 2014/12/17 17:20:19, Michael - OOO until 2 January wrote: > Overriding destructors use override now instead of virtual. Done. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:143: virtual void SetDelegate(Delegate* delegate) override { On 2014/12/17 17:20:19, Michael - OOO until 2 January wrote: > When using override, omit virtual. Here and below. Done. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:143: virtual void SetDelegate(Delegate* delegate) override { On 2014/12/17 17:20:19, Michael - OOO until 2 January wrote: > Does this take ownership of the delegate? Maybe this should be document in > permission_bubble_view.h No, it doesn't take ownership. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:148: const std::vector<PermissionBubbleRequest*>& requests, On 2014/12/17 17:20:19, Michael - OOO until 2 January wrote: > From permission_bubble_view.h it seems this does not take ownership of the raw > pointers. Is that correct? If so: ok. Correct https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:162: return shown_; On 2014/12/17 17:20:19, Michael - OOO until 2 January wrote: > Maybe rename shown_ to visible_ for consistency? Done. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:195: explicit GeolocationNotificationObserver( On 2014/12/17 17:20:19, Michael - OOO until 2 January wrote: > No need for explicit anymore. Done. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:196: bool wait_for_prompt, MockView* view); On 2014/12/17 17:20:19, Michael - OOO until 2 January wrote: > Does this take ownership of the view? No, added note. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:297: (new content::MessageLoopRunner())->Run(); On 2014/12/17 17:20:19, Michael - OOO until 2 January wrote: > I think this runner is leaking, as no scoped_refptr is used. Anyway, I think you > could replace this line with: > base::RunLoop().Run(); > or > content::RunMessageLoop(); Done. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:300: } On 2014/12/17 17:20:19, Michael - OOO until 2 January wrote: > No need for curly braces for oneliners. Done. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:364: // For |requesting_url| if |allowed| is true accept the ptompt. Otherwise On 2014/12/17 17:20:19, Michael - OOO until 2 January wrote: > prompt Done. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:447: current_browser_->tab_strip_model()->GetActiveWebContents(); On 2014/12/17 17:20:19, Michael - OOO until 2 January wrote: > There's some pretty verbose duplication here. It would be nice to do one of > these: > [1] Define a GetWebContents() method on GeolocationBrowserTest > or > [2] Extract something like SetMockBubbleView() Given that they're only used in this method for initialization, seems a bit like overkill. I monkeyed with the if-else to make it less repetitive though. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_browsertest.cc:625: content::RunAllPendingInMessageLoop(); On 2014/12/17 17:20:19, Michael - OOO until 2 January wrote: > Could this be part of SetPromptResponse? Done. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (right): https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:305: PermissionBubbleManager* mgr) { On 2014/12/17 17:20:19, Michael - OOO until 2 January wrote: > s/mgr/manager/ - > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin... > > Here and below. Done. https://codereview.chromium.org/787033004/diff/60001/chrome/browser/geolocati... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:522: if (BubbleEnabled()) { On 2014/12/17 17:20:19, Michael - OOO until 2 January wrote: > These kinds of branches make the code harder to read, and it seems fragile as > well. How about some helper methods like GetNumberOfPrompts that would > internally be something like: > if (BubbleEnabled()) > return GetBubblesQueueSize(mgr) > return infobar_service()->infobar_count() OK, I added a few helper methods. I think the code is simpler now in many places.
felt@chromium.org changed reviewers: + markusheintz@google.com
markusheintz@, would you PTAL? michael is OOO now until jan.
felt@chromium.org changed reviewers: + timvolodine@chromium.org
Oops, confused. timvolodine@: could you review c/b/geolocation/* since michael is OOO until jan? markusheintz@, could you review c/b/ui/website_settings/*?
thanks felt@, quite some re-wiring.. it may be more readable to have separate tests for bubbles and infobars, but I am fine with the parameterized approach as well. a few comments/questions below. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (right): https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:284: int GeolocationPermissionContextTests::GetBubblesQueueSize( nit: no need for cast? you could just return a size_t similar to GetNumberOfPrompts(), where the cast can be dropped as well. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:289: void GeolocationPermissionContextTests::AcceptBubble( why are those methods needed: AcceptBubble, DenyBubble, CloseBubble? they appear to be one-liners why not use the code directly inside the test? https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:305: PermissionBubbleManager::FromWebContents(web_contents())-> nit: BubbleManagerDocumentLoadCompleted(web_contents()); https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:323: bool BubbleEnabled() { nit: .. const https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:380: // Tests ---------------------------------------------------------------------- do we have/want a test for bubbles when user_gesture is false after content load? maybe it should be a higher-level (i.e. non-geolocation) test though. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:434: web_contents(), RequestID(0), requesting_frame_0, BubbleEnabled()); you are using BubbleEnabled() to pass user_gesture? this seems confusing I thought BubbleEnabled() meant that the bubbles are used instead of inforbars.. (here and in other places) https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:519: // TODO(felt): The bubble is rejecting file:// permission requests. is there a crbug for this? https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:576: ASSERT_NE(base::string16(), text_0); ASSERT_FALSE(text_0.empty()) ? https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:808: profile()->GetHostContentSettingsMap()->GetContentSetting( nit: I guess you could extract this into a helper method (also seems to be used in other placed quite a lot), but that's maybe beyond this patch. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:957: #if defined(OS_ANDROID) why is this android-only? we support infobars on other platforms right? https://codereview.chromium.org/787033004/diff/140001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/mock_permission_bubble_view.h (right): https://codereview.chromium.org/787033004/diff/140001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/mock_permission_bubble_view.h:35: bool visible_; should the members be private?
I (personally) find it easier to check that the functionality is perfectly parallel by having parameterized tests. The original version of this refactor had separate tests & that glossed over the very subtle differences in behavior that are in fact bugs. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (right): https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:284: int GeolocationPermissionContextTests::GetBubblesQueueSize( On 2014/12/19 16:02:20, timvolodine wrote: > nit: no need for cast? you could just return a size_t similar to > GetNumberOfPrompts(), where the cast can be dropped as well. Done. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:289: void GeolocationPermissionContextTests::AcceptBubble( On 2014/12/19 16:02:21, timvolodine wrote: > why are those methods needed: AcceptBubble, DenyBubble, CloseBubble? they > appear to be one-liners why not use the code directly inside the test? The methods ->Accept and ->Deny are private; I have friend tested GeolocationPermissionContextTests to make them accessible up here. So it seems the cleanest way to handle both of these is to have the wrapper. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:305: PermissionBubbleManager::FromWebContents(web_contents())-> On 2014/12/19 16:02:21, timvolodine wrote: > nit: BubbleManagerDocumentLoadCompleted(web_contents()); Done. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:323: bool BubbleEnabled() { On 2014/12/19 16:02:21, timvolodine wrote: > nit: .. const Done. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:380: // Tests ---------------------------------------------------------------------- On 2014/12/19 16:02:21, timvolodine wrote: > do we have/want a test for bubbles when user_gesture is false after content > load? maybe it should be a higher-level (i.e. non-geolocation) test though. that's a good question, I will check, but it belongs in the main bubble tests (& not here). https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:434: web_contents(), RequestID(0), requesting_frame_0, BubbleEnabled()); On 2014/12/19 16:02:21, timvolodine wrote: > you are using BubbleEnabled() to pass user_gesture? this seems confusing I > thought BubbleEnabled() meant that the bubbles are used instead of inforbars.. > (here and in other places) I'm forcing the user gesture to be true for the bubble tests, which needs it because these tests happen after page load. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:519: // TODO(felt): The bubble is rejecting file:// permission requests. On 2014/12/19 16:02:20, timvolodine wrote: > is there a crbug for this? crbug.com/444047. added to comment. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:576: ASSERT_NE(base::string16(), text_0); On 2014/12/19 16:02:20, timvolodine wrote: > ASSERT_FALSE(text_0.empty()) ? Done. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:576: ASSERT_NE(base::string16(), text_0); On 2014/12/19 16:02:20, timvolodine wrote: > ASSERT_FALSE(text_0.empty()) ? Done. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:808: profile()->GetHostContentSettingsMap()->GetContentSetting( On 2014/12/19 16:02:20, timvolodine wrote: > nit: I guess you could extract this into a helper method (also seems to be used > in other placed quite a lot), but that's maybe beyond this patch. Done. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:957: #if defined(OS_ANDROID) On 2014/12/19 16:02:21, timvolodine wrote: > why is this android-only? we support infobars on other platforms right? I don't know. It was already if-def'ed, I just moved it down. I don't really want to start enabling other people's tests on new platforms though, if I can avoid it. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/mock_permission_bubble_view.h (right): https://codereview.chromium.org/787033004/diff/140001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/mock_permission_bubble_view.h:35: bool visible_; On 2014/12/19 16:02:21, timvolodine wrote: > should the members be private? Done.
lgtm modulo comments https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (right): https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:380: // Tests ---------------------------------------------------------------------- On 2014/12/19 16:46:31, felt wrote: > On 2014/12/19 16:02:21, timvolodine wrote: > > do we have/want a test for bubbles when user_gesture is false after content > > load? maybe it should be a higher-level (i.e. non-geolocation) test though. > > that's a good question, I will check, but it belongs in the main bubble tests (& > not here). it's up to you where to put it, but I think this file could be a reasonable place as well. It seems user_gesture is currently always true in the bubble tests, in which case it could be dropped as a parameter. A test with user_gesture=false would basically check that there is no prompt, which seems not hard to do given all the context in this class. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:434: web_contents(), RequestID(0), requesting_frame_0, BubbleEnabled()); On 2014/12/19 16:46:31, felt wrote: > On 2014/12/19 16:02:21, timvolodine wrote: > > you are using BubbleEnabled() to pass user_gesture? this seems confusing I > > thought BubbleEnabled() meant that the bubbles are used instead of inforbars.. > > (here and in other places) > > I'm forcing the user gesture to be true for the bubble tests, which needs it > because these tests happen after page load. I see, in this case could you make it explicitly "true"? otherwise it looks like user_gesture and BubbleEnabled() are somehow connected. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:957: #if defined(OS_ANDROID) On 2014/12/19 16:46:30, felt wrote: > On 2014/12/19 16:02:21, timvolodine wrote: > > why is this android-only? we support infobars on other platforms right? > > I don't know. It was already if-def'ed, I just moved it down. I don't really > want to start enabling other people's tests on new platforms though, if I can > avoid it. fair enough ;) https://codereview.chromium.org/787033004/diff/160001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (right): https://codereview.chromium.org/787033004/diff/160001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:443: // Ensure only one infobar is created. nit: infobar -> prompt
markusheintz, can you please review the website_settings files? https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (right): https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:380: // Tests ---------------------------------------------------------------------- On 2014/12/19 19:47:03, timvolodine wrote: > On 2014/12/19 16:46:31, felt wrote: > > On 2014/12/19 16:02:21, timvolodine wrote: > > > do we have/want a test for bubbles when user_gesture is false after content > > > load? maybe it should be a higher-level (i.e. non-geolocation) test though. > > > > that's a good question, I will check, but it belongs in the main bubble tests > (& > > not here). > > it's up to you where to put it, but I think this file could be a reasonable > place as well. It seems user_gesture is currently always true in the bubble > tests, in which case it could be dropped as a parameter. A test with > user_gesture=false would basically check that there is no prompt, which seems > not hard to do given all the context in this class. I added a test to PermissionBubbleManagerTest in another CL, but it's now sort of moot because I removed the user_gesture requirement in practice. I'm leaving the code there for now in case we want to turn it back on in 43 but my guess is that we will completely remove the user_gesture in a future CL. If that changes I will add more tests here. https://codereview.chromium.org/787033004/diff/140001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:434: web_contents(), RequestID(0), requesting_frame_0, BubbleEnabled()); On 2014/12/19 19:47:03, timvolodine wrote: > On 2014/12/19 16:46:31, felt wrote: > > On 2014/12/19 16:02:21, timvolodine wrote: > > > you are using BubbleEnabled() to pass user_gesture? this seems confusing I > > > thought BubbleEnabled() meant that the bubbles are used instead of > inforbars.. > > > (here and in other places) > > > > I'm forcing the user gesture to be true for the bubble tests, which needs it > > because these tests happen after page load. > > I see, in this case could you make it explicitly "true"? otherwise it looks like > user_gesture and BubbleEnabled() are somehow connected. Done. https://codereview.chromium.org/787033004/diff/160001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (right): https://codereview.chromium.org/787033004/diff/160001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:443: // Ensure only one infobar is created. On 2014/12/19 19:47:03, timvolodine wrote: > nit: infobar -> prompt Done.
felt@chromium.org changed reviewers: + markusheintz@chromium.org, timvolodine@google.com - markusheintz@google.com, timvolodine@chromium.org
Oops, Markus I had put the wrong username for you. PTAL at the website_settings files?
LGTM https://codereview.chromium.org/787033004/diff/240001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (right): https://codereview.chromium.org/787033004/diff/240001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:288: return manager->requests_.front()->GetMessageText(); This is only used once in the I actually think this could be test method "GetPromptText(". I think in lining this line and adding a comment for what this line means makes it easier to follow the code. https://codereview.chromium.org/787033004/diff/240001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/mock_permission_bubble_view.h (right): https://codereview.chromium.org/787033004/diff/240001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/mock_permission_bubble_view.h:11: class MockPermissionBubbleView : public PermissionBubbleView { optional: I think a few words about when and how to use this Mock would be helpfull. Some things are not obvious e.g. how and where the delegate is set,
https://codereview.chromium.org/787033004/diff/240001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (right): https://codereview.chromium.org/787033004/diff/240001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:288: return manager->requests_.front()->GetMessageText(); On 2015/02/02 10:46:52, markusheintz_ wrote: > This is only used once in the > > I actually think this could be test method "GetPromptText(". I think in lining > this line and adding a comment for what this line means makes it easier to > follow the code. > Done. https://codereview.chromium.org/787033004/diff/240001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/mock_permission_bubble_view.h (right): https://codereview.chromium.org/787033004/diff/240001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/mock_permission_bubble_view.h:11: class MockPermissionBubbleView : public PermissionBubbleView { On 2015/02/02 10:46:52, markusheintz_ wrote: > optional: I think a few words about when and how to use this Mock would be > helpfull. Some things are not obvious e.g. how and where the delegate is set, Done.
The CQ bit was checked by felt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/787033004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by felt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/787033004/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by felt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/787033004/340001
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/f57c61952870c6027dbf220eff8b2d703bfed3c8 Cr-Commit-Position: refs/heads/master@{#314541}
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:340001) has been created in https://codereview.chromium.org/902643003/ by ccameron@chromium.org. The reason for reverting is: Getting intermittent failures in GeolocationBrowserTest after this patch Failing build: https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/bui... Output: GeolocationBrowserTestWithParams/GeolocationBrowserTest.NoLeakFromOffTheRecord/1 (run #1): [ RUN ] GeolocationBrowserTestWithParams/GeolocationBrowserTest.NoLeakFromOffTheRecord/1 [3952:1032:0204/052930:WARNING:data_reduction_proxy_settings.cc(345)] SPDY proxy OFF at startup [3952:1032:0204/052930:INFO:legacy_render_widget_host_win.cc(156)] LegacyRenderWidgetHostHWND::OnGetObject message=61 w_param=0 l_param=-12 obj_id=4294967284 host_=00000000 [3952:1032:0204/052930:INFO:legacy_render_widget_host_win.cc(129)] LegacyRenderWidgetHostHWND::Init hwnd=04CF00A6 [3952:1032:0204/052930:WARNING:geolocation_browsertest.cc(383)] before navigate [3952:1032:0204/052930:INFO:legacy_render_widget_host_win.cc(156)] LegacyRenderWidgetHostHWND::OnGetObject message=61 w_param=0 l_param=-12 obj_id=4294967284 host_=00000000 [3952:1032:0204/052930:INFO:legacy_render_widget_host_win.cc(129)] LegacyRenderWidgetHostHWND::Init hwnd=08B500B0 [3952:1032:0204/052931:WARNING:geolocation_browsertest.cc(398)] after navigate [3952:1032:0204/052931:WARNING:geolocation_browsertest.cc(234)] will add geolocation watch for bubble [3952:1032:0204/052931:WARNING:geolocation_browsertest.cc(201)] javascript_response 1 [3952:3868:0204/052931:WARNING:embedded_test_server.cc(248)] Request not handled. Returning 404: /favicon.ico Output not very helpful here, but perhaps it makes sense to a non-sheriff..
The CQ bit was checked by felt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/787033004/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by felt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/787033004/380001
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/d9f24fc092eba7accd05df923b1a1aa911a8ca38 Cr-Commit-Position: refs/heads/master@{#315883}
Message was sent while issue was closed.
A revert of this CL (patchset #20 id:380001) has been created in https://codereview.chromium.org/924043002/ by felt@chromium.org. The reason for reverting is: Still flaky. See https://code.google.com/p/chromium/issues/detail?id=458563.
The CQ bit was checked by felt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/787033004/400001
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/9740ee70258c4a4ce0f91a0fd9bc56c104caf990 Cr-Commit-Position: refs/heads/master@{#316261} |
