webapps: propogate name and shortName from manifest to Java

webapps: propogate name and shortName from manifest to Java

In the past we simply retrieved the name and short name and picked the latter
to be the title if it existed or otherwise we used the former. The user was
then able to edit the title and save the web app on their homescreen. This title
was then used on the launcher as well as in recents.

However, with the new splash screen which might be up soon, we need the long
name from the website to display on the splash screen. It also looks better
if recents utilizes the short name which is specified by the website rather than
a user specified title which is used on the launcher.

Because of all this, this CL propogates the info through various C++ and Java
classes to allow the above usage. Now the user title is used just for the home
screen icon. The name is used on the splash screen and the short name is used
in recents.

A note on backward compatibility: current webapps will simply use the
user specified title which is currently saved in the intent - this CL only
affects new webapps which the user adds after it lands.

BUG=508628
Committed: https://crrev.com/f3ee51858aec43abdb498cb7d7ce778a121fd999
Cr-Commit-Position: refs/heads/master@{#339697}

[Lalit Maganti, 2015-07-09 15:00:36]

lalitm@google.com changed reviewers:
+ mounirri@chromium.org

[Lalit Maganti, 2015-07-09 15:03:12]

lalitm@google.com changed reviewers:
- mounirri@chromium.org

[Lalit Maganti, 2015-07-09 15:03:37]

lalitm@google.com changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-07-09 16:34:17]

It looks good. Could you add some tests?

https://codereview.chromium.org/1224273003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://codereview.chromium.org/1224273003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:778:

nit: remove empty line

https://codereview.chromium.org/1224273003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:784:
String webappTitle = IntentUtils.safeGetStringExtra(intent,
ShortcutHelper.EXTRA_TITLE);
nit: add comment explaining this is for backward compatibility.

https://codereview.chromium.org/1224273003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/1224273003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:51:
String title = intent.getStringExtra(ShortcutHelper.EXTRA_TITLE);
nit: add comment explaining this is for backward compatibility?

https://codereview.chromium.org/1224273003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:56:
name = name == null ? shortName : name;
s/shortName/title/

We should avoid ending up in a situation where EXTRA_NAME is set but not
EXTRA_SHORT_NAME.

https://codereview.chromium.org/1224273003/diff/40001/chrome/browser/android/...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/1224273003/diff/40001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.cc:103: void
ShortcutHelper::AddShortcut(JNIEnv* env, jobject obj, jstring jusertitle) {
Hmm, "jtitle" was border line wrt coding style, jusertitle is clearly not okay.
I would do j_user_title.

https://codereview.chromium.org/1224273003/diff/40001/chrome/browser/android/...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/1224273003/diff/40001/chrome/browser/android/...
chrome/browser/android/shortcut_info.cc:29: }
That means we might end up with short_name or name not being define. I think it
might be better if when one is missing, we set it with the other one so when the
intent information are used, the consumer do not have to worry about fallback.

[mlamouri (slow - plz ping), 2015-07-09 16:38:39]

https://codereview.chromium.org/1224273003/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java
(right):

