Added tabs to the App Info dialog.

Added tabs to the App Info dialog, and removed the title.

Removed the title to make the tabs start at the very top of the dialog.

Has 3 tabs: Summary, Permissions and Manage
- The Summary tab displays the app's icon, name, version and description.
- The Permissions tab displays the app's permissions as a scrollable list
- The Manage tab is currently empty.

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

[sashab, 2014-02-27 06:20:03]

There are a few more changes coming after this that contain small features. This
review is mainly for feedback on the structure of the dialog.

[Matt Giuca, 2014-02-27 06:48:40]

Having not looked at the code yet, a few comments:
- Well done, this is looking really great!
- Also, great that you included screenshots in the review. This really helps.
- In future, you should upload screenshots as attachments on crbug.com, not to
an external site. There are a number of reasons for this -- we want them hosted
somewhere we control (for posterity -- we can't assume imgur will be up
forever), and if we need to mark the bug private for some reason, the images
will be private as well.
- My gut reaction to an >800-line review that claims to be adding a lot of
separate new features is that it should be split up into smaller CLs. (I've read
multiple external guidelines that say the ideal code review is 200-400 lines;
anything more than that is likely to fatigue the reviewer and they tend to miss
things.) It's also good to split up into smaller CLs because then each feature
can be vetted independently, and if necessary, rolled back independently.

I think you should strip this CL down so it simply refactors the existing
app_info_dialog_views into the individual tab view classes (this CL should not
change or add any new behaviour; it is just a refactor). Following that, you
should mail out several individual (independent) CLs that add new functionality
to each tab view. I think that would be much more palatable. You promised more
features coming soon -- those should be in their own CLs as well.

Thanks

[sashab, 2014-02-27 08:42:52]

I have removed the whole 'BulletedScrollList' and similar refactoring of
the permissions tab. Now the permissions tab works and looks the same way
as it did before.


On Thu, Feb 27, 2014 at 5:48 PM, <mgiuca@chromium.org> wrote:

> Having not looked at the code yet, a few comments:
> - Well done, this is looking really great!
> - Also, great that you included screenshots in the review. This really
> helps.
> - In future, you should upload screenshots as attachments on crbug.com,
> not to
> an external site. There are a number of reasons for this -- we want them
> hosted
> somewhere we control (for posterity -- we can't assume imgur will be up
> forever), and if we need to mark the bug private for some reason, the
> images
> will be private as well.
> - My gut reaction to an >800-line review that claims to be adding a lot of
> separate new features is that it should be split up into smaller CLs.
> (I've read
> multiple external guidelines that say the ideal code review is 200-400
> lines;
> anything more than that is likely to fatigue the reviewer and they tend to
> miss
> things.) It's also good to split up into smaller CLs because then each
> feature
> can be vetted independently, and if necessary, rolled back independently.
>
> I think you should strip this CL down so it simply refactors the existing
> app_info_dialog_views into the individual tab view classes (this CL should
> not
> change or add any new behaviour; it is just a refactor). Following that,
> you
> should mail out several individual (independent) CLs that add new
> functionality
> to each tab view. I think that would be much more palatable. You promised
> more
> features coming soon -- those should be in their own CLs as well.
>
> Thanks
>
> https://codereview.chromium.org/180723010/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Matt Giuca, 2014-03-04 01:36:56]

Good, I'm quite happy with the amount of work being done in this CL and it
generally looks good. Just a lot of small issues and a few questions (note that
some of my comments are really addressed at the existing code but I think it's
still good to consider them).

https://codereview.chromium.org/180723010/diff/50001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/180723010/diff/50001/chrome/app/generated_res...
chrome/app/generated_resources.grd:2220: +      <message
name="IDS_APPLICATION_INFO_SUMMARY_TAB_TITLE" desc="Title of the first tab in
the dialog, which displays general summary information about the app. ">
Don't you need title case and normal case versions of these strings?

(What is there is another language where "Summary" is two words, and the second
word should be capitalized or not depending on platform?)

