Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(140)

Issue 1099403005: [AiS] changing mac omnibox suggestions form NSMatrix to NSTableView (Closed)

Created:
5 years, 8 months ago by dschuyler
Modified:
5 years, 6 months ago
CC:
chromium-reviews, James Su, shrike, Alexei Svitkine (slow)
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[AiS] changing mac omnibox suggestions form NSMatrix to NSTableView This initial change should look the same from the user's perspective. BUG=449593 Committed: https://crrev.com/aa726aa5c1121fc55c9cf5b40e18ca1d0a9b7337 Cr-Commit-Position: refs/heads/master@{#334305}

Patch Set 1 #

Total comments: 43

Patch Set 2 : review changes #

Total comments: 39

Patch Set 3 : review changes, changed hover and selection tracking #

Patch Set 4 : removed old code, fix for unit test #

Patch Set 5 : fixes for mac unit tests; changed cpp delegate function names #

Patch Set 6 : cl format changes #

Patch Set 7 : Reworked AiS table view data source #

Patch Set 8 : Removed unneeded code #

Total comments: 17

Patch Set 9 : fix for unit test #

Patch Set 10 : Reduced cell redraw on hover #

Total comments: 15

Patch Set 11 : Review changes and rounding error fix #

Total comments: 40

Patch Set 12 : Int to float changes and cleanup some table overrides #

Patch Set 13 : added comment about copy with zone #

Patch Set 14 : Added popup cell data class #

Patch Set 15 : unit test fix #

Total comments: 17

Patch Set 16 : Review changes #

Total comments: 45

Patch Set 17 : changed accessors for OmniboxPopupCellData #

Patch Set 18 : Removed cell data rect #

Total comments: 36

Patch Set 19 : The role of the DataSource is more clear #

Patch Set 20 : Removed withView arg from highlightRowAt #

Total comments: 101

Patch Set 21 : Review changes #

Patch Set 22 : Merge from master #

Patch Set 23 : Fix for interactive_ui_tests. #

Patch Set 24 : Changed OmniboxPopupCellData to be in objectValue rather than representedObject #

Patch Set 25 : Merge from master to fix unit test #

Patch Set 26 : merging master; unit test fix #

Patch Set 27 : moved description access to test code #

Total comments: 8

Patch Set 28 : Moved cell drawing code #

Patch Set 29 : added NSCopying to fix unit test #

Patch Set 30 : fix for browswer tests #

Patch Set 31 : Merge form master #

Total comments: 41

Patch Set 32 : Review changes #

Patch Set 33 : moved separator init #

Total comments: 40

Patch Set 34 : reveiw changes #

Patch Set 35 : removed prefix centering #

Patch Set 36 : Fix for unused var #

Total comments: 25

Patch Set 37 : Fixes for init of content offset #

Patch Set 38 : Added member declarations #

Patch Set 39 : review changes; comments; objc casts #

Patch Set 40 : autorelease on column data cell #

Unified diffs Side-by-side diffs Delta from patch set Stats (+444 lines, -296 lines) Patch
M chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 1 chunk +37 lines, -22 lines 0 comments Download
M chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 5 chunks +159 lines, -149 lines 0 comments Download
M chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell_unittest.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 3 chunks +32 lines, -17 lines 0 comments Download
M chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 3 chunks +45 lines, -1 line 0 comments Download
M chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 7 chunks +134 lines, -40 lines 0 comments Download
M chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix_unittest.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 6 chunks +15 lines, -10 lines 0 comments Download
M chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 7 chunks +19 lines, -54 lines 0 comments Download

Messages

