Moved the version number in App Info Dialog

Moved the version number in App Info Dialog

As per UI review, moved the version in the App Info Dialog from the
header section to its own Details area. Also cleaned up and
simplified a lot of the code that laid out the name and version in
the header since, now that there's no version displayed there, its
no longer needed.

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

[Matt Giuca, 2014-07-14 01:57:23]

https://codereview.chromium.org/383953002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (left):

https://codereview.chromium.org/383953002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:2178: <message
name="IDS_APPLICATION_INFO_DESCRIPTION_TITLE" desc="Title of the first section
in the dialog, which displays a general summary of the app (the app's
description).">
Change "general summary" to "high-level overview". (If the string is changing,
the way you describe it probably should change too.)

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

https://codereview.chromium.org/383953002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:2188: +          Permissions
This change is unrelated and should be split into a separate CL (even if it's
just a one-line change).

https://codereview.chromium.org/383953002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_panel.h (right):

https://codereview.chromium.org/383953002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_panel.h:32: views::Label*
CreateLeftAlignedLabel(const base::string16& text) const;
I don't think this is a worthwhile abstraction -- it's only used four times and
only saves 1 line of code each time, but it means you force the reader to go and
track down this function. I suggest you just remove it.

(As opposed to CreateHeading, which I think is useful, because it doesn't just
do exactly what its title says; its title is semantic and it makes styling
decisions that *should* be abstracted.)

https://codereview.chromium.org/383953002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_panel.h:35: // views
appended to it. |child_spacing| defaults to the spacing between
This new comment doesn't really make sense. Maybe just say:
|child_spacing| defaults to |views::kRelatedControlVerticalSpacing|.
or
|child_spacing| defaults to a reasonable value.

https://codereview.chromium.org/383953002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc
(right):

https://codereview.chromium.org/383953002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc:220: }
Suggestion:
} else {
  DCHECK(!version_value_);
}

(i.e., always make sure that version_title_ and version_value_ are either both
present, or both absent.)

[sashab, 2014-07-14 23:19:21]

https://codereview.chromium.org/383953002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (left):

https://codereview.chromium.org/383953002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:2178: <message
name="IDS_APPLICATION_INFO_DESCRIPTION_TITLE" desc="Title of the first section
in the dialog, which displays a general summary of the app (the app's
description).">
On 2014/07/14 01:57:22, Matt Giuca wrote:
> Change "general summary" to "high-level overview". (If the string is changing,
> the way you describe it probably should change too.)

Done.

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

https://codereview.chromium.org/383953002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:2188: +          Permissions
On 2014/07/14 01:57:22, Matt Giuca wrote:
> This change is unrelated and should be split into a separate CL (even if it's
> just a one-line change).

I don't think this change is unrelated - we wanted to move the version number,
which had some knock-on effects and required we change some other areas so this
new layout made sense (e.g. it doesn't make sense to have a version with the 'It
can' text and the 'Overview' text - they only make sense together, in this
agreed UI design). Maybe I should change the CL title to "Rearranging things in
the AID"?

https://codereview.chromium.org/383953002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_panel.h (right):

https://codereview.chromium.org/383953002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_panel.h:32: views::Label*
CreateLeftAlignedLabel(const base::string16& text) const;
On 2014/07/14 01:57:23, Matt Giuca wrote:
> I don't think this is a worthwhile abstraction -- it's only used four times
and
> only saves 1 line of code each time, but it means you force the reader to go
and
> track down this function. I suggest you just remove it.
> 
> (As opposed to CreateHeading, which I think is useful, because it doesn't just
> do exactly what its title says; its title is semantic and it makes styling
> decisions that *should* be abstracted.)

Agreed. Done.

https://codereview.chromium.org/383953002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_panel.h:35: // views
appended to it. |child_spacing| defaults to the spacing between
On 2014/07/14 01:57:22, Matt Giuca wrote:
> This new comment doesn't really make sense. Maybe just say:
> |child_spacing| defaults to |views::kRelatedControlVerticalSpacing|.
> or
> |child_spacing| defaults to a reasonable value.

Done.

https://codereview.chromium.org/383953002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc
(right):

https://codereview.chromium.org/383953002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc:220: }
On 2014/07/14 01:57:23, Matt Giuca wrote:
> Suggestion:
> } else {
>   DCHECK(!version_value_);
> }
> 
> (i.e., always make sure that version_title_ and version_value_ are either both
> present, or both absent.)

Done.

[Matt Giuca, 2014-07-14 23:55:05]

lgtm

https://codereview.chromium.org/383953002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc
(right):

