Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1772363002:
Start using the whitelist icon on the NTP. (Closed)
DescriptionStart using the whitelist icon on the NTP.
When the whitelist is created we save the path to the locally stored
icon. During the suggestions creation save that information and when
trying to get the large icon for the NTP, first check if we have a
locally stored one first.
This does not yet handle the case when whitelist entry point becomes
part of most visited.
For more information on whitelists - go/chrome-whitelists
Screenshot - https://screenshot.googleplex.com/3eTZ3wghRvH.png
BUG=586097
Committed: https://crrev.com/c3c25d1e44a7f9ff9fd8dfe75b162d9a0b597931
Cr-Commit-Position: refs/heads/master@{#381756}
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : Addressing comments #
Total comments: 12
Patch Set 4 : Addressing newt's comments #Patch Set 5 : Fixing tests #
Total comments: 6
Patch Set 6 : #
Total comments: 14
Patch Set 7 : Addressing Bernhard's comments #Patch Set 8 : Addressing newt's comments #
Total comments: 18
Patch Set 9 : More comments #
Total comments: 6
Patch Set 10 : #
Total comments: 6
Patch Set 11 : Final touches #
Total comments: 4
Patch Set 12 : #Dependent Patchsets: Messages
Total messages: 48 (12 generated)
Description was changed from ========== Start using the whitelist icon on the NTP. When the whitelist is created we save the path to the locally stored icon. During the suggestions creation save that information and when trying to get the large icon for the NTP, first check if we have a locally stored one first. This does not yet handle the case when whitelist entry point becomes part of most visited. BUG=586097 ========== to ========== Start using the whitelist icon on the NTP. When the whitelist is created we save the path to the locally stored icon. During the suggestions creation save that information and when trying to get the large icon for the NTP, first check if we have a locally stored one first. This does not yet handle the case when whitelist entry point becomes part of most visited. BUG=586097 ==========
atanasova@chromium.org changed reviewers: + treib@chromium.org
atanasova@chromium.org changed reviewers: + bauerb@chromium.org
https://codereview.chromium.org/1772363002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java (right): https://codereview.chromium.org/1772363002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java:52: private String mLocalIconPath; IMO "local" is redundant, "path" already implies it. (Also everywhere else) https://codereview.chromium.org/1772363002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:949: icon = bitmap != null ? bitmap : icon; Regular "if" please, rather than assigning icon to itself :) Also, maybe output some warning if the path is non-empty, but loading fails? https://codereview.chromium.org/1772363002/diff/20001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1772363002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:172: local_large_icon_path(base::FilePath()), This is unnecessary, also below https://codereview.chromium.org/1772363002/diff/20001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1772363002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:92: base::FilePath local_large_icon_path; Maybe add a comment that this will usually be empty? Otherwise this is definitely going to confuse some poor future engineer.
atanasova@chromium.org changed reviewers: + tedchoc@chromium.org
https://codereview.chromium.org/1772363002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java (right): https://codereview.chromium.org/1772363002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java:52: private String mLocalIconPath; On 2016/03/08 14:27:12, Marc Treib wrote: > IMO "local" is redundant, "path" already implies it. (Also everywhere else) Done. https://codereview.chromium.org/1772363002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:949: icon = bitmap != null ? bitmap : icon; On 2016/03/08 14:27:12, Marc Treib wrote: > Regular "if" please, rather than assigning icon to itself :) > Also, maybe output some warning if the path is non-empty, but loading fails? Done. https://codereview.chromium.org/1772363002/diff/20001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1772363002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:172: local_large_icon_path(base::FilePath()), On 2016/03/08 14:27:12, Marc Treib wrote: > This is unnecessary, also below Done. https://codereview.chromium.org/1772363002/diff/20001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1772363002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:92: base::FilePath local_large_icon_path; On 2016/03/08 14:27:12, Marc Treib wrote: > Maybe add a comment that this will usually be empty? Otherwise this is > definitely going to confuse some poor future engineer. Done.
tedchoc@chromium.org changed reviewers: + newt@chromium.org
newt@ should be the reviewer for NTP things
https://codereview.chromium.org/1772363002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:795: if (oldItem != null && TextUtils.equals(url, oldItem.getUrl()) We also need to update this check to see whether the largeIconPath changed. Unless largeIconPath can never change for some reason. https://codereview.chromium.org/1772363002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:949: if (!mItem.getLargeIconPath().isEmpty()) { Where are these icons stored and how did they get there? Are they inside the Chrome app directory? Can we trust that this path isn't malicious? https://codereview.chromium.org/1772363002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:954: Log.w(TAG, "Icon path did not result in an image: " Is this really a condition worth logging? Avoid logging (especially at the warning level) unless it's a significant situation that might be helpful when reviewing crash reports or other bug reports. Log spam is a big problem. Also, don't concatenate strings in logs. Use format strings, e.g. Log.d(TAG, "Bad path: %s", mItem.getLargeIconPath()); which results in less wasted work if the log is going to be discarded. Also, if you use Log.d or Log.i, those logs will be stripped in release builds. https://codereview.chromium.org/1772363002/diff/40001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1772363002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:92: // This will be empty for all suggestions expect whitelist entry points. s/expect/except/ Or phrase this as "Only valid for source == WHITELIST (empty otherwise)", like provider_index below Also, move large_icon_path below "source" since large_icon_source is optional https://codereview.chromium.org/1772363002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:103: const base::FilePath& large_icon_path, for consistency, "large_icon_path" should go after "source"
https://codereview.chromium.org/1772363002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:795: if (oldItem != null && TextUtils.equals(url, oldItem.getUrl()) On 2016/03/08 22:09:04, newt wrote: > We also need to update this check to see whether the largeIconPath changed. > Unless largeIconPath can never change for some reason. Done. It can change since this is coming from the Web store and the owner of the "whitelist" can change the icon. https://codereview.chromium.org/1772363002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:949: if (!mItem.getLargeIconPath().isEmpty()) { On 2016/03/08 22:09:04, newt wrote: > Where are these icons stored and how did they get there? Are they inside the > Chrome app directory? Can we trust that this path isn't malicious? These icons come from the whitelist extension installation. They are stored in the app data directory and we get and save the path during the extension installation. The extension manifest has a category called icons and there is has the path to the icon. I am not sure if we can know for certain whether the content specified is not malicious, so do you have any suggestions on the Android side for us to check? @treib do you think we can have a problem here? https://codereview.chromium.org/1772363002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:954: Log.w(TAG, "Icon path did not result in an image: " On 2016/03/08 22:09:03, newt wrote: > Is this really a condition worth logging? Avoid logging (especially at the > warning level) unless it's a significant situation that might be helpful when > reviewing crash reports or other bug reports. Log spam is a big problem. > > Also, don't concatenate strings in logs. Use format strings, e.g. > > Log.d(TAG, "Bad path: %s", mItem.getLargeIconPath()); > > which results in less wasted work if the log is going to be discarded. Also, if > you use Log.d or Log.i, those logs will be stripped in release builds. This situation is not supposed to happen, because if we have saved a path and it does not result in an image something went wrong - either the path is saved badly or the image has a problem. I think we need to log that. I changed it to Log.d. https://codereview.chromium.org/1772363002/diff/40001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1772363002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:92: // This will be empty for all suggestions expect whitelist entry points. On 2016/03/08 22:09:04, newt wrote: > s/expect/except/ > > Or phrase this as "Only valid for source == WHITELIST (empty otherwise)", like > provider_index below > > Also, move large_icon_path below "source" since large_icon_source is optional Done. https://codereview.chromium.org/1772363002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:103: const base::FilePath& large_icon_path, On 2016/03/08 22:09:04, newt wrote: > for consistency, "large_icon_path" should go after "source" Done.
https://codereview.chromium.org/1772363002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:949: if (!mItem.getLargeIconPath().isEmpty()) { On 2016/03/09 11:40:55, atanasova wrote: > On 2016/03/08 22:09:04, newt wrote: > > Where are these icons stored and how did they get there? Are they inside the > > Chrome app directory? Can we trust that this path isn't malicious? > > These icons come from the whitelist extension installation. They are stored in > the app data directory and we get and save the path during the extension > installation. > The extension manifest has a category called icons and there is has the path to > the icon. > I am not sure if we can know for certain whether the content specified is not > malicious, so do you have any suggestions on the Android side for us to check? > > @treib do you think we can have a problem here? I think we can be reasonably sure that the path itself is okay - we (more specifically, the component updater) put the file there. The file contents themselves are of course not trusted, so we need to take care when decoding the image. But that applies to all downloaded images.
Are there mocks for this, or if not, could you post a screenshot on the bug? Thanks!
This whole whitelist business is still rather mysterious to me. The bug doesn't explain anything about where the whitelist comes from. Is there an explanation somewhere, perhaps a design doc? In the future, please be sure to include this sort of information (either in the CL description or on the bug) before sending out a CL for review. Thanks! https://codereview.chromium.org/1772363002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:949: if (!mItem.getLargeIconPath().isEmpty()) { On 2016/03/09 12:01:46, Marc Treib wrote: > On 2016/03/09 11:40:55, atanasova wrote: > > On 2016/03/08 22:09:04, newt wrote: > > > Where are these icons stored and how did they get there? Are they inside the > > > Chrome app directory? Can we trust that this path isn't malicious? > > > > These icons come from the whitelist extension installation. They are stored in > > the app data directory and we get and save the path during the extension > > installation. > > The extension manifest has a category called icons and there is has the path > to > > the icon. > > I am not sure if we can know for certain whether the content specified is not > > malicious, so do you have any suggestions on the Android side for us to check? > > > > @treib do you think we can have a problem here? > > I think we can be reasonably sure that the path itself is okay - we (more > specifically, the component updater) put the file there. > The file contents themselves are of course not trusted, so we need to take care > when decoding the image. But that applies to all downloaded images. Ok, is there a design doc somewhere describing how the images are downloaded? This should be fine, but you should definitely loop in security so they can take a look. https://codereview.chromium.org/1772363002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java (right): https://codereview.chromium.org/1772363002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java:65: * @param largeIconPath The path of the icon associated with the URL that is stored locally. Be sure to indicate that this can be empty if there's no local icon https://codereview.chromium.org/1772363002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java:108: public String getLargeIconPath() { nit: put this after getTitle() to keep the ordering consistent throughout this class https://codereview.chromium.org/1772363002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:955: Log.d(TAG, "Icon path did not result in an image: %s", How about "Image decoding failed: %s"?
Description was changed from ========== Start using the whitelist icon on the NTP. When the whitelist is created we save the path to the locally stored icon. During the suggestions creation save that information and when trying to get the large icon for the NTP, first check if we have a locally stored one first. This does not yet handle the case when whitelist entry point becomes part of most visited. BUG=586097 ========== to ========== Start using the whitelist icon on the NTP. When the whitelist is created we save the path to the locally stored icon. During the suggestions creation save that information and when trying to get the large icon for the NTP, first check if we have a locally stored one first. This does not yet handle the case when whitelist entry point becomes part of most visited. For more information on whitelists - go/chrome-whitelists Screenshot - https://screenshot.googleplex.com/3eTZ3wghRvH.png BUG=586097 ==========
Here is a link to the main Chrome Whitelist document - go/chrome-whitelists There is no design doc for the installation of the whitelists (as far as I know). This was implemented by Bernhard (one of the reviewers here) as part of the component_updater. Whitelists are in a way similar to extensions and are installed on the device, so that is where we get the icon from. Here is a screenshot of the changes - https://screenshot.googleplex.com/3eTZ3wghRvH.png There is nothing different about the NTP besides using icons that are stored locally, so there are no mocks for this. I have updated the bug with the link to the document and updated the Cls description to have the screenshot and he document link. https://codereview.chromium.org/1772363002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java (right): https://codereview.chromium.org/1772363002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java:65: * @param largeIconPath The path of the icon associated with the URL that is stored locally. On 2016/03/09 21:09:58, newt wrote: > Be sure to indicate that this can be empty if there's no local icon Done. https://codereview.chromium.org/1772363002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java:108: public String getLargeIconPath() { On 2016/03/09 21:09:58, newt wrote: > nit: put this after getTitle() to keep the ordering consistent throughout this > class Done. https://codereview.chromium.org/1772363002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:955: Log.d(TAG, "Icon path did not result in an image: %s", On 2016/03/09 21:09:58, newt wrote: > How about "Image decoding failed: %s"? Done.
Friendly ping @newt - Bernhard is now back, so if you have any more questions about what and how we install whitelists he will be able to explain it better
https://codereview.chromium.org/1772363002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java (right): https://codereview.chromium.org/1772363002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java:51: private static final String[] FAKE_MOST_VISITED_LARGE_ICON_PATHS = new String[] {""}; Nit: For consistency with the line above, add spaces inside of the braces? https://codereview.chromium.org/1772363002/diff/100001/chrome/browser/android... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1772363002/diff/100001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:93: // Only valid for source == WHITELIST (empty otherwise). Please add empty lines before comments, to make it look less cramped. https://codereview.chromium.org/1772363002/diff/100001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:105: Suggestion(const base::string16& title, Do we really need all of these constructors? Could you at least use delegation to get rid of all the duplicated code? Other possibilities: * A single constructor, with static convenience methods for the various kinds of suggestions (whitelist, suggestions, etc.) that take only the parameters needed for that type * A builder class.
On 2016/03/15 11:23:17, atanasova wrote: > Friendly ping > > @newt - Bernhard is now back, so if you have any more questions about what and > how we install whitelists he will be able to explain it better There is a (recently updated) design doc at go/unichrome-whitelists-clank that might have some more information?
Thanks for sending the design docs. The context makes much more sense now :) https://codereview.chromium.org/1772363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java (right): https://codereview.chromium.org/1772363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java:66: * This can be empty if there is no local icon. nit: indent this to line up with "The path..." on the line above I think this still needs clarification. In particular, it should be clear that for "normal" most visited items, this param will be empty (even though a normal most visited item will have a "locally stored" icon in the favicon database). Let's be more explicit here and call this the "whitelistIconPath" and describe it as "The path to the icon image file, if this is a whitelisted most visited item. Empty otherwise." Also, could you update all the other places in this CL to say "whitelistIconPath" instead of "largeIconPath"? https://codereview.chromium.org/1772363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:949: // Try to use the locally stored icon Do we only want to load the whitelist icon as a fallback if "icon" is null? Or do we always want to use the whitelist icon? In the former case, you should also check "icon == null" in the if statement on line 950. In the latter case, you should load the whitelist icon in createMostVisitedItemView(), instead of calling mManager.getLargeIconForUrl(). Also, we should maybe load/decode these icons on a background thread using AsyncTask. Could you measure how long it takes to decode each icon on your dev device (using System.nanoTime() before and after the call to BitmapFactory.decodeFile())?
https://codereview.chromium.org/1772363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java (right): https://codereview.chromium.org/1772363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java:66: * This can be empty if there is no local icon. On 2016/03/15 17:21:31, newt wrote: > nit: indent this to line up with "The path..." on the line above > > I think this still needs clarification. In particular, it should be clear that > for "normal" most visited items, this param will be empty (even though a normal > most visited item will have a "locally stored" icon in the favicon database). > > Let's be more explicit here and call this the "whitelistIconPath" and describe > it as "The path to the icon image file, if this is a whitelisted most visited > item. Empty otherwise." Also, could you update all the other places in this CL > to say "whitelistIconPath" instead of "largeIconPath"? Done. https://codereview.chromium.org/1772363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:949: // Try to use the locally stored icon On 2016/03/15 17:21:31, newt wrote: > Do we only want to load the whitelist icon as a fallback if "icon" is null? Or > do we always want to use the whitelist icon? > > In the former case, you should also check "icon == null" in the if statement on > line 950. In the latter case, you should load the whitelist icon in > createMostVisitedItemView(), instead of calling mManager.getLargeIconForUrl(). > > Also, we should maybe load/decode these icons on a background thread using > AsyncTask. Could you measure how long it takes to decode each icon on your dev > device (using System.nanoTime() before and after the call to > BitmapFactory.decodeFile())? We have decided to have priority over the whitelist icon, so I have moved that logic out. I did the measurement and these are the results: Average - 5276265.7 nanoseconds Median - 4747370 nanoseconds Low - 4004010 nanoseconds High - 8692760 nanoseconds Should we create an AsyncTask? https://codereview.chromium.org/1772363002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java (right): https://codereview.chromium.org/1772363002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java:51: private static final String[] FAKE_MOST_VISITED_LARGE_ICON_PATHS = new String[] {""}; On 2016/03/15 12:03:35, bauerb catching up on reviews wrote: > Nit: For consistency with the line above, add spaces inside of the braces? git cl format keeps removing the spaces. Should I still use them? https://codereview.chromium.org/1772363002/diff/100001/chrome/browser/android... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1772363002/diff/100001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:93: // Only valid for source == WHITELIST (empty otherwise). On 2016/03/15 12:03:35, bauerb catching up on reviews wrote: > Please add empty lines before comments, to make it look less cramped. Done. https://codereview.chromium.org/1772363002/diff/100001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:105: Suggestion(const base::string16& title, On 2016/03/15 12:03:35, bauerb catching up on reviews wrote: > Do we really need all of these constructors? Could you at least use delegation > to get rid of all the duplicated code? > > Other possibilities: > * A single constructor, with static convenience methods for the various kinds of > suggestions (whitelist, suggestions, etc.) that take only the parameters needed > for that type > * A builder class. Created a builder class
https://codereview.chromium.org/1772363002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java (right): https://codereview.chromium.org/1772363002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java:61: * view. nit: merge into the previous line please. https://codereview.chromium.org/1772363002/diff/140001/chrome/browser/android... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1772363002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:168: class SuggestionBuilder { nit: empty line after "namespace {" please. https://codereview.chromium.org/1772363002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:183: GURL url_; Maybe this should store references to the title and url, to avoid the extra copy? https://codereview.chromium.org/1772363002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:536: new SuggestionBuilder(visited.title, visited.url, TOP_SITES); Memleak! Just SuggestionBuilder builder(...); please! https://codereview.chromium.org/1772363002/diff/140001/chrome/browser/android... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1772363002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:83: // The source of the Most Visited sites. All this is now public only so that the Builder can access it, right? If so, I'd make the Builder a member (you can just declare it here, like class SuggestionBuilder; i.e. no need to move the definition here), and keep this private.
https://codereview.chromium.org/1772363002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java (right): https://codereview.chromium.org/1772363002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java:51: private static final String[] FAKE_MOST_VISITED_LARGE_ICON_PATHS = new String[] {""}; On 2016/03/16 10:55:24, atanasova wrote: > On 2016/03/15 12:03:35, bauerb catching up on reviews wrote: > > Nit: For consistency with the line above, add spaces inside of the braces? > > git cl format keeps removing the spaces. Should I still use them? I would. clang-format is more built for C(/C++) than Java, and it doesn't take precedence over the style guide. https://codereview.chromium.org/1772363002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:990: if (!item.getWhitelistIconPath().isEmpty()) { Invert these conditions to remove some levels of indentation? I.e. return false if the path is empty, return false if the bitmap is null, set the icon and return true otherwise. https://codereview.chromium.org/1772363002/diff/140001/chrome/browser/android... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1772363002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:86: struct Suggestion { Actually, now that I see this: If this is a struct, i.e. all the members are public anyway, you could just create an empty one and then modify the members (in particular if you're doing that from the implementation of this class anyway). You're not really doing much different with the builder anyway. Sorry for jumping around here!
https://codereview.chromium.org/1772363002/diff/140001/chrome/browser/android... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1772363002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:86: struct Suggestion { On 2016/03/16 12:14:18, bauerb catching up on reviews wrote: > Actually, now that I see this: If this is a struct, i.e. all the members are > public anyway, you could just create an empty one and then modify the members > (in particular if you're doing that from the implementation of this class > anyway). You're not really doing much different with the builder anyway. Sorry > for jumping around here! Huh. Or, alternatively, make it a class!
https://codereview.chromium.org/1772363002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java (right): https://codereview.chromium.org/1772363002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java:51: private static final String[] FAKE_MOST_VISITED_LARGE_ICON_PATHS = new String[] {""}; On 2016/03/16 12:14:18, bauerb catching up on reviews wrote: > On 2016/03/16 10:55:24, atanasova wrote: > > On 2016/03/15 12:03:35, bauerb catching up on reviews wrote: > > > Nit: For consistency with the line above, add spaces inside of the braces? > > > > git cl format keeps removing the spaces. Should I still use them? > > I would. clang-format is more built for C(/C++) than Java, and it doesn't take > precedence over the style guide. Done. https://codereview.chromium.org/1772363002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java (right): https://codereview.chromium.org/1772363002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java:61: * view. On 2016/03/16 11:06:03, Marc Treib wrote: > nit: merge into the previous line please. Done. https://codereview.chromium.org/1772363002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:990: if (!item.getWhitelistIconPath().isEmpty()) { On 2016/03/16 12:14:18, bauerb catching up on reviews wrote: > Invert these conditions to remove some levels of indentation? I.e. return false > if the path is empty, return false if the bitmap is null, set the icon and > return true otherwise. Done. https://codereview.chromium.org/1772363002/diff/140001/chrome/browser/android... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1772363002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:168: class SuggestionBuilder { On 2016/03/16 11:06:03, Marc Treib wrote: > nit: empty line after "namespace {" please. Done. https://codereview.chromium.org/1772363002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:536: new SuggestionBuilder(visited.title, visited.url, TOP_SITES); On 2016/03/16 11:06:03, Marc Treib wrote: > Memleak! Just > SuggestionBuilder builder(...); > please! Done. https://codereview.chromium.org/1772363002/diff/140001/chrome/browser/android... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1772363002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:83: // The source of the Most Visited sites. On 2016/03/16 11:06:03, Marc Treib wrote: > All this is now public only so that the Builder can access it, right? > If so, I'd make the Builder a member (you can just declare it here, like > class SuggestionBuilder; > i.e. no need to move the definition here), and keep this private. Done. https://codereview.chromium.org/1772363002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:86: struct Suggestion { On 2016/03/16 12:51:35, Marc Treib wrote: > On 2016/03/16 12:14:18, bauerb catching up on reviews wrote: > > Actually, now that I see this: If this is a struct, i.e. all the members are > > public anyway, you could just create an empty one and then modify the members > > (in particular if you're doing that from the implementation of this class > > anyway). You're not really doing much different with the builder anyway. Sorry > > for jumping around here! > > Huh. Or, alternatively, make it a class! Done.
https://codereview.chromium.org/1772363002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:981: if (!setWhitelistIcon(item, view, isInitialLoad)) { For simplicity and to minimize differences in code paths, how about using the following structure: LargeIconCallback iconCallback = new LargeIconCallbackImpl(item, isInitialLoad); if (isInitialLoad) mPendingLoadTasks++; if (!loadWhitelistIcon(item, view, iconCallback)) { mManager.getLargeIconForUrl(item.getUrl(), mMinIconSize, iconCallback); } ... private boolean loadWhitelistIcon(item, view, iconCallback) { if (item.getWhitelistIconPath().isEmpty()) return false; Bitmap bitmap = BitmapFactory.decodeFile(item.getWhitelistIconPath()); if (bitmap == null) { Log(...); return false; } else { iconCallback.onLargeIconAvailable(bitmap, Color.BLACK); return true; } } This way, everything funnels through LargeIconCallback.onLargeIconAvailable(). We don't have duplicate code for rounding the icons, and there's less trickiness around calls to loadTaskCompleted() and mPendingLoadTasks++. https://codereview.chromium.org/1772363002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:988: public boolean setWhitelistIcon( should be private
https://codereview.chromium.org/1772363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:949: // Try to use the locally stored icon On 2016/03/16 10:55:24, atanasova wrote: > On 2016/03/15 17:21:31, newt wrote: > > Do we only want to load the whitelist icon as a fallback if "icon" is null? Or > > do we always want to use the whitelist icon? > > > > In the former case, you should also check "icon == null" in the if statement > on > > line 950. In the latter case, you should load the whitelist icon in > > createMostVisitedItemView(), instead of calling mManager.getLargeIconForUrl(). > > > > Also, we should maybe load/decode these icons on a background thread using > > AsyncTask. Could you measure how long it takes to decode each icon on your dev > > device (using System.nanoTime() before and after the call to > > BitmapFactory.decodeFile())? > > We have decided to have priority over the whitelist icon, so I have moved that > logic out. > > I did the measurement and these are the results: > Average - 5276265.7 nanoseconds > Median - 4747370 nanoseconds > Low - 4004010 nanoseconds > High - 8692760 nanoseconds > > Should we create an AsyncTask? An AsyncTask would help reduce jank a little bit, but it adds some complexity and there's larger low-hanging fruit for reducing jank on the NTP. So, I think it's fine not to use AsyncTask for now.
https://codereview.chromium.org/1772363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:991: return false; You could inline this on the previous line. https://codereview.chromium.org/1772363002/diff/160001/chrome/browser/android... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1772363002/diff/160001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:485: suggestions.push_back(make_scoped_ptr(&suggestion)); You can't take ownership of an object on the stack, that will lead to a use-after-free. Create the new Suggestion on the heap and put it into a scoped_ptr, then push_back(std::move(suggestion)).
https://codereview.chromium.org/1772363002/diff/160001/chrome/browser/android... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1772363002/diff/160001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:167: MostVisitedSites::Suggestion::Suggestion() {} Please initialize provider_index (it has an undefined value otherwise). https://codereview.chromium.org/1772363002/diff/160001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:480: MostVisitedSites::Suggestion suggestion; You don't need "MostVisitedSites::", you're already in that namespace :)
https://codereview.chromium.org/1772363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:949: // Try to use the locally stored icon On 2016/03/16 18:08:28, newt wrote: > On 2016/03/16 10:55:24, atanasova wrote: > > On 2016/03/15 17:21:31, newt wrote: > > > Do we only want to load the whitelist icon as a fallback if "icon" is null? > Or > > > do we always want to use the whitelist icon? > > > > > > In the former case, you should also check "icon == null" in the if statement > > on > > > line 950. In the latter case, you should load the whitelist icon in > > > createMostVisitedItemView(), instead of calling > mManager.getLargeIconForUrl(). > > > > > > Also, we should maybe load/decode these icons on a background thread using > > > AsyncTask. Could you measure how long it takes to decode each icon on your > dev > > > device (using System.nanoTime() before and after the call to > > > BitmapFactory.decodeFile())? > > > > We have decided to have priority over the whitelist icon, so I have moved that > > logic out. > > > > I did the measurement and these are the results: > > Average - 5276265.7 nanoseconds > > Median - 4747370 nanoseconds > > Low - 4004010 nanoseconds > > High - 8692760 nanoseconds > > > > Should we create an AsyncTask? > > An AsyncTask would help reduce jank a little bit, but it adds some complexity > and there's larger low-hanging fruit for reducing jank on the NTP. So, I think > it's fine not to use AsyncTask for now. Acknowledged. https://codereview.chromium.org/1772363002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:981: if (!setWhitelistIcon(item, view, isInitialLoad)) { On 2016/03/16 18:06:33, newt wrote: > For simplicity and to minimize differences in code paths, how about using the > following structure: > > LargeIconCallback iconCallback = new LargeIconCallbackImpl(item, > isInitialLoad); > if (isInitialLoad) mPendingLoadTasks++; > if (!loadWhitelistIcon(item, view, iconCallback)) { > mManager.getLargeIconForUrl(item.getUrl(), mMinIconSize, iconCallback); > } > > ... > > private boolean loadWhitelistIcon(item, view, iconCallback) { > if (item.getWhitelistIconPath().isEmpty()) return false; > Bitmap bitmap = BitmapFactory.decodeFile(item.getWhitelistIconPath()); > if (bitmap == null) { > Log(...); > return false; > } else { > iconCallback.onLargeIconAvailable(bitmap, Color.BLACK); > return true; > } > } > > This way, everything funnels through LargeIconCallback.onLargeIconAvailable(). > We don't have duplicate code for rounding the icons, and there's less trickiness > around calls to loadTaskCompleted() and mPendingLoadTasks++. Done. https://codereview.chromium.org/1772363002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:988: public boolean setWhitelistIcon( On 2016/03/16 18:06:33, newt wrote: > should be private Done. https://codereview.chromium.org/1772363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:991: return false; On 2016/03/16 18:16:36, bauerb catching up on reviews wrote: > You could inline this on the previous line. Done. https://codereview.chromium.org/1772363002/diff/160001/chrome/browser/android... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1772363002/diff/160001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:485: suggestions.push_back(make_scoped_ptr(&suggestion)); On 2016/03/16 18:16:36, bauerb catching up on reviews wrote: > You can't take ownership of an object on the stack, that will lead to a > use-after-free. Create the new Suggestion on the heap and put it into a > scoped_ptr, then push_back(std::move(suggestion)). Done.
LGTM https://codereview.chromium.org/1772363002/diff/180001/chrome/browser/android... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1772363002/diff/180001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:480: scoped_ptr<Suggestion> suggestion = make_scoped_ptr(new Suggestion()); You can directly initialize the scoped_ptr with the newly allocated object.
lgtm
And, just for completeness' sake, LGTM too :) https://codereview.chromium.org/1772363002/diff/180001/chrome/browser/android... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1772363002/diff/180001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:522: suggestion.providers_size() > 0 ? suggestion.providers(0) : -1; nit: I'd do if (suggestion.providers_size() > 0) generated_suggestion->provider_index = suggestion.providers(0); https://codereview.chromium.org/1772363002/diff/180001/chrome/browser/android... File chrome/browser/android/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/1772363002/diff/180001/chrome/browser/android... chrome/browser/android/most_visited_sites_unittest.cc:91: return std::move(suggestion); nit: std::move shouldn't be required here.
The CQ bit was checked by atanasova@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772363002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772363002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@newt I am sorry, but can you PTAL? I had to change the NewTabPageView a bit, because the logic that we had was a bit wrong. When using the callback for the whitelist icon, the view is not yet associated with the item. So to still reuse that logic, I have added another constructor and parameter to the callback. https://codereview.chromium.org/1772363002/diff/180001/chrome/browser/android... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1772363002/diff/180001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:480: scoped_ptr<Suggestion> suggestion = make_scoped_ptr(new Suggestion()); On 2016/03/16 19:25:34, Bernhard Bauer wrote: > You can directly initialize the scoped_ptr with the newly allocated object. Done. https://codereview.chromium.org/1772363002/diff/180001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:522: suggestion.providers_size() > 0 ? suggestion.providers(0) : -1; On 2016/03/17 09:19:44, Marc Treib wrote: > nit: I'd do > if (suggestion.providers_size() > 0) > generated_suggestion->provider_index = suggestion.providers(0); Done. https://codereview.chromium.org/1772363002/diff/180001/chrome/browser/android... File chrome/browser/android/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/1772363002/diff/180001/chrome/browser/android... chrome/browser/android/most_visited_sites_unittest.cc:91: return std::move(suggestion); On 2016/03/17 09:19:44, Marc Treib wrote: > nit: std::move shouldn't be required here. Done.
https://codereview.chromium.org/1772363002/diff/200001/chrome/browser/android... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1772363002/diff/200001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:480: scoped_ptr<Suggestion> suggestion(make_scoped_ptr(new Suggestion())); Actually, you don't even need the make_scoped_ptr here, just scoped_ptr<Suggestion> suggestion(new Suggestion()); works fine. Don't bother making yet another patch set though, this can be changed later!
one comment, then lgtm again :) https://codereview.chromium.org/1772363002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:943: public LargeIconCallbackImpl(MostVisitedItem item, boolean isInitialLoad) { Let's delete this constructor to keep things simple. It's only used in one place, which can easily call the other constructor.
https://codereview.chromium.org/1772363002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1772363002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:943: public LargeIconCallbackImpl(MostVisitedItem item, boolean isInitialLoad) { On 2016/03/17 17:02:19, newt wrote: > Let's delete this constructor to keep things simple. It's only used in one > place, which can easily call the other constructor. Done. https://codereview.chromium.org/1772363002/diff/200001/chrome/browser/android... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1772363002/diff/200001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:480: scoped_ptr<Suggestion> suggestion(make_scoped_ptr(new Suggestion())); On 2016/03/17 10:18:29, Marc Treib wrote: > Actually, you don't even need the make_scoped_ptr here, just > scoped_ptr<Suggestion> suggestion(new Suggestion()); > works fine. Don't bother making yet another patch set though, this can be > changed later! Done.
The CQ bit was checked by atanasova@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1772363002/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772363002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772363002/220001
Message was sent while issue was closed.
Description was changed from ========== Start using the whitelist icon on the NTP. When the whitelist is created we save the path to the locally stored icon. During the suggestions creation save that information and when trying to get the large icon for the NTP, first check if we have a locally stored one first. This does not yet handle the case when whitelist entry point becomes part of most visited. For more information on whitelists - go/chrome-whitelists Screenshot - https://screenshot.googleplex.com/3eTZ3wghRvH.png BUG=586097 ========== to ========== Start using the whitelist icon on the NTP. When the whitelist is created we save the path to the locally stored icon. During the suggestions creation save that information and when trying to get the large icon for the NTP, first check if we have a locally stored one first. This does not yet handle the case when whitelist entry point becomes part of most visited. For more information on whitelists - go/chrome-whitelists Screenshot - https://screenshot.googleplex.com/3eTZ3wghRvH.png BUG=586097 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Start using the whitelist icon on the NTP. When the whitelist is created we save the path to the locally stored icon. During the suggestions creation save that information and when trying to get the large icon for the NTP, first check if we have a locally stored one first. This does not yet handle the case when whitelist entry point becomes part of most visited. For more information on whitelists - go/chrome-whitelists Screenshot - https://screenshot.googleplex.com/3eTZ3wghRvH.png BUG=586097 ========== to ========== Start using the whitelist icon on the NTP. When the whitelist is created we save the path to the locally stored icon. During the suggestions creation save that information and when trying to get the large icon for the NTP, first check if we have a locally stored one first. This does not yet handle the case when whitelist entry point becomes part of most visited. For more information on whitelists - go/chrome-whitelists Screenshot - https://screenshot.googleplex.com/3eTZ3wghRvH.png BUG=586097 Committed: https://crrev.com/c3c25d1e44a7f9ff9fd8dfe75b162d9a0b597931 Cr-Commit-Position: refs/heads/master@{#381756} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/c3c25d1e44a7f9ff9fd8dfe75b162d9a0b597931 Cr-Commit-Position: refs/heads/master@{#381756} |
