Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2949733002:
Show Play Store rating and price in app list (Closed)
DescriptionReland: Show Play Store rating and price in app list
1. Provide interfaces in app_list::SearchResult to set price and rating
for instant and play store apps based on specs.
2. Change the layouts of SearchResultTileItemListView and
SearchResultTileItemView to show price and rating properly based on
specs.
Specifications:
https://screenshot.googleplex.com/AFQ5HnrMYch
https://screenshot.googleplex.com/0opcNunaJrZ
The screenshot: https://screenshot.googleplex.com/Kv0xzOVZLqs
BUG=734841
Review-Url: https://codereview.chromium.org/2949733002
Cr-Commit-Position: refs/heads/master@{#481660}
Committed: https://chromium.googlesource.com/chromium/src/+/d8c99c46b5cb4ad91fc69229127369bf6d0403ae
Patch Set 1 #
Total comments: 20
Patch Set 2 #Patch Set 3 : Rebase #
Total comments: 14
Patch Set 4 : Apply fix to patch set 3 #
Total comments: 2
Patch Set 5 : Apply fix to patch set 4. #
Total comments: 3
Patch Set 6 : Apply fix to patch set 5 #
Messages
Total messages: 44 (22 generated)
The CQ bit was checked by weidongg@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...
weidongg@chromium.org changed reviewers: + hejq@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Show Play Store rating and price in app list 1. Provide interfaces in app_list::SearchResult to set price and rating for instant and play store apps based on specs. 2. Change the layouts of SearchResultTileItemListView and SearchResultTileItemView to show price and rating properly based on specs. Specifications: https://screenshot.googleplex.com/AFQ5HnrMYch https://screenshot.googleplex.com/0opcNunaJrZ The screenshot: https://screenshot.googleplex.com/Kv0xzOVZLqs BUG=734841 ========== to ========== Show Play Store rating and price in app list 1. Provide interfaces in app_list::SearchResult to set price and rating for instant and play store apps based on specs. 2. Change the layouts of SearchResultTileItemListView and SearchResultTileItemView to show price and rating properly based on specs. Specifications: https://screenshot.googleplex.com/AFQ5HnrMYch https://screenshot.googleplex.com/0opcNunaJrZ The screenshot: https://screenshot.googleplex.com/Kv0xzOVZLqs BUG=734841 ==========
weidongg@chromium.org changed reviewers: + xiyuan@chromium.org
xiyuan@chromium.org: Please review changes.
https://codereview.chromium.org/2949733002/diff/1/ui/app_list/app_list_consta... File ui/app_list/app_list_constants.h (right): https://codereview.chromium.org/2949733002/diff/1/ui/app_list/app_list_consta... ui/app_list/app_list_constants.h:54: APP_LIST_EXPORT extern const SkColor kAppRatingColor; The const should stay with the file where they are used. Put them in this file only when the const needs to be access from multiple files. https://codereview.chromium.org/2949733002/diff/1/ui/app_list/search_result.h File ui/app_list/search_result.h (right): https://codereview.chromium.org/2949733002/diff/1/ui/app_list/search_result.h... ui/app_list/search_result.h:118: void SetPrice(double price); Is there a way to clear price and rating info? That is, how do we plan to support the case where price and rating is not available? https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/search_re... File ui/app_list/views/search_result_tile_item_view.cc (right): https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/search_re... ui/app_list/views/search_result_tile_item_view.cc:52: // Todo(weidongg) add condition: instant or play store app. nit: TODO style is wrong. // TODO(weidongg): Only show for instant or play store app. https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/search_re... ui/app_list/views/search_result_tile_item_view.cc:53: nit: strip extra empty lines here and Line 55. https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/tile_item... File ui/app_list/views/tile_item_view.cc (right): https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/tile_item... ui/app_list/views/tile_item_view.cc:90: price_->SetFontList(gfx::FontList(kAppPriceFontList)); Roboto is the current default UI font. Let's try not create UI font like this. Otherwise, when UI spec changes (which happened in the past), we would have to revisit and change the font family. Instead, create FontList like this: ui::ResourceBundle::GetSharedInstance().GetFontList(ui::ResourceBundle::BaseFont) and do DeriveWithSizeDelta(delta) if the font size does not match spec. https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/tile_item... ui/app_list/views/tile_item_view.cc:264: else Don't use else after return. See https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/tile_item... File ui/app_list/views/tile_item_view.h (right): https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/tile_item... ui/app_list/views/tile_item_view.h:67: nit: strip empty lines and put the new method in the same block of the setters in line 62-64. https://codereview.chromium.org/2949733002/diff/1/ui/strings/ui_strings.grd File ui/strings/ui_strings.grd (right): https://codereview.chromium.org/2949733002/diff/1/ui/strings/ui_strings.grd#n... ui/strings/ui_strings.grd:700: $<ph name="price">{0}<ex>19.98</ex></ph> Currency is tricky. Using "$" directly here is not i18n friendly. Not sure what is the best way to get this. We have CurrencyFormatter [1] that can do this. But I am not sure whether it is okay to have deps on it but you can see how to use ICU to format currency. [1]: https://cs.chromium.org/chromium/src/components/payments/core/currency_format... https://codereview.chromium.org/2949733002/diff/1/ui/strings/ui_strings.grd#n... ui/strings/ui_strings.grd:703: <ph name="rating">{0}<ex>4.5</ex></ph>★ Get rid of "*". Think it should be displayed via an image resource.
warx@chromium.org changed reviewers: + warx@chromium.org
https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/tile_item... File ui/app_list/views/tile_item_view.cc (right): https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/tile_item... ui/app_list/views/tile_item_view.cc:189: void TileItemView::Layout() { I think we would better not do Layout here. TileItemView is no longer a common Layout() method for both start page and search box searched results. My idea is to leave the Layout() not changed for TileItemView for old behavior. Instead, we implement it in SearchResultTileItemView based on the display_type, whether it is SearchResult::DISPLAY_RECOMMENDATION or DISPLAY_TITLE (in your case). I have a CL about this prototype. Let us see how it works.
I am confirming with the UX designer whether the star should be text or image. I will upload another patch set to add the star. https://codereview.chromium.org/2949733002/diff/1/ui/app_list/app_list_consta... File ui/app_list/app_list_constants.h (right): https://codereview.chromium.org/2949733002/diff/1/ui/app_list/app_list_consta... ui/app_list/app_list_constants.h:54: APP_LIST_EXPORT extern const SkColor kAppRatingColor; On 2017/06/20 22:19:22, xiyuan wrote: > The const should stay with the file where they are used. Put them in this file > only when the const needs to be access from multiple files. Done. https://codereview.chromium.org/2949733002/diff/1/ui/app_list/search_result.h File ui/app_list/search_result.h (right): https://codereview.chromium.org/2949733002/diff/1/ui/app_list/search_result.h... ui/app_list/search_result.h:118: void SetPrice(double price); On 2017/06/20 22:19:22, xiyuan wrote: > Is there a way to clear price and rating info? That is, how do we plan to > support the case where price and rating is not available? Yes, Done. https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/search_re... File ui/app_list/views/search_result_tile_item_view.cc (right): https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/search_re... ui/app_list/views/search_result_tile_item_view.cc:52: // Todo(weidongg) add condition: instant or play store app. On 2017/06/20 22:19:22, xiyuan wrote: > nit: TODO style is wrong. > > // TODO(weidongg): Only show for instant or play store app. Done. https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/search_re... ui/app_list/views/search_result_tile_item_view.cc:53: On 2017/06/20 22:19:22, xiyuan wrote: > nit: strip extra empty lines here and Line 55. Done. https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/tile_item... File ui/app_list/views/tile_item_view.cc (right): https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/tile_item... ui/app_list/views/tile_item_view.cc:90: price_->SetFontList(gfx::FontList(kAppPriceFontList)); On 2017/06/20 22:19:23, xiyuan wrote: > Roboto is the current default UI font. Let's try not create UI font like this. > Otherwise, when UI spec changes (which happened in the past), we would have to > revisit and change the font family. > > Instead, create FontList like this: > > ui::ResourceBundle::GetSharedInstance().GetFontList(ui::ResourceBundle::BaseFont) > > and do DeriveWithSizeDelta(delta) if the font size does not match spec. Good to know, thanks. Done. https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/tile_item... ui/app_list/views/tile_item_view.cc:189: void TileItemView::Layout() { On 2017/06/21 17:43:12, Qiang(Joe) Xu wrote: > I think we would better not do Layout here. TileItemView is no longer a common > Layout() method for both start page and search box searched results. > > My idea is to leave the Layout() not changed for TileItemView for old behavior. > Instead, we implement it in SearchResultTileItemView based on the display_type, > whether it is SearchResult::DISPLAY_RECOMMENDATION or DISPLAY_TITLE (in your > case). > > I have a CL about this prototype. Let us see how it works. Yes, agree. Rebased to your change. https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/tile_item... ui/app_list/views/tile_item_view.cc:264: else On 2017/06/20 22:19:23, xiyuan wrote: > Don't use else after return. > > See > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > Done. https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/tile_item... File ui/app_list/views/tile_item_view.h (right): https://codereview.chromium.org/2949733002/diff/1/ui/app_list/views/tile_item... ui/app_list/views/tile_item_view.h:67: On 2017/06/20 22:19:23, xiyuan wrote: > nit: strip empty lines and put the new method in the same block of the setters > in line 62-64. Done. https://codereview.chromium.org/2949733002/diff/1/ui/strings/ui_strings.grd File ui/strings/ui_strings.grd (right): https://codereview.chromium.org/2949733002/diff/1/ui/strings/ui_strings.grd#n... ui/strings/ui_strings.grd:700: $<ph name="price">{0}<ex>19.98</ex></ph> On 2017/06/20 22:19:23, xiyuan wrote: > Currency is tricky. Using "$" directly here is not i18n friendly. > > Not sure what is the best way to get this. We have CurrencyFormatter [1] that > can do this. But I am not sure whether it is okay to have deps on it but you can > see how to use ICU to format currency. > > [1]: > https://cs.chromium.org/chromium/src/components/payments/core/currency_format... I confirmed with the @hejq. The price should be formated string with currency symbol. It seems that I don't need to worry about this. Thanks. https://codereview.chromium.org/2949733002/diff/1/ui/strings/ui_strings.grd#n... ui/strings/ui_strings.grd:703: <ph name="rating">{0}<ex>4.5</ex></ph>★ On 2017/06/20 22:19:23, xiyuan wrote: > Get rid of "*". Think it should be displayed via an image resource. I am confirming with UX designer about this. In the specs, the star seems to have font which seems to indicate a text, but I am not sure.
The CQ bit was checked by weidongg@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...
Mostly nits https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/search_resu... File ui/app_list/search_result.h (right): https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/search_resu... ui/app_list/search_result.h:208: float rating_ = -1.0; nit: -1.0f, to make it clear to compiler this is a float. Otherwise, this could produce a compiler warning on some platforms (e.g. win). https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/search_resu... ui/app_list/search_result.h:210: // Price of the app in play store. Not exist if set to empty. nit: Price -> Price label, to make it clear that it is a label text for the price. Another optinal nit is to change the name, price_ -> price_label_, and relevant code to make this clear to future readers of the code. https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_view.cc (right): https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_view.cc:54: gfx::FontList base_font = nit: gfx::FontList -> const gfx::FontList&, to save a copy https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_view.cc:104: gfx::FontList base_font = ui::ResourceBundle::GetSharedInstance().GetFontList( nit: ditto, const gfx::FontList& and move it inside "if (is_fullscreen_app_list_enabled_)" since it is only used there. https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_view.cc:255: price_rect.Inset(0, title()->GetPreferredSize().height(), 0, 0); Would this overlap with |reating_rect|? https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_view.h (right): https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_view.h:70: views::Label* price_; // Owned by views hierarchy. nit: forward declare views::Label. Slightly prefer to initialize members in header. Would you mind move them here (these two and |item_|)?
https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/search_resu... File ui/app_list/search_result.h (right): https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/search_resu... ui/app_list/search_result.h:208: float rating_ = -1.0; On 2017/06/22 17:07:34, xiyuan wrote: > nit: -1.0f, to make it clear to compiler this is a float. Otherwise, this could > produce a compiler warning on some platforms (e.g. win). Good to know, thanks. Done. https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/search_resu... ui/app_list/search_result.h:210: // Price of the app in play store. Not exist if set to empty. On 2017/06/22 17:07:34, xiyuan wrote: > nit: Price -> Price label, to make it clear that it is a label text for the > price. > > Another optinal nit is to change the name, price_ -> price_label_, and relevant > code to make this clear to future readers of the code. Yes, if we are to change the name, formatted_price_ may be better, as hejq@ is using this name for price get from play store. Done. https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_view.cc (right): https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_view.cc:54: gfx::FontList base_font = On 2017/06/22 17:07:34, xiyuan wrote: > nit: gfx::FontList -> const gfx::FontList&, to save a copy Done. https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_view.cc:104: gfx::FontList base_font = ui::ResourceBundle::GetSharedInstance().GetFontList( On 2017/06/22 17:07:34, xiyuan wrote: > nit: ditto, const gfx::FontList& > > and move it inside "if (is_fullscreen_app_list_enabled_)" since it is only used > there. Done. https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_view.cc:255: price_rect.Inset(0, title()->GetPreferredSize().height(), 0, 0); On 2017/06/22 17:07:34, xiyuan wrote: > Would this overlap with |reating_rect|? Yes, it is overlapping with rating_rect. I want to find a way to make rating align left and price align right. But if I set the width of price to preferred width, then the price ends up aligning left. It seems the "price_->SetHorizontalAlignment(gfx::ALIGN_RIGHT)" only apply to the alignment of text inside the label. https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_view.h (right): https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_view.h:70: views::Label* price_; // Owned by views hierarchy. On 2017/06/22 17:07:34, xiyuan wrote: > nit: forward declare views::Label. > > Slightly prefer to initialize members in header. Would you mind move them here > (these two and |item_|)? Done.
lgtm https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/search_resu... File ui/app_list/search_result.h (right): https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/search_resu... ui/app_list/search_result.h:210: // Price of the app in play store. Not exist if set to empty. On 2017/06/22 17:58:29, weidongg wrote: > On 2017/06/22 17:07:34, xiyuan wrote: > > nit: Price -> Price label, to make it clear that it is a label text for the > > price. > > > > Another optinal nit is to change the name, price_ -> price_label_, and > relevant > > code to make this clear to future readers of the code. > > Yes, if we are to change the name, formatted_price_ may be better, as hejq@ is > using this name for price get from play store. Done. formatted_price_ sounds good. https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_view.cc (right): https://codereview.chromium.org/2949733002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_view.cc:255: price_rect.Inset(0, title()->GetPreferredSize().height(), 0, 0); On 2017/06/22 17:58:30, weidongg wrote: > On 2017/06/22 17:07:34, xiyuan wrote: > > Would this overlap with |reating_rect|? > > Yes, it is overlapping with rating_rect. I want to find a way to make rating > align left and price align right. But if I set the width of price to preferred > width, then the price ends up aligning left. It seems the > "price_->SetHorizontalAlignment(gfx::ALIGN_RIGHT)" only apply to the alignment > of text inside the label. I see. I would recommend creating a container view and use a horizontal BoxLayout with a padding view in middle for that. We can revisit this in the future if those two needs to interact with mouse/touch. https://codereview.chromium.org/2949733002/diff/60001/ui/app_list/search_resu... File ui/app_list/search_result_observer.h (right): https://codereview.chromium.org/2949733002/diff/60001/ui/app_list/search_resu... ui/app_list/search_result_observer.h:24: virtual void OnPriceChanged() {} OnPriceChanged -> OnFormattedPriceChanged ?
Thanks for the review. For the star in rating, I am gonna upload another CL since UX has not replied me and I do not have asset images for the star. https://codereview.chromium.org/2949733002/diff/60001/ui/app_list/search_resu... File ui/app_list/search_result_observer.h (right): https://codereview.chromium.org/2949733002/diff/60001/ui/app_list/search_resu... ui/app_list/search_result_observer.h:24: virtual void OnPriceChanged() {} On 2017/06/22 18:17:21, xiyuan wrote: > OnPriceChanged -> OnFormattedPriceChanged ? Done.
The CQ bit was checked by weidongg@chromium.org
The CQ bit was unchecked by weidongg@chromium.org
The CQ bit was checked by weidongg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2949733002/#ps80001 (title: "Apply fix to patch set 4.")
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": 80001, "attempt_start_ts": 1498156553002020,
"parent_rev": "be4e46518a93692f402a69363bcaab66ae11b1e3", "commit_rev":
"63ddedc4cd3f0a35990b14b618b5415d1a9baeba"}
Message was sent while issue was closed.
Description was changed from ========== Show Play Store rating and price in app list 1. Provide interfaces in app_list::SearchResult to set price and rating for instant and play store apps based on specs. 2. Change the layouts of SearchResultTileItemListView and SearchResultTileItemView to show price and rating properly based on specs. Specifications: https://screenshot.googleplex.com/AFQ5HnrMYch https://screenshot.googleplex.com/0opcNunaJrZ The screenshot: https://screenshot.googleplex.com/Kv0xzOVZLqs BUG=734841 ========== to ========== Show Play Store rating and price in app list 1. Provide interfaces in app_list::SearchResult to set price and rating for instant and play store apps based on specs. 2. Change the layouts of SearchResultTileItemListView and SearchResultTileItemView to show price and rating properly based on specs. Specifications: https://screenshot.googleplex.com/AFQ5HnrMYch https://screenshot.googleplex.com/0opcNunaJrZ The screenshot: https://screenshot.googleplex.com/Kv0xzOVZLqs BUG=734841 Review-Url: https://codereview.chromium.org/2949733002 Cr-Commit-Position: refs/heads/master@{#481618} Committed: https://chromium.googlesource.com/chromium/src/+/63ddedc4cd3f0a35990b14b618b5... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/63ddedc4cd3f0a35990b14b618b5...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2947313003/ by scottmg@chromium.org. The reason for reverting is: dbg cros failed to compile, unfortunately i guess that's not on the cq :/ log here: https://luci-milo.appspot.com/buildbot/chromium.chromiumos/Linux%20ChromiumOS....
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) confirmed this CL at revision 481618 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2949353002/ by fsamuel@google.com. The reason for reverting is: Findit reports this is the cause for this: https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20C....
Message was sent while issue was closed.
https://codereview.chromium.org/2949733002/diff/80001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_view.cc (right): https://codereview.chromium.org/2949733002/diff/80001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_view.cc:34: constexpr SkColor kSearchTitleColor = SkColorSetA(SK_ColorBLACK, 223); Missed this, we should use SkColorSetARGBMacro instead.
Message was sent while issue was closed.
halcanary@google.com changed reviewers: + halcanary@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/2949733002/diff/80001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_view.cc (right): https://codereview.chromium.org/2949733002/diff/80001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_view.cc:34: constexpr SkColor kSearchTitleColor = SkColorSetA(SK_ColorBLACK, 223); On 2017/06/22 19:55:05, xiyuan wrote: > Missed this, we should use SkColorSetARGBMacro instead. I just came here to say this.
Message was sent while issue was closed.
Message was sent while issue was closed.
Thanks for the review. Should I commit in this code review thread again? https://codereview.chromium.org/2949733002/diff/80001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_view.cc (right): https://codereview.chromium.org/2949733002/diff/80001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_view.cc:34: constexpr SkColor kSearchTitleColor = SkColorSetA(SK_ColorBLACK, 223); On 2017/06/22 19:55:05, xiyuan wrote: > Missed this, we should use SkColorSetARGBMacro instead. Done.
Message was sent while issue was closed.
On 2017/06/22 20:22:42, weidongg wrote: > Thanks for the review. Should I commit in this code review thread again? Yep. Re-open and send it to CQ again.
Message was sent while issue was closed.
On 2017/06/22 20:25:30, xiyuan wrote:
> On 2017/06/22 20:22:42, weidongg wrote:
> > Thanks for the review. Should I commit in this code review thread again?
>
> Yep. Re-open and send it to CQ again.
Oh, you might also want to adjust the commit message:
- Prepend "Reland: " to subject
- And clean up
Cr-Commit-Position: refs/heads/master@{#481618}
Committed:
https://chromium.googlesource.com/chromium/src/+/63ddedc4cd3f0a35990b14b618b5...
etc.
Message was sent while issue was closed.
Description was changed from ========== Show Play Store rating and price in app list 1. Provide interfaces in app_list::SearchResult to set price and rating for instant and play store apps based on specs. 2. Change the layouts of SearchResultTileItemListView and SearchResultTileItemView to show price and rating properly based on specs. Specifications: https://screenshot.googleplex.com/AFQ5HnrMYch https://screenshot.googleplex.com/0opcNunaJrZ The screenshot: https://screenshot.googleplex.com/Kv0xzOVZLqs BUG=734841 Review-Url: https://codereview.chromium.org/2949733002 Cr-Commit-Position: refs/heads/master@{#481618} Committed: https://chromium.googlesource.com/chromium/src/+/63ddedc4cd3f0a35990b14b618b5... ========== to ========== Reland: Show Play Store rating and price in app list 1. Provide interfaces in app_list::SearchResult to set price and rating for instant and play store apps based on specs. 2. Change the layouts of SearchResultTileItemListView and SearchResultTileItemView to show price and rating properly based on specs. Specifications: https://screenshot.googleplex.com/AFQ5HnrMYch https://screenshot.googleplex.com/0opcNunaJrZ The screenshot: https://screenshot.googleplex.com/Kv0xzOVZLqs BUG=734841 ==========
The CQ bit was checked by weidongg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2949733002/#ps100001 (title: "Apply fix to patch set 5")
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": 1498163376111230,
"parent_rev": "8290f1360214501d2dfd2734647c57576a1c49ef", "commit_rev":
"d8c99c46b5cb4ad91fc69229127369bf6d0403ae"}
Message was sent while issue was closed.
Description was changed from ========== Reland: Show Play Store rating and price in app list 1. Provide interfaces in app_list::SearchResult to set price and rating for instant and play store apps based on specs. 2. Change the layouts of SearchResultTileItemListView and SearchResultTileItemView to show price and rating properly based on specs. Specifications: https://screenshot.googleplex.com/AFQ5HnrMYch https://screenshot.googleplex.com/0opcNunaJrZ The screenshot: https://screenshot.googleplex.com/Kv0xzOVZLqs BUG=734841 ========== to ========== Reland: Show Play Store rating and price in app list 1. Provide interfaces in app_list::SearchResult to set price and rating for instant and play store apps based on specs. 2. Change the layouts of SearchResultTileItemListView and SearchResultTileItemView to show price and rating properly based on specs. Specifications: https://screenshot.googleplex.com/AFQ5HnrMYch https://screenshot.googleplex.com/0opcNunaJrZ The screenshot: https://screenshot.googleplex.com/Kv0xzOVZLqs BUG=734841 Review-Url: https://codereview.chromium.org/2949733002 Cr-Commit-Position: refs/heads/master@{#481660} Committed: https://chromium.googlesource.com/chromium/src/+/d8c99c46b5cb4ad91fc692291273... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/d8c99c46b5cb4ad91fc692291273... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
