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

Issue 11044020: Make Web Intents picker in Views conform to latest mocks (Closed)

Created:
8 years, 2 months ago by please use gerrit instead
Modified:
8 years, 2 months ago
CC:
chromium-reviews, groby+watch_chromium.org, tfarina, rdsmith+dwatch_chromium.org, gbillock+watch_chromium.org, smckay+watch_chromium.org, rouslan+watch_chromium.org, Mike Wittman
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Make Web Intents picker in Views conform to latest mocks BUG=148615 TBR=jhawkins Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162287

Patch Set 1 #

Patch Set 2 : Comment typo fix. #

Patch Set 3 : Merge master. #

Patch Set 4 : Fix compiler error 'only static const integral data members can be initialized within a class' #

Patch Set 5 : Updated strings from Glen. #

Patch Set 6 : Fixing unresolved external symbol error in win_aura build. #

Patch Set 7 : Using new constrained window #

Patch Set 8 : Include changes from CL 11077006 #

Total comments: 32

Patch Set 9 : Adjust 'uh-oh' view padding #

Patch Set 10 : Do not use defined(OS_MACOSX) for a global const #

Patch Set 11 : Fix mac build #

Patch Set 12 : Fix mac build part 2 #

Patch Set 13 : Fix windows dcheck for 0x0 window #

Patch Set 14 : Addressed comments #

Patch Set 15 : Cleanup #

Patch Set 16 : Cleanup #

Patch Set 17 : Remove resizing jank #

Patch Set 18 : Better state handling #

Patch Set 19 : Show use another service link when data becomes available #

Patch Set 20 : Hide choose another service link when model says so #

Patch Set 21 : Remove suggestion and update icon on install #

Patch Set 22 : Padding #

Patch Set 23 : Move link underline into a separate CL #

Total comments: 83

Patch Set 24 : Moved GetDefaultInsets() into ConstrainedWindowViews #

Patch Set 25 : Remove unused include #

Patch Set 26 : Address comments #

Total comments: 14

Patch Set 27 : Address comments #

Total comments: 21

Patch Set 28 : Address comments #

Patch Set 29 : Add a .cc file for constants to be able to compile in debug mode #