https://codereview.chromium.org/383953002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc:172:
description_label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
nit: Can you swap these two lines (just so they are as they were before, to
reduce the diff size).

[sashab, 2014-07-15 00:01:39]

https://codereview.chromium.org/383953002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc
(right):

https://codereview.chromium.org/383953002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc:172:
description_label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
On 2014/07/14 23:55:05, Matt Giuca wrote:
> nit: Can you swap these two lines (just so they are as they were before, to
> reduce the diff size).

Ahh yup, nice. Done.

[sashab, 2014-07-15 00:17:17]

The CQ bit was checked by sashab@chromium.org

[sashab, 2014-07-15 00:18:02]

The CQ bit was unchecked by sashab@chromium.org

[Matt Giuca, 2014-07-15 00:22:12]

lgtm again.

[sashab, 2014-07-17 23:22:59]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-07-17 23:24:26]

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

[commit-bot: I haz the power, 2014-07-18 02:39:11]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/30134)

[commit-bot: I haz the power, 2014-07-18 02:43:41]

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

[commit-bot: I haz the power, 2014-07-18 02:43:42]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/30151)

[sashab, 2014-07-20 23:08:18]

The CQ bit was checked by sashab@chromium.org

[sashab, 2014-07-20 23:08:22]

The CQ bit was unchecked by sashab@chromium.org

[sashab, 2014-07-20 23:29:18]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-07-20 23:30:08]

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

[commit-bot: I haz the power, 2014-07-21 01:41:26]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-07-21 01:43:49]

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

[commit-bot: I haz the power, 2014-07-21 01:43:49]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[sashab, 2014-07-21 02:51:48]

Oh, sorry - tapted, please review, haha!

I was wondering why it wasn't passing presubmit...

[sashab, 2014-07-21 02:53:05]

Actually sorry, tapted - I think benwells is a more suitable reviewer for this,
he has reviewed App Info Dialog layout changes in the past :)

benwells, please review! :)

[benwells, 2014-07-22 03:26:29]

just some minor things ...

https://codereview.chromium.org/383953002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc
(right):

https://codereview.chromium.org/383953002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc:219: if
(version_title_) {
Wouldn't (details_heading_ && !version_title_) be an error?

https://codereview.chromium.org/383953002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc:221:
views::View* horizontal_stack = CreateHorizontalStack(3);
I've seen this 3 twice now - it should be  in a constant somewhere.

[sashab, 2014-07-22 05:02:10]

https://codereview.chromium.org/383953002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc
(right):

https://codereview.chromium.org/383953002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc:219: if
(version_title_) {
On 2014/07/22 03:26:29, benwells wrote:
> Wouldn't (details_heading_ && !version_title_) be an error?

Right now, yes. But this is part of a more general structure where
details_heading_ is set if there are _any_ details, and then each title is set
with its corresponding version. I've actually changed this if-statement a bit,
but there are more details to come in here. Maybe we should even generalise it
to a map of strings that gets converted into these labels...

https://codereview.chromium.org/383953002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc:221:
views::View* horizontal_stack = CreateHorizontalStack(3);
On 2014/07/22 03:26:29, benwells wrote:
> I've seen this 3 twice now - it should be  in a constant somewhere.

Sure. This k-v display will become more common as more details are added. I
won't extract it into a function just yet, but I will in the next patch that
modifies this file.

https://codereview.chromium.org/383953002/diff/120001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/383953002/diff/120001/chrome/app/generated_re...
chrome/app/generated_resources.grd:2185: +          Version:
I'm also now considering whether this should be 'Version: $1', so the version is
substituted in, or concatenated like it is right now. I guess we'd never want a
'version' label that wasn't at the end or start (for RTL)...

[benwells, 2014-07-22 06:22:43]

https://codereview.chromium.org/383953002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_header_panel.cc
(right):

https://codereview.chromium.org/383953002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_header_panel.cc:116:
views::View* horizontal_links_container = CreateHorizontalStack(3);
Here is the other 3. Can you use the constant here too?

[sashab, 2014-07-22 23:22:12]

https://codereview.chromium.org/383953002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_header_panel.cc
(right):

https://codereview.chromium.org/383953002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_header_panel.cc:116:
views::View* horizontal_links_container = CreateHorizontalStack(3);
On 2014/07/22 06:22:43, benwells wrote:
> Here is the other 3. Can you use the constant here too?

Hmm, interesting that you found this, these are actually two different 3s...

Added a separate constant :)

[benwells, 2014-07-23 10:07:05]

lgtm

[sashab, 2014-07-23 22:44:11]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-07-23 22:46:40]

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

[commit-bot: I haz the power, 2014-07-24 08:30:14]

Change committed as 285160