Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
986333006:
Center permission bubble if location bar is hidden in MacOS. (Closed)
DescriptionCenter permission bubble if location bar is hidden in MacOS.
BUG=440403
Committed: https://crrev.com/d24ad3c586bab3c033617c764d47cd55ba71e890
Cr-Commit-Position: refs/heads/master@{#326575}
Patch Set 1 #Patch Set 2 : Fix test compile #Patch Set 3 : Fix crashing tests #Patch Set 4 : Fix more tests #
Total comments: 2
Patch Set 5 : Fallthrough #
Total comments: 4
Patch Set 6 : Add tests #Patch Set 7 : Omnibar -> LocationBar #Patch Set 8 : Attempt to fix failures in new tests #
Total comments: 32
Patch Set 9 : Apply Feedback Part 1 #
Total comments: 29
Patch Set 10 : Apply Feedback Part 2 #Patch Set 11 : rebase #Patch Set 12 : Fix broken tests #
Total comments: 44
Patch Set 13 : Apply Feedback #
Total comments: 17
Patch Set 14 : Apply feedback #Patch Set 15 : Apply feedback #Patch Set 16 : Forgotten Feedback #
Total comments: 3
Patch Set 17 : Apply Feedback #Patch Set 18 : Fix trybots #
Total comments: 4
Patch Set 19 : Fix test names #Messages
Total messages: 45 (7 generated)
hcarmona@chromium.org changed reviewers: + felt@chromium.org, groby@chromium.org
This change is specific to Mac OS and will only be relevant when a page is in app mode since the omnibar is still available in fullscreen on the mac.
https://codereview.chromium.org/986333006/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/base_bubble_controller.mm (right): https://codereview.chromium.org/986333006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/base_bubble_controller.mm:344: case info_bubble::kNoArrow: If we _must_ fall through, mark this as // FALLTHROUGH But I don't think this works - the zoom bubble uses kNoArrow https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... So does new credit card bubble: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... You should probably check where we want those anchored in the no-arrow case (and compare with where Views anchors noarrow)
https://codereview.chromium.org/986333006/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/base_bubble_controller.mm (right): https://codereview.chromium.org/986333006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/base_bubble_controller.mm:344: case info_bubble::kNoArrow: On 2015/03/10 23:34:13, groby wrote: > If we _must_ fall through, mark this as // FALLTHROUGH > > But I don't think this works - the zoom bubble uses kNoArrow > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > So does new credit card bubble: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > You should probably check where we want those anchored in the no-arrow case (and > compare with where Views anchors noarrow) Updated with // FALLTHROUGH. Looked into the zoom and credit card bubbles: both use kAlignRightEdgeToAnchorEdge to align with the anchor and not draw an arrow. This change will center bubbles that want to kAlignArrowToAnchor but not show an arrow.
https://codereview.chromium.org/986333006/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.h (right): https://codereview.chromium.org/986333006/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.h:47: bool HasOmnibar(); Why is this public? https://codereview.chromium.org/986333006/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/986333006/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:90: bool PermissionBubbleCocoa::HasOmnibar() { Please add tests.
hcarmona@chromium.org changed reviewers: + msw@chromium.org
+msw b/c change in chrome/browser/ui/startup/startup_browser_creator_impl.h requires another owner. https://codereview.chromium.org/986333006/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.h (right): https://codereview.chromium.org/986333006/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.h:47: bool HasOmnibar(); On 2015/03/16 22:09:35, groby wrote: > Why is this public? Doesn't need to be public. Made protected. Or would private + friend be preferred? https://codereview.chromium.org/986333006/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/986333006/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:90: bool PermissionBubbleCocoa::HasOmnibar() { On 2015/03/16 22:09:35, groby wrote: > Please add tests. Done.
Where did you get the term "Omnibar"? I think you should probably change all occurrences of that term added in this CL to "LocationBar", unless "Omnibar" is equally superfluous and the intended dominant name. Our terminology should be as consistent as possible. Otherwise, the changes look okay (I didn't inspect the test code closely). I can rubber stamp startup_browser_creator_impl.h, or look closer as needed.
On 2015/03/30 20:55:36, msw wrote: > Where did you get the term "Omnibar"? I think you should probably change all > occurrences of that term added in this CL to "LocationBar", unless "Omnibar" is > equally superfluous and the intended dominant name. Our terminology should be as > consistent as possible. Otherwise, the changes look okay (I didn't inspect the > test code closely). I can rubber stamp startup_browser_creator_impl.h, or look > closer as needed. Updated Omnibar -> LocationBar for consistency.
https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:93: if (location_bar) { Please add a comment why even if we have a location bar, we need to test for a feature as well. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm (right): https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:26: // To be used when getting the anchor with a location bar. The bubble should be General Q: How much of this testing framework (for Kiosk mode etc.) can be shared with Views? https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:30: class TestPermissionBubbleCocoa: public PermissionBubbleCocoa { Space in front of ':'. Does git cl format not catch that? https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:32: TestPermissionBubbleCocoa(NSWindow* parent_window): space in front of ':' https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:36: // Returns the base class value for testing |HasLocationBar|. It seems you either want to mock, or you don't, but never both - consider having two test fixtures, both for clarity and to avoid naming awkwardness. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:46: bool HasLocationBar() override { return location_bar_present_; } private: DISALLOW_COPY_AND_ASSIGN... https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:59: class PermissionBubbleCocoaBrowsertest : public ExtensionBrowserTest { Huh. I had no idea ExtensionBrowserTest stuff lived in BrowserTest, too. Oh well. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:110: IN_PROC_BROWSER_TEST_F(PermissionBubbleCocoaBrowsertest, NormalHasLocationBar) { "Normal" is an unclear naming choice. "HasLocationBarByDefault", maybe? https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:139: const extensions::Extension* GetExtension() { GetTestExtension? Also, are we guaranteed to have that extension installed? https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:201: ShowPermissionBubble(cur_browser); Up until here, this seems to be a verbatim copy from browser_browsertest.cc - please don't program via copy/paste. 1) Is that really _all_ required to put us into app mode? 2) Regardless of yes or no, it might be worth extracting into a helper function - it looks like quite a few tests would like to force app mode. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:205: EXPECT_NE(new_bookmark_apps_enabled, cur_browser->is_app()); So this doesn't quite know if it tests in app mode or not. After all, new_bookmark_apps_enabled could be false or true here. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:207: permission_bubble_->BaseHasLocationBar()); Wait, the name says "AppHasNoLocationBar", but this test ensures that if apps_enabled is true, we do have a location bar - one or the other is wrong. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:210: cur_browser->tab_strip_model()->CloseSelectedTabs(); Why?
There's still some things I need to finish before this CL is done, but I wanted to send out this update since it addresses most groby@'s feedback and I had a few questions. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:93: if (location_bar) { On 2015/04/01 00:19:14, groby wrote: > Please add a comment why even if we have a location bar, we need to test for a > feature as well. Done. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm (right): https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:26: // To be used when getting the anchor with a location bar. The bubble should be On 2015/04/01 00:19:14, groby wrote: > General Q: How much of this testing framework (for Kiosk mode etc.) can be > shared with Views? Most of these classes look like they can be shared with views if I refactor the cocoa specifics into a delegate. Where would be a good place for the shared code? https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:30: class TestPermissionBubbleCocoa: public PermissionBubbleCocoa { On 2015/04/01 00:19:14, groby wrote: > Space in front of ':'. > > Does git cl format not catch that? Had not run git cl format. I've fixed the formatting issues. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:32: TestPermissionBubbleCocoa(NSWindow* parent_window): On 2015/04/01 00:19:14, groby wrote: > space in front of ':' Done. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:36: // Returns the base class value for testing |HasLocationBar|. On 2015/04/01 00:19:14, groby wrote: > It seems you either want to mock, or you don't, but never both - consider having > two test fixtures, both for clarity and to avoid naming awkwardness. Done. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:46: bool HasLocationBar() override { return location_bar_present_; } On 2015/04/01 00:19:14, groby wrote: > private: > DISALLOW_COPY_AND_ASSIGN... Done. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:59: class PermissionBubbleCocoaBrowsertest : public ExtensionBrowserTest { On 2015/04/01 00:19:14, groby wrote: > Huh. I had no idea ExtensionBrowserTest stuff lived in BrowserTest, too. Oh > well. I'm a bit confused about what I should do here... https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:110: IN_PROC_BROWSER_TEST_F(PermissionBubbleCocoaBrowsertest, NormalHasLocationBar) { On 2015/04/01 00:19:14, groby wrote: > "Normal" is an unclear naming choice. "HasLocationBarByDefault", maybe? Done. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:139: const extensions::Extension* GetExtension() { On 2015/04/01 00:19:14, groby wrote: > GetTestExtension? Also, are we guaranteed to have that extension installed? Refactored so this method is no longer needed. Also changed the app that's loaded from "app" to "packaged_app" to avoid setting up a local test server. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:201: ShowPermissionBubble(cur_browser); On 2015/04/01 00:19:14, groby wrote: > Up until here, this seems to be a verbatim copy from browser_browsertest.cc - > please don't program via copy/paste. > > 1) Is that really _all_ required to put us into app mode? No, not everything is necessary. Refactored test to remove anything not required. > 2) Regardless of yes or no, it might be worth extracting into a helper function > - it looks like quite a few tests would like to force app mode. Good idea. Should that be a pre-requisite for this CL or should it be another CL after this one? https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:205: EXPECT_NE(new_bookmark_apps_enabled, cur_browser->is_app()); On 2015/04/01 00:19:14, groby wrote: > So this doesn't quite know if it tests in app mode or not. After all, > new_bookmark_apps_enabled could be false or true here. Updated test so it doesn't try to guess if app mode is possible. Test will fail if no app mode. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:207: permission_bubble_->BaseHasLocationBar()); On 2015/04/01 00:19:14, groby wrote: > Wait, the name says "AppHasNoLocationBar", but this test ensures that if > apps_enabled is true, we do have a location bar - one or the other is wrong. Updated test so test matches the name. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:210: cur_browser->tab_strip_model()->CloseSelectedTabs(); On 2015/04/01 00:19:14, groby wrote: > Why? This should be in TearDownOnMainThread: moved there. This is necessary because BrowserWindowController verifies that the tab_strip_model is empty on windowWillClose. https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm (right): https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:175: // enabled. I'm currently looking into this. Spoke with benwells@, he mentioned that this is a bug, but I can't repro on canary. I'm not sure if this is caused by my change yet.
Sorry, more stuff :( https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm (right): https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:59: class PermissionBubbleCocoaBrowsertest : public ExtensionBrowserTest { On 2015/04/02 01:30:14, Hector Carmona wrote: > On 2015/04/01 00:19:14, groby wrote: > > Huh. I had no idea ExtensionBrowserTest stuff lived in BrowserTest, too. Oh > > well. > > I'm a bit confused about what I should do here... Nothing. I would've just expected ExtensionBrowserTest files to have a different suffix and was surprised. Ignore :) https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:201: ShowPermissionBubble(cur_browser); On 2015/04/02 01:30:14, Hector Carmona wrote: > On 2015/04/01 00:19:14, groby wrote: > > Up until here, this seems to be a verbatim copy from browser_browsertest.cc - > > please don't program via copy/paste. > > > > 1) Is that really _all_ required to put us into app mode? > > No, not everything is necessary. Refactored test to remove anything not > required. > > > 2) Regardless of yes or no, it might be worth extracting into a helper > function > > - it looks like quite a few tests would like to force app mode. > > Good idea. Should that be a pre-requisite for this CL or should it be another CL > after this one? Follow-up CL is fine, but please file a crbug for it https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:210: cur_browser->tab_strip_model()->CloseSelectedTabs(); On 2015/04/02 01:30:14, Hector Carmona wrote: > On 2015/04/01 00:19:14, groby wrote: > > Why? > > This should be in TearDownOnMainThread: moved there. > This is necessary because BrowserWindowController verifies that the > tab_strip_model is empty on windowWillClose. It already was there, that's why I asked :) https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm (right): https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:28: const int kTolerance = 200; I don't think 200 pixels is an acceptable tolerance. Why so high? https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:44: class MockPermissionBubbleCocoa : public PermissionBubbleCocoa { FWIW: You can just add a mock class via gmock.h class MockBubble : public CocoaBubble { public: MockBubble(...) {...} MOCK_METHOD0(HasLocationBar,bool()); }; And then where it's used: ON_CALL(mockInstance, HasLocationBar).WillByDefault(Return(true)); // replace with false if necessary. Less code, more flavor ;) https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:62: class TestPermissionBubbleViewDelegate : public PermissionBubbleView::Delegate { Since this does exactly squat, gmock to the rescue - scrap the class, and then use testing::NiceMock<PermissionBubbleView::Delegate> delegate_; in the test fixture. https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:100: STLDeleteElements(&requests_); Use a ScopedVector instead. https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:115: controller_ = [[BrowserWindowController alloc] initWithBrowser:cur_browser Why create one? This sequence should yield the BWC for the current browser (since BrowserTests always have a browser, they also have a BWC ready-made) NSWindow* window = browser()->window()->GetNativeWindow(); BrowserWindowController* bwc = [BrowserWindowController browserWindowControllerForWindow:window]; No need to worry about closing that way. Actually, you don't even need the BWC. Just use the NSWindow from the first call as parent window. https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:120: permission_delegate_.reset(new TestPermissionBubbleViewDelegate()); The mock already takes care of this, but: Why do you new all the things? Unless you need to dynamically create, prefer static allocation. https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:173: bool OpenExtensionAppWindow(const extensions::Extension* extension) { Question: Can this code be shared in a util place? I feel we do that in several tests? https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:175: // enabled. On 2015/04/02 01:30:14, Hector Carmona wrote: > I'm currently looking into this. Spoke with benwells@, he mentioned that this is > a bug, but I can't repro on canary. I'm not sure if this is caused by my change > yet. Well, then let's not land this until we know we're not causing the issue. Also, even if we're not: Any way to opt out of the bookmark apps being enabled via command line switch? https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:182: chrome::startup::IsFirstRun first_run = Why do we care about first_run status? Just launch it with IS_NOT_FIRST_RUN? Or do we want to see a different behavior in the first run case? https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:193: if (browser_count != 2) Please ASSERT here - that way, we know what exactly went wrong. https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:269: NSPoint expected = NSMakePoint(0, frame.size.height); You want NSMinX(frame), NSMaxY(frame) (it's not guaranteed the frame starts at 0,0) https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:278: EXPECT_LE(expected.y - kTolerance, anchor.y); That means exepcted.y should be within kTolerance of anchor.y, more or less, correct? ASSERT_NEAR(expected.y, anchor.y, kTolerance) https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:295: EXPECT_EQ(expected.x, anchor.x); EXPECT_TRUE(NSEqualPoints(expected, anchor)); https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.h:81: friend class PermissionBubbleCocoaAppTest; Ugh. Discuss this with the owner of c/b/ui/startup, please - I'd love it if we didn't have to endlessly expend a friends list here just because a test wants to start in kiosk or app mode.
Applied feedback. Rebase for trybots. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm (right): https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:26: // To be used when getting the anchor with a location bar. The bubble should be On 2015/04/02 01:30:14, Hector Carmona wrote: > On 2015/04/01 00:19:14, groby wrote: > > General Q: How much of this testing framework (for Kiosk mode etc.) can be > > shared with Views? > > Most of these classes look like they can be shared with views if I refactor the > cocoa specifics into a delegate. Where would be a good place for the shared > code? These classes are now split so only the tests are cocoa specific :-) Setup should work for views https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm (right): https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:28: const int kTolerance = 200; On 2015/04/02 22:21:13, groby wrote: > I don't think 200 pixels is an acceptable tolerance. Why so high? Made tolerance smaller and moved my expected point closer to the lock icon. Is there a constant that I can use? https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:44: class MockPermissionBubbleCocoa : public PermissionBubbleCocoa { On 2015/04/02 22:21:12, groby wrote: > FWIW: You can just add a mock class via gmock.h > > class MockBubble : public CocoaBubble { > public: > MockBubble(...) {...} > MOCK_METHOD0(HasLocationBar,bool()); > }; > > And then where it's used: > > ON_CALL(mockInstance, HasLocationBar).WillByDefault(Return(true)); // replace > with false if necessary. > > Less code, more flavor ;) > Awesome! Done. https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:62: class TestPermissionBubbleViewDelegate : public PermissionBubbleView::Delegate { On 2015/04/02 22:21:12, groby wrote: > Since this does exactly squat, gmock to the rescue - scrap the class, and then > use > > testing::NiceMock<PermissionBubbleView::Delegate> delegate_; > > in the test fixture. Done. https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:100: STLDeleteElements(&requests_); On 2015/04/02 22:21:12, groby wrote: > Use a ScopedVector instead. Done. https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:115: controller_ = [[BrowserWindowController alloc] initWithBrowser:cur_browser On 2015/04/02 22:21:13, groby wrote: > Why create one? > > This sequence should yield the BWC for the current browser (since BrowserTests > always have a browser, they also have a BWC ready-made) > > NSWindow* window = browser()->window()->GetNativeWindow(); > BrowserWindowController* bwc = > [BrowserWindowController browserWindowControllerForWindow:window]; > > No need to worry about closing that way. > > Actually, you don't even need the BWC. Just use the NSWindow from the first call > as parent window. Done. https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:120: permission_delegate_.reset(new TestPermissionBubbleViewDelegate()); On 2015/04/02 22:21:12, groby wrote: > The mock already takes care of this, but: Why do you new all the things? Unless > you need to dynamically create, prefer static allocation. Done. https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:173: bool OpenExtensionAppWindow(const extensions::Extension* extension) { On 2015/04/02 22:21:13, groby wrote: > Question: Can this code be shared in a util place? I feel we do that in several > tests? Would it make sense to move it to the ExtensionBrowserTest or should it be its own thing? https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:175: // enabled. On 2015/04/02 22:21:12, groby wrote: > On 2015/04/02 01:30:14, Hector Carmona wrote: > > I'm currently looking into this. Spoke with benwells@, he mentioned that this > is > > a bug, but I can't repro on canary. I'm not sure if this is caused by my > change > > yet. > > Well, then let's not land this until we know we're not causing the issue. > > Also, even if we're not: Any way to opt out of the bookmark apps being enabled > via command line switch? Issue is not related to this change, created bug 473394. New code is agnostic to bookmark apps because we specify the container. https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:182: chrome::startup::IsFirstRun first_run = On 2015/04/02 22:21:13, groby wrote: > Why do we care about first_run status? Just launch it with IS_NOT_FIRST_RUN? > > Or do we want to see a different behavior in the first run case? Code no longer needed in this test. https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:193: if (browser_count != 2) On 2015/04/02 22:21:12, groby wrote: > Please ASSERT here - that way, we know what exactly went wrong. Code changed so we get the browser and don't need to guess. https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:269: NSPoint expected = NSMakePoint(0, frame.size.height); On 2015/04/02 22:21:13, groby wrote: > You want NSMinX(frame), NSMaxY(frame) (it's not guaranteed the frame starts at > 0,0) Done. https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:278: EXPECT_LE(expected.y - kTolerance, anchor.y); On 2015/04/02 22:21:12, groby wrote: > That means exepcted.y should be within kTolerance of anchor.y, more or less, > correct? > > ASSERT_NEAR(expected.y, anchor.y, kTolerance) Done. https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:295: EXPECT_EQ(expected.x, anchor.x); On 2015/04/02 22:21:12, groby wrote: > EXPECT_TRUE(NSEqualPoints(expected, anchor)); Done. https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/986333006/diff/160001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.h:81: friend class PermissionBubbleCocoaAppTest; On 2015/04/02 22:21:13, groby wrote: > Ugh. Discuss this with the owner of c/b/ui/startup, please - I'd love it if we > didn't have to endlessly expend a friends list here just because a test wants to > start in kiosk or app mode. Removed dependency and only using public interfaces, no need to friend.
https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... File chrome/browser/ui/test/permission_bubble_browsertest.h (right): https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browsertest.h:17: class TestPermissionBubbleViewDelegate : public PermissionBubbleView::Delegate { Had to bring this class back. testing::NiceMock doesn't implement the pure methods.
On 2015/04/07 01:02:47, Hector Carmona wrote: > https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... > File chrome/browser/ui/test/permission_bubble_browsertest.h (right): > > https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... > chrome/browser/ui/test/permission_bubble_browsertest.h:17: class > TestPermissionBubbleViewDelegate : public PermissionBubbleView::Delegate { > Had to bring this class back. testing::NiceMock doesn't implement the pure > methods. Indeed - you still want NiceMock, though. It handles all the "Unexpected call" messages, because most of them are completely uninteresting
On 2015/04/07 01:27:15, groby wrote: > On 2015/04/07 01:02:47, Hector Carmona wrote: > > > https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... > > File chrome/browser/ui/test/permission_bubble_browsertest.h (right): > > > > > https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... > > chrome/browser/ui/test/permission_bubble_browsertest.h:17: class > > TestPermissionBubbleViewDelegate : public PermissionBubbleView::Delegate { > > Had to bring this class back. testing::NiceMock doesn't implement the pure > > methods. > > Indeed - you still want NiceMock, though. It handles all the "Unexpected call" > messages, because most of them are completely uninteresting groby@, we spoke offline about not having to mock this class since all methods are uninteresting. Could you take another look at this CL? Thanks!
https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/base_bubble_controller.mm (right): https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/base_bubble_controller.mm:344: // FALLTHROUGH Please always close comments with a period - see style guide. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:96: return location_bar_bridge->browser()->SupportsWindowFeature( I'm wondering if we shouldn't simply pass the browser to PermissionBubbleCocoa when we create it. That avoids a lot of dancing around here - it just becomes a call to SupportsWindowFeature. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:102: info_bubble::BubbleArrowLocation PermissionBubbleCocoa::GetArrowLocation() { Is there any way we can hoist this policy out into platform independent code? After all, we don't want an arrow on Views either in the fullscreen case, do we? https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm (right): https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:12: Put all this (constants, test helper classes, static function) into an anonymous namespace, please. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:15: const int kTolerance = 50; Still a huge tolerance. Why so big? https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:23: bool HasLocationBar() override { You could just friend the test, saving you a lot of pain. (FRIEND_TEST_ALL_PREFIXES) https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:42: static void CloseController(Browser* browser) { No need to be static in anon namespace. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:52: // Default Window Test ///////////////////////////////////////////////////////// I'd get rid of these separators - the test names should already express that. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:59: CloseController(browser()); Why do we need this at all? Shouldn't browser tests close everything? https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... File chrome/browser/ui/test/permission_bubble_browsertest.cc (right): https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browsertest.cc:58: LoadExtension(test_data_dir_.AppendASCII("app_with_panel_container/")); I take it leaking extensions is OK here? https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... File chrome/browser/ui/test/permission_bubble_browsertest.h (right): https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browsertest.h:7: Why a header file? We usually declare classes inline for any kind of test? If it's shared with other tests, we usually would name it something like permission_bubble_browser_test_utils.h https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browsertest.h:8: #include "base/command_line.h" Just forward declare CommandLine https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browsertest.h:15: // TestPermissionBubbleViewDelegate //////////////////////////////////////////// Please don't add separator comments. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browsertest.h:17: class TestPermissionBubbleViewDelegate : public PermissionBubbleView::Delegate { On 2015/04/07 01:02:47, Hector Carmona wrote: > Had to bring this class back. testing::NiceMock doesn't implement the pure > methods. Acknowledged. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browsertest.h:72: // Use this class to test on a kiosk window. Class comments go directly on top of class. Also, no separators :)
https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/base_bubble_controller.mm (right): https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/base_bubble_controller.mm:344: // FALLTHROUGH On 2015/04/08 18:39:15, groby wrote: > Please always close comments with a period - see style guide. Done. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:96: return location_bar_bridge->browser()->SupportsWindowFeature( On 2015/04/08 18:39:15, groby wrote: > I'm wondering if we shouldn't simply pass the browser to PermissionBubbleCocoa > when we create it. That avoids a lot of dancing around here - it just becomes a > call to SupportsWindowFeature. |parent_window_| can be updated. When that happens we may have a different browser. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:102: info_bubble::BubbleArrowLocation PermissionBubbleCocoa::GetArrowLocation() { On 2015/04/08 18:39:15, groby wrote: > Is there any way we can hoist this policy out into platform independent code? > After all, we don't want an arrow on Views either in the fullscreen case, do we? In cocoa we use info_bubble::BubbleArrowLocation https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... In views we use views::BubbleBorder::Arrow https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/bubble/bu... We can refactor this in a separate CL. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm (right): https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:12: On 2015/04/08 18:39:15, groby wrote: > Put all this (constants, test helper classes, static function) into an anonymous > namespace, please. Done. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:15: const int kTolerance = 50; On 2015/04/08 18:39:15, groby wrote: > Still a huge tolerance. Why so big? I've changed my expected value to be exactly the anchor and the test expects it to be within 5 pts. This test seems like it could fail very easily just by moving the anchor a little. I also don't want to get a UI element and figure out the exact point programmatically because then I feel like it's testing the implementation rather than the behavior. Suggestions? https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:23: bool HasLocationBar() override { On 2015/04/08 18:39:15, groby wrote: > You could just friend the test, saving you a lot of pain. > (FRIEND_TEST_ALL_PREFIXES) Friended. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:42: static void CloseController(Browser* browser) { On 2015/04/08 18:39:15, groby wrote: > No need to be static in anon namespace. Done. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:52: // Default Window Test ///////////////////////////////////////////////////////// On 2015/04/08 18:39:15, groby wrote: > I'd get rid of these separators - the test names should already express that. Done. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:59: CloseController(browser()); On 2015/04/08 18:39:15, groby wrote: > Why do we need this at all? Shouldn't browser tests close everything? I remember having issues without it, but everything is working fine after removing it. Maybe I had a different issue that I've since fixed. Done, no more |CloseController|. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... File chrome/browser/ui/test/permission_bubble_browsertest.cc (right): https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browsertest.cc:58: LoadExtension(test_data_dir_.AppendASCII("app_with_panel_container/")); On 2015/04/08 18:39:15, groby wrote: > I take it leaking extensions is OK here? I think the extension is owned by ExtensionRegistry and it should be cleaned up. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... File chrome/browser/ui/test/permission_bubble_browsertest.h (right): https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browsertest.h:7: On 2015/04/08 18:39:15, groby wrote: > Why a header file? We usually declare classes inline for any kind of test? If > it's shared with other tests, we usually would name it something like > permission_bubble_browser_test_utils.h Yes, the intent is that this be shared. Renamed to permission_bubble_browser_test_utils. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browsertest.h:8: #include "base/command_line.h" On 2015/04/08 18:39:15, groby wrote: > Just forward declare CommandLine Done. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browsertest.h:15: // TestPermissionBubbleViewDelegate //////////////////////////////////////////// On 2015/04/08 18:39:15, groby wrote: > Please don't add separator comments. Done. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browsertest.h:72: // Use this class to test on a kiosk window. On 2015/04/08 18:39:15, groby wrote: > Class comments go directly on top of class. Also, no separators :) Done.
https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:73: NSView* content_view = parent_window_.contentView; [parent_window_ contentView]; https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:74: anchor.y = content_view.frame.size.height; NSMaxY([content_view frame]); (This is not height, since frame can start at offset>0) https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:75: anchor.x = (content_view.frame.size.width) / 2; NSMidX([content_view frame]); https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:96: return location_bar_bridge->browser()->SupportsWindowFeature( On 2015/04/10 18:54:07, Hector Carmona wrote: > On 2015/04/08 18:39:15, groby wrote: > > I'm wondering if we shouldn't simply pass the browser to PermissionBubbleCocoa > > when we create it. That avoids a lot of dancing around here - it just becomes > a > > call to SupportsWindowFeature. > > |parent_window_| can be updated. When that happens we may have a different > browser. In what case would that happen? https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:102: info_bubble::BubbleArrowLocation PermissionBubbleCocoa::GetArrowLocation() { On 2015/04/10 18:54:07, Hector Carmona wrote: > On 2015/04/08 18:39:15, groby wrote: > > Is there any way we can hoist this policy out into platform independent code? > > After all, we don't want an arrow on Views either in the fullscreen case, do > we? > > In cocoa we use info_bubble::BubbleArrowLocation > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > In views we use views::BubbleBorder::Arrow > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/bubble/bu... > > We can refactor this in a separate CL. Please file a bug for that so we don't forget :) https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:103: return HasLocationBar() ? info_bubble::kTopLeft : info_bubble::kNoArrow; Sigh. I'm slow, sorry - I only just realized that if you return kTopCenter instead of kNoArrow here, you should be good without modifying BaseBubbleController. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... File chrome/browser/ui/test/permission_bubble_browsertest.h (right): https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browsertest.h:7: On 2015/04/10 18:54:08, Hector Carmona wrote: > On 2015/04/08 18:39:15, groby wrote: > > Why a header file? We usually declare classes inline for any kind of test? If > > it's shared with other tests, we usually would name it something like > > permission_bubble_browser_test_utils.h > > Yes, the intent is that this be shared. Renamed to > permission_bubble_browser_test_utils. I assume this is not shared yet? But will be with Views? https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm (right): https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:17: const int kAnchorTop = 61; Hrm. It works, but is fragile. Can you compute this based on bubble size? Yes, it duplicates code. That's probably an indicator you should just compare that the location for kNoArrow is the same as kTopCenter. Also, these are exceedingly odd coordinates for the anchor - the code in GetAnchorPoint should give you the middle of the screen, no? https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... File chrome/browser/ui/test/permission_bubble_browser_test_util.cc (right): https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browser_test_util.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. Move to c/b/ui/website_settings. https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... File chrome/browser/ui/test/permission_bubble_browser_test_util.h (right): https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browser_test_util.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. File belongs into c/b/ui/website_settings, since it's specific to website_settings. https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browser_test_util.h:19: TestPermissionBubbleViewDelegate(); empty ctor here. (See below) https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browser_test_util.h:36: PermissionBubbleBrowsertest(); Just define an empty ctor inline - you're not doing any work in the ctor. https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browser_test_util.h:37: ~PermissionBubbleBrowsertest() override; Same goes for dtor https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browser_test_util.h:70: PermissionBubbleKioskBrowsertest(); empty ctor here.
https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm (right): https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:17: const int kAnchorTop = 61; On 2015/04/10 20:58:55, groby wrote: > Hrm. It works, but is fragile. Can you compute this based on bubble size? Yes, > it duplicates code. That's probably an indicator you should just compare that > the location for kNoArrow is the same as kTopCenter. > > Also, these are exceedingly odd coordinates for the anchor - the code in > GetAnchorPoint should give you the middle of the screen, no? The size of the bubble doesn't affect the anchor. These constants are for verifying that the anchor is on the top right corner (under the lock icon). The coordinates are weird because that's where the lock icon is. https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... File chrome/browser/ui/test/permission_bubble_browser_test_util.cc (right): https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browser_test_util.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/04/10 20:58:55, groby wrote: > Move to c/b/ui/website_settings. Done. https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... File chrome/browser/ui/test/permission_bubble_browser_test_util.h (right): https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browser_test_util.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/04/10 20:58:56, groby wrote: > File belongs into c/b/ui/website_settings, since it's specific to > website_settings. Done. https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browser_test_util.h:19: TestPermissionBubbleViewDelegate(); On 2015/04/10 20:58:55, groby wrote: > empty ctor here. (See below) Acknowledged. https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browser_test_util.h:36: PermissionBubbleBrowsertest(); On 2015/04/10 20:58:56, groby wrote: > Just define an empty ctor inline - you're not doing any work in the ctor. Can't do that b/c these classes are considered complex, so the compiler complains. https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browser_test_util.h:37: ~PermissionBubbleBrowsertest() override; On 2015/04/10 20:58:56, groby wrote: > Same goes for dtor Acknowledged. https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browser_test_util.h:70: PermissionBubbleKioskBrowsertest(); On 2015/04/10 20:58:55, groby wrote: > empty ctor here. Acknowledged.
https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm (right): https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:17: const int kAnchorTop = 61; On 2015/04/10 22:28:05, Hector Carmona wrote: > On 2015/04/10 20:58:55, groby wrote: > > Hrm. It works, but is fragile. Can you compute this based on bubble size? Yes, > > it duplicates code. That's probably an indicator you should just compare that > > the location for kNoArrow is the same as kTopCenter. > > > > Also, these are exceedingly odd coordinates for the anchor - the code in > > GetAnchorPoint should give you the middle of the screen, no? > > The size of the bubble doesn't affect the anchor. These constants are for > verifying that the anchor is on the top right corner (under the lock icon). The > coordinates are weird because that's where the lock icon is. OIC - Can we just test against location_bar_bridge->GetPageInfoBubblePoint()? I don't want to tie us to a specific screen coordinate, because UI elements always move. Plus, that allows us to get rid of the tolerance. (Probably want to add a second test to check that it's _different_ from the point we'd get without location bar - that way, our test guarantees that with location bar it's in a different position than without, which is really all that makes sense to verify) https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... File chrome/browser/ui/test/permission_bubble_browser_test_util.h (right): https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browser_test_util.h:36: PermissionBubbleBrowsertest(); On 2015/04/10 22:28:05, Hector Carmona wrote: > On 2015/04/10 20:58:56, groby wrote: > > Just define an empty ctor inline - you're not doing any work in the ctor. > > Can't do that b/c these classes are considered complex, so the compiler > complains. Sigh. You're right - I forgot that any templated member triggers this.
Applied feedback so we compare against the page info location. Also added the new test to make sure the two locations (with and without locationbar) are different. https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm (right): https://codereview.chromium.org/986333006/diff/240001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:17: const int kAnchorTop = 61; On 2015/04/10 23:00:33, groby wrote: > On 2015/04/10 22:28:05, Hector Carmona wrote: > > On 2015/04/10 20:58:55, groby wrote: > > > Hrm. It works, but is fragile. Can you compute this based on bubble size? > Yes, > > > it duplicates code. That's probably an indicator you should just compare > that > > > the location for kNoArrow is the same as kTopCenter. > > > > > > Also, these are exceedingly odd coordinates for the anchor - the code in > > > GetAnchorPoint should give you the middle of the screen, no? > > > > The size of the bubble doesn't affect the anchor. These constants are for > > verifying that the anchor is on the top right corner (under the lock icon). > The > > coordinates are weird because that's where the lock icon is. > > OIC - Can we just test against location_bar_bridge->GetPageInfoBubblePoint()? I > don't want to tie us to a specific screen coordinate, because UI elements always > move. Plus, that allows us to get rid of the tolerance. (Probably want to add a > second test to check that it's _different_ from the point we'd get without > location bar - that way, our test guarantees that with location bar it's in a > different position than without, which is really all that makes sense to verify) > > Done.
There's still some unresolved comments on PS12. (I wish code review carried those forward so you didn't have to do archaeology...)
Applied feedback to forgotten comments. Also took a quick look at previous comments to make sure others weren't lost. https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm (right): https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:201: ShowPermissionBubble(cur_browser); On 2015/04/02 22:21:12, groby wrote: > On 2015/04/02 01:30:14, Hector Carmona wrote: > > On 2015/04/01 00:19:14, groby wrote: > > > Up until here, this seems to be a verbatim copy from browser_browsertest.cc > - > > > please don't program via copy/paste. > > > > > > 1) Is that really _all_ required to put us into app mode? > > > > No, not everything is necessary. Refactored test to remove anything not > > required. > > > > > 2) Regardless of yes or no, it might be worth extracting into a helper > > function > > > - it looks like quite a few tests would like to force app mode. > > > > Good idea. Should that be a pre-requisite for this CL or should it be another > CL > > after this one? > > Follow-up CL is fine, but please file a crbug for it The test that goes into app mode is now its own class in a shared location. Do we need to refactor this further? https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:73: NSView* content_view = parent_window_.contentView; On 2015/04/10 20:58:55, groby wrote: > [parent_window_ contentView]; Done. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:74: anchor.y = content_view.frame.size.height; On 2015/04/10 20:58:55, groby wrote: > NSMaxY([content_view frame]); (This is not height, since frame can start at > offset>0) Done. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:75: anchor.x = (content_view.frame.size.width) / 2; On 2015/04/10 20:58:55, groby wrote: > NSMidX([content_view frame]); Done. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:96: return location_bar_bridge->browser()->SupportsWindowFeature( On 2015/04/10 20:58:55, groby wrote: > On 2015/04/10 18:54:07, Hector Carmona wrote: > > On 2015/04/08 18:39:15, groby wrote: > > > I'm wondering if we shouldn't simply pass the browser to > PermissionBubbleCocoa > > > when we create it. That avoids a lot of dancing around here - it just > becomes > > a > > > call to SupportsWindowFeature. > > > > |parent_window_| can be updated. When that happens we may have a different > > browser. > > In what case would that happen? This can happen when a site requests to transition to fullscreen. A new window is created and the permission bubble should now be part of that window. Currently broken: crbug.com/165980 https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:102: info_bubble::BubbleArrowLocation PermissionBubbleCocoa::GetArrowLocation() { On 2015/04/10 20:58:55, groby wrote: > On 2015/04/10 18:54:07, Hector Carmona wrote: > > On 2015/04/08 18:39:15, groby wrote: > > > Is there any way we can hoist this policy out into platform independent > code? > > > After all, we don't want an arrow on Views either in the fullscreen case, do > > we? > > > > In cocoa we use info_bubble::BubbleArrowLocation > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > In views we use views::BubbleBorder::Arrow > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/bubble/bu... > > > > We can refactor this in a separate CL. > > Please file a bug for that so we don't forget :) Done. crbug.com/476662 https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:103: return HasLocationBar() ? info_bubble::kTopLeft : info_bubble::kNoArrow; On 2015/04/10 20:58:55, groby wrote: > Sigh. I'm slow, sorry - I only just realized that if you return kTopCenter > instead of kNoArrow here, you should be good without modifying > BaseBubbleController. We should return kNoArrow to avoid having an arrow pointing at the top of the window. https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... File chrome/browser/ui/test/permission_bubble_browsertest.h (right): https://codereview.chromium.org/986333006/diff/220001/chrome/browser/ui/test/... chrome/browser/ui/test/permission_bubble_browsertest.h:7: On 2015/04/10 20:58:55, groby wrote: > On 2015/04/10 18:54:08, Hector Carmona wrote: > > On 2015/04/08 18:39:15, groby wrote: > > > Why a header file? We usually declare classes inline for any kind of test? > If > > > it's shared with other tests, we usually would name it something like > > > permission_bubble_browser_test_utils.h > > > > Yes, the intent is that this be shared. Renamed to > > permission_bubble_browser_test_utils. > > I assume this is not shared yet? But will be with Views? Correct. Not shared yet, but it's in a state where views tests can be added. gypi files may need to be updated, since I haven't tried to add views only tests for this CL.
lgtm https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm (right): https://codereview.chromium.org/986333006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm:201: ShowPermissionBubble(cur_browser); On 2015/04/13 21:13:24, Hector Carmona wrote: > On 2015/04/02 22:21:12, groby wrote: > > On 2015/04/02 01:30:14, Hector Carmona wrote: > > > On 2015/04/01 00:19:14, groby wrote: > > > > Up until here, this seems to be a verbatim copy from > browser_browsertest.cc > > - > > > > please don't program via copy/paste. > > > > > > > > 1) Is that really _all_ required to put us into app mode? > > > > > > No, not everything is necessary. Refactored test to remove anything not > > > required. > > > > > > > 2) Regardless of yes or no, it might be worth extracting into a helper > > > function > > > > - it looks like quite a few tests would like to force app mode. > > > > > > Good idea. Should that be a pre-requisite for this CL or should it be > another > > CL > > > after this one? > > > > Follow-up CL is fine, but please file a crbug for it > > The test that goes into app mode is now its own class in a shared location. Do > we need to refactor this further? Possibly, if others are using it. I wouldn't mind if you filed a follow-up bug.
> > The test that goes into app mode is now its own class in a shared location. Do > > we need to refactor this further? > > Possibly, if others are using it. I wouldn't mind if you filed a follow-up bug. Followup bug: crbug.com/476733
The CQ bit was checked by hcarmona@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/986333006/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
felt@, would you be able to LG the new files in chrome/browser/ui/website_settings/ ?
The permission_bubble_browser_test_util.{cc|h} files look at first glance like
they're a general util for permission bubble browser tests, but in fact they are
specific to extension/apps tests. Can you make that more clear, both in terms of
naming the files & naming the classes?
https://codereview.chromium.org/986333006/diff/300001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_browser_test_util.h
(right):
https://codereview.chromium.org/986333006/diff/300001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_browser_test_util.h:33: //
testing app mode requires extensions.
Is this class then specific to apps/extensions? Should only apps/extensions
based tests use this class? If so you might want to name it accordingly (e.g.,
PermissionBubbleExtensionBrowserTest)
On 2015/04/17 21:14:58, felt wrote:
> The permission_bubble_browser_test_util.{cc|h} files look at first glance like
> they're a general util for permission bubble browser tests, but in fact they
are
> specific to extension/apps tests. Can you make that more clear, both in terms
of
> naming the files & naming the classes?
They're supposed to be generic enough to test permission bubbles in any
context they can be found in: regular window, full screen window, app
window (from an extension), and kiosk mode. I've changed the
inheritance hierachy so that PermissionBubbleBrowsertest doesn't need to
inherit from ExtensionBrowserTest. Only PermissionBubbleAppBrowsertest
needs to know about extensions.
https://codereview.chromium.org/986333006/diff/300001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_browser_test_util.h
(right):
https://codereview.chromium.org/986333006/diff/300001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_browser_test_util.h:33: //
testing app mode requires extensions.
On 2015/04/17 21:14:58, felt wrote:
> Is this class then specific to apps/extensions? Should only apps/extensions
> based tests use this class? If so you might want to name it accordingly (e.g.,
> PermissionBubbleExtensionBrowserTest)
Updated classes so that only PermissionBubbleAppBrowsertest inherits
from ExtensionBrowserTest. That's the only class that needs to load an
extension in order to test a permission bubble in app mode.
lgtm https://codereview.chromium.org/986333006/diff/300001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/permission_bubble_browser_test_util.h (right): https://codereview.chromium.org/986333006/diff/300001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_bubble_browser_test_util.h:33: // testing app mode requires extensions. On 2015/04/17 22:21:05, Hector Carmona wrote: > On 2015/04/17 21:14:58, felt wrote: > > Is this class then specific to apps/extensions? Should only apps/extensions > > based tests use this class? If so you might want to name it accordingly (e.g., > > PermissionBubbleExtensionBrowserTest) > > Updated classes so that only PermissionBubbleAppBrowsertest inherits > from ExtensionBrowserTest. That's the only class that needs to load an > extension in order to test a permission bubble in app mode. Thanks, the new inheritance structure looks better.
hcarmona@chromium.org changed reviewers: - msw@chromium.org
Fixed trybots, PTAL Thanks!
still lgtm % nits https://codereview.chromium.org/986333006/diff/340001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/permission_bubble_browser_test_util.cc (right): https://codereview.chromium.org/986333006/diff/340001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_bubble_browser_test_util.cc:68: void PermissionBubbleAppBrowsertest::SetUpCommandLine( nit: I just noticed that your class is named "...Browsertest" but all the others are named "...BrowserTest." Can you make it consistent? https://codereview.chromium.org/986333006/diff/340001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/permission_bubble_browser_test_util.h (right): https://codereview.chromium.org/986333006/diff/340001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_bubble_browser_test_util.h:58: // Override from ExtensionBrowserTest to avoid "inheritance via domiance". did you mean "dominance"?
https://codereview.chromium.org/986333006/diff/340001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/permission_bubble_browser_test_util.cc (right): https://codereview.chromium.org/986333006/diff/340001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_bubble_browser_test_util.cc:68: void PermissionBubbleAppBrowsertest::SetUpCommandLine( On 2015/04/21 21:49:45, felt wrote: > nit: I just noticed that your class is named "...Browsertest" but all the others > are named "...BrowserTest." Can you make it consistent? Done. https://codereview.chromium.org/986333006/diff/340001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/permission_bubble_browser_test_util.h (right): https://codereview.chromium.org/986333006/diff/340001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_bubble_browser_test_util.h:58: // Override from ExtensionBrowserTest to avoid "inheritance via domiance". On 2015/04/21 21:49:45, felt wrote: > did you mean "dominance"? Yes, fixed.
Any objections to committing this?
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org, felt@chromium.org Link to the patchset: https://codereview.chromium.org/986333006/#ps360001 (title: "Fix test names")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/986333006/360001
Message was sent while issue was closed.
Committed patchset #19 (id:360001)
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/d24ad3c586bab3c033617c764d47cd55ba71e890 Cr-Commit-Position: refs/heads/master@{#326575}
Message was sent while issue was closed.
A revert of this CL (patchset #19 id:360001) has been created in https://codereview.chromium.org/1092933006/ by jdonnelly@chromium.org. The reason for reverting is: I believe this is causing the Mac ASan 64 Tests (1) bot to fail: ==56836==ERROR: AddressSanitizer: stack-use-after-return on address 0x00011a38dc78 at pc 0x000104e8028f bp 0x7fff5f6e5600 sp 0x7fff5f6e55f8 See http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%.... |
