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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1878963003: Convert BookmarkBubbleAppView to a DialogDelegateView (Closed)

Created:
4 years, 8 months ago by Evan Stade
Modified:
4 years, 8 months ago
Reviewers:
msw
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, tfarina, extensions-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Convert BookmarkBubbleAppView to a DialogDelegateView It's actually a dialog, not a bubble. BUG=585312 Committed: https://crrev.com/dde4b7928d207c039052846d01a4d3b6bcfd2cf8 Cr-Commit-Position: refs/heads/master@{#387104}

Patch Set 1 #

Total comments: 1

Patch Set 2 : iwyu #

Total comments: 14

Patch Set 3 : msw review #

Unified diffs Side-by-side diffs Delta from patch set Stats (+250 lines, -386 lines) Patch
D chrome/browser/ui/views/extensions/bookmark_app_bubble_view.h View 1 chunk +0 lines, -108 lines 0 comments Download
D chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc View 1 chunk +0 lines, -274 lines 0 comments Download
A chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.h View 1 2 1 chunk +77 lines, -0 lines 0 comments Download
A chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc View 1 2 1 chunk +168 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/frame/browser_view.cc View 1 2 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 15 (4 generated)
Evan Stade
4 years, 8 months ago (2016-04-12 22:05:07 UTC) #3
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1878963003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1878963003/1
4 years, 8 months ago (2016-04-12 22:05:23 UTC) #4
Evan Stade
https://codereview.chromium.org/1878963003/diff/1/chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc File chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc (left): https://codereview.chromium.org/1878963003/diff/1/chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc#oldcode117 chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc:117: new views::LabelButton(this, l10n_util::GetStringUTF16(IDS_ADD)); oops, this string needs to be ...
4 years, 8 months ago (2016-04-12 22:20:13 UTC) #5
msw
nits + IDS_ADD question https://codereview.chromium.org/1878963003/diff/20001/chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc File chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc (right): https://codereview.chromium.org/1878963003/diff/20001/chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc#newcode143 chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc:143: return button == ui::DIALOG_BUTTON_OK ? ...
4 years, 8 months ago (2016-04-13 00:54:38 UTC) #6
Evan Stade
https://codereview.chromium.org/1878963003/diff/20001/chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc File chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc (right): https://codereview.chromium.org/1878963003/diff/20001/chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc#newcode143 chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc:143: return button == ui::DIALOG_BUTTON_OK ? !GetTrimmedTitle().empty() : true; On ...
4 years, 8 months ago (2016-04-13 20:40:45 UTC) #7
msw
lgtm
4 years, 8 months ago (2016-04-13 20:44:37 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1878963003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1878963003/40001
4 years, 8 months ago (2016-04-13 21:02:25 UTC) #10
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 8 months ago (2016-04-13 21:33:11 UTC) #11
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/dde4b7928d207c039052846d01a4d3b6bcfd2cf8 Cr-Commit-Position: refs/heads/master@{#387104}
4 years, 8 months ago (2016-04-13 21:35:06 UTC) #13
Matt Giuca
This DCHECKs upon opening (https://crbug.com/605008). FYI, you can make a Release build with DCHECKs enabled ...
4 years, 8 months ago (2016-04-20 06:59:28 UTC) #14
Evan Stade
4 years, 8 months ago (2016-04-20 21:29:36 UTC) #15
Message was sent while issue was closed. 
On 2016/04/20 06:59:28, Matt Giuca wrote:
> This DCHECKs upon opening (https://crbug.com/605008). FYI, you can make a
> Release build with DCHECKs enabled with "dcheck_always_on = true" in args.gn
> (which I always do now, since I made a similar mistake awhile back). It's not
> noticeably slower than a normal Release build.

yes, I should do that, but I have also made the careless mistake of putting
something with an important side effect inside a DCHECK, and you wouldn't notice
that mistake with that config. Really, this needs a unit test so it would have
been caught in the CQ.

Powered by Google App Engine
This is Rietveld 408576698