Chromium Code Reviews
Help | Chromium Project | Sign in
(489)

Issue 12985002: Add a new App launcher promo to the apps page / NTP. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year ago by MAD
Modified:
1 year ago
CC:
chromium-reviews_chromium.org, dbeam+watch-ntp_chromium.org, tfarina, arv+watch_chromium.org, chromium-apps-reviews_chromium.org, estade+watch_chromium.org, pedrosimonetti+watch_chromium.org, benwells, rsesek
Visibility:
Public.

Description

Add a new App launcher promo to the apps page / NTP.

Simple change in chrome/browser/chrome_browser_main.cc
TBR=ben@chromium.org
BUG=180475
TEST=Make sure the apps promo only show on apps page of the NTP (or chrome://apps) and that it links to webstore (specific page to be added later on the webstore) and that it doesn't come back on a given installation of Chrome once dismissed with the X close button. That dimiss state can be reset with the command line switch --reset-show-apps-promo to help test it. Also, this shouldn't show when the App Launcher is installer... since no apps page should be shown anyway (tested with --show-app-list-shortcut)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190772

Patch Set 1 : #

Total comments: 22

Patch Set 2 : CR Comments 1. #

Patch Set 3 : Sync'd to ToT. #

Patch Set 4 : Testing command line switch only available in Debug builds. #

Patch Set 5 : Now using a Field Trial. #

Patch Set 6 : Some minor nits... #

Patch Set 7 : Fixed unit tests. #

Total comments: 4

Patch Set 8 : FieldTrial CR Comments 1 + one more test fix. #

Total comments: 29

Patch Set 9 : Sync'd to ToT. #

Patch Set 10 : OWNERS review 2. #

Total comments: 14

Patch Set 11 : OWNERS review, round 3. #

Total comments: 3

Patch Set 12 : OWNERS review round 4. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+290 lines, -43 lines) Lint Patch
M apps/apps.gypi View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments ? errors Download
A apps/field_trial_names.h View 1 2 3 4 5 6 7 1 chunk +18 lines, -0 lines 0 comments 1 errors Download
A apps/field_trial_names.cc View 1 2 3 4 5 6 7 1 chunk +19 lines, -0 lines 0 comments ? errors Download
M apps/pref_names.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments ? errors Download
M apps/pref_names.cc View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -0 lines 0 comments ? errors Download
M apps/prefs.cc View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -1 line 0 comments ? errors Download
M chrome/app/generated_resources.grd View 1 chunk +4 lines, -0 lines 0 comments ? errors Download
M chrome/browser/chrome_browser_field_trials.h View 1 2 3 4 5 6 7 2 chunks +11 lines, -5 lines 0 comments ? errors Download
M chrome/browser/chrome_browser_field_trials.cc View 1 2 3 4 5 6 7 4 chunks +20 lines, -4 lines 0 comments 0 errors Download
M chrome/browser/chrome_browser_main.cc View 1 2 3 4 1 chunk +2 lines, -4 lines 0 comments ? errors Download
A chrome/browser/resources/ntp4/app_launcher_promo.png View 0 chunks +-1 lines, --1 lines 0 comments ? errors Download
M chrome/browser/resources/ntp4/apps_page.css View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +62 lines, -0 lines 0 comments ? errors Download
M chrome/browser/resources/ntp4/new_tab.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +11 lines, -0 lines 0 comments ? errors Download
M chrome/browser/resources/ntp4/new_tab.js View 3 chunks +12 lines, -1 line 0 comments ? errors Download
M chrome/browser/resources/ntp4/page_list_view.js View 1 5 chunks +49 lines, -15 lines 0 comments ? errors Download
M chrome/browser/ui/webui/ntp/app_launcher_handler.h View 1 2 3 4 5 6 7 8 3 chunks +10 lines, -2 lines 0 comments ? errors Download
M chrome/browser/ui/webui/ntp/app_launcher_handler.cc View 1 2 3 4 5 6 7 8 7 chunks +32 lines, -5 lines 0 comments ? errors Download
M chrome/browser/ui/webui/ntp/ntp_resource_cache.h View 1 2 1 chunk +2 lines, -1 line 0 comments ? errors Download
M chrome/browser/ui/webui/ntp/ntp_resource_cache.cc View 1 2 3 4 5 6 7 5 chunks +28 lines, -6 lines 0 comments 0 errors Download
Commit:

Messages