https://codereview.chromium.org/1224273003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:83:
public void testIntentShortNameFallBack() {
We should fallback to title if we have a title which is the case here.

Otherwise, you should add a test that check that if you have no name but a
title, the name is set to the empty string.

[Lalit Maganti, 2015-07-09 17:48:42]

lalitm@google.com changed reviewers:
+ dfalcantara@chromium.org

[gone, 2015-07-09 18:05:41]

dfalcantara@chromium.org changed reviewers:
+ palmer@chromium.org

[gone, 2015-07-09 18:05:42]

+palmer for security concerns

Chris: this CL seems to be introducing a difference between the title that shows
up in Android's Recents menu and the title that is displayed on the Android home
screen.  Is security OK with that?

https://codereview.chromium.org/1224273003/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/1224273003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:46: void
onUserTitleAvailable(String title);
This isn't a user title.  This is the page title.

https://codereview.chromium.org/1224273003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://codereview.chromium.org/1224273003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:784:
// shortName - they'll just have title
Follow the style guide and how other comments are formatted.  At the least, end
the sentence with a period.

[gone, 2015-07-09 18:07:55]

Also: what's the bug number?

[Lalit Maganti, 2015-07-09 18:19:02]

Currently I don't think there is a bug open for this. I will consult with Mounir
and open one.

https://codereview.chromium.org/1224273003/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/1224273003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:46: void
onUserTitleAvailable(String title);
On 2015/07/09 at 18:05:42, dfalcantara wrote:
> This isn't a user title.  This is the page title.

Not exactly. It could be one of three things in the following order of
preference:
1. The short name from the manifest
2. The name from the manifest
3. The page title

The reason why I prefixed it with User is because this is shown to the user in
the
Add to Homescreen dialog. I found leaving it as title made it a bit confusing
since
it might be interpreted as being the page title wheras this is not necessarily
the
case.

https://codereview.chromium.org/1224273003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://codereview.chromium.org/1224273003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:784:
// shortName - they'll just have title
On 2015/07/09 at 18:05:42, dfalcantara wrote:
> Follow the style guide and how other comments are formatted.  At the least,
end the sentence with a period.

Will fix.

[palmer, 2015-07-09 18:26:55]

Definitely need a bug number.

I'm a little skeptical of this change, because it seems potentially phishy to
allow sites to arbitrarily choose their name in Android system UI. Imagine
popularblog.com changing itself to appear as bank.com on your home screen or in
recents.

I'd rather have the user-chosen name, and — if necessary — also the eTLD+1 as a
short name (see
https://www.chromium.org/Home/chromium-security/enamel#TOC-Eliding-Origin-Nam...).

Is there a design document or other motivating argument for this feature?

[mlamouri (slow - plz ping), 2015-07-09 19:44:36]

On 2015/07/09 at 18:26:55, palmer wrote:
> Definitely need a bug number.

Done.

> I'm a little skeptical of this change, because it seems potentially phishy to
allow sites to arbitrarily choose their name in Android system UI. Imagine
popularblog.com changing itself to appear as bank.com on your home screen or in
recents.

With Lalit's change, we still use the user defined title in the homescreen and
the user will still see the icon being put in the homescreen. The difference is
that the Recents title will be the Manifest short name. Note that it is not
different from a tab in L document mode: the Recents view will show the tab's
icon and the tab's title. A user still can't be spoofed into thinking that
popuplarblog.com is actually bank.com but after the web app is loaded, can be
spoofed the same way it could by loading popularblog.com from Chrome.

This said, I agree that we are exposing fishy information which even though are
used safely now might be used in a different way later. Maybe we could add a
comment explaining that the name and short name are not validated by the user
and should not be shown in any security sensitive UI?

> I'd rather have the user-chosen name, and — if necessary — also the eTLD+1 as
a short name (see
https://www.chromium.org/Home/chromium-security/enamel#TOC-Eliding-Origin-Nam...).
> 
> Is there a design document or other motivating argument for this feature?

The short term goal is to use that for the splashscreen. The general idea is to
use the power of the Manifest and be able to have two different names to use
depending on the context.

[gone, 2015-07-09 20:31:14]

> With Lalit's change, we still use the user defined title in the homescreen and
> the user will still see the icon being put in the homescreen. The difference
is
> that the Recents title will be the Manifest short name. Note that it is not
> different from a tab in L document mode: the Recents view will show the tab's
> icon and the tab's title. A user still can't be spoofed into thinking that
> http://popuplarblog.com is actually http://bank.com but after the web app is
loaded, can be
> spoofed the same way it could by loading http://popularblog.com from Chrome.

Web apps don't currently change the title of their task, though.

[gone, 2015-07-09 20:31:39]

On 2015/07/09 20:31:14, dfalcantara wrote:
> > With Lalit's change, we still use the user defined title in the homescreen
and
> > the user will still see the icon being put in the homescreen. The difference
> is
> > that the Recents title will be the Manifest short name. Note that it is not
> > different from a tab in L document mode: the Recents view will show the
tab's
> > icon and the tab's title. A user still can't be spoofed into thinking that
> > http://popuplarblog.com is actually http://bank.com but after the web app is
> loaded, can be
> > spoofed the same way it could by loading http://popularblog.com from Chrome.
> 
> Web apps don't currently change the title of their task, though.

More specifically: the web app task title doesn't change as the user navigates
on L.

[mlamouri (slow - plz ping), 2015-07-09 20:38:55]

On 2015/07/09 at 20:31:39, dfalcantara wrote:
> On 2015/07/09 20:31:14, dfalcantara wrote:
> > > With Lalit's change, we still use the user defined title in the homescreen
and
> > > the user will still see the icon being put in the homescreen. The
difference
> > is
> > > that the Recents title will be the Manifest short name. Note that it is
not
> > > different from a tab in L document mode: the Recents view will show the
tab's
> > > icon and the tab's title. A user still can't be spoofed into thinking that
> > > http://popuplarblog.com is actually http://bank.com but after the web app
is
> > loaded, can be
> > > spoofed the same way it could by loading http://popularblog.com from
Chrome.
> > 
> > Web apps don't currently change the title of their task, though.
> 
> More specifically: the web app task title doesn't change as the user navigates
on L.

Yes, but I'm not sure why allowing any malicious website to have "Bank" in the
Recents view is considered less a security concern than having Web Apps doing
that.

[gone, 2015-07-10 01:44:57]

On 2015/07/09 20:38:55, Mounir Lamouri wrote:
> On 2015/07/09 at 20:31:39, dfalcantara wrote:
> > On 2015/07/09 20:31:14, dfalcantara wrote:
> > > > With Lalit's change, we still use the user defined title in the
homescreen
> and
> > > > the user will still see the icon being put in the homescreen. The
> difference
> > > is
> > > > that the Recents title will be the Manifest short name. Note that it is
> not
> > > > different from a tab in L document mode: the Recents view will show the
> tab's
> > > > icon and the tab's title. A user still can't be spoofed into thinking
that
> > > > http://popuplarblog.com is actually http://bank.com but after the web
app
> is
> > > loaded, can be
> > > > spoofed the same way it could by loading http://popularblog.com from
> Chrome.
> > > 
> > > Web apps don't currently change the title of their task, though.
> > 
> > More specifically: the web app task title doesn't change as the user
navigates
> on L.
> 
> Yes, but I'm not sure why allowing any malicious website to have "Bank" in the
> Recents view is considered less a security concern than having Web Apps doing
> that.

Going to hold off on the review until palmer@ gives a OK security-wise.

[palmer, 2015-07-10 19:05:39]

What's the procedure I follow to see how this works and looks in real life
(assuming I've built Chrome with this patch applied)? (And/or, can you post
screenshots on the bug?)

[palmer, 2015-07-10 20:34:31]

Also, the patch doesn't seem to apply. Maybe you need to rebase?

/ssd/clank/src $ git apply diff
error: patch failed:
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:140
error: chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:
patch does not apply
error:
chrome/android/javatests_shell/src/org/chromium/chrome/browser/ShortcutHelperTest.java:
No such file or directory

I have:

/ssd/clank/src $ search -N ShortcutHelperTest.java
./chrome/android/javatests/src/org/chromium/chrome/browser/ShortcutHelperTest.java

(i.e. javatests instead of javatests_shell)

[Lalit Maganti, 2015-07-10 23:53:13]

On 2015/07/10 at 20:34:31, palmer wrote:
> Also, the patch doesn't seem to apply. Maybe you need to rebase?
> 
> /ssd/clank/src $ git apply diff
> error: patch failed:
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:140
> error:
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java: patch
does not apply
> error:
chrome/android/javatests_shell/src/org/chromium/chrome/browser/ShortcutHelperTest.java:
No such file or directory
> 
> I have:
> 
> /ssd/clank/src $ search -N ShortcutHelperTest.java
>
./chrome/android/javatests/src/org/chromium/chrome/browser/ShortcutHelperTest.java
> 
> (i.e. javatests instead of javatests_shell)

Unfortunately I don't have remote access since I'm an intern and Mounir is away
at the moment so I won't be able to fix/rebase the CL or post screenshots until
Monday. However, if you force the patch to apply and simply ignore the broken
test then the following is important:
1. The user approved icon and title are used on the home screen - this hasn't
changed from the current system
2. The recents menu now uses the website's short name from the manifest or a
page title if that specific page asks for one to be set.
3. The splash screen (which will be there in a separate CL and isn't strictly a
part of this one - this CL facilitates that CL though) will always use the long
name of the website from the manifest.

[Lalit Maganti, 2015-07-13 10:13:59]

Have rebased the patch. Hopefully should be good for review on the security
side.

[mlamouri (slow - plz ping), 2015-07-15 14:28:52]

On 2015/07/10 at 23:53:13, lalitm wrote:
> On 2015/07/10 at 20:34:31, palmer wrote:
> > Also, the patch doesn't seem to apply. Maybe you need to rebase?
> > 
> > /ssd/clank/src $ git apply diff
> > error: patch failed:
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:140
> > error:
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java: patch
does not apply
> > error:
chrome/android/javatests_shell/src/org/chromium/chrome/browser/ShortcutHelperTest.java:
No such file or directory
> > 
> > I have:
> > 
> > /ssd/clank/src $ search -N ShortcutHelperTest.java
> >
./chrome/android/javatests/src/org/chromium/chrome/browser/ShortcutHelperTest.java
> > 
> > (i.e. javatests instead of javatests_shell)
> 
> Unfortunately I don't have remote access since I'm an intern and Mounir is
away at the moment so I won't be able to fix/rebase the CL or post screenshots
until Monday. However, if you force the patch to apply and simply ignore the
broken test then the following is important:
> 1. The user approved icon and title are used on the home screen - this hasn't
changed from the current system
> 2. The recents menu now uses the website's short name from the manifest or a
page title if that specific page asks for one to be set.
> 3. The splash screen (which will be there in a separate CL and isn't strictly
a part of this one - this CL facilitates that CL though) will always use the
long name of the website from the manifest.

That sounds correct. Though I don't think we show the page title for Web Apps.
Regular tabs will have their title in the Recents menu.

[mlamouri (slow - plz ping), 2015-07-17 11:39:08]

palmer@, is there anything you need to help with the security review on this
change?

[palmer, 2015-07-20 18:08:56]

OK, I think I get this now. LGTM.

[gone, 2015-07-20 22:19:13]

nits, but lgtm with security approval

https://codereview.chromium.org/1224273003/diff/200001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/1224273003/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:61:
name = name == null ? title : name;
You've effectively duplicated this logic.  Can you move it somewhere so that
ChromeLauncherActivity and WebappInfo can share it?

https://codereview.chromium.org/1224273003/diff/200001/chrome/browser/android...
File chrome/browser/android/shortcut_data_fetcher.cc (right):

https://codereview.chromium.org/1224273003/diff/200001/chrome/browser/android...
chrome/browser/android/shortcut_data_fetcher.cc:68: // Simply set the user
editable title to be the page's title
nit:user-editable

https://codereview.chromium.org/1224273003/diff/200001/chrome/browser/android...
File chrome/browser/android/shortcut_info.h (right):

https://codereview.chromium.org/1224273003/diff/200001/chrome/browser/android...
chrome/browser/android/shortcut_info.h:32: ~ShortcutInfo();
1) ~ShortcutInfo() = default;
2) Remove empty destructor from cc file

[gone, 2015-07-20 22:20:17]

https://codereview.chromium.org/1224273003/diff/200001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java
(right):

https://codereview.chromium.org/1224273003/diff/200001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:77:
assertNotNull(info);
nit: the expected values are always first in test asserts.

[mlamouri (slow - plz ping), 2015-07-20 23:55:59]

lgtm with dfalcantara@ comments applied.

https://codereview.chromium.org/1224273003/diff/200001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/1224273003/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:33:
public static final String EXTRA_TITLE =
"org.chromium.chrome.browser.webapp_title";
nit: could you live a comment that EXTRA_TITLE is only here for backward
compatibility?

https://codereview.chromium.org/1224273003/diff/200001/chrome/browser/android...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/1224273003/diff/200001/chrome/browser/android...
chrome/browser/android/shortcut_info.cc:30: }
If neither manifest.short_name or manifest.name are null, does that mean
|short_name| and |name| will not be set? I'm pretty sure I'm missing something
:)