Total messages: 77 (6 generated)
dschuyler
Hi, I need to work on the unit tests for this CL, so it's not ...
5 years, 8 months ago (2015-04-24 18:43:09 UTC) #2
groby-ooo-7-16
Just a quick scan... https://codereview.chromium.org/1099403005/diff/1/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (left): https://codereview.chromium.org/1099403005/diff/1/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h#oldcode21 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:21: OmniboxPopupViewMac* parent_; On 2015/04/24 18:43:08, ...
5 years, 8 months ago (2015-04-24 20:11:17 UTC) #3
Robert Sesek
shess@ should be at least CC'd on all Mac Omnibox reviews (unless he doesn't want ...
5 years, 8 months ago (2015-04-24 23:20:57 UTC) #5
dschuyler
https://codereview.chromium.org/1099403005/diff/1/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/1099403005/diff/1/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm#newcode213 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:213: NSAttributedString* separator = separator_.release(); On 2015/04/24 20:11:16, groby wrote: ...
5 years, 8 months ago (2015-04-25 01:05:43 UTC) #6
groby-ooo-7-16
https://codereview.chromium.org/1099403005/diff/1/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/1099403005/diff/1/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm#newcode213 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:213: NSAttributedString* separator = separator_.release(); On 2015/04/25 01:05:42, dschuyler wrote: ...
5 years, 8 months ago (2015-04-25 01:31:11 UTC) #7
Scott Hess - ex-Googler
On 2015/04/24 23:20:57, Robert Sesek (OOO 24.4-11.5) wrote: > shess@ should be at least CC'd ...
5 years, 8 months ago (2015-04-25 06:04:28 UTC) #8
groby-ooo-7-16
https://codereview.chromium.org/1099403005/diff/20001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): https://codereview.chromium.org/1099403005/diff/20001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm#newcode59 chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:59: return array_; On 2015/04/25 06:04:27, Scott Hess wrote: > ...
5 years, 8 months ago (2015-04-25 19:01:12 UTC) #9
shrike
Aside from other reviewer comments, LGTM. https://codereview.chromium.org/1099403005/diff/20001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/1099403005/diff/20001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm#newcode224 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:224: // Enlarge the ...
5 years, 8 months ago (2015-04-27 18:30:11 UTC) #11
dschuyler
Hi, This has been reworked quite a bit. Thanks to all three of you for ...
5 years, 7 months ago (2015-05-01 21:50:54 UTC) #12
Scott Hess - ex-Googler
I am feeling a little wobbly on the NSTableView specifics going on, here, like I ...
5 years, 7 months ago (2015-05-04 19:40:21 UTC) #13
dschuyler
https://codereview.chromium.org/1099403005/diff/140001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/1099403005/diff/140001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm#newcode221 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:221: } On 2015/05/04 19:40:20, Scott Hess wrote: > WRT ...
5 years, 7 months ago (2015-05-04 23:51:41 UTC) #14
dschuyler
Hi Scott, This latest patch greatly reduced the number of cell redraws (based on your ...
5 years, 7 months ago (2015-05-05 00:29:39 UTC) #15
groby-ooo-7-16
Once more, a drive-by. https://codereview.chromium.org/1099403005/diff/180001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/1099403005/diff/180001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h#newcode24 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:24: @interface OmniboxPopupCell : NSCell { ...
5 years, 7 months ago (2015-05-07 02:39:59 UTC) #16
dschuyler
https://codereview.chromium.org/1099403005/diff/180001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/1099403005/diff/180001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h#newcode24 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:24: @interface OmniboxPopupCell : NSCell { On 2015/05/07 02:39:58, groby ...
5 years, 7 months ago (2015-05-07 20:53:10 UTC) #17
Scott Hess - ex-Googler
As mentioned earlier, I feel like we might be missing someone who knows answers to ...
5 years, 7 months ago (2015-05-07 22:35:43 UTC) #18
groby-ooo-7-16
Dave and I discussed the magic of -copyWithZone: offline, sorry for forgetting to update the ...
5 years, 7 months ago (2015-05-08 03:24:09 UTC) #19
Scott Hess - ex-Googler
https://codereview.chromium.org/1099403005/diff/200001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/1099403005/diff/200001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm#newcode209 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:209: NSAttributedString* prefix = prefix_.release(); On 2015/05/08 03:24:08, groby wrote: ...
5 years, 7 months ago (2015-05-08 04:11:48 UTC) #20
groby-ooo-7-16
My apologies... At least I didn't discuss the variant that involves separator(separator_.autorelease()) :) https://codereview.chromium.org/1099403005/diff/200001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm File ...
5 years, 7 months ago (2015-05-08 05:08:29 UTC) #21
Scott Hess - ex-Googler
https://codereview.chromium.org/1099403005/diff/200001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/1099403005/diff/200001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm#newcode209 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:209: NSAttributedString* prefix = prefix_.release(); On 2015/05/08 05:08:29, groby wrote: ...
5 years, 7 months ago (2015-05-08 06:00:13 UTC) #22
groby-ooo-7-16
Oh, memcpy is callable from Swift. You shouldn't, but neither should you do the nonsense ...
5 years, 7 months ago (2015-05-08 06:20:44 UTC) #23
Scott Hess - ex-Googler
https://codereview.chromium.org/1099403005/diff/200001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/1099403005/diff/200001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm#newcode209 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:209: NSAttributedString* prefix = prefix_.release(); On 2015/05/08 06:20:43, groby wrote: ...
5 years, 7 months ago (2015-05-08 18:25:50 UTC) #24
groby-ooo-7-16
On 2015/05/08 18:25:50, Scott Hess wrote: > https://codereview.chromium.org/1099403005/diff/200001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm > File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): > > https://codereview.chromium.org/1099403005/diff/200001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm#newcode209 ...
5 years, 7 months ago (2015-05-08 18:46:29 UTC) #25
groby-ooo-7-16
On 2015/05/08 18:46:29, groby wrote: > On 2015/05/08 18:25:50, Scott Hess wrote: > > > ...
5 years, 7 months ago (2015-05-08 18:48:17 UTC) #26
dschuyler
I had a couple false-starts on different directions. How does this one look? It now ...
5 years, 7 months ago (2015-05-13 01:41:12 UTC) #27
shrike
https://codereview.chromium.org/1099403005/diff/280001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/1099403005/diff/280001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h#newcode37 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:37: bool isContentsRTL_; Should this be C++ bool or ObjC ...
5 years, 7 months ago (2015-05-14 18:02:39 UTC) #28
dschuyler
https://codereview.chromium.org/1099403005/diff/280001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/1099403005/diff/280001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h#newcode37 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:37: bool isContentsRTL_; On 2015/05/14 18:02:38, shrike wrote: > Should ...
5 years, 7 months ago (2015-05-14 22:28:20 UTC) #29
Scott Hess - ex-Googler
Thanks for bearing with us. https://codereview.chromium.org/1099403005/diff/200001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): https://codereview.chromium.org/1099403005/diff/200001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm#newcode36 chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:36: } On 2015/05/13 01:41:11, ...
5 years, 7 months ago (2015-05-15 00:09:39 UTC) #30
groby-ooo-7-16
https://codereview.chromium.org/1099403005/diff/280001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/1099403005/diff/280001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h#newcode37 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:37: bool isContentsRTL_; On 2015/05/15 00:09:38, Scott Hess wrote: > ...
5 years, 7 months ago (2015-05-15 16:53:57 UTC) #31
Scott Hess - ex-Googler
https://codereview.chromium.org/1099403005/diff/300001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/1099403005/diff/300001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm#newcode237 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:237: // dschuyler about why it's written with swaps and ...
5 years, 7 months ago (2015-05-15 17:29:00 UTC) #32
dschuyler
Thanks again! In an email I asked about how to test the TAIL and prefix ...
5 years, 7 months ago (2015-05-15 21:52:04 UTC) #33
Scott Hess - ex-Googler
https://codereview.chromium.org/1099403005/diff/300001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/1099403005/diff/300001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm#newcode165 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:165: return self; On 2015/05/15 21:52:04, dschuyler wrote: > On ...
5 years, 7 months ago (2015-05-15 22:02:08 UTC) #34
groby-ooo-7-16
https://codereview.chromium.org/1099403005/diff/300001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/1099403005/diff/300001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm#newcode165 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:165: return self; On 2015/05/15 22:02:07, Scott Hess wrote: > ...
5 years, 7 months ago (2015-05-15 22:41:33 UTC) #35
dschuyler
https://codereview.chromium.org/1099403005/diff/300001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/1099403005/diff/300001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm#newcode165 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:165: return self; On 2015/05/15 22:02:07, Scott Hess wrote: > ...
5 years, 7 months ago (2015-05-18 23:38:44 UTC) #36
groby-ooo-7-16
Sorry, more... I still think we're not building the data source properly - pretty much ...
5 years, 7 months ago (2015-05-20 01:02:31 UTC) #37
dschuyler
https://codereview.chromium.org/1099403005/diff/340001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/1099403005/diff/340001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h#newcode61 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:61: @interface OmniboxPopupCell : NSCell { On 2015/05/20 01:02:30, groby ...
5 years, 7 months ago (2015-05-21 00:38:39 UTC) #38
groby-ooo-7-16
Sorry for dragging this out... But we're almost there, I promise! https://codereview.chromium.org/1099403005/diff/340001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): ...
5 years, 7 months ago (2015-05-21 03:01:49 UTC) #40
Scott Hess - ex-Googler
https://codereview.chromium.org/1099403005/diff/340001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): https://codereview.chromium.org/1099403005/diff/340001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm#newcode35 chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:35: return nil; On 2015/05/21 03:01:48, groby wrote: > On ...
5 years, 7 months ago (2015-05-21 04:46:36 UTC) #41
Scott Hess - ex-Googler
https://codereview.chromium.org/1099403005/diff/380001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/1099403005/diff/380001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h#newcode41 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:41: - (id)initWithMatch:(const AutocompleteMatch&)match image:(NSImage*)image; Instead of (id) use (instancetype). ...
5 years, 7 months ago (2015-05-21 20:40:28 UTC) #42
groby-ooo-7-16
https://codereview.chromium.org/1099403005/diff/380001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/1099403005/diff/380001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm#newcode354 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:354: // The representedObject is set to nil in the ...
5 years, 7 months ago (2015-05-22 05:03:07 UTC) #43
Scott Hess - ex-Googler
https://codereview.chromium.org/1099403005/diff/380001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell_unittest.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell_unittest.mm (right): https://codereview.chromium.org/1099403005/diff/380001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell_unittest.mm#newcode14 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell_unittest.mm:14: @end On 2015/05/22 05:03:07, groby wrote: > On 2015/05/21 ...
5 years, 7 months ago (2015-05-22 05:17:58 UTC) #44
Scott Hess - ex-Googler
I'm going OOT starting tomorrow, and hope to be out of reach of shess@chromium.org . ...
5 years, 7 months ago (2015-05-22 21:24:07 UTC) #45
dschuyler
WIP on the objectValue vs representedObject discussion. This is an interim patch trying to tie ...
5 years, 7 months ago (2015-05-26 18:40:22 UTC) #46
groby-ooo-7-16
Waiting for -objectValue for a full pass :) https://codereview.chromium.org/1099403005/diff/340001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/1099403005/diff/340001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h#newcode61 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:61: @interface ...
5 years, 7 months ago (2015-05-26 21:23:22 UTC) #47
dschuyler
https://codereview.chromium.org/1099403005/diff/340001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/1099403005/diff/340001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h#newcode61 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:61: @interface OmniboxPopupCell : NSCell { On 2015/05/26 21:23:22, groby ...
5 years, 6 months ago (2015-05-28 20:34:22 UTC) #48
Scott Hess - ex-Googler
minor style-type responses which really probably don't matter to the CL. https://codereview.chromium.org/1099403005/diff/380001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): ...
5 years, 6 months ago (2015-05-29 18:00:26 UTC) #49
groby-ooo-7-16
A few quick drive-by questions. https://codereview.chromium.org/1099403005/diff/520001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/1099403005/diff/520001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm#newcode363 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:363: - (id)copyWithZone:(NSZone*)zone { Why ...
5 years, 6 months ago (2015-06-05 01:28:47 UTC) #50
dschuyler
https://codereview.chromium.org/1099403005/diff/380001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): https://codereview.chromium.org/1099403005/diff/380001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm#newcode37 chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:37: base::scoped_nsobject<NSMutableArray> array([[NSMutableArray alloc] init]); On 2015/05/29 18:00:26, Scott Hess ...
5 years, 6 months ago (2015-06-09 01:30:47 UTC) #51
dschuyler
https://codereview.chromium.org/1099403005/diff/380001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): https://codereview.chromium.org/1099403005/diff/380001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm#newcode40 chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:40: for (size_t ii = 0; ii < rows; ++ii) ...
5 years, 6 months ago (2015-06-10 20:48:31 UTC) #52
groby-ooo-7-16
Almost, with a few more fixes. https://codereview.chromium.org/1099403005/diff/600001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/1099403005/diff/600001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h#newcode14 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:14: @class OmniboxPopupCell; I ...
5 years, 6 months ago (2015-06-11 01:22:18 UTC) #53
Scott Hess - ex-Googler
https://codereview.chromium.org/1099403005/diff/600001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/1099403005/diff/600001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm#newcode371 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:371: OmniboxPopupCellData* copy = [[OmniboxPopupCellData alloc] init]; On 2015/06/11 01:22:17, ...
5 years, 6 months ago (2015-06-11 21:52:03 UTC) #54
dschuyler
https://codereview.chromium.org/1099403005/diff/600001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/1099403005/diff/600001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h#newcode14 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:14: @class OmniboxPopupCell; On 2015/06/11 01:22:17, groby wrote: > I ...
5 years, 6 months ago (2015-06-11 22:34:23 UTC) #55
Scott Hess - ex-Googler
https://codereview.chromium.org/1099403005/diff/600001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): https://codereview.chromium.org/1099403005/diff/600001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm#newcode64 chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:64: array_.reset([array retain]); On 2015/06/11 22:34:22, dschuyler wrote: > On ...
5 years, 6 months ago (2015-06-11 22:36:19 UTC) #56
dschuyler
https://codereview.chromium.org/1099403005/diff/600001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): https://codereview.chromium.org/1099403005/diff/600001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm#newcode64 chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:64: array_.reset([array retain]); On 2015/06/11 22:36:19, Scott Hess wrote: > ...
5 years, 6 months ago (2015-06-12 00:43:26 UTC) #57
groby-ooo-7-16
https://codereview.chromium.org/1099403005/diff/640001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/1099403005/diff/640001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h#newcode38 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:38: @property(retain, nonatomic) NSAttributedString* contents; We ultimately want this readonly, ...
5 years, 6 months ago (2015-06-12 18:51:07 UTC) #58
Scott Hess - ex-Googler
https://codereview.chromium.org/1099403005/diff/600001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): https://codereview.chromium.org/1099403005/diff/600001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm#newcode64 chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:64: array_.reset([array retain]); On 2015/06/12 00:43:26, dschuyler wrote: > On ...
5 years, 6 months ago (2015-06-12 20:21:32 UTC) #59
groby-ooo-7-16
https://codereview.chromium.org/1099403005/diff/640001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/1099403005/diff/640001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm#newcode358 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:358: description_ = [answerString.release() retain]; On 2015/06/12 20:21:32, Scott Hess ...
5 years, 6 months ago (2015-06-12 21:01:54 UTC) #60
dschuyler
https://codereview.chromium.org/1099403005/diff/640001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/1099403005/diff/640001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h#newcode38 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:38: @property(retain, nonatomic) NSAttributedString* contents; On 2015/06/12 18:51:06, groby wrote: ...
5 years, 6 months ago (2015-06-12 21:39:14 UTC) #61
dschuyler
https://codereview.chromium.org/1099403005/diff/600001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): https://codereview.chromium.org/1099403005/diff/600001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm#newcode64 chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:64: array_.reset([array retain]); On 2015/06/12 20:21:32, Scott Hess wrote: > ...
5 years, 6 months ago (2015-06-12 21:44:31 UTC) #62
Scott Hess - ex-Googler
I'm losing the will to live, here. I'll try to be more pedantic if I ...
5 years, 6 months ago (2015-06-12 22:21:59 UTC) #63
groby-ooo-7-16
https://codereview.chromium.org/1099403005/diff/600001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): https://codereview.chromium.org/1099403005/diff/600001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm#newcode64 chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:64: array_.reset([array retain]); On 2015/06/12 21:44:30, dschuyler wrote: > On ...
5 years, 6 months ago (2015-06-12 22:24:30 UTC) #64
groby-ooo-7-16
5 years, 6 months ago (2015-06-12 22:24:39 UTC) #65
groby-ooo-7-16
LGTM % comments. https://codereview.chromium.org/1099403005/diff/700001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): https://codereview.chromium.org/1099403005/diff/700001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm#newcode123 chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:123: [column setDataCell:[[OmniboxPopupCell alloc] init]]; On 2015/06/12 ...
5 years, 6 months ago (2015-06-12 22:43:02 UTC) #66
Scott Hess - ex-Googler
https://codereview.chromium.org/1099403005/diff/700001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): https://codereview.chromium.org/1099403005/diff/700001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm#newcode123 chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:123: [column setDataCell:[[OmniboxPopupCell alloc] init]]; On 2015/06/12 22:43:01, groby wrote: ...
5 years, 6 months ago (2015-06-12 22:49:20 UTC) #67
groby-ooo-7-16
https://codereview.chromium.org/1099403005/diff/700001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): https://codereview.chromium.org/1099403005/diff/700001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm#newcode123 chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:123: [column setDataCell:[[OmniboxPopupCell alloc] init]]; On 2015/06/12 22:49:20, Scott Hess ...
5 years, 6 months ago (2015-06-12 22:58:46 UTC) #68
dschuyler
https://codereview.chromium.org/1099403005/diff/700001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/1099403005/diff/700001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h#newcode17 chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:17: // NSAttributedString instances for various match components. On 2015/06/12 ...
5 years, 6 months ago (2015-06-12 23:07:54 UTC) #69
groby-ooo-7-16
On 2015/06/12 23:07:54, dschuyler wrote: > https://codereview.chromium.org/1099403005/diff/700001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h > File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): > > https://codereview.chromium.org/1099403005/diff/700001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h#newcode17 > ...
5 years, 6 months ago (2015-06-12 23:17:15 UTC) #70
Scott Hess - ex-Googler
LGTM stands. https://codereview.chromium.org/1099403005/diff/700001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): https://codereview.chromium.org/1099403005/diff/700001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm#newcode82 chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:82: row:(NSInteger)rowIndex { On 2015/06/12 23:07:54, dschuyler wrote: ...
5 years, 6 months ago (2015-06-12 23:18:13 UTC) #71
dschuyler
https://codereview.chromium.org/1099403005/diff/700001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): https://codereview.chromium.org/1099403005/diff/700001/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm#newcode123 chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:123: [column setDataCell:[[OmniboxPopupCell alloc] init]]; On 2015/06/12 23:18:13, Scott Hess ...
5 years, 6 months ago (2015-06-13 00:11:04 UTC) #72
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1099403005/770001
5 years, 6 months ago (2015-06-13 00:38:42 UTC) #75
commit-bot: I haz the power
Committed patchset #40 (id:770001)
5 years, 6 months ago (2015-06-13 00:48:33 UTC) #76
commit-bot: I haz the power
5 years, 6 months ago (2015-06-13 00:49:36 UTC) #77
Message was sent while issue was closed.
Patchset 40 (id:??) landed as
https://crrev.com/aa726aa5c1121fc55c9cf5b40e18ca1d0a9b7337
Cr-Commit-Position: refs/heads/master@{#334305}

Powered by Google App Engine
This is Rietveld 408576698