chrome/browser/android/shortcut_info.h - Issue 2453423002: Send all of the icon URLs listed in Web Manifest to WebAPK Server.

Side by Side Diff: chrome/browser/android/shortcut_info.h

Issue 2453423002: Send all of the icon URLs listed in Web Manifest to WebAPK Server. (Closed)

Patch Set: use space to separate icon_url and icon_hash. Created 4 years, 1 month ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java ('K') | « chrome/browser/android/shortcut_helper.cc ('k') | chrome/browser/android/shortcut_info.cc » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 // Copyright 2015 The Chromium Authors. All rights reserved.	1 // Copyright 2015 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #ifndef CHROME_BROWSER_ANDROID_SHORTCUT_INFO_H_	5 #ifndef CHROME_BROWSER_ANDROID_SHORTCUT_INFO_H_

6 #define CHROME_BROWSER_ANDROID_SHORTCUT_INFO_H_	6 #define CHROME_BROWSER_ANDROID_SHORTCUT_INFO_H_

7	7

8 #include <stdint.h>	8 #include <stdint.h>

9	9

	10 #include <vector>

	11

10 #include "base/strings/string16.h"	12 #include "base/strings/string16.h"

11 #include "content/public/common/manifest.h"	13 #include "content/public/common/manifest.h"

12 #include "third_party/WebKit/public/platform/modules/screen_orientation/WebScree nOrientationLockType.h"	14 #include "third_party/WebKit/public/platform/modules/screen_orientation/WebScree nOrientationLockType.h"

13 #include "url/gurl.h"	15 #include "url/gurl.h"

14	16

15 // Information needed to create a shortcut via ShortcutHelper.	17 // Information needed to create a shortcut via ShortcutHelper.

16 struct ShortcutInfo {	18 struct ShortcutInfo {

17	19

18 // This enum is used to back a UMA histogram, and must be treated as	20 // This enum is used to back a UMA histogram, and must be treated as

19 // append-only.	21 // append-only.

(...skipping 24 matching lines...) Expand all Loading...
44 GURL url;	46 GURL url;

45 GURL scope;	47 GURL scope;

46 base::string16 user_title;	48 base::string16 user_title;

47 base::string16 name;	49 base::string16 name;

48 base::string16 short_name;	50 base::string16 short_name;

49 blink::WebDisplayMode display;	51 blink::WebDisplayMode display;

50 blink::WebScreenOrientationLockType orientation;	52 blink::WebScreenOrientationLockType orientation;

51 Source source;	53 Source source;

52 int64_t theme_color;	54 int64_t theme_color;

53 int64_t background_color;	55 int64_t background_color;

54 GURL icon_url;	56 GURL best_icon_url;

	57 std::vector<GURL> icon_urls;
	dominickn 2016/11/17 02:34:38 Why is this a vector but it's a set in Java? Also Why is this a vector but it's a set in Java? Also, I'm not a big fan of the "best" terminology. I'll think about it overnight... Also, is there any need for this to be a container of GURLs? It looks like everything that accesses this member calls spec() on each object (or starts with a string and has to call GURL()). Why not have a container of strings? Xi Han 2016/11/17 18:09:58 In c++, we only need to get these URLs; while in J Show quoted text On 2016/11/17 02:34:38, dominickn wrote: > Why is this a vector but it's a set in Java? In c++, we only need to get these URLs; while in Java, we need to compare them with the ones backed in WebAPK's metadata to see whether they changed or not. Show quoted text > Also, I'm not a big fan of the "best" terminology. I'll think about it > overnight... All right. Show quoted text > Also, is there any need for this to be a container of GURLs? It looks like > everything that accesses this member calls spec() on each object (or starts with > a string and has to call GURL()). Why not have a container of strings? It looks like every variable which is called ".._url" is declared as GURL in c++, I think it's better to keep this conventions. dominickn 2016/11/17 18:50:53 You've actually broken this convention in manifest Show quoted text On 2016/11/17 18:09:58, Xi Han wrote: > > Also, is there any need for this to be a container of GURLs? It looks like > > everything that accesses this member calls spec() on each object (or starts > with > > a string and has to call GURL()). Why not have a container of strings? > It looks like every variable which is called ".._url" is declared as GURL in > c++, I think it's better to keep this conventions. You've actually broken this convention in manifest_upgrade_detector_fetcher.cc, with std::vector<std::string> icon_urls. ;) There are plenty of cases where we have some url member which isn't a GURL type and I think this makes sense as one of them. It will save extra string <-> GURL type conversions in webapk_update_manager, webapk_installer, and manifest_upgrade_detector_fetcher so it's definitely worth having. Xi Han 2016/11/17 20:42:38 Done. Show quoted text On 2016/11/17 18:50:53, dominickn wrote: > On 2016/11/17 18:09:58, Xi Han wrote: > > > Also, is there any need for this to be a container of GURLs? It looks like > > > everything that accesses this member calls spec() on each object (or starts > > with > > > a string and has to call GURL()). Why not have a container of strings? > > It looks like every variable which is called ".._url" is declared as GURL in > > c++, I think it's better to keep this conventions. > > You've actually broken this convention in manifest_upgrade_detector_fetcher.cc, > with std::vector<std::string> icon_urls. ;) > > There are plenty of cases where we have some url member which isn't a GURL type > and I think this makes sense as one of them. It will save extra string <-> GURL > type conversions in webapk_update_manager, webapk_installer, and > manifest_upgrade_detector_fetcher so it's definitely worth having. Done.
55 };	58 };

56	59

57 #endif // CHROME_BROWSER_ANDROID_SHORTCUT_INFO_H_	60 #endif // CHROME_BROWSER_ANDROID_SHORTCUT_INFO_H_

OLD	NEW