[Lalit Maganti, 2015-07-21 09:07:26]

lalitm@google.com changed reviewers:
+ bauerb@chromium.org

[Lalit Maganti, 2015-07-21 09:07:27]

mlamouri@: dfalcantara@: please see my replies/fixes to your comments.
bauerb@: could you please review the patch as you own all of the remaining
non-owned files.

https://codereview.chromium.org/1224273003/diff/200001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/1224273003/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:33:
public static final String EXTRA_TITLE =
"org.chromium.chrome.browser.webapp_title";
On 2015/07/20 at 23:55:59, Mounir Lamouri wrote:
> nit: could you live a comment that EXTRA_TITLE is only here for backward
compatibility?

Done.

https://codereview.chromium.org/1224273003/diff/200001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/1224273003/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:61:
name = name == null ? title : name;
On 2015/07/20 at 22:19:12, dfalcantara wrote:
> You've effectively duplicated this logic.  Can you move it somewhere so that
ChromeLauncherActivity and WebappInfo can share it?

Created three new methods in WebappInfo which can be used to get the name,
shortName and title.

https://codereview.chromium.org/1224273003/diff/200001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java
(right):

https://codereview.chromium.org/1224273003/diff/200001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:77:
assertNotNull(info);
On 2015/07/20 at 22:20:16, dfalcantara wrote:
> nit: the expected values are always first in test asserts.

