|
|
Created:
3 years, 7 months ago by tommycli Modified:
3 years, 6 months ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews, jdonnelly+watch_chromium.org, mac-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionOmnibox UI Experiments: Port Vertical Layout experiment to Mac Cocoa.
Ports https://codereview.chromium.org/2901953002 Omnibox vertical
layout experiment to Mac Cocoa.
BUG=725599
Review-Url: https://codereview.chromium.org/2906893004
Cr-Commit-Position: refs/heads/master@{#476001}
Committed: https://chromium.googlesource.com/chromium/src/+/81081694021693dc29c3ff9bde3da5003510a0ce
Patch Set 1 #
Total comments: 1
Patch Set 2 : fix camel case #
Total comments: 15
Patch Set 3 : address groby@ comments #Patch Set 4 : fix usage of BOOL #Patch Set 5 : fix a mistake... #Patch Set 6 : Prevent dimming in swap case #
Total comments: 3
Messages
Total messages: 21 (14 generated)
tommycli@chromium.org changed reviewers: + groby@chromium.org
groby: PTAL, should be a straight port of changes in https://codereview.chromium.org/2901953002/diff/60001/chrome/browser/ui/views... Screenshots in bug. Thanks! https://codereview.chromium.org/2906893004/diff/1/chrome/browser/ui/cocoa/omn... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (left): https://codereview.chromium.org/2906893004/diff/1/chrome/browser/ui/cocoa/omn... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:428: maxLines_ = 1; Unused for non-answers. Probably good to followup and change it to answersInSuggestLines_ or something.
The CQ bit was checked by tommycli@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:71: + (CGFloat)getContentTextHeight:(BOOL)isTwoLine; Goal for cocoa functions is usually to be readable without additional header search. Bool parameters make that hard :) Suggestion: getContentTextHeightForDoubleLine:(BOOL) (I'm not super-happy with that either, but we already do it above with ForDarkTheme) https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:434: // Somtimes swap the contents and description in vertical layouts. When is "sometimes"? https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:436: // in the underlying match instead of the UI code. Rendering order should not really affect anything *but* UI code. https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:437: NSAttributedString* temp = contents_; std::swap(contents_, description_) https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:563: origin.x = left; If you assign the whole point: origin = NSMakePoint(left, halfLineHeight * 2); https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:111: bool isAnswer = [[array_ objectAtIndex:row] isAnswer]; BOOL, please :) https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:112: bool isTwoLine = !isAnswer && base::FeatureList::IsEnabled( isDoubleLine or usesTwoLines, please. (isTwoLine seems like it's missing a word)
The CQ bit was checked by tommycli@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...
groby: thanks! https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:71: + (CGFloat)getContentTextHeight:(BOOL)isTwoLine; On 2017/05/31 00:14:51, groby wrote: > Goal for cocoa functions is usually to be readable without additional header > search. Bool parameters make that hard :) > > Suggestion: > getContentTextHeightForDoubleLine:(BOOL) > > (I'm not super-happy with that either, but we already do it above with > ForDarkTheme) Done. https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:434: // Somtimes swap the contents and description in vertical layouts. On 2017/05/31 00:14:51, groby wrote: > When is "sometimes"? Done. https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:436: // in the underlying match instead of the UI code. On 2017/05/31 00:14:51, groby wrote: > Rendering order should not really affect anything *but* UI code. Done. https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:437: NSAttributedString* temp = contents_; On 2017/05/31 00:14:51, groby wrote: > std::swap(contents_, description_) Done. Always forget that ObjC is also C++ ... or is it... i'm not completely sure. https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:563: origin.x = left; On 2017/05/31 00:14:51, groby wrote: > If you assign the whole point: > > origin = NSMakePoint(left, halfLineHeight * 2); Hey. I actually need to add to the y coordinate, rather than re-assign it. https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm (right): https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:111: bool isAnswer = [[array_ objectAtIndex:row] isAnswer]; On 2017/05/31 00:14:51, groby wrote: > BOOL, please :) Done. https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm:112: bool isTwoLine = !isAnswer && base::FeatureList::IsEnabled( On 2017/05/31 00:14:51, groby wrote: > isDoubleLine or usesTwoLines, please. (isTwoLine seems like it's missing a word) Done. https://codereview.chromium.org/2906893004/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/2906893004/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:436: : DimTextColor(isDarkTheme), I restructured the code... and addressed your comments. One extra addition is not applying DimTextColor if swapMatchText is YES. That is necessary for correct display.
The CQ bit was checked by tommycli@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...
LGTM % text formatting issue. https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/2906893004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:563: origin.x = left; On 2017/05/31 18:20:39, tommycli wrote: > On 2017/05/31 00:14:51, groby wrote: > > If you assign the whole point: > > > > origin = NSMakePoint(left, halfLineHeight * 2); > > Hey. I actually need to add to the y coordinate, rather than re-assign it. And so you do. TIL my brain skips characters at color boundaries, sorry :( https://codereview.chromium.org/2906893004/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/2906893004/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:436: : DimTextColor(isDarkTheme), On 2017/05/31 18:20:40, tommycli wrote: > I restructured the code... and addressed your comments. One extra addition is > not applying DimTextColor if swapMatchText is YES. That is necessary for correct > display. Do you need to do the same thing for |contents_|? :) If so, it might be easier to pull out two strings, swap if necessary, and then apply Content/Description styling without a ternary.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanks! https://codereview.chromium.org/2906893004/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/2906893004/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:436: : DimTextColor(isDarkTheme), On 2017/05/31 19:24:17, groby wrote: > On 2017/05/31 18:20:40, tommycli wrote: > > I restructured the code... and addressed your comments. One extra addition is > > not applying DimTextColor if swapMatchText is YES. That is necessary for > correct > > display. > > Do you need to do the same thing for |contents_|? :) > > If so, it might be easier to pull out two strings, swap if necessary, and then > apply Content/Description styling without a ternary. > Hey, I don't. I just need to cancel the force-dimming of the description.
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496261246903660, "parent_rev": "a7e95918de8e2e63e460132f21070ac82a6140db", "commit_rev": "81081694021693dc29c3ff9bde3da5003510a0ce"}
Message was sent while issue was closed.
Description was changed from ========== Omnibox UI Experiments: Port Vertical Layout experiment to Mac Cocoa. Ports https://codereview.chromium.org/2901953002 Omnibox vertical layout experiment to Mac Cocoa. BUG=725599 ========== to ========== Omnibox UI Experiments: Port Vertical Layout experiment to Mac Cocoa. Ports https://codereview.chromium.org/2901953002 Omnibox vertical layout experiment to Mac Cocoa. BUG=725599 Review-Url: https://codereview.chromium.org/2906893004 Cr-Commit-Position: refs/heads/master@{#476001} Committed: https://chromium.googlesource.com/chromium/src/+/81081694021693dc29c3ff9bde3d... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/81081694021693dc29c3ff9bde3d... |