Total messages: 39
MAD
Can you do an initial CR before I ask dbeam@ for OWNERs approval? Thanks! BYE ...
1 year ago #1
MAD
Adding benwells@ for the apps stuff...
1 year ago #2
Dan Beam
why is this not using the promo infrastructure and just setting some arbitrary data in ...
1 year ago #3
MAD
For the question about using the promo server, I will let benwells@ (and maybe rsesek@?) ...
1 year ago #4
rsesek
On 2013/03/21 16:47:46, MAD wrote: > For the question about using the promo server, I ...
1 year ago #5
gideonwald
On 2013/03/21 17:49:21, rsesek wrote: > On 2013/03/21 16:47:46, MAD wrote: > > For the ...
1 year ago #6
rsesek
On 2013/03/21 17:58:08, gideonwald wrote: > On 2013/03/21 17:49:21, rsesek wrote: > > On 2013/03/21 ...
1 year ago #7
guohui
On 2013/03/21 17:59:19, rsesek wrote: > On 2013/03/21 17:58:08, gideonwald wrote: > > On 2013/03/21 ...
1 year ago #8
Dan Beam
On 2013/03/21 17:58:08, gideonwald wrote: > On 2013/03/21 17:49:21, rsesek wrote: > > On 2013/03/21 ...
1 year ago #9
MAD
I don't mind changing the code to get the strings and URL from the promo ...
1 year ago #10
rsesek
On 2013/03/21 18:41:16, MAD wrote: > I don't mind changing the code to get the ...
1 year ago #11
MAD
I have no experience with this... Do you think this is feasible for M27? I'll ...
1 year ago #12
MAD
Ho! And wouldn't this interfere with a text base app launcher promo? I guess we ...
1 year ago #13
rsesek
The text promo already has a type, so it wouldn't interfere (though they could run ...
1 year ago #14
MAD
Yeah, I'm toying with that right now, using the promo_text for the text shown in ...
1 year ago #15
rsesek
You could just craft the JSON in the right format and use a simple local ...
1 year ago #16
MAD
OK, I added a Field Trial to control the showing of the promo and removed ...
1 year ago #17
benwells
src/apps lgtm Thanks MAD!
1 year ago #18
MAD
Adding ASvitkine@ for the field trial initialization changes. Feel free to look at the other ...
1 year ago #19
MAD
Ping Dan? Can you continue the review? Hui? Do you have any comment? Alexei? Should ...
1 year ago #20
guohui
On 2013/03/22 15:44:39, MAD wrote: > Ping Dan? Can you continue the review? > > ...
1 year ago #21
asvitkine
Looking at it now. On Fri, Mar 22, 2013 at 9:47 AM, <guohui@chromium.org> wrote: > ...
1 year ago #22
Alexei Svitkine
https://codereview.chromium.org/12985002/diff/67005/apps/field_trial_names.cc File apps/field_trial_names.cc (right): https://codereview.chromium.org/12985002/diff/67005/apps/field_trial_names.cc#newcode14 apps/field_trial_names.cc:14: const char kResetShowLauncherPromoPrefGroup[] = "ResetShowPromoPref"; Nit: Change suffix to ...
1 year ago #23
MAD
Thanks! All done, Let me know if it's OK... BYE MAD https://codereview.chromium.org/12985002/diff/67005/apps/field_trial_names.cc File apps/field_trial_names.cc (right): ...
1 year ago #24
Alexei Svitkine
lgtm
1 year ago #25
MAD
Ping Dan? Will you have time for this before branch point? Or should I find ...
1 year ago #26
Dan Beam
estade@/jhawkins@ are out, arv@ might be able to help
1 year ago #27
MAD
Thanks Dan! arv@ will you have time to look at this today? Hoping to get ...
1 year ago #28
arv
https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/ntp4/apps_page.css File chrome/browser/resources/ntp4/apps_page.css (right): https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/ntp4/apps_page.css#newcode126 chrome/browser/resources/ntp4/apps_page.css:126: font: 16px normal Arial, Helvetica, sans-serif; Why does the ...
1 year ago #29
MAD
Thanks a lot... How about this? BYE MAD https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/ntp4/apps_page.css File chrome/browser/resources/ntp4/apps_page.css (right): https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/ntp4/apps_page.css#newcode126 chrome/browser/resources/ntp4/apps_page.css:126: font: ...
1 year ago #30
arv
https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/ntp4/apps_page.css File chrome/browser/resources/ntp4/apps_page.css (right): https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/ntp4/apps_page.css#newcode126 chrome/browser/resources/ntp4/apps_page.css:126: font: 16px normal Arial, Helvetica, sans-serif; On 2013/03/25 15:32:24, ...
1 year ago #31
MAD
OK, how about this now? Some of your suggestions didn't work and I explained why ...
1 year ago #32
arv
https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/ntp4/new_tab.html File chrome/browser/resources/ntp4/new_tab.html (right): https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/ntp4/new_tab.html#newcode96 chrome/browser/resources/ntp4/new_tab.html:96: <div class="app-launcher-promo" id="app-launcher-promo" hidden> On 2013/03/26 15:42:43, MAD wrote: ...
1 year ago #33
MAD
Answered/Done... Anything else? Thanks again! BYE MAD https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/ntp4/new_tab.html File chrome/browser/resources/ntp4/new_tab.html (right): https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/ntp4/new_tab.html#newcode96 chrome/browser/resources/ntp4/new_tab.html:96: <div class="app-launcher-promo" ...
1 year ago #34
arv
LGTM
1 year ago #35
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/12985002/91021
1 year ago #36
I haz the power (commit-bot)
Presubmit check for 12985002-91021 failed and returned exit status 1. INFO:root:Found 19 file(s). Running presubmit ...
1 year ago #37
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/12985002/91021
1 year ago #38
I haz the power (commit-bot)
1 year ago #39
Message was sent while issue was closed.
Change committed as 190772
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6