Swapped.

https://codereview.chromium.org/1224273003/diff/200001/chrome/browser/android...
File chrome/browser/android/shortcut_data_fetcher.cc (right):

https://codereview.chromium.org/1224273003/diff/200001/chrome/browser/android...
chrome/browser/android/shortcut_data_fetcher.cc:68: // Simply set the user
editable title to be the page's title
On 2015/07/20 at 22:19:12, dfalcantara wrote:
> nit:user-editable

Done.

https://codereview.chromium.org/1224273003/diff/200001/chrome/browser/android...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/1224273003/diff/200001/chrome/browser/android...
chrome/browser/android/shortcut_info.cc:30: }
On 2015/07/20 at 23:55:59, Mounir Lamouri wrote:
> If neither manifest.short_name or manifest.name are null, does that mean
|short_name| and |name| will not be set? I'm pretty sure I'm missing something
:)

No you aren't missing anything - I was missing the else branch of the if
statement.

https://codereview.chromium.org/1224273003/diff/200001/chrome/browser/android...
chrome/browser/android/shortcut_info.cc:30: }
On 2015/07/20 at 23:55:59, Mounir Lamouri wrote:
> If neither manifest.short_name or manifest.name are null, does that mean
|short_name| and |name| will not be set? I'm pretty sure I'm missing something
:)

