Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1272563005:
Fix title of webapps being empty (Closed)
|
Created:
5 years, 4 months ago by Lalit Maganti Modified:
5 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix title of webapps being empty
* With the new manifest name/shortname changes, we don't always have null in the intent for the shortname.
It could also be the empty string which we need to handle by displaying the page title in recents.
* This issue is present with Slashdot which does not have a shortname and so has an empty title.
TODO(lalitm): this is actually a temporary fix for the bigger issue of short name not being set to the meta tag title of the website if the short name is not present in the manifest. Some discussion is required for this before a CL which correctly fixes the issue is submitted.
BUG=517647
Committed: https://crrev.com/acd5d3f1f3a4dc0433a4a9bb9b87dc0567d7acb7
Cr-Commit-Position: refs/heads/master@{#342394}
Patch Set 1 #Patch Set 2 : Add TODO comment #Messages
Total messages: 17 (3 generated)
lalitm@google.com changed reviewers: + dfalcantara@chromium.org, mlamouri@chromium.org
Dan and Mounir: please take a look. This fixes the Slashdot issue but I'm not sure if other places are also relying on null meaning empty.
On 2015/08/07 09:22:52, Lalit Maganti wrote: > Dan and Mounir: please take a look. This fixes the Slashdot issue but I'm not > sure if other places are also relying on null meaning empty. You need to fix this so that the shortcut and the recent task always use the same title (phishing thing). That line used to use the title before you changed it to use shortName, didn't it?
On 2015/08/07 15:57:01, dfalcantara wrote: > On 2015/08/07 09:22:52, Lalit Maganti wrote: > > Dan and Mounir: please take a look. This fixes the Slashdot issue but I'm not > > sure if other places are also relying on null meaning empty. > > You need to fix this so that the shortcut and the recent task always use the > same title (phishing thing). That line used to use the title before you changed > it to use shortName, didn't it? Ah, right that is stored in shortName at some point. Phone code reviews, FTW. lgtm
On 2015/08/07 16:00:16, dfalcantara wrote: > On 2015/08/07 15:57:01, dfalcantara wrote: > > On 2015/08/07 09:22:52, Lalit Maganti wrote: > > > Dan and Mounir: please take a look. This fixes the Slashdot issue but I'm > not > > > sure if other places are also relying on null meaning empty. > > > > You need to fix this so that the shortcut and the recent task always use the > > same title (phishing thing). That line used to use the title before you > changed > > it to use shortName, didn't it? > > Ah, right that is stored in shortName at some point. Phone code reviews, FTW. > > lgtm I'll add a Lollipop unit test once this lands so this doesn't happen again.
On 2015/08/07 16:01:42, dfalcantara wrote: > On 2015/08/07 16:00:16, dfalcantara wrote: > > On 2015/08/07 15:57:01, dfalcantara wrote: > > > On 2015/08/07 09:22:52, Lalit Maganti wrote: > > > > Dan and Mounir: please take a look. This fixes the Slashdot issue but I'm > > not > > > > sure if other places are also relying on null meaning empty. > > > > > > You need to fix this so that the shortcut and the recent task always use the > > > same title (phishing thing). That line used to use the title before you > > changed > > > it to use shortName, didn't it? > > > > Ah, right that is stored in shortName at some point. Phone code reviews, FTW. > > > > lgtm > > I'll add a Lollipop unit test once this lands so this doesn't happen again. Actually: how was this not an issue before your patch? This CL still leaves an opening for the title of the shortcut and the task to be different.
On 2015/08/07 at 16:06:46, dfalcantara wrote: > On 2015/08/07 16:01:42, dfalcantara wrote: > > On 2015/08/07 16:00:16, dfalcantara wrote: > > > On 2015/08/07 15:57:01, dfalcantara wrote: > > > > On 2015/08/07 09:22:52, Lalit Maganti wrote: > > > > > Dan and Mounir: please take a look. This fixes the Slashdot issue but I'm > > > not > > > > > sure if other places are also relying on null meaning empty. > > > > > > > > You need to fix this so that the shortcut and the recent task always use the > > > > same title (phishing thing). That line used to use the title before you > > > changed > > > > it to use shortName, didn't it? > > > > > > Ah, right that is stored in shortName at some point. Phone code reviews, FTW. > > > > > > lgtm > > > > I'll add a Lollipop unit test once this lands so this doesn't happen again. > > Actually: how was this not an issue before your patch? This CL still leaves an opening for the title of the shortcut and the task to be different. palmer@ decided that it wasn't a security problem anyway as apps can intentionally ask the recents title to be set to something different.
On 2015/08/07 at 16:11:08, Lalit Maganti wrote: > On 2015/08/07 at 16:06:46, dfalcantara wrote: > > On 2015/08/07 16:01:42, dfalcantara wrote: > > > On 2015/08/07 16:00:16, dfalcantara wrote: > > > > On 2015/08/07 15:57:01, dfalcantara wrote: > > > > > On 2015/08/07 09:22:52, Lalit Maganti wrote: > > > > > > Dan and Mounir: please take a look. This fixes the Slashdot issue but I'm > > > > not > > > > > > sure if other places are also relying on null meaning empty. > > > > > > > > > > You need to fix this so that the shortcut and the recent task always use the > > > > > same title (phishing thing). That line used to use the title before you > > > > changed > > > > > it to use shortName, didn't it? > > > > > > > > Ah, right that is stored in shortName at some point. Phone code reviews, FTW. > > > > > > > > lgtm > > > > > > I'll add a Lollipop unit test once this lands so this doesn't happen again. > > > > Actually: how was this not an issue before your patch? This CL still leaves an opening for the title of the shortcut and the task to be different. > > palmer@ decided that it wasn't a security problem anyway as apps can intentionally ask the recents title to be set to something different. Or rather websites which are not webapps can request this - webapps ATM do not change title based on the page the user is on. palmer@ agreed that a new vulnerability was not introduced by my original patch since in document mode on L+, normal websites change the recents title.
On 2015/08/07 16:06:46, dfalcantara wrote: > On 2015/08/07 16:01:42, dfalcantara wrote: > > On 2015/08/07 16:00:16, dfalcantara wrote: > > > On 2015/08/07 15:57:01, dfalcantara wrote: > > > > On 2015/08/07 09:22:52, Lalit Maganti wrote: > > > > > Dan and Mounir: please take a look. This fixes the Slashdot issue but > I'm > > > not > > > > > sure if other places are also relying on null meaning empty. > > > > > > > > You need to fix this so that the shortcut and the recent task always use > the > > > > same title (phishing thing). That line used to use the title before you > > > changed > > > > it to use shortName, didn't it? > > > > > > Ah, right that is stored in shortName at some point. Phone code reviews, > FTW. > > > > > > lgtm > > > > I'll add a Lollipop unit test once this lands so this doesn't happen again. > > Actually: how was this not an issue before your patch? This CL still leaves an > opening for the title of the shortcut and the task to be different. Not lgtm. Your logic for setting the short name in https://codereview.chromium.org/1224273003/diff/280001/chrome/browser/android... doesn't take the title into account if the short name is empty.
On 2015/08/07 at 16:31:43, dfalcantara wrote: > On 2015/08/07 16:06:46, dfalcantara wrote: > > On 2015/08/07 16:01:42, dfalcantara wrote: > > > On 2015/08/07 16:00:16, dfalcantara wrote: > > > > On 2015/08/07 15:57:01, dfalcantara wrote: > > > > > On 2015/08/07 09:22:52, Lalit Maganti wrote: > > > > > > Dan and Mounir: please take a look. This fixes the Slashdot issue but > > I'm > > > > not > > > > > > sure if other places are also relying on null meaning empty. > > > > > > > > > > You need to fix this so that the shortcut and the recent task always use > > the > > > > > same title (phishing thing). That line used to use the title before you > > > > changed > > > > > it to use shortName, didn't it? > > > > > > > > Ah, right that is stored in shortName at some point. Phone code reviews, > > FTW. > > > > > > > > lgtm > > > > > > I'll add a Lollipop unit test once this lands so this doesn't happen again. > > > > Actually: how was this not an issue before your patch? This CL still leaves an > > opening for the title of the shortcut and the task to be different. > > Not lgtm. > > Your logic for setting the short name in https://codereview.chromium.org/1224273003/diff/280001/chrome/browser/android... doesn't take the title into account if the short name is empty. Yes that's what Mounir and I discussed offline. The short name and name would always be empty if they are not present in the manifest. The title is set from short name but not vice versa. This is reflected in the tests.
On 2015/08/07 16:36:40, Lalit Maganti wrote: > On 2015/08/07 at 16:31:43, dfalcantara wrote: > > On 2015/08/07 16:06:46, dfalcantara wrote: > > > On 2015/08/07 16:01:42, dfalcantara wrote: > > > > On 2015/08/07 16:00:16, dfalcantara wrote: > > > > > On 2015/08/07 15:57:01, dfalcantara wrote: > > > > > > On 2015/08/07 09:22:52, Lalit Maganti wrote: > > > > > > > Dan and Mounir: please take a look. This fixes the Slashdot issue > but > > > I'm > > > > > not > > > > > > > sure if other places are also relying on null meaning empty. > > > > > > > > > > > > You need to fix this so that the shortcut and the recent task always > use > > > the > > > > > > same title (phishing thing). That line used to use the title before > you > > > > > changed > > > > > > it to use shortName, didn't it? > > > > > > > > > > Ah, right that is stored in shortName at some point. Phone code > reviews, > > > FTW. > > > > > > > > > > lgtm > > > > > > > > I'll add a Lollipop unit test once this lands so this doesn't happen > again. > > > > > > Actually: how was this not an issue before your patch? This CL still leaves > an > > > opening for the title of the shortcut and the task to be different. > > > > Not lgtm. > > > > Your logic for setting the short name in > https://codereview.chromium.org/1224273003/diff/280001/chrome/browser/android... > doesn't take the title into account if the short name is empty. > > Yes that's what Mounir and I discussed offline. The short name and name would > always be empty if they are not present in the manifest. The title is set from > short name but not vice versa. This is reflected in the tests. But then you end up in this situation where the title of the shortcut and the title of the task can be different no?
On 2015/08/07 16:39:48, dfalcantara wrote: > On 2015/08/07 16:36:40, Lalit Maganti wrote: > > On 2015/08/07 at 16:31:43, dfalcantara wrote: > > > On 2015/08/07 16:06:46, dfalcantara wrote: > > > > On 2015/08/07 16:01:42, dfalcantara wrote: > > > > > On 2015/08/07 16:00:16, dfalcantara wrote: > > > > > > On 2015/08/07 15:57:01, dfalcantara wrote: > > > > > > > On 2015/08/07 09:22:52, Lalit Maganti wrote: > > > > > > > > Dan and Mounir: please take a look. This fixes the Slashdot issue > > but > > > > I'm > > > > > > not > > > > > > > > sure if other places are also relying on null meaning empty. > > > > > > > > > > > > > > You need to fix this so that the shortcut and the recent task always > > use > > > > the > > > > > > > same title (phishing thing). That line used to use the title before > > you > > > > > > changed > > > > > > > it to use shortName, didn't it? > > > > > > > > > > > > Ah, right that is stored in shortName at some point. Phone code > > reviews, > > > > FTW. > > > > > > > > > > > > lgtm > > > > > > > > > > I'll add a Lollipop unit test once this lands so this doesn't happen > > again. > > > > > > > > Actually: how was this not an issue before your patch? This CL still > leaves > > an > > > > opening for the title of the shortcut and the task to be different. > > > > > > Not lgtm. > > > > > > Your logic for setting the short name in > > > https://codereview.chromium.org/1224273003/diff/280001/chrome/browser/android... > > doesn't take the title into account if the short name is empty. > > > > Yes that's what Mounir and I discussed offline. The short name and name would > > always be empty if they are not present in the manifest. The title is set from > > short name but not vice versa. This is reflected in the tests. > > But then you end up in this situation where the title of the shortcut and the > title of the task can be different no? lgtm for the temporary workaround. Add the TODO in the code, though.
The CQ bit was checked by lalitm@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/1272563005/#ps20001 (title: "Add TODO comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272563005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272563005/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/acd5d3f1f3a4dc0433a4a9bb9b87dc0567d7acb7 Cr-Commit-Position: refs/heads/master@{#342394} |