Unified diffs Side-by-side diffs Delta from patch set Stats (+782 lines, -611 lines) Patch
M chrome/app/generated_resources.grd 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 3 chunks +7 lines, -7 lines 0 comments Download
M chrome/browser/ui/constrained_window_constants.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 2 chunks +14 lines, -13 lines 0 comments Download
A chrome/browser/ui/constrained_window_constants.cc 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 1 chunk +22 lines, -0 lines 0 comments Download
M chrome/browser/ui/intents/web_intent_picker.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/intents/web_intent_picker_controller.cc 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 1 chunk +2 lines, -3 lines 0 comments Download
M chrome/browser/ui/intents/web_intent_picker_model.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/ui/intents/web_intent_picker_model.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +13 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/collected_cookies_views.cc 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 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc 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 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/views/constrained_window_frame_simple.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 3 chunks +6 lines, -27 lines 0 comments Download
M chrome/browser/ui/views/constrained_window_frame_simple.cc 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 2 chunks +78 lines, -148 lines 0 comments Download
M chrome/browser/ui/views/constrained_window_views.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 4 chunks +29 lines, -7 lines 0 comments Download
M chrome/browser/ui/views/constrained_window_views.cc 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 12 chunks +52 lines, -66 lines 0 comments Download
M chrome/browser/ui/views/constrained_window_views_browsertest.cc 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 3 chunks +9 lines, -6 lines 0 comments Download
M chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc 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 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/views/login_prompt_views.cc 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 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/ui/views/ssl_client_certificate_selector.cc 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 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/tab_modal_confirm_dialog_views.cc 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 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/web_intent_picker_views.cc 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 chunks +529 lines, -322 lines 0 comments Download
M chrome/chrome_browser_ui.gypi 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 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 39 (0 generated)
please use gerrit instead
------------------------------------------ pkasting@: Please review the custom insets in constrained window and UI changes to WebIntentPickerViews. ...
8 years, 2 months ago (2012-10-11 21:08:51 UTC) #1
rouslan
Adding cpu@ and pkasting@ to the list of recipients. On Thu, Oct 11, 2012 at ...
8 years, 2 months ago (2012-10-11 21:12:20 UTC) #2
cpu_(ooo_6.6-7.5)
grd changes ok.
8 years, 2 months ago (2012-10-11 21:37:53 UTC) #3
sail
http://codereview.chromium.org/11044020/diff/30001/chrome/browser/ui/constrained_window.h File chrome/browser/ui/constrained_window.h (right): http://codereview.chromium.org/11044020/diff/30001/chrome/browser/ui/constrained_window.h#newcode28 chrome/browser/ui/constrained_window.h:28: #if defined(OS_MACOSX) we shouldn't use #ifdefs unless we have ...
8 years, 2 months ago (2012-10-11 21:40:28 UTC) #4
please use gerrit instead
http://codereview.chromium.org/11044020/diff/30001/chrome/browser/ui/constrained_window.h File chrome/browser/ui/constrained_window.h (right): http://codereview.chromium.org/11044020/diff/30001/chrome/browser/ui/constrained_window.h#newcode28 chrome/browser/ui/constrained_window.h:28: #if defined(OS_MACOSX) On 2012/10/11 21:40:28, sail wrote: > we ...
8 years, 2 months ago (2012-10-11 22:01:54 UTC) #5
sail
cocoa/* LGTM
8 years, 2 months ago (2012-10-11 22:02:54 UTC) #6
Peter Kasting
https://codereview.chromium.org/11044020/diff/30001/chrome/browser/ui/views/constrained_window_views.h File chrome/browser/ui/views/constrained_window_views.h (right): https://codereview.chromium.org/11044020/diff/30001/chrome/browser/ui/views/constrained_window_views.h#newcode64 chrome/browser/ui/views/constrained_window_views.h:64: ConstrainedWindowViews(content::WebContents* web_contents, Seems like we can avoid having two ...
8 years, 2 months ago (2012-10-12 03:23:32 UTC) #7
please use gerrit instead
PTLA http://codereview.chromium.org/11044020/diff/30001/chrome/browser/ui/views/constrained_window_views.h File chrome/browser/ui/views/constrained_window_views.h (right): http://codereview.chromium.org/11044020/diff/30001/chrome/browser/ui/views/constrained_window_views.h#newcode64 chrome/browser/ui/views/constrained_window_views.h:64: ConstrainedWindowViews(content::WebContents* web_contents, On 2012/10/12 03:23:32, Peter Kasting wrote: ...
8 years, 2 months ago (2012-10-14 18:40:45 UTC) #8
Steve McKay
This CL looks pretty wide ranging. Consider breaking ui/views/controls/link.{cc,h} into a separate CL.
8 years, 2 months ago (2012-10-15 18:00:39 UTC) #9
Peter Kasting
http://codereview.chromium.org/11044020/diff/30001/chrome/browser/ui/views/web_intent_picker_views.cc File chrome/browser/ui/views/web_intent_picker_views.cc (right): http://codereview.chromium.org/11044020/diff/30001/chrome/browser/ui/views/web_intent_picker_views.cc#newcode302 chrome/browser/ui/views/web_intent_picker_views.cc:302: class SpinnerProgressIndicator : public views::View { On 2012/10/14 18:40:45, ...
8 years, 2 months ago (2012-10-15 18:24:23 UTC) #10
Peter Kasting
Also, in the first comment on this CL you asked for people to ignore a ...
8 years, 2 months ago (2012-10-15 19:06:48 UTC) #11
please use gerrit instead
PTAL ---------------------------------------------- pkasting@: I've addressed your comments. Files from the other CL have now disappeared. ...
8 years, 2 months ago (2012-10-15 20:24:04 UTC) #12
please use gerrit instead
ben@: I've moved the changes to link.h and link.cc into a separate CL: http://codereview.chromium.org/11143024/
8 years, 2 months ago (2012-10-15 20:27:04 UTC) #13
Greg Billock
On 2012/10/15 20:27:04, Rouslan Solomakhin wrote: > ben@: I've moved the changes to link.h and ...
8 years, 2 months ago (2012-10-15 20:59:10 UTC) #14
please use gerrit instead
On 2012/10/15 20:59:10, Greg Billock wrote: > lgtm for intents stuff. I see sail sent ...
8 years, 2 months ago (2012-10-15 21:02:58 UTC) #15
please use gerrit instead
sail@: I've moved GetDefaultInsets() into ConstrainedWindowViews and undone the change to ConstrainedWindowConstants::kClientTopPadding. PTAL.
8 years, 2 months ago (2012-10-15 21:46:00 UTC) #16
Peter Kasting
http://codereview.chromium.org/11044020/diff/57027/chrome/browser/ui/views/constrained_window_frame_simple.cc File chrome/browser/ui/views/constrained_window_frame_simple.cc (right): http://codereview.chromium.org/11044020/diff/57027/chrome/browser/ui/views/constrained_window_frame_simple.cc#newcode34 chrome/browser/ui/views/constrained_window_frame_simple.cc:34: ConstrainedWindowConstants::kCloseButtonPadding; Nit: Add a comment about why we subtract ...
8 years, 2 months ago (2012-10-15 21:46:54 UTC) #17
sail
LGTM!
8 years, 2 months ago (2012-10-15 21:53:46 UTC) #18
please use gerrit instead
pkasting@: Thank you for your review. I have addressed your comments. PTAL. http://codereview.chromium.org/11044020/diff/57027/chrome/browser/ui/views/constrained_window_frame_simple.cc File chrome/browser/ui/views/constrained_window_frame_simple.cc ...
8 years, 2 months ago (2012-10-16 00:12:23 UTC) #19
please use gerrit instead
Disambiguated a sentence of mine. http://codereview.chromium.org/11044020/diff/57027/chrome/browser/ui/views/constrained_window_views.h File chrome/browser/ui/views/constrained_window_views.h (right): http://codereview.chromium.org/11044020/diff/57027/chrome/browser/ui/views/constrained_window_views.h#newcode122 chrome/browser/ui/views/constrained_window_views.h:122: // and the close ...
8 years, 2 months ago (2012-10-16 00:15:59 UTC) #20
Peter Kasting
http://codereview.chromium.org/11044020/diff/57027/chrome/browser/ui/views/web_intent_picker_views.cc File chrome/browser/ui/views/web_intent_picker_views.cc (right): http://codereview.chromium.org/11044020/diff/57027/chrome/browser/ui/views/web_intent_picker_views.cc#newcode356 chrome/browser/ui/views/web_intent_picker_views.cc:356: SpinnerProgressIndicator(); On 2012/10/16 00:12:23, Rouslan Solomakhin wrote: > On ...
8 years, 2 months ago (2012-10-16 00:55:06 UTC) #21
please use gerrit instead
pkasting@: I've added a virtual destructor to WaitingView already, but forgot to update my response ...
8 years, 2 months ago (2012-10-16 00:57:19 UTC) #22
cpu_(ooo_6.6-7.5)
grd changes lgtm
8 years, 2 months ago (2012-10-16 01:10:13 UTC) #23
Peter Kasting
This is pretty close. http://codereview.chromium.org/11044020/diff/60072/chrome/browser/ui/views/constrained_window_frame_simple.cc File chrome/browser/ui/views/constrained_window_frame_simple.cc (right): http://codereview.chromium.org/11044020/diff/60072/chrome/browser/ui/views/constrained_window_frame_simple.cc#newcode30 chrome/browser/ui/views/constrained_window_frame_simple.cc:30: using namespace views; This is ...
8 years, 2 months ago (2012-10-16 02:26:53 UTC) #24
please use gerrit instead
pkasting@: I've addressing your comments. PTAL. http://codereview.chromium.org/11044020/diff/60072/chrome/browser/ui/views/constrained_window_frame_simple.cc File chrome/browser/ui/views/constrained_window_frame_simple.cc (right): http://codereview.chromium.org/11044020/diff/60072/chrome/browser/ui/views/constrained_window_frame_simple.cc#newcode30 chrome/browser/ui/views/constrained_window_frame_simple.cc:30: using namespace views; ...
8 years, 2 months ago (2012-10-16 16:29:37 UTC) #25
Peter Kasting
LGTM with one real concern http://codereview.chromium.org/11044020/diff/67001/chrome/browser/ui/views/constrained_window_frame_simple.cc File chrome/browser/ui/views/constrained_window_frame_simple.cc (right): http://codereview.chromium.org/11044020/diff/67001/chrome/browser/ui/views/constrained_window_frame_simple.cc#newcode32 chrome/browser/ui/views/constrained_window_frame_simple.cc:32: using views::View; Nit: These ...
8 years, 2 months ago (2012-10-16 17:28:58 UTC) #26
Ben Goodger (Google)
http://codereview.chromium.org/11044020/diff/67001/chrome/browser/ui/views/constrained_window_frame_simple.cc File chrome/browser/ui/views/constrained_window_frame_simple.cc (right): http://codereview.chromium.org/11044020/diff/67001/chrome/browser/ui/views/constrained_window_frame_simple.cc#newcode32 chrome/browser/ui/views/constrained_window_frame_simple.cc:32: using views::View; On 2012/10/16 17:28:59, Peter Kasting wrote: > ...
8 years, 2 months ago (2012-10-16 18:07:56 UTC) #27
tfarina
On Tue, Oct 16, 2012 at 3:07 PM, <ben@chromium.org> wrote: > > http://codereview.chromium.org/11044020/diff/67001/chrome/browser/ui/views/constrained_window_frame_simple.cc > File ...
8 years, 2 months ago (2012-10-16 18:12:34 UTC) #28
please use gerrit instead
Addressed the comments. Committing. http://codereview.chromium.org/11044020/diff/67001/chrome/browser/ui/views/constrained_window_frame_simple.cc File chrome/browser/ui/views/constrained_window_frame_simple.cc (right): http://codereview.chromium.org/11044020/diff/67001/chrome/browser/ui/views/constrained_window_frame_simple.cc#newcode32 chrome/browser/ui/views/constrained_window_frame_simple.cc:32: using views::View; On 2012/10/16 17:28:59, ...
8 years, 2 months ago (2012-10-16 19:12:36 UTC) #29
please use gerrit instead
On 2012/10/16 18:12:34, tfarina wrote: > Yes, please try to avoid doing "using views::" and ...
8 years, 2 months ago (2012-10-16 19:13:41 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/11044020/87001
8 years, 2 months ago (2012-10-16 19:18:48 UTC) #31
Peter Kasting
http://codereview.chromium.org/11044020/diff/67001/chrome/browser/ui/views/constrained_window_frame_simple.cc File chrome/browser/ui/views/constrained_window_frame_simple.cc (right): http://codereview.chromium.org/11044020/diff/67001/chrome/browser/ui/views/constrained_window_frame_simple.cc#newcode96 chrome/browser/ui/views/constrained_window_frame_simple.cc:96: ConstrainedWindowConstants::kHorizontalPadding, On 2012/10/16 19:12:37, Rouslan Solomakhin wrote: > > ...
8 years, 2 months ago (2012-10-16 19:20:04 UTC) #32
please use gerrit instead
On 2012/10/16 19:20:04, Peter Kasting wrote: > Yeah, that's what I was thinking: presumably NO_INSETS ...
8 years, 2 months ago (2012-10-16 19:22:42 UTC) #33
Peter Kasting
On 2012/10/16 19:22:42, Rouslan Solomakhin wrote: > On 2012/10/16 19:20:04, Peter Kasting wrote: > > ...
8 years, 2 months ago (2012-10-16 19:27:02 UTC) #34
please use gerrit instead
On 2012/10/16 19:27:02, Peter Kasting wrote: > On 2012/10/16 19:22:42, Rouslan Solomakhin wrote: > > ...
8 years, 2 months ago (2012-10-16 19:57:57 UTC) #35
commit-bot: I haz the power
Change committed as 162252
8 years, 2 months ago (2012-10-16 21:22:13 UTC) #36
please use gerrit instead
jhawkins@: I am TBRing your because I've added a file to chrome/chrome_browser_ui.gypi.
8 years, 2 months ago (2012-10-16 22:13:48 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/11044020/80006
8 years, 2 months ago (2012-10-16 22:14:51 UTC) #38
commit-bot: I haz the power
8 years, 2 months ago (2012-10-17 00:31:36 UTC) #39
Change committed as 162287

Powered by Google App Engine
This is Rietveld 408576698