No you're not missing anything - I was missing the else branch of this if
statement.

https://codereview.chromium.org/1224273003/diff/200001/chrome/browser/android...
File chrome/browser/android/shortcut_info.h (right):

https://codereview.chromium.org/1224273003/diff/200001/chrome/browser/android...
chrome/browser/android/shortcut_info.h:32: ~ShortcutInfo();
On 2015/07/20 at 22:19:12, dfalcantara wrote:
> 1) ~ShortcutInfo() = default;
> 2) Remove empty destructor from cc file

Done.

[mlamouri (slow - plz ping), 2015-07-21 10:24:47]

https://codereview.chromium.org/1224273003/diff/220001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/1224273003/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:39:
public static String titleFromIntent(Intent intent) {
I don't think you need to expose titleFromIntent. You can just expose name and
shortName which will try to get the EXTRA_TITLE if EXTRA_NAME/EXTRA_SHORT_NAME
are not available.

https://codereview.chromium.org/1224273003/diff/220001/chrome/browser/android...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/1224273003/diff/220001/chrome/browser/android...
chrome/browser/android/shortcut_info.cc:30: }
I think this will explode if manifest.short_name and manifest.name are null.

What about:
if (!manifest.short_name.is_null())
  short_name = manifest.short_name.string();
if (!manifest.name.is_null())
  name = manifest.name.string();
if (manifest.short_name.is_null() != manifest.name.is_null()) {
  if (manifest.short_name.is_null())
    short_name = name;
  name = short_name;
}

[Lalit Maganti, 2015-07-21 10:26:44]

https://codereview.chromium.org/1224273003/diff/220001/chrome/browser/android...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/1224273003/diff/220001/chrome/browser/android...
chrome/browser/android/shortcut_info.cc:30: }
On 2015/07/21 at 10:24:47, Mounir Lamouri wrote:
> I think this will explode if manifest.short_name and manifest.name are null.
> 
> What about:
> if (!manifest.short_name.is_null())
>   short_name = manifest.short_name.string();
> if (!manifest.name.is_null())
>   name = manifest.name.string();
> if (manifest.short_name.is_null() != manifest.name.is_null()) {
>   if (manifest.short_name.is_null())
>     short_name = name;
>   name = short_name;
> }

Ahhhhh I totally forgot this method gets called regardless of whether the
manifest is valid or not. Maybe we should change that? You're code looks good to
me :)

[Bernhard Bauer, 2015-07-21 14:43:32]

LGTM if ShortcutInfo::UpdateFromManifest() gets updated to deal with both values
being null (or doesn't get called in that case).

[Lalit Maganti, 2015-07-21 16:57:41]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-07-21 16:57:42]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
dfalcantara@chromium.org, mlamouri@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1224273003/#ps260001
(title: "Fix nullness of name/shortname")

[commit-bot: I haz the power, 2015-07-21 16:58:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1224273003/260001

[Lalit Maganti, 2015-07-21 17:22:24]

https://codereview.chromium.org/1224273003/diff/200001/chrome/browser/android...
File chrome/browser/android/shortcut_info.h (right):

https://codereview.chromium.org/1224273003/diff/200001/chrome/browser/android...
chrome/browser/android/shortcut_info.h:32: ~ShortcutInfo();
On 2015/07/21 at 09:07:27, Lalit Maganti wrote:
> On 2015/07/20 at 22:19:12, dfalcantara wrote:
> > 1) ~ShortcutInfo() = default;
> > 2) Remove empty destructor from cc file
> 
> Done.
I've had to revert this because of chromium style issues on Clang.
See
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...

[gone, 2015-07-21 17:23:15]

Weird.  Guess there's more going on in your destructor than is obvious.

[commit-bot: I haz the power, 2015-07-21 17:25:27]

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

[commit-bot: I haz the power, 2015-07-21 17:25:28]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[Lalit Maganti, 2015-07-21 17:30:54]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-07-21 17:30:54]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
palmer@chromium.org, dfalcantara@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1224273003/#ps280001
(title: "Add back explict destructor")

[commit-bot: I haz the power, 2015-07-21 17:31:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1224273003/280001

[commit-bot: I haz the power, 2015-07-21 18:13:21]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-07-21 18:15:22]

Patchset 15 (id:??) landed as
https://crrev.com/f3ee51858aec43abdb498cb7d7ce778a121fd999
Cr-Commit-Position: refs/heads/master@{#339697}

[Bernhard Bauer, 2015-07-22 10:00:52]

On 2015/07/21 17:23:15, dfalcantara wrote:
> Weird.  Guess there's more going on in your destructor than is obvious.

Yeah, there are three base::string16 destructors plus a GURL destructor, which
pushes the automatically generated destructor over the limit I guess.