Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2751573002:
[Mac] Refactor bookmark bar controller (Closed)
DescriptionYes, with RTL thrown in, since this is specifically designed to make it almost free.
Two major things here:
- The bar is no longer relaid-out directly in response to changes in view size, bookmark model etc. Instead, a new UI-direction-agnostic view model (BookmarkBarLayout) is created from the current state, and if it's different from before, it's applied to the view.
- Removed a bunch of layout-related code that's no longer necessary
BUG=648560
Review-Url: https://codereview.chromium.org/2751573002
Cr-Commit-Position: refs/heads/master@{#468364}
Committed: https://chromium.googlesource.com/chromium/src/+/2644729cb7722a702a76cc2758d0ce372e1e6f92
Patch Set 1 #Patch Set 2 : Clean up #Patch Set 3 : More WIP #Patch Set 4 : Restore unused button pool #
Total comments: 74
Patch Set 5 : Clean up #Patch Set 6 : Comment expansion/clean up #Patch Set 7 : Revert whitespace, fix syntax after symbol rename #
Total comments: 19
Patch Set 8 : Address CL comments #Patch Set 9 : Whitespace/TODO #
Total comments: 6
Patch Set 10 : Factor layout tests to helper, address CL comments #
Total comments: 1
Messages
Total messages: 40 (26 generated)
ellyjones@chromium.org changed reviewers: + ellyjones@chromium.org
:) https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_bridge.mm (left): https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_bridge.mm:53: void BookmarkBarBridge::BookmarkModelBeingDeleted(BookmarkModel* model) { can this happen in other circumstances than the browser being torn down? https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (left): https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1602: // Adjust the horizontal width, x position and the visibility of the "For quick it warms my heart to see all this code being removed :) https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right): https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:182: typedef NSUInteger VisibleElements; I don't know that this typedef is adding any extra clarity - "unsigned int visible_elements;" is easier to understand for me than "VisibleElements visible_elements", which I assumed was a container type until I went and actually looked up the typedef. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:184: struct BookmarkBarLayout { I really like this approach of bundling the state up into a struct. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:370: - (BookmarkButton*)bookmarkButtonToPulseForNode:(const BookmarkNode*)node { this comment only needs to exist because this function name is very opaque, so perhaps we can change the function name to something more descriptive? findAncestorButtonOnBarForNode or something :) https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1086: - (BookmarkButton*)buttonForNode:(const BookmarkNode*)node { the name of this reads like a simple accessor to me. Since it can actually allocate a new button, I would call it "getOrCreateButtonForNode:", which is longer but I think clearer. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1110: nodeIdToButtonMap_.insert( why does this happen even if we found the button in nodeIdToButtonMap_ earlier? https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1275: [[offTheSideButton_ draggableButton] setDraggable:NO]; this may be a silly question, but if the offTheSideButton_ is never draggable, why does it have a draggableButton? https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1365: // Puts stuff into the final state without animating, stopping a running Can you expand on what "stuff" is? What state specifically is being finalized here? https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1717: // TODO(mrossetti,jrg): Yet more duplicated code. is the mentioned duplicated code actually still here? https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1769: // Gets called only by the BookmarkBarView. is it important that this is the case? https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1795: (for example, a drag from the URL bar, a link on the desktop, We use the style with a leading // on every line. At least for me, since I don't use syntax highlighting, I also find the leading-// style easier to read :) https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1835: - (void)rebuildLayout:(BOOL)animated { okay, I really like this - it is much easier to follow and much more test-able than what we had before. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1933: // 1) Hide all buttons initially It might be nice to actually break this function into three functions, each of which does one of these steps, because then instead of this comment, the code would say: [self hideAllButtons]; [self showAllButtonsInLayout:layout]; [self removeAllHiddenButtons]; which I think would be quite nice. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2000: - (void)updateSpecialButton:(BookmarkButton*)button "update" is a bit generic - could this function name be more specific about what it's doing? repositionSpecialButton:toXOffset:hidden: perhaps? https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2011: - (void)applyXOffset:(CGFloat)offset the previous function has the button before the offset, so this one probably should as well... perhaps this could be part of updateSpecialButton: itself? https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2101: - (void)updateExtraButtonsVisibility { the code seems to alternate between "special" and "extra" as the word for these buttons https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2621: BOOL isMoveWithinOffTheSideMenu = I like that you have broken this out into a semantically-named boolean - it makes the resulting if statement less cluttered & easier to read. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.h (right): https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.h:22: base::scoped_nsobject<NSTextField> noItemTextField_; thank you for fixing this https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right): https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:456: + (NSFont*)fontForBookmarkBarCell { I like this pattern of breaking sorta-constants out into descriptively named helpers.
avi@chromium.org changed reviewers: + avi@chromium.org
I'm liking where you're going here. More style nits than anything else. Some thoughts: - More blank lines. I don't know what Maf was thinking when he wrote the code in all one block, but now that you're in here, add blank lines where appropriate to space things out and make the flow more obvious. - This is a huge tangle. You will likely need to break up this cleanup into smaller bits. This CL is already at the edge of how big I think is a good idea to land at once. The smaller the pieces, the more comprehensible each is and the easier it is to get them reviewed. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right): https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:157: // Min width for no item text field and import bookmarks button full sentences; end with a . https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:179: kVisibleElementsImportBookmarksButton = 1 << 6, You should have "Mask" in these names (i.e. kVisibleElementsMaskOffTheSideButton; see NSEventMaskXxxx). Otherwise from context it will seem like these are enumerated objects rather than things to AND against a value. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:207: VisibleElements visible_elements; blank line to separate the accessors from the data? https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:222: bool operator==(const BookmarkBarLayout& lhs, const BookmarkBarLayout& rhs) { C++11 style is now to use std::tie for building these operators. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:259: @implementation BookmarkBarController { Excellent! You're moving the variables one-by-one, as you figure them out? https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:265: nodeIdToButtonMap_; Can you add a comment for this object? The buttons in this map seem dually-owned: once by the map, once by the view. True? https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:266: // A place to stash bookmark buttons that have been removed from the bar Have an extra line before comments like this to visually group them with the variable they describe. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:281: if ((self = [super initWithNibName:nil bundle:nil])) { Yay! Can you remove the nib file too in this change? https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:359: [self createAppsPageShortcutButton]; Adding this line between the other two just brings up questions. Those two are 1. create a button 2. add it. This interrupts that pattern, but also doesn't add it. If it doesn't need to be added, can you say why? e.g. [self createOffTheSideButton]; [buttonView_ addSubview:offTheSideButton_]; [self createAppsPageShortcutButton]; // Don't (yet) add the apps page shortcut button because _reasons_. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:364: // viewDidLoad became part of the API in 10.10 . for sentences; might as well add them as you go. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:505: // Don't pass ourself along (as 'self') until our init is completely This is another situation where we should have a blank line before this comment section, to visually group it with the code under it that it talks about. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:962: // +1 is a fudge factor to make it line up period at the end of sentences Side question re lining up: tested on both retina and non-retina screens? This is a bug fix? https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:972: // if |animate| is YES, the changes are made using the animator; otherwise they Full sentences; capitalize the first letter. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1077: - (void)openBookmarkMenuItem:(id)sender { Getting rid of IBAction makes me happier than it reasonably should. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1090: button = nodeIdToButtonMap_.at(node->id()).get(); auto buttonIt = nodeIdToButtonMap_.find(nodeId); if (buttonIt != nodeIdToButtonMap_.end() { button = *buttonIt; lets you reuse the iterator. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1109: // Do setup blank line above, period at end https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1138: // Folders show a tooltip iff the title is truncated. blank line above this comment this function needs more blank lines to clarify flow https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1692: BOOL isRTL = cocoa_l10n_util::ShouldDoExperimentalRTLLayout(); Mixing in RTL support at the same time? Good luck... 😬 https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1712: // |point| is in the base coordinate system of the destination window; Fix run-on sentence. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1763: layout_ = {}; // Force layout . https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1802: is to the left of the drag position, make room. */ Don't use /* comments. Use // ones for a block. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1862: } moar blank lines through this function https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2161: BookmarkButton* button = nodeIdToButtonMap_[node->id()]; do the find outside the if, then you can reuse the iterator here.
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ok, this is now ready for real review. I peeled a few pieces off last week, but I think splitting it up anymore is unfortunately quite challenging due to the nature of this change. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right): https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:157: // Min width for no item text field and import bookmarks button On 2017/03/23 15:44:52, Avi wrote: > full sentences; end with a . Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:179: kVisibleElementsImportBookmarksButton = 1 << 6, On 2017/03/23 15:44:53, Avi wrote: > You should have "Mask" in these names (i.e. > kVisibleElementsMaskOffTheSideButton; see NSEventMaskXxxx). Otherwise from > context it will seem like these are enumerated objects rather than things to AND > against a value. Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:182: typedef NSUInteger VisibleElements; On 2017/03/23 15:40:22, Elly Fong-Jones wrote: > I don't know that this typedef is adding any extra clarity - "unsigned int > visible_elements;" is easier to understand for me than "VisibleElements > visible_elements", which I assumed was a container type until I went and > actually looked up the typedef. Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:207: VisibleElements visible_elements; On 2017/03/23 15:44:53, Avi wrote: > blank line to separate the accessors from the data? Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:222: bool operator==(const BookmarkBarLayout& lhs, const BookmarkBarLayout& rhs) { On 2017/03/23 15:44:52, Avi wrote: > C++11 style is now to use std::tie for building these operators. Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:265: nodeIdToButtonMap_; On 2017/03/23 15:44:53, Avi wrote: > Can you add a comment for this object? The buttons in this map seem > dually-owned: once by the map, once by the view. True? Done. Yes that's correct re: ownership. My read is that they won't leak because: 1) In applyLayout:, any button that's not visible gets removed from here 2) On dealloc, they'll get removed from superview by NSView, then will get released when the map is destructed (But I'm a C++ noob, so maybe that's wrong?) https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:266: // A place to stash bookmark buttons that have been removed from the bar On 2017/03/23 15:44:53, Avi wrote: > Have an extra line before comments like this to visually group them with the > variable they describe. Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:281: if ((self = [super initWithNibName:nil bundle:nil])) { On 2017/03/23 15:44:53, Avi wrote: > Yay! > > Can you remove the nib file too in this change? Already done in crrev.com/2729603008 ! https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:359: [self createAppsPageShortcutButton]; On 2017/03/23 15:44:52, Avi (slow and ooo 10-23 April) wrote: > Adding this line between the other two just brings up questions. Those two are > 1. create a button 2. add it. > > This interrupts that pattern, but also doesn't add it. If it doesn't need to be > added, can you say why? e.g. > > [self createOffTheSideButton]; > [buttonView_ addSubview:offTheSideButton_]; > > [self createAppsPageShortcutButton]; > // Don't (yet) add the apps page shortcut button because _reasons_. Factored the "extra" button creation stuff out https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:364: // viewDidLoad became part of the API in 10.10 On 2017/03/23 15:44:53, Avi (slow and ooo 10-23 April) wrote: > . for sentences; might as well add them as you go. Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:370: - (BookmarkButton*)bookmarkButtonToPulseForNode:(const BookmarkNode*)node { On 2017/03/23 15:40:21, Elly Fong-Jones wrote: > this comment only needs to exist because this function name is very opaque, so > perhaps we can change the function name to something more descriptive? > findAncestorButtonOnBarForNode or something :) Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:505: // Don't pass ourself along (as 'self') until our init is completely On 2017/03/23 15:44:53, Avi (slow and ooo 10-23 April) wrote: > This is another situation where we should have a blank line before this comment > section, to visually group it with the code under it that it talks about. Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:962: // +1 is a fudge factor to make it line up On 2017/03/23 15:44:52, Avi (slow and ooo 10-23 April) wrote: > period at the end of sentences > > Side question re lining up: tested on both retina and non-retina screens? This > is a bug fix? Turns out this whole thing isn't necessary and a static y origin is fine. I'm changing the view hierarchy to be shallower, so the fudge factor was to compensate for a superview that's not there anymore. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:972: // if |animate| is YES, the changes are made using the animator; otherwise they On 2017/03/23 15:44:53, Avi (slow and ooo 10-23 April) wrote: > Full sentences; capitalize the first letter. Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1090: button = nodeIdToButtonMap_.at(node->id()).get(); On 2017/03/23 15:44:52, Avi (slow and ooo 10-23 April) wrote: > auto buttonIt = nodeIdToButtonMap_.find(nodeId); > if (buttonIt != nodeIdToButtonMap_.end() { > button = *buttonIt; > > lets you reuse the iterator. Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1109: // Do setup On 2017/03/23 15:44:52, Avi (OOO 10-12 April) wrote: > blank line above, period at end Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1110: nodeIdToButtonMap_.insert( On 2017/03/23 15:40:22, Elly Fong-Jones wrote: > why does this happen even if we found the button in nodeIdToButtonMap_ earlier? A certain someone got sloppy rearranging while iterating on this. Fixed. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1138: // Folders show a tooltip iff the title is truncated. On 2017/03/23 15:44:52, Avi (slow and ooo 10-23 April) wrote: > blank line above this comment > > this function needs more blank lines to clarify flow Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1275: [[offTheSideButton_ draggableButton] setDraggable:NO]; On 2017/03/23 15:40:21, Elly Fong-Jones wrote: > this may be a silly question, but if the offTheSideButton_ is never draggable, > why does it have a draggableButton? BookmarkButton is a subclass of DraggableButton. If the question is why is this a BookmarkButton vs. something else, I'm not totally sure (maybe the |bookmarkNode| property?) I can look into this in a follow-up CL, does that sound good? https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1365: // Puts stuff into the final state without animating, stopping a running On 2017/03/23 15:40:21, Elly Fong-Jones wrote: > Can you expand on what "stuff" is? What state specifically is being finalized > here? Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1712: // |point| is in the base coordinate system of the destination window; On 2017/03/23 15:44:53, Avi (OOO 10-12 April) wrote: > Fix run-on sentence. No wonder -- I accidentally smashed two docstrings together while copying them over! Thanks for the catch. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1717: // TODO(mrossetti,jrg): Yet more duplicated code. On 2017/03/23 15:40:21, Elly Fong-Jones wrote: > is the mentioned duplicated code actually still here? I regret to say yes https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/bookmarks/bookma... https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1763: layout_ = {}; // Force layout On 2017/03/23 15:44:53, Avi (slow and ooo 10-23 April) wrote: > . Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1769: // Gets called only by the BookmarkBarView. On 2017/03/23 15:40:21, Elly Fong-Jones wrote: > is it important that this is the case? Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1795: (for example, a drag from the URL bar, a link on the desktop, On 2017/03/23 15:40:21, Elly Fong-Jones wrote: > We use the style with a leading // on every line. At least for me, since I don't > use syntax highlighting, I also find the leading-// style easier to read :) Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1802: is to the left of the drag position, make room. */ On 2017/03/23 15:44:53, Avi (slow and ooo 10-23 April) wrote: > Don't use /* comments. Use // ones for a block. Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1862: } On 2017/03/23 15:44:53, Avi (slow and ooo 10-23 April) wrote: > moar blank lines through this function Done. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2000: - (void)updateSpecialButton:(BookmarkButton*)button On 2017/03/23 15:40:21, Elly Fong-Jones wrote: > "update" is a bit generic - could this function name be more specific about what > it's doing? repositionSpecialButton:toXOffset:hidden: perhaps? Decided to just inline this without the conditional, since not applying an offset when it's hidden isn't much of an optimization and probably isn't worth the indirection cost. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2011: - (void)applyXOffset:(CGFloat)offset On 2017/03/23 15:40:22, Elly Fong-Jones wrote: > the previous function has the button before the offset, so this one probably > should as well... perhaps this could be part of updateSpecialButton: itself? It's also used for the vanilla bookmark buttons. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2101: - (void)updateExtraButtonsVisibility { On 2017/03/23 15:40:21, Elly Fong-Jones wrote: > the code seems to alternate between "special" and "extra" as the word for these > buttons Stuck with extra since that terminology is used outside of Cocoa, though I'm personally not a huge fan. https://codereview.chromium.org/2751573002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2161: BookmarkButton* button = nodeIdToButtonMap_[node->id()]; On 2017/03/23 15:44:53, Avi (slow and ooo 10-23 April) wrote: > do the find outside the if, then you can reuse the iterator here. Done.
Description was changed from ========== Bookmark bar WIP BUG= ========== to ========== Yes, with RTL thrown in, since this is specifically designed to make it almost free. Two major things here: - The bar is no longer relaid-out directly in response to changes in view size, bookmark model etc. Instead, a new UI-direction-agnostic view model (BookmarkBarLayout) is created from the current state, and if it's different from before, it's applied to the view. - Removed a bunch of layout-related code that's no longer necessary BUG=648560 ==========
General encouragement to add more blank lines when appropriate. Maf might have been afraid of empty space but you don't have to be. https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right): https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:351: - (BookmarkButton*)findAncestorButtonOnBarForNode:(const BookmarkNode*)node { Nice rename. https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:451: for (BookmarkButton* button in buttons_.get()) { Do you mean for (.... in buttons.get()) ? Otherwise the new buttons array you're creating above isn't used. https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:493: [self layoutSubviews]; extra line before, so that's it's not clustered under the fullscreen comment https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1662: // If nothing is after me, I am at the end! "me" in this comment is strange. The only other comment in the function has a pronoun "we", which is equally confusing. Can you reword both? https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1779: where < offset + NSWidth(buttonFrame)) { This line feels funny. Why do we have one branch of the OR have an isRTL but not the other? isRTL use should be symmetrical. https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1875: - (void)rebuildLayout:(BOOL)animated { You need to have a label for your parameter, otherwise calls look like [self rebuildLayout:YES]; and it's not obvious what that parameter is. Perhaps -rebuildLayoutWithAnimation:(BOOL)animated ? https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2003: //} blank line, not comment https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2071: // while in the middle of a drag. The "drag completed" code Wow. That sounds like a hell of a story.
https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right): https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1779: where < offset + NSWidth(buttonFrame)) { On 2017/04/10 20:25:54, Avi (concussion NO REVIEWS) wrote: > This line feels funny. Why do we have one branch of the OR have an isRTL but not > the other? isRTL use should be symmetrical. sometimes this happens because even in RTL the coordinate system still has its origin at the bottom-left corner, so flipping a view's coordinates often involves adding/subtracting the width of a view or some padding only in the RTL case. Perhaps there should be a comment here, or a descriptively named helper function to make this a bit more self-documenting. https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2071: // while in the middle of a drag. The "drag completed" code On 2017/04/10 20:25:54, Avi (concussion NO REVIEWS) wrote: > Wow. That sounds like a hell of a story. yeah... is there a crbug for that? https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm (right): https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm:535: TEST_F(BookmarkBarControllerTest, ApplyLayoutSupervisedButton) { I wonder - can we rewrite this as a parameterized test, parameterized on: 1) the selector to invoke on the bar (supervisedBookmarksButton here) 2) the visible element mask to set 3) the x-coord to use? These tests are a bit repetitious.
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
In addition to below comments, added some bug fixes for dragging in RTL and a race condition with the button's dragging code. (Sorry for the late response, was out a while) avi@, I added some more whitespace, let me know if there are any specific areas you find particularly dense. https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right): https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:451: for (BookmarkButton* button in buttons_.get()) { On 2017/04/10 20:25:54, Avi (concussion NO REVIEWS) wrote: > Do you mean > > for (.... in buttons.get()) ? > > Otherwise the new buttons array you're creating above isn't used. Doh! Thanks for this catch https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:493: [self layoutSubviews]; On 2017/04/10 20:25:54, Avi (concussion NO REVIEWS) wrote: > extra line before, so that's it's not clustered under the fullscreen comment Done. https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1662: // If nothing is after me, I am at the end! On 2017/04/10 20:25:54, Avi (concussion NO REVIEWS) wrote: > "me" in this comment is strange. The only other comment in the function has a > pronoun "we", which is equally confusing. Can you reword both? Done. Simplified the logic a bit so the other comment isn't needed any longer. https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1779: where < offset + NSWidth(buttonFrame)) { On 2017/04/26 13:36:02, Elly Fong-Jones wrote: > On 2017/04/10 20:25:54, Avi (concussion NO REVIEWS) wrote: > > This line feels funny. Why do we have one branch of the OR have an isRTL but > not > > the other? isRTL use should be symmetrical. > > sometimes this happens because even in RTL the coordinate system still has its > origin at the bottom-left corner, so flipping a view's coordinates often > involves adding/subtracting the width of a view or some padding only in the RTL > case. Perhaps there should be a comment here, or a descriptively named helper > function to make this a bit more self-documenting. Agree that this was confusing. It was also hiding a nasty (and in hindsight really obvious) logic bug, so thanks for making me look at it again! FWIW, this wasn't actually asymmetric. Rather, the logic for the else condition was more straightforward[1] so I baked it into the conditional. Hopefully this is more clear now? [1] Famous last words, as it turns out. https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1875: - (void)rebuildLayout:(BOOL)animated { On 2017/04/10 20:25:54, Avi (concussion NO REVIEWS) wrote: > You need to have a label for your parameter, otherwise calls look like > > [self rebuildLayout:YES]; > > and it's not obvious what that parameter is. Perhaps > -rebuildLayoutWithAnimation:(BOOL)animated ? Done. https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2003: //} On 2017/04/10 20:25:54, Avi (concussion NO REVIEWS) wrote: > blank line, not comment Done. https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2071: // while in the middle of a drag. The "drag completed" code On 2017/04/26 13:36:02, Elly Fong-Jones wrote: > On 2017/04/10 20:25:54, Avi (concussion NO REVIEWS) wrote: > > Wow. That sounds like a hell of a story. > > yeah... is there a crbug for that? Agree :D Blame on this comment goes back to https://chromium.googlesource.com/chromium/src/+log/7d791652c7ede4209a2014d88... Not sure how to dig back before that https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm (right): https://codereview.chromium.org/2751573002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm:535: TEST_F(BookmarkBarControllerTest, ApplyLayoutSupervisedButton) { On 2017/04/26 13:36:02, Elly Fong-Jones wrote: > I wonder - can we rewrite this as a parameterized test, parameterized on: > > 1) the selector to invoke on the bar (supervisedBookmarksButton here) > 2) the visible element mask to set > 3) the x-coord to use? > > These tests are a bit repetitious. SGTM, will follow up with something like this in the next patchset
Looks reasonable; good luck. LGTM https://codereview.chromium.org/2751573002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right): https://codereview.chromium.org/2751573002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1575: : array; [array objectEnumerator] for parallelism? https://codereview.chromium.org/2751573002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1806: // Lay out "extra" buttons (apps, managed, supervised, "Other"). double space https://codereview.chromium.org/2751573002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm (right): https://codereview.chromium.org/2751573002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm:907: EXPECT_EQ(layout.VisibleButtonCount(), buttonCountBeforeAdding); You may want to space this out. At least a blank line before the comment lines.
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Updated with promised test updates https://codereview.chromium.org/2751573002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right): https://codereview.chromium.org/2751573002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1575: : array; On 2017/04/26 20:25:41, Avi (ping after 24h) wrote: > [array objectEnumerator] for parallelism? Done. https://codereview.chromium.org/2751573002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1806: // Lay out "extra" buttons (apps, managed, supervised, "Other"). On 2017/04/26 20:25:41, Avi (ping after 24h) wrote: > double space Done. https://codereview.chromium.org/2751573002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm (right): https://codereview.chromium.org/2751573002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm:907: EXPECT_EQ(layout.VisibleButtonCount(), buttonCountBeforeAdding); On 2017/04/26 20:25:42, Avi (ping after 24h) wrote: > You may want to space this out. At least a blank line before the comment lines. Done.
Still LGTM. https://codereview.chromium.org/2751573002/diff/180001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right): https://codereview.chromium.org/2751573002/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1572: id<NSFastEnumeration> buttonsToCheck = NSEnumerator*
lgtm nice work deduplicating the test logic :)
The CQ bit was checked by lgrey@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Elly and Avi, thanks for the great review!
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493662106544220,
"parent_rev": "b24d35ae72703093d610e86b149d7f21010381ea", "commit_rev":
"2644729cb7722a702a76cc2758d0ce372e1e6f92"}
Message was sent while issue was closed.
Description was changed from ========== Yes, with RTL thrown in, since this is specifically designed to make it almost free. Two major things here: - The bar is no longer relaid-out directly in response to changes in view size, bookmark model etc. Instead, a new UI-direction-agnostic view model (BookmarkBarLayout) is created from the current state, and if it's different from before, it's applied to the view. - Removed a bunch of layout-related code that's no longer necessary BUG=648560 ========== to ========== Yes, with RTL thrown in, since this is specifically designed to make it almost free. Two major things here: - The bar is no longer relaid-out directly in response to changes in view size, bookmark model etc. Instead, a new UI-direction-agnostic view model (BookmarkBarLayout) is created from the current state, and if it's different from before, it's applied to the view. - Removed a bunch of layout-related code that's no longer necessary BUG=648560 Review-Url: https://codereview.chromium.org/2751573002 Cr-Commit-Position: refs/heads/master@{#468364} Committed: https://chromium.googlesource.com/chromium/src/+/2644729cb7722a702a76cc2758d0... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/2644729cb7722a702a76cc2758d0...
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/2853123002/ by sky@chromium.org. The reason for reverting is: Reverting as likely caused msan failurs. See https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.memory%2FMac_AS... for example: ==77706==ERROR: AddressSanitizer: heap-use-after-free on address 0x603000445b40 at pc 0x0001156ab4fe bp 0x7fff5c399fd0 sp 0x7fff5c399fc8 READ of size 8 at 0x603000445b40 thread T0 #0 0x1156ab4fd in -[BookmarkBarController applyLayout:animated:] ??:0:0 #1 0x1156a9db1 in -[BookmarkBarController rebuildLayoutWithAnimated:] ??:0:0 #2 0x1156acf25 in -[BookmarkBarController nodeRemoved:parent:index:] ??:0:0 #3 0x1127774c7 in bookmarks::BookmarkModel::RemoveAndDeleteNode(bookmarks::BookmarkNode*) ??:0:0 #4 0x112776d40 in bookmarks::BookmarkModel::Remove(bookmarks::BookmarkNode const*) ??:0:0 #5 0x10607a64c in (anonymous namespace)::BookmarkFolderAppleScriptTest_DeleteBookmarkItems_Test::RunTestOnMainThread() ??:0:0 #6 0x10d1f6ac3 in content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() ??:0:0 #7 0x10bb87dc3 in ChromeBrowserMainParts::PreMainMessageLoopRunImpl() ??:0:0 #8 0x10bb851e6 in ChromeBrowserMainParts::PreMainMessageLoopRun() ??:0:0 #9 0x1077b02dd in content::BrowserMainLoop::PreMainMessageLoopRun() ??:0:0 #10 0x1084ba8a3 in content::StartupTaskRunner::RunAllTasksNow() ??:0:0 #11 0x1077abc3a in content::BrowserMainLoop::CreateStartupTasks() ??:0:0 #12 0x1077b94ec in content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) ??:0:0 #13 0x1077a40b5 in content::BrowserMain(content::MainFunctionParams const&) ??:0:0 #14 0x10b6a9604 in content::RunNamedProcessTypeMain(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, content::MainFunctionParams const&, content::ContentMainDelegate*) ??:0:0 #15 0x10b6ab3c7 in content::ContentMainRunnerImpl::Run() ??:0:0 #16 0x111afc88b in service_manager::Main(service_manager::MainParams const&) ??:0:0 #17 0x10b6a8ff4 in content::ContentMain(content::ContentMainParams const&) ??:0:0 #18 0x10d1f5c4f in content::BrowserTestBase::SetUp() ??:0:0 #19 0x10ba09201 in InProcessBrowserTest::SetUp() ??:0:0 #20 0x10ed76b2f in testing::Test::Run() ??:0:0 #21 0x10ed78b43 in testing::TestInfo::Run() ??:0:0 #22 0x10ed79e86 in testing::TestCase::Run() ??:0:0 #23 0x10ed8cfb6 in testing::internal::UnitTestImpl::RunAllTests() ??:0:0 #24 0x10ed8c588 in testing::UnitTest::Run() ??:0:0 #25 0x10ba51d3e in base::TestSuite::Run() ??:0:0 #26 0x10b6fc3ac in ChromeTestSuiteRunner::RunTestSuite(int, char**) ??:0:0 #27 0x10d2c7991 in content::LaunchTests(content::TestLauncherDelegate*, int, int, char**) ??:0:0 #28 0x10b6fc203 in main ??:0:0 #29 0x7fff8a0125fc in start ??:0:0 . |