FYI I think it's quite ridiculous that we distinguish between the two, but
that's how it is.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:19: void
ShowChromeAppInfoDialog(gfx::NativeWindow parent_window,
Can this just be called ShowAppInfoDialog?

(I think you probably copied this off ShowCreateChromeAppShortcutsDialog. In
that case, the "Chrome" was there to distinguish from
ShowCreateWebAppShortcutsDialog.)

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h
(right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:22: class
AppInfoView : public views::DialogDelegateView,
In general, the header file should have the same name as the primary class
declared in that file (camel case replaced with underscores). You seem to be
violating this rule throughout these classes.

Rename to app_info_view.h, as well as renaming the other .h and .cc files.
(Remember to change the include guard macro names as well.)

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_manage_views.cc
(right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_manage_views.cc:13: :
AppInfoTabView(parent_window, profile, app, close_callback) {
Nit: // TODO(sashab): Populate this tab.

(Also, nit: if you don't have anything in a function, "{}"; this won't apply if
you add a TODO.)

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_manage_views.h
(right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_manage_views.h:17:
class AppInfoManageTabView : public AppInfoTabView {
Add a header explaining what sort of things go in this view.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_permissions_views.cc
(right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_permissions_views.cc:23:
// Get the permission messages for the app
Nit: Comments are complete sentences so they should always end in a full stop.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_permissions_views.h
(right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_permissions_views.h:19:
class PermissionsScrollView : public message_center::BoundedScrollView {
You shouldn't need this class in the header file (unless I'm missing something).
Move to the .cc file.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_permissions_views.h:29:
class AppInfoPermissionsTabView : public AppInfoTabView {
Add a header explaining what sort of things go in this view.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_summary_views.cc
(right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_summary_views.cc:33:
weak_ptr_factory_(this) {
Maybe I'm mis-remembering, but didn't we decide that it would be unsafe to have
a weak_ptr_factory in the same class that has weak pointers pointing at it?
Because if the object dies and then you try to get() the weak pointer, it will
consult the weak pointer factory which is now a dangling pointer?

I'm not too familiar with how weak pointers work to know whether this is true.
Maybe you have a better understanding of this so you can explain whether I'm
wrong or whether this is a legitimate concern.

NOTE: If you were to change the way weak pointers are structured, you should do
it in a separate CL to this, because the weak pointer code is already in the
existing codebase.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_summary_views.h
(right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_summary_views.h:23:
class AppInfoSummaryTabView
Add a header explaining what sort of things go in this view.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_views.h (right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_views.h:22: // profile
and app.
Nit: No newline here.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog_views.h (left):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog_views.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
CL description:

The description seems to be out of date (describing the bigger patch you had
prepared before you split it up).

Can you remove the imgur links. This text is going to be permanently in the
Chrome SVN history. I think in general you should link to screenshots in review
discussion, not in the CL description. Please paste the relevant ones into a
comment on this CL, and delete from the description.

Say that you removed the heading from the dialog.

Don't need to say that it still requires the flag (describe what you changed,
not what you didn't change). Don't be vague ("kinda works with hosted apps");
say what is broken about it. Don't rely on screenshots.

[sashab, 2014-03-04 04:10:58]

Fixed up comments & renamed classes as per discussion.

https://codereview.chromium.org/180723010/diff/50001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/180723010/diff/50001/chrome/app/generated_res...
chrome/app/generated_resources.grd:2220: +      <message
name="IDS_APPLICATION_INFO_SUMMARY_TAB_TITLE" desc="Title of the first tab in
the dialog, which displays general summary information about the app. ">
On 2014/03/04 01:36:57, Matt Giuca wrote:
> Don't you need title case and normal case versions of these strings?
> 
> (What is there is another language where "Summary" is two words, and the
second
> word should be capitalized or not depending on platform?)
> 
> FYI I think it's quite ridiculous that we distinguish between the two, but
> that's how it is.

Done.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:19: void
ShowChromeAppInfoDialog(gfx::NativeWindow parent_window,
On 2014/03/04 01:36:57, Matt Giuca wrote:
> Can this just be called ShowAppInfoDialog?
> 
> (I think you probably copied this off ShowCreateChromeAppShortcutsDialog. In
> that case, the "Chrome" was there to distinguish from
> ShowCreateWebAppShortcutsDialog.)

Done.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h
(right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:22: class
AppInfoView : public views::DialogDelegateView,
On 2014/03/04 01:36:57, Matt Giuca wrote:
> In general, the header file should have the same name as the primary class
> declared in that file (camel case replaced with underscores). You seem to be
> violating this rule throughout these classes.
> 
> Rename to app_info_view.h, as well as renaming the other .h and .cc files.
> (Remember to change the include guard macro names as well.)

Since this is soon to be implemented in cocoa, it needs the _views extension on
the app_info_dialog class. I have removed the suffix from the other classes, and
renamed them appropriately.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_manage_views.cc
(right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_manage_views.cc:13: :
AppInfoTabView(parent_window, profile, app, close_callback) {
On 2014/03/04 01:36:57, Matt Giuca wrote:
> Nit: // TODO(sashab): Populate this tab.
> 
> (Also, nit: if you don't have anything in a function, "{}"; this won't apply
if
> you add a TODO.)

Done.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_manage_views.h
(right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_manage_views.h:17:
class AppInfoManageTabView : public AppInfoTabView {
On 2014/03/04 01:36:57, Matt Giuca wrote:
> Add a header explaining what sort of things go in this view.

Done.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_permissions_views.cc
(right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_permissions_views.cc:23:
// Get the permission messages for the app
On 2014/03/04 01:36:57, Matt Giuca wrote:
> Nit: Comments are complete sentences so they should always end in a full stop.

Done to all comments like this.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_permissions_views.h
(right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_permissions_views.h:19:
class PermissionsScrollView : public message_center::BoundedScrollView {
On 2014/03/04 01:36:57, Matt Giuca wrote:
> You shouldn't need this class in the header file (unless I'm missing
something).
> Move to the .cc file.

Moved, and moved all corresponding #includes as well.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_permissions_views.h:29:
class AppInfoPermissionsTabView : public AppInfoTabView {
On 2014/03/04 01:36:57, Matt Giuca wrote:
> Add a header explaining what sort of things go in this view.

Done.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_summary_views.cc
(right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_summary_views.cc:33:
weak_ptr_factory_(this) {
On 2014/03/04 01:36:57, Matt Giuca wrote:
> Maybe I'm mis-remembering, but didn't we decide that it would be unsafe to
have
> a weak_ptr_factory in the same class that has weak pointers pointing at it?
> Because if the object dies and then you try to get() the weak pointer, it will
> consult the weak pointer factory which is now a dangling pointer?
> 
> I'm not too familiar with how weak pointers work to know whether this is true.
> Maybe you have a better understanding of this so you can explain whether I'm
> wrong or whether this is a legitimate concern.
> 
> NOTE: If you were to change the way weak pointers are structured, you should
do
> it in a separate CL to this, because the weak pointer code is already in the
> existing codebase.

I investigated this in that review and decided it was totally OK:
https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_summary_views.h
(right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_summary_views.h:23:
class AppInfoSummaryTabView
On 2014/03/04 01:36:57, Matt Giuca wrote:
> Add a header explaining what sort of things go in this view.

Done.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_views.h (right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_views.h:22: // profile
and app.
On 2014/03/04 01:36:57, Matt Giuca wrote:
> Nit: No newline here.

Done.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog_views.h (left):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog_views.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2014/03/04 01:36:57, Matt Giuca wrote:
> CL description:
> 
> The description seems to be out of date (describing the bigger patch you had
> prepared before you split it up).

Done

> 
> Can you remove the imgur links. This text is going to be permanently in the
> Chrome SVN history. I think in general you should link to screenshots in
review
> discussion, not in the CL description. Please paste the relevant ones into a
> comment on this CL, and delete from the description.

Done - I don't think any screenshots are needed for this one right now.

> 
> Say that you removed the heading from the dialog.

Done.

> 
> Don't need to say that it still requires the flag (describe what you changed,
> not what you didn't change).

Done.

> Don't be vague ("kinda works with hosted apps");
> say what is broken about it. Don't rely on screenshots.

Removed this and put it in a separate bug report:
https://code.google.com/p/chromium/issues/detail?id=348857

[Matt Giuca, 2014-03-04 04:51:12]

LGTM but please revert the ShowChromeAppInfoDialog change.

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_summary_views.cc
(right):

https://codereview.chromium.org/180723010/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab_summary_views.cc:33:
weak_ptr_factory_(this) {
Oh yeah whoops. Sorry I picked up the same issue as last time which we already
discussed.

Yes I've gone over the weak ref implementation now and it makes sense to me why
this works. (A weak ptr does not rely on its weak ptr factory being alive; it
only relies on a hidden flag object being alive, and that object is refcounted
by the weak ptrs themselves. The death of the factory just makes the weak refs
null, not invalid.)

https://codereview.chromium.org/180723010/diff/70001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_controller_delegate.cc (right):

https://codereview.chromium.org/180723010/diff/70001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_controller_delegate.cc:99:
ShowAppInfoDialog(
Whoops, I didn't realise my asking you to do this would change a whole lot of
existing files. Sorry to do this but would you be able to revert this change and
do it in a separate CL (if you think it's important enough).

It's not worth doing a "just while you're here" change if it means bringing 5
new files into a CL. (And feel free to push back on any reviewer who asks you to
do such a change --- they might not have realised what they were asking you to
do!)

[sashab, 2014-03-04 21:26:37]

Thanks, reverted. benwells can you review?

[benwells, 2014-03-05 05:11:02]

looking really good!

https://codereview.chromium.org/180723010/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/180723010/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:53:
tabbed_pane_->set_listener(this);
Is this needed? I tried removing it and everything seemed to work. Note that if
you remove this, you don't need to implement the TabbedPaneListener class, and
you don't need to hold on to tabbed_pane_ as an instance field.

https://codereview.chromium.org/180723010/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h
(right):

https://codereview.chromium.org/180723010/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:49: //
Profile in which the shortcuts will be created.
Nit: What shortcuts?

[sashab, 2014-03-06 23:31:03]

Removed TabbedPane implementation - nice find! :-)

https://codereview.chromium.org/180723010/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/180723010/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:53:
tabbed_pane_->set_listener(this);
On 2014/03/05 05:11:02, benwells wrote:
> Is this needed? I tried removing it and everything seemed to work. Note that
if
> you remove this, you don't need to implement the TabbedPaneListener class, and
> you don't need to hold on to tabbed_pane_ as an instance field.

You're right. After taking a look at tabbed_pane.cc:280 (the implementation of
TabbedPane::SelectTabAt), the selected tab is automatically laid out and
painted. If there's a listener, the listener's TabSelectedAt method is called as
well (so, in my code, this would lay out the tab twice).

The reason website_settings_popup_view.cc (the place I based this code off) uses
this is because it automatically resizes the tab contents when the tab changes.
This may be something I want to add in later... but not now. Thanks for pointing
this out :)

https://codereview.chromium.org/180723010/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h
(right):

https://codereview.chromium.org/180723010/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:49: //
Profile in which the shortcuts will be created.
On 2014/03/05 05:11:02, benwells wrote:
> Nit: What shortcuts?

Artifact from copied code. Removed (and I moved profile_ into the list of
members below).

[benwells, 2014-03-07 00:00:33]

https://codereview.chromium.org/180723010/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h
(right):

https://codereview.chromium.org/180723010/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:50:
views::TabbedPane* tabbed_pane_;
This member isn't needed any more.

[sashab, 2014-03-07 02:28:46]

Fixed, thanks for finding that for me. Only weird thing now is that
chrome/chrome_browser_ui.gypi seems to have been changing throughout the review.
Looks like its from rebasing/generated files.

https://codereview.chromium.org/180723010/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h
(right):

https://codereview.chromium.org/180723010/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:50:
views::TabbedPane* tabbed_pane_;
On 2014/03/07 00:00:34, benwells wrote:
> This member isn't needed any more.

Done.

[benwells, 2014-03-07 02:33:31]

lgtm

[sashab, 2014-03-07 02:33:56]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-03-07 02:37:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sashab@chromium.org/180723010/130001

[commit-bot: I haz the power, 2014-03-07 05:23:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 05:23:57]

Try jobs failed on following builders: mac_chromium_rel

[sashab, 2014-03-07 06:12:57]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-03-07 06:14:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sashab@chromium.org/180723010/130001

[commit-bot: I haz the power, 2014-03-07 15:16:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 15:16:23]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[sashab, 2014-03-09 22:28:39]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-03-09 22:28:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sashab@chromium.org/180723010/130001

[commit-bot: I haz the power, 2014-03-10 06:20:22]

Change committed